[PATCH net-next] net: introduce netdev_napi_exit()

[PATCH net-next] net: introduce netdev_napi_exit()

[Eric Dumazet]

After 1b23cdbd2bbc ("net: protect netdev->napi_list with netdev_lock()")
it makes sense to iterate through dev->napi_list while holding
the device lock.

Also call synchronize_net() at most one time.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/core/dev.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index fab4899b83f745a3c13c982775e287b1ff2f547d..d7cbe6ff5249a5c22dd7e0e4c929a535e1f28612 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -11568,6 +11568,22 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
 }
 EXPORT_SYMBOL(alloc_netdev_mqs);
 
+static void netdev_napi_exit(struct net_device *dev)
+{
+	if (!list_empty(&dev->napi_list)) {
+		struct napi_struct *p, *n;
+
+		netdev_lock(dev);
+		list_for_each_entry_safe(p, n, &dev->napi_list, dev_list)
+			__netif_napi_del_locked(p);
+		netdev_unlock(dev);
+
+		synchronize_net();
+	}
+
+	kvfree(dev->napi_config);
+}
+
 /**
  * free_netdev - free network device
  * @dev: device
@@ -11579,8 +11595,6 @@ EXPORT_SYMBOL(alloc_netdev_mqs);
  */
 void free_netdev(struct net_device *dev)
 {
-	struct napi_struct *p, *n;
-
 	might_sleep();
 
 	/* When called immediately after register_netdevice() failed the unwind
@@ -11602,10 +11616,7 @@ void free_netdev(struct net_device *dev)
 	/* Flush device addresses */
 	dev_addr_flush(dev);
 
-	list_for_each_entry_safe(p, n, &dev->napi_list, dev_list)
-		netif_napi_del(p);
-
-	kvfree(dev->napi_config);
+	netdev_napi_exit(dev);
 
 	ref_tracker_dir_exit(&dev->refcnt_tracker);
 #ifdef CONFIG_PCPU_DEV_REFCNT
-- 
2.48.0.rc2.279.g1de40edade-goog

Re: [PATCH net-next] net: introduce netdev_napi_exit()

[Jakub Kicinski]

On Fri, 17 Jan 2025 23:21:13 +0000 Eric Dumazet wrote:
> After 1b23cdbd2bbc ("net: protect netdev->napi_list with netdev_lock()")
> it makes sense to iterate through dev->napi_list while holding
> the device lock.
> 
> Also call synchronize_net() at most one time.

I suspected you may send this :)

I was wondering whether we have to call sync_rcu() at all, Joe moved
unhashing the NAPI to napi_disable(). Assuming the driver is sane 
it will call napi_disable() at latest in ndo_uninit(), and there's
already a synchronize_net() between ndo_uninit() and free.

But maybe drivers are not sane. We don't have

	/* napi_disable() sets the SCHED bit */
	WARN_ON(!test_bit(NAPI_STATE_SCHED, &val));

in netif_napi_del(). I think we should add it, after Joe's changes
if driver doesn't disable the napi it will leave a stale pointer
in the hash table.

In any case, your change makes sense:

Reviewed-by: Jakub Kicinski <kuba@kernel.org>

Re: [PATCH net-next] net: introduce netdev_napi_exit()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Fri, 17 Jan 2025 23:21:13 +0000 you wrote:
> After 1b23cdbd2bbc ("net: protect netdev->napi_list with netdev_lock()")
> it makes sense to iterate through dev->napi_list while holding
> the device lock.
> 
> Also call synchronize_net() at most one time.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> 
> [...]

Here is the summary with links:
  - [net-next] net: introduce netdev_napi_exit()
    https://git.kernel.org/netdev/net-next/c/185e5b869071

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html