linux-kernel - Re: [PATCH] softirq: Provide a handshake for canceling tasklets via polling on PREEMPT

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] [day] [month] [year] [list]

Message-ID: <20250821092827.zcFpdnNy@linutronix.de>
Date: Thu, 21 Aug 2025 11:28:27 +0200
From: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To: Tejun Heo <tj@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-rt-devel@...ts.linux.dev,
	Lai Jiangshan <jiangshanlai@...il.com>,
	Ingo Molnar <mingo@...hat.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH] softirq: Provide a handshake for canceling tasklets via
 polling on PREEMPT_RT

On 2025-08-20 09:44:53 [-1000], Tejun Heo wrote:
> Hello,
Hi Tejun,

> On Wed, Aug 20, 2025 at 12:55:18PM +0200, Sebastian Andrzej Siewior wrote:
> > On 2025-08-20 12:36:59 [+0200], To Tejun Heo wrote:
> > > Subject: [PATCH] workqueue: Provide a handshake for canceling BH workers
> > …
> > > This will flush all BH-work items assigned to that pool.
> > 
> > We need to flush all items because the inserted wq_barrier is at the
> > end of the queue. So if the cb_lock is dropped after
> > worker->current_func(work) then we will live lock. Just tested, I
> > somehow assumed it polls on worker.
> 
> Is flushing all a problem tho? 

It is not wrong as in something will explode. The priority-inheritance
boost is meant to give the lower priority task runtime in order to leave
its critical section. So the task with the higher priority can continue
to make progress instead of sitting around. Spinning while waiting for
completion will not succeed.
In this case "leaving the critical section" would mean complete the one
work item. But instead we flush all of them. It is more of semantics in
this case. That is why the looping-cancel in tasklet cancels just that
one workitem.

> I think the main focus is keeping the
> semantics matching on RT, right?

Yes, but having the semantics with busy waiting on a BH work is kind of
the problem. And there is no need for it which makes it a bit difficult.
The previous patch would match the !RT bits but we flush all work, have
and the lock for no reason. That is why I don't like it. The majority of
tasklet users don't need it. It is in my opinion bad semantics.

But if you insist on it, the previous patch will make it work and has
been tested. There is not much I can do.

> ...
> > -	if (from_cancel) {
> > +	if (from_cancel && !IS_ENABLED(CONFIG_PREEMPT_RT)) {
> >  		unsigned long data = *work_data_bits(work);
> >  
> >  		if (!WARN_ON_ONCE(data & WORK_STRUCT_PWQ) &&
> >  		    (data & WORK_OFFQ_BH)) {
> > -			/*
> > -			 * On RT, prevent a live lock when %current preempted
> > -			 * soft interrupt processing or prevents ksoftirqd from
> > -			 * running by keeping flipping BH. If the BH work item
> > -			 * runs on a different CPU then this has no effect other
> > -			 * than doing the BH disable/enable dance for nothing.
> > -			 * This is copied from
> > -			 * kernel/softirq.c::tasklet_unlock_spin_wait().
> > -			 */
> >  			while (!try_wait_for_completion(&barr.done)) {
> > -				if (IS_ENABLED(CONFIG_PREEMPT_RT)) {
> > -					local_bh_disable();
> > -					local_bh_enable();
> > -				} else {
> > -					cpu_relax();
> > -				}
> > +				cpu_relax();
> 
> I'm most likely missing something about RT but wouldn't the above still lead
> to deadlocks if the caller is non-hardirq but higher priority thread?

Not sure what you refer to. Right now there is this lock in
local_bh_disable() which forces PI.
Removing the whole section for RT as in this snippet gets us to the
wait_for_completion() below. It lets the task with higher priority
schedule out allowing task with lower priority to run. Eventually the
barrier item completes and with the wakeup the high priority task will
continue.
So the high-priority task will lose runtime (allowing task with lower
priority to run). I don't think it will be a problem because it is
mostly used in "quit" scenarios (not during normal workload) and matches
tasklet_disable().

> Thanks.

Sebastian