Re: [PATCH] workqueue: Always use wq_select_unbound_cpu() for WORK_CPU_UNBOUND.

[Tejun Heo <tj@kernel.org>]

Hello, Sebastian.

On Wed, Feb 26, 2025 at 05:18:47PM +0100, Sebastian Andrzej Siewior wrote:
> > 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.

All variants that use per-cpu workqueues.

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

Yes and adding WQ_UNBOUND to custom per-cpu workqueues.

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

We can rename APIs too and it's going to be a slog, which, to be fair, this
is going to be no matter what.

> > 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()

If we're renaming, I'd deprecate the schedule_*() interface and always use
queue_*() and maybe:

- Replace WQ_UNBOUND with its complement WQ_PERCPU.
- Add WQ_PREFER_PERCPU.
- Rename system_wq -> system_percpu_wq.
- Add system_prefer_percpu_wq.
- Rename system_unbound_wq -> system_dfl_wq.

> ? 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.

I'm not sure allowing queueing on per-cpu worker on a different CPU would be
all that useful. Might as well just turn them into when necessary.

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

It's a correctness issue. We can't change the gurantees without auditing and
transitining the users.

Thanks.

-- 
tejun