[PATCH rcu/dev 1/3] net: Use call_rcu_flush() for qdisc_free_cb

[PATCH rcu/dev 1/3] net: Use call_rcu_flush() for qdisc_free_cb

[Joel Fernandes (Google)]

In a networking test on ChromeOS, we find that using the new CONFIG_RCU_LAZY
causes a networking test to fail in the teardown phase.

The failure happens during: ip netns del <name>

Using ftrace, I found the callbacks it was queuing which this series fixes. Use
call_rcu_flush() to revert to the old behavior. With that, the test passes.

Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 net/sched/sch_generic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index a9aadc4e6858..63fbf640d3b2 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -1067,7 +1067,7 @@ static void qdisc_destroy(struct Qdisc *qdisc)
 
 	trace_qdisc_destroy(qdisc);
 
-	call_rcu(&qdisc->rcu, qdisc_free_cb);
+	call_rcu_flush(&qdisc->rcu, qdisc_free_cb);
 }
 
 void qdisc_put(struct Qdisc *qdisc)
-- 
2.38.1.584.g0f3c55d4c2-goog

[PATCH rcu/dev 2/3] net: Use call_rcu_flush() for in_dev_rcu_put

[Joel Fernandes (Google)]

In a networking test on ChromeOS, we find that using the new CONFIG_RCU_LAZY
causes a networking test to fail in the teardown phase.

The failure happens during: ip netns del <name>

Using ftrace, I found the callbacks it was queuing which this series fixes. Use
call_rcu_flush() to revert to the old behavior. With that, the test passes.

Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 net/ipv4/devinet.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index e8b9a9202fec..98b20f333e00 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -328,7 +328,7 @@ static void inetdev_destroy(struct in_device *in_dev)
 	neigh_parms_release(&arp_tbl, in_dev->arp_parms);
 	arp_ifdown(dev);
 
-	call_rcu(&in_dev->rcu_head, in_dev_rcu_put);
+	call_rcu_flush(&in_dev->rcu_head, in_dev_rcu_put);
 }
 
 int inet_addr_onlink(struct in_device *in_dev, __be32 a, __be32 b)
-- 
2.38.1.584.g0f3c55d4c2-goog

[PATCH rcu/dev 3/3] net: Use call_rcu_flush() for dst_destroy_rcu

[Joel Fernandes (Google)]

In a networking test on ChromeOS, we find that using the new CONFIG_RCU_LAZY
causes a networking test to fail in the teardown phase.

The failure happens during: ip netns del <name>

Using ftrace, I found the callbacks it was queuing which this series fixes. Use
call_rcu_flush() to revert to the old behavior. With that, the test passes.

Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 net/core/dst.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/dst.c b/net/core/dst.c
index bc9c9be4e080..15b16322703f 100644
--- a/net/core/dst.c
+++ b/net/core/dst.c
@@ -174,7 +174,7 @@ void dst_release(struct dst_entry *dst)
 			net_warn_ratelimited("%s: dst:%p refcnt:%d\n",
 					     __func__, dst, newrefcnt);
 		if (!newrefcnt)
-			call_rcu(&dst->rcu_head, dst_destroy_rcu);
+			call_rcu_flush(&dst->rcu_head, dst_destroy_rcu);
 	}
 }
 EXPORT_SYMBOL(dst_release);
-- 
2.38.1.584.g0f3c55d4c2-goog

Re: [PATCH rcu/dev 3/3] net: Use call_rcu_flush() for dst_destroy_rcu

[Eric Dumazet]

On Wed, Nov 16, 2022 at 7:16 PM Joel Fernandes (Google)
<joel@joelfernandes.org> wrote:
>
> In a networking test on ChromeOS, we find that using the new CONFIG_RCU_LAZY
> causes a networking test to fail in the teardown phase.
>
> The failure happens during: ip netns del <name>

And ? What happens then next ?

>
> Using ftrace, I found the callbacks it was queuing which this series fixes. Use
> call_rcu_flush() to revert to the old behavior. With that, the test passes.

What is this test about ? What barrier was used to make it not flaky ?

Was it depending on some undocumented RCU behavior ?

Maybe adding a sysctl to force the flush would be better for functional tests ?

I would rather change the test(s), than adding call_rcu_flush(),
adding merge conflicts to future backports.

Re: [PATCH rcu/dev 3/3] net: Use call_rcu_flush() for dst_destroy_rcu

[Joel Fernandes]

Hello Eric,

On Wed, Nov 16, 2022 at 07:44:41PM -0800, Eric Dumazet wrote:
> On Wed, Nov 16, 2022 at 7:16 PM Joel Fernandes (Google)
> <joel@joelfernandes.org> wrote:
> >
> > In a networking test on ChromeOS, we find that using the new CONFIG_RCU_LAZY
> > causes a networking test to fail in the teardown phase.
> >
> > The failure happens during: ip netns del <name>
> 
> And ? What happens then next ?

The test is doing the 'ip netns del <name>' and then polling for the
disappearance of a network interface name for upto 5 seconds. I believe it is
using netlink to get a table of interfaces. That polling is timing out.

Here is some more details from the test's owner (copy pasting from another
bug report):
In the cleanup, we remove the netns, and thus will cause the veth pair being
removed automatically, so we use a poll to check that if the veth in the root
netns still exists to know whether the cleanup is done.

Here is a public link to the code that is failing (its in golang):
https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform/tast-tests/src/chromiumos/tast/local/network/virtualnet/env/env.go;drc=6c2841d6cc3eadd23e07912ec331943ee33d7de8;l=161

Here is a public link to the line of code in the actual test leading up to the above
path (this is the test that is run:
network.RoutingFallthrough.ipv4_only_primary) :
https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform/tast-tests/src/chromiumos/tast/local/bundles/cros/network/routing_fallthrough.go;drc=8fbf2c53960bc8917a6a01fda5405cad7c17201e;l=52

> > Using ftrace, I found the callbacks it was queuing which this series fixes. Use
> > call_rcu_flush() to revert to the old behavior. With that, the test passes.
> 
> What is this test about ? What barrier was used to make it not flaky ?

I provided the links above, let me know if you have any questions.

> Was it depending on some undocumented RCU behavior ?

This is a new RCU feature posted here for significant power-savings on
battery-powered devices:
https://lore.kernel.org/rcu/20221017140726.GG5600@paulmck-ThinkPad-P17-Gen-1/T/#m7a54809b8903b41538850194d67eb34f203c752a

There is also an LPC presentation about the same, I can dig the link if you
are interested.

> Maybe adding a sysctl to force the flush would be better for functional tests ?
> 
> I would rather change the test(s), than adding call_rcu_flush(),
> adding merge conflicts to future backports.

I am not too sure about that, I think a user might expect the network
interface to disappear from the networking tables quickly enough without
dealing with barriers or kernel iternals. However, I added the authors of the
test to this email in the hopes he can provide is point of views as well.

The general approach we are taking with this sort of thing is to use
call_rcu_flush() which is basically the same as call_rcu() for systems with
CALL_RCU_LAZY=n. You can see some examples of that in the patch series link
above. Just to note, CALL_RCU_LAZY depends on CONFIG_RCU_NOCB_CPU so its only
Android and ChromeOS that are using it. I am adding Jie to share any input,
he is from the networking team and knows this test well.

thanks,

 - Joel

Re: [PATCH rcu/dev 3/3] net: Use call_rcu_flush() for dst_destroy_rcu

[Eric Dumazet]

On Thu, Nov 17, 2022 at 7:58 AM Joel Fernandes <joel@joelfernandes.org> wrote:
>
> Hello Eric,
>
> On Wed, Nov 16, 2022 at 07:44:41PM -0800, Eric Dumazet wrote:
> > On Wed, Nov 16, 2022 at 7:16 PM Joel Fernandes (Google)
> > <joel@joelfernandes.org> wrote:
> > >
> > > In a networking test on ChromeOS, we find that using the new CONFIG_RCU_LAZY
> > > causes a networking test to fail in the teardown phase.
> > >
> > > The failure happens during: ip netns del <name>
> >
> > And ? What happens then next ?
>
> The test is doing the 'ip netns del <name>' and then polling for the
> disappearance of a network interface name for upto 5 seconds. I believe it is
> using netlink to get a table of interfaces. That polling is timing out.
>
> Here is some more details from the test's owner (copy pasting from another
> bug report):
> In the cleanup, we remove the netns, and thus will cause the veth pair being
> removed automatically, so we use a poll to check that if the veth in the root
> netns still exists to know whether the cleanup is done.
>
> Here is a public link to the code that is failing (its in golang):
> https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform/tast-tests/src/chromiumos/tast/local/network/virtualnet/env/env.go;drc=6c2841d6cc3eadd23e07912ec331943ee33d7de8;l=161
>
> Here is a public link to the line of code in the actual test leading up to the above
> path (this is the test that is run:
> network.RoutingFallthrough.ipv4_only_primary) :
> https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform/tast-tests/src/chromiumos/tast/local/bundles/cros/network/routing_fallthrough.go;drc=8fbf2c53960bc8917a6a01fda5405cad7c17201e;l=52
>
> > > Using ftrace, I found the callbacks it was queuing which this series fixes. Use
> > > call_rcu_flush() to revert to the old behavior. With that, the test passes.
> >
> > What is this test about ? What barrier was used to make it not flaky ?
>
> I provided the links above, let me know if you have any questions.
>
> > Was it depending on some undocumented RCU behavior ?
>
> This is a new RCU feature posted here for significant power-savings on
> battery-powered devices:
> https://lore.kernel.org/rcu/20221017140726.GG5600@paulmck-ThinkPad-P17-Gen-1/T/#m7a54809b8903b41538850194d67eb34f203c752a
>
> There is also an LPC presentation about the same, I can dig the link if you
> are interested.
>
> > Maybe adding a sysctl to force the flush would be better for functional tests ?
> >
> > I would rather change the test(s), than adding call_rcu_flush(),
> > adding merge conflicts to future backports.
>
> I am not too sure about that, I think a user might expect the network
> interface to disappear from the networking tables quickly enough without
> dealing with barriers or kernel iternals. However, I added the authors of the
> test to this email in the hopes he can provide is point of views as well.
>
> The general approach we are taking with this sort of thing is to use
> call_rcu_flush() which is basically the same as call_rcu() for systems with
> CALL_RCU_LAZY=n. You can see some examples of that in the patch series link
> above. Just to note, CALL_RCU_LAZY depends on CONFIG_RCU_NOCB_CPU so its only
> Android and ChromeOS that are using it. I am adding Jie to share any input,
> he is from the networking team and knows this test well.
>
>

