Performance regression on kernels 3.10 and newer

Yesterday I tripped over a bit of an issue and it seems like we are seeing significant cache thrash on kernels 3.10 and newer when running multiple streams of small packet stress on multiple NUMA nodes for a single NIC.

I did some bisection and found that I was able to trace it back to upstream commit 093162553c33e9479283e107b4431378271c735d (tcp: force a dst refcount when prequeue packet).

Recreating this issue is pretty strait forward. All I did was setup 2 dual socket Xeon systems connected back to back with ixgbe and ran the following script after disabling tcp_autocork on the transmitting system:
for i in seq 0 19
do

    for j in `seq 0 2`
    do
      netperf -H 192.168.10.1 -t TCP_STREAM \
              -l 10 -c -C -T $i,$i -P 0 -- \
              -m 64 -s 64K -D
    done

done

The current net tree as-is will give me about 2Gb/s of data w/ 100% CPU utilization on the receiving system, and with the patch above reverted on that system it gives me about 4Gb/s with only 21% CPU utilization. If I set tcp_low_latency=1 I can get the CPU utilization down to about 12% on the same test with about 4Gb/s of throughput.

I'm still working on determining the exact root cause but it looks to me like there is some significant cache thrash going on in regards to the dst entries.

Below is a quick breakdown of the top CPU users for tcp_low_latency on/off using perf top:

tcp_low_latency = 0

tcp_low_latency = 1
Any input/advice on where I should look or patches to possibly test would be appreciated.

I believe you answered your own question : prequeue mode does not work very well when one host has hundred of active TCP flows to one other.

In real life, applications do not use prequeue, because nobody wants one thread per flow.

Each socket has its own dst now route cache was removed, but if your netperf migrates cpu (and NUMA node), we do not detect the dst should be re-created onto a different NUMA node.

But really, I am not sure we want to care about prequeue, as modern applications uses epoll()/poll()/select() instead of blocking on recvmsg()

I believe you answered your own question : prequeue mode does not work very well when one host has hundred of active TCP flows to one other.

In real life, applications do not use prequeue, because nobody wants one thread per flow.

Each socket has its own dst now route cache was removed, but if your netperf migrates cpu (and NUMA node), we do not detect the dst should be re-created onto a different NUMA node.

Quoted 5 lines · Eric Dumazet

I believe you answered your own question : prequeue mode does not work very well when one host has hundred of active TCP flows to one other.

In real life, applications do not use prequeue, because nobody wants one thread per flow.

Quoted 3 lines

Each socket has its own dst now route cache was removed, but if your netperf migrates cpu (and NUMA node), we do not detect the dst should be re-created onto a different NUMA node.

Presumably, the -T $i,$j option in Alex's netperf command lines will have bound netperf and netserver to a specific CPU where they will have remained.

rick jones

My concern here is that netperf is a standard tool to use for testing network performance, and the kernel default is to run with tcp_low_latency disabled. As such the prequeue is a part of the standard path is it not? If the prequeue isn't really useful anymore should we consider pulling it out of the kernel, or disabling it by making tcp_low_latency the default?

Presumably, the -T $i,$j option in Alex's netperf command lines will have bound netperf and netserver to a specific CPU where they will have remained.

In real life, applications do not use prequeue, because nobody wants one thread per flow.

Each socket has its own dst now route cache was removed, but if your netperf migrates cpu (and NUMA node), we do not detect the dst should be re-created onto a different NUMA node.

From what I can tell the issue is that the patch made it so tcp_prequeue

Are you sure about each socket having it's own DST? Everything I see seems to indicate it is somehow associated with IP.

From: Alexander Duyck <[email protected]>
Date: Thu, 14 Aug 2014 16:16:36 -0700

Quoted 2 lines

Are you sure about each socket having it's own DST? Everything I see seems to indicate it is somehow associated with IP.

Right it should be, unless you have exception entries created by path MTU or redirects.

WRT prequeue, it does the right thing for dumb apps that block in receive. But because it causes the packet to cross domains as it does, we can't do a lot of tricks which we normally can do, and that's why the refcounting on the dst is there now.

Perhaps we can find a clever way to elide that refcount, who knows.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

I don't know if it's the same problem, but I did post a patch back in January that would resolve false sharing of dst->__refcnt and rt_genid in the same cacheline. We could revisit that if it helps.

From: Tom Herbert <[email protected]>
Date: Thu, 14 Aug 2014 16:25:53 -0700

Quoted 3 lines

I don't know if it's the same problem, but I did post a patch back in January that would resolve false sharing of dst->__refcnt and rt_genid in the same cacheline. We could revisit that if it helps.

I think that regardless of what happens in the discussion here, you should repost that patch.

From: Eric Dumazet <[email protected]>

Alexander Duyck reported high false sharing on dst refcount in tcp stack when prequeue is used. prequeue is the mechanism used when a thread is blocked in recvmsg()/read() on the TCP socket.

We already try to use RCU in input path as much as possible, but we were forced to take a refcount on the dst when skb escaped RCU protected region. When/if the user thread runs on different cpu, dst_release() will then touch dst refcount again.

Commit 093162553c33 (tcp: force a dst refcount when prequeue packet) was an example of a race fix.

It turns out the only remaining usage of skb->dst for a packet stored in a TCP socket prequeue is IP early demux.

We can add a logic to detect when IP early demux is probably going to use skb->dst.

In IPv4 stack, we also can drop dst when skb has to be queued into socket backlog. IPv6 needs extra care before doing the same.

Many thanks to Alexander for providing a nice bug report, git bisection, and reproducer.

Signed-off-by Eric Dumazet <[email protected]>

Reported-by Alexander Duyck <[email protected]>

Works fine on IPv4, not on IPv6, I will submit a v2 (with a proper title btw)

Quoted 2 lines · Eric Dumazet

Works fine on IPv4, not on IPv6, I will submit a v2 (with a proper title btw)

Oh well, we first need to fix IPv6 early demux.

Current net-next kernel profile

(quoted diff) +4 -0

+  20.95%      netserver  [kernel.kallsyms]    [k] dst_release     
+  19.33%      netserver  [kernel.kallsyms]    [k] ip6_pol_route.isra.46
+  11.75%      netserver  [kernel.kallsyms]    [k] inet6_sk_rx_dst_set
+   3.72%      netserver  [kernel.kallsyms]    [k] ip6_input_finish

So we repeat over and over calls to inet6_sk_rx_dst_set(), something is surely wrong.

From: Eric Dumazet <[email protected]>

While tracking IPv6 poor performance, I found that IPv6 early demux was broken in recent kernels. perf profiles show inet6_sk_rx_dst_set() being called for every incoming TCP segment :

  20.95%      netserver  [kernel.kallsyms]    [k] dst_release     
  19.33%      netserver  [kernel.kallsyms]    [k] ip6_pol_route
  11.75%      netserver  [kernel.kallsyms]    [k] inet6_sk_rx_dst_set
   3.72%      netserver  [kernel.kallsyms]    [k] ip6_input_finish

Regression came in linux-3.6 with commit 6f3118b571b8 ("ipv6: use net->rt_genid to check dst validity")

When a route found in ip6_pol_route() is cloned (either using rt6_alloc_cow() or rt6_alloc_clone()), copy gets an updated rt6i_genid.

But when original route is selected, we need to refresh its rt6i_genid that could be obsolete. If we do not refresh rt6i_genid, ip6_dst_check() will fail.

Signed-off-by Eric Dumazet <[email protected]>

Fixes 6f3118b571b8 ("ipv6: use net->rt_genid to check dst validity")

This might be broken.

We are dealing here with persistent entries in the ipv6 routine trie.

If you just bump the genid on the next person to look it up, other sockets and cached entities might not have validated the route yet, and now will falsely see the route as valid. We have to ensure that they too drop this route and perform a relookup.

Quoted 9 lines

This might be broken.

We are dealing here with persistent entries in the ipv6 routine trie.

If you just bump the genid on the next person to look it up, other sockets and cached entities might not have validated the route yet, and now will falsely see the route as valid. We have to ensure that they too drop this route and perform a relookup.

I am confused, I thought it was the role of the cookie.

(Ie socket has to store its own cookie to be able to validate a route)

Before 6f3118b571b8 patch, how was this done anyway ?

If persistent routes cannot refresh the genid, then they are useless ?

From: Eric Dumazet <[email protected]>
Date: Sun, 07 Sep 2014 21:18:25 -0700

Quoted 11 lines · David Miller

Quoted 9 lines

This might be broken.

We are dealing here with persistent entries in the ipv6 routine trie.

If you just bump the genid on the next person to look it up, other sockets and cached entities might not have validated the route yet, and now will falsely see the route as valid. We have to ensure that they too drop this route and perform a relookup.

Quoted 7 lines

I am confused, I thought it was the role of the cookie.

(Ie socket has to store its own cookie to be able to validate a route)

Before 6f3118b571b8 patch, how was this done anyway ?

If persistent routes cannot refresh the genid, then they are useless ?

I just speak about the genid aspect.

I understand that cookie (via node->fn_sernum) invalidates the path in the fib_trie, but the genid protects against other circumstances (matching IPSEC rule, f.e.)

You have to make sure all other sockets did a full route lookup (including IPSEC) before you can safely adjust the genid.

I could be wrong, recheck my analysis please :-)

Quoted 31 lines

From: Eric Dumazet <[email protected]>
Date: Sun, 07 Sep 2014 21:18:25 -0700

Quoted 11 lines · David Miller

Quoted 9 lines

This might be broken.

