Re: [PATCH net-next v2 6/8] netdevsim: add queue management API support

[Jakub Kicinski <kuba@kernel.org>]

On Tue, 7 Jan 2025 14:53:09 -0800 Mina Almasry wrote:
> > +/* Queue reset mode is controlled by ns->rq_reset_mode.
> > + * - normal - new NAPI new pool (old NAPI enabled when new added)  
> 
> Nit, probably not worth a respin: Normal seems to me to delete old
> napi after the new one is added and enabled.

Yes, the comment doesn't really focus on removal order, it's less
important. I may be missing your point..

> queue stop -> napi_disable(old)
> queue alloc -> netif_napi_add_config(new)
> queue start -> napi_enable(new)
> queue free -> netif_napi_del(old)

I think you have stop and alloc swapped here.

> > + * - mode 1 - allocate new pool (NAPI is only disabled / enabled)
> > + * - mode 2 - new NAPI new pool (old NAPI removed before new added)
> > + * - mode 3 - new NAPI new pool (old NAPI disabled when new added)
> > + */  
> 
> Which modes are 'correct' for a driver to implement? 2/3 is for
> testing only, as you note in the code, the add/del functions should
> really be called from alloc/free and not from queue_start. I assume
> modes normal and 1 are both correct implementations of the queue API
> and the driver gets to pick whether to reuse the napi instance or not?
> Asking because IIRC GVE implements mode 1, not what you consider
> 'normal'.

I can't think why any of them would be "incorrect" per se, but 2 and 3
are likely weird and unnatural. I thought mode 0 would simply be most
common, maybe I shouldn't have called it "normal". But because of all
the "modes" netdevsim implementation seemed too messy to be considered
a blueprint..