linux-kernel - Re: [PATCH] workqueue: Always use wq_select_unbound_cpu() for WORK_CPU

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: <20250226161847.eYrJFpIg@linutronix.de>
Date: Wed, 26 Feb 2025 17:18:47 +0100
From: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To: Tejun Heo <tj@...nel.org>
Cc: Frederic Weisbecker <frederic@...nel.org>,
	open list <linux-kernel@...r.kernel.org>,
	Lai Jiangshan <jiangshanlai@...il.com>,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH] workqueue: Always use wq_select_unbound_cpu() for
 WORK_CPU_UNBOUND.

On 2025-02-21 06:48:16 [-1000], Tejun Heo wrote:
> Hello,
Hi,

> > I'm tempted to just assume that none of its users depend on the
> > work locality?
> 
> That's API guarantee and there are plenty of users who depend on
> queue_work() and schedule_work() on per-cpu workqueues to be actually
> per-cpu. 

You mean queue_work(), not queue_work_on()?
Even with the latter you need to ensure the CPU does not go away and
hardly someone does this.

>          I don't think we can pull the rug from under them. If we want to do
> this, which I think is a good idea, we should:
>
> 1. Convert per-cpu workqueue users to unbound workqueues. Most users don't
>    care whether work item is executed locally or not. However, historically,
>    we've been preferring per-cpu workqueues because unbound workqueues had a
>    lot worse locality properties. Unbound workqueue's topology awareness is
>    a lot better now, so this should be less of a problem and we should be
>    able to move a lot of users over to unbound workqueues.

you mean convert each schedule_work() to schedule_unbound_work() which
uses system_unbound_wq instead?

I would really like to make it default because otherwise most people
will stick to the old function and the "convert" is never ending.

Maybe I misunderstood you.

> 2. There still are cases where local execution isn't required for
>    correctness but local & concurrency controlled executions yield
>    performance gains. Workqueue API currently doesn't distinguish these two
>    cases. We should add a new API which prefers local execution but doesn't
>    require it, which can then do what's suggested in this patch.

I see. So we get rid of the old naming and have them something like
	schedule_bound_work()
	schedule_unbound_work()
	schedule_hopefully_local_work()

? The last one would attempt the local CPU for performance reasons
unless the CPU is not part the workqueue' cpumask. So the difference is
that the middle one would be queued on WQ_UNBOUND while the latter might
be queued on a different CPU but on WQ without WQ_UNBOUND. Both would
respect workqueue' cpumask.

> Unfortunately, I don't see a way forward without auditing and converting the
> users.

So tried to pull the "in WORK_CPU_UNBOUND the has unbound" card and
comment where it says "prefer local CPU" card.
We have already different behaviour with queue_delayed_work(,,0) vs
queue_delayed_work(,,!0) but this does not count here?
I don't insist in doing this always, just if the CPU is "isolated" as in
not part of the workmask. But then, this path gets probably less testing
so it might be not a good idea if something relies on but does not know…

> Thanks.

Sebastian