netdev - Re: [PATCH net-next v5] Add support to set napi threaded for individual napi

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: <CAAywjhR0TPKZ-xzqjSP709OVmZWUisDNv2CVc_VxgOrXRtop+g@mail.gmail.com>
Date: Mon, 28 Apr 2025 12:25:13 -0700
From: Samiullah Khawaja <skhawaja@...gle.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Joe Damato <jdamato@...tly.com>, "David S . Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, almasrymina@...gle.com, 
	willemb@...gle.com, mkarsten@...terloo.ca, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v5] Add support to set napi threaded for
 individual napi

On Mon, Apr 28, 2025 at 11:23 AM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Fri, 25 Apr 2025 20:53:14 -0700 Samiullah Khawaja wrote:
> > > > We should check the discussions we had when threaded NAPI was added.
> > > > I feel nothing was exposed in terms of observability so leaving the
> > > > thread running didn't seem all that bad back then. Stopping the NAPI
> > > > polling safely is not entirely trivial, we'd need to somehow grab
> > > > the SCHED bit like busy polling does, and then re-schedule.
> > > > Or have the thread figure out that it's done and exit.
> > >
> > > Actually, we ended up adding the explicit ownership bits so it may not
> > > be all that hard any more.. Worth trying.
> > Agreed. NAPI kthread lets go of the ownership by unsetting the
> > SCHED_THREADED flag at napi_complete_done. This makes sure that the
> > next SCHED is scheduled when new IRQ arrives and no packets are
> > missed. We just have to make sure that it does that if it sees the
> > kthread_should_stop. Do you think we should handle this maybe as a
> > separate series/patch orthogonal to this?
>
> We need to handle the case Joe pointed out. The new Netlink attributes
> must make sense from day 1. I think it will be cleanest to work on
> killing the thread first, but it can be a separate series.
Yes, I will be sending a revision to handle the case that Joe pointed
out where the napi threaded is not disabled after setting it at device
level. The switch from threaded NAPI and the normal softirq based
polling happens already, this is existing behaviour. The thread goes
to sleep after doing polling cycle but it is "kthread_stop" in
napi_del and that aligns with your comment below and also Alexander
Duyck's suggestion on original change. Please correct me if I read
your comment below incorrectly.
>
> > Also some clarification, we can remove the kthread when disabling napi
> > threaded state using device level or napi level setting using netlink.
> > But do you think we should also stop the thread when disabling a NAPI?
> > That would mean the NAPI would lose any configurations done on this
> > kthread by the user and those configurations won't be restored when
> > this NAPI is enabled again. Some drivers use enable/disable as a
> > mechanism to do soft reset, so a simple softreset to change queue
> > length etc might revert these configurations.
>
> That part I think needs to stay as is, the thread can be started and
> stopped on napi_add / del, IMO.
Agreed. This is currently existing behaviour and this change doesn't
modify that. The threads are created when the
napi_set_threaded/dev_set_threaded is done if napis are already added
Or when napi_add is called. The threads are stopped/killed when
napi_del is called. And this matches Alexander Dayck's suggestion when
threaded mode was added originally:
https://lore.kernel.org/netdev/CAKgT0UdjWGBrv9wOUyOxon5Sn7qSBHL5-KfByPS4uB1_TJ3WiQ@mail.gmail.com/