I do not know what is this RCU_LAZY thing, but IMO this should be opt-in

For instance, only kfree_rcu() should use it.

We can not review hundreds of call_rcu() call sites and decide if
adding arbitrary delays cou hurt .

Re: [PATCH rcu/dev 3/3] net: Use call_rcu_flush() for dst_destroy_rcu

[Eric Dumazet]

On Thu, Nov 17, 2022 at 9:17 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Thu, Nov 17, 2022 at 7:58 AM Joel Fernandes <joel@joelfernandes.org> wrote:
> >
> > Hello Eric,
> >
> > On Wed, Nov 16, 2022 at 07:44:41PM -0800, Eric Dumazet wrote:
> > > On Wed, Nov 16, 2022 at 7:16 PM Joel Fernandes (Google)
> > > <joel@joelfernandes.org> wrote:
> > > >
> > > > In a networking test on ChromeOS, we find that using the new CONFIG_RCU_LAZY
> > > > causes a networking test to fail in the teardown phase.
> > > >
> > > > The failure happens during: ip netns del <name>
> > >
> > > And ? What happens then next ?
> >
> > The test is doing the 'ip netns del <name>' and then polling for the
> > disappearance of a network interface name for upto 5 seconds. I believe it is
> > using netlink to get a table of interfaces. That polling is timing out.
> >
> > Here is some more details from the test's owner (copy pasting from another
> > bug report):
> > In the cleanup, we remove the netns, and thus will cause the veth pair being
> > removed automatically, so we use a poll to check that if the veth in the root
> > netns still exists to know whether the cleanup is done.
> >
> > Here is a public link to the code that is failing (its in golang):
> > https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform/tast-tests/src/chromiumos/tast/local/network/virtualnet/env/env.go;drc=6c2841d6cc3eadd23e07912ec331943ee33d7de8;l=161
> >
> > Here is a public link to the line of code in the actual test leading up to the above
> > path (this is the test that is run:
> > network.RoutingFallthrough.ipv4_only_primary) :
> > https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform/tast-tests/src/chromiumos/tast/local/bundles/cros/network/routing_fallthrough.go;drc=8fbf2c53960bc8917a6a01fda5405cad7c17201e;l=52
> >
> > > > Using ftrace, I found the callbacks it was queuing which this series fixes. Use
> > > > call_rcu_flush() to revert to the old behavior. With that, the test passes.
> > >
> > > What is this test about ? What barrier was used to make it not flaky ?
> >
> > I provided the links above, let me know if you have any questions.
> >
> > > Was it depending on some undocumented RCU behavior ?
> >
> > This is a new RCU feature posted here for significant power-savings on
> > battery-powered devices:
> > https://lore.kernel.org/rcu/20221017140726.GG5600@paulmck-ThinkPad-P17-Gen-1/T/#m7a54809b8903b41538850194d67eb34f203c752a
> >
> > There is also an LPC presentation about the same, I can dig the link if you
> > are interested.
> >
> > > Maybe adding a sysctl to force the flush would be better for functional tests ?
> > >
> > > I would rather change the test(s), than adding call_rcu_flush(),
> > > adding merge conflicts to future backports.
> >
> > I am not too sure about that, I think a user might expect the network
> > interface to disappear from the networking tables quickly enough without
> > dealing with barriers or kernel iternals. However, I added the authors of the
> > test to this email in the hopes he can provide is point of views as well.
> >
> > The general approach we are taking with this sort of thing is to use
> > call_rcu_flush() which is basically the same as call_rcu() for systems with
> > CALL_RCU_LAZY=n. You can see some examples of that in the patch series link
> > above. Just to note, CALL_RCU_LAZY depends on CONFIG_RCU_NOCB_CPU so its only
> > Android and ChromeOS that are using it. I am adding Jie to share any input,
> > he is from the networking team and knows this test well.
> >
> >
>
> I do not know what is this RCU_LAZY thing, but IMO this should be opt-in
>
> For instance, only kfree_rcu() should use it.
>
> We can not review hundreds of call_rcu() call sites and decide if
> adding arbitrary delays cou hurt .

At a very minimum, things like rcu_barrier() should make sure that all
'lazy' callbacks are processed in a reasonable amount of time.

Re: [PATCH rcu/dev 3/3] net: Use call_rcu_flush() for dst_destroy_rcu

[Joel Fernandes]

On Thu, Nov 17, 2022 at 5:17 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Thu, Nov 17, 2022 at 7:58 AM Joel Fernandes <joel@joelfernandes.org> wrote:
> >
> > Hello Eric,
> >
> > On Wed, Nov 16, 2022 at 07:44:41PM -0800, Eric Dumazet wrote:
> > > On Wed, Nov 16, 2022 at 7:16 PM Joel Fernandes (Google)
> > > <joel@joelfernandes.org> wrote:
> > > >
> > > > In a networking test on ChromeOS, we find that using the new CONFIG_RCU_LAZY
> > > > causes a networking test to fail in the teardown phase.
> > > >
> > > > The failure happens during: ip netns del <name>
> > >
> > > And ? What happens then next ?
> >
> > The test is doing the 'ip netns del <name>' and then polling for the
> > disappearance of a network interface name for upto 5 seconds. I believe it is
> > using netlink to get a table of interfaces. That polling is timing out.
> >
> > Here is some more details from the test's owner (copy pasting from another
> > bug report):
> > In the cleanup, we remove the netns, and thus will cause the veth pair being
> > removed automatically, so we use a poll to check that if the veth in the root
> > netns still exists to know whether the cleanup is done.
> >
> > Here is a public link to the code that is failing (its in golang):
> > https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform/tast-tests/src/chromiumos/tast/local/network/virtualnet/env/env.go;drc=6c2841d6cc3eadd23e07912ec331943ee33d7de8;l=161
> >
> > Here is a public link to the line of code in the actual test leading up to the above
> > path (this is the test that is run:
> > network.RoutingFallthrough.ipv4_only_primary) :
> > https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform/tast-tests/src/chromiumos/tast/local/bundles/cros/network/routing_fallthrough.go;drc=8fbf2c53960bc8917a6a01fda5405cad7c17201e;l=52
> >
> > > > Using ftrace, I found the callbacks it was queuing which this series fixes. Use
> > > > call_rcu_flush() to revert to the old behavior. With that, the test passes.
> > >
> > > What is this test about ? What barrier was used to make it not flaky ?
> >
> > I provided the links above, let me know if you have any questions.
> >
> > > Was it depending on some undocumented RCU behavior ?
> >
> > This is a new RCU feature posted here for significant power-savings on
> > battery-powered devices:
> > https://lore.kernel.org/rcu/20221017140726.GG5600@paulmck-ThinkPad-P17-Gen-1/T/#m7a54809b8903b41538850194d67eb34f203c752a
> >
> > There is also an LPC presentation about the same, I can dig the link if you
> > are interested.
> >
> > > Maybe adding a sysctl to force the flush would be better for functional tests ?
> > >
> > > I would rather change the test(s), than adding call_rcu_flush(),
> > > adding merge conflicts to future backports.
> >
> > I am not too sure about that, I think a user might expect the network
> > interface to disappear from the networking tables quickly enough without
> > dealing with barriers or kernel iternals. However, I added the authors of the
> > test to this email in the hopes he can provide is point of views as well.
> >
> > The general approach we are taking with this sort of thing is to use
> > call_rcu_flush() which is basically the same as call_rcu() for systems with
> > CALL_RCU_LAZY=n. You can see some examples of that in the patch series link
> > above. Just to note, CALL_RCU_LAZY depends on CONFIG_RCU_NOCB_CPU so its only
> > Android and ChromeOS that are using it. I am adding Jie to share any input,
> > he is from the networking team and knows this test well.
> >
> >
>
> I do not know what is this RCU_LAZY thing, but IMO this should be opt-in

You should read the links I sent you. We did already try opt-in,
Thomas Gleixner made a point at LPC that we should not add new APIs
for this purpose and confuse kernel developers.

> For instance, only kfree_rcu() should use it.

No. Most of the call_rcu() usages are for freeing memory, so the
consensus is we should apply this as opt out and fix issues along the
way. We already did a lot of research/diligence on seeing which users
need conversion.

> We can not review hundreds of call_rcu() call sites and decide if
> adding arbitrary delays cou hurt .

That work has already been done as much as possible, please read the
links I sent.

Thanks.

Re: [PATCH rcu/dev 3/3] net: Use call_rcu_flush() for dst_destroy_rcu

[Eric Dumazet]

