Re: [RFC PATCH 0/3] workqueue: Enable unbound cpumask update on ordered workqueues

[Waiman Long <longman@redhat.com>]

On 2/1/24 05:18, Juri Lelli wrote:
> On 31/01/24 10:31, Waiman Long wrote:
>> On 1/31/24 08:01, Juri Lelli wrote:
>>> Hi Waiman,
>>>
>>> Thanks for working on this!
>>>
>>> On 30/01/24 13:33, Waiman Long wrote:
>>>> Ordered workqueues does not currently follow changes made to the
>>>> global unbound cpumask because per-pool workqueue changes may break
>>>> the ordering guarantee. IOW, a work function in an ordered workqueue
>>>> may run on a cpuset isolated CPU.
>>>>
>>>> This series enables ordered workqueues to follow changes made to the
>>>> global unbound cpumask by temporaily saving the work items in an
>>>> internal queue until the old pwq has been properly flushed and to be
>>>> freed. At that point, those work items, if present, are queued back to
>>>> the new pwq to be executed.
>>> I took it for a quick first spin (on top of wq/for-6.9) and this is what
>>> I'm seeing.
>>>
>>> Let's take edac-poller ordered wq, as the behavior seems to be the same
>>> for the rest.
>>>
>>> Initially we have (using wq_dump.py)
>>>
>>> wq_unbound_cpumask=0xffffffff 000000ff
>>> ...
>>> pool[80] ref= 44 nice=  0 idle/workers=  2/  2 cpus=0xffffffff 000000ff pod_cpus=0xffffffff 000000ff
>>> ...
>>> edac-poller                      ordered    80 80 80 80 80 80 80 80 ...
>>> ...
>>> edac-poller                      0xffffffff 000000ff    345 0xffffffff 000000ff
>>>
>>> after I
>>>
>>> # echo 3 >/sys/devices/virtual/workqueue/cpumask
>>>
>>> I get
>>>
>>> wq_unbound_cpumask=00000003
>>> ...
>>> pool[86] ref= 44 nice=  0 idle/workers=  2/  2 cpus=00000003 pod_cpus=00000003
>>> ...
>>> edac-poller                      ordered    86 86 86 86 86 86 86 86 86 86 ...
>>> ...
>>> edac-poller                      0xffffffff 000000ff    345 0xffffffff 000000ff
>>>
>>> So, IIUC, the pool and wq -> pool settings are updated correctly, but
>>> the wq.unbound_cpus (and its associated rescure affinity) are left
>>> untouched. Is this expected or are we maybe still missing an additional
>>> step?
>> Isn't this what the 4th patch of your RFC workqueue patch series does?
>>
>> https://lore.kernel.org/lkml/20240116161929.232885-5-juri.lelli@redhat.com/
>>
>> The focus of this series is to make sure that we can update the pool cpumask
>> of ordered workqueue to follow changes in global unbound workqueue cpumask.
>> So I haven't touched anything related to rescuer at all.
> My patch only uses the wq->unbound_attrs->cpumask to change the
> associated rescuer cpumask, but I don't think your series modifies the
> former?

I don't think so. The calling sequence of apply_wqattrs_prepare() and 
apply_wqattrs_commit() will copy unbound_cpumask into ctx->attrs which 
is copied into unbound_attrs. So unbound_attrs->cpumask should reflect 
the new global unbound cpumask. This code is there all along. The only 
difference is that ordered workqueues were skipped for unbound cpumask 
update before. This patch series now includes those ordered workqueues 
when the unbound cpumask is updated.

Cheers,
Longman