| Message ID | 20250117232113.1612899-1-edumazet@google.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: introduce netdev_napi_exit() | expand |
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>
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
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(-)