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

[Tejun Heo <tj@kernel.org>]

Hello, Breno.

The following is a Claude-generated review.

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?

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

> N is the online CPUs the unbound backend of the workqueue may actually
> use. For a workqueue whose own cpumask has been narrowed, that is
> unbound_effective_cpumask(), not the global wq_unbound_cpumask -- the
> latter overcounts and makes percpu_max_active come out too low.
> wq_update_node_max_active() already distributes max_active the same way;
> match it.

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?

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

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

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

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

Thanks.

-- 
tejun