Re: [PATCH wq/for-7.5 v4 2/2] workqueue: Keep the limits of both max_active domains current

[Tejun Heo <tj@kernel.org>]

Hello, Breno.

On Thu, Oct 29, 2026 at 05:33:46AM -0700, Breno Leitao wrote:
> The planned PERCPU affinity scope will let a workqueue switch backends
> through its attributes, and a switch should land on a sensible limit.
> Derive the other domain whenever the limits of the backend in use are
> set.

I think the two domains should be separated completely at the interface,
each with a fixed meaning regardless of the backend in use, as discussed in
the RFC thread. Otherwise, it gets too confusing. On the sysfs side, the
existing max_active would become compat only, with max_active_percpu and
max_active_total as the main interfaces. The kernel API would match:
workqueue_set_max_active_percpu() and workqueue_set_max_active_total(),
with the conversion to the other domain done in those setters.

This patch only makes sense as a way to get there, and it's difficult to
tell whether it's in line with that without the actual conversion.

> +static int wq_user_max_active(struct workqueue_struct *wq)
> +{
> +	if (wq->flags & WQ_UNBOUND)
> +		return READ_ONCE(wq->saved_max_active);
> +
> +	return READ_ONCE(wq->saved_percpu_max_active);
> +}

Maybe write it as "if () return X; else return Y;"? Neither branch is the
special case. The same goes for wq_nr_effective_cpus() and wherever the
choice between the two domains ends up.

> -	if (!from_cancel && (wq->saved_max_active == 1 || wq->rescuer))
> +	if (!from_cancel && (wq_user_max_active(wq) == 1 || wq->rescuer))

I wonder whether this condition is right. It should trigger when the
effective max_active is 1, whether the workqueue is in percpu or unbound
mode, but this only looks at the domain the workqueue was created in. Once a
workqueue can switch, the limit in force may be the derived one, and that
can reach 1 even for a workqueue created with a higher max_active, e.g.
after min_active is lowered.

Maybe this means there should be further restrictions on how low max_active
can be pushed. For example, a workqueue created with max_active > 1
shouldn't be driven down to 1 in either domain, however it gets adjusted, so
that whether a workqueue can end up at 1 is decided by the max_active it was
created with. One way could be a floor of 2 in the setters and the
derivation for workqueues created with max_active > 1, while one created
with max_active 1 can still go back to 1, e.g. after being raised. That's
just one option though.

> +static int wq_nr_online_cpus(const struct cpumask *mask)
> +{
> +	return max(cpumask_weight_and(mask, cpu_online_mask), 1);
> +}
> ...
> +static int wq_scale_to_percpu(int max_active, int min_active, int nr_cpus)
> +{
> +	return max(DIV_ROUND_UP(max_active, nr_cpus), min_active);
> +}

I'm not sure single-use helpers without any comment are an improvement. They
seem to obscure more than they help. Maybe open-code them with comments
explaining why? That would include wq_nr_online_cpus(),
wq_nr_effective_cpus(), wq_scale_to_unbound(), wq_scale_to_percpu() and
wq_init_bh_max_active().

> +/*
> + * Derive the saved limits of the max_active domain @wq's backend doesn't use
> + * from those of the one it does, with the current number of usable CPUs, and
> + * publish both.
> + */
> +static void wq_rescale_max_active(struct workqueue_struct *wq)

Would it make sense to make the rescale a part of wq_adjust_max_active() and
use that everywhere, on init, any config change, freeze and so on? With the
separate setters, the workqueue could remember which domain was set last,
and the derivation would always go from that one with the current CPU count,
so calling it again couldn't ratchet the limits. BH would stay out, and
hotplug could keep using the saved values.

Thanks.

--
tejun