[PATCH] ipv6: Enable RFS sk_rxhash tracking for ipv6 sockets

[PATCH] ipv6: Enable RFS sk_rxhash tracking for ipv6 sockets

[Neil Horman]

properly record sk_rxhash in ipv6 sockets

Noticed while working on another project that flows to sockets which I had open
on a test systems weren't getting steered properly when I had RFS enabled.
Looking more closely I found that:

1) The affected sockets were all ipv6
2) They weren't getting steered because sk->sk_rxhash was never set from the
incomming skbs on that socket.

This was occuring because there are several points in the IPv4 tcp and udp code
which save the rxhash value when a new connection is established.  Those calls
to sock_rps_save_rxhash were never added to the corresponding ipv6 code paths.
This patch adds those calls.  Tested by myself to properly enable RFS
functionalty on ipv6.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: "David S. Miller" <davem@davemloft.net>
CC: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
CC: "Pekka Savola (ipv6)" <pekkas@netcore.fi>
CC: James Morris <jmorris@namei.org>
CC: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
CC: Patrick McHardy <kaber@trash.net>
---
 net/ipv6/tcp_ipv6.c |    4 +++-
 net/ipv6/udp.c      |    2 ++
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 2b0c186..97917bb 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1621,6 +1621,7 @@ static int tcp_v6_do_rcv(struct sock *sk, struct sk_buff *skb)
 		opt_skb = skb_clone(skb, GFP_ATOMIC);
 
 	if (sk->sk_state == TCP_ESTABLISHED) { /* Fast path */
+		sock_rps_save_rxhash(sk, skb->rxhash);
 		if (tcp_rcv_established(sk, skb, tcp_hdr(skb), skb->len))
 			goto reset;
 		if (opt_skb)
@@ -1648,7 +1649,8 @@ static int tcp_v6_do_rcv(struct sock *sk, struct sk_buff *skb)
 				__kfree_skb(opt_skb);
 			return 0;
 		}
-	}
+	} else
+		sock_rps_save_rxhash(sk, skb->rxhash);
 
 	if (tcp_rcv_state_process(sk, skb, tcp_hdr(skb), skb->len))
 		goto reset;
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index d7037c0..aa3e327 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -505,6 +505,8 @@ int udpv6_queue_rcv_skb(struct sock * sk, struct sk_buff *skb)
 	int rc;
 	int is_udplite = IS_UDPLITE(sk);
 
+	sock_rps_save_rxhash(sk, skb->rxhash);
+
 	if (!xfrm6_policy_check(sk, XFRM_POLICY_IN, skb))
 		goto drop;
 
-- 
1.7.4

Re: [PATCH] ipv6: Enable RFS sk_rxhash tracking for ipv6 sockets

[Eric Dumazet]

Le mercredi 06 avril 2011 à 13:54 -0400, Neil Horman a écrit :

> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index d7037c0..aa3e327 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -505,6 +505,8 @@ int udpv6_queue_rcv_skb(struct sock * sk, struct sk_buff *skb)
>  	int rc;
>  	int is_udplite = IS_UDPLITE(sk);
>  
> +	sock_rps_save_rxhash(sk, skb->rxhash);
> +
>  	if (!xfrm6_policy_check(sk, XFRM_POLICY_IN, skb))
>  		goto drop;
>  

Problem is every packet received might have a different rxhash if
multiple senders are active. 

This can hurt performance (cache misses on RFS table) on a DNS server
workload for example.

This is why we used on ipv4 :

if (inet_sk(sk)->inet_daddr)
	sock_rps_save_rxhash(sk, skb->rxhash);


Only arm RFS on UDP if socket is bound to a given remote peer.

BTW you forgot Tom Herbert. (I added him in CC)

Re: [PATCH] ipv6: Enable RFS sk_rxhash tracking for ipv6 sockets

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 06 Apr 2011 20:07:33 +0200

> This is why we used on ipv4 :
> 
> if (inet_sk(sk)->inet_daddr)
> 	sock_rps_save_rxhash(sk, skb->rxhash);
> 
> 
> Only arm RFS on UDP if socket is bound to a given remote peer.