On Thu, Nov 17, 2022 at 9:38 AM Joel Fernandes <joel@joelfernandes.org> wrote:
>
> On Thu, Nov 17, 2022 at 5:17 PM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Thu, Nov 17, 2022 at 7:58 AM Joel Fernandes <joel@joelfernandes.org> wrote:
> > >
> > > Hello Eric,
> > >
> > > On Wed, Nov 16, 2022 at 07:44:41PM -0800, Eric Dumazet wrote:
> > > > On Wed, Nov 16, 2022 at 7:16 PM Joel Fernandes (Google)
> > > > <joel@joelfernandes.org> wrote:
> > > > >
> > > > > In a networking test on ChromeOS, we find that using the new CONFIG_RCU_LAZY
> > > > > causes a networking test to fail in the teardown phase.
> > > > >
> > > > > The failure happens during: ip netns del <name>
> > > >
> > > > And ? What happens then next ?
> > >
> > > The test is doing the 'ip netns del <name>' and then polling for the
> > > disappearance of a network interface name for upto 5 seconds. I believe it is
> > > using netlink to get a table of interfaces. That polling is timing out.
> > >
> > > Here is some more details from the test's owner (copy pasting from another
> > > bug report):
> > > In the cleanup, we remove the netns, and thus will cause the veth pair being
> > > removed automatically, so we use a poll to check that if the veth in the root
> > > netns still exists to know whether the cleanup is done.
> > >
> > > Here is a public link to the code that is failing (its in golang):
> > > https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform/tast-tests/src/chromiumos/tast/local/network/virtualnet/env/env.go;drc=6c2841d6cc3eadd23e07912ec331943ee33d7de8;l=161
> > >
> > > Here is a public link to the line of code in the actual test leading up to the above
> > > path (this is the test that is run:
> > > network.RoutingFallthrough.ipv4_only_primary) :
> > > https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform/tast-tests/src/chromiumos/tast/local/bundles/cros/network/routing_fallthrough.go;drc=8fbf2c53960bc8917a6a01fda5405cad7c17201e;l=52
> > >
> > > > > Using ftrace, I found the callbacks it was queuing which this series fixes. Use
> > > > > call_rcu_flush() to revert to the old behavior. With that, the test passes.
> > > >
> > > > What is this test about ? What barrier was used to make it not flaky ?
> > >
> > > I provided the links above, let me know if you have any questions.
> > >
> > > > Was it depending on some undocumented RCU behavior ?
> > >
> > > This is a new RCU feature posted here for significant power-savings on
> > > battery-powered devices:
> > > https://lore.kernel.org/rcu/20221017140726.GG5600@paulmck-ThinkPad-P17-Gen-1/T/#m7a54809b8903b41538850194d67eb34f203c752a
> > >
> > > There is also an LPC presentation about the same, I can dig the link if you
> > > are interested.
> > >
> > > > Maybe adding a sysctl to force the flush would be better for functional tests ?
> > > >
> > > > I would rather change the test(s), than adding call_rcu_flush(),
> > > > adding merge conflicts to future backports.
> > >
> > > I am not too sure about that, I think a user might expect the network
> > > interface to disappear from the networking tables quickly enough without
> > > dealing with barriers or kernel iternals. However, I added the authors of the
> > > test to this email in the hopes he can provide is point of views as well.
> > >
> > > The general approach we are taking with this sort of thing is to use
> > > call_rcu_flush() which is basically the same as call_rcu() for systems with
> > > CALL_RCU_LAZY=n. You can see some examples of that in the patch series link
> > > above. Just to note, CALL_RCU_LAZY depends on CONFIG_RCU_NOCB_CPU so its only
> > > Android and ChromeOS that are using it. I am adding Jie to share any input,
> > > he is from the networking team and knows this test well.
> > >
> > >
> >
> > I do not know what is this RCU_LAZY thing, but IMO this should be opt-in
>
> You should read the links I sent you. We did already try opt-in,
> Thomas Gleixner made a point at LPC that we should not add new APIs
> for this purpose and confuse kernel developers.
>
> > For instance, only kfree_rcu() should use it.
>
> No. Most of the call_rcu() usages are for freeing memory, so the
> consensus is we should apply this as opt out and fix issues along the
> way. We already did a lot of research/diligence on seeing which users
> need conversion.
>
> > We can not review hundreds of call_rcu() call sites and decide if
> > adding arbitrary delays cou hurt .
>
> That work has already been done as much as possible, please read the
> links I sent.

Oh well. No.

I will leave it to other folks dealing with this crazy thing.

Re: [PATCH rcu/dev 3/3] net: Use call_rcu_flush() for dst_destroy_rcu

[Joel Fernandes]

On Thu, Nov 17, 2022 at 5:38 PM Joel Fernandes <joel@joelfernandes.org> wrote:
>
> On Thu, Nov 17, 2022 at 5:17 PM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Thu, Nov 17, 2022 at 7:58 AM Joel Fernandes <joel@joelfernandes.org> wrote:
> > >
> > > Hello Eric,
> > >
> > > On Wed, Nov 16, 2022 at 07:44:41PM -0800, Eric Dumazet wrote:
> > > > On Wed, Nov 16, 2022 at 7:16 PM Joel Fernandes (Google)
> > > > <joel@joelfernandes.org> wrote:
> > > > >
> > > > > In a networking test on ChromeOS, we find that using the new CONFIG_RCU_LAZY
> > > > > causes a networking test to fail in the teardown phase.
> > > > >
> > > > > The failure happens during: ip netns del <name>
> > > >
> > > > And ? What happens then next ?
> > >
> > > The test is doing the 'ip netns del <name>' and then polling for the
> > > disappearance of a network interface name for upto 5 seconds. I believe it is
> > > using netlink to get a table of interfaces. That polling is timing out.
> > >
> > > Here is some more details from the test's owner (copy pasting from another
> > > bug report):
> > > In the cleanup, we remove the netns, and thus will cause the veth pair being
> > > removed automatically, so we use a poll to check that if the veth in the root
> > > netns still exists to know whether the cleanup is done.
> > >
> > > Here is a public link to the code that is failing (its in golang):
> > > https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform/tast-tests/src/chromiumos/tast/local/network/virtualnet/env/env.go;drc=6c2841d6cc3eadd23e07912ec331943ee33d7de8;l=161
> > >
> > > Here is a public link to the line of code in the actual test leading up to the above
> > > path (this is the test that is run:
> > > network.RoutingFallthrough.ipv4_only_primary) :
> > > https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform/tast-tests/src/chromiumos/tast/local/bundles/cros/network/routing_fallthrough.go;drc=8fbf2c53960bc8917a6a01fda5405cad7c17201e;l=52
> > >
> > > > > Using ftrace, I found the callbacks it was queuing which this series fixes. Use
> > > > > call_rcu_flush() to revert to the old behavior. With that, the test passes.
> > > >
> > > > What is this test about ? What barrier was used to make it not flaky ?
> > >
> > > I provided the links above, let me know if you have any questions.
> > >
> > > > Was it depending on some undocumented RCU behavior ?
> > >
> > > This is a new RCU feature posted here for significant power-savings on
> > > battery-powered devices:
> > > https://lore.kernel.org/rcu/20221017140726.GG5600@paulmck-ThinkPad-P17-Gen-1/T/#m7a54809b8903b41538850194d67eb34f203c752a
> > >
> > > There is also an LPC presentation about the same, I can dig the link if you
> > > are interested.
> > >
> > > > Maybe adding a sysctl to force the flush would be better for functional tests ?
> > > >
> > > > I would rather change the test(s), than adding call_rcu_flush(),
> > > > adding merge conflicts to future backports.
> > >
> > > I am not too sure about that, I think a user might expect the network
> > > interface to disappear from the networking tables quickly enough without
> > > dealing with barriers or kernel iternals. However, I added the authors of the
> > > test to this email in the hopes he can provide is point of views as well.
> > >
> > > The general approach we are taking with this sort of thing is to use
> > > call_rcu_flush() which is basically the same as call_rcu() for systems with
> > > CALL_RCU_LAZY=n. You can see some examples of that in the patch series link
> > > above. Just to note, CALL_RCU_LAZY depends on CONFIG_RCU_NOCB_CPU so its only
> > > Android and ChromeOS that are using it. I am adding Jie to share any input,
> > > he is from the networking team and knows this test well.
> > >
> > >
> >
> > I do not know what is this RCU_LAZY thing, but IMO this should be opt-in
>
> You should read the links I sent you. We did already try opt-in,
> Thomas Gleixner made a point at LPC that we should not add new APIs
> for this purpose and confuse kernel developers.
>
> > For instance, only kfree_rcu() should use it.
>
> No. Most of the call_rcu() usages are for freeing memory, so the
> consensus is we should apply this as opt out and fix issues along the
> way. We already did a lot of research/diligence on seeing which users
> need conversion.
>
> > We can not review hundreds of call_rcu() call sites and decide if
> > adding arbitrary delays cou hurt .
>
> That work has already been done as much as possible, please read the
> links I sent.

Also just to add, this test is a bit weird / corner case, as in anyone
expecting a quick response from call_rcu() is broken by design.
However, for these callbacks, it does not matter much which API they
use as they are quite infrequent for power savings.

Thanks.

Re: [PATCH rcu/dev 3/3] net: Use call_rcu_flush() for dst_destroy_rcu

[Joel Fernandes]

