Received-SPF: pass (google.com: domain of linux-kernel+bounces-75860-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240221174333.700197-1-tj@kernel.org>
In-Reply-To: <20240221174333.700197-1-tj@kernel.org>
From: Lai Jiangshan <jiangshanlai@gmail.com>
Date: Thu, 22 Feb 2024 11:33:24 +0800
Message-ID: <CAJhGHyCJS7Pb_5dwTQtcZ25yOVzxFULJEYT4o3id_3xdj32EYA@mail.gmail.com>
Subject: Re: [PATCHSET v2 wq/6.10] workqueue: Implement disable/enable_work()
To: Tejun Heo <tj@kernel.org>
Cc: torvalds@linux-foundation.org, linux-kernel@vger.kernel.org, 
	allen.lkml@gmail.com, kernel-team@meta.com, 
	Thomas Gleixner <tglx@linutronix.de>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hello, Tejun

+cc tglx

On Thu, Feb 22, 2024 at 1:43=E2=80=AFAM Tejun Heo <tj@kernel.org> wrote:

> 4cb1ef64609f ("workqueue: Implement BH workqueues to eventually replace
> tasklets") implemented workqueues that execute work items in the BH conte=
xt
> with the goal of eventually replacing tasklet.
>
> While the existing workqueue API covers the basic queueing and canceling
> operations, tasklet also has tasklet_disable*() which blocks the executio=
n
> of the tasklet until it's re-enabled with tasklet_enable(). The interface=
 if
> fairly widely used and workqueue currently doesn't have a counterpart.
>
> This patchset implements disable/enable_work() and the delayed_work
> counterparts to address the gap. The ability to block future executions i=
s
> something which some users asked for in the past, and, while not essentia=
l
> as the caller can and often has to shutdown the queuer anyway, it's a nic=
e
> convenience to have. Also, timer grew a similar feature recently with
> timer_shutdown().
>

From the last patch:
> - tasklet_disable_nosync()      -> disable_work()
> - tasklet_disable[_in_atomic]() -> disable_work_sync()

I think it is a misuse-prone conversion.

A developer familiar with tasklet_disable() might happily use disable_work(=
)
and, to her/his surprise, leave the running works unsynced.

And tasklet_disable_nosync() is used at only 3 places while tasklet_disable=
()
is used a lot.  I think the shorter name for the most common cases is bette=
r.

Moreover, IMHO the unsynchronized variants of tasklet/work disabling functi=
ons
never have a strong scenario. I think it should be discouraged.

Although it will be inconsistent with the name of cancel_work[_sync](),
I still suggest:
tasklet_disable_nosync() -> disable_work_nosync()
tasklet_disable() -> disable_work().

Even cancel_work_sync() is used a lot more than cancel_work(), so I
also suggest rename cancel_work() to cancel_work_nosync() and leave
cancel_work_sync() unchanged (at least for a while).

[changed topic:]

I feel uncomfortable with tasklet_disable_in_atomic() implicitly
being implemented in disable_work_sync().

I think it is a revert of the commit ca5f62511895 ("tasklets: Provide
tasklet_disable_in_atomic()") in which tglx discouraged the usage of
tasklet_disable_in_atomic() and marked it "error prone".

And even tasklet_disable_in_atomic() is implemented in disable_work_sync(),
I prefer to sleepable-wait than spinning-wait when disable_work_sync() is
called in a sleepable context for BH work item.

All the above is just my feeling, not reasoning, nor rejection of the patch=
es.

Thanks
Lai