linux-kernel - Re: [RFC PATCH 3/4] kernel/workqueue: Distinguish between general unbound and WQ

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZafQwMw8ZKztunMU@localhost.localdomain>
Date: Wed, 17 Jan 2024 14:06:08 +0100
From: Juri Lelli <juri.lelli@...hat.com>
To: Tejun Heo <tj@...nel.org>
Cc: Lai Jiangshan <jiangshanlai@...il.com>,
	Aaron Tomlin <atomlin@...mlin.com>,
	Valentin Schneider <vschneid@...hat.com>,
	Waiman Long <longman@...hat.com>, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 3/4] kernel/workqueue: Distinguish between general
 unbound and WQ_SYSFS cpumask changes

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@...hat.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