netdev - Re: [PATCH net-next v2] net: stop napi kthreads when THREADED napi is disabled

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: <20250520190941.56523ded@kernel.org>
Date: Tue, 20 May 2025 19:09:41 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Samiullah Khawaja <skhawaja@...gle.com>
Cc: "David S . Miller " <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
 almasrymina@...gle.com, willemb@...gle.com, jdamato@...tly.com,
 mkarsten@...terloo.ca, weiwan@...gle.com, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v2] net: stop napi kthreads when THREADED napi
 is disabled

On Mon, 19 May 2025 22:43:25 +0000 Samiullah Khawaja wrote:
> -/* Called with irq disabled */
> -static inline void ____napi_schedule(struct softnet_data *sd,
> -				     struct napi_struct *napi)
> +static inline bool ____try_napi_schedule_threaded(struct softnet_data *sd,
> +						  struct napi_struct *napi)
>  {
>  	struct task_struct *thread;
> +	unsigned long new, val;
>  
> -	lockdep_assert_irqs_disabled();
> +	do {
> +		val = READ_ONCE(napi->state);
> +
> +		if (!(val & NAPIF_STATE_THREADED))
> +			return false;

Do we really need to complicate the fastpath to make the slowpath easy?

Plus I'm not sure it works. 

          CPU 0 (IRQ)             CPU 1 (NAPI thr)          CPU 2 (config)
                         if (test_bit(NAPI_STATE_SCHED_THREADED))
                               ...

  ____napi_schedule()
  cmpxchg(...)
  wake_up_process(thread);
                                                       clear_bit(NAPI_STATE_THREADED)
                                                       kthread_stop(thread)

                         if (kthread_should_stop())
                                exit

Right?

I think the shutting down thread should do this:

	while (true) {
		state = READ_ONCE()

		// safe to clear if thread owns the NAPI,
		// or NAPI is completely idle
		if (state & SCHED_THREADED || !(state & SCHED)) {
			state &= ~THREADED;
		} else {
			msleep(1);
			continue;
		}

		if (try_cmpxchg())
			break;
	}

But that's just an idea, it could also be wrong... :S