Received-SPF: pass (google.com: domain of linux-kernel+bounces-4927-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Tue, 19 Dec 2023 09:55:21 +0100
From: Juri Lelli <juri.lelli@redhat.com>
To: Tejun Heo <tj@kernel.org>
Cc: Aaron Tomlin <atomlin@atomlin.com>, linux-kernel@vger.kernel.org,
	jiangshanlai@gmail.com, peterz@infradead.org
Subject: Re: [RFC PATCH 0/2] workqueue: Introduce PF_WQ_RESCUE_WORKER
Message-ID: <ZYFaeR-83eYNWQIz@localhost.localdomain>
References: <ZXdXdBzvbkI4Y4fL@slm.duckdns.org>
 <ZXguMgcKLCLn16T4@localhost.localdomain>
 <ZXiVCOKk90Fjpmhw@slm.duckdns.org>
 <ZXlyfjDsFGbYcMU6@localhost.localdomain>
 <ZXnPVtISKQ2JFDNn@slm.duckdns.org>
 <ZXn4qiMetd7zY1sb@localhost.localdomain>
 <ZXn6J5bN-dPC1WSk@slm.duckdns.org>
 <ZXrmJYvekzrLSaGo@localhost.localdomain>
 <ZXtb066P-ZnjxfgK@slm.duckdns.org>
 <ZXv3RnYNkpaPGYb_@localhost.localdomain>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZXv3RnYNkpaPGYb_@localhost.localdomain>

Hello again,

On 15/12/23 07:50, Juri Lelli wrote:
> On 14/12/23 09:47, Tejun Heo wrote:
> > Hello,
> > 
> > On Thu, Dec 14, 2023 at 12:25:25PM +0100, Juri Lelli wrote:
> > > > So, we have to use set_cpus_allowed_ptr() but we still don't want to change
> > > > the affinity of a rescuer which is already running a task for a pool.
> > > 
> > > But then, even today, a rescuer might keep handling work on a cpu
> > > outside its wq cpumask if the associated wq cpumask change can proceed
> > > w/o waiting for it to finish the iteration?
> > 
> > Yeah, that can happen and pool cpumasks naturally being subsets of the wq's
> > cpumask that they're serving, your original approach likely isn't broken
> > either.
> > 
> > > BTW, apologies for all the questions, but I'd like to make sure I can
> > > get the implications hopefully right. :)
> > 
> > I obviously haven't thought through it very well, so thanks for the
> > questions. So, yeah, I think we actually need to set the rescuer's cpumask
> > when wq's cpumask changes and doing it where you were suggesting should
> > probably work.
> 
> OK. Going to send a proper patch asap.

I actually didn't do that yet as it turns out the proposed approach
doesn't cover !WQ_SYSFS unbounded wqs. Well, I thought those should be
covered as well, since we have (initiated by echo <mask> into
/sys/devices/virtual/workqueue/cpumask)

workqueue_apply_unbound_cpumask ->
  apply_wqattrs_commit

but for some reason the mask change is not reflected into rescuers
affinity.

Trying to dig deeper I went ahead and extended the recent wq_dump.py
addition with the following

---
ls/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('')
---

which shows the following situation after an

# echo 00,00000003 > /sys/devices/virtual/workqueue/cpumask

on the system I'm testing with:

...
Workqueue -> rescuer
=====================
wq_unbound_cpumask=00000003

[    workqueue     \     type            unbound_cpumask     rescuer                  pid   cpumask]
events                   percpu
events_highpri           percpu
events_long              percpu
events_unbound           unbound    0xffffffff 000000ff
events_freezable         percpu
events_power_efficient   percpu
events_freezable_power_  percpu
rcu_gp                   percpu                              kworker/R-rcu_g              4 0xffffffff 000000ff
rcu_par_gp               percpu                              kworker/R-rcu_p              5 0xffffffff 000000ff
slub_flushwq             percpu                              kworker/R-slub_              6 0xffffffff 000000ff
netns                    ordered    0xffffffff 000000ff      kworker/R-netns              7 0xffffffff 000000ff
mm_percpu_wq             percpu                              kworker/R-mm_pe             13 0xffffffff 000000ff
cpuset_migrate_mm        ordered    0xffffffff 000000ff
inet_frag_wq             percpu                              kworker/R-inet_            300 0xffffffff 000000ff
pm                       percpu
cgroup_destroy           percpu
cgroup_pidlist_destroy   percpu
writeback                unbound    0xffffffff 000000ff      kworker/R-write            308 0xffffffff 000000ff
cgwb_release             percpu
cryptd                   percpu                              kworker/R-crypt            314 0xffffffff 000000ff
kintegrityd              percpu                              kworker/R-kinte            315 0xffffffff 000000ff
kblockd                  percpu                              kworker/R-kbloc            316 0xffffffff 000000ff
kacpid                   percpu
kacpi_notify             percpu
kacpi_hotplug            ordered    0xffffffff 000000ff
kec                      ordered    0xffffffff 000000ff
kec_query                percpu
tpm_dev_wq               percpu                              kworker/R-tpm_d            352 0xffffffff 000000ff
usb_hub_wq               percpu
md                       percpu                              kworker/R-md               353 0xffffffff 000000ff
md_misc                  percpu
md_bitmap                unbound    0xffffffff 000000ff      kworker/R-md_bi            354 0xffffffff 000000ff
edac-poller              ordered    0xffffffff 000000ff      kworker/R-edac-            355 0xffffffff 000000ff
...

I guess I expected wq_unbound_cpumask and unbound_cpumask for each
unbound wq to be kept in sync, so I'm evidently missing details. :)

Can you please help me here understanding what am I missing?

Thanks!
Juri