[PATCH net-next] udp: use sk_protocol instead of pcflag to detect udplite sockets

[PATCH net-next] udp: use sk_protocol instead of pcflag to detect udplite sockets

[Paolo Abeni]

In the udp_sock struct, the 'forward_deficit' and 'pcflag' fields
share the same cacheline. While the first is dirtied by
udp_recvmsg, the latter is read, possibly several times, by the
bottom half processing to discriminate between udp and udplite
sockets.

With this patch, sk->sk_protocol is used to check is the socket is
really an udplite one, avoiding some cache misses per
packet and improving the performance under udp_flood with
small packet up to 10%.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 include/linux/udp.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/udp.h b/include/linux/udp.h
index c0f5308..6cb4061 100644
--- a/include/linux/udp.h
+++ b/include/linux/udp.h
@@ -115,6 +115,6 @@ static inline bool udp_get_no_check6_rx(struct sock *sk)
 #define udp_portaddr_for_each_entry_rcu(__sk, list) \
 	hlist_for_each_entry_rcu(__sk, list, __sk_common.skc_portaddr_node)
 
-#define IS_UDPLITE(__sk) (udp_sk(__sk)->pcflag)
+#define IS_UDPLITE(__sk) (__sk->sk_protocol == IPPROTO_UDPLITE)
 
 #endif	/* _LINUX_UDP_H */
-- 
2.9.3

Re: [PATCH net-next] udp: use sk_protocol instead of pcflag to detect udplite sockets

[Eric Dumazet]

On Fri, 2017-03-31 at 11:47 +0200, Paolo Abeni wrote:
> In the udp_sock struct, the 'forward_deficit' and 'pcflag' fields
> share the same cacheline. While the first is dirtied by
> udp_recvmsg, the latter is read, possibly several times, by the
> bottom half processing to discriminate between udp and udplite
> sockets.
> 
> With this patch, sk->sk_protocol is used to check is the socket is
> really an udplite one, avoiding some cache misses per
> packet and improving the performance under udp_flood with
> small packet up to 10%.
> 
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  include/linux/udp.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/udp.h b/include/linux/udp.h
> index c0f5308..6cb4061 100644
> --- a/include/linux/udp.h
> +++ b/include/linux/udp.h
> @@ -115,6 +115,6 @@ static inline bool udp_get_no_check6_rx(struct sock *sk)
>  #define udp_portaddr_for_each_entry_rcu(__sk, list) \
>  	hlist_for_each_entry_rcu(__sk, list, __sk_common.skc_portaddr_node)
>  
> -#define IS_UDPLITE(__sk) (udp_sk(__sk)->pcflag)
> +#define IS_UDPLITE(__sk) (__sk->sk_protocol == IPPROTO_UDPLITE)
>  
>  #endif	/* _LINUX_UDP_H */



I am pretty sure we agreed in the past that forward_deficit would need
to be placed on a cache line of its own. Somehow we manage to not
implement this properly.

What about other fields like encap_rcv, encap_destroy, gro_receive,
gro_complete. They really should have the same false sharing issue.

Proper fix is :

 include/linux/udp.h |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/linux/udp.h b/include/linux/udp.h
index c0f530809d1f3db7323e51a52224eb49d8f97da0..bdfd11823c12f6c08dc6b2adb247b8e0a7f2e5d5 100644
--- a/include/linux/udp.h
+++ b/include/linux/udp.h
@@ -80,8 +80,10 @@ struct udp_sock {
 						struct sk_buff *skb,
 						int nhoff);
 
-	/* This field is dirtied by udp_recvmsg() */
-	int		forward_deficit;
+	/* This field is dirtied by udp_recvmsg().
+	 * Make sure it wont share a cache line with prior fields.
+	 */
+	int		forward_deficit ____cacheline_aligned_in_smp;
 };
 
 static inline struct udp_sock *udp_sk(const struct sock *sk)

Re: [PATCH net-next] udp: use sk_protocol instead of pcflag to detect udplite sockets

[Paolo Abeni]

