Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Message-ID: <59eaab91-9102-3a13-b787-f0409314bb4d@redhat.com>
Date:   Wed, 11 Oct 2023 10:42:58 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.14.0
Subject: Re: [PATCH] workqueue: Override implicit ordered attribute in
 workqueue_apply_unbound_cpumask()
Content-Language: en-US
To:     Lai Jiangshan <jiangshanlai@gmail.com>
Cc:     Tejun Heo <tj@kernel.org>, linux-kernel@vger.kernel.org
References: <20231011024842.3348767-1-longman@redhat.com>
 <CAJhGHyDc540v+45xNx4mtWF9O=3soxEh_p7eyGQGP48n+_o2Vg@mail.gmail.com>
From:   Waiman Long <longman@redhat.com>
In-Reply-To: <CAJhGHyDc540v+45xNx4mtWF9O=3soxEh_p7eyGQGP48n+_o2Vg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 10/11/23 08:16, Lai Jiangshan wrote:
> On Wed, Oct 11, 2023 at 10:49 AM Waiman Long <longman@redhat.com> wrote:
>> Commit 5c0338c68706 ("workqueue: restore WQ_UNBOUND/max_active==1
>> to be ordered") enabled implicit ordered attribute to be added to
>> WQ_UNBOUND workqueues with max_active of 1. This prevented the changing
>> of attributes to these workqueues leading to fix commit 0a94efb5acbb
>> ("workqueue: implicit ordered attribute should be overridable").
>>
>> However, workqueue_apply_unbound_cpumask() was not updated at that time.
>> So sysfs changes to wq_unbound_cpumask has no effect on WQ_UNBOUND
>> workqueues with implicit ordered attribute. Since not all WQ_UNBOUND
>> workqueues are visible on sysfs, we are not able to make all the
>> necessary cpumask changes even if we iterates all the workqueue cpumasks
>> in sysfs and changing them one by one.
>>
>> Fix this problem by applying the corresponding change made
>> to apply_workqueue_attrs_locked() in the fix commit to
>> workqueue_apply_unbound_cpumask().
>>
>> Fixes: 5c0338c68706 ("workqueue: restore WQ_UNBOUND/max_active==1 to be ordered")
>> Signed-off-by: Waiman Long <longman@redhat.com>
> Hello Waiman Long
>
> Thanks for the fix.
>
>> ---
>>   kernel/workqueue.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
>> index d141bd8eb2b7..19d403aa41b0 100644
>> --- a/kernel/workqueue.c
>> +++ b/kernel/workqueue.c
>> @@ -5785,9 +5785,13 @@ static int workqueue_apply_unbound_cpumask(const cpumask_var_t unbound_cpumask)
>>          list_for_each_entry(wq, &workqueues, list) {
>>                  if (!(wq->flags & WQ_UNBOUND))
>>                          continue;
>> +
>>                  /* creating multiple pwqs breaks ordering guarantee */
>> -               if (wq->flags & __WQ_ORDERED)
>> -                       continue;
>> +               if (!list_empty(&wq->pwqs)) {
> I don't remember why the same test is needed in 0a94efb5acbb.
> And I can't figure it out now.
>
> I think it needs some comments or to be removed.

Is it because there will be no active work if there is no pool 
workqueue? Anyway, I just make it to be the same as that in 
apply_workqueue_attrs_locked() as the function call sequence are 
similar. If we remove the list_empty() test, we will have to remove it 
in both.

Cheers,
Longman

>
> Thanks
> Lai
>