Re: [PATCH for-6.1 1/3] net: flag sockets supporting msghdr originated zerocopy

[Pavel Begunkov <asml.silence@gmail.com>]

On 10/24/22 15:12, Stefan Metzmacher wrote:
> Am 24.10.22 um 14:49 schrieb Stefan Metzmacher:
>> Am 22.10.22 um 18:07 schrieb Jens Axboe:
>>> On 10/22/22 9:57 AM, Pavel Begunkov wrote:
>>>> On 10/21/22 17:14, Jakub Kicinski wrote:
>>>>> On Fri, 21 Oct 2022 11:16:39 +0100 Pavel Begunkov wrote:
>>>>>> We need an efficient way in io_uring to check whether a socket supports
>>>>>> zerocopy with msghdr provided ubuf_info. Add a new flag into the struct
>>>>>> socket flags fields.
>>>>>>
>>>>>> Cc: <stable@vger.kernel.org> # 6.0
>>>>>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>>>>>> ---
>>>>>> ? include/linux/net.h | 1 +
>>>>>> ? net/ipv4/tcp.c????? | 1 +
>>>>>> ? net/ipv4/udp.c????? | 1 +
>>>>>> ? 3 files changed, 3 insertions(+)
>>>>>>
>>>>>> diff --git a/include/linux/net.h b/include/linux/net.h
>>>>>> index 711c3593c3b8..18d942bbdf6e 100644
>>>>>> --- a/include/linux/net.h
>>>>>> +++ b/include/linux/net.h
>>>>>> @@ -41,6 +41,7 @@ struct net;
>>>>>> ? #define SOCK_NOSPACE??????? 2
>>>>>> ? #define SOCK_PASSCRED??????? 3
>>>>>> ? #define SOCK_PASSSEC??????? 4
>>>>>> +#define SOCK_SUPPORT_ZC??????? 5
>>>>>
>>>>> Acked-by: Jakub Kicinski <kuba@kernel.org>
>>>>>
>>>>> Any idea on when this will make it to Linus? If within a week we can
>>>>> probably delay:
>>>>
>>>> After a chat with Jens, IIUC he can take it and send out to
>>>> Linus early. So, sounds like a good plan
>>>
>>> Yes, and let's retain the name for now, can always be changed if we need
>>> to make it more granular. I'll ship this off before -rc2.
>>
>> I'm now always getting -EOPNOTSUPP from SENDMSG_ZC for tcp connections...
> 
> The problem is that inet_accept() doesn't set SOCK_SUPPORT_ZC...

Yeah, you're right, accept is not handled, not great. Thanks
for tracking it down. And I'll add a test for it


> This hack below fixes it for me, but I'm sure there's a nicer way...
> 
> The natural way would be to overload inet_csk_accept for tcp,
> but sk1->sk_prot->accept() is called with sk2->sk_socket == NULL,
> because sock_graft() is called later...

I think it's acceptable for a fix. sk_is_tcp() sounds a bit better
assuming that sk_type/protocol are set there.


> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> index 3dd02396517d..0f35f2a32756 100644
> --- a/net/ipv4/af_inet.c
> +++ b/net/ipv4/af_inet.c
> @@ -736,6 +736,7 @@ EXPORT_SYMBOL(inet_stream_connect);
>    *    Accept a pending connection. The TCP layer now gives BSD semantics.
>    */
> 
> +#include <net/transp_v6.h>
>   int inet_accept(struct socket *sock, struct socket *newsock, int flags,
>           bool kern)
>   {
> @@ -754,6 +755,8 @@ int inet_accept(struct socket *sock, struct socket *newsock, int flags,
>             (TCPF_ESTABLISHED | TCPF_SYN_RECV |
>             TCPF_CLOSE_WAIT | TCPF_CLOSE)));
> 
> +    if (sk2->sk_prot == &tcp_prot || sk2->sk_prot == &tcpv6_prot)
> +        set_bit(SOCK_SUPPORT_ZC, &newsock->flags);
>       sock_graft(sk2, newsock);
> 
>       newsock->state = SS_CONNECTED;

-- 
Pavel Begunkov