Agreed, Neil please make this change to your ipv6 code.

Thanks!

Re: [PATCH] ipv6: Enable RFS sk_rxhash tracking for ipv6 sockets

[Eric Dumazet]

Le mercredi 06 avril 2011 à 11:17 -0700, David Miller a écrit :
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Wed, 06 Apr 2011 20:07:33 +0200
> 
> > This is why we used on ipv4 :
> > 
> > if (inet_sk(sk)->inet_daddr)
> > 	sock_rps_save_rxhash(sk, skb->rxhash);
> > 
> > 
> > Only arm RFS on UDP if socket is bound to a given remote peer.
> 
> Agreed, Neil please make this change to your ipv6 code.


BTW, do you guys know if NFS is using RFS right now (if TCP transport is
used) ?

Re: [PATCH] ipv6: Enable RFS sk_rxhash tracking for ipv6 sockets

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 06 Apr 2011 20:23:08 +0200

> Le mercredi 06 avril 2011 à 11:17 -0700, David Miller a écrit :
>> From: Eric Dumazet <eric.dumazet@gmail.com>
>> Date: Wed, 06 Apr 2011 20:07:33 +0200
>> 
>> > This is why we used on ipv4 :
>> > 
>> > if (inet_sk(sk)->inet_daddr)
>> > 	sock_rps_save_rxhash(sk, skb->rxhash);
>> > 
>> > 
>> > Only arm RFS on UDP if socket is bound to a given remote peer.
>> 
>> Agreed, Neil please make this change to your ipv6 code.
> 
> BTW, do you guys know if NFS is using RFS right now (if TCP transport is
> used) ?

It ought to be.  Are you specifically concerned that it might be
missing the rxhash setting calls because of the APIs it uses to
do socket I/O?

The SunRPC layer is a major user of tcp_read_sock(), for example.

But the packet processing will be done via the normal means,
therefore we'll hit tcp_v4_do_rcv() and therefore save the
rxhash.

Re: [PATCH] ipv6: Enable RFS sk_rxhash tracking for ipv6 sockets

[Eric Dumazet]

Le mercredi 06 avril 2011 à 11:27 -0700, David Miller a écrit :
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Wed, 06 Apr 2011 20:23:08 +0200
> 
> > Le mercredi 06 avril 2011 à 11:17 -0700, David Miller a écrit :
> >> From: Eric Dumazet <eric.dumazet@gmail.com>
> >> Date: Wed, 06 Apr 2011 20:07:33 +0200
> >> 
> >> > This is why we used on ipv4 :
> >> > 
> >> > if (inet_sk(sk)->inet_daddr)
> >> > 	sock_rps_save_rxhash(sk, skb->rxhash);
> >> > 
> >> > 
> >> > Only arm RFS on UDP if socket is bound to a given remote peer.
> >> 
> >> Agreed, Neil please make this change to your ipv6 code.
> > 
> > BTW, do you guys know if NFS is using RFS right now (if TCP transport is
> > used) ?
> 
> It ought to be.  Are you specifically concerned that it might be
> missing the rxhash setting calls because of the APIs it uses to
> do socket I/O?
> 
> The SunRPC layer is a major user of tcp_read_sock(), for example.
> 
> But the packet processing will be done via the normal means,
> therefore we'll hit tcp_v4_do_rcv() and therefore save the
> rxhash.

Hmm, are multiple tcp sessions used between NFS client and server (One
per cpu on client ?)

Re: [PATCH] ipv6: Enable RFS sk_rxhash tracking for ipv6 sockets

[Neil Horman]