On Fri, 2017-03-31 at 06:25 -0700, Eric Dumazet wrote:
> On Fri, 2017-03-31 at 11:47 +0200, Paolo Abeni wrote:
> > In the udp_sock struct, the 'forward_deficit' and 'pcflag' fields
> > share the same cacheline. While the first is dirtied by
> > udp_recvmsg, the latter is read, possibly several times, by the
> > bottom half processing to discriminate between udp and udplite
> > sockets.
> > 
> > With this patch, sk->sk_protocol is used to check is the socket is
> > really an udplite one, avoiding some cache misses per
> > packet and improving the performance under udp_flood with
> > small packet up to 10%.
> > 
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> >  include/linux/udp.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/udp.h b/include/linux/udp.h
> > index c0f5308..6cb4061 100644
> > --- a/include/linux/udp.h
> > +++ b/include/linux/udp.h
> > @@ -115,6 +115,6 @@ static inline bool udp_get_no_check6_rx(struct sock *sk)
> >  #define udp_portaddr_for_each_entry_rcu(__sk, list) \
> >  	hlist_for_each_entry_rcu(__sk, list, __sk_common.skc_portaddr_node)
> >  
> > -#define IS_UDPLITE(__sk) (udp_sk(__sk)->pcflag)
> > +#define IS_UDPLITE(__sk) (__sk->sk_protocol == IPPROTO_UDPLITE)
> >  
> >  #endif	/* _LINUX_UDP_H */
> 
> 
> 
> I am pretty sure we agreed in the past that forward_deficit would need
> to be placed on a cache line of its own. Somehow we manage to not
> implement this properly.
> 
> What about other fields like encap_rcv, encap_destroy, gro_receive,
> gro_complete. They really should have the same false sharing issue.

I did the above to avoid increasing the udp_sock struct size; this will
costs more than a whole cacheline.

I did not hit others false sharing issues because:
- gro_receive/gro_complete are touched only for packets coming from 
devices with udp tunnel offload enabled, that hit the tunnel offload
path on the nic; such packets will most probably land in the udp tunnel
 and will not use 'forward_deficit'
- encap_destroy is touched only socket shutdown
- encap_rcv is protected by the 'udp_encap_needed' static key

I think this latter is problematic, so I'm ok with the patch you
suggested.

The above change could still make sense, the udp code is already
checking for udplite sockets with either pcflag and protocol;
testing always the same data will make the code more cleaner.

Paolo

Re: [PATCH net-next] udp: use sk_protocol instead of pcflag to detect udplite sockets

[Eric Dumazet]

On Fri, 2017-03-31 at 16:33 +0200, Paolo Abeni wrote:

> I did the above to avoid increasing the udp_sock struct size; this will
> costs more than a whole cacheline.

Yes, but who cares :)

Also note that we discussed about having a secondary receive queue in
the future, to decouple the fact that producers/consumer have to grab a
contended spinlock for every enqueued and dequeued packet.

With a secondary queue, the consumer can transfer one queue into another
in one batch.

Or simply use ptr_ring / skb_array now these infras are available thanks
to Michael.

So we will likely increase UDP socket size in a near future...

> 
> I did not hit others false sharing issues because:
> - gro_receive/gro_complete are touched only for packets coming from 
> devices with udp tunnel offload enabled, that hit the tunnel offload
> path on the nic; such packets will most probably land in the udp tunnel
>  and will not use 'forward_deficit'


> - encap_destroy is touched only socket shutdown
> - encap_rcv is protected by the 'udp_encap_needed' static key
> 
> I think this latter is problematic, so I'm ok with the patch you
> suggested.
> 
> The above change could still make sense, the udp code is already
> checking for udplite sockets with either pcflag and protocol;
> testing always the same data will make the code more cleaner.

Where are we testing sk->sk_prototocol in receive path ?

Thanks Paolo !

Re: [PATCH net-next] udp: use sk_protocol instead of pcflag to detect udplite sockets

[Paolo Abeni]