On Thu, Nov 17, 2022 at 5:40 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Thu, Nov 17, 2022 at 9:38 AM Joel Fernandes <joel@joelfernandes.org> wrote:
> >
> > On Thu, Nov 17, 2022 at 5:17 PM Eric Dumazet <edumazet@google.com> wrote:
> > >
> > > On Thu, Nov 17, 2022 at 7:58 AM Joel Fernandes <joel@joelfernandes.org> wrote:
> > > >
> > > > Hello Eric,
> > > >
> > > > On Wed, Nov 16, 2022 at 07:44:41PM -0800, Eric Dumazet wrote:
> > > > > On Wed, Nov 16, 2022 at 7:16 PM Joel Fernandes (Google)
> > > > > <joel@joelfernandes.org> wrote:
> > > > > >
> > > > > > In a networking test on ChromeOS, we find that using the new CONFIG_RCU_LAZY
> > > > > > causes a networking test to fail in the teardown phase.
> > > > > >
> > > > > > The failure happens during: ip netns del <name>
> > > > >
> > > > > And ? What happens then next ?
> > > >
> > > > The test is doing the 'ip netns del <name>' and then polling for the
> > > > disappearance of a network interface name for upto 5 seconds. I believe it is
> > > > using netlink to get a table of interfaces. That polling is timing out.
> > > >
> > > > Here is some more details from the test's owner (copy pasting from another
> > > > bug report):
> > > > In the cleanup, we remove the netns, and thus will cause the veth pair being
> > > > removed automatically, so we use a poll to check that if the veth in the root
> > > > netns still exists to know whether the cleanup is done.
> > > >
> > > > Here is a public link to the code that is failing (its in golang):
> > > > https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform/tast-tests/src/chromiumos/tast/local/network/virtualnet/env/env.go;drc=6c2841d6cc3eadd23e07912ec331943ee33d7de8;l=161
> > > >
> > > > Here is a public link to the line of code in the actual test leading up to the above
> > > > path (this is the test that is run:
> > > > network.RoutingFallthrough.ipv4_only_primary) :
> > > > https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform/tast-tests/src/chromiumos/tast/local/bundles/cros/network/routing_fallthrough.go;drc=8fbf2c53960bc8917a6a01fda5405cad7c17201e;l=52
> > > >
> > > > > > Using ftrace, I found the callbacks it was queuing which this series fixes. Use
> > > > > > call_rcu_flush() to revert to the old behavior. With that, the test passes.
> > > > >
> > > > > What is this test about ? What barrier was used to make it not flaky ?
> > > >
> > > > I provided the links above, let me know if you have any questions.
> > > >
> > > > > Was it depending on some undocumented RCU behavior ?
> > > >
> > > > This is a new RCU feature posted here for significant power-savings on
> > > > battery-powered devices:
> > > > https://lore.kernel.org/rcu/20221017140726.GG5600@paulmck-ThinkPad-P17-Gen-1/T/#m7a54809b8903b41538850194d67eb34f203c752a
> > > >
> > > > There is also an LPC presentation about the same, I can dig the link if you
> > > > are interested.
> > > >
> > > > > Maybe adding a sysctl to force the flush would be better for functional tests ?
> > > > >
> > > > > I would rather change the test(s), than adding call_rcu_flush(),
> > > > > adding merge conflicts to future backports.
> > > >
> > > > I am not too sure about that, I think a user might expect the network
> > > > interface to disappear from the networking tables quickly enough without
> > > > dealing with barriers or kernel iternals. However, I added the authors of the
> > > > test to this email in the hopes he can provide is point of views as well.
> > > >
> > > > The general approach we are taking with this sort of thing is to use
> > > > call_rcu_flush() which is basically the same as call_rcu() for systems with
> > > > CALL_RCU_LAZY=n. You can see some examples of that in the patch series link
> > > > above. Just to note, CALL_RCU_LAZY depends on CONFIG_RCU_NOCB_CPU so its only
> > > > Android and ChromeOS that are using it. I am adding Jie to share any input,
> > > > he is from the networking team and knows this test well.
> > > >
> > > >
> > >
> > > I do not know what is this RCU_LAZY thing, but IMO this should be opt-in
> >
> > You should read the links I sent you. We did already try opt-in,
> > Thomas Gleixner made a point at LPC that we should not add new APIs
> > for this purpose and confuse kernel developers.
> >
> > > For instance, only kfree_rcu() should use it.
> >
> > No. Most of the call_rcu() usages are for freeing memory, so the
> > consensus is we should apply this as opt out and fix issues along the
> > way. We already did a lot of research/diligence on seeing which users
> > need conversion.
> >
> > > We can not review hundreds of call_rcu() call sites and decide if
> > > adding arbitrary delays cou hurt .
> >
> > That work has already been done as much as possible, please read the
> > links I sent.
>
> Oh well. No.
>
> I will leave it to other folks dealing with this crazy thing.

Yes, I agree. Your comments here have not been useful (or respectful)
so I am Ok with that.

 - Joel

Re: [PATCH rcu/dev 3/3] net: Use call_rcu_flush() for dst_destroy_rcu

[Eric Dumazet]

On Thu, Nov 17, 2022 at 9:42 AM Joel Fernandes <joel@joelfernandes.org> wrote:
>

>
> Yes, I agree. Your comments here have not been useful (or respectful)
> so I am Ok with that.
>
>  - Joel

Well, I have discovered that some changes went in networking tree
without network maintainers being involved nor CCed.

What can I say ?

It seems I have no say, right ?


commit f32846476afbe1f296c41d036219178b3dfb6a9d
Author: Joel Fernandes (Google) <joel@joelfernandes.org>
Date:   Sun Oct 16 16:23:04 2022 +0000

    rxrpc: Use call_rcu_flush() instead of call_rcu()

    call_rcu() changes to save power may cause slowness. Use the
    call_rcu_flush() API instead which reverts to the old behavior.

    We find this via inspection that the RCU callback does a wakeup of a
    thread. This usually indicates that something is waiting on it. To be
    safe, let us use call_rcu_flush() here instead.

    Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
    Signed-off-by: Paul E. McKenney <paulmck@kernel.org>

diff --git a/net/rxrpc/conn_object.c b/net/rxrpc/conn_object.c
index 22089e37e97f0628f780855f9e219e5c33d4afa1..fdcfb509cc4434b0781b76623532aff9c854ce60
100644
--- a/net/rxrpc/conn_object.c
+++ b/net/rxrpc/conn_object.c
@@ -253,7 +253,7 @@ void rxrpc_kill_connection(struct rxrpc_connection *conn)
         * must carry a ref on the connection to prevent us getting here whilst
         * it is queued or running.
         */
