Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
MIME-Version: 1.0
References: <20231011024842.3348767-1-longman@redhat.com>
In-Reply-To: <20231011024842.3348767-1-longman@redhat.com>
From:   Lai Jiangshan <jiangshanlai@gmail.com>
Date:   Wed, 11 Oct 2023 20:16:56 +0800
Message-ID: <CAJhGHyDc540v+45xNx4mtWF9O=3soxEh_p7eyGQGP48n+_o2Vg@mail.gmail.com>
Subject: Re: [PATCH] workqueue: Override implicit ordered attribute in workqueue_apply_unbound_cpumask()
To:     Waiman Long <longman@redhat.com>
Cc:     Tejun Heo <tj@kernel.org>, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Oct 11, 2023 at 10:49=E2=80=AFAM Waiman Long <longman@redhat.com> w=
rote:
>
> Commit 5c0338c68706 ("workqueue: restore WQ_UNBOUND/max_active=3D=3D1
> 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=3D=3D1 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 c=
pumask_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