From: Martin KaFai Lau <hidden> Date: 2017-09-26 00:56:25
On Mon, Sep 25, 2017 at 05:35:22PM +0000, Wei Wang wrote:
quoted hunk
From: Wei Wang <redacted>
fib6_del() generates WARN_ON() when rt->dst.obsolete > 0. This does not
make sense because it is possible that the route passed in is already
deleted by some other thread and rt->dst.obsolete is set to
DST_OBSOLETE_DEAD.
So this commit deletes this WARN_ON() and also remove the
"#ifdef RT6_DEBUG >= 2" condition so that if the route is already
obsolete, we return right at the beginning of fib6_del().
On Mon, Sep 25, 2017 at 5:56 PM, Martin KaFai Lau [off-list ref] wrote:
On Mon, Sep 25, 2017 at 05:35:22PM +0000, Wei Wang wrote:
quoted
From: Wei Wang <redacted>
fib6_del() generates WARN_ON() when rt->dst.obsolete > 0. This does not
make sense because it is possible that the route passed in is already
deleted by some other thread and rt->dst.obsolete is set to
DST_OBSOLETE_DEAD.
So this commit deletes this WARN_ON() and also remove the
"#ifdef RT6_DEBUG >= 2" condition so that if the route is already
obsolete, we return right at the beginning of fib6_del().
fn should have already been set to NULL if it is removed
from the fib6 tree?
That is true. rt->rt6i_node (fn) should already be marked as NULL.
That means the check on rt->dst.obsolete is redundant.
I will remove it in v2.
Thanks Martin.
From: Martin KaFai Lau <hidden> Date: 2017-09-26 02:07:50
On Tue, Sep 26, 2017 at 01:16:05AM +0000, Wei Wang wrote:
On Mon, Sep 25, 2017 at 5:56 PM, Martin KaFai Lau [off-list ref] wrote:
quoted
On Mon, Sep 25, 2017 at 05:35:22PM +0000, Wei Wang wrote:
quoted
From: Wei Wang <redacted>
fib6_del() generates WARN_ON() when rt->dst.obsolete > 0. This does not
make sense because it is possible that the route passed in is already
deleted by some other thread and rt->dst.obsolete is set to
DST_OBSOLETE_DEAD.
So this commit deletes this WARN_ON() and also remove the
"#ifdef RT6_DEBUG >= 2" condition so that if the route is already
obsolete, we return right at the beginning of fib6_del().
fn should have already been set to NULL if it is removed
from the fib6 tree?
That is true. rt->rt6i_node (fn) should already be marked as NULL.
I am probably still missing something.
Considering the del operation should be under the writer lock,
if rt->rt6i_node should be NULL (for rt that has already been
removed from fib6), why this WARN_ON() is triggered?
An example may help.
That means the check on rt->dst.obsolete is redundant.
I will remove it in v2.
Thanks Martin.
From: Eric Dumazet <edumazet@google.com> Date: 2017-09-26 02:24:00
On Mon, Sep 25, 2017 at 7:07 PM, Martin KaFai Lau [off-list ref] wrote:
I am probably still missing something.
Considering the del operation should be under the writer lock,
if rt->rt6i_node should be NULL (for rt that has already been
removed from fib6), why this WARN_ON() is triggered?
An example may help.
Look at the stack trace, you'll find the answers...
ip6_link_failure() -> ip6_del_rt()
Note that rt might have been deleted from the _tree_ already.
On Mon, Sep 25, 2017 at 7:23 PM, Eric Dumazet [off-list ref] wrote:
On Mon, Sep 25, 2017 at 7:07 PM, Martin KaFai Lau [off-list ref] wrote:
quoted
I am probably still missing something.
Considering the del operation should be under the writer lock,
if rt->rt6i_node should be NULL (for rt that has already been
removed from fib6), why this WARN_ON() is triggered?
An example may help.
Look at the stack trace, you'll find the answers...
ip6_link_failure() -> ip6_del_rt()
Note that rt might have been deleted from the _tree_ already.
Had a brief talk with Martin.
He has a valid point.
The current WARN_ON() code is as follows:
#if RT6_DEBUG >= 2
if (rt->dst.obsolete > 0) {
WARN_ON(fn);
return -ENOENT;
}
#endif
The WARN_ON() only triggers when fn is not NULL. (I missed it before.)
In theory, fib6_del() calls fib6_del_route() which should set
rt->rt6i_node to NULL and rt->dst.obsolete to DST_OBSOLETE_DEAD within
the same write_lock session.
If those 2 values are inconsistent, it indicates something is wrong.
Will need more time to root cause the issue.
Please ignore this patch. Sorry about the confusion.
From: Eric Dumazet <edumazet@google.com> Date: 2017-09-26 13:20:32
On Mon, Sep 25, 2017 at 10:52 PM, Wei Wang [off-list ref] wrote:
On Mon, Sep 25, 2017 at 7:23 PM, Eric Dumazet [off-list ref] wrote:
quoted
On Mon, Sep 25, 2017 at 7:07 PM, Martin KaFai Lau [off-list ref] wrote:
quoted
I am probably still missing something.
Considering the del operation should be under the writer lock,
if rt->rt6i_node should be NULL (for rt that has already been
removed from fib6), why this WARN_ON() is triggered?
An example may help.
Look at the stack trace, you'll find the answers...
ip6_link_failure() -> ip6_del_rt()
Note that rt might have been deleted from the _tree_ already.
Had a brief talk with Martin.
He has a valid point.
The current WARN_ON() code is as follows:
#if RT6_DEBUG >= 2
if (rt->dst.obsolete > 0) {
WARN_ON(fn);
return -ENOENT;
}
#endif
The WARN_ON() only triggers when fn is not NULL. (I missed it before.)
In theory, fib6_del() calls fib6_del_route() which should set
rt->rt6i_node to NULL and rt->dst.obsolete to DST_OBSOLETE_DEAD within
the same write_lock session.
If those 2 values are inconsistent, it indicates something is wrong.
Will need more time to root cause the issue.
Please ignore this patch. Sorry about the confusion.
Oh well, for some reason I was seeing WARN_ON(1) here, since this is
a construct I often add in my tests ...
On Tue, Sep 26, 2017 at 6:20 AM, Eric Dumazet [off-list ref] wrote:
On Mon, Sep 25, 2017 at 10:52 PM, Wei Wang [off-list ref] wrote:
quoted
On Mon, Sep 25, 2017 at 7:23 PM, Eric Dumazet [off-list ref] wrote:
quoted
On Mon, Sep 25, 2017 at 7:07 PM, Martin KaFai Lau [off-list ref] wrote:
quoted
I am probably still missing something.
Considering the del operation should be under the writer lock,
if rt->rt6i_node should be NULL (for rt that has already been
removed from fib6), why this WARN_ON() is triggered?
An example may help.
Look at the stack trace, you'll find the answers...
ip6_link_failure() -> ip6_del_rt()
Note that rt might have been deleted from the _tree_ already.
Had a brief talk with Martin.
He has a valid point.
The current WARN_ON() code is as follows:
#if RT6_DEBUG >= 2
if (rt->dst.obsolete > 0) {
WARN_ON(fn);
return -ENOENT;
}
#endif
The WARN_ON() only triggers when fn is not NULL. (I missed it before.)
In theory, fib6_del() calls fib6_del_route() which should set
rt->rt6i_node to NULL and rt->dst.obsolete to DST_OBSOLETE_DEAD within
the same write_lock session.
If those 2 values are inconsistent, it indicates something is wrong.
Will need more time to root cause the issue.
Please ignore this patch. Sorry about the confusion.
Oh well, for some reason I was seeing WARN_ON(1) here, since this is
a construct I often add in my tests ...
Just an update on this issue:
This WARNING issue should already be fixed by commit
7483cea79957312e9f8e9cf760a1bc5d6c507113:
Author: Ido Schimmel [off-list ref]
Date: Thu Aug 3 13:28:22 2017 +0200
ipv6: fib: Unlink replaced routes from their nodes
When a route is deleted its node pointer is set to NULL to indicate it's
no longer linked to its node. Do the same for routes that are replaced.
This will later allow us to test if a route is still in the FIB by
checking its node pointer instead of its reference count.
Signed-off-by: Ido Schimmel [off-list ref]
Signed-off-by: Jiri Pirko [off-list ref]
Signed-off-by: David S. Miller [off-list ref]
So no further action is needed on this.
Thanks.
Wei