Re: [RFC PATCH 3/4] kernel/workqueue: Distinguish between general unbound and WQ_SYSFS cpumask changes

[Tejun Heo]

Hello, Juri.

On Tue, Jan 16, 2024 at 05:19:28PM +0100, Juri Lelli wrote:
> Both general unbound cpumask and per-wq (WQ_SYSFS) cpumask changes end
> up calling apply_wqattrs_prepare for preparing for the change, but this
> doesn't work well for general unbound cpumask changes as current
> implementation won't be looking at the new unbound_cpumask.
> 
> Fix the prepare phase for general unbound cpumask changes by checking
> which type of attributes (general vs. WQ_SYSFS) are actually changing.
> 
> Signed-off-by: Juri Lelli <juri.lelli@xxxxxxxxxx>
> ---
>  kernel/workqueue.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 3a1d5a67bd66a..2ef6573909070 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -4359,7 +4359,17 @@ apply_wqattrs_prepare(struct workqueue_struct *wq,
>  	 * it even if we don't use it immediately.
>  	 */
>  	copy_workqueue_attrs(new_attrs, attrs);
> -	wqattrs_actualize_cpumask(new_attrs, unbound_cpumask);
> +
> +	/*
> +	 * Is the user changing the general unbound_cpumask or is this a
> +	 * WQ_SYSFS cpumask change?
> +	 */
> +	if (attrs == wq->unbound_attrs)
> +		cpumask_copy(new_attrs->cpumask, unbound_cpumask);
> +	else
> +		wqattrs_actualize_cpumask(new_attrs, unbound_cpumask);
> +
> +	cpumask_and(new_attrs->cpumask, new_attrs->cpumask, cpu_possible_mask);

This looks rather hacky. Can you elaborate how the current code misbehaves
with an example? For the unbound_cpumask update path, the intention is
supplying the current ->attrs (wq user's request) and the new
unbound_cpumask expecting wqattrs_actualize_cpumask() to do the right thing.
Is the problem that you want to have access to the effective cpumask for the
entire workqueue? If so, we don't want to do that with ctx->attrs as that's
supposed to carry the user's requested configuration rather than the
currently effective. We can just add a new field.

>  	cpumask_copy(new_attrs->__pod_cpumask, new_attrs->cpumask);
>  	ctx->dfl_pwq = alloc_unbound_pwq(wq, new_attrs);
>  	if (!ctx->dfl_pwq)
> @@ -4377,12 +4387,7 @@ apply_wqattrs_prepare(struct workqueue_struct *wq,
>  		}
>  	}
>  
> -	/* save the user configured attrs and sanitize it. */
> -	copy_workqueue_attrs(new_attrs, attrs);
> -	cpumask_and(new_attrs->cpumask, new_attrs->cpumask, cpu_possible_mask);
> -	cpumask_copy(new_attrs->__pod_cpumask, new_attrs->cpumask);
>  	ctx->attrs = new_attrs;

This part exists to store the user requested configuration rather than the
applied effective configuration, so that e.g. later if the unbound_cpumask
changes, we can apply the effective mask according to the user's latest
requested configuration. I'm not sure why this is being dropped.

Thanks.

-- 
tejun