Re: [PATCHSET v2 wq/6.10] workqueue: Implement disable/enable_work()

[Lai Jiangshan]

Hello

On Thu, Feb 22, 2024 at 11:33 AM Lai Jiangshan <jiangshanlai@xxxxxxxxx> wrote:
>
> Hello, Tejun
>
> +cc tglx
>
> On Thu, Feb 22, 2024 at 1:43 AM Tejun Heo <tj@xxxxxxxxxx> wrote:
>
> > 4cb1ef64609f ("workqueue: Implement BH workqueues to eventually replace
> > tasklets") implemented workqueues that execute work items in the BH context
> > 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 execution
> > 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 is
> > something which some users asked for in the past, and, while not essential
> > as the caller can and often has to shutdown the queuer anyway, it's a nice
> > 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 better.
>
> Moreover, IMHO the unsynchronized variants of tasklet/work disabling functions
> 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 patches.
>

I think it would be better if the tasklet developers were CCed in this patchset.

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 item
> automatically according to whether the work item was queued while disabled.
> While the caller can track this separately, unconditionally scheduling the
> 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