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@xxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxx>
> ---
>  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)