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

[Lai Jiangshan]

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.

Thanks
Lai