Received-SPF: pass (google.com: domain of linux-kernel+bounces-72464-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240216180559.208276-1-tj@kernel.org> <20240216180559.208276-13-tj@kernel.org>
In-Reply-To: <20240216180559.208276-13-tj@kernel.org>
From: Lai Jiangshan <jiangshanlai@gmail.com>
Date: Tue, 20 Feb 2024 15:22:26 +0800
Message-ID: <CAJhGHyCOgrU9TC9o97LpvAEL2p+Dgt=uN0mo01Gr7HadVSD-Ag@mail.gmail.com>
Subject: Re: [PATCH 12/17] workqueue: Implement disable/enable for (delayed)
 work items
To: Tejun Heo <tj@kernel.org>
Cc: torvalds@linux-foundation.org, linux-kernel@vger.kernel.org, 
	allen.lkml@gmail.com, kernel-team@meta.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hello, Tejun

On Sat, Feb 17, 2024 at 2:06=E2=80=AFAM Tejun Heo <tj@kernel.org> wrote:

> - A work item carries 10bit disable count in work->data while not queued.
>   The access to the count is synchronized by the PENDING bit like all oth=
er
>   parts of work->data.

It is 16bit disable count in the code.


> @@ -2417,7 +2437,8 @@ bool queue_work_on(int cpu, struct workqueue_struct=
 *wq,
>
>         local_irq_save(irq_flags);
>
> -       if (!test_and_set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(wor=
k))) {
> +       if (!test_and_set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(wor=
k)) &&
> +           !clear_pending_if_disabled(work)) {
>                 __queue_work(cpu, wq, work);
>                 ret =3D true;
>         }
> @@ -2577,7 +2598,8 @@ bool queue_delayed_work_on(int cpu, struct workqueu=
e_struct *wq,
>         /* read the comment in __queue_work() */
>         local_irq_save(irq_flags);
>
> -       if (!test_and_set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(wor=
k))) {
> +       if (!test_and_set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(wor=
k)) &&
> +           !clear_pending_if_disabled(work)) {
>                 __queue_delayed_work(cpu, wq, dwork, delay);
>                 ret =3D true;
>         }

It misses the same handling  at queue_work_node() and queue_rcu_work().

But it is bad idea to cancel or disable rcu work since it can be possibly
in the rcu's waiting list.

> @@ -4173,20 +4195,46 @@ bool flush_rcu_work(struct rcu_work *rwork)
>  }
>  EXPORT_SYMBOL(flush_rcu_work);
>

Thanks
Lai