[RFC PATCH 0/4] Fix handling of rescuers affinity

[RFC PATCH 0/4] Fix handling of rescuers affinity

[Juri Lelli]

Hello,

Recently I've been pointed at the fact that workqueue rescuers seem not
to follow unbound workqueues cpumask changes. This small series is a
first stab at possibly fixing what I considered to be different from
what I expected (it might very well be the case that my expectations are
wrong :). Long story short, it seems to me that we currently have
several cases where a change of the general unbound cpumask or a change
of the per-workqueue cpumask (for WQ_SYSFS workqueues) is not reflected
into the corresponding rescuer affinity (if a rescuer is present).

In the following:

 Patch 01/04 - Adds debug information to wq_dump.py script so that we
               can more easily check workqueues and rescuers cpumasks
       02/04 - Fixes cpumask discrepancies when rescuers are created
       03/04 - Streamlines behavior of general unbound vs. WQ_SYSFS
               cpumask changes
       04/04 - Makes sure existing rescuers affinity follows their
               workqueue cpumask changes

Please take a look, I'm all for feedback and better understanding of the
details I'm certainly missing.

For additional context, a related discussion can be found at

https://lore.kernel.org/lkml/um77hym4t6zyypfbhwbaeqxpfdzc657oa7vgowdfah7cuctjak@pexots3mfb24/

Branch for testing available at

git@github.com:jlelli/linux.git workqueue/rescuers-cpumask

Best,
Juri

Juri Lelli (4):
  workqueue: Add rescuers printing to wq_dump.py
  kernel/workqueue: Bind rescuer to unbound cpumask for WQ_UNBOUND
  kernel/workqueue: Distinguish between general unbound and WQ_SYSFS
    cpumask changes
  kernel/workqueue: Let rescuers follow unbound wq cpumask changes

 kernel/workqueue.c         | 28 +++++++++++++++++++++-------
 tools/workqueue/wq_dump.py | 29 +++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+), 7 deletions(-)

-- 
2.43.0

[RFC PATCH 1/4] tools/workqueue: Add rescuers printing to wq_dump.py

[Juri Lelli]

Retrieving rescuers information (e.g., affinity and name) is quite
useful when debugging workqueues configurations.

Add printing of such information to the existing wq_dump.py script.

Signed-off-by: Juri Lelli <juri.lelli@redhat.com>
---
 tools/workqueue/wq_dump.py | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/tools/workqueue/wq_dump.py b/tools/workqueue/wq_dump.py
index d0df5833f2c18..6da621989e210 100644
--- a/tools/workqueue/wq_dump.py
+++ b/tools/workqueue/wq_dump.py
@@ -175,3 +175,32 @@ for wq in list_for_each_entry('struct workqueue_struct', workqueues.address_of_(
     if wq.flags & WQ_UNBOUND:
         print(f' {wq.dfl_pwq.pool.id.value_():{max_pool_id_len}}', end='')
     print('')
+
+print('')
+print('Workqueue -> rescuer')
+print('=====================')
+print(f'wq_unbound_cpumask={cpumask_str(wq_unbound_cpumask)}')
+print('')
+print('[    workqueue     \     type            unbound_cpumask     rescuer                  pid   cpumask]')
+
+for wq in list_for_each_entry('struct workqueue_struct', workqueues.address_of_(), 'list'):
+    print(f'{wq.name.string_().decode()[-24:]:24}', end='')
+    if wq.flags & WQ_UNBOUND:
+        if wq.flags & WQ_ORDERED:
+            print(' ordered   ', end='')
+        else:
+            print(' unbound', end='')
+            if wq.unbound_attrs.affn_strict:
+                print(',S ', end='')
+            else:
+                print('   ', end='')
+        print(f' {cpumask_str(wq.unbound_attrs.cpumask):24}', end='')
+    else:
+        print(' percpu    ', end='')
+        print('                         ', end='')
+
+    if wq.flags & WQ_MEM_RECLAIM:
+        print(f' {wq.rescuer.task.comm.string_().decode()[-24:]:24}', end='')
+        print(f' {wq.rescuer.task.pid.value_():5}', end='')
+        print(f' {cpumask_str(wq.rescuer.task.cpus_ptr)}', end='')
+    print('')
-- 
2.43.0

[RFC PATCH 2/4] kernel/workqueue: Bind rescuer to unbound cpumask for WQ_UNBOUND

[Juri Lelli]

At the time they are created unbound workqueues rescuers currently use
cpu_possible_mask as their affinity, but this can be too wide in case a
workqueue unbound mask has been set as a subset of cpu_possible_mask.

Make new rescuers use their associated workqueue unbound cpumask from
the start.

Signed-off-by: Juri Lelli <juri.lelli@redhat.com>
---
 kernel/workqueue.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 76e60faed8923..3a1d5a67bd66a 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -4652,7 +4652,10 @@ static int init_rescuer(struct workqueue_struct *wq)
 	}
 
 	wq->rescuer = rescuer;
-	kthread_bind_mask(rescuer->task, cpu_possible_mask);
+	if (wq->flags & WQ_UNBOUND)
+		kthread_bind_mask(rescuer->task, wq->unbound_attrs->cpumask);
+	else
+		kthread_bind_mask(rescuer->task, cpu_possible_mask);
 	wake_up_process(rescuer->task);
 
 	return 0;
-- 
2.43.0

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

[Juri Lelli]

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);
 	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;
