don't fallback if sock doesn't undestand zc

PATCH Applied Pavel Begunkov 3 participants 10 messages Oct 20, 2022

Subsystems (3)

Networking Network drivers io_uring

lore

Share & tools

lore

Pavel Begunkov

author

2022-10-20 01:43 UTC #0 lore raw

cover don't fallback if sock doesn't undestand zc

Prefer failinig requests over copying when we're dealing with protocols that don't support io_uring zerocopy send. The patches are split to make backporting easier.

Pavel Begunkov (2):
io_uring/net: fail zc send for unsupported protocols io_uring/net: fail zc sendmsg for unsupported protocols

 io_uring/net.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

Signature · 1 line

2.38.0

Pavel Begunkov

author

2022-10-20 01:43 UTC #1 lore raw

PATCH 1/2 io_uring/net: fail zc send for unsupported protocols

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 <[email protected]> # 6.0

Signed-off-by Pavel Begunkov <[email protected]>

---
 io_uring/net.c | 9 +++++++++
 1 file changed, 9 insertions(+)

io_uring/net.c +9 -0

index 8c7226b5bf41..28127f1de1f0 100644
@@ -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));
+}
+
 static struct io_async_msghdr *io_msg_alloc_async(struct io_kiocb *req,
 						  unsigned int issue_flags)
 {

@@ -1056,6 +1063,8 @@ int io_send_zc(struct io_kiocb *req, unsigned int issue_flags)
 	sock = sock_from_file(req->file);
 	if (unlikely(!sock))
 		return -ENOTSOCK;
+	if (!io_sock_support_zc(sock))
+		return -EOPNOTSUPP;
 
 	msg.msg_name = NULL;
 	msg.msg_control = NULL;

Signature · 1 line

2.38.0

Stefan Metzmacher

2022-10-20 09:14 UTC #2 lore raw

Re: [PATCH for-6.1 1/2] io_uring/net: fail zc send for unsupported protocols

Hi Pavel,

Quoted 17 lines

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 <[email protected]> # 6.0
Signed-off-by Pavel Begunkov <[email protected]>
---
  io_uring/net.c | 9 +++++++++
  1 file changed, 9 insertions(+)
io_uring/net.c +0 -0
index 8c7226b5bf41..28127f1de1f0 100644

io_uring/net.c static void io_netmsg_recycle(struct io_kiocb *req, unsigned int issue_flags) · line 128

Quoted 9 lines

io_uring/net.c +6 -0

@@ -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

Jens Axboe

maintainer

2022-10-20 12:49 UTC #3 lore raw

Re: [PATCH for-6.1 1/2] io_uring/net: fail zc send for unsupported protocols

Quoted 51 lines · Stefan Metzmacher

Hi Pavel,
Quoted 17 lines
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 <[email protected]> # 6.0
Signed-off-by Pavel Begunkov <[email protected]>
---
io_uring/net.c | 9 +++++++++
  1 file changed, 9 insertions(+)
io_uring/net.c +0 -0
index 8c7226b5bf41..28127f1de1f0 100644
io_uring/net.c static void io_netmsg_recycle(struct io_kiocb *req, unsigned int issue_flags) · line 127
Quoted 8 lines
io_uring/net.c +5 -0
@@ -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;

Quoted 6 lines

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.

Signature · 1 line

Jens Axboe

Pavel Begunkov

author

2022-10-20 12:55 UTC #4 lore raw

Re: [PATCH for-6.1 1/2] io_uring/net: fail zc send for unsupported protocols

Quoted 57 lines · Jens Axboe

Quoted 56 lines · Stefan Metzmacher
Hi Pavel,
Quoted 17 lines
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 <[email protected]> # 6.0
Signed-off-by Pavel Begunkov <[email protected]>
---
   io_uring/net.c | 9 +++++++++
   1 file changed, 9 insertions(+)
io_uring/net.c +0 -0
index 8c7226b5bf41..28127f1de1f0 100644
io_uring/net.c static void io_netmsg_recycle(struct io_kiocb *req, unsigned int issue_flags) · line 127
Quoted 8 lines
io_uring/net.c +5 -0
@@ -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;

Quoted 4 lines

Quoted 1 line
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.

Signature · 1 line

Pavel Begunkov

Jens Axboe

maintainer

2022-10-20 12:59 UTC #5 lore raw

Re: [PATCH for-6.1 1/2] io_uring/net: fail zc send for unsupported protocols

Quoted 58 lines · Pavel Begunkov

Quoted 57 lines · Jens Axboe
Quoted 56 lines · Stefan Metzmacher
Hi Pavel,
Quoted 17 lines
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 <[email protected]> # 6.0
Signed-off-by Pavel Begunkov <[email protected]>
---
?? io_uring/net.c | 9 +++++++++
?? 1 file changed, 9 insertions(+)
io_uring/net.c +0 -0
index 8c7226b5bf41..28127f1de1f0 100644
io_uring/net.c static void io_netmsg_recycle(struct io_kiocb *req, unsigned int issue_flags) · line 127
Quoted 8 lines
io_uring/net.c +5 -0
@@ -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;

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
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).

