Skip to contentSkip to conversation

ipv6_fib limit spinlock hold times for /proc/net/ipv6_route

RFC PATCH In review Chris Mason 4 participants 13 messages Apr 24, 2014 – Apr 28, 2014

Subsystems (3)
Networking Network drivers IPv4/IPv6
lore
Share & tools
lore

Chris Mason

author
#0 lore raw

RFC PATCH 1/1 ipv6_fib limit spinlock hold times for /proc/net/ipv6_route

The ipv6 code to dump routes in /proc/net/ipv6_route can hold a read lock on the table for a very long time. This ends up blocking writers and triggering softlockups.

This patch is a simple work around to limit the number of entries we'll walk while processing /proc/net/ipv6_route. It intentionally slows down proc file reading to make sure we don't lock out the real ipv6 traffic.

This patch is also horrible, and doesn't actually fix the entire problem. We still have rcu_read_lock held the whole time we cat /proc/net/ipv6_route. On an unpatched machine, I've clocked the time required to cat /proc/net/ipv6_route at 14 minutes.

java cats this proc file on startup to search for local routes, and the resulting contention on the table lock makes our boxes fall over.

So, I'm sending the partial fix to get discussion started.

Signed-off-by Chris Mason <[email protected]>
---
 net/ipv6/ip6_fib.c | 9 +++++++++
 1 file changed, 9 insertions(+)
net/ipv6/ip6_fib.c +9 -0
index 87891f5..19b0f78 100644
@@ -1814,6 +1814,7 @@ struct ipv6_route_iter {
 	loff_t skip;
 	struct fib6_table *tbl;
 	__u32 sernum;
+	int max_walk;
 };
 
 static int ipv6_route_seq_show(struct seq_file *seq, void *v)

@@ -1853,8 +1854,11 @@ static int ipv6_route_yield(struct fib6_walker_t *w)
 		iter->skip--;
 		if (!iter->skip && iter->w.leaf)
 			return 1;
+		iter->max_walk--;
 	} while (iter->w.leaf);
 
+	if (iter->max_walk <= 0)
+		return -EAGAIN;
 	return 0;
 }
 

@@ -1867,6 +1871,7 @@ static void ipv6_route_seq_setup_walk(struct ipv6_route_iter *iter)
 	iter->w.node = iter->w.root;
 	iter->w.args = iter;
 	iter->sernum = iter->w.root->fn_sernum;
+	iter->max_walk = 128;
 	INIT_LIST_HEAD(&iter->w.lh);
 	fib6_walker_link(&iter->w);
 }

@@ -1921,7 +1926,9 @@ static void *ipv6_route_seq_next(struct seq_file *seq, void *v, loff_t *pos)
 
 iter_table:
 	ipv6_route_check_sernum(iter);
+iter_again:
 	read_lock(&iter->tbl->tb6_lock);
+	iter->max_walk = 128;
 	r = fib6_walk_continue(&iter->w);
 	read_unlock(&iter->tbl->tb6_lock);
 	if (r > 0) {

@@ -1929,6 +1936,8 @@ iter_table:
 			++*pos;
 		return iter->w.leaf;
 	} else if (r < 0) {
+		if (r == -EAGAIN)
+			goto iter_again;
 		fib6_walker_unlink(&iter->w);
 		return NULL;
 	}
Signature · 1 line
1.8.1
#1 lore raw

Hi!

Quoted 8 lines · Chris Mason

The ipv6 code to dump routes in /proc/net/ipv6_route can hold a read lock on the table for a very long time. This ends up blocking writers and triggering softlockups.

This patch is a simple work around to limit the number of entries we'll walk while processing /proc/net/ipv6_route. It intentionally slows down proc file reading to make sure we don't lock out the real ipv6 traffic.

I guess most time is spent in formatting and printing the rt6_info details to the procfs file. Have you tried excluding !(rt6_info->rt6i_flags & RTF_CACHE) routes?

Maybe this is a viable alternative. A patch could also check for RTF_DYNAMIC and RTF_MODIFIED so we would still show redirected and mtu-caching nodes.

Quoted 7 lines

