Re: [Intel-wired-lan] [PATCH iwl-net] ice: add missing napi deletion

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

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.

> 
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan