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

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 <redacted>
CC: "Pekka Savola (ipv6)" <redacted>
CC: James Morris <jmorris@namei.org>
CC: Hideaki YOSHIFUJI <redacted>
CC: Patrick McHardy <redacted>
---
 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

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

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)

From: Eric Dumazet <redacted>
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!

Le mercredi 06 avril 2011 à 11:17 -0700, David Miller a écrit :

From: Eric Dumazet <redacted>
Date: Wed, 06 Apr 2011 20:07:33 +0200

quoted

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

From: Eric Dumazet <redacted>
Date: Wed, 06 Apr 2011 20:23:08 +0200

Le mercredi 06 avril 2011 à 11:17 -0700, David Miller a écrit :

quoted

From: Eric Dumazet <redacted>
Date: Wed, 06 Apr 2011 20:07:33 +0200

quoted

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.

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 :

quoted

From: Eric Dumazet <redacted>
Date: Wed, 06 Apr 2011 20:07:33 +0200

quoted

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

Le mercredi 06 avril 2011 à 11:27 -0700, David Miller a écrit :

From: Eric Dumazet <redacted>
Date: Wed, 06 Apr 2011 20:23:08 +0200

quoted

Le mercredi 06 avril 2011 à 11:17 -0700, David Miller a écrit :

quoted

From: Eric Dumazet <redacted>
Date: Wed, 06 Apr 2011 20:07:33 +0200

quoted

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

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 ]

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 :

quoted

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

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 :

quoted

From: Eric Dumazet <redacted>
Date: Wed, 06 Apr 2011 20:23:08 +0200

quoted

Le mercredi 06 avril 2011 à 11:17 -0700, David Miller a écrit :

quoted

From: Eric Dumazet <redacted>
Date: Wed, 06 Apr 2011 20:07:33 +0200

quoted

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

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

On Wed, Apr 06, 2011 at 02:53:11PM -0400, Brian Haley wrote:

On 04/06/2011 02:39 PM, Neil Horman wrote: 

quoted

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

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 <redacted>
CC: "Pekka Savola (ipv6)" <redacted>
CC: James Morris <jmorris@namei.org>
CC: Hideaki YOSHIFUJI <redacted>
CC: Patrick McHardy <redacted>
CC: Tom Herbert <redacted>
---
 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

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.

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

quoted

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