On Wed, Apr 06, 2011 at 08:37:44PM +0200, Eric Dumazet wrote:
> Le mercredi 06 avril 2011 à 11:27 -0700, David Miller a écrit :
> > From: Eric Dumazet <eric.dumazet@gmail.com>
> > Date: Wed, 06 Apr 2011 20:23:08 +0200
> > 
> > > Le mercredi 06 avril 2011 à 11:17 -0700, David Miller a écrit :
> > >> From: Eric Dumazet <eric.dumazet@gmail.com>
> > >> Date: Wed, 06 Apr 2011 20:07:33 +0200
> > >> 
> > >> > This is why we used on ipv4 :
> > >> > 
> > >> > if (inet_sk(sk)->inet_daddr)
> > >> > 	sock_rps_save_rxhash(sk, skb->rxhash);
> > >> > 
> > >> > 
> > >> > Only arm RFS on UDP if socket is bound to a given remote peer.
> > >> 
> > >> Agreed, Neil please make this change to your ipv6 code.
> > > 
> > > BTW, do you guys know if NFS is using RFS right now (if TCP transport is
> > > used) ?
> > 
> > It ought to be.  Are you specifically concerned that it might be
> > missing the rxhash setting calls because of the APIs it uses to
> > do socket I/O?
> > 
> > The SunRPC layer is a major user of tcp_read_sock(), for example.
> > 
> > But the packet processing will be done via the normal means,
> > therefore we'll hit tcp_v4_do_rcv() and therefore save the
> > rxhash.
> 
> Hmm, are multiple tcp sessions used between NFS client and server (One
> per cpu on client ?)
nope, its all nominally done per mount (i.e. all applications writing to a given
mount point will share the same socket).
Neil

> 
> 
> 
> 

Re: [PATCH] ipv6: Enable RFS sk_rxhash tracking for ipv6 sockets

[Neil Horman]

On Wed, Apr 06, 2011 at 08:23:08PM +0200, Eric Dumazet wrote:
> Le mercredi 06 avril 2011 à 11:17 -0700, David Miller a écrit :
> > From: Eric Dumazet <eric.dumazet@gmail.com>
> > Date: Wed, 06 Apr 2011 20:07:33 +0200
> > 
> > > This is why we used on ipv4 :
> > > 
> > > if (inet_sk(sk)->inet_daddr)
> > > 	sock_rps_save_rxhash(sk, skb->rxhash);
> > > 
> > > 
> > > Only arm RFS on UDP if socket is bound to a given remote peer.
> > 
> > Agreed, Neil please make this change to your ipv6 code.
> 
> 
> BTW, do you guys know if NFS is using RFS right now (if TCP transport is
> used) ?
> 
Thats kind of a tricky question, are you referring to clients or servers?  Both
should be able to use RFS as Dave notes, but since sockets in NFS clients tend to be per
mount, rather than per application (as RFS nominally expects when doing flow
steering), theres likely to be some conflict in where RFS decides to steer
packets for that NFS socket  as different applications on different cpus make
use of the same socket.
Neil

> 
> 
> 

Re: [PATCH] ipv6: Enable RFS sk_rxhash tracking for ipv6 sockets

[Eric Dumazet]

Le mercredi 06 avril 2011 à 14:33 -0400, Neil Horman a écrit :

> Thats kind of a tricky question, are you referring to clients or servers?  Both
> should be able to use RFS as Dave notes, but since sockets in NFS clients tend to be per
> mount, rather than per application (as RFS nominally expects when doing flow
> steering), theres likely to be some conflict in where RFS decides to steer
> packets for that NFS socket  as different applications on different cpus make
> use of the same socket.

Yes, maybe there are some updates to make NFS scale better with
multiqueue NICS [ or RPS/RFS ]

Re: [PATCH] ipv6: Enable RFS sk_rxhash tracking for ipv6 sockets

[Neil Horman]

