Re: [PATCH wq/for-7.4 v3 2/3] workqueue: Keep the limits of both max_active domains current

[Breno Leitao <leitao@debian.org>]

Hello Tejun,

On Fri, Oct 16, 2026 at 07:21:06AM -1000, Tejun Heo wrote:
> On Fri, Oct 09, 2026 at 05:18:35AM -0700, Breno Leitao wrote:
> > Given that we are introducing a new PERCPU affinity scope which will be
> > able to do CM, we want to keep both values set, so, the transition from
> > scopes would be able to succeed. This cannot be done under the flip, given
> > this is RCU protected. See discussion at [1]
>
> "CM" and "the flip" aren't defined, and concurrency management doesn't
> come into this patch. The limits are WRITE_ONCE()/READ_ONCE() fields, not
> RCU-protected. Maybe say directly that a later scope switch should land on
> a sensible limit, so each setter derives the other domain?

Right, the RCU part was about the pwq table being swapped one CPU at a
time, not about the limits, and it doesn't belong in this patch. I will
reword it as you suggest.

> > Use the scale function suggested by Tejun in [1].
>
> The mapping is only in the cover letter, which doesn't land in git. Maybe
> spell out the conversions here?

Ack, the U/L/P/N block moves into the changelog.

> N is sampled only when a setter runs. Narrowing the cpumask afterwards
> through apply_workqueue_attrs() or sysfs redistributes max_active across
> the nodes but leaves percpu_max_active alone. max_active 256 on 64 CPUs
> gives percpu_max_active 8, and it stays 8 after narrowing to 4 CPUs where
> 64 would match. Nothing reads it yet. Should apply_wqattrs_commit()
> rederive it too?

I think so. N is meant to match the CPUs wq_update_node_max_active()
spreads max_active over, and apply_wqattrs_commit() is what changes that
mask.

For v4 I am moving the conversion into a helper that derives the saved
limits of the other domain from the backend's own, as you suggested on
3/3. The two setters, apply_wqattrs_commit() and
workqueue_init_topology() call it, followed by wq_adjust_max_active().
apply_wqattrs_commit() also runs at creation, so wq_init_max_active()
only needs to set the backend's own limits. Hotplug keeps the saved
values, as you suggested on v1.

apply_wqattrs_commit() runs on scope changes too, which is harmless
today: a scope change doesn't change N, and the helper always derives
from the domain of the backend the workqueue was created on. Once the
PERCPU scope can switch backends, it has to keep deriving from the
domain the user set. Otherwise switching back and forth ratchets the
limit up: max_active 256 on 64 CPUs gives percpu_max_active 8, which
scales back to 512.

> > +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);
> > +}
>
> The reads are marked but the setters write these fields with plain
> stores. Maybe pair them with WRITE_ONCE()?

Ack!

> > -	if (!from_cancel && (wq->saved_max_active == 1 || wq->rescuer))
> > +	if (!from_cancel && (wq_user_max_active(wq) == 1 || wq->rescuer))
>
> The description doesn't explain this and the max_active_show() change.
> For a percpu workqueue, saved_max_active now holds the derived unbound
> limit, so these readers switch to the backend's own limit. Maybe add a
> sentence?

Ack!

> > +	if (!installed_pwq(wq, -1))
> > +		return wq_nr_online_cpus(wq_unbound_cpumask);
>
> This tests for a percpu workqueue indirectly. Percpu workqueues never get
> a dfl_pwq (apply_wqattrs_prepare() skips it for WQ_PERCPU), and unbound
> ones always have one by the time a setter runs. Maybe test WQ_UNBOUND
> directly?

Ack!

> > +	 * Which domain a pwq honours follows its pool, see
> > +	 * pwq_tryinc_nr_active(). Keeping both current means a pwq is never
> > +	 * metered against a limit nobody set.
>
> Today a pwq's pool always matches its workqueue's backend, so this
> describes the future PERCPU scope. Maybe state the contract instead, that
> both are kept current so that a pwq on either kind of pool has a valid
> limit?

Ack, I will take your wording.

Thanks for the review,
--breno