[Devel] [PATCH VZ9 2/3] net: zerocopy over unix sockets

[Pavel Tikhomirov]

On 16/01/2024 19:26, Alexey Kuznetsov wrote:
> Hello!
> 
> On Tue, Jan 16, 2024 at 1:13 PM Pavel Tikhomirov
> <ptikhomirov at virtuozzo.com> wrote:
>>> --- a/net/core/sock.c
>>> +++ b/net/core/sock.c
>>> @@ -1405,6 +1405,9 @@ int sk_setsockopt(struct sock *sk, int level, int optname,
>>>                              (sk->sk_type == SOCK_DGRAM &&
>>>                               sk->sk_protocol == IPPROTO_UDP)))
>>>                                ret = -EOPNOTSUPP;
>>> +             } else if (sk->sk_family == PF_UNIX) {
>>> +                     if (sk->sk_type == SOCK_DGRAM)
>>> +                             ret = -EOPNOTSUPP;
>>
>> Likely we don't want to enable zerocopy for SOCK_SEQPACKET. Maybe change
>> to: "if (sk->sk_type != SOCK_STREAM)"
> 
> Indeed. Thanks.
> 
> 
>>> +     if (unlikely(flags & MSG_ERRQUEUE))
>>> +             return unix_recv_error(sk, msg, size);
>>
>> In case of MSG_PEEK it might be not what user wants to receive zero-copy
>> notification error message.
> 
> MSG_ERRQUEUE means user requested error messages, them and only them.
> If MSG_PEEK (or another flag) is also set it means either peeking from
> error queue or this flag
> is ignored (likeMSG_DONTWAIT) or rejected, whatever is appropriate. At
> least it is how I originally designed
> it. But in case it was messed up with time, I have to recheck current semantics
> in tcp and align to it, whatever it is.

Ah sorry I was confused, you are right of course, when we ask for errors 
it is perfectly OK to receive an error.

Though yes, there would be no way to peek on error queue, so CRIU will 
not be able to handle such error messages, but that is different story.

> 
> 
>>> +     int err = skb_orphan_frags_rx(skb, GFP_KERNEL);
>>> +
>>
>> I prefer not to put function call on the same line with variable
> 
> Ok-ok. If my old pants are old-fashioned granpa's style, we replace them.
> 
> But why that creepy checkpatch.pl does not complain? Is it broken and
> already not haute couture? :-)

I don't think that it is a strict rule, there are many places where one 
can see such things in kernel code (e.g. osf_statfs, bpf_copy_from_user, 
update_parent_effective_cpumask or nsfs_init_fs_context). So I don't insist.

-- 
Best regards, Tikhomirov Pavel
Senior Software Developer, Virtuozzo.