[PATCH net-next] net: avoid unneeded atomic operation in ip*_append_data()

[PATCH net-next] net: avoid unneeded atomic operation in ip*_append_data()

[Paolo Abeni]

After commit 694aba690de0 ("ipv4: factorize sk_wmem_alloc updates
done by __ip_append_data()") and commit 1f4c6eb24029 ("ipv6:
factorize sk_wmem_alloc updates done by __ip6_append_data()"),
when transmitting sub MTU datagram, an addtional, unneeded atomic
operation is performed in ip*_append_data() to update wmem_alloc:
in the above condition the delta is 0.

The above cause small but measurable performance regression in UDP
xmit tput test with packet size below MTU.

This change avoids such overhead updating wmem_alloc only if
wmem_alloc_delta is non zero.

The error path is left intentionally unmodified: it's a slow path
and simplicity is preferred to performances.

Fixes: 694aba690de0 ("ipv4: factorize sk_wmem_alloc updates done by __ip_append_data()")
Fixes: 1f4c6eb24029 ("ipv6: factorize sk_wmem_alloc updates done by __ip6_append_data()")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/ipv4/ip_output.c  | 3 ++-
 net/ipv6/ip6_output.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index 94cacae76aca..4c11b810a447 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -1090,7 +1090,8 @@ static int __ip_append_data(struct sock *sk,
 		length -= copy;
 	}
 
-	refcount_add(wmem_alloc_delta, &sk->sk_wmem_alloc);
+	if (wmem_alloc_delta)
+		refcount_add(wmem_alloc_delta, &sk->sk_wmem_alloc);
 	return 0;
 
 error_efault:
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index e6eaa4dd9f60..b4ae9b7941fa 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -1536,7 +1536,8 @@ static int __ip6_append_data(struct sock *sk,
 		length -= copy;
 	}
 
-	refcount_add(wmem_alloc_delta, &sk->sk_wmem_alloc);
+	if (wmem_alloc_delta)
+		refcount_add(wmem_alloc_delta, &sk->sk_wmem_alloc);
 	return 0;
 
 error_efault:
-- 
2.14.3

Re: [PATCH net-next] net: avoid unneeded atomic operation in ip*_append_data()

[Eric Dumazet]

On 04/04/2018 05:30 AM, Paolo Abeni wrote:
> After commit 694aba690de0 ("ipv4: factorize sk_wmem_alloc updates
> done by __ip_append_data()") and commit 1f4c6eb24029 ("ipv6:
> factorize sk_wmem_alloc updates done by __ip6_append_data()"),
> when transmitting sub MTU datagram, an addtional, unneeded atomic
> operation is performed in ip*_append_data() to update wmem_alloc:
> in the above condition the delta is 0.
> 
> The above cause small but measurable performance regression in UDP
> xmit tput test with packet size below MTU.
> 
> This change avoids such overhead updating wmem_alloc only if
> wmem_alloc_delta is non zero.
> 
> The error path is left intentionally unmodified: it's a slow path
> and simplicity is preferred to performances.
> 
> Fixes: 694aba690de0 ("ipv4: factorize sk_wmem_alloc updates done by __ip_append_data()")
> Fixes: 1f4c6eb24029 ("ipv6: factorize sk_wmem_alloc updates done by __ip6_append_data()")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---

SGTM, thanks Paolo

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

My intent was to modify sock_alloc_send_pskb() to accept to opt-out
the skb_set_owner_w() call, but I forgot that the merge window was starting last week-end.

So if a UDP datagram needs 2 skb, only one sk_wmem_alloc change would happen, not 1 + 1.

Re: [PATCH net-next] net: avoid unneeded atomic operation in ip*_append_data()

[David Miller]

From: Paolo Abeni <pabeni@redhat.com>
Date: Wed,  4 Apr 2018 14:30:01 +0200

> After commit 694aba690de0 ("ipv4: factorize sk_wmem_alloc updates
> done by __ip_append_data()") and commit 1f4c6eb24029 ("ipv6:
> factorize sk_wmem_alloc updates done by __ip6_append_data()"),
> when transmitting sub MTU datagram, an addtional, unneeded atomic
> operation is performed in ip*_append_data() to update wmem_alloc:
> in the above condition the delta is 0.
> 
> The above cause small but measurable performance regression in UDP
> xmit tput test with packet size below MTU.
> 
> This change avoids such overhead updating wmem_alloc only if
> wmem_alloc_delta is non zero.
> 
> The error path is left intentionally unmodified: it's a slow path
> and simplicity is preferred to performances.
> 
> Fixes: 694aba690de0 ("ipv4: factorize sk_wmem_alloc updates done by __ip_append_data()")
> Fixes: 1f4c6eb24029 ("ipv6: factorize sk_wmem_alloc updates done by __ip6_append_data()")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
 ...
> -	refcount_add(wmem_alloc_delta, &sk->sk_wmem_alloc);
> +	if (wmem_alloc_delta)
> +		refcount_add(wmem_alloc_delta, &sk->sk_wmem_alloc);
 ...
> -	refcount_add(wmem_alloc_delta, &sk->sk_wmem_alloc);
> +	if (wmem_alloc_delta)
> +		refcount_add(wmem_alloc_delta, &sk->sk_wmem_alloc);

This is simple enough, so applied.

But I wonder if atomic_{add,sub} and refcount_{add,sub}() should just check
for zero inline, just like the {set,clear}_bit() implementations avoid the
atomic operation if the bit already has the desired value.