* Re: [PATCH for-6.1 1/2] io_uring/net: fail zc send for unsupported protocols [not found] ` <ee7c163db8cea65b208d327610a6a96f936c1c6f.1666229889.git.asml.silence@gmail.com> @ 2022-10-20 9:13 ` Stefan Metzmacher 2022-10-20 12:48 ` Pavel Begunkov 2022-10-20 12:49 ` Jens Axboe 0 siblings, 2 replies; 6+ messages in thread From: Stefan Metzmacher @ 2022-10-20 9:13 UTC (permalink / raw) To: Pavel Begunkov, io-uring; +Cc: Jens Axboe, netdev Hi Pavel, > If a protocol doesn't support zerocopy it will silently fall back to > copying. This type of behaviour has always been a source of troubles > so it's better to fail such requests instead. For now explicitly > whitelist supported protocols in io_uring, which should be turned later > into a socket flag. > > Cc: <stable@vger.kernel.org> # 6.0 > Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> > --- > io_uring/net.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/io_uring/net.c b/io_uring/net.c > index 8c7226b5bf41..28127f1de1f0 100644 > --- a/io_uring/net.c > +++ b/io_uring/net.c > @@ -120,6 +120,13 @@ static void io_netmsg_recycle(struct io_kiocb *req, unsigned int issue_flags) > } > } > > +static inline bool io_sock_support_zc(struct socket *sock) > +{ > + return likely(sock->sk && sk_fullsock(sock->sk) && > + (sock->sk->sk_protocol == IPPROTO_TCP || > + sock->sk->sk_protocol == IPPROTO_UDP)); > +} Can we please make this more generic (at least for 6.1, which is likely be an lts release) It means my out of tree smbdirect driver would not be able to provide SENDMSG_ZC. Currently sk_setsockopt has this logic: case SO_ZEROCOPY: if (sk->sk_family == PF_INET || sk->sk_family == PF_INET6) { if (!(sk_is_tcp(sk) || (sk->sk_type == SOCK_DGRAM && sk->sk_protocol == IPPROTO_UDP))) ret = -EOPNOTSUPP; } else if (sk->sk_family != PF_RDS) { ret = -EOPNOTSUPP; } if (!ret) { if (val < 0 || val > 1) ret = -EINVAL; else sock_valbool_flag(sk, SOCK_ZEROCOPY, valbool); } break; Maybe the socket creation code could set unsigned char skc_so_zerocopy_supported:1; and/or unsigned char skc_zerocopy_msg_ubuf_supported:1; In order to avoid the manual complex tests. What do you think? metze ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH for-6.1 1/2] io_uring/net: fail zc send for unsupported protocols 2022-10-20 9:13 ` [PATCH for-6.1 1/2] io_uring/net: fail zc send for unsupported protocols Stefan Metzmacher @ 2022-10-20 12:48 ` Pavel Begunkov 2022-10-20 13:05 ` Stefan Metzmacher 2022-10-20 12:49 ` Jens Axboe 1 sibling, 1 reply; 6+ messages in thread From: Pavel Begunkov @ 2022-10-20 12:48 UTC (permalink / raw) To: Stefan Metzmacher, io-uring; +Cc: Jens Axboe, netdev On 10/20/22 10:13, Stefan Metzmacher wrote: > Hi Pavel, > >> If a protocol doesn't support zerocopy it will silently fall back to >> copying. This type of behaviour has always been a source of troubles >> so it's better to fail such requests instead. For now explicitly >> whitelist supported protocols in io_uring, which should be turned later >> into a socket flag. >> >> Cc: <stable@vger.kernel.org> # 6.0 >> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> >> --- >> io_uring/net.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/io_uring/net.c b/io_uring/net.c >> index 8c7226b5bf41..28127f1de1f0 100644 >> --- a/io_uring/net.c >> +++ b/io_uring/net.c >> @@ -120,6 +120,13 @@ static void io_netmsg_recycle(struct io_kiocb *req, unsigned int issue_flags) >> } >> } >> +static inline bool io_sock_support_zc(struct socket *sock) >> +{ >> + return likely(sock->sk && sk_fullsock(sock->sk) && >> + (sock->sk->sk_protocol == IPPROTO_TCP || >> + sock->sk->sk_protocol == IPPROTO_UDP)); >> +} > > Can we please make this more generic (at least for 6.1, which is likely be an lts release) > > It means my out of tree smbdirect driver would not be able to provide SENDMSG_ZC. > > Currently sk_setsockopt has this logic: > > case SO_ZEROCOPY: > if (sk->sk_family == PF_INET || sk->sk_family == PF_INET6) { > if (!(sk_is_tcp(sk) || > (sk->sk_type == SOCK_DGRAM && > sk->sk_protocol == IPPROTO_UDP))) > ret = -EOPNOTSUPP; > } else if (sk->sk_family != PF_RDS) { > ret = -EOPNOTSUPP; > } > if (!ret) { > if (val < 0 || val > 1) > ret = -EINVAL; > else > sock_valbool_flag(sk, SOCK_ZEROCOPY, valbool); > } > break; > > Maybe the socket creation code could set > unsigned char skc_so_zerocopy_supported:1; > and/or > unsigned char skc_zerocopy_msg_ubuf_supported:1; > > In order to avoid the manual complex tests. > > What do you think? Ok, wanted to do it rather later but let me to try fiddle with it. btw, what's happening with smbdirect? Do you plan upstream it one day and it's just maturing out of tree? -- Pavel Begunkov ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH for-6.1 1/2] io_uring/net: fail zc send for unsupported protocols 2022-10-20 12:48 ` Pavel Begunkov @ 2022-10-20 13:05 ` Stefan Metzmacher 0 siblings, 0 replies; 6+ messages in thread From: Stefan Metzmacher @ 2022-10-20 13:05 UTC (permalink / raw) To: Pavel Begunkov, io-uring; +Cc: Jens Axboe, netdev Hi Pavel, >> Maybe the socket creation code could set >> unsigned char skc_so_zerocopy_supported:1; >> and/or >> unsigned char skc_zerocopy_msg_ubuf_supported:1; >> >> In order to avoid the manual complex tests. >> >> What do you think? > > Ok, wanted to do it rather later but let me to try fiddle with it. Thanks! > btw, what's happening with smbdirect? Do you plan upstream it one day > and it's just maturing out of tree? Yes, once its stable and useful. My current plan (as time permits) is this: 1. get the samba_io_uring_ev tevent backend working (with current kernels), see my other recent mail on that. 2. add OP_SENDMSG[_ZC]/OP_RECVMSG and OP_SPLICE support for the file server part of Samba ready (based on 1.) 3. try to get a stripped down version of the smbdirect module ready to be used in cifs.ko (without exporting smbdirect sockets to userspace) upstream 4. extend the smbdirect module to be able to be used by ksmbd upstreamed 5. get the uapi for MSG_OOB and msg_control stable for samba's client and server into a useful state and then export AF_SMBDIRECT exported to userspace I hope to get 1 and 2 ready in the next weeks... metze ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH for-6.1 1/2] io_uring/net: fail zc send for unsupported protocols 2022-10-20 9:13 ` [PATCH for-6.1 1/2] io_uring/net: fail zc send for unsupported protocols Stefan Metzmacher 2022-10-20 12:48 ` Pavel Begunkov @ 2022-10-20 12:49 ` Jens Axboe 2022-10-20 12:53 ` Pavel Begunkov 1 sibling, 1 reply; 6+ messages in thread From: Jens Axboe @ 2022-10-20 12:49 UTC (permalink / raw) To: Stefan Metzmacher, Pavel Begunkov, io-uring; +Cc: netdev On 10/20/22 2:13 AM, Stefan Metzmacher wrote: > Hi Pavel, > >> If a protocol doesn't support zerocopy it will silently fall back to >> copying. This type of behaviour has always been a source of troubles >> so it's better to fail such requests instead. For now explicitly >> whitelist supported protocols in io_uring, which should be turned later >> into a socket flag. >> >> Cc: <stable@vger.kernel.org> # 6.0 >> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> >> --- >> io_uring/net.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/io_uring/net.c b/io_uring/net.c >> index 8c7226b5bf41..28127f1de1f0 100644 >> --- a/io_uring/net.c >> +++ b/io_uring/net.c >> @@ -120,6 +120,13 @@ static void io_netmsg_recycle(struct io_kiocb *req, unsigned int issue_flags) >> } >> } >> +static inline bool io_sock_support_zc(struct socket *sock) >> +{ >> + return likely(sock->sk && sk_fullsock(sock->sk) && >> + (sock->sk->sk_protocol == IPPROTO_TCP || >> + sock->sk->sk_protocol == IPPROTO_UDP)); >> +} > > Can we please make this more generic (at least for 6.1, which is likely be an lts release) > > It means my out of tree smbdirect driver would not be able to provide SENDMSG_ZC. > > Currently sk_setsockopt has this logic: > > case SO_ZEROCOPY: > if (sk->sk_family == PF_INET || sk->sk_family == PF_INET6) { > if (!(sk_is_tcp(sk) || > (sk->sk_type == SOCK_DGRAM && > sk->sk_protocol == IPPROTO_UDP))) > ret = -EOPNOTSUPP; > } else if (sk->sk_family != PF_RDS) { > ret = -EOPNOTSUPP; > } > if (!ret) { > if (val < 0 || val > 1) > ret = -EINVAL; > else > sock_valbool_flag(sk, SOCK_ZEROCOPY, valbool); > } > break; > > Maybe the socket creation code could set > unsigned char skc_so_zerocopy_supported:1; > and/or > unsigned char skc_zerocopy_msg_ubuf_supported:1; > > In order to avoid the manual complex tests. I agree that would be cleaner, even for 6.1. Let's drop these two for now. -- Jens Axboe ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH for-6.1 1/2] io_uring/net: fail zc send for unsupported protocols 2022-10-20 12:49 ` Jens Axboe @ 2022-10-20 12:53 ` Pavel Begunkov 2022-10-20 12:59 ` Jens Axboe 0 siblings, 1 reply; 6+ messages in thread From: Pavel Begunkov @ 2022-10-20 12:53 UTC (permalink / raw) To: Jens Axboe, Stefan Metzmacher, io-uring; +Cc: netdev On 10/20/22 13:49, Jens Axboe wrote: > On 10/20/22 2:13 AM, Stefan Metzmacher wrote: >> Hi Pavel, >> >>> If a protocol doesn't support zerocopy it will silently fall back to >>> copying. This type of behaviour has always been a source of troubles >>> so it's better to fail such requests instead. For now explicitly >>> whitelist supported protocols in io_uring, which should be turned later >>> into a socket flag. >>> >>> Cc: <stable@vger.kernel.org> # 6.0 >>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> >>> --- >>> io_uring/net.c | 9 +++++++++ >>> 1 file changed, 9 insertions(+) >>> >>> diff --git a/io_uring/net.c b/io_uring/net.c >>> index 8c7226b5bf41..28127f1de1f0 100644 >>> --- a/io_uring/net.c >>> +++ b/io_uring/net.c >>> @@ -120,6 +120,13 @@ static void io_netmsg_recycle(struct io_kiocb *req, unsigned int issue_flags) >>> } >>> } >>> +static inline bool io_sock_support_zc(struct socket *sock) >>> +{ >>> + return likely(sock->sk && sk_fullsock(sock->sk) && >>> + (sock->sk->sk_protocol == IPPROTO_TCP || >>> + sock->sk->sk_protocol == IPPROTO_UDP)); >>> +} >> >> Can we please make this more generic (at least for 6.1, which is likely be an lts release) >> >> It means my out of tree smbdirect driver would not be able to provide SENDMSG_ZC. >> >> Currently sk_setsockopt has this logic: >> >> case SO_ZEROCOPY: >> if (sk->sk_family == PF_INET || sk->sk_family == PF_INET6) { >> if (!(sk_is_tcp(sk) || >> (sk->sk_type == SOCK_DGRAM && >> sk->sk_protocol == IPPROTO_UDP))) >> ret = -EOPNOTSUPP; >> } else if (sk->sk_family != PF_RDS) { >> ret = -EOPNOTSUPP; >> } >> if (!ret) { >> if (val < 0 || val > 1) >> ret = -EINVAL; >> else >> sock_valbool_flag(sk, SOCK_ZEROCOPY, valbool); >> } >> break; >> >> Maybe the socket creation code could set >> unsigned char skc_so_zerocopy_supported:1; >> and/or >> unsigned char skc_zerocopy_msg_ubuf_supported:1; >> >> In order to avoid the manual complex tests. > > I agree that would be cleaner, even for 6.1. Let's drop these two > for now. As I mentioned let's drop, but if not for smb I do think it's better as doesn't require changes in multiple /net files. -- Pavel Begunkov ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH for-6.1 1/2] io_uring/net: fail zc send for unsupported protocols 2022-10-20 12:53 ` Pavel Begunkov @ 2022-10-20 12:59 ` Jens Axboe 0 siblings, 0 replies; 6+ messages in thread From: Jens Axboe @ 2022-10-20 12:59 UTC (permalink / raw) To: Pavel Begunkov, Stefan Metzmacher, io-uring; +Cc: netdev On 10/20/22 5:53 AM, Pavel Begunkov wrote: > On 10/20/22 13:49, Jens Axboe wrote: >> On 10/20/22 2:13 AM, Stefan Metzmacher wrote: >>> Hi Pavel, >>> >>>> If a protocol doesn't support zerocopy it will silently fall back to >>>> copying. This type of behaviour has always been a source of troubles >>>> so it's better to fail such requests instead. For now explicitly >>>> whitelist supported protocols in io_uring, which should be turned later >>>> into a socket flag. >>>> >>>> Cc: <stable@vger.kernel.org> # 6.0 >>>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> >>>> --- >>>> ?? io_uring/net.c | 9 +++++++++ >>>> ?? 1 file changed, 9 insertions(+) >>>> >>>> diff --git a/io_uring/net.c b/io_uring/net.c >>>> index 8c7226b5bf41..28127f1de1f0 100644 >>>> --- a/io_uring/net.c >>>> +++ b/io_uring/net.c >>>> @@ -120,6 +120,13 @@ static void io_netmsg_recycle(struct io_kiocb *req, unsigned int issue_flags) >>>> ?????? } >>>> ?? } >>>> ?? +static inline bool io_sock_support_zc(struct socket *sock) >>>> +{ >>>> +??? return likely(sock->sk && sk_fullsock(sock->sk) && >>>> +???????????? (sock->sk->sk_protocol == IPPROTO_TCP || >>>> +????????????? sock->sk->sk_protocol == IPPROTO_UDP)); >>>> +} >>> >>> Can we please make this more generic (at least for 6.1, which is likely be an lts release) >>> >>> It means my out of tree smbdirect driver would not be able to provide SENDMSG_ZC. >>> >>> Currently sk_setsockopt has this logic: >>> >>> ???????? case SO_ZEROCOPY: >>> ???????????????? if (sk->sk_family == PF_INET || sk->sk_family == PF_INET6) { >>> ???????????????????????? if (!(sk_is_tcp(sk) || >>> ?????????????????????????????? (sk->sk_type == SOCK_DGRAM && >>> ??????????????????????????????? sk->sk_protocol == IPPROTO_UDP))) >>> ???????????????????????????????? ret = -EOPNOTSUPP; >>> ???????????????? } else if (sk->sk_family != PF_RDS) { >>> ???????????????????????? ret = -EOPNOTSUPP; >>> ???????????????? } >>> ???????????????? if (!ret) { >>> ???????????????????????? if (val < 0 || val > 1) >>> ???????????????????????????????? ret = -EINVAL; >>> ???????????????????????? else >>> ???????????????????????????????? sock_valbool_flag(sk, SOCK_ZEROCOPY, valbool); >>> ???????????????? } >>> ???????????????? break; >>> >>> Maybe the socket creation code could set >>> unsigned char skc_so_zerocopy_supported:1; >>> and/or >>> unsigned char skc_zerocopy_msg_ubuf_supported:1; >>> >>> In order to avoid the manual complex tests. >> >> I agree that would be cleaner, even for 6.1. Let's drop these two >> for now. > > As I mentioned let's drop, but if not for smb I do think it's > better as doesn't require changes in multiple /net files. I do think it's cleaner to do as a socket flag rather than hardcode it in the caller (and potentially making bad assumptions, even if the out-of-tree code is a bit of a reach for sure). -- Jens Axboe ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-10-20 13:05 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <cover.1666229889.git.asml.silence@gmail.com>
[not found] ` <ee7c163db8cea65b208d327610a6a96f936c1c6f.1666229889.git.asml.silence@gmail.com>
2022-10-20 9:13 ` [PATCH for-6.1 1/2] io_uring/net: fail zc send for unsupported protocols Stefan Metzmacher
2022-10-20 12:48 ` Pavel Begunkov
2022-10-20 13:05 ` Stefan Metzmacher
2022-10-20 12:49 ` Jens Axboe
2022-10-20 12:53 ` Pavel Begunkov
2022-10-20 12:59 ` Jens Axboe
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).