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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250107164529.27603de6@kernel.org>
Date: Tue, 7 Jan 2025 16:45:29 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: Mina Almasry <almasrymina@...gle.com>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
 pabeni@...hat.com, andrew+netdev@...n.ch, horms@...nel.org,
 willemdebruijn.kernel@...il.com, sdf@...ichev.me, Willem de Bruijn
 <willemb@...gle.com>
Subject: Re: [PATCH net-next v2 6/8] netdevsim: add queue management API
 support

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..