On Wed, Apr 06, 2011 at 08:07:33PM +0200, Eric Dumazet wrote:
> Le mercredi 06 avril 2011 à 13:54 -0400, Neil Horman a écrit :
> 
> > diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> > index d7037c0..aa3e327 100644
> > --- a/net/ipv6/udp.c
> > +++ b/net/ipv6/udp.c
> > @@ -505,6 +505,8 @@ int udpv6_queue_rcv_skb(struct sock * sk, struct sk_buff *skb)
> >  	int rc;
> >  	int is_udplite = IS_UDPLITE(sk);
> >  
> > +	sock_rps_save_rxhash(sk, skb->rxhash);
> > +
> >  	if (!xfrm6_policy_check(sk, XFRM_POLICY_IN, skb))
> >  		goto drop;
> >  
> 
> Problem is every packet received might have a different rxhash if
> multiple senders are active. 
> 
> This can hurt performance (cache misses on RFS table) on a DNS server
> workload for example.
> 
> This is why we used on ipv4 :
> 
> if (inet_sk(sk)->inet_daddr)
> 	sock_rps_save_rxhash(sk, skb->rxhash);
> 
Ok, I can absolutely do that, I'll respin shortly.  Just to make sure I
understand how to do this, the ipv6 equivalant of inet_daddr is
inet6_sk(sk)->daddr, correct? I ask because the above v4 equivalent is stored in
the sk_common struct, and I want to make sure I'm testing the right data. 

Also, do we have a macro already to see if an ipv6 address is all zeros?  I'm
not finding one, but I'd hate to re-invent the wheel if I'm just missing it.

> 
> Only arm RFS on UDP if socket is bound to a given remote peer.
> 
> BTW you forgot Tom Herbert. (I added him in CC)
Thanks, sorry Tom, Just took the output of get_maintainers.pl without checking
all the results.
Neil

> 
> 
> 
> 

Re: [PATCH] ipv6: Enable RFS sk_rxhash tracking for ipv6 sockets

[Brian Haley]

On 04/06/2011 02:39 PM, Neil Horman wrote: 
> Also, do we have a macro already to see if an ipv6 address is all zeros?  I'm
> not finding one, but I'd hate to re-invent the wheel if I'm just missing it.

ipv6_addr_any() inline in ipv6.h

-Brian

Re: [PATCH] ipv6: Enable RFS sk_rxhash tracking for ipv6 sockets

[Neil Horman]

On Wed, Apr 06, 2011 at 02:53:11PM -0400, Brian Haley wrote:
> On 04/06/2011 02:39 PM, Neil Horman wrote: 
> > Also, do we have a macro already to see if an ipv6 address is all zeros?  I'm
> > not finding one, but I'd hate to re-invent the wheel if I'm just missing it.
> 
> ipv6_addr_any() inline in ipv6.h
> 
Ah thanks!
Neil

> -Brian
> 

[PATCH] ipv6: Enable RFS sk_rxhash tracking for ipv6 sockets (v2)

[Neil Horman]

properly record sk_rxhash in ipv6 sockets (v2)

Noticed while working on another project that flows to sockets which I had open
on a test systems weren't getting steered properly when I had RFS enabled.
Looking more closely I found that:

1) The affected sockets were all ipv6
2) They weren't getting steered because sk->sk_rxhash was never set from the
incomming skbs on that socket.

This was occuring because there are several points in the IPv4 tcp and udp code
which save the rxhash value when a new connection is established.  Those calls
to sock_rps_save_rxhash were never added to the corresponding ipv6 code paths.
This patch adds those calls.  Tested by myself to properly enable RFS
functionalty on ipv6.

Change notes:
v2:
	Filtered UDP to only arm RFS on bound sockets (Eric Dumazet)

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: "David S. Miller" <davem@davemloft.net>
CC: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
CC: "Pekka Savola (ipv6)" <pekkas@netcore.fi>
CC: James Morris <jmorris@namei.org>
CC: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
CC: Patrick McHardy <kaber@trash.net>
CC: Tom Herbert <therbert@google.com>
---
 net/ipv6/tcp_ipv6.c |    4 +++-
 net/ipv6/udp.c      |    3 +++
 2 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 2b0c186..97917bb 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1621,6 +1621,7 @@ static int tcp_v6_do_rcv(struct sock *sk, struct sk_buff *skb)
 		opt_skb = skb_clone(skb, GFP_ATOMIC);
 
 	if (sk->sk_state == TCP_ESTABLISHED) { /* Fast path */
+		sock_rps_save_rxhash(sk, skb->rxhash);
 		if (tcp_rcv_established(sk, skb, tcp_hdr(skb), skb->len))
 			goto reset;
 		if (opt_skb)
@@ -1648,7 +1649,8 @@ static int tcp_v6_do_rcv(struct sock *sk, struct sk_buff *skb)
 				__kfree_skb(opt_skb);
 			return 0;
 		}
