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: <CAAywjhTsPXtKGQejc_vOWzgF18u9XG74LzjZeP9i3TQGxUi6NA@mail.gmail.com>
Date: Fri, 25 Apr 2025 20:53:14 -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 Fri, Apr 25, 2025 at 8:12 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Fri, 25 Apr 2025 19:47:42 -0700 Jakub Kicinski wrote:
> > > > I haven't looked at the code, but I think it may be something more
> > > > trivial, namely that napi_enable() return void, so it can't fail.
> > > > Also it may be called under a spin lock.
> > >
> > > If you don't mind me asking: what do you think at a higher level
> > > on the discussion about threaded NAPI being disabled?
> > >
> > > It seems like the current behavior is:
> > >   - If you write 1 to the threaded NAPI sysfs path, kthreads are
> > >     kicked off and start running.
> > >
> > >   - If you write 0, the threads are not killed but don't do any
> > >     processing and their pids are still exported in netlink.
> > >
> > > I was arguing in favor of disabling threading means the thread is
> > > killed and the pid is no longer exported (as a side effect) because
> > > it seemed weird to me that the netlink output would say:
> > >
> > >    pid: 1234
> > >    threaded: 0
> > >
> > > In the current implementation.
> >
> > 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?

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.