Received-SPF: pass (google.com: domain of linux-kernel+bounces-75918-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> <CAJhGHyCJS7Pb_5dwTQtcZ25yOVzxFULJEYT4o3id_3xdj32EYA@mail.gmail.com>
In-Reply-To: <CAJhGHyCJS7Pb_5dwTQtcZ25yOVzxFULJEYT4o3id_3xdj32EYA@mail.gmail.com>
From: Lai Jiangshan <jiangshanlai@gmail.com>
Date: Thu, 22 Feb 2024 12:59:17 +0800
Message-ID: <CAJhGHyB-i-1S3ToYYKtri0Zz1ERikctGQHSjr=tbkD-WXHJe8g@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

On Thu, Feb 22, 2024 at 11:33=E2=80=AFAM Lai Jiangshan <jiangshanlai@gmail.=
com> wrote:
>
> 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 con=
text
> > with the goal of eventually replacing tasklet.
> >
> > While the existing workqueue API covers the basic queueing and cancelin=
g
> > operations, tasklet also has tasklet_disable*() which blocks the execut=
ion
> > of the tasklet until it's re-enabled with tasklet_enable(). The interfa=
ce 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=
 is
> > something which some users asked for in the past, and, while not essent=
ial
> > as the caller can and often has to shutdown the queuer anyway, it's a n=
ice
> > 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_wor=
k()
> and, to her/his surprise, leave the running works unsynced.
>
> And tasklet_disable_nosync() is used at only 3 places while tasklet_disab=
le()
> is used a lot.  I think the shorter name for the most common cases is bet=
ter.
>
> Moreover, IMHO the unsynchronized variants of tasklet/work disabling func=
tions
> 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 pat=
ches.
>

I think it would be better if the tasklet developers were CCed in this patc=
hset.

Intended API conversions, along with their rationale and any possible
changes to semantics, can be included in the cover letter too.

> - tasklet_setup/init()          -> INIT_WORK()
> - tasklet_schedule()            -> queue_work(system_bh_wq, ...)
> - tasklet_hi_schedule()         -> queue_work(system_bh_highpri_wq, ...)
> - tasklet_disable_nosync()      -> disable_work()
> - tasklet_disable[_in_atomic]() -> disable_work_sync()
> - tasklet_enable()              -> enable_work() + queue_work()
> - tasklet_kill()                -> cancel_work_sync()
>
> Note that unlike tasklet_enable(), enable_work() doesn't queue the work i=
tem
> automatically according to whether the work item was queued while disable=
d.
> While the caller can track this separately, unconditionally scheduling th=
e
> work item after enable_work() returns %true should work for most users.
>

unconditionally re-scheduling the work item isn't correct to me which might
queue a work unexpectedly.

tasklet_enable() should be changed to return true if and only if when
the disable count reached 0 and the disabling (action and status)
have prevented a queuing request.

To record whether the disabling prevented a queuing request, an extra bit
in the work->data has to be employed for it.

Thanks
Lai