We are dealing here with persistent entries in the ipv6 routine trie.

If you just bump the genid on the next person to look it up, other sockets and cached entities might not have validated the route yet, and now will falsely see the route as valid. We have to ensure that they too drop this route and perform a relookup.

Quoted 7 lines

I am confused, I thought it was the role of the cookie.

(Ie socket has to store its own cookie to be able to validate a route)

Before 6f3118b571b8 patch, how was this done anyway ?

If persistent routes cannot refresh the genid, then they are useless ?

I just speak about the genid aspect.

I understand that cookie (via node->fn_sernum) invalidates the path in the fib_trie, but the genid protects against other circumstances (matching IPSEC rule, f.e.)

You have to make sure all other sockets did a full route lookup (including IPSEC) before you can safely adjust the genid.

Quoted 1 line

I could be wrong, recheck my analysis please :-)

So this whole genid protection can not work, unless we make sure a socket cannot share a route with another socket.

This means we have to clone all routes.

From: Eric Dumazet <[email protected]>
Date: Sun, 07 Sep 2014 21:43:54 -0700

Quoted 37 lines · David Miller

Quoted 24 lines

From: Eric Dumazet <[email protected]>
Date: Sun, 07 Sep 2014 21:18:25 -0700

Quoted 11 lines · David Miller

Quoted 9 lines

This might be broken.

We are dealing here with persistent entries in the ipv6 routine trie.

If you just bump the genid on the next person to look it up, other sockets and cached entities might not have validated the route yet, and now will falsely see the route as valid. We have to ensure that they too drop this route and perform a relookup.

Quoted 7 lines

I am confused, I thought it was the role of the cookie.

(Ie socket has to store its own cookie to be able to validate a route)

Before 6f3118b571b8 patch, how was this done anyway ?

If persistent routes cannot refresh the genid, then they are useless ?

I just speak about the genid aspect.

Quoted 8 lines

I understand that cookie (via node->fn_sernum) invalidates the path in the fib_trie, but the genid protects against other circumstances (matching IPSEC rule, f.e.)

You have to make sure all other sockets did a full route lookup (including IPSEC) before you can safely adjust the genid.

I could be wrong, recheck my analysis please :-)

So this whole genid protection can not work, unless we make sure a socket cannot share a route with another socket.

Quoted 1 line

This means we have to clone all routes.

I'm willing to revert the change in question if you think that's the sanest way forward.

The bug fix for more obscure use cases (IPSEC) if pointless if it breaks more common things (TCP input route caching).

Quoted 42 lines · David Miller

From: Eric Dumazet <[email protected]>
Date: Sun, 07 Sep 2014 21:43:54 -0700

Quoted 38 lines · David Miller

Quoted 24 lines

From: Eric Dumazet <[email protected]>
Date: Sun, 07 Sep 2014 21:18:25 -0700

Quoted 11 lines · David Miller

Quoted 9 lines

This might be broken.

We are dealing here with persistent entries in the ipv6 routine trie.

If you just bump the genid on the next person to look it up, other sockets and cached entities might not have validated the route yet, and now will falsely see the route as valid. We have to ensure that they too drop this route and perform a relookup.

Quoted 7 lines

I am confused, I thought it was the role of the cookie.

(Ie socket has to store its own cookie to be able to validate a route)

Before 6f3118b571b8 patch, how was this done anyway ?

If persistent routes cannot refresh the genid, then they are useless ?

I just speak about the genid aspect.

Quoted 8 lines

I understand that cookie (via node->fn_sernum) invalidates the path in the fib_trie, but the genid protects against other circumstances (matching IPSEC rule, f.e.)

You have to make sure all other sockets did a full route lookup (including IPSEC) before you can safely adjust the genid.

I could be wrong, recheck my analysis please :-)

So this whole genid protection can not work, unless we make sure a socket cannot share a route with another socket.

This means we have to clone all routes.

Quoted 5 lines

I'm willing to revert the change in question if you think that's the sanest way forward.

The bug fix for more obscure use cases (IPSEC) if pointless if it breaks more common things (TCP input route caching).

Lets wait for Nicolas and/or Hannes input, they might have some ideas...

Quoted 49 lines · Eric Dumazet

Quoted 47 lines · David Miller

From: Eric Dumazet <[email protected]>
Date: Sun, 07 Sep 2014 21:43:54 -0700

Quoted 32 lines · David Miller

Quoted 31 lines

From: Eric Dumazet <[email protected]>
Date: Sun, 07 Sep 2014 21:18:25 -0700

Quoted 11 lines · David Miller

Quoted 9 lines

This might be broken.

We are dealing here with persistent entries in the ipv6 routine trie.

If you just bump the genid on the next person to look it up, other sockets and cached entities might not have validated the route yet, and now will falsely see the route as valid. We have to ensure that they too drop this route and perform a relookup.

Quoted 7 lines

I am confused, I thought it was the role of the cookie.

(Ie socket has to store its own cookie to be able to validate a route)

Before 6f3118b571b8 patch, how was this done anyway ?

If persistent routes cannot refresh the genid, then they are useless ?

I just speak about the genid aspect.

I understand that cookie (via node->fn_sernum) invalidates the path in the fib_trie, but the genid protects against other circumstances (matching IPSEC rule, f.e.)

You have to make sure all other sockets did a full route lookup (including IPSEC) before you can safely adjust the genid.

Quoted 6 lines

Quoted 1 line

I could be wrong, recheck my analysis please :-)

So this whole genid protection can not work, unless we make sure a socket cannot share a route with another socket.

This means we have to clone all routes.

I'm willing to revert the change in question if you think that's the sanest way forward.

The bug fix for more obscure use cases (IPSEC) if pointless if it breaks more common things (TCP input route caching).

Quoted 1 line

Lets wait for Nicolas and/or Hannes input, they might have some ideas...

The initial problem was in SCTP. Here is the thread after the v1 patch:
http://patchwork.ozlabs.org/patch/182235/

Before the patch, SCTP stored the IPv6 route in its cache and if an IPsec rules was inserted after that operation, SCTP never invalidated the cached route to use a new xfrm route.

Quoted 52 lines · Nicolas Dichtel

Quoted 50 lines · Eric Dumazet

Quoted 40 lines · David Miller

From: Eric Dumazet <[email protected]>
Date: Sun, 07 Sep 2014 21:43:54 -0700

Quoted 37 lines · David Miller

Quoted 24 lines

From: Eric Dumazet <[email protected]>
Date: Sun, 07 Sep 2014 21:18:25 -0700

Quoted 11 lines · David Miller

Quoted 9 lines

This might be broken.

We are dealing here with persistent entries in the ipv6 routine trie.

If you just bump the genid on the next person to look it up, other sockets and cached entities might not have validated the route yet, and now will falsely see the route as valid. We have to ensure that they too drop this route and perform a relookup.

Quoted 7 lines

I am confused, I thought it was the role of the cookie.

(Ie socket has to store its own cookie to be able to validate a route)

Before 6f3118b571b8 patch, how was this done anyway ?

If persistent routes cannot refresh the genid, then they are useless ?

I just speak about the genid aspect.

Quoted 8 lines

I understand that cookie (via node->fn_sernum) invalidates the path in the fib_trie, but the genid protects against other circumstances (matching IPSEC rule, f.e.)

You have to make sure all other sockets did a full route lookup (including IPSEC) before you can safely adjust the genid.

I could be wrong, recheck my analysis please :-)

So this whole genid protection can not work, unless we make sure a socket cannot share a route with another socket.

Quoted 7 lines

Quoted 1 line

This means we have to clone all routes.

I'm willing to revert the change in question if you think that's the sanest way forward.

The bug fix for more obscure use cases (IPSEC) if pointless if it breaks more common things (TCP input route caching).

Lets wait for Nicolas and/or Hannes input, they might have some ideas...

Quoted 6 lines

The initial problem was in SCTP. Here is the thread after the v1 patch:
http://patchwork.ozlabs.org/patch/182235/

Before the patch, SCTP stored the IPv6 route in its cache and if an IPsec rules was inserted after that operation, SCTP never invalidated the cached route to use a new xfrm route.

This thread mentions output route.

The problem I currently have with IPv6 early demux is for input routes.

Quoted 42 lines · David Miller

From: Eric Dumazet <[email protected]>
Date: Sun, 07 Sep 2014 21:43:54 -0700

Quoted 38 lines · David Miller

Quoted 24 lines

From: Eric Dumazet <[email protected]>
Date: Sun, 07 Sep 2014 21:18:25 -0700

Quoted 11 lines · David Miller

Quoted 9 lines

This might be broken.

We are dealing here with persistent entries in the ipv6 routine trie.

If you just bump the genid on the next person to look it up, other sockets and cached entities might not have validated the route yet, and now will falsely see the route as valid. We have to ensure that they too drop this route and perform a relookup.

Quoted 7 lines

I am confused, I thought it was the role of the cookie.

(Ie socket has to store its own cookie to be able to validate a route)

Before 6f3118b571b8 patch, how was this done anyway ?

If persistent routes cannot refresh the genid, then they are useless ?

I just speak about the genid aspect.

Quoted 8 lines

I understand that cookie (via node->fn_sernum) invalidates the path in the fib_trie, but the genid protects against other circumstances (matching IPSEC rule, f.e.)

You have to make sure all other sockets did a full route lookup (including IPSEC) before you can safely adjust the genid.

I could be wrong, recheck my analysis please :-)

So this whole genid protection can not work, unless we make sure a socket cannot share a route with another socket.

This means we have to clone all routes.

Quoted 5 lines