On Fri, 2017-03-31 at 08:09 -0700, Eric Dumazet wrote:
> On Fri, 2017-03-31 at 16:33 +0200, Paolo Abeni wrote:
> 
> > I did the above to avoid increasing the udp_sock struct size; this will
> > costs more than a whole cacheline.
> 
> Yes, but who cares :)
> 
> Also note that we discussed about having a secondary receive queue in
> the future, to decouple the fact that producers/consumer have to grab a
> contended spinlock for every enqueued and dequeued packet.
> 
> With a secondary queue, the consumer can transfer one queue into another
> in one batch.
> 
> Or simply use ptr_ring / skb_array now these infras are available thanks
> to Michael.
> 
> So we will likely increase UDP socket size in a near future...
> 
> > 
> > I did not hit others false sharing issues because:
> > - gro_receive/gro_complete are touched only for packets coming from 
> > devices with udp tunnel offload enabled, that hit the tunnel offload
> > path on the nic; such packets will most probably land in the udp tunnel
> >  and will not use 'forward_deficit'
> 
> 
> > - encap_destroy is touched only socket shutdown
> > - encap_rcv is protected by the 'udp_encap_needed' static key
> > 
> > I think this latter is problematic, so I'm ok with the patch you
> > suggested.
> > 
> > The above change could still make sense, the udp code is already
> > checking for udplite sockets with either pcflag and protocol;
> > testing always the same data will make the code more cleaner.
> 
> Where are we testing sk->sk_prototocol in receive path ?

Sorry, I was ambiguous: sk->sk_protocol is not used yet; before the
socket lockup, __udp4_lib_rcv() and __udp6_lib_rcv() use the protocol
number provided by the caller to properly account udp vs udplite stats.

Cheers,

Paolo

RE: [PATCH net-next] udp: use sk_protocol instead of pcflag to detect udplite sockets

[David Laight]

From: Eric Dumazet
> Sent: 31 March 2017 14:25
> On Fri, 2017-03-31 at 11:47 +0200, Paolo Abeni wrote:
> > In the udp_sock struct, the 'forward_deficit' and 'pcflag' fields
> > share the same cacheline. While the first is dirtied by
> > udp_recvmsg, the latter is read, possibly several times, by the
> > bottom half processing to discriminate between udp and udplite
> > sockets.
> >
> > With this patch, sk->sk_protocol is used to check is the socket is
> > really an udplite one, avoiding some cache misses per
> > packet and improving the performance under udp_flood with
> > small packet up to 10%.
...
> I am pretty sure we agreed in the past that forward_deficit would need
> to be placed on a cache line of its own. Somehow we manage to not
> implement this properly.
> 
> What about other fields like encap_rcv, encap_destroy, gro_receive,
> gro_complete. They really should have the same false sharing issue.
> 
> Proper fix is :
...
> -	/* This field is dirtied by udp_recvmsg() */
> -	int		forward_deficit;
> +	/* This field is dirtied by udp_recvmsg().
> +	 * Make sure it wont share a cache line with prior fields.
> +	 */
> +	int		forward_deficit ____cacheline_aligned_in_smp;

Is that really sensible on systems with large cache lines?

	David

Re: [PATCH net-next] udp: use sk_protocol instead of pcflag to detect udplite sockets

[Eric Dumazet]

On Fri, 2017-03-31 at 15:03 +0000, David Laight wrote:

> Is that really sensible on systems with large cache lines?

Yes it is.

We mostly do our perf analysis on x86, and it turns out that linux
networking on PowerPC is not great because of this.

struct dst_entry is showing problems, simply because we aligned __refcnt
on 64 byte boundary, which is not enough on PowerPC.

With bigger cache lines, you have bigger chances of false sharing.

Re: [PATCH net-next] udp: use sk_protocol instead of pcflag to detect udplite sockets

[David Miller]

From: Paolo Abeni <pabeni@redhat.com>
Date: Fri, 31 Mar 2017 11:47:39 +0200

> In the udp_sock struct, the 'forward_deficit' and 'pcflag' fields
> share the same cacheline. While the first is dirtied by
> udp_recvmsg, the latter is read, possibly several times, by the
> bottom half processing to discriminate between udp and udplite
> sockets.
> 
> With this patch, sk->sk_protocol is used to check is the socket is
> really an udplite one, avoiding some cache misses per
> packet and improving the performance under udp_flood with
> small packet up to 10%.
> 
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Applied, thanks.