-
 	ctx->wq = wq;
 	return ctx;
 
-- 
2.43.0

[RFC PATCH 4/4] kernel/workqueue: Let rescuers follow unbound wq cpumask changes

[Juri Lelli]

When workqueue cpumask changes are committed the associated rescuer (if
one exists) affinity is not touched and this might be a problem down the
line for isolated setups.

Make sure rescuers affinity is updated every time a workqueue cpumask
changes, so that rescuers can't break isolation.

Signed-off-by: Juri Lelli <juri.lelli@redhat.com>
---
 kernel/workqueue.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 2ef6573909070..df7f2f2bfd0c8 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -4416,6 +4416,12 @@ static void apply_wqattrs_commit(struct apply_wqattrs_ctx *ctx)
 	link_pwq(ctx->dfl_pwq);
 	swap(ctx->wq->dfl_pwq, ctx->dfl_pwq);
 
+	/* rescuer needs to respect wq cpumask changes */
+	if (ctx->wq->rescuer) {
+		set_cpus_allowed_ptr(ctx->wq->rescuer->task, ctx->attrs->cpumask);
+		wake_up_process(ctx->wq->rescuer->task);
+	}
+
 	mutex_unlock(&ctx->wq->mutex);
 }
 
-- 
2.43.0

Re: [RFC PATCH 2/4] kernel/workqueue: Bind rescuer to unbound cpumask for WQ_UNBOUND

[Tejun Heo]

On Tue, Jan 16, 2024 at 05:19:27PM +0100, Juri Lelli wrote:
> At the time they are created unbound workqueues rescuers currently use
> cpu_possible_mask as their affinity, but this can be too wide in case a
> workqueue unbound mask has been set as a subset of cpu_possible_mask.
> 
> Make new rescuers use their associated workqueue unbound cpumask from
> the start.
> 
> Signed-off-by: Juri Lelli <juri.lelli@redhat.com>

Apply 1-2 to wq/for-6.9.

Thanks.

-- 
tejun

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

Re: [RFC PATCH 4/4] kernel/workqueue: Let rescuers follow unbound wq cpumask changes

[Lai Jiangshan]

Hello, Juri

On Wed, Jan 17, 2024 at 12:20 AM Juri Lelli <juri.lelli@redhat.com> wrote:

> +       /* rescuer needs to respect wq cpumask changes */
> +       if (ctx->wq->rescuer) {
> +               set_cpus_allowed_ptr(ctx->wq->rescuer->task, ctx->attrs->cpumask);
> +               wake_up_process(ctx->wq->rescuer->task);
> +       }
> +

What's the reason to wake up the rescuer?

I support this patch except for the wakeup:
Reviewed-by: Lai Jiangshan <jiangshanlai@gmail.com>

Thanks

Lai

Re: [RFC PATCH 4/4] kernel/workqueue: Let rescuers follow unbound wq cpumask changes