-	}
+	} else
+		sock_rps_save_rxhash(sk, skb->rxhash);
 
 	if (tcp_rcv_state_process(sk, skb, tcp_hdr(skb), skb->len))
 		goto reset;
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index d7037c0..15c3774 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -505,6 +505,9 @@ int udpv6_queue_rcv_skb(struct sock * sk, struct sk_buff *skb)
 	int rc;
 	int is_udplite = IS_UDPLITE(sk);
 
+	if (!ipv6_addr_any(&inet6_sk(sk)->daddr))
+		sock_rps_save_rxhash(sk, skb->rxhash);
+
 	if (!xfrm6_policy_check(sk, XFRM_POLICY_IN, skb))
 		goto drop;
 
-- 
1.7.4

Re: [PATCH] ipv6: Enable RFS sk_rxhash tracking for ipv6 sockets (v2)

[David Miller]

From: Neil Horman <nhorman@tuxdriver.com>
Date: Wed,  6 Apr 2011 15:37:27 -0400

> properly record sk_rxhash in ipv6 sockets (v2)
> 
> Noticed while working on another project that flows to sockets which I had open
> on a test systems weren't getting steered properly when I had RFS enabled.
> Looking more closely I found that:
> 
> 1) The affected sockets were all ipv6
> 2) They weren't getting steered because sk->sk_rxhash was never set from the
> incomming skbs on that socket.
> 
> This was occuring because there are several points in the IPv4 tcp and udp code
> which save the rxhash value when a new connection is established.  Those calls
> to sock_rps_save_rxhash were never added to the corresponding ipv6 code paths.
> This patch adds those calls.  Tested by myself to properly enable RFS
> functionalty on ipv6.
> 
> Change notes:
> v2:
> 	Filtered UDP to only arm RFS on bound sockets (Eric Dumazet)
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>

Ok, I've decided to apply this to net-2.6

The current behavior even surprised me, I was pretty sure we had added
the hooks to both ipv4 and ipv6.

Thanks a lot Neil.

Re: [PATCH] ipv6: Enable RFS sk_rxhash tracking for ipv6 sockets (v2)

[Neil Horman]

On Wed, Apr 06, 2011 at 01:07:49PM -0700, David Miller wrote:
> From: Neil Horman <nhorman@tuxdriver.com>
> Date: Wed,  6 Apr 2011 15:37:27 -0400
> 
> > properly record sk_rxhash in ipv6 sockets (v2)
> > 
> > Noticed while working on another project that flows to sockets which I had open
> > on a test systems weren't getting steered properly when I had RFS enabled.
> > Looking more closely I found that:
> > 
> > 1) The affected sockets were all ipv6
> > 2) They weren't getting steered because sk->sk_rxhash was never set from the
> > incomming skbs on that socket.
> > 
> > This was occuring because there are several points in the IPv4 tcp and udp code
> > which save the rxhash value when a new connection is established.  Those calls
> > to sock_rps_save_rxhash were never added to the corresponding ipv6 code paths.
> > This patch adds those calls.  Tested by myself to properly enable RFS
> > functionalty on ipv6.
> > 
> > Change notes:
> > v2:
> > 	Filtered UDP to only arm RFS on bound sockets (Eric Dumazet)
> > 
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> 
> Ok, I've decided to apply this to net-2.6
> 
> The current behavior even surprised me, I was pretty sure we had added
> the hooks to both ipv4 and ipv6.
> 
> Thanks a lot Neil.
> 
Thanks Dave, I was kind of suprised too, but we definately weren't recording the
rxhash. FWIW It doesn't appear to be a regression.  I can't find where in the
history we ever had the hooks.  Perhaps at one point tcp and udp rcv paths were
common between ipv4 and 6 address families, and then we bifurcated without
replicating the sock_rps_save_rxhash calls?

Neil