[PATCH] workqueue: Override implicit ordered attribute in workqueue_apply_unbound_cpumask()

[PATCH] workqueue: Override implicit ordered attribute in workqueue_apply_unbound_cpumask()

[Waiman Long]

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>
---
 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)) {
+			if (wq->flags & __WQ_ORDERED_EXPLICIT)
+				continue;
+			wq->flags &= ~__WQ_ORDERED;
+		}
 
 		ctx = apply_wqattrs_prepare(wq, wq->unbound_attrs, unbound_cpumask);
 		if (IS_ERR(ctx)) {
-- 
2.39.3

Re: [PATCH] workqueue: Override implicit ordered attribute in workqueue_apply_unbound_cpumask()

[Lai Jiangshan]

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.

Thanks
Lai

Re: [PATCH] workqueue: Override implicit ordered attribute in workqueue_apply_unbound_cpumask()

[Waiman Long]

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
>

Re: [PATCH] workqueue: Override implicit ordered attribute in workqueue_apply_unbound_cpumask()

[Tejun Heo]

On Tue, Oct 10, 2023 at 10:48:42PM -0400, Waiman Long 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>

Applied to wq/for-6.6-fixes.

Thanks.

-- 
tejun