Signature · 1 line

Jens Axboe

Pavel Begunkov

author

2022-10-20 12:50 UTC #6 lore raw

Re: [PATCH for-6.1 1/2] io_uring/net: fail zc send for unsupported protocols

In reply to Stefan Metzmacher

Quoted 51 lines · Stefan Metzmacher

Hi Pavel,
Quoted 17 lines
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 <[email protected]> # 6.0
Signed-off-by Pavel Begunkov <[email protected]>
---
io_uring/net.c | 9 +++++++++
  1 file changed, 9 insertions(+)
io_uring/net.c +0 -0
index 8c7226b5bf41..28127f1de1f0 100644
io_uring/net.c static void io_netmsg_recycle(struct io_kiocb *req, unsigned int issue_flags) · line 127
Quoted 8 lines
io_uring/net.c +6 -0
@@ -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;

Quoted 8 lines

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?

Signature · 1 line

Pavel Begunkov

Stefan Metzmacher

2022-10-20 13:05 UTC #7 lore raw

Re: [PATCH for-6.1 1/2] io_uring/net: fail zc send for unsupported protocols

Hi Pavel,

Quoted 10 lines

Quoted 8 lines
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!

Quoted 2 lines

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

Pavel Begunkov

author

2022-10-20 01:43 UTC #8 lore raw

PATCH 2/2 io_uring/net: fail zc sendmsg for unsupported protocols

In reply to Pavel Begunkov

The previous patch fails zerocopy send requests for protocols that don't support it, do the same for zerocopy sendmsg.

Signed-off-by Pavel Begunkov <[email protected]>

---
 io_uring/net.c | 2 ++
 1 file changed, 2 insertions(+)

io_uring/net.c +2 -0

index 28127f1de1f0..735eec545115 100644
@@ -1160,6 +1160,8 @@ int io_sendmsg_zc(struct io_kiocb *req, unsigned int issue_flags)
 	sock = sock_from_file(req->file);
 	if (unlikely(!sock))
 		return -ENOTSOCK;
+	if (!io_sock_support_zc(sock))
+		return -EOPNOTSUPP;
 
 	if (req_has_async_data(req)) {
 		kmsg = req->async_data;

Signature · 1 line

2.38.0

Jens Axboe

maintainer Applied

2022-10-20 02:26 UTC #9 lore raw

In reply to Pavel Begunkov

Quoted 9 lines · Pavel Begunkov

Prefer failinig requests over copying when we're dealing with protocols that don't support io_uring zerocopy send. The patches are split to make backporting easier.
Pavel Begunkov (2):
io_uring/net: fail zc send for unsupported protocols io_uring/net: fail zc sendmsg for unsupported protocols
[...]

Applied, thanks!

[1/2] io_uring/net: fail zc send for unsupported protocols

      commit: cf6d01d79d3ea2274891c79cb51fa80dfde39acd
[2/2] io_uring/net: fail zc sendmsg for unsupported protocols
      commit: 9f92d171efe73abc3d026ed9e21f08b482075782

Best regards,

Signature · 1 line

Jens Axboe