netdev - Re: [PATCH net-next v6] 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: <aBJVi0LmwqAtQxv_@LQ3V64L9R2>
Date: Wed, 30 Apr 2025 09:53:31 -0700
From: Joe Damato <jdamato@...tly.com>
To: Samiullah Khawaja <skhawaja@...gle.com>
Cc: Jakub Kicinski <kuba@...nel.org>,
	"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 v6] Add support to set napi threaded for
 individual napi

On Tue, Apr 29, 2025 at 06:16:29PM -0700, Samiullah Khawaja wrote:
> On Tue, Apr 29, 2025 at 4:57 PM Joe Damato <jdamato@...tly.com> wrote:
> >
> > On Tue, Apr 29, 2025 at 10:26:56PM +0000, Samiullah Khawaja wrote:
> >
> > > v6:
> > >  - Set the threaded property at device level even if the currently set
> > >    value is same. This is to override any per napi settings. Update
> > >    selftest to verify this scenario.
> > >  - Use u8 instead of uint in netdev_nl_napi_set_config implementation.
> > >  - Extend the selftest to verify the existing behaviour that the PID
> > >    stays valid once threaded napi is enabled. It stays valid even after
> > >    disabling the threaded napi. Also verify that the same kthread(PID)
> > >    is reused when threaded napi is enabled again. Will keep this
> > >    behaviour as based on the discussion on v5.
> >
> > This doesn't address the feedback from Jakub in the v5 [1] [2]:
> >
> >  - Jakub said the netlink attributes need to make sense from day 1.
> >    Threaded = 0 and pid = 1234 does not make sense, and
> Jakub mentioned following in v5 and that is the existing behaviour:
> ```
> That part I think needs to stay as is, the thread can be started and
> stopped on napi_add / del, IMO.
> ```
> Please see my reply to him in v5 also to confirm this. I also quoted
> the original reason, when this was added, behind not doing
> kthread_stop when unsetting napi threaded.

Here's what [2] says:

  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.

In this v6 as I understand it, it is possible to get:

    threaded = 0
    pid = 1234

I don't think that makes sense and it goes against my interpretation
of Jakub's message which seemed clear to me that this must be fixed.

If you disagree and think my interpretation of what Jakub said
is off, we can let him weigh in again.

> >
> >  - The thread should be started and stopped.
> >
> > [1]: https://lore.kernel.org/netdev/20250425201220.58bf25d7@kernel.org/
> > [2]: https://lore.kernel.org/netdev/20250428112306.62ff198b@kernel.org/