| Message ID | 20230620082454.377001-1-maciej.fijalkowski@intel.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [iwl-net] ice: add missing napi deletion | expand |
On 6/20/23 10:24, Maciej Fijalkowski wrote: > Error path from ice_probe() is missing ice_napi_del() calls, add it to > ice_deinit_eth() as ice_init_eth() was the one to add napi instances. It > is also a good habit to delete napis when removing driver and this also > addresses that. FWIW ice_napi_del() had no callsites. > > Fixes: 6624e780a577 ("ice: split ice_vsi_setup into smaller functions") > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> > --- > drivers/net/ethernet/intel/ice/ice_main.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c > index cd562856f23a..f6b041490154 100644 > --- a/drivers/net/ethernet/intel/ice/ice_main.c > +++ b/drivers/net/ethernet/intel/ice/ice_main.c > @@ -4485,6 +4485,7 @@ static void ice_deinit_eth(struct ice_pf *pf) > if (!vsi) > return; > > + ice_napi_del(vsi); > ice_vsi_close(vsi); > ice_unregister_netdev(vsi); > ice_devlink_destroy_pf_port(pf); Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
On Tue, 20 Jun 2023 10:24:54 +0200 Maciej Fijalkowski wrote: > Error path from ice_probe() is missing ice_napi_del() calls, add it to > ice_deinit_eth() as ice_init_eth() was the one to add napi instances. It > is also a good habit to delete napis when removing driver and this also > addresses that. FWIW ice_napi_del() had no callsites. > > Fixes: 6624e780a577 ("ice: split ice_vsi_setup into smaller functions") > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> Is there user visible impact? I agree that it's a good habit, but since unregister cleans up NAPI instances automatically the patch is not necessarily a fix.
On Tue, Jun 20, 2023 at 09:53:35AM -0700, Jakub Kicinski wrote: > On Tue, 20 Jun 2023 10:24:54 +0200 Maciej Fijalkowski wrote: > > Error path from ice_probe() is missing ice_napi_del() calls, add it to > > ice_deinit_eth() as ice_init_eth() was the one to add napi instances. It > > is also a good habit to delete napis when removing driver and this also > > addresses that. FWIW ice_napi_del() had no callsites. > > > > Fixes: 6624e780a577 ("ice: split ice_vsi_setup into smaller functions") > > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> > > Is there user visible impact? I agree that it's a good habit, but > since unregister cleans up NAPI instances automatically the patch > is not necessarily a fix. It's rather free_netdev() not unregistering per se, no? I sent this patch as I found that cited commit didn't delete napis on ice_probe()'s error path - I just saw that as a regression. But as you're saying when getting rid of netdev we actually do netif_napi_del() - it seems redundant to do explicit napi delete on remove path as it is supposed do free the netdev. Does it mean that many drivers should be verified against that? Sorta tired so might be missing something, pardon. If not, I'll send a v2 that just removes ice_napi_del().
On Tue, 20 Jun 2023 19:22:01 +0200 Maciej Fijalkowski wrote: > On Tue, Jun 20, 2023 at 09:53:35AM -0700, Jakub Kicinski wrote: > > Is there user visible impact? I agree that it's a good habit, but > > since unregister cleans up NAPI instances automatically the patch > > is not necessarily a fix. > > It's rather free_netdev() not unregistering per se, no? I sent this patch > as I found that cited commit didn't delete napis on ice_probe()'s error > path - I just saw that as a regression. > > But as you're saying when getting rid of netdev we actually do > netif_napi_del() - it seems redundant to do explicit napi delete on remove > path as it is supposed do free the netdev. Does it mean that many drivers > should be verified against that? Sorta tired so might be missing > something, pardon. If not, I'll send a v2 that just removes > ice_napi_del(). I personally prefer to keep track of my resources, so I avoid devm_* and delete NAPI instances by hand. It's up to the author and/or maintainer of the driver in question. My only real ask is to no route this via net and drop the Fixes tag. Whether you prefer to keep the patch as is or drop ice_napi_del() -- up to you.
On Tue, Jun 20, 2023 at 10:49:11AM -0700, Jakub Kicinski wrote: > On Tue, 20 Jun 2023 19:22:01 +0200 Maciej Fijalkowski wrote: > > On Tue, Jun 20, 2023 at 09:53:35AM -0700, Jakub Kicinski wrote: > > > Is there user visible impact? I agree that it's a good habit, but > > > since unregister cleans up NAPI instances automatically the patch > > > is not necessarily a fix. > > > > It's rather free_netdev() not unregistering per se, no? I sent this patch > > as I found that cited commit didn't delete napis on ice_probe()'s error > > path - I just saw that as a regression. > > > > But as you're saying when getting rid of netdev we actually do > > netif_napi_del() - it seems redundant to do explicit napi delete on remove > > path as it is supposed do free the netdev. Does it mean that many drivers > > should be verified against that? Sorta tired so might be missing > > something, pardon. If not, I'll send a v2 that just removes > > ice_napi_del(). > > I personally prefer to keep track of my resources, so I avoid devm_* > and delete NAPI instances by hand. It's up to the author and/or > maintainer of the driver in question. Hmm I am not a fan of devm either but I didn't mean that in my response at all. There are quite a few drivers that do this: net/core/dev.c: void free_netdev(struct net_device *dev) { (...) list_for_each_entry_safe(p, n, &dev->napi_list, dev_list) netif_napi_del(p); (...) } static inline void netif_napi_del(struct napi_struct *napi) { __netif_napi_del(napi); synchronize_net(); } drivers/net/ethernet/xxxcorp/xxx/xxx_main.c: static void xxx_remove(struct pci_dev *pdev) { // retrieve net_device and napi_struct ptrs netif_napi_del(napi); // redundant, covered below (...) free_netdev(netdev); (...) } I believe this is what you were referring to originally and I said that after a short drivers audit there is a bunch going via flow shown above...plus my patch was trying to introduce that :) Although in such case __netif_napi_del() will exit early as NAPI_STATE_LISTED bit was cleared, if driver holds multiple napi instances we will be going unnecessarily via synchronize_net() calls. > > My only real ask is to no route this via net and drop the Fixes tag. > Whether you prefer to keep the patch as is or drop ice_napi_del() -- > up to you. I'll go through -next and remove ice_napi_del(), given the above explanation what I meant previously. >
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c index cd562856f23a..f6b041490154 100644 --- a/drivers/net/ethernet/intel/ice/ice_main.c +++ b/drivers/net/ethernet/intel/ice/ice_main.c @@ -4485,6 +4485,7 @@ static void ice_deinit_eth(struct ice_pf *pf) if (!vsi) return; + ice_napi_del(vsi); ice_vsi_close(vsi); ice_unregister_netdev(vsi); ice_devlink_destroy_pf_port(pf);
Error path from ice_probe() is missing ice_napi_del() calls, add it to ice_deinit_eth() as ice_init_eth() was the one to add napi instances. It is also a good habit to delete napis when removing driver and this also addresses that. FWIW ice_napi_del() had no callsites. Fixes: 6624e780a577 ("ice: split ice_vsi_setup into smaller functions") Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> --- drivers/net/ethernet/intel/ice/ice_main.c | 1 + 1 file changed, 1 insertion(+)