I'm willing to revert the change in question if you think that's the sanest way forward.

The bug fix for more obscure use cases (IPSEC) if pointless if it breaks more common things (TCP input route caching).

Lets wait for Nicolas and/or Hannes input, they might have some ideas...

--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 42 lines · David Miller

From: Eric Dumazet <[email protected]>
Date: Sun, 07 Sep 2014 21:43:54 -0700

Quoted 38 lines · David Miller

Quoted 24 lines

From: Eric Dumazet <[email protected]>
Date: Sun, 07 Sep 2014 21:18:25 -0700

Quoted 11 lines · David Miller

Quoted 9 lines

This might be broken.

We are dealing here with persistent entries in the ipv6 routine trie.

If you just bump the genid on the next person to look it up, other sockets and cached entities might not have validated the route yet, and now will falsely see the route as valid. We have to ensure that they too drop this route and perform a relookup.

Quoted 7 lines

I am confused, I thought it was the role of the cookie.

(Ie socket has to store its own cookie to be able to validate a route)

Before 6f3118b571b8 patch, how was this done anyway ?

If persistent routes cannot refresh the genid, then they are useless ?

I just speak about the genid aspect.

Quoted 8 lines

I understand that cookie (via node->fn_sernum) invalidates the path in the fib_trie, but the genid protects against other circumstances (matching IPSEC rule, f.e.)

You have to make sure all other sockets did a full route lookup (including IPSEC) before you can safely adjust the genid.

I could be wrong, recheck my analysis please :-)

So this whole genid protection can not work, unless we make sure a socket cannot share a route with another socket.

This means we have to clone all routes.

Quoted 5 lines

I'm willing to revert the change in question if you think that's the sanest way forward.

The bug fix for more obscure use cases (IPSEC) if pointless if it breaks more common things (TCP input route caching).

Lets wait for Nicolas and/or Hannes input, they might have some ideas...

Some special routes will never be cloned in IPv6. Those are mainly DST_HOST routes without RTF_NONEXTHOP or RTF_GATEWAY flags, thus mostly routes handling the input path.

rt_genid depends on rt6_info clones being removed from the trie and recreated with current rt6i_genid, thus bumping the genid would invalidate all routes cached in the sockets and relookup will recreate them. But in case a non-cloned route ends up in the per-socket cache, it will never be recreated, thus will never get a current rt_genid.

After the rt_genid is incremented for the first time all those routes will always get invalidated by ip6_dst_check on the next check, thus making early socket demultiplexing absolutely pointless for IPv6. It is not possible to solve this with rt6i_genid, thus remove it.

In case we need to force the sockets to relookup the routes we now increase the fn_sernum on all fibnodes in the routing tree. This is a costly operation but should only happen if we have major routing/policy changes in the kernel (e.g. manual route adding/removal, xfrm policy changes). Also this patch optimized the walk over the trie a bit, we don't touch every rt6_info but only touch the fib6_nodes.

Thanks to Eric Dumazet who noticed this problem.

Fixes 6f3118b571b8 ("ipv6: use net->rt_genid to check dst validity")

Cc Eric Dumazet <[email protected]>

Cc Nicolas Dichtel <[email protected]>

Signed-off-by Hannes Frederic Sowa <[email protected]>

---

Notes:
I based this patch on net-next, because it "only" fixes a performance problem so far and want it to be a bit more exposed to testing before a possible submission to stable.

 include/net/ip6_fib.h       |  5 ++--
 include/net/net_namespace.h |  4 +++-
 net/core/dst.c              |  8 +++++++
 net/ipv6/ip6_fib.c          | 57 ++++++++++++++++++++++++++++++++++++---------
 net/ipv6/route.c            |  4 ----
 5 files changed, 59 insertions(+), 19 deletions(-)

include/net/ip6_fib.h +2 -3