This patch is also horrible, and doesn't actually fix the entire problem. We still have rcu_read_lock held the whole time we cat /proc/net/ipv6_route. On an unpatched machine, I've clocked the time required to cat /proc/net/ipv6_route at 14 minutes.

java cats this proc file on startup to search for local routes, and the resulting contention on the table lock makes our boxes fall over.

Urks, does plain openjdk do that or is this something in your application?

Quoted 2 lines

So, I'm sending the partial fix to get discussion started.

I am planing to submit patches which reduce the caching of DST_HOST entries in the ipv6 fib next month which will result in a much smaller fib to walk by then.

Greetings,

Hannes

#2 lore raw
Quoted 3 lines · Hannes Frederic Sowa

I am planing to submit patches which reduce the caching of DST_HOST entries in the ipv6 fib next month which will result in a much smaller fib to walk by then.

Or, if caching is still needed, we might use a separate hash table for this, as it is more scalable.

author
#3 lore raw
In reply to Hannes Frederic Sowa
Quoted 7 lines · Hannes Frederic Sowa

Hi!

Quoted 4 lines · Chris Mason

The ipv6 code to dump routes in /proc/net/ipv6_route can hold a read lock on the table for a very long time. This ends up blocking writers and triggering softlockups.

Quoted 8 lines
Quoted 4 lines

This patch is a simple work around to limit the number of entries we'll walk while processing /proc/net/ipv6_route. It intentionally slows down proc file reading to make sure we don't lock out the real ipv6 traffic.

I guess most time is spent in formatting and printing the rt6_info details to the procfs file. Have you tried excluding !(rt6_info->rt6i_flags & RTF_CACHE) routes?

We do have a separate patch from Paul Saab that excludes the cached routes and it has a big impact (~10x fewer entries). But the softlockups still flow.

I was going to discuss the cache exclusion on a separate thread, but the short version is that I don't have any clue of how many people we'd upset by unconditionally leaving out the cached entries.

Quoted 10 lines

Maybe this is a viable alternative. A patch could also check for RTF_DYNAMIC and RTF_MODIFIED so we would still show redirected and mtu-caching nodes.

Quoted 5 lines

This patch is also horrible, and doesn't actually fix the entire problem. We still have rcu_read_lock held the whole time we cat /proc/net/ipv6_route. On an unpatched machine, I've clocked the time required to cat /proc/net/ipv6_route at 14 minutes.

Quoted 5 lines
Quoted 2 lines

java cats this proc file on startup to search for local routes, and the resulting contention on the table lock makes our boxes fall over.

Urks, does plain openjdk do that or is this something in your application?

Seems to be built into the jdk, and not our app.

Quoted 6 lines
Quoted 2 lines

So, I'm sending the partial fix to get discussion started.

I am planing to submit patches which reduce the caching of DST_HOST entries in the ipv6 fib next month which will result in a much smaller fib to walk by then.

Great.

-chris

#4 lore raw
Quoted 3 lines · Chris Mason

I was going to discuss the cache exclusion on a separate thread, but the short version is that I don't have any clue of how many people we'd upset by unconditionally leaving out the cached entries.

In my opinion this would be ok.

We definitely must reduce number of entries in the fib and I don't think it is wise to synthesize them thereafter, especially if we don't need to track state at all, e.g. forwarding.

maintainer
#5 lore raw

From: Hannes Frederic Sowa <[email protected]>
Date: Fri, 25 Apr 2014 23:31:40 +0200

Quoted 10 lines · Chris Mason
Quoted 3 lines

I was going to discuss the cache exclusion on a separate thread, but the short version is that I don't have any clue of how many people we'd upset by unconditionally leaving out the cached entries.

In my opinion this would be ok.

We definitely must reduce number of entries in the fib and I don't think it is wise to synthesize them thereafter, especially if we don't need to track state at all, e.g. forwarding.

I think this is a dangerous idea, to limit what is shown in the dumps.

We must always output the entry that would actually be used by a lookup otherwise you are asking for impossible to debug scenerios.

On the other side, once we change ipv6 to work like ipv4, those cached entries simply won't be there at all and then automatically this huge dump issue just goes away.

