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

[Breno Leitao <leitao@debian.org>]

Hello Tejun,

On Thu, Oct 29, 2026 at 09:07:56AM -1000, Tejun Heo wrote:
> 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.

Agree, I tried to separate them make it easier to review, but, it was
not ideal.  I will pause this for now, and come back with the actual
coversion.

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

Ack. I was following netdev's approch of having `if (foo) return bar; return
baz`.

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

Sure. Should the floor cover min_active as well?

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

Ack.

> > +/*
> > + * 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.

Yes, that is simpler, and it fixes the ratchet I raised on v3.
wq_rescale_max_active() goes away. The workqueue records which domain
was set last, at creation and in each setter, and wq_adjust_max_active()
derives the other one from it with the current CPU count before
publishing both.

BH stays out, and hotplug only calls wq_update_node_max_active(), so it
keeps using the saved values.

Thanks for the review,
--breno