-       call_rcu(&conn->rcu, rxrpc_destroy_connection);
+       call_rcu_flush(&conn->rcu, rxrpc_destroy_connection);
 }

 /*

Re: [PATCH rcu/dev 3/3] net: Use call_rcu_flush() for dst_destroy_rcu

[Joel Fernandes]

On Thu, Nov 17, 2022 at 5:49 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Thu, Nov 17, 2022 at 9:42 AM Joel Fernandes <joel@joelfernandes.org> wrote:
> >
>
> >
> > Yes, I agree. Your comments here have not been useful (or respectful)
> > so I am Ok with that.
> >
> >  - Joel
>
> Well, I have discovered that some changes went in networking tree
> without network maintainers being involved nor CCed.
>
> What can I say ?
>
> It seems I have no say, right ?

Sorry, I take responsibility for that. FWIW, the rxrpc change is not
yet in Linus's tree.

Also FWIW, the rxrpc case came up because we detected that it does a
scheduler wakeup from the callback. We did both static and dynamic
testing to identify callbacks that do wakeups throughout the kernel
(kernel patch available on request), as the pattern observed is things
doing wakeups typically are for use cases that are not freeing memory
but something blocking, similar to synchronize_rcu(). So it was a
"trivial/obvious" change to make for rxrpc which I might have assumed
did not need much supervision because it just reverts that API to the
old behavior -- still probably no excuse.

Again, we can talk this out no problem. But I would strongly recommend
not calling it "crazy thing", as we did all due diligence for almost a
year (talking about it at LPC, working through various code paths and
bugs, 4 different patch redesigns on the idea (including the opt-in
that you are bringing up), including a late night debugging session to
figure this out etc).

Just to clarify, I know you review/maintain a lot of the networking
code and I really appreciate that (not praising just for the sake).
And I care about the kernel too, just like you.

thanks,

 - Joel


> commit f32846476afbe1f296c41d036219178b3dfb6a9d
> Author: Joel Fernandes (Google) <joel@joelfernandes.org>
> Date:   Sun Oct 16 16:23:04 2022 +0000
>
>     rxrpc: Use call_rcu_flush() instead of call_rcu()
>
>     call_rcu() changes to save power may cause slowness. Use the
>     call_rcu_flush() API instead which reverts to the old behavior.
>
>     We find this via inspection that the RCU callback does a wakeup of a
>     thread. This usually indicates that something is waiting on it. To be
>     safe, let us use call_rcu_flush() here instead.
>
>     Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>     Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
>
> diff --git a/net/rxrpc/conn_object.c b/net/rxrpc/conn_object.c
> index 22089e37e97f0628f780855f9e219e5c33d4afa1..fdcfb509cc4434b0781b76623532aff9c854ce60
> 100644
> --- a/net/rxrpc/conn_object.c
> +++ b/net/rxrpc/conn_object.c
> @@ -253,7 +253,7 @@ void rxrpc_kill_connection(struct rxrpc_connection *conn)
>          * must carry a ref on the connection to prevent us getting here whilst
>          * it is queued or running.
>          */
> -       call_rcu(&conn->rcu, rxrpc_destroy_connection);
> +       call_rcu_flush(&conn->rcu, rxrpc_destroy_connection);
>  }
>
>  /*

Re: [PATCH rcu/dev 3/3] net: Use call_rcu_flush() for dst_destroy_rcu

[Eric Dumazet]

On Thu, Nov 17, 2022 at 10:18 AM Joel Fernandes <joel@joelfernandes.org> wrote:
>
> On Thu, Nov 17, 2022 at 5:49 PM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Thu, Nov 17, 2022 at 9:42 AM Joel Fernandes <joel@joelfernandes.org> wrote:
> > >
> >
> > >
> > > Yes, I agree. Your comments here have not been useful (or respectful)
> > > so I am Ok with that.
> > >
> > >  - Joel
> >
> > Well, I have discovered that some changes went in networking tree
> > without network maintainers being involved nor CCed.
> >
> > What can I say ?
> >
> > It seems I have no say, right ?
>
> Sorry, I take responsibility for that. FWIW, the rxrpc change is not
> yet in Linus's tree.
>
> Also FWIW, the rxrpc case came up because we detected that it does a
> scheduler wakeup from the callback. We did both static and dynamic
> testing to identify callbacks that do wakeups throughout the kernel
> (kernel patch available on request), as the pattern observed is things
> doing wakeups typically are for use cases that are not freeing memory
> but something blocking, similar to synchronize_rcu(). So it was a
> "trivial/obvious" change to make for rxrpc which I might have assumed
> did not need much supervision because it just reverts that API to the
> old behavior -- still probably no excuse.
>
> Again, we can talk this out no problem. But I would strongly recommend
> not calling it "crazy thing", as we did all due diligence for almost a
> year (talking about it at LPC, working through various code paths and
> bugs, 4 different patch redesigns on the idea (including the opt-in
> that you are bringing up), including a late night debugging session to
> figure this out etc).

Apologies.

For me "crazy" does not have the same meaning, apparently.

I will try to use more neutral words in the future.

>
> Just to clarify, I know you review/maintain a lot of the networking
> code and I really appreciate that (not praising just for the sake).
> And I care about the kernel too, just like you.

I had no doubts about that, really.

Re: [PATCH rcu/dev 3/3] net: Use call_rcu_flush() for dst_destroy_rcu

[Paul E. McKenney]

On Thu, Nov 17, 2022 at 05:40:40PM +0000, Joel Fernandes wrote:
> On Thu, Nov 17, 2022 at 5:38 PM Joel Fernandes <joel@joelfernandes.org> wrote:
> >
> > On Thu, Nov 17, 2022 at 5:17 PM Eric Dumazet <edumazet@google.com> wrote:
> > >
> > > On Thu, Nov 17, 2022 at 7:58 AM Joel Fernandes <joel@joelfernandes.org> wrote:
> > > >
> > > > Hello Eric,
> > > >
> > > > On Wed, Nov 16, 2022 at 07:44:41PM -0800, Eric Dumazet wrote:
> > > > > On Wed, Nov 16, 2022 at 7:16 PM Joel Fernandes (Google)
> > > > > <joel@joelfernandes.org> wrote:
> > > > > >
> > > > > > In a networking test on ChromeOS, we find that using the new CONFIG_RCU_LAZY
> > > > > > causes a networking test to fail in the teardown phase.
> > > > > >
> > > > > > The failure happens during: ip netns del <name>
> > > > >
> > > > > And ? What happens then next ?
> > > >
> > > > The test is doing the 'ip netns del <name>' and then polling for the
> > > > disappearance of a network interface name for upto 5 seconds. I believe it is
> > > > using netlink to get a table of interfaces. That polling is timing out.
> > > >
> > > > Here is some more details from the test's owner (copy pasting from another
> > > > bug report):
> > > > In the cleanup, we remove the netns, and thus will cause the veth pair being
> > > > removed automatically, so we use a poll to check that if the veth in the root
> > > > netns still exists to know whether the cleanup is done.
> > > >
> > > > Here is a public link to the code that is failing (its in golang):
> > > > https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform/tast-tests/src/chromiumos/tast/local/network/virtualnet/env/env.go;drc=6c2841d6cc3eadd23e07912ec331943ee33d7de8;l=161
> > > >
> > > > Here is a public link to the line of code in the actual test leading up to the above
> > > > path (this is the test that is run:
> > > > network.RoutingFallthrough.ipv4_only_primary) :
> > > > https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform/tast-tests/src/chromiumos/tast/local/bundles/cros/network/routing_fallthrough.go;drc=8fbf2c53960bc8917a6a01fda5405cad7c17201e;l=52
> > > >
> > > > > > Using ftrace, I found the callbacks it was queuing which this series fixes. Use
> > > > > > call_rcu_flush() to revert to the old behavior. With that, the test passes.
> > > > >
> > > > > What is this test about ? What barrier was used to make it not flaky ?
> > > >
> > > > I provided the links above, let me know if you have any questions.
> > > >
> > > > > Was it depending on some undocumented RCU behavior ?
> > > >
> > > > This is a new RCU feature posted here for significant power-savings on
> > > > battery-powered devices:
> > > > https://lore.kernel.org/rcu/20221017140726.GG5600@paulmck-ThinkPad-P17-Gen-1/T/#m7a54809b8903b41538850194d67eb34f203c752a
> > > >
> > > > There is also an LPC presentation about the same, I can dig the link if you
> > > > are interested.
> > > >
> > > > > Maybe adding a sysctl to force the flush would be better for functional tests ?
> > > > >
> > > > > I would rather change the test(s), than adding call_rcu_flush(),
> > > > > adding merge conflicts to future backports.
> > > >
> > > > I am not too sure about that, I think a user might expect the network
> > > > interface to disappear from the networking tables quickly enough without
> > > > dealing with barriers or kernel iternals. However, I added the authors of the
> > > > test to this email in the hopes he can provide is point of views as well.
> > > >
> > > > The general approach we are taking with this sort of thing is to use
> > > > call_rcu_flush() which is basically the same as call_rcu() for systems with
> > > > CALL_RCU_LAZY=n. You can see some examples of that in the patch series link
> > > > above. Just to note, CALL_RCU_LAZY depends on CONFIG_RCU_NOCB_CPU so its only
> > > > Android and ChromeOS that are using it. I am adding Jie to share any input,
> > > > he is from the networking team and knows this test well.
> > > >
> > > >
> > >
> > > I do not know what is this RCU_LAZY thing, but IMO this should be opt-in
> >
> > You should read the links I sent you. We did already try opt-in,
> > Thomas Gleixner made a point at LPC that we should not add new APIs
> > for this purpose and confuse kernel developers.
> >
> > > For instance, only kfree_rcu() should use it.
> >
> > No. Most of the call_rcu() usages are for freeing memory, so the
> > consensus is we should apply this as opt out and fix issues along the
> > way. We already did a lot of research/diligence on seeing which users
> > need conversion.
> >
> > > We can not review hundreds of call_rcu() call sites and decide if
> > > adding arbitrary delays cou hurt .
> >
> > That work has already been done as much as possible, please read the
> > links I sent.
> 
> Also just to add, this test is a bit weird / corner case, as in anyone
> expecting a quick response from call_rcu() is broken by design.
> However, for these callbacks, it does not matter much which API they
> use as they are quite infrequent for power savings.

The "broken by design" is a bit strong.  Some of those call_rcu()
invocations have been around for the better part of 20 years, after all.

That aside, I do hope that we can arrive at something that will enhance
battery lifetime while avoiding unnecessary disruption.  But we are
unlikely to be able to completely avoid disruption.  As this email
thread illustrates.  ;-)

							Thanx, Paul

Re: [PATCH rcu/dev 3/3] net: Use call_rcu_flush() for dst_destroy_rcu

[Joel Fernandes]

> On Nov 17, 2022, at 2:29 PM, Paul E. McKenney <paulmck@kernel.org> wrote:
> 
> On Thu, Nov 17, 2022 at 05:40:40PM +0000, Joel Fernandes wrote:
>>> On Thu, Nov 17, 2022 at 5:38 PM Joel Fernandes <joel@joelfernandes.org> wrote:
>>> 
>>> On Thu, Nov 17, 2022 at 5:17 PM Eric Dumazet <edumazet@google.com> wrote:
>>>> 
>>>> On Thu, Nov 17, 2022 at 7:58 AM Joel Fernandes <joel@joelfernandes.org> wrote:
>>>>> 
>>>>> Hello Eric,
>>>>> 
>>>>> On Wed, Nov 16, 2022 at 07:44:41PM -0800, Eric Dumazet wrote:
>>>>>> On Wed, Nov 16, 2022 at 7:16 PM Joel Fernandes (Google)
>>>>>> <joel@joelfernandes.org> wrote:
>>>>>>> 
>>>>>>> In a networking test on ChromeOS, we find that using the new CONFIG_RCU_LAZY
>>>>>>> causes a networking test to fail in the teardown phase.
>>>>>>> 
>>>>>>> The failure happens during: ip netns del <name>
>>>>>> 
>>>>>> And ? What happens then next ?
>>>>> 
>>>>> The test is doing the 'ip netns del <name>' and then polling for the
>>>>> disappearance of a network interface name for upto 5 seconds. I believe it is
>>>>> using netlink to get a table of interfaces. That polling is timing out.
>>>>> 
>>>>> Here is some more details from the test's owner (copy pasting from another
>>>>> bug report):
>>>>> In the cleanup, we remove the netns, and thus will cause the veth pair being
>>>>> removed automatically, so we use a poll to check that if the veth in the root
>>>>> netns still exists to know whether the cleanup is done.
>>>>> 
>>>>> Here is a public link to the code that is failing (its in golang):
>>>>> https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform/tast-tests/src/chromiumos/tast/local/network/virtualnet/env/env.go;drc=6c2841d6cc3eadd23e07912ec331943ee33d7de8;l=161
>>>>> 
>>>>> Here is a public link to the line of code in the actual test leading up to the above
>>>>> path (this is the test that is run:
>>>>> network.RoutingFallthrough.ipv4_only_primary) :
>>>>> https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform/tast-tests/src/chromiumos/tast/local/bundles/cros/network/routing_fallthrough.go;drc=8fbf2c53960bc8917a6a01fda5405cad7c17201e;l=52
>>>>> 
>>>>>>> Using ftrace, I found the callbacks it was queuing which this series fixes. Use
>>>>>>> call_rcu_flush() to revert to the old behavior. With that, the test passes.
>>>>>> 
>>>>>> What is this test about ? What barrier was used to make it not flaky ?
>>>>> 
>>>>> I provided the links above, let me know if you have any questions.
>>>>> 
>>>>>> Was it depending on some undocumented RCU behavior ?
>>>>> 
>>>>> This is a new RCU feature posted here for significant power-savings on
>>>>> battery-powered devices:
>>>>> https://lore.kernel.org/rcu/20221017140726.GG5600@paulmck-ThinkPad-P17-Gen-1/T/#m7a54809b8903b41538850194d67eb34f203c752a
>>>>> 
>>>>> There is also an LPC presentation about the same, I can dig the link if you
>>>>> are interested.
>>>>> 
>>>>>> Maybe adding a sysctl to force the flush would be better for functional tests ?
>>>>>> 
>>>>>> I would rather change the test(s), than adding call_rcu_flush(),
>>>>>> adding merge conflicts to future backports.
>>>>> 
>>>>> I am not too sure about that, I think a user might expect the network
>>>>> interface to disappear from the networking tables quickly enough without
>>>>> dealing with barriers or kernel iternals. However, I added the authors of the
>>>>> test to this email in the hopes he can provide is point of views as well.
>>>>> 
>>>>> The general approach we are taking with this sort of thing is to use
>>>>> call_rcu_flush() which is basically the same as call_rcu() for systems with
>>>>> CALL_RCU_LAZY=n. You can see some examples of that in the patch series link
>>>>> above. Just to note, CALL_RCU_LAZY depends on CONFIG_RCU_NOCB_CPU so its only
>>>>> Android and ChromeOS that are using it. I am adding Jie to share any input,
>>>>> he is from the networking team and knows this test well.
>>>>> 
>>>>> 
>>>> 
>>>> I do not know what is this RCU_LAZY thing, but IMO this should be opt-in
>>> 
>>> You should read the links I sent you. We did already try opt-in,
>>> Thomas Gleixner made a point at LPC that we should not add new APIs
>>> for this purpose and confuse kernel developers.
>>> 
>>>> For instance, only kfree_rcu() should use it.
>>> 
>>> No. Most of the call_rcu() usages are for freeing memory, so the
>>> consensus is we should apply this as opt out and fix issues along the
>>> way. We already did a lot of research/diligence on seeing which users
>>> need conversion.
>>> 
>>>> We can not review hundreds of call_rcu() call sites and decide if
>>>> adding arbitrary delays cou hurt .
>>> 
>>> That work has already been done as much as possible, please read the
>>> links I sent.
>> 
>> Also just to add, this test is a bit weird / corner case, as in anyone
>> expecting a quick response from call_rcu() is broken by design.
>> However, for these callbacks, it does not matter much which API they
>> use as they are quite infrequent for power savings.
> 
> The "broken by design" is a bit strong.  Some of those call_rcu()
> invocations have been around for the better part of 20 years, after all.
> 
> That aside, I do hope that we can arrive at something that will enhance
> battery lifetime while avoiding unnecessary disruption.  But we are
> unlikely to be able to completely avoid disruption.  As this email
> thread illustrates.  ;-)

Another approach, with these 3 patches could be to keep the call_rcu() but add an rcu_barrier() after them. I think people running ip del netns should not have to wait for their RCU cb to take too long to run and remove user visible state. But I would need suggestions from networking experts which CBs of these 3, to do this for. Or for all of them.

Alternatively, we can also patch just the test with a new knob that does rcu_barrier. But I dislike that as it does not fix it for all users. Probably the ip utilities will also need a patch then.

Thanks,

- Joel 


> 
>                            Thanx, Paul

Re: [PATCH rcu/dev 3/3] net: Use call_rcu_flush() for dst_destroy_rcu

[Eric Dumazet]

On Thu, Nov 17, 2022 at 1:16 PM Joel Fernandes <joel@joelfernandes.org> wrote:
>
>
>
> > On Nov 17, 2022, at 2:29 PM, Paul E. McKenney <paulmck@kernel.org> wrote:
> >
> > On Thu, Nov 17, 2022 at 05:40:40PM +0000, Joel Fernandes wrote:
> >>> On Thu, Nov 17, 2022 at 5:38 PM Joel Fernandes <joel@joelfernandes.org> wrote:
> >>>
> >>> On Thu, Nov 17, 2022 at 5:17 PM Eric Dumazet <edumazet@google.com> wrote:
> >>>>
> >>>> On Thu, Nov 17, 2022 at 7:58 AM Joel Fernandes <joel@joelfernandes.org> wrote:
> >>>>>
> >>>>> Hello Eric,
> >>>>>
> >>>>> On Wed, Nov 16, 2022 at 07:44:41PM -0800, Eric Dumazet wrote:
> >>>>>> On Wed, Nov 16, 2022 at 7:16 PM Joel Fernandes (Google)
> >>>>>> <joel@joelfernandes.org> wrote:
> >>>>>>>
> >>>>>>> In a networking test on ChromeOS, we find that using the new CONFIG_RCU_LAZY
> >>>>>>> causes a networking test to fail in the teardown phase.
> >>>>>>>
> >>>>>>> The failure happens during: ip netns del <name>
> >>>>>>
> >>>>>> And ? What happens then next ?
> >>>>>
> >>>>> The test is doing the 'ip netns del <name>' and then polling for the
> >>>>> disappearance of a network interface name for upto 5 seconds. I believe it is
> >>>>> using netlink to get a table of interfaces. That polling is timing out.
> >>>>>
> >>>>> Here is some more details from the test's owner (copy pasting from another
> >>>>> bug report):
> >>>>> In the cleanup, we remove the netns, and thus will cause the veth pair being
> >>>>> removed automatically, so we use a poll to check that if the veth in the root
> >>>>> netns still exists to know whether the cleanup is done.
> >>>>>
> >>>>> Here is a public link to the code that is failing (its in golang):
> >>>>> https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform/tast-tests/src/chromiumos/tast/local/network/virtualnet/env/env.go;drc=6c2841d6cc3eadd23e07912ec331943ee33d7de8;l=161
> >>>>>
> >>>>> Here is a public link to the line of code in the actual test leading up to the above
> >>>>> path (this is the test that is run:
> >>>>> network.RoutingFallthrough.ipv4_only_primary) :
> >>>>> https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform/tast-tests/src/chromiumos/tast/local/bundles/cros/network/routing_fallthrough.go;drc=8fbf2c53960bc8917a6a01fda5405cad7c17201e;l=52
> >>>>>
> >>>>>>> Using ftrace, I found the callbacks it was queuing which this series fixes. Use
> >>>>>>> call_rcu_flush() to revert to the old behavior. With that, the test passes.
> >>>>>>
> >>>>>> What is this test about ? What barrier was used to make it not flaky ?
> >>>>>
> >>>>> I provided the links above, let me know if you have any questions.
> >>>>>
> >>>>>> Was it depending on some undocumented RCU behavior ?
> >>>>>
> >>>>> This is a new RCU feature posted here for significant power-savings on
> >>>>> battery-powered devices:
> >>>>> https://lore.kernel.org/rcu/20221017140726.GG5600@paulmck-ThinkPad-P17-Gen-1/T/#m7a54809b8903b41538850194d67eb34f203c752a
> >>>>>
> >>>>> There is also an LPC presentation about the same, I can dig the link if you
> >>>>> are interested.
> >>>>>
> >>>>>> Maybe adding a sysctl to force the flush would be better for functional tests ?
> >>>>>>
> >>>>>> I would rather change the test(s), than adding call_rcu_flush(),
> >>>>>> adding merge conflicts to future backports.
> >>>>>
> >>>>> I am not too sure about that, I think a user might expect the network
> >>>>> interface to disappear from the networking tables quickly enough without
> >>>>> dealing with barriers or kernel iternals. However, I added the authors of the
> >>>>> test to this email in the hopes he can provide is point of views as well.
> >>>>>
> >>>>> The general approach we are taking with this sort of thing is to use
> >>>>> call_rcu_flush() which is basically the same as call_rcu() for systems with
> >>>>> CALL_RCU_LAZY=n. You can see some examples of that in the patch series link
> >>>>> above. Just to note, CALL_RCU_LAZY depends on CONFIG_RCU_NOCB_CPU so its only
> >>>>> Android and ChromeOS that are using it. I am adding Jie to share any input,
> >>>>> he is from the networking team and knows this test well.
> >>>>>
> >>>>>
> >>>>
> >>>> I do not know what is this RCU_LAZY thing, but IMO this should be opt-in
> >>>
> >>> You should read the links I sent you. We did already try opt-in,
> >>> Thomas Gleixner made a point at LPC that we should not add new APIs
> >>> for this purpose and confuse kernel developers.
> >>>
> >>>> For instance, only kfree_rcu() should use it.
> >>>
> >>> No. Most of the call_rcu() usages are for freeing memory, so the
> >>> consensus is we should apply this as opt out and fix issues along the
> >>> way. We already did a lot of research/diligence on seeing which users
> >>> need conversion.
> >>>
> >>>> We can not review hundreds of call_rcu() call sites and decide if
> >>>> adding arbitrary delays cou hurt .
> >>>
> >>> That work has already been done as much as possible, please read the
> >>> links I sent.
> >>
> >> Also just to add, this test is a bit weird / corner case, as in anyone
> >> expecting a quick response from call_rcu() is broken by design.
> >> However, for these callbacks, it does not matter much which API they
> >> use as they are quite infrequent for power savings.
> >
> > The "broken by design" is a bit strong.  Some of those call_rcu()
> > invocations have been around for the better part of 20 years, after all.
> >
> > That aside, I do hope that we can arrive at something that will enhance
> > battery lifetime while avoiding unnecessary disruption.  But we are
> > unlikely to be able to completely avoid disruption.  As this email
> > thread illustrates.  ;-)
>
> Another approach, with these 3 patches could be to keep the call_rcu() but add an rcu_barrier() after them. I think people running ip del netns should not have to wait for their RCU cb to take too long to run and remove user visible state. But I would need suggestions from networking experts which CBs of these 3, to do this for. Or for all of them.
>
> Alternatively, we can also patch just the test with a new knob that does rcu_barrier. But I dislike that as it does not fix it for all users. Probably the ip utilities will also need a patch then.
>

Normally we have an rcu_barrier() in netns dismantle path already at a
strategic location ( in cleanup_net() )

Maybe the issue here is that some particular layers need another one.
Or we need to release a blocking reference before the call_rcu().
Some call_rcu() usages might not be optimal in this respect.

We should not add an rcu_barrier() after a call_rcu(), we prefer
factoring these expensive operations.

Re: [PATCH rcu/dev 1/3] net: Use call_rcu_flush() for qdisc_free_cb

[Eric Dumazet]

On Wed, Nov 16, 2022 at 7:16 PM Joel Fernandes (Google)
<joel@joelfernandes.org> wrote:
>
> In a networking test on ChromeOS, we find that using the new CONFIG_RCU_LAZY
> causes a networking test to fail in the teardown phase.
>
> The failure happens during: ip netns del <name>
>
> Using ftrace, I found the callbacks it was queuing which this series fixes. Use
> call_rcu_flush() to revert to the old behavior. With that, the test passes.
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> ---
>  net/sched/sch_generic.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index a9aadc4e6858..63fbf640d3b2 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -1067,7 +1067,7 @@ static void qdisc_destroy(struct Qdisc *qdisc)
>
>         trace_qdisc_destroy(qdisc);
>
> -       call_rcu(&qdisc->rcu, qdisc_free_cb);
> +       call_rcu_flush(&qdisc->rcu, qdisc_free_cb);
>  }

I took a look at this one.

qdisc_free_cb() is essentially freeing : Some per-cpu memory, and the
'struct Qdisc'

I do not see why we need to force a flush for this (small ?) piece of memory.

Re: [PATCH rcu/dev 2/3] net: Use call_rcu_flush() for in_dev_rcu_put

[Eric Dumazet]

On Wed, Nov 16, 2022 at 7:16 PM Joel Fernandes (Google)
<joel@joelfernandes.org> wrote:
>
> In a networking test on ChromeOS, we find that using the new CONFIG_RCU_LAZY
> causes a networking test to fail in the teardown phase.
>
> The failure happens during: ip netns del <name>
>
> Using ftrace, I found the callbacks it was queuing which this series fixes. Use
> call_rcu_flush() to revert to the old behavior. With that, the test passes.
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> ---
>  net/ipv4/devinet.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
> index e8b9a9202fec..98b20f333e00 100644
> --- a/net/ipv4/devinet.c
> +++ b/net/ipv4/devinet.c
> @@ -328,7 +328,7 @@ static void inetdev_destroy(struct in_device *in_dev)
>         neigh_parms_release(&arp_tbl, in_dev->arp_parms);
>         arp_ifdown(dev);
>
> -       call_rcu(&in_dev->rcu_head, in_dev_rcu_put);
> +       call_rcu_flush(&in_dev->rcu_head, in_dev_rcu_put);
>  }

For this one, I suspect the issue is about device refcount lingering ?

I think we should release refcounts earlier (and only delegate the
freeing part after RCU grace period, which can be 'lazy' just fine)

Something like:

diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index e8b9a9202fecd913137f169f161dfdccc16f7edf..e0258aef4211ec6a72d062963470a32776e6d010
100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -234,13 +234,21 @@ static void inet_free_ifa(struct in_ifaddr *ifa)
        call_rcu(&ifa->rcu_head, inet_rcu_free_ifa);
 }

+static void in_dev_free_rcu(struct rcu_head *head)
+{
+       struct in_device *idev = container_of(head, struct in_device, rcu_head);
+
+       kfree(rcu_dereference_protected(idev->mc_hash, 1));
+       kfree(idev);
+}
+
 void in_dev_finish_destroy(struct in_device *idev)
 {
        struct net_device *dev = idev->dev;

        WARN_ON(idev->ifa_list);
        WARN_ON(idev->mc_list);
-       kfree(rcu_dereference_protected(idev->mc_hash, 1));
+
 #ifdef NET_REFCNT_DEBUG
        pr_debug("%s: %p=%s\n", __func__, idev, dev ? dev->name : "NIL");
 #endif
@@ -248,7 +256,7 @@ void in_dev_finish_destroy(struct in_device *idev)
        if (!idev->dead)
                pr_err("Freeing alive in_device %p\n", idev);
        else
-               kfree(idev);
+               call_rcu(&idev->rcu_head, in_dev_free_rcu);
 }
 EXPORT_SYMBOL(in_dev_finish_destroy);

@@ -298,12 +306,6 @@ static struct in_device *inetdev_init(struct
net_device *dev)
        goto out;
 }

-static void in_dev_rcu_put(struct rcu_head *head)
-{
-       struct in_device *idev = container_of(head, struct in_device, rcu_head);
-       in_dev_put(idev);
-}
-
 static void inetdev_destroy(struct in_device *in_dev)
 {
        struct net_device *dev;
@@ -328,7 +330,7 @@ static void inetdev_destroy(struct in_device *in_dev)
        neigh_parms_release(&arp_tbl, in_dev->arp_parms);
        arp_ifdown(dev);

-       call_rcu(&in_dev->rcu_head, in_dev_rcu_put);
+       in_dev_put(in_dev);
 }

 int inet_addr_onlink(struct in_device *in_dev, __be32 a, __be32 b)

Re: [PATCH rcu/dev 1/3] net: Use call_rcu_flush() for qdisc_free_cb

[Joel Fernandes]

> On Nov 17, 2022, at 4:44 PM, Eric Dumazet <edumazet@google.com> wrote:
> 
> On Wed, Nov 16, 2022 at 7:16 PM Joel Fernandes (Google)
> <joel@joelfernandes.org> wrote:
>> 
>> In a networking test on ChromeOS, we find that using the new CONFIG_RCU_LAZY
>> causes a networking test to fail in the teardown phase.
>> 
>> The failure happens during: ip netns del <name>
>> 
>> Using ftrace, I found the callbacks it was queuing which this series fixes. Use
>> call_rcu_flush() to revert to the old behavior. With that, the test passes.
>> 
>> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>> ---
>> net/sched/sch_generic.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
>> index a9aadc4e6858..63fbf640d3b2 100644
>> --- a/net/sched/sch_generic.c
>> +++ b/net/sched/sch_generic.c
>> @@ -1067,7 +1067,7 @@ static void qdisc_destroy(struct Qdisc *qdisc)
>> 
>>        trace_qdisc_destroy(qdisc);
>> 
>> -       call_rcu(&qdisc->rcu, qdisc_free_cb);
>> +       call_rcu_flush(&qdisc->rcu, qdisc_free_cb);
>> }
> 
> I took a look at this one.
> 
> qdisc_free_cb() is essentially freeing : Some per-cpu memory, and the
> 'struct Qdisc'
> 
> I do not see why we need to force a flush for this (small ?) piece of memory.

I’ll try to drop that and rerun the test, and get back to you. It could be that there is a different callback that this flush() is compensating for, or something. I am pretty sure at one point, dropping this patch made the test fail most of the time. Now it passes 100%.

I’ll also attempt to collect a complete trace, maybe I’ll learn some networking code in the process..

Thanks!

Re: [PATCH rcu/dev 1/3] net: Use call_rcu_flush() for qdisc_free_cb

[Joel Fernandes]

On Thu, Nov 17, 2022 at 01:44:12PM -0800, Eric Dumazet wrote:
> On Wed, Nov 16, 2022 at 7:16 PM Joel Fernandes (Google)
> <joel@joelfernandes.org> wrote:
> >
> > In a networking test on ChromeOS, we find that using the new CONFIG_RCU_LAZY
> > causes a networking test to fail in the teardown phase.
> >
> > The failure happens during: ip netns del <name>
> >
> > Using ftrace, I found the callbacks it was queuing which this series fixes. Use
> > call_rcu_flush() to revert to the old behavior. With that, the test passes.
> >
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > ---
> >  net/sched/sch_generic.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> > index a9aadc4e6858..63fbf640d3b2 100644
> > --- a/net/sched/sch_generic.c
> > +++ b/net/sched/sch_generic.c
> > @@ -1067,7 +1067,7 @@ static void qdisc_destroy(struct Qdisc *qdisc)
> >
> >         trace_qdisc_destroy(qdisc);
> >
> > -       call_rcu(&qdisc->rcu, qdisc_free_cb);
> > +       call_rcu_flush(&qdisc->rcu, qdisc_free_cb);
> >  }
> 
> I took a look at this one.
> 
> qdisc_free_cb() is essentially freeing : Some per-cpu memory, and the
> 'struct Qdisc'
> 
> I do not see why we need to force a flush for this (small ?) piece of memory.

Indeed! Just tested and dropping this one still makes the test pass.

I believe this patch was papering over the issues fixed by the other
patches, so it stuck.

I will drop this one and move over to trying your suggestions for 2/3.

Thanks for taking a look,

 - Joel

Re: [PATCH rcu/dev 2/3] net: Use call_rcu_flush() for in_dev_rcu_put

[Joel Fernandes]

Hi Eric,

On Thu, Nov 17, 2022 at 01:58:18PM -0800, Eric Dumazet wrote:
> On Wed, Nov 16, 2022 at 7:16 PM Joel Fernandes (Google)
> <joel@joelfernandes.org> wrote:
> >
> > In a networking test on ChromeOS, we find that using the new CONFIG_RCU_LAZY
> > causes a networking test to fail in the teardown phase.
> >
> > The failure happens during: ip netns del <name>
> >
> > Using ftrace, I found the callbacks it was queuing which this series fixes. Use
> > call_rcu_flush() to revert to the old behavior. With that, the test passes.
> >
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > ---
> >  net/ipv4/devinet.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
> > index e8b9a9202fec..98b20f333e00 100644
> > --- a/net/ipv4/devinet.c
> > +++ b/net/ipv4/devinet.c
> > @@ -328,7 +328,7 @@ static void inetdev_destroy(struct in_device *in_dev)
> >         neigh_parms_release(&arp_tbl, in_dev->arp_parms);
> >         arp_ifdown(dev);
> >
> > -       call_rcu(&in_dev->rcu_head, in_dev_rcu_put);
> > +       call_rcu_flush(&in_dev->rcu_head, in_dev_rcu_put);
> >  }
> 
> For this one, I suspect the issue is about device refcount lingering ?
> 
> I think we should release refcounts earlier (and only delegate the
> freeing part after RCU grace period, which can be 'lazy' just fine)
> 
> Something like:

The below diff where you reduce refcount before RCU grace period, also makes the
test pass.

If you are Ok with it, I can roll it into a patch with your Author tag and my
Tested-by. Let me know what you prefer?

Also, looking through the patch, I don't see any issue. One thing is
netdev_put() now happens before a grace period, instead of after. But I don't
think that's an issue.

thanks!

 - Joel


> 
> diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
> index e8b9a9202fecd913137f169f161dfdccc16f7edf..e0258aef4211ec6a72d062963470a32776e6d010
> 100644
> --- a/net/ipv4/devinet.c
> +++ b/net/ipv4/devinet.c
> @@ -234,13 +234,21 @@ static void inet_free_ifa(struct in_ifaddr *ifa)
>         call_rcu(&ifa->rcu_head, inet_rcu_free_ifa);
>  }
> 
> +static void in_dev_free_rcu(struct rcu_head *head)
> +{
> +       struct in_device *idev = container_of(head, struct in_device, rcu_head);
> +
> +       kfree(rcu_dereference_protected(idev->mc_hash, 1));
> +       kfree(idev);
> +}
> +
>  void in_dev_finish_destroy(struct in_device *idev)
>  {
>         struct net_device *dev = idev->dev;
> 
>         WARN_ON(idev->ifa_list);
>         WARN_ON(idev->mc_list);
> -       kfree(rcu_dereference_protected(idev->mc_hash, 1));
> +
>  #ifdef NET_REFCNT_DEBUG
>         pr_debug("%s: %p=%s\n", __func__, idev, dev ? dev->name : "NIL");
>  #endif
> @@ -248,7 +256,7 @@ void in_dev_finish_destroy(struct in_device *idev)
>         if (!idev->dead)
>                 pr_err("Freeing alive in_device %p\n", idev);
>         else
> -               kfree(idev);
> +               call_rcu(&idev->rcu_head, in_dev_free_rcu);
>  }
>  EXPORT_SYMBOL(in_dev_finish_destroy);
> 
> @@ -298,12 +306,6 @@ static struct in_device *inetdev_init(struct
> net_device *dev)
>         goto out;
>  }
> 
> -static void in_dev_rcu_put(struct rcu_head *head)
> -{
> -       struct in_device *idev = container_of(head, struct in_device, rcu_head);
> -       in_dev_put(idev);
> -}
> -
>  static void inetdev_destroy(struct in_device *in_dev)
>  {
>         struct net_device *dev;
> @@ -328,7 +330,7 @@ static void inetdev_destroy(struct in_device *in_dev)
>         neigh_parms_release(&arp_tbl, in_dev->arp_parms);
>         arp_ifdown(dev);
> 
> -       call_rcu(&in_dev->rcu_head, in_dev_rcu_put);
> +       in_dev_put(in_dev);
>  }
> 
>  int inet_addr_onlink(struct in_device *in_dev, __be32 a, __be32 b)

Re: [PATCH rcu/dev 3/3] net: Use call_rcu_flush() for dst_destroy_rcu

[Joel Fernandes]

On Thu, Nov 17, 2022 at 9:29 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Thu, Nov 17, 2022 at 1:16 PM Joel Fernandes <joel@joelfernandes.org> wrote:
> >
> >
> >
> > > On Nov 17, 2022, at 2:29 PM, Paul E. McKenney <paulmck@kernel.org> wrote:
> > >
> > > On Thu, Nov 17, 2022 at 05:40:40PM +0000, Joel Fernandes wrote:
> > >>> On Thu, Nov 17, 2022 at 5:38 PM Joel Fernandes <joel@joelfernandes.org> wrote:
> > >>>
> > >>> On Thu, Nov 17, 2022 at 5:17 PM Eric Dumazet <edumazet@google.com> wrote:
> > >>>>
> > >>>> On Thu, Nov 17, 2022 at 7:58 AM Joel Fernandes <joel@joelfernandes.org> wrote:
> > >>>>>
> > >>>>> Hello Eric,
> > >>>>>
> > >>>>> On Wed, Nov 16, 2022 at 07:44:41PM -0800, Eric Dumazet wrote:
> > >>>>>> On Wed, Nov 16, 2022 at 7:16 PM Joel Fernandes (Google)
> > >>>>>> <joel@joelfernandes.org> wrote:
> > >>>>>>>
> > >>>>>>> In a networking test on ChromeOS, we find that using the new CONFIG_RCU_LAZY
> > >>>>>>> causes a networking test to fail in the teardown phase.
> > >>>>>>>
> > >>>>>>> The failure happens during: ip netns del <name>
> > >>>>>>
> > >>>>>> And ? What happens then next ?
> > >>>>>
> > >>>>> The test is doing the 'ip netns del <name>' and then polling for the
> > >>>>> disappearance of a network interface name for upto 5 seconds. I believe it is
> > >>>>> using netlink to get a table of interfaces. That polling is timing out.
> > >>>>>
> > >>>>> Here is some more details from the test's owner (copy pasting from another
> > >>>>> bug report):
> > >>>>> In the cleanup, we remove the netns, and thus will cause the veth pair being
> > >>>>> removed automatically, so we use a poll to check that if the veth in the root
> > >>>>> netns still exists to know whether the cleanup is done.
> > >>>>>
> > >>>>> Here is a public link to the code that is failing (its in golang):
> > >>>>> https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform/tast-tests/src/chromiumos/tast/local/network/virtualnet/env/env.go;drc=6c2841d6cc3eadd23e07912ec331943ee33d7de8;l=161
> > >>>>>
> > >>>>> Here is a public link to the line of code in the actual test leading up to the above
> > >>>>> path (this is the test that is run:
> > >>>>> network.RoutingFallthrough.ipv4_only_primary) :
> > >>>>> https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform/tast-tests/src/chromiumos/tast/local/bundles/cros/network/routing_fallthrough.go;drc=8fbf2c53960bc8917a6a01fda5405cad7c17201e;l=52
> > >>>>>
> > >>>>>>> Using ftrace, I found the callbacks it was queuing which this series fixes. Use
> > >>>>>>> call_rcu_flush() to revert to the old behavior. With that, the test passes.
> > >>>>>>
> > >>>>>> What is this test about ? What barrier was used to make it not flaky ?
> > >>>>>
> > >>>>> I provided the links above, let me know if you have any questions.
> > >>>>>
> > >>>>>> Was it depending on some undocumented RCU behavior ?
> > >>>>>
> > >>>>> This is a new RCU feature posted here for significant power-savings on
> > >>>>> battery-powered devices:
> > >>>>> https://lore.kernel.org/rcu/20221017140726.GG5600@paulmck-ThinkPad-P17-Gen-1/T/#m7a54809b8903b41538850194d67eb34f203c752a
> > >>>>>
> > >>>>> There is also an LPC presentation about the same, I can dig the link if you
> > >>>>> are interested.
> > >>>>>
> > >>>>>> Maybe adding a sysctl to force the flush would be better for functional tests ?
> > >>>>>>
> > >>>>>> I would rather change the test(s), than adding call_rcu_flush(),
> > >>>>>> adding merge conflicts to future backports.
> > >>>>>
> > >>>>> I am not too sure about that, I think a user might expect the network
> > >>>>> interface to disappear from the networking tables quickly enough without
> > >>>>> dealing with barriers or kernel iternals. However, I added the authors of the
> > >>>>> test to this email in the hopes he can provide is point of views as well.
> > >>>>>
> > >>>>> The general approach we are taking with this sort of thing is to use
> > >>>>> call_rcu_flush() which is basically the same as call_rcu() for systems with
> > >>>>> CALL_RCU_LAZY=n. You can see some examples of that in the patch series link
> > >>>>> above. Just to note, CALL_RCU_LAZY depends on CONFIG_RCU_NOCB_CPU so its only
> > >>>>> Android and ChromeOS that are using it. I am adding Jie to share any input,
> > >>>>> he is from the networking team and knows this test well.
> > >>>>>
> > >>>>>
> > >>>>
> > >>>> I do not know what is this RCU_LAZY thing, but IMO this should be opt-in
> > >>>
> > >>> You should read the links I sent you. We did already try opt-in,
> > >>> Thomas Gleixner made a point at LPC that we should not add new APIs
> > >>> for this purpose and confuse kernel developers.
> > >>>
> > >>>> For instance, only kfree_rcu() should use it.
> > >>>
> > >>> No. Most of the call_rcu() usages are for freeing memory, so the
> > >>> consensus is we should apply this as opt out and fix issues along the
> > >>> way. We already did a lot of research/diligence on seeing which users
> > >>> need conversion.
> > >>>
> > >>>> We can not review hundreds of call_rcu() call sites and decide if
> > >>>> adding arbitrary delays cou hurt .
> > >>>
> > >>> That work has already been done as much as possible, please read the
> > >>> links I sent.
> > >>
> > >> Also just to add, this test is a bit weird / corner case, as in anyone
> > >> expecting a quick response from call_rcu() is broken by design.
> > >> However, for these callbacks, it does not matter much which API they
> > >> use as they are quite infrequent for power savings.
> > >
> > > The "broken by design" is a bit strong.  Some of those call_rcu()
> > > invocations have been around for the better part of 20 years, after all.
> > >
> > > That aside, I do hope that we can arrive at something that will enhance
> > > battery lifetime while avoiding unnecessary disruption.  But we are
> > > unlikely to be able to completely avoid disruption.  As this email
> > > thread illustrates.  ;-)
> >
> > Another approach, with these 3 patches could be to keep the call_rcu() but add an rcu_barrier() after them. I think people running ip del netns should not have to wait for their RCU cb to take too long to run and remove user visible state. But I would need suggestions from networking experts which CBs of these 3, to do this for. Or for all of them.
> >
> > Alternatively, we can also patch just the test with a new knob that does rcu_barrier. But I dislike that as it does not fix it for all users. Probably the ip utilities will also need a patch then.
> >
>
> Normally we have an rcu_barrier() in netns dismantle path already at a
> strategic location ( in cleanup_net() )
>
> Maybe the issue here is that some particular layers need another one.
> Or we need to release a blocking reference before the call_rcu().
> Some call_rcu() usages might not be optimal in this respect.
>
> We should not add an rcu_barrier() after a call_rcu(), we prefer
> factoring these expensive operations.

Sounds good! The dst_destroy_rcu() function appears complex to break
down (similar to how you suggested in 2/3). The dst->ops->destroy()
can decrement refcounts and so forth. We could audit all such dst->ops
users, but I wonder if it is safer to use call_rcu_flush() for this
patch.

Thanks,

 - Joel

Re: [PATCH rcu/dev 2/3] net: Use call_rcu_flush() for in_dev_rcu_put

[Eric Dumazet]

On Thu, Nov 17, 2022 at 4:52 PM Joel Fernandes <joel@joelfernandes.org> wrote:
>
> Hi Eric,
>
> On Thu, Nov 17, 2022 at 01:58:18PM -0800, Eric Dumazet wrote:
> > On Wed, Nov 16, 2022 at 7:16 PM Joel Fernandes (Google)
> > <joel@joelfernandes.org> wrote:
> > >
> > > In a networking test on ChromeOS, we find that using the new CONFIG_RCU_LAZY
> > > causes a networking test to fail in the teardown phase.
> > >
> > > The failure happens during: ip netns del <name>
> > >
> > > Using ftrace, I found the callbacks it was queuing which this series fixes. Use
> > > call_rcu_flush() to revert to the old behavior. With that, the test passes.
> > >
> > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > > ---
> > >  net/ipv4/devinet.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
> > > index e8b9a9202fec..98b20f333e00 100644
> > > --- a/net/ipv4/devinet.c
> > > +++ b/net/ipv4/devinet.c
> > > @@ -328,7 +328,7 @@ static void inetdev_destroy(struct in_device *in_dev)
> > >         neigh_parms_release(&arp_tbl, in_dev->arp_parms);
> > >         arp_ifdown(dev);
> > >
> > > -       call_rcu(&in_dev->rcu_head, in_dev_rcu_put);
> > > +       call_rcu_flush(&in_dev->rcu_head, in_dev_rcu_put);
> > >  }
> >
> > For this one, I suspect the issue is about device refcount lingering ?
> >
> > I think we should release refcounts earlier (and only delegate the
> > freeing part after RCU grace period, which can be 'lazy' just fine)
> >
> > Something like:
>
> The below diff where you reduce refcount before RCU grace period, also makes the
> test pass.
>
> If you are Ok with it, I can roll it into a patch with your Author tag and my
> Tested-by. Let me know what you prefer?
>
> Also, looking through the patch, I don't see any issue. One thing is
> netdev_put() now happens before a grace period, instead of after. But I don't
> think that's an issue.

Normally the early netdev_put() is fine, because these netdev are
already fully RCU protected.

Sure, feel free to take this patch as is, thanks.