[PATCH net-next 0/2] minor tcp io_uring zc optimisations

[PATCH net-next 0/2] minor tcp io_uring zc optimisations

[Pavel Begunkov]

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(-)

-- 
2.40.0

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

[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 <asml.silence@gmail.com>
---
 net/ipv4/tcp.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 4d6392c16b7a..40f591f7fce1 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -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;
-- 
2.40.0

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

[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 <asml.silence@gmail.com>
---
 net/ipv4/tcp.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 40f591f7fce1..3d18e295bb2f 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -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)) {
-- 
2.40.0

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

[David Ahern]

On 5/15/23 10:06 AM, Pavel Begunkov wrote:
> 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 <asml.silence@gmail.com>
> ---
>  net/ipv4/tcp.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

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

[David Ahern]

On 5/15/23 10:06 AM, Pavel Begunkov wrote:
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 40f591f7fce1..3d18e295bb2f 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -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)) {

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

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

[Eric Dumazet]

On Mon, May 15, 2023 at 7:27 PM David Ahern <dsahern@kernel.org> wrote:
>
> On 5/15/23 10:06 AM, Pavel Begunkov wrote:
> > 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 <asml.silence@gmail.com>
> > ---
> >  net/ipv4/tcp.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
>
> Reviewed-by: David Ahern <dsahern@kernel.org>


Reviewed-by: Eric Dumazet <edumazet@google.com>

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

[Eric Dumazet]

On Mon, May 15, 2023 at 7:29 PM David Ahern <dsahern@kernel.org> wrote:
>
> On 5/15/23 10:06 AM, Pavel Begunkov wrote:
> > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > index 40f591f7fce1..3d18e295bb2f 100644
> > --- a/net/ipv4/tcp.c
> > +++ b/net/ipv4/tcp.c
> > @@ -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)) {
>
> 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 <edumazet@google.com>

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

[David Ahern]

On 5/15/23 12:14 PM, Eric Dumazet wrote:
> On Mon, May 15, 2023 at 7:29 PM David Ahern <dsahern@kernel.org> wrote:
>>
>> On 5/15/23 10:06 AM, Pavel Begunkov wrote:
>>> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
>>> index 40f591f7fce1..3d18e295bb2f 100644
>>> --- a/net/ipv4/tcp.c
>>> +++ b/net/ipv4/tcp.c
>>> @@ -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)) {
>>
>> 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.

agreed.

> 
> Compiler will probably remove the second test (uarg) from net_zcopy_put()
> 
> Reviewed-by: Eric Dumazet <edumazet@google.com>

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.

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

[Pavel Begunkov]

On 5/15/23 19:40, David Ahern wrote:
> On 5/15/23 12:14 PM, Eric Dumazet wrote:
>> On Mon, May 15, 2023 at 7:29 PM David Ahern <dsahern@kernel.org> wrote:
>>>
>>> On 5/15/23 10:06 AM, Pavel Begunkov wrote:
>>>> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
>>>> index 40f591f7fce1..3d18e295bb2f 100644
>>>> --- a/net/ipv4/tcp.c
>>>> +++ b/net/ipv4/tcp.c
>>>> @@ -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)) {
>>>
>>> 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.
> 
> 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.


>> Compiler will probably remove the second test (uarg) from net_zcopy_put()
>>
>> Reviewed-by: Eric Dumazet <edumazet@google.com>

Thank you for reviews!


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

-- 
Pavel Begunkov

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

[David Ahern]

On 5/16/23 6:59 AM, Pavel Begunkov wrote:
> 
> 
>> 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.

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

[Pavel Begunkov]

On 5/16/23 15:37, David Ahern wrote:
> On 5/16/23 6:59 AM, Pavel Begunkov wrote:
>>
>>
>>> 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.

-- 
Pavel Begunkov

Re: [PATCH net-next 0/2] minor tcp io_uring zc optimisations

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by David S. Miller <davem@davemloft.net>:

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html