Received-SPF: pass (google.com: domain of linux-kernel+bounces-27718-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Sender: Tejun Heo <htejun@gmail.com>
Date: Tue, 16 Jan 2024 08:57:24 -1000
From: Tejun Heo <tj@kernel.org>
To: Juri Lelli <juri.lelli@redhat.com>
Cc: Lai Jiangshan <jiangshanlai@gmail.com>,
	Aaron Tomlin <atomlin@atomlin.com>,
	Valentin Schneider <vschneid@redhat.com>,
	Waiman Long <longman@redhat.com>, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH 3/4] kernel/workqueue: Distinguish between general
 unbound and WQ_SYSFS cpumask changes
Message-ID: <ZabRlEklmuqwFPj-@slm.duckdns.org>
References: <20240116161929.232885-1-juri.lelli@redhat.com>
 <20240116161929.232885-4-juri.lelli@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240116161929.232885-4-juri.lelli@redhat.com>

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@redhat.com>
> ---
>  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