index 9bcb220..a5e09b8 100644
@@ -119,8 +119,6 @@ struct rt6_info {
 	struct inet6_dev		*rt6i_idev;
 	unsigned long			_rt6i_peer;
 
-	u32				rt6i_genid;
-
 	/* more non-fragment space at head required */
 	unsigned short			rt6i_nfheader_len;
 

@@ -281,7 +279,8 @@ struct fib6_node *fib6_locate(struct fib6_node *root,
 			      const struct in6_addr *daddr, int dst_len,
 			      const struct in6_addr *saddr, int src_len);
 
-void fib6_clean_all(struct net *net, int (*func)(struct rt6_info *, void *arg),
+void fib6_clean_all(struct net *net,
+		    int (*rt_visit)(struct rt6_info *, void *arg),
 		    void *arg);
 
 int fib6_add(struct fib6_node *root, struct rt6_info *rt, struct nl_info *info,

include/net/net_namespace.h +3 -1

index 361d260..e0b0476 100644
@@ -358,9 +358,11 @@ static inline int rt_genid_ipv6(struct net *net)
 	return atomic_read(&net->ipv6.rt_genid);
 }
 
+extern void (*__rt_genid_bump_ipv6)(struct net *net);
 static inline void rt_genid_bump_ipv6(struct net *net)
 {
-	atomic_inc(&net->ipv6.rt_genid);
+	if (__rt_genid_bump_ipv6)
+		__rt_genid_bump_ipv6(net);
 }
 #else
 static inline int rt_genid_ipv6(struct net *net)

net/core/dst.c +8 -0

index a028409..995183b 100644
@@ -23,6 +23,14 @@
 
 #include <net/dst.h>
 
+/* as soon as the ipv6 module registers, this function is used to
+ * force all cached routing lookups to relookup
+ */
+#if IS_ENABLED(CONFIG_IPV6)
+void (*__rt_genid_bump_ipv6)(struct net *net);
+EXPORT_SYMBOL(__rt_genid_bump_ipv6);
+#endif
+
 /*
  * Theory of operations:
  * 1) We use a list, protected by a spinlock, to add

net/ipv6/ip6_fib.c +46 -11

index 76b7f5e..d23b76f 100644
@@ -106,10 +106,15 @@ static inline void fib6_walker_unlink(struct fib6_walker_t *w)
 }
 static __inline__ u32 fib6_new_sernum(void)
 {
-	u32 n = ++rt_sernum;
-	if ((__s32)n <= 0)
-		rt_sernum = n = 1;
-	return n;
+	u32 new, old;
+
+	do {
+		old = ACCESS_ONCE(rt_sernum);
+		new = old + 1;
+		if ((s32)new <= 0)
+			new = 1;
+	} while (cmpxchg(&rt_sernum, old, new) != old);
+	return new;
 }
 
 /*

@@ -1553,25 +1558,28 @@ static int fib6_clean_node(struct fib6_walker_t *w)
  */
 
 static void fib6_clean_tree(struct net *net, struct fib6_node *root,
-			    int (*func)(struct rt6_info *, void *arg),
+			    int (*node_visit)(struct fib6_walker_t *),
+			    int (*rt_visit)(struct rt6_info *, void *arg),
 			    int prune, void *arg)
 {
 	struct fib6_cleaner_t c;
 
 	c.w.root = root;
-	c.w.func = fib6_clean_node;
+	c.w.func = node_visit;
 	c.w.prune = prune;
 	c.w.count = 0;
 	c.w.skip = 0;
-	c.func = func;
+	c.func = rt_visit;
 	c.arg = arg;
 	c.net = net;
 
 	fib6_walk(&c.w);
 }
 
-void fib6_clean_all(struct net *net, int (*func)(struct rt6_info *, void *arg),
-		    void *arg)
+static void __fib6_clean_all(struct net *net,
+			     int (*node_visit)(struct fib6_walker_t *),
+			     int (*rt_visit)(struct rt6_info *, void *arg),
+			     void *arg)
 {
 	struct fib6_table *table;
 	struct hlist_head *head;

@@ -1583,13 +1591,20 @@ void fib6_clean_all(struct net *net, int (*func)(struct rt6_info *, void *arg),
 		hlist_for_each_entry_rcu(table, head, tb6_hlist) {
 			write_lock_bh(&table->tb6_lock);
 			fib6_clean_tree(net, &table->tb6_root,
-					func, 0, arg);
+					node_visit, rt_visit, 0, arg);
 			write_unlock_bh(&table->tb6_lock);
 		}
 	}
 	rcu_read_unlock();
 }
 
+void fib6_clean_all(struct net *net,
+		    int (*rt_visit)(struct rt6_info *, void *arg),
+		    void *arg)
+{
+	__fib6_clean_all(net, fib6_clean_node, rt_visit, arg);
+}
+
 static int fib6_prune_clone(struct rt6_info *rt, void *arg)
 {
 	if (rt->rt6i_flags & RTF_CACHE) {

@@ -1602,7 +1617,25 @@ static int fib6_prune_clone(struct rt6_info *rt, void *arg)
 
 static void fib6_prune_clones(struct net *net, struct fib6_node *fn)
 {
-	fib6_clean_tree(net, fn, fib6_prune_clone, 1, NULL);
+	fib6_clean_tree(net, fn, fib6_clean_node, fib6_prune_clone, 1, NULL);
+}
+
+static int fib6_upd_sernum(struct fib6_walker_t *w)
+{
+	struct fib6_cleaner_t *c = container_of(w, struct fib6_cleaner_t, w);
+	int *sernum = c->arg;
+
+	w->node->fn_sernum = *sernum;
+	w->leaf = NULL;
+	return 0;
+}
+
+static void fib6_genid_bump(struct net *net)
+{
+	int sernum;
+
+	sernum = fib6_new_sernum();
+	__fib6_clean_all(net, fib6_upd_sernum, NULL, &sernum);
 }
 
 /*

@@ -1788,6 +1821,8 @@ int __init fib6_init(void)
 			      NULL);
 	if (ret)
 		goto out_unregister_subsys;
+
+	__rt_genid_bump_ipv6 = fib6_genid_bump;
 out:
 	return ret;
 

net/ipv6/route.c +0 -4

index f74b041..a318dd89 100644
@@ -314,7 +314,6 @@ static inline struct rt6_info *ip6_dst_alloc(struct net *net,
 
 		memset(dst + 1, 0, sizeof(*rt) - sizeof(*dst));
 		rt6_init_peer(rt, table ? &table->tb6_peers : net->ipv6.peers);
-		rt->rt6i_genid = rt_genid_ipv6(net);
 		INIT_LIST_HEAD(&rt->rt6i_siblings);
 	}
 	return rt;

@@ -1096,9 +1095,6 @@ static struct dst_entry *ip6_dst_check(struct dst_entry *dst, u32 cookie)
 	 * DST_OBSOLETE_FORCE_CHK which forces validation calls down
 	 * into this function always.
 	 */
-	if (rt->rt6i_genid != rt_genid_ipv6(dev_net(rt->dst.dev)))
-		return NULL;
-
 	if (!rt->rt6i_node || (rt->rt6i_node->fn_sernum != cookie))
 		return NULL;
 

Quoted 15 lines · Hannes Frederic Sowa

Some special routes will never be cloned in IPv6. Those are mainly DST_HOST routes without RTF_NONEXTHOP or RTF_GATEWAY flags, thus mostly routes handling the input path.

rt_genid depends on rt6_info clones being removed from the trie and recreated with current rt6i_genid, thus bumping the genid would invalidate all routes cached in the sockets and relookup will recreate them. But in case a non-cloned route ends up in the per-socket cache, it will never be recreated, thus will never get a current rt_genid.

After the rt_genid is incremented for the first time all those routes will always get invalidated by ip6_dst_check on the next check, thus making early socket demultiplexing absolutely pointless for IPv6. It is not possible to solve this with rt6i_genid, thus remove it.

Quoted 8 lines

In case we need to force the sockets to relookup the routes we now increase the fn_sernum on all fibnodes in the routing tree. This is a costly operation but should only happen if we have major routing/policy changes in the kernel (e.g. manual route adding/removal, xfrm policy changes). Also this patch optimized the walk over the trie a bit, we don't touch every rt6_info but only touch the fib6_nodes.

Thanks to Eric Dumazet who noticed this problem.

I also noticed the ipv6_ifa_notify() is called a lot with even being
0. This will only trigger rtnl notification, but would not trigger any routing table changes, but would trigger a call to bump the gen_id which now would perform a rather expensive clean-table operation.

In particular the loop in manage_tempaddrs() is very scary as it can bump the rev multiples times.

I think it the genid bump in __ipv6_ifa_notify() should only happen if there was an actual address change.

In case we need to force the sockets to relookup the routes we now increase the fn_sernum on all fibnodes in the routing tree. This is a costly operation but should only happen if we have major routing/policy changes in the kernel (e.g. manual route adding/removal, xfrm policy changes).

From: Hannes Frederic Sowa <[email protected]>
Date: Wed, 10 Sep 2014 11:31:28 +0200

Quoted 5 lines

In case we need to force the sockets to relookup the routes we now increase the fn_sernum on all fibnodes in the routing tree. This is a costly operation but should only happen if we have major routing/policy changes in the kernel (e.g. manual route adding/removal, xfrm policy changes).

Core routers can update thousands of route updates per second, and they do this via what you refer to as "manual route adding/removal".

I don't think we want to put such a scalability problem into the tree.

There has to be a lightweight way to address this.

Quoted 11 lines · David Miller

From: Hannes Frederic Sowa <[email protected]>
Date: Wed, 10 Sep 2014 11:31:28 +0200

Quoted 5 lines

In case we need to force the sockets to relookup the routes we now increase the fn_sernum on all fibnodes in the routing tree. This is a costly operation but should only happen if we have major routing/policy changes in the kernel (e.g. manual route adding/removal, xfrm policy changes).

Core routers can update thousands of route updates per second, and they do this via what you refer to as "manual route adding/removal".

Sorry, I was too unspecific here. Route changes because of address removal/addition on the local stack.

The reason why we do the bump_id here is that we want to flush all the socket caches in case we have either lost or gained access to a new source address.

If you think about e.g. BGP routers which update lots of routes, they aren't affected and the flush won't happen on every route change.

Quoted 3 lines

I don't think we want to put such a scalability problem into the tree.

There has to be a lightweight way to address this.

I am still investigating why this bump_id actually happened. Seems the reason is only sctp ontop of IPv6 and maybe we can build something much more lightweight, yes.

Thanks,
Hannes

--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 25 lines · Hannes Frederic Sowa

Quoted 11 lines · David Miller

From: Hannes Frederic Sowa <[email protected]>
Date: Wed, 10 Sep 2014 11:31:28 +0200

Quoted 5 lines

In case we need to force the sockets to relookup the routes we now increase the fn_sernum on all fibnodes in the routing tree. This is a costly operation but should only happen if we have major routing/policy changes in the kernel (e.g. manual route adding/removal, xfrm policy changes).

Core routers can update thousands of route updates per second, and they do this via what you refer to as "manual route adding/removal".

Sorry, I was too unspecific here. Route changes because of address removal/addition on the local stack.

The reason why we do the bump_id here is that we want to flush all the socket caches in case we have either lost or gained access to a new source address.

If you think about e.g. BGP routers which update lots of routes, they aren't affected and the flush won't happen on every route change.

Quoted 2 lines

I don't think we want to put such a scalability problem into the tree.

Quoted 5 lines

Quoted 1 line

There has to be a lightweight way to address this.

I am still investigating why this bump_id actually happened. Seems the reason is only sctp ontop of IPv6 and maybe we can build something much more lightweight, yes.

No. It was proven that a regular TCP socket could continue sending traffic using an address that was removed.

From: Hannes Frederic Sowa <[email protected]>
Date: Wed, 10 Sep 2014 11:31:28 +0200

Quoted 5 lines

In case we need to force the sockets to relookup the routes we now increase the fn_sernum on all fibnodes in the routing tree. This is a costly operation but should only happen if we have major routing/policy changes in the kernel (e.g. manual route adding/removal, xfrm policy changes).

Core routers can update thousands of route updates per second, and they do this via what you refer to as "manual route adding/removal".

I don't think we want to put such a scalability problem into the tree.

There has to be a lightweight way to address this.

Quoted 9 lines · David Miller

From: Hannes Frederic Sowa <[email protected]>
Date: Wed, 10 Sep 2014 11:31:28 +0200

Quoted 5 lines

In case we need to force the sockets to relookup the routes we now increase the fn_sernum on all fibnodes in the routing tree. This is a costly operation but should only happen if we have major routing/policy changes in the kernel (e.g. manual route adding/removal, xfrm policy changes).

Quoted 6 lines

Core routers can update thousands of route updates per second, and they do this via what you refer to as "manual route adding/removal".

I don't think we want to put such a scalability problem into the tree.

There has to be a lightweight way to address this.

An alternative approach without traversing the routing table, but each newly inserted route (even only cached ones) might bump all other routes out of the per-socket caches:

include/net/ip6_fib.h +0 -2

index 9bcb220..a7e45b9 100644
@@ -119,8 +119,6 @@ struct rt6_info {
 	struct inet6_dev		*rt6i_idev;
 	unsigned long			_rt6i_peer;
 
-	u32				rt6i_genid;
-
 	/* more non-fragment space at head required */
 	unsigned short			rt6i_nfheader_len;
 

include/net/net_namespace.h +14 -4

index 361d260..428fdcb 100644
@@ -358,18 +358,28 @@ static inline int rt_genid_ipv6(struct net *net)
 	return atomic_read(&net->ipv6.rt_genid);
 }
 
-static inline void rt_genid_bump_ipv6(struct net *net)
+static inline int rt_genid_bump_ipv6(struct net *net)
 {
-	atomic_inc(&net->ipv6.rt_genid);
+	int new, old;
+
+	do {
+		old = atomic_read(&net->ipv6.rt_genid);
+		new = old + 1;
+		if (new <= 0)
+			new = 1;
+	} while (atomic_cmpxchg(&net->ipv6.rt_genid, old, new) != old);
+	return new;
+
 }
 #else
 static inline int rt_genid_ipv6(struct net *net)
 {
-	return 0;
+	return 1;
 }
 
-static inline void rt_genid_bump_ipv6(struct net *net)
+static inline int rt_genid_bump_ipv6(struct net *net)
 {
+	return 1;
 }
 #endif
 

net/ipv6/ip6_fib.c +14 -16

index 76b7f5e..4a2f130 100644
@@ -84,7 +84,10 @@ static int fib6_walk_continue(struct fib6_walker_t *w);
  *	result of redirects, path MTU changes, etc.
  */
 
-static __u32 rt_sernum;
+static int fib6_new_sernum(struct net *net)
+{
+	return rt_genid_bump_ipv6(net);
+}
 
 static void fib6_gc_timer_cb(unsigned long arg);
 

@@ -104,13 +107,6 @@ static inline void fib6_walker_unlink(struct fib6_walker_t *w)
 	list_del(&w->lh);
 	write_unlock_bh(&fib6_walker_lock);
 }
-static __inline__ u32 fib6_new_sernum(void)
-{
-	u32 n = ++rt_sernum;
-	if ((__s32)n <= 0)
-		rt_sernum = n = 1;
-	return n;
-}
 
 /*
  *	Auxiliary address test functions for the radix tree.

@@ -421,16 +417,15 @@ out:
  */
 
 static struct fib6_node *fib6_add_1(struct fib6_node *root,
-				     struct in6_addr *addr, int plen,
-				     int offset, int allow_create,
-				     int replace_required)
+				    struct in6_addr *addr, int plen,
+				    int offset, int allow_create,
+				    int replace_required, int sernum)
 {
 	struct fib6_node *fn, *in, *ln;
 	struct fib6_node *pn = NULL;
 	struct rt6key *key;
 	int	bit;
 	__be32	dir = 0;
-	__u32	sernum = fib6_new_sernum();
 
 	RT6_TRACE("fib6_add_1\n");
 

@@ -844,6 +839,7 @@ void fib6_force_start_gc(struct net *net)
 int fib6_add(struct fib6_node *root, struct rt6_info *rt, struct nl_info *info,
 	     struct nlattr *mx, int mx_len)
 {
+	struct net *net = dev_net(rt->dst.dev);
 	struct fib6_node *fn, *pn = NULL;
 	int err = -ENOMEM;
 	int allow_create = 1;

@@ -860,7 +856,7 @@ int fib6_add(struct fib6_node *root, struct rt6_info *rt, struct nl_info *info,
 
 	fn = fib6_add_1(root, &rt->rt6i_dst.addr, rt->rt6i_dst.plen,
 			offsetof(struct rt6_info, rt6i_dst), allow_create,
-			replace_required);
+			replace_required, fib6_new_sernum(net));
 	if (IS_ERR(fn)) {
 		err = PTR_ERR(fn);
 		fn = NULL;

@@ -894,14 +890,15 @@ int fib6_add(struct fib6_node *root, struct rt6_info *rt, struct nl_info *info,
 			sfn->leaf = info->nl_net->ipv6.ip6_null_entry;
 			atomic_inc(&info->nl_net->ipv6.ip6_null_entry->rt6i_ref);
 			sfn->fn_flags = RTN_ROOT;
-			sfn->fn_sernum = fib6_new_sernum();
+			sfn->fn_sernum = fib6_new_sernum(net);
 
 			/* Now add the first leaf node to new subtree */
 
 			sn = fib6_add_1(sfn, &rt->rt6i_src.addr,
 					rt->rt6i_src.plen,
 					offsetof(struct rt6_info, rt6i_src),
-					allow_create, replace_required);
+					allow_create, replace_required,
+					fib6_new_sernum(net));
 
 			if (IS_ERR(sn)) {
 				/* If it is failed, discard just allocated

@@ -920,7 +917,8 @@ int fib6_add(struct fib6_node *root, struct rt6_info *rt, struct nl_info *info,
 			sn = fib6_add_1(fn->subtree, &rt->rt6i_src.addr,
 					rt->rt6i_src.plen,
 					offsetof(struct rt6_info, rt6i_src),
-					allow_create, replace_required);
+					allow_create, replace_required,
+					fib6_new_sernum(net));
 
 			if (IS_ERR(sn)) {
 				err = PTR_ERR(sn);

net/ipv6/route.c +0 -1

index f74b041..54b7d81 100644
@@ -314,7 +314,6 @@ static inline struct rt6_info *ip6_dst_alloc(struct net *net,
 
 		memset(dst + 1, 0, sizeof(*rt) - sizeof(*dst));
 		rt6_init_peer(rt, table ? &table->tb6_peers : net->ipv6.peers);
-		rt->rt6i_genid = rt_genid_ipv6(net);
 		INIT_LIST_HEAD(&rt->rt6i_siblings);
 	}
 	return rt;

@@ -1096,10 +1095,7 @@ static struct dst_entry *ip6_dst_check(struct dst_entry *dst, u32 cookie)
 	 * DST_OBSOLETE_FORCE_CHK which forces validation calls down

Quoted 189 lines · Hannes Frederic Sowa

Quoted 9 lines · David Miller

From: Hannes Frederic Sowa <[email protected]>
Date: Wed, 10 Sep 2014 11:31:28 +0200

Quoted 5 lines

In case we need to force the sockets to relookup the routes we now increase the fn_sernum on all fibnodes in the routing tree. This is a costly operation but should only happen if we have major routing/policy changes in the kernel (e.g. manual route adding/removal, xfrm policy changes).

Quoted 6 lines

Core routers can update thousands of route updates per second, and they do this via what you refer to as "manual route adding/removal".

I don't think we want to put such a scalability problem into the tree.

There has to be a lightweight way to address this.

An alternative approach without traversing the routing table, but each newly inserted route (even only cached ones) might bump all other routes out of the per-socket caches:

include/net/ip6_fib.h +0 -2

index 9bcb220..a7e45b9 100644
@@ -119,8 +119,6 @@ struct rt6_info {
 	struct inet6_dev		*rt6i_idev;
 	unsigned long			_rt6i_peer;
 
-	u32				rt6i_genid;
-
 	/* more non-fragment space at head required */
 	unsigned short			rt6i_nfheader_len;
 

include/net/net_namespace.h +14 -4

index 361d260..428fdcb 100644
@@ -358,18 +358,28 @@ static inline int rt_genid_ipv6(struct net *net)
 	return atomic_read(&net->ipv6.rt_genid);
 }
 
-static inline void rt_genid_bump_ipv6(struct net *net)
+static inline int rt_genid_bump_ipv6(struct net *net)
 {
-	atomic_inc(&net->ipv6.rt_genid);
+	int new, old;
+
+	do {
+		old = atomic_read(&net->ipv6.rt_genid);
+		new = old + 1;
+		if (new <= 0)
+			new = 1;
+	} while (atomic_cmpxchg(&net->ipv6.rt_genid, old, new) != old);
+	return new;
+
 }
 #else
 static inline int rt_genid_ipv6(struct net *net)
 {
-	return 0;
+	return 1;
 }
 
-static inline void rt_genid_bump_ipv6(struct net *net)
+static inline int rt_genid_bump_ipv6(struct net *net)
 {
+	return 1;
 }
 #endif
 

net/ipv6/ip6_fib.c +14 -16

index 76b7f5e..4a2f130 100644
@@ -84,7 +84,10 @@ static int fib6_walk_continue(struct fib6_walker_t *w);
  *	result of redirects, path MTU changes, etc.
  */
 
-static __u32 rt_sernum;
+static int fib6_new_sernum(struct net *net)
+{
+	return rt_genid_bump_ipv6(net);
+}
 
 static void fib6_gc_timer_cb(unsigned long arg);
 

@@ -104,13 +107,6 @@ static inline void fib6_walker_unlink(struct fib6_walker_t *w)
 	list_del(&w->lh);
 	write_unlock_bh(&fib6_walker_lock);
 }
-static __inline__ u32 fib6_new_sernum(void)
-{
-	u32 n = ++rt_sernum;
-	if ((__s32)n <= 0)
-		rt_sernum = n = 1;
-	return n;
-}
 
 /*
  *	Auxiliary address test functions for the radix tree.

@@ -421,16 +417,15 @@ out:
  */
 
 static struct fib6_node *fib6_add_1(struct fib6_node *root,
-				     struct in6_addr *addr, int plen,
-				     int offset, int allow_create,
-				     int replace_required)
+				    struct in6_addr *addr, int plen,
+				    int offset, int allow_create,
+				    int replace_required, int sernum)
 {
 	struct fib6_node *fn, *in, *ln;
 	struct fib6_node *pn = NULL;
 	struct rt6key *key;
 	int	bit;
 	__be32	dir = 0;
-	__u32	sernum = fib6_new_sernum();
 
 	RT6_TRACE("fib6_add_1\n");
 

@@ -844,6 +839,7 @@ void fib6_force_start_gc(struct net *net)
 int fib6_add(struct fib6_node *root, struct rt6_info *rt, struct nl_info *info,
 	     struct nlattr *mx, int mx_len)
 {
+	struct net *net = dev_net(rt->dst.dev);
 	struct fib6_node *fn, *pn = NULL;
 	int err = -ENOMEM;
 	int allow_create = 1;

@@ -860,7 +856,7 @@ int fib6_add(struct fib6_node *root, struct rt6_info *rt, struct nl_info *info,
 
 	fn = fib6_add_1(root, &rt->rt6i_dst.addr, rt->rt6i_dst.plen,
 			offsetof(struct rt6_info, rt6i_dst), allow_create,
-			replace_required);
+			replace_required, fib6_new_sernum(net));
 	if (IS_ERR(fn)) {
 		err = PTR_ERR(fn);
 		fn = NULL;

@@ -894,14 +890,15 @@ int fib6_add(struct fib6_node *root, struct rt6_info *rt, struct nl_info *info,
 			sfn->leaf = info->nl_net->ipv6.ip6_null_entry;
 			atomic_inc(&info->nl_net->ipv6.ip6_null_entry->rt6i_ref);
 			sfn->fn_flags = RTN_ROOT;
-			sfn->fn_sernum = fib6_new_sernum();
+			sfn->fn_sernum = fib6_new_sernum(net);
 
 			/* Now add the first leaf node to new subtree */
 
 			sn = fib6_add_1(sfn, &rt->rt6i_src.addr,
 					rt->rt6i_src.plen,
 					offsetof(struct rt6_info, rt6i_src),
-					allow_create, replace_required);
+					allow_create, replace_required,
+					fib6_new_sernum(net));
 
 			if (IS_ERR(sn)) {
 				/* If it is failed, discard just allocated

@@ -920,7 +917,8 @@ int fib6_add(struct fib6_node *root, struct rt6_info *rt, struct nl_info *info,
 			sn = fib6_add_1(fn->subtree, &rt->rt6i_src.addr,
 					rt->rt6i_src.plen,
 					offsetof(struct rt6_info, rt6i_src),
-					allow_create, replace_required);
+					allow_create, replace_required,
+					fib6_new_sernum(net));
 
 			if (IS_ERR(sn)) {
 				err = PTR_ERR(sn);

net/ipv6/route.c +1 -5

index f74b041..54b7d81 100644
@@ -314,7 +314,6 @@ static inline struct rt6_info *ip6_dst_alloc(struct net *net,
 
 		memset(dst + 1, 0, sizeof(*rt) - sizeof(*dst));
 		rt6_init_peer(rt, table ? &table->tb6_peers : net->ipv6.peers);
-		rt->rt6i_genid = rt_genid_ipv6(net);
 		INIT_LIST_HEAD(&rt->rt6i_siblings);
 	}
 	return rt;

@@ -1096,10 +1095,7 @@ static struct dst_entry *ip6_dst_check(struct dst_entry *dst, u32 cookie)
 	 * DST_OBSOLETE_FORCE_CHK which forces validation calls down
 	 * into this function always.
 	 */
-	if (rt->rt6i_genid != rt_genid_ipv6(dev_net(rt->dst.dev)))
-		return NULL;
-
-	if (!rt->rt6i_node || (rt->rt6i_node->fn_sernum != cookie))
+	if (!rt->rt6i_node || rt_genid_ipv6(dev_net(rt->dst.dev)) != cookie)
 		return NULL;
 

net/ipv6/route.c static struct dst_entry *ip6_dst_check(struct dst_entry *dst, u32 cookie) · line 1101

Quoted 2 lines

net/ipv6/route.c +0 -0

 	if (rt6_check_expired(rt))

Ok, so now we bump the gen_id every time we add a route and use that as fn_sernum for that route. But this doesn't solve the problem that we are seeing in that a re-lookup of the route still gives us an older route with an older gen_id.

Quoted 190 lines · Vlad Yasevich

Quoted 189 lines · Hannes Frederic Sowa

Quoted 9 lines · David Miller

From: Hannes Frederic Sowa <[email protected]>
Date: Wed, 10 Sep 2014 11:31:28 +0200

Quoted 5 lines

In case we need to force the sockets to relookup the routes we now increase the fn_sernum on all fibnodes in the routing tree. This is a costly operation but should only happen if we have major routing/policy changes in the kernel (e.g. manual route adding/removal, xfrm policy changes).

Quoted 6 lines

Core routers can update thousands of route updates per second, and they do this via what you refer to as "manual route adding/removal".

I don't think we want to put such a scalability problem into the tree.

There has to be a lightweight way to address this.

An alternative approach without traversing the routing table, but each newly inserted route (even only cached ones) might bump all other routes out of the per-socket caches:

include/net/ip6_fib.h +0 -2

index 9bcb220..a7e45b9 100644
@@ -119,8 +119,6 @@ struct rt6_info {
 	struct inet6_dev		*rt6i_idev;
 	unsigned long			_rt6i_peer;
 
-	u32				rt6i_genid;
-
 	/* more non-fragment space at head required */
 	unsigned short			rt6i_nfheader_len;
 

include/net/net_namespace.h +14 -4

index 361d260..428fdcb 100644
@@ -358,18 +358,28 @@ static inline int rt_genid_ipv6(struct net *net)
 	return atomic_read(&net->ipv6.rt_genid);
 }
 
-static inline void rt_genid_bump_ipv6(struct net *net)
+static inline int rt_genid_bump_ipv6(struct net *net)
 {
-	atomic_inc(&net->ipv6.rt_genid);
+	int new, old;
+
+	do {
+		old = atomic_read(&net->ipv6.rt_genid);
+		new = old + 1;
+		if (new <= 0)
+			new = 1;
+	} while (atomic_cmpxchg(&net->ipv6.rt_genid, old, new) != old);
+	return new;
+
 }
 #else
 static inline int rt_genid_ipv6(struct net *net)
 {
-	return 0;
+	return 1;
 }
 
-static inline void rt_genid_bump_ipv6(struct net *net)
+static inline int rt_genid_bump_ipv6(struct net *net)
 {
+	return 1;
 }
 #endif
 

net/ipv6/ip6_fib.c +14 -16

index 76b7f5e..4a2f130 100644
@@ -84,7 +84,10 @@ static int fib6_walk_continue(struct fib6_walker_t *w);
  *	result of redirects, path MTU changes, etc.
  */
 
-static __u32 rt_sernum;
+static int fib6_new_sernum(struct net *net)
+{
+	return rt_genid_bump_ipv6(net);
+}
 
 static void fib6_gc_timer_cb(unsigned long arg);
 

@@ -104,13 +107,6 @@ static inline void fib6_walker_unlink(struct fib6_walker_t *w)
 	list_del(&w->lh);
 	write_unlock_bh(&fib6_walker_lock);
 }
-static __inline__ u32 fib6_new_sernum(void)
-{
-	u32 n = ++rt_sernum;
-	if ((__s32)n <= 0)
-		rt_sernum = n = 1;
-	return n;
-}
 
 /*
  *	Auxiliary address test functions for the radix tree.

@@ -421,16 +417,15 @@ out:
  */
 
 static struct fib6_node *fib6_add_1(struct fib6_node *root,
-				     struct in6_addr *addr, int plen,
-				     int offset, int allow_create,
-				     int replace_required)
+				    struct in6_addr *addr, int plen,
+				    int offset, int allow_create,
+				    int replace_required, int sernum)
 {
 	struct fib6_node *fn, *in, *ln;
 	struct fib6_node *pn = NULL;
 	struct rt6key *key;
 	int	bit;
 	__be32	dir = 0;
-	__u32	sernum = fib6_new_sernum();
 
 	RT6_TRACE("fib6_add_1\n");
 

@@ -844,6 +839,7 @@ void fib6_force_start_gc(struct net *net)
 int fib6_add(struct fib6_node *root, struct rt6_info *rt, struct nl_info *info,
 	     struct nlattr *mx, int mx_len)
 {
+	struct net *net = dev_net(rt->dst.dev);
 	struct fib6_node *fn, *pn = NULL;
 	int err = -ENOMEM;
 	int allow_create = 1;

@@ -860,7 +856,7 @@ int fib6_add(struct fib6_node *root, struct rt6_info *rt, struct nl_info *info,
 
 	fn = fib6_add_1(root, &rt->rt6i_dst.addr, rt->rt6i_dst.plen,
 			offsetof(struct rt6_info, rt6i_dst), allow_create,
-			replace_required);
+			replace_required, fib6_new_sernum(net));
 	if (IS_ERR(fn)) {
 		err = PTR_ERR(fn);
 		fn = NULL;

@@ -894,14 +890,15 @@ int fib6_add(struct fib6_node *root, struct rt6_info *rt, struct nl_info *info,
 			sfn->leaf = info->nl_net->ipv6.ip6_null_entry;
 			atomic_inc(&info->nl_net->ipv6.ip6_null_entry->rt6i_ref);
 			sfn->fn_flags = RTN_ROOT;
-			sfn->fn_sernum = fib6_new_sernum();
+			sfn->fn_sernum = fib6_new_sernum(net);
 
 			/* Now add the first leaf node to new subtree */
 
 			sn = fib6_add_1(sfn, &rt->rt6i_src.addr,
 					rt->rt6i_src.plen,
 					offsetof(struct rt6_info, rt6i_src),
-					allow_create, replace_required);
+					allow_create, replace_required,
+					fib6_new_sernum(net));
 
 			if (IS_ERR(sn)) {
 				/* If it is failed, discard just allocated

@@ -920,7 +917,8 @@ int fib6_add(struct fib6_node *root, struct rt6_info *rt, struct nl_info *info,
 			sn = fib6_add_1(fn->subtree, &rt->rt6i_src.addr,
 					rt->rt6i_src.plen,
 					offsetof(struct rt6_info, rt6i_src),
-					allow_create, replace_required);
+					allow_create, replace_required,
+					fib6_new_sernum(net));
 
 			if (IS_ERR(sn)) {
 				err = PTR_ERR(sn);

net/ipv6/route.c +1 -5

index f74b041..54b7d81 100644
@@ -314,7 +314,6 @@ static inline struct rt6_info *ip6_dst_alloc(struct net *net,
 
 		memset(dst + 1, 0, sizeof(*rt) - sizeof(*dst));
 		rt6_init_peer(rt, table ? &table->tb6_peers : net->ipv6.peers);
-		rt->rt6i_genid = rt_genid_ipv6(net);
 		INIT_LIST_HEAD(&rt->rt6i_siblings);
 	}
 	return rt;

@@ -1096,10 +1095,7 @@ static struct dst_entry *ip6_dst_check(struct dst_entry *dst, u32 cookie)
 	 * DST_OBSOLETE_FORCE_CHK which forces validation calls down
 	 * into this function always.
 	 */
-	if (rt->rt6i_genid != rt_genid_ipv6(dev_net(rt->dst.dev)))
-		return NULL;
-
-	if (!rt->rt6i_node || (rt->rt6i_node->fn_sernum != cookie))
+	if (!rt->rt6i_node || rt_genid_ipv6(dev_net(rt->dst.dev)) != cookie)
 		return NULL;
 

Quoted 6 lines

net/ipv6/route.c static struct dst_entry *ip6_dst_check(struct dst_entry *dst, u32 cookie) · line 1101

Quoted 2 lines

net/ipv6/route.c +0 -0

 	if (rt6_check_expired(rt))

Ok, so now we bump the gen_id every time we add a route and use that as fn_sernum for that route. But this doesn't solve the problem that we are seeing in that a re-lookup of the route still gives us an older route with an older gen_id.

Hmm, this patch completely removes rt6i_genid from rt6_info (first hunk). We decide if we need to do the relookup based on the socket's cookie and the per-netns serial number of the ipv6 routing tables.

Bye,
Hannes

Quoted 198 lines · Hannes Frederic Sowa

Quoted 190 lines · Vlad Yasevich

Quoted 189 lines · Hannes Frederic Sowa

Quoted 9 lines · David Miller

From: Hannes Frederic Sowa <[email protected]>
Date: Wed, 10 Sep 2014 11:31:28 +0200

Quoted 5 lines

In case we need to force the sockets to relookup the routes we now increase the fn_sernum on all fibnodes in the routing tree. This is a costly operation but should only happen if we have major routing/policy changes in the kernel (e.g. manual route adding/removal, xfrm policy changes).

Quoted 6 lines

Core routers can update thousands of route updates per second, and they do this via what you refer to as "manual route adding/removal".

I don't think we want to put such a scalability problem into the tree.

There has to be a lightweight way to address this.

An alternative approach without traversing the routing table, but each newly inserted route (even only cached ones) might bump all other routes out of the per-socket caches:

include/net/ip6_fib.h +0 -2

index 9bcb220..a7e45b9 100644
@@ -119,8 +119,6 @@ struct rt6_info {
 	struct inet6_dev		*rt6i_idev;
 	unsigned long			_rt6i_peer;
 
-	u32				rt6i_genid;
-
 	/* more non-fragment space at head required */
 	unsigned short			rt6i_nfheader_len;
 

include/net/net_namespace.h +14 -4

index 361d260..428fdcb 100644
@@ -358,18 +358,28 @@ static inline int rt_genid_ipv6(struct net *net)
 	return atomic_read(&net->ipv6.rt_genid);
 }
 
-static inline void rt_genid_bump_ipv6(struct net *net)
+static inline int rt_genid_bump_ipv6(struct net *net)
 {
-	atomic_inc(&net->ipv6.rt_genid);
+	int new, old;
+
+	do {
+		old = atomic_read(&net->ipv6.rt_genid);
+		new = old + 1;
+		if (new <= 0)
+			new = 1;
+	} while (atomic_cmpxchg(&net->ipv6.rt_genid, old, new) != old);
+	return new;
+
 }
 #else
 static inline int rt_genid_ipv6(struct net *net)
 {
-	return 0;
+	return 1;
 }
 
-static inline void rt_genid_bump_ipv6(struct net *net)
+static inline int rt_genid_bump_ipv6(struct net *net)
 {
+	return 1;
 }
 #endif
 

net/ipv6/ip6_fib.c +14 -16

index 76b7f5e..4a2f130 100644
@@ -84,7 +84,10 @@ static int fib6_walk_continue(struct fib6_walker_t *w);
  *	result of redirects, path MTU changes, etc.
  */
 
-static __u32 rt_sernum;
+static int fib6_new_sernum(struct net *net)
+{
+	return rt_genid_bump_ipv6(net);
+}
 
 static void fib6_gc_timer_cb(unsigned long arg);
 

@@ -104,13 +107,6 @@ static inline void fib6_walker_unlink(struct fib6_walker_t *w)
 	list_del(&w->lh);
 	write_unlock_bh(&fib6_walker_lock);
 }
-static __inline__ u32 fib6_new_sernum(void)
-{
-	u32 n = ++rt_sernum;
-	if ((__s32)n <= 0)
-		rt_sernum = n = 1;
-	return n;
-}
 
 /*
  *	Auxiliary address test functions for the radix tree.

@@ -421,16 +417,15 @@ out:
  */
 
 static struct fib6_node *fib6_add_1(struct fib6_node *root,
-				     struct in6_addr *addr, int plen,
-				     int offset, int allow_create,
-				     int replace_required)
+				    struct in6_addr *addr, int plen,
+				    int offset, int allow_create,
+				    int replace_required, int sernum)
 {
 	struct fib6_node *fn, *in, *ln;
 	struct fib6_node *pn = NULL;
 	struct rt6key *key;
 	int	bit;
 	__be32	dir = 0;
-	__u32	sernum = fib6_new_sernum();
 
 	RT6_TRACE("fib6_add_1\n");
 

@@ -844,6 +839,7 @@ void fib6_force_start_gc(struct net *net)
 int fib6_add(struct fib6_node *root, struct rt6_info *rt, struct nl_info *info,
 	     struct nlattr *mx, int mx_len)
 {
+	struct net *net = dev_net(rt->dst.dev);
 	struct fib6_node *fn, *pn = NULL;
 	int err = -ENOMEM;
 	int allow_create = 1;

@@ -860,7 +856,7 @@ int fib6_add(struct fib6_node *root, struct rt6_info *rt, struct nl_info *info,
 
 	fn = fib6_add_1(root, &rt->rt6i_dst.addr, rt->rt6i_dst.plen,
 			offsetof(struct rt6_info, rt6i_dst), allow_create,
-			replace_required);
+			replace_required, fib6_new_sernum(net));
 	if (IS_ERR(fn)) {
 		err = PTR_ERR(fn);
 		fn = NULL;

@@ -894,14 +890,15 @@ int fib6_add(struct fib6_node *root, struct rt6_info *rt, struct nl_info *info,
 			sfn->leaf = info->nl_net->ipv6.ip6_null_entry;
 			atomic_inc(&info->nl_net->ipv6.ip6_null_entry->rt6i_ref);
 			sfn->fn_flags = RTN_ROOT;
-			sfn->fn_sernum = fib6_new_sernum();
+			sfn->fn_sernum = fib6_new_sernum(net);
 
 			/* Now add the first leaf node to new subtree */
 
 			sn = fib6_add_1(sfn, &rt->rt6i_src.addr,
 					rt->rt6i_src.plen,
 					offsetof(struct rt6_info, rt6i_src),
-					allow_create, replace_required);
+					allow_create, replace_required,
+					fib6_new_sernum(net));
 
 			if (IS_ERR(sn)) {
 				/* If it is failed, discard just allocated

@@ -920,7 +917,8 @@ int fib6_add(struct fib6_node *root, struct rt6_info *rt, struct nl_info *info,
 			sn = fib6_add_1(fn->subtree, &rt->rt6i_src.addr,
 					rt->rt6i_src.plen,
 					offsetof(struct rt6_info, rt6i_src),
-					allow_create, replace_required);
+					allow_create, replace_required,
+					fib6_new_sernum(net));
 
 			if (IS_ERR(sn)) {
 				err = PTR_ERR(sn);

net/ipv6/route.c +1 -5

index f74b041..54b7d81 100644
@@ -314,7 +314,6 @@ static inline struct rt6_info *ip6_dst_alloc(struct net *net,
 
 		memset(dst + 1, 0, sizeof(*rt) - sizeof(*dst));
 		rt6_init_peer(rt, table ? &table->tb6_peers : net->ipv6.peers);
-		rt->rt6i_genid = rt_genid_ipv6(net);
 		INIT_LIST_HEAD(&rt->rt6i_siblings);
 	}
 	return rt;

@@ -1096,10 +1095,7 @@ static struct dst_entry *ip6_dst_check(struct dst_entry *dst, u32 cookie)
 	 * DST_OBSOLETE_FORCE_CHK which forces validation calls down
 	 * into this function always.
 	 */
-	if (rt->rt6i_genid != rt_genid_ipv6(dev_net(rt->dst.dev)))
-		return NULL;
-
-	if (!rt->rt6i_node || (rt->rt6i_node->fn_sernum != cookie))
+	if (!rt->rt6i_node || rt_genid_ipv6(dev_net(rt->dst.dev)) != cookie)
 		return NULL;
 

Quoted 6 lines

net/ipv6/route.c static struct dst_entry *ip6_dst_check(struct dst_entry *dst, u32 cookie) · line 1101

Quoted 2 lines

net/ipv6/route.c +0 -0

 	if (rt6_check_expired(rt))

Ok, so now we bump the gen_id every time we add a route and use that as fn_sernum for that route. But this doesn't solve the problem that we are seeing in that a re-lookup of the route still gives us an older route with an older gen_id.

Quoted 4 lines

Hmm, this patch completely removes rt6i_genid from rt6_info (first hunk). We decide if we need to do the relookup based on the socket's cookie and the per-netns serial number of the ipv6 routing tables.

Hi Hannes

Right, by you still compare the per-netns serial number to the cookie. The cookie is typically taken from the route. The route takes it from the per-netns serial number. So, if you add a route with serial=1 and when the socket looks up that route, it stashes 1 into the cookie.
Then you add another route and serial = 2. Now the dst_check on the socket fails (cookie != rt_getid_ipv6(..)) and socket has to re-lookup a route. It gets the same old route as before with fn_sernum = 1. That's stashed in the cookie again. Next dst_check is done, the route is invalidated again!.

Am I missing something?

net/ipv4/tcp_ipv4.c +1 -1

@@ -1559,7 +1559,17 @@ bool tcp_prequeue(struct sock *sk, struct sk_buff *skb)
 	    skb_queue_len(&tp->ucopy.prequeue) == 0)
 		return false;
 
-	skb_dst_force(skb);
+	/* Before escaping RCU protected region, we need to take care of skb

From: Eric Dumazet <[email protected]>
Date: Mon, 08 Sep 2014 08:06:07 -0700

Quoted 6 lines

net/ipv4/tcp_ipv4.c +1 -1

@@ -1559,7 +1559,17 @@ bool tcp_prequeue(struct sock *sk, struct sk_buff *skb)
 	    skb_queue_len(&tp->ucopy.prequeue) == 0)
 		return false;
 
-	skb_dst_force(skb);
+	/* Before escaping RCU protected region, we need to take care of skb

net/ipv4/tcp_ipv4.c bool tcp_prequeue(struct sock *sk, struct sk_buff *skb) · line 1572

Quoted 10 lines

net/ipv4/tcp_ipv4.c +10 -0

+	 * dst. Prequeue is only enabled for established sockets.
+	 * For such sockets, we might need the skb dst only to set sk->sk_rx_dst
+	 * Instead of doing full sk_rx_dst validity here, let's perform
+	 * an optimistic check.
+	 */
+	if (likely(sk->sk_rx_dst))
+		skb_dst_drop(skb);
+	else
+		skb_dst_force(skb);
+

This might not be a strong enough test.

We have to also make all of the checks that would cause the input path to invalidate sk->sk_rx_dst too.

Otherwise, if it does, we'll crash when we try to do a dst_hold() on skb_dst(skb) in sk->sk_rx_dst_set().

In the worst case, here is what is happening :

sk_rx_dst is checked and invalidated in tcp_v4_do_rcv()

Next packets coming from prequeue might then have a NULL dst, and we'll do nothing special (sk_rx_dst will stay NULL), because we do handle NULL dst properly in inet_sk_rx_dst_set() and inet6_sk_rx_dst_set()

But next packet to be processed (either in non prequeue mode or prequeue) will carry skb->dst and we will set sk->sk_rx_dst

I decided to not copy/paste the tests we do in the family dependent parts, because it was not worth the pain.

In the worst case, here is what is happening :

sk_rx_dst is checked and invalidated in tcp_v4_do_rcv()

Next packets coming from prequeue might then have a NULL dst, and we'll do nothing special (sk_rx_dst will stay NULL), because we do handle NULL dst properly in inet_sk_rx_dst_set() and inet6_sk_rx_dst_set()

But next packet to be processed (either in non prequeue mode or prequeue) will carry skb->dst and we will set sk->sk_rx_dst

I decided to not copy/paste the tests we do in the family dependent parts, because it was not worth the pain.

From: Alexander Duyck <[email protected]>
Date: Thu, 14 Aug 2014 16:16:36 -0700

Quoted 2 lines

Are you sure about each socket having it's own DST? Everything I see seems to indicate it is somehow associated with IP.

Right it should be, unless you have exception entries created by path MTU or redirects.

WRT prequeue, it does the right thing for dumb apps that block in receive. But because it causes the packet to cross domains as it does, we can't do a lot of tricks which we normally can do, and that's why the refcounting on the dst is there now.

Perhaps we can find a clever way to elide that refcount, who knows.

I realize most of my data is anecdotal as I only have the ixgbe/igb adapters and netperf to work with. This is one of the reasons why I keep asking if someone can tell me what the use case is for this where it performs well. From what I can tell it might have had some value back in the day before the introduction of things such as RPS/RFS where some of the socket processing would be offloaded to other CPUs for a single queue device, but even that use case is now deprecated since RPS/RFS are there and function better than this. What I am basically looking for is a way to weight the gain versus the penalties to determine if this code is even viable anymore.

In the meantime I think I will put together a patch to default tcp_low_latency to 1 for net and stable, and if we cannot find a good reason for keeping it then I can submit a patch to net-next that will strip it out since I don't see any benefit to having this code.

Quoted 9 lines · David Miller

From: Alexander Duyck <[email protected]>
Date: Thu, 14 Aug 2014 16:16:36 -0700

Quoted 2 lines

Are you sure about each socket having it's own DST? Everything I see seems to indicate it is somehow associated with IP.

Right it should be, unless you have exception entries created by path MTU or redirects.

Quoted 6 lines

WRT prequeue, it does the right thing for dumb apps that block in receive. But because it causes the packet to cross domains as it does, we can't do a lot of tricks which we normally can do, and that's why the refcounting on the dst is there now.

Perhaps we can find a clever way to elide that refcount, who knows.

Actually I would consider the refcount issue just the coffin nail in all of this. It seems like there are multiple issues that have been there for some time and they are just getting worse with the refcount change in 3.10.

With the prequeue disabled what happens is that the frames are making it up and hitting tcp_rcv_established before being pushed into the backlog queues and coalesced there. I believe the lack of coalescing on the prequeue path is one of the reasons why it is twice as expensive as the non-prequeue path CPU wise even if you eliminate the refcount issue.

I realize most of my data is anecdotal as I only have the ixgbe/igb adapters and netperf to work with. This is one of the reasons why I keep asking if someone can tell me what the use case is for this where it performs well. From what I can tell it might have had some value back in the day before the introduction of things such as RPS/RFS where some of the socket processing would be offloaded to other CPUs for a single queue device, but even that use case is now deprecated since RPS/RFS are there and function better than this. What I am basically looking for is a way to weight the gain versus the penalties to determine if this code is even viable anymore.

In the meantime I think I will put together a patch to default tcp_low_latency to 1 for net and stable, and if we cannot find a good reason for keeping it then I can submit a patch to net-next that will strip it out since I don't see any benefit to having this code.

Thanks,

Alex

--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Alex, I tried to repro your problem running your script (on bnx2x). Didn't see see the issue and in fact ip_dest_check did not appear in top perf functions on perf. I assume this is more related to the steering configuration rather than the device (although flow director might be a fundamental difference).

Quoted 6 lines · Tom Herbert

Alex, I tried to repro your problem running your script (on bnx2x). Didn't see see the issue and in fact ip_dest_check did not appear in top perf functions on perf. I assume this is more related to the steering configuration rather than the device (although flow director might be a fundamental difference).

So the original script I put out had a typo. It was supposed to run all 60 at the same time, not one at a time. So make sure you add an ampersand to the end of the netperf command line if you run the test so that it is 60 at once, not 60 in series.

Also one other thing I had to do was disable tcp_autocork. Without that the test is a large packets test instead of a small packet test.

Thanks,

Alex

Quoted 8 lines · Alexander Duyck

Quoted 6 lines · Tom Herbert

Alex, I tried to repro your problem running your script (on bnx2x). Didn't see see the issue and in fact ip_dest_check did not appear in top perf functions on perf. I assume this is more related to the steering configuration rather than the device (although flow director might be a fundamental difference).

Quoted 8 lines

So the original script I put out had a typo. It was supposed to run all 60 at the same time, not one at a time. So make sure you add an ampersand to the end of the netperf command line if you run the test so that it is 60 at once, not 60 in series.

Also one other thing I had to do was disable tcp_autocork. Without that the test is a large packets test instead of a small packet test.

Okay, by running netperf in background, disabling autoconf, and turning off RPS/RFS I'm able to get ipv4_dst_check to come up in perf; but t's not nearly as bad as what you've reported though, only about 1.5%. When I applied path to move rt_genid to different cacheline ipv4_dst_check goes away (ipv4: move rt_genid to different cache line). Can you try this patch in your setup?

...
Another test I tried was to hack the nettest_bsd.c file in netperf to perform a poll() based receive. That resolved the issue and had all the performance of the tcp_low_latency case. I may see if I can work with Rick to push something like that into netperf as I really would prefer to avoid having to advise everyone on how to setup the sysctl for tcp_low_latency.

From: Alexander Duyck

Quoted 7 lines

...
Another test I tried was to hack the nettest_bsd.c file in netperf to perform a poll() based receive. That resolved the issue and had all the performance of the tcp_low_latency case. I may see if I can work with Rick to push something like that into netperf as I really would prefer to avoid having to advise everyone on how to setup the sysctl for tcp_low_latency.

Doesn't that generate 2 system calls per receive? Unless it now returns more data per receive I'm surprised that it actually faster.

OTOH I've some code that runs a lot better when I run while :; do :; done for all but one of the cpus.
I think that is because the processes spinning in userspace don't get pre-empted.

David

This is one of the reasons why I keep asking if someone can tell me what the use case is for this where it performs well.

Quoted 2 lines · Eric Dumazet

In real life, applications do not use prequeue, because nobody wants one thread per flow.

I still say this is just an argument to remove it. It looks like you submitted a patch to allow stripping it from the build about 7 years go.
I assume it was rejected.

Quoted 3 lines

Each socket has its own dst now route cache was removed, but if your netperf migrates cpu (and NUMA node), we do not detect the dst should be re-created onto a different NUMA node.

Are you sure about each socket having it's own DST? Everything I see seems to indicate it is somehow associated with IP. For example I can actually work around the issue by setting up a second subnet on the same port and then running the tests with each subnet affinitized to a specific node.

Here is the thing

nh_pcpu_rth_output is a per cpu cache.

But nh_rth_input is not, because we did not optimize the case where dst has to be refcounted, yet.

Rationale is explained in d26b3a7c4b3b26319f18bb645de93eba8f4bdcd5 ("ipv4: percpu nh_rth_output cache")

If you guys really believe we should have a percpu dst, go for it, but again, if the softirq handler runs on a different cpu than the application thread, it wont work.

And, given 72 core servers are now on the way, we'll consume more ram, for netperf users.