[Juri Lelli]

Hello Lai,

On 17/01/24 11:56, Lai Jiangshan wrote:
> Hello, Juri
> 
> On Wed, Jan 17, 2024 at 12:20 AM Juri Lelli <juri.lelli@redhat.com> wrote:
> 
> > +       /* rescuer needs to respect wq cpumask changes */
> > +       if (ctx->wq->rescuer) {
> > +               set_cpus_allowed_ptr(ctx->wq->rescuer->task, ctx->attrs->cpumask);
> > +               wake_up_process(ctx->wq->rescuer->task);
> > +       }
> > +
> 
> What's the reason to wake up the rescuer?

I believe we want to wake it up so that it can possibly be moved
"instantly" to a cpu inside its new cpumask affinity. If we don't wake
it up it might be sleeping on a cpu outside its affinity which might
have become isolated and this cpu could be affected by that wakeup if
that only happens later on when possibly the rescuer needs to perform
some work.

Does is make more sense to you?

> I support this patch except for the wakeup:
> Reviewed-by: Lai Jiangshan <jiangshanlai@gmail.com>

Thanks for looking at this!

Juri

Re: [RFC PATCH 2/4] kernel/workqueue: Bind rescuer to unbound cpumask for WQ_UNBOUND

[Juri Lelli]

Hello,

On 16/01/24 08:47, Tejun Heo wrote:
> On Tue, Jan 16, 2024 at 05:19:27PM +0100, Juri Lelli wrote:
> > At the time they are created unbound workqueues rescuers currently use
> > cpu_possible_mask as their affinity, but this can be too wide in case a
> > workqueue unbound mask has been set as a subset of cpu_possible_mask.
> > 
> > Make new rescuers use their associated workqueue unbound cpumask from
> > the start.
> > 
> > Signed-off-by: Juri Lelli <juri.lelli@redhat.com>
> 
> Apply 1-2 to wq/for-6.9.

Thank you!

Best,
Juri

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

[Juri Lelli]

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

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

[Tejun Heo]

Hello,

On Wed, Jan 17, 2024 at 02:06:08PM +0100, Juri Lelli wrote:
> > 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?

They aren't special and should follow the normal unbound workqueue cpumask.

Thanks.

-- 
tejun

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

[Waiman Long]

On 1/17/24 12:12, Tejun Heo wrote:
> Hello,
>
> On Wed, Jan 17, 2024 at 02:06:08PM +0100, Juri Lelli wrote:
>>> 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?
> They aren't special and should follow the normal unbound workqueue cpumask.

My impression is that changing the workqueue cpumask of ordered unbound 
workqueue may break the ordering guarantee momentarily. I was planning 
to look into this further to see if that is true when I have time. If it 
is not a concern, we should certainly apply the global unbound cpumask 
change to those workqueues as well.

Cheers,
Longman

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

[Tejun Heo]

Hello,

On Wed, Jan 17, 2024 at 02:32:34PM -0500, Waiman Long wrote:
> My impression is that changing the workqueue cpumask of ordered unbound
> workqueue may break the ordering guarantee momentarily. I was planning to

Ah, you're right. Changing cpumask would require changing the dfl_pwq and
that can introduce extra concurrency and break ordering and it's exempt from
unbound_cpumask updates. We likely need to add a mechanism for updating
ordered wq's so that the new pwq doesn't become until the previous one is
drained.

Thanks.

-- 
tejun

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

[Juri Lelli]

On 17/01/24 09:42, Tejun Heo wrote:
> Hello,
> 
> On Wed, Jan 17, 2024 at 02:32:34PM -0500, Waiman Long wrote:
> > My impression is that changing the workqueue cpumask of ordered unbound
> > workqueue may break the ordering guarantee momentarily. I was planning to
> 
> Ah, you're right. Changing cpumask would require changing the dfl_pwq and
> that can introduce extra concurrency and break ordering and it's exempt from
> unbound_cpumask updates. We likely need to add a mechanism for updating
> ordered wq's so that the new pwq doesn't become until the previous one is
> drained.

Thanks for the additional info! Guess I'll need to think more about this
and possibly coordinate the effort with Waiman.

Best,
Juri