Re: [RFC net-next v2 1/9] net: napi: Add napi_storage

[Stanislav Fomichev <stfomichev@gmail.com>]

On 09/08, Joe Damato wrote:
> On Sun, Sep 08, 2024 at 04:06:35PM +0000, Joe Damato wrote:
> > Add a persistent NAPI storage area for NAPI configuration to the core.
> > Drivers opt-in to setting the storage for a NAPI by passing an index
> > when calling netif_napi_add_storage.
> > 
> > napi_storage is allocated in alloc_netdev_mqs, freed in free_netdev
> > (after the NAPIs are deleted), and set to 0 when napi_enable is called.
> > 
> > Signed-off-by: Joe Damato <jdamato@fastly.com>
> > ---
> >  .../networking/net_cachelines/net_device.rst  |  1 +
> >  include/linux/netdevice.h                     | 34 +++++++++++++++++++
> >  net/core/dev.c                                | 18 +++++++++-
> >  3 files changed, 52 insertions(+), 1 deletion(-)
> > 
> 
> [...]
> 
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -6719,6 +6719,9 @@ void napi_enable(struct napi_struct *n)
> >  		if (n->dev->threaded && n->thread)
> >  			new |= NAPIF_STATE_THREADED;
> >  	} while (!try_cmpxchg(&n->state, &val, new));
> > +
> > +	if (n->napi_storage)
> > +		memset(n->napi_storage, 0, sizeof(*n->napi_storage));
> 
> This part is very obviously wrong ;)
> 
> I think when I was reading the other thread about resetting on
> channel change [1] I got a bit confused.
> 
> Maybe what was intended was on napi_enable, do nothing / remove the
> above code (which I suppose would give the persistence desired?).
> 
> But modify net/ethtool/channels.c to reset NAPIs to the global
> (sysfs) settings? Not sure how to balance both persistence with
> queue count changes in a way that makes sense for users of the API.
> 
> And, I didn't quite follow the bits about:
>   1. The proposed ring to NAPI mapping

[..]

>   2. The two step "takeover" which seemed to imply that we might
>      pull napi_id into napi_storage? Or maybe I just read that part
>      wrong?

Yes, the suggestion here is to drop patch #2 from your series and
keep napi_id as a user facing 'id' for the persistent storage. But,
obviously, this requires persistent napi_id(s) that survive device
resets.

The function that allocates new napi_id is napi_hash_add
from netif_napi_add_weight. So we can do either of the following:
1. Keep everything as is, but add the napi_rehash somewhere
   around napi_enable to 'takeover' previously allocated napi_id.
2. (preferred) Separate napi_hash_add out of netif_napi_add_weight.
   And have some new napi_hash_with_id(previous_napi_id) to expose it to the
   userspace. Then convert mlx5 to this new interface.