#6 lore raw
In reply to Chris Mason
Quoted 14 lines · Chris Mason

The ipv6 code to dump routes in /proc/net/ipv6_route can hold a read lock on the table for a very long time. This ends up blocking writers and triggering softlockups.

This patch is a simple work around to limit the number of entries we'll walk while processing /proc/net/ipv6_route. It intentionally slows down proc file reading to make sure we don't lock out the real ipv6 traffic.

This patch is also horrible, and doesn't actually fix the entire problem. We still have rcu_read_lock held the whole time we cat /proc/net/ipv6_route. On an unpatched machine, I've clocked the time required to cat /proc/net/ipv6_route at 14 minutes.

Quoted 8 lines

java cats this proc file on startup to search for local routes, and the resulting contention on the table lock makes our boxes fall over.

So, I'm sending the partial fix to get discussion started.

Signed-off-by Chris Mason <[email protected]>

---

It might be time to convert this to RCU and/or a better data structure.

maintainer
#7 lore raw
In reply to Chris Mason

From: Chris Mason <[email protected]>
Date: Thu, 24 Apr 2014 09:59:24 -0400

Quoted 2 lines

java cats this proc file on startup to search for local routes, and the resulting contention on the table lock makes our boxes fall over.

I know it's tangental, but Java should open a netlink socket and probe for such things properly.

maintainer
#8 lore raw
In reply to Chris Mason

From: Chris Mason <[email protected]>
Date: Thu, 24 Apr 2014 09:59:24 -0400

Quoted 9 lines

The ipv6 code to dump routes in /proc/net/ipv6_route can hold a read lock on the table for a very long time. This ends up blocking writers and triggering softlockups.

This patch is a simple work around to limit the number of entries we'll walk while processing /proc/net/ipv6_route. It intentionally slows down proc file reading to make sure we don't lock out the real ipv6 traffic.

Quoted 4 lines

This patch is also horrible, and doesn't actually fix the entire problem. We still have rcu_read_lock held the whole time we cat /proc/net/ipv6_route. On an unpatched machine, I've clocked the time required to cat /proc/net/ipv6_route at 14 minutes.

There is another way to more effectively mitigate this.

Take the rtnl mutex over the traversals.

The tables cannot change if you hold it.

Then you can use rcu_dereference_rtnl() in the table traversals and get rid of the RCU locking entirely.

Now you're only left with the read locking over the individual trees. And as in your patch we can drop it temporarily after a limit is hit.

But yes, longer term we need to convert the ipv6 route trees over to RCU or similar.

Even better would be to align the ipv6 routing with how ipv4 works since the routing-cache removal.

author
#9 lore raw
Quoted 17 lines · David Miller

From: Chris Mason <[email protected]>
Date: Thu, 24 Apr 2014 09:59:24 -0400

Quoted 9 lines

The ipv6 code to dump routes in /proc/net/ipv6_route can hold a read lock on the table for a very long time. This ends up blocking writers and triggering softlockups.

This patch is a simple work around to limit the number of entries we'll walk while processing /proc/net/ipv6_route. It intentionally slows down proc file reading to make sure we don't lock out the real ipv6 traffic.

Quoted 4 lines

This patch is also horrible, and doesn't actually fix the entire problem. We still have rcu_read_lock held the whole time we cat /proc/net/ipv6_route. On an unpatched machine, I've clocked the time required to cat /proc/net/ipv6_route at 14 minutes.

Quoted 8 lines

There is another way to more effectively mitigate this.

Take the rtnl mutex over the traversals.

The tables cannot change if you hold it.

Then you can use rcu_dereference_rtnl() in the table traversals and get rid of the RCU locking entirely.

Ah ok, so the rtnl mutex can replace rcu_read_lock(). Will it end up blocking any traffic? (sorry, filesystem guys are a little slow)

Quoted 3 lines

Now you're only left with the read locking over the individual trees. And as in your patch we can drop it temporarily after a limit is hit.

That would be wonderful because I can use some cond_resched() variant, and get rid of the max_walk counter completely.

Quoted 3 lines

