Received-SPF: pass (google.com: domain of linux-kernel+bounces-29002-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Wed, 17 Jan 2024 14:06:08 +0100
From: Juri Lelli <juri.lelli@redhat.com>
To: Tejun Heo <tj@kernel.org>
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: <ZafQwMw8ZKztunMU@localhost.localdomain>
References: <20240116161929.232885-1-juri.lelli@redhat.com>
 <20240116161929.232885-4-juri.lelli@redhat.com>
 <ZabRlEklmuqwFPj-@slm.duckdns.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZabRlEklmuqwFPj-@slm.duckdns.org>

On 16/01/24 08:57, Tejun Heo wrote:
> 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?

I was trying to address the fact that ordered unbound workqueues didn't
seem to reflect unbound_cpumask changes, e.g.

wq_unbound_cpumask=00000003

edac-poller              ordered,E  0xffffffff 000000ff      kworker/R-edac-            351 0xffffffff 000000ff

vs. 

edac-poller              ordered,E  00000003                 kworker/R-edac-            349 00000003

with the patch applied. But honestly, I'm now also not convinced what
I'm proposing is correct, so I'll need to think more about it.

Can you please confirm though that ordered unbound workqueues are not
"special" for some reason and we would like them to follow
unbound_cpumask changes as normal ubound workqueues?

Thanks,
Juri