linux-kernel - Re: [PATCHSET v2 wq/6.10] workqueue: Implement disable/enable

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJhGHyCJS7Pb_5dwTQtcZ25yOVzxFULJEYT4o3id_3xdj32EYA@mail.gmail.com>
Date: Thu, 22 Feb 2024 11:33:24 +0800
From: Lai Jiangshan <jiangshanlai@...il.com>
To: Tejun Heo <tj@...nel.org>
Cc: torvalds@...ux-foundation.org, linux-kernel@...r.kernel.org, 
	allen.lkml@...il.com, kernel-team@...a.com, 
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCHSET v2 wq/6.10] workqueue: Implement disable/enable_work()

Hello, Tejun

+cc tglx

On Thu, Feb 22, 2024 at 1:43 AM Tejun Heo <tj@...nel.org> 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