But yes, longer term we need to convert the ipv6 route trees over to RCU or similar.

Instead of the ->skip counter, can we get a cursor into the tree and just resume walking at the first entry after that cursor? It would have to be a key that we copy out instead of a pointer so we can drop the rcu_read_lock()

Quoted 4 lines

Even better would be to align the ipv6 routing with how ipv4 works since the routing-cache removal.

I'll shop task that around here.

-chris

#10 lore raw
Quoted 18 lines · Chris Mason
Quoted 17 lines · David Miller

From: Chris Mason <[email protected]>
Date: Thu, 24 Apr 2014 09:59:24 -0400

Quoted 13 lines

The ipv6 code to dump routes in /proc/net/ipv6_route can hold a read lock on the table for a very long time. This ends up blocking writers and triggering softlockups.

This patch is a simple work around to limit the number of entries we'll walk while processing /proc/net/ipv6_route. It intentionally slows down proc file reading to make sure we don't lock out the real ipv6 traffic.

This patch is also horrible, and doesn't actually fix the entire problem. We still have rcu_read_lock held the whole time we cat /proc/net/ipv6_route. On an unpatched machine, I've clocked the time required to cat /proc/net/ipv6_route at 14 minutes.

Quoted 8 lines
Quoted 8 lines

There is another way to more effectively mitigate this.

Take the rtnl mutex over the traversals.

The tables cannot change if you hold it.

Then you can use rcu_dereference_rtnl() in the table traversals and get rid of the RCU locking entirely.

That's a pretty good idea!

Quoted 2 lines

Ah ok, so the rtnl mutex can replace rcu_read_lock(). Will it end up blocking any traffic? (sorry, filesystem guys are a little slow)

Traffic flow shouldn't get stopped but any network stack configuration would wait for the time being completly, this could also affect ipv4.

Quoted 7 lines
Quoted 2 lines

Now you're only left with the read locking over the individual trees. And as in your patch we can drop it temporarily after a limit is hit.

That would be wonderful because I can use some cond_resched() variant, and get rid of the max_walk counter completely.

Quoted 1 line
Quoted 7 lines
Quoted 2 lines

But yes, longer term we need to convert the ipv6 route trees over to RCU or similar.

Instead of the ->skip counter, can we get a cursor into the tree and just resume walking at the first entry after that cursor? It would have to be a key that we copy out instead of a pointer so we can drop the rcu_read_lock()

The keys could be struct rt6key (rt6i_dst, rt6i_src) and the table (which won't get dropped because of rtnl lock).

The function to locate the node would be fib6_locate, because we would have to respect the prefix lengths during lookup.

Quoted 5 lines
Quoted 3 lines

Even better would be to align the ipv6 routing with how ipv4 works since the routing-cache removal.

I'll shop task that around here.

I'll send out some patches reducing DST_CACHE entries soon (hopefully next week), so we can build up on them.

I am very keen on getting this task done.

Thanks,

Hannes

maintainer
#11 lore raw
In reply to Chris Mason

From: Chris Mason <[email protected]>
Date: Fri, 25 Apr 2014 16:27:21 -0400

Quoted 2 lines

Ah ok, so the rtnl mutex can replace rcu_read_lock(). Will it end up blocking any traffic? (sorry, filesystem guys are a little slow)

It stops networking configuration operations.

BTW, this means that netlink based ipv6 route dumps can be optimized similarly. Because those code paths implicitly hold the RTNL mutex.

author
#12 lore raw
Quoted 12 lines · David Miller

From: Chris Mason <[email protected]>
Date: Fri, 25 Apr 2014 16:27:21 -0400

Quoted 2 lines

Ah ok, so the rtnl mutex can replace rcu_read_lock(). Will it end up blocking any traffic? (sorry, filesystem guys are a little slow)

It stops networking configuration operations.

BTW, this means that netlink based ipv6 route dumps can be optimized similarly. Because those code paths implicitly hold the RTNL mutex.

Ok, the rtnl mutex looks like a much better path. I'll give it a shot this week.

If anyone working on patches in this area wants help testing, just let me know.

-chris