Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 18 Nov 2022 14:23:48 -0800
From:   "Paul E. McKenney" <paulmck@kernel.org>
To:     "Joel Fernandes (Google)" <joel@joelfernandes.org>
Cc:     linux-kernel@vger.kernel.org, Eric Dumazet <edumazet@google.com>,
        David Ahern <dsahern@kernel.org>,
        "David S. Miller" <davem@davemloft.net>,
        Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>,
        Jakub Kicinski <kuba@kernel.org>, netdev@vger.kernel.org,
        Paolo Abeni <pabeni@redhat.com>, rcu@vger.kernel.org,
        rostedt@goodmis.org, fweisbec@gmail.com
Subject: Re: [PATCH v2 2/2] net: devinet: Reduce refcount before grace period
Message-ID: <20221118222348.GQ4001@paulmck-ThinkPad-P17-Gen-1>
Reply-To: paulmck@kernel.org
References: <20221118191909.1756624-1-joel@joelfernandes.org>
 <20221118191909.1756624-2-joel@joelfernandes.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221118191909.1756624-2-joel@joelfernandes.org>
Precedence: bulk

On Fri, Nov 18, 2022 at 07:19:09PM +0000, Joel Fernandes (Google) wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> Currently, the inetdev_destroy() function waits for an RCU grace period
> before decrementing the refcount and freeing memory. This causes a delay
> with a new RCU configuration that tries to save power, which results in the
> network interface disappearing later than expected. The resulting delay
> causes test failures on ChromeOS.
> 
> Refactor the code such that the refcount is freed before the grace period
> and memory is freed after. With this a ChromeOS network test passes that
> does 'ip netns del' and polls for an interface disappearing, now passes.
> 
> Reported-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>

Queued and pushed, thank you both!

This patch can go as-is based on Eric's Signed-off-by, but the first
one of course needs at least an ack.

							Thanx, Paul

> ---
>  net/ipv4/devinet.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
> index e8b9a9202fec..b0acf6e19aed 100644
> --- a/net/ipv4/devinet.c
> +++ b/net/ipv4/devinet.c
> @@ -234,13 +234,20 @@ 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 +255,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 +305,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 +329,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)
> -- 
> 2.38.1.584.g0f3c55d4c2-goog
>