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

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

[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.

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 <clm@fb.com>

---
 net/ipv6/ip6_fib.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index 87891f5..19b0f78 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -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;
 	}
-- 
1.8.1

Re: [PATCH RFC] ipv6_fib limit spinlock hold times for /proc/net/ipv6_route

[Hannes Frederic Sowa]

Hi!

On Thu, Apr 24, 2014 at 09:59:24AM -0400, Chris Mason wrote:
> 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.

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

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

Re: [PATCH RFC] ipv6_fib limit spinlock hold times for /proc/net/ipv6_route

[Eric Dumazet]

On Thu, 2014-04-24 at 09:59 -0400, Chris Mason wrote:
> 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 <clm@fb.com>
> 
> ---

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

Re: [PATCH RFC] ipv6_fib limit spinlock hold times for /proc/net/ipv6_route

[Eric Dumazet]

On Thu, 2014-04-24 at 16:20 +0200, Hannes Frederic Sowa wrote:

> 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.

Re: [PATCH RFC] ipv6_fib limit spinlock hold times for /proc/net/ipv6_route

[Chris Mason]

On 04/24/2014 10:20 AM, Hannes Frederic Sowa wrote:
> Hi!
>
> On Thu, Apr 24, 2014 at 09:59:24AM -0400, Chris Mason wrote:
>> 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?

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.

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

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

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

Re: [PATCH RFC] ipv6_fib limit spinlock hold times for /proc/net/ipv6_route

[David Miller]

From: Chris Mason <clm@fb.com>
Date: Thu, 24 Apr 2014 09:59:24 -0400

> 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.

Re: [PATCH RFC] ipv6_fib limit spinlock hold times for /proc/net/ipv6_route

[David Miller]

From: Chris Mason <clm@fb.com>
Date: Thu, 24 Apr 2014 09:59:24 -0400

> 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.

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.

Re: [PATCH RFC] ipv6_fib limit spinlock hold times for /proc/net/ipv6_route

[Chris Mason]

On 04/25/2014 04:09 PM, David Miller wrote:
> From: Chris Mason <clm@fb.com>
> Date: Thu, 24 Apr 2014 09:59:24 -0400
>
>> 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.
>
> 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)

>
> 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.

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

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

Re: [PATCH RFC] ipv6_fib limit spinlock hold times for /proc/net/ipv6_route

[Hannes Frederic Sowa]

On Thu, Apr 24, 2014 at 10:41:11AM -0400, Chris Mason wrote:
> 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.

Re: [PATCH RFC] ipv6_fib limit spinlock hold times for /proc/net/ipv6_route

[Hannes Frederic Sowa]

On Fri, Apr 25, 2014 at 04:27:21PM -0400, Chris Mason wrote:
> On 04/25/2014 04:09 PM, David Miller wrote:
> >From: Chris Mason <clm@fb.com>
> >Date: Thu, 24 Apr 2014 09:59:24 -0400
> >
> >>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.
> >
> >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!

> 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.

> >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.
> 
> >
> >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.

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

Re: [PATCH RFC] ipv6_fib limit spinlock hold times for /proc/net/ipv6_route

[David Miller]

From: Hannes Frederic Sowa <hannes@stressinduktion.org>
Date: Fri, 25 Apr 2014 23:31:40 +0200

> On Thu, Apr 24, 2014 at 10:41:11AM -0400, Chris Mason wrote:
>> 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.

Re: [PATCH RFC] ipv6_fib limit spinlock hold times for /proc/net/ipv6_route

[David Miller]

From: Chris Mason <clm@fb.com>
Date: Fri, 25 Apr 2014 16:27:21 -0400

> 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.

Re: [PATCH RFC] ipv6_fib limit spinlock hold times for /proc/net/ipv6_route

[Chris Mason]

On 04/26/2014 12:11 AM, David Miller wrote:
> From: Chris Mason <clm@fb.com>
> Date: Fri, 25 Apr 2014 16:27:21 -0400
>
>> 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