minor tcp io_uring zc optimisations

PATCH Applied Pavel Begunkov 4 participants 12 messages May 15, 2023 – May 17, 2023

Subsystems (4)

Networking Network drivers TCP io_uring

lore

Share & tools

lore

Pavel Begunkov

author

2023-05-15 16:08 UTC #0 lore raw

cover minor tcp io_uring zc optimisations

Patch 1 is a simple cleanup, patch 2 gives removes 2 atomics from the io_uring zc TCP submission path, which yielded extra 0.5% for my throughput CPU bound tests based on liburing/examples/send-zerocopy.c

Pavel Begunkov (2):
net/tcp: don't peek at tail for io_uring zc
net/tcp: optimise io_uring zc ubuf refcounting

 net/ipv4/tcp.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Signature · 1 line

2.40.0

Pavel Begunkov

author

2023-05-15 16:08 UTC #1 lore raw

PATCH 1/2 net/tcp: don't peek at tail for io_uring zc

Move tcp_write_queue_tail() to SOCK_ZEROCOPY specific flag as zerocopy setup for msghdr->ubuf_info doesn't need to peek into the last request.

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

---
 net/ipv4/tcp.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

net/ipv4/tcp.c +1 -2

index 4d6392c16b7a..40f591f7fce1 100644
@@ -1229,13 +1229,12 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
 	flags = msg->msg_flags;
 
 	if ((flags & MSG_ZEROCOPY) && size) {
-		skb = tcp_write_queue_tail(sk);
-
 		if (msg->msg_ubuf) {
 			uarg = msg->msg_ubuf;
 			net_zcopy_get(uarg);
 			zc = sk->sk_route_caps & NETIF_F_SG;
 		} else if (sock_flag(sk, SOCK_ZEROCOPY)) {
+			skb = tcp_write_queue_tail(sk);
 			uarg = msg_zerocopy_realloc(sk, size, skb_zcopy(skb));
 			if (!uarg) {
 				err = -ENOBUFS;

Signature · 1 line

2.40.0

David Ahern

Reviewed-by on v1 1/2

2023-05-15 17:27 UTC #2 lore raw

Re: [PATCH net-next 1/2] net/tcp: don't peek at tail for io_uring zc

Quoted 8 lines · Pavel Begunkov

Move tcp_write_queue_tail() to SOCK_ZEROCOPY specific flag as zerocopy setup for msghdr->ubuf_info doesn't need to peek into the last request.
Signed-off-by Pavel Begunkov <[email protected]>
---
 net/ipv4/tcp.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Reviewed-by David Ahern <[email protected]>

Eric Dumazet

Reviewed-by

2023-05-15 18:11 UTC #3 lore raw

Re: [PATCH net-next 1/2] net/tcp: don't peek at tail for io_uring zc

Quoted 5 lines · David Ahern

Quoted 3 lines · Pavel Begunkov
Move tcp_write_queue_tail() to SOCK_ZEROCOPY specific flag as zerocopy setup for msghdr->ubuf_info doesn't need to peek into the last request.

Quoted 7 lines

Quoted 5 lines
Signed-off-by Pavel Begunkov <[email protected]>
---
 net/ipv4/tcp.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by David Ahern <[email protected]>

Reviewed-by Eric Dumazet <[email protected]>

Pavel Begunkov

author

2023-05-15 16:08 UTC #4 lore raw

PATCH 2/2 net/tcp: optimise io_uring zc ubuf refcounting

In reply to Pavel Begunkov

io_uring keeps a reference to ubuf_info during submission, so if tcp_sendmsg_locked() sees msghdr::msg_ubuf in can be sure the buffer will be kept alive and doesn't need to additionally pin it.

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

---
 net/ipv4/tcp.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

net/ipv4/tcp.c +6 -3

index 40f591f7fce1..3d18e295bb2f 100644
@@ -1231,7 +1231,6 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
 	if ((flags & MSG_ZEROCOPY) && size) {
 		if (msg->msg_ubuf) {
 			uarg = msg->msg_ubuf;
-			net_zcopy_get(uarg);
 			zc = sk->sk_route_caps & NETIF_F_SG;
 		} else if (sock_flag(sk, SOCK_ZEROCOPY)) {
 			skb = tcp_write_queue_tail(sk);

@@ -1458,7 +1457,9 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
 		tcp_push(sk, flags, mss_now, tp->nonagle, size_goal);
 	}
 out_nopush:
-	net_zcopy_put(uarg);
+	/* msg->msg_ubuf is pinned by the caller so we don't take extra refs */
+	if (uarg && !msg->msg_ubuf)
+		net_zcopy_put(uarg);
 	return copied + copied_syn;
 
 do_error:

@@ -1467,7 +1468,9 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
 	if (copied + copied_syn)
 		goto out;
 out_err:
-	net_zcopy_put_abort(uarg, true);
+	/* msg->msg_ubuf is pinned by the caller so we don't take extra refs */
+	if (uarg && !msg->msg_ubuf)
+		net_zcopy_put_abort(uarg, true);
 	err = sk_stream_error(sk, flags, err);
 	/* make sure we wake any epoll edge trigger waiter */
 	if (unlikely(tcp_rtx_and_write_queues_empty(sk) && err == -EAGAIN)) {

Signature · 1 line

2.40.0

David Ahern

2023-05-15 17:29 UTC #5 lore raw

Re: [PATCH net-next 2/2] net/tcp: optimise io_uring zc ubuf refcounting

Quoted 24 lines · Pavel Begunkov

net/ipv4/tcp.c +3 -2

index 40f591f7fce1..3d18e295bb2f 100644
@@ -1231,7 +1231,6 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
 	if ((flags & MSG_ZEROCOPY) && size) {
 		if (msg->msg_ubuf) {
 			uarg = msg->msg_ubuf;
-			net_zcopy_get(uarg);
 			zc = sk->sk_route_caps & NETIF_F_SG;
 		} else if (sock_flag(sk, SOCK_ZEROCOPY)) {
 			skb = tcp_write_queue_tail(sk);

@@ -1458,7 +1457,9 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
 		tcp_push(sk, flags, mss_now, tp->nonagle, size_goal);
 	}
 out_nopush:
-	net_zcopy_put(uarg);
+	/* msg->msg_ubuf is pinned by the caller so we don't take extra refs */
+	if (uarg && !msg->msg_ubuf)
+		net_zcopy_put(uarg);
 	return copied + copied_syn;
 
 do_error:

net/ipv4/tcp.c int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size) · line 1476

Quoted 10 lines

net/ipv4/tcp.c +3 -1

@@ -1467,7 +1468,9 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
 	if (copied + copied_syn)
 		goto out;
 out_err:
-	net_zcopy_put_abort(uarg, true);
+	/* msg->msg_ubuf is pinned by the caller so we don't take extra refs */
+	if (uarg && !msg->msg_ubuf)
+		net_zcopy_put_abort(uarg, true);
 	err = sk_stream_error(sk, flags, err);
 	/* make sure we wake any epoll edge trigger waiter */
 	if (unlikely(tcp_rtx_and_write_queues_empty(sk) && err == -EAGAIN)) {

Both net_zcopy_put_abort and net_zcopy_put have an if (uarg) check.

Eric Dumazet

Reviewed-by

2023-05-15 18:14 UTC #6 lore raw

Re: [PATCH net-next 2/2] net/tcp: optimise io_uring zc ubuf refcounting

Quoted 24 lines · David Ahern

Quoted 22 lines · Pavel Begunkov

net/ipv4/tcp.c +3 -2

index 40f591f7fce1..3d18e295bb2f 100644
@@ -1231,7 +1231,6 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
      if ((flags & MSG_ZEROCOPY) && size) {
              if (msg->msg_ubuf) {
                      uarg = msg->msg_ubuf;
-                     net_zcopy_get(uarg);
                      zc = sk->sk_route_caps & NETIF_F_SG;
              } else if (sock_flag(sk, SOCK_ZEROCOPY)) {
                      skb = tcp_write_queue_tail(sk);

@@ -1458,7 +1457,9 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
              tcp_push(sk, flags, mss_now, tp->nonagle, size_goal);
      }
 out_nopush:
-     net_zcopy_put(uarg);
+     /* msg->msg_ubuf is pinned by the caller so we don't take extra refs */
+     if (uarg && !msg->msg_ubuf)
+             net_zcopy_put(uarg);
      return copied + copied_syn;

Quoted 14 lines

net/ipv4/tcp.c int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size) · line 1476

Quoted 12 lines

net/ipv4/tcp.c +3 -1

 do_error:

@@ -1467,7 +1468,9 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
      if (copied + copied_syn)
              goto out;
 out_err:
-     net_zcopy_put_abort(uarg, true);
+     /* msg->msg_ubuf is pinned by the caller so we don't take extra refs */
+     if (uarg && !msg->msg_ubuf)
+             net_zcopy_put_abort(uarg, true);
      err = sk_stream_error(sk, flags, err);
      /* make sure we wake any epoll edge trigger waiter */
      if (unlikely(tcp_rtx_and_write_queues_empty(sk) && err == -EAGAIN)) {

Both net_zcopy_put_abort and net_zcopy_put have an if (uarg) check.

Right, but here this might avoid a read of msg->msg_ubuf, which might be more expensive to fetch.

Compiler will probably remove the second test (uarg) from net_zcopy_put()

Reviewed-by Eric Dumazet <[email protected]>

David Ahern

2023-05-15 18:40 UTC #7 lore raw

Re: [PATCH net-next 2/2] net/tcp: optimise io_uring zc ubuf refcounting

Quoted 40 lines · Eric Dumazet

Quoted 24 lines · David Ahern

Quoted 22 lines · Pavel Begunkov

net/ipv4/tcp.c +3 -2

index 40f591f7fce1..3d18e295bb2f 100644
@@ -1231,7 +1231,6 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
      if ((flags & MSG_ZEROCOPY) && size) {
              if (msg->msg_ubuf) {
                      uarg = msg->msg_ubuf;
-                     net_zcopy_get(uarg);
                      zc = sk->sk_route_caps & NETIF_F_SG;
              } else if (sock_flag(sk, SOCK_ZEROCOPY)) {
                      skb = tcp_write_queue_tail(sk);

@@ -1458,7 +1457,9 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
              tcp_push(sk, flags, mss_now, tp->nonagle, size_goal);
      }
 out_nopush:
-     net_zcopy_put(uarg);
+     /* msg->msg_ubuf is pinned by the caller so we don't take extra refs */
+     if (uarg && !msg->msg_ubuf)
+             net_zcopy_put(uarg);
      return copied + copied_syn;

Quoted 14 lines

net/ipv4/tcp.c int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size) · line 1476

Quoted 12 lines

net/ipv4/tcp.c +3 -1

 do_error:

@@ -1467,7 +1468,9 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
      if (copied + copied_syn)
              goto out;
 out_err:
-     net_zcopy_put_abort(uarg, true);
+     /* msg->msg_ubuf is pinned by the caller so we don't take extra refs */
+     if (uarg && !msg->msg_ubuf)
+             net_zcopy_put_abort(uarg, true);
      err = sk_stream_error(sk, flags, err);
      /* make sure we wake any epoll edge trigger waiter */
      if (unlikely(tcp_rtx_and_write_queues_empty(sk) && err == -EAGAIN)) {

Both net_zcopy_put_abort and net_zcopy_put have an if (uarg) check.

Quoted 2 lines

Right, but here this might avoid a read of msg->msg_ubuf, which might be more expensive to fetch.

agreed.

Quoted 4 lines

Compiler will probably remove the second test (uarg) from net_zcopy_put()
Reviewed-by Eric Dumazet <[email protected]>

The one in net_zcopy_put can be removed with the above change. It's other caller is net_zcopy_put_abort which has already checked uarg is set.

Pavel Begunkov

author

2023-05-16 13:01 UTC #8 lore raw

Re: [PATCH net-next 2/2] net/tcp: optimise io_uring zc ubuf refcounting

Quoted 41 lines · David Ahern

Quoted 40 lines · Eric Dumazet

Quoted 38 lines · David Ahern

Quoted 24 lines · Pavel Begunkov

net/ipv4/tcp.c +3 -2

index 40f591f7fce1..3d18e295bb2f 100644
@@ -1231,7 +1231,6 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
       if ((flags & MSG_ZEROCOPY) && size) {
               if (msg->msg_ubuf) {
                       uarg = msg->msg_ubuf;
-                     net_zcopy_get(uarg);
                       zc = sk->sk_route_caps & NETIF_F_SG;
               } else if (sock_flag(sk, SOCK_ZEROCOPY)) {
                       skb = tcp_write_queue_tail(sk);

@@ -1458,7 +1457,9 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
               tcp_push(sk, flags, mss_now, tp->nonagle, size_goal);
       }
  out_nopush:
-     net_zcopy_put(uarg);
+     /* msg->msg_ubuf is pinned by the caller so we don't take extra refs */
+     if (uarg && !msg->msg_ubuf)
+             net_zcopy_put(uarg);
       return copied + copied_syn;
 
  do_error:

net/ipv4/tcp.c int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size) · line 1476

Quoted 10 lines

net/ipv4/tcp.c +3 -1

@@ -1467,7 +1468,9 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
       if (copied + copied_syn)
               goto out;
  out_err:
-     net_zcopy_put_abort(uarg, true);
+     /* msg->msg_ubuf is pinned by the caller so we don't take extra refs */
+     if (uarg && !msg->msg_ubuf)
+             net_zcopy_put_abort(uarg, true);
       err = sk_stream_error(sk, flags, err);
       /* make sure we wake any epoll edge trigger waiter */
       if (unlikely(tcp_rtx_and_write_queues_empty(sk) && err == -EAGAIN)) {

Both net_zcopy_put_abort and net_zcopy_put have an if (uarg) check.

Quoted 4 lines

Quoted 2 lines
Right, but here this might avoid a read of msg->msg_ubuf, which might be more expensive to fetch.
agreed.

I put it there to avoid one extra check in the non-zerocopy path. msg->msg_ubuf is null there, the conditional will pass and it'll still have to test uarg.

Quoted 3 lines

Quoted 3 lines
Compiler will probably remove the second test (uarg) from net_zcopy_put()
Reviewed-by Eric Dumazet <[email protected]>

Thank you for reviews!

Quoted 2 lines

The one in net_zcopy_put can be removed with the above change. It's other caller is net_zcopy_put_abort which has already checked uarg is set.

Ah yes, do you want me to fold it in?

Signature · 1 line

Pavel Begunkov

David Ahern

2023-05-16 14:37 UTC #9 lore raw

Re: [PATCH net-next 2/2] net/tcp: optimise io_uring zc ubuf refcounting

Quoted 8 lines · Pavel Begunkov

Quoted 5 lines
The one in net_zcopy_put can be removed with the above change. It's other caller is net_zcopy_put_abort which has already checked uarg is set.
Ah yes, do you want me to fold it in?

no preference.

Pavel Begunkov

author

2023-05-16 17:49 UTC #10 lore raw

Re: [PATCH net-next 2/2] net/tcp: optimise io_uring zc ubuf refcounting

Quoted 11 lines · David Ahern

Quoted 8 lines · Pavel Begunkov
Quoted 5 lines
The one in net_zcopy_put can be removed with the above change. It's other caller is net_zcopy_put_abort which has already checked uarg is set.
Ah yes, do you want me to fold it in?
no preference.

I'll leave it for another patch then. It might be interesting to try remove null checks for all *_zcopy_* helpers, but it didn't feel right last time I tried.

Signature · 1 line

Pavel Begunkov

[email protected]

CI Applied

2023-05-17 07:40 UTC #11 lore raw

In reply to Pavel Begunkov

This series was applied to netdev/net-next.git (main) by David S. Miller <[email protected]>:

Hello:

This series was applied to netdev/net-next.git (main) by David S. Miller <[email protected]>:

Quoted 9 lines · On Mon, 15 May 2023 17:06:35 +0100 you wrote:

Patch 1 is a simple cleanup, patch 2 gives removes 2 atomics from the io_uring zc TCP submission path, which yielded extra 0.5% for my throughput CPU bound tests based on liburing/examples/send-zerocopy.c
Pavel Begunkov (2):
net/tcp: don't peek at tail for io_uring zc
net/tcp: optimise io_uring zc ubuf refcounting
[...]

Here is the summary with links:
- [net-next,1/2] net/tcp: don't peek at tail for io_uring zc

    https://git.kernel.org/netdev/net-next/c/eea96a3e2c90
  - [net-next,2/2] net/tcp: optimise io_uring zc ubuf refcounting
    https://git.kernel.org/netdev/net-next/c/a7533584728d

You are awesome, thank you!

Signature · 2 lines

Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html