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: <CAAywjhQVdYuc3NuLYNMgf90Ng_zjhFyTQRWLnPR7Mk-2MWQ2JA@mail.gmail.com>
Date: Wed, 30 Apr 2025 10:54:16 -0700
From: Samiullah Khawaja <skhawaja@...gle.com>
To: Joe Damato <jdamato@...tly.com>, Samiullah Khawaja <skhawaja@...gle.com>, 
	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 Wed, Apr 30, 2025 at 9:53 AM Joe Damato <jdamato@...tly.com> wrote:
>
> 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.
Agreed. Also my interpretation of his statement might be incorrect.
Considering the possible actions,

- Hiding the PID when "napi threaded" is disabled is likely not ideal,
as you have pointed out, though I find it acceptable.
- Stopping the thread when "napi threaded" is set to 0 doesn't align
with Jakub's following statement,
```
That part I think needs to stay as is, the thread can be started and
stopped on napi_add / del, IMO.
```
Also please note the discussion on stopping the thread I shared earlier:
https://lore.kernel.org/netdev/CAKgT0UdjWGBrv9wOUyOxon5Sn7qSBHL5-KfByPS4uB1_TJ3WiQ@mail.gmail.com/

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