| Message ID | 20221117031551.1142289-2-joel@joelfernandes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [rcu/dev,1/3] net: Use call_rcu_flush() for qdisc_free_cb | expand |
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)
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)
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.
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)
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(-)