Messages in this thread | | | Date | Wed, 21 Feb 2024 22:56:52 -1000 | From | Tejun Heo <> | Subject | Re: [PATCHSET v2 wq/6.10] workqueue: Implement disable/enable_work() |
| |
Hello,
On Thu, Feb 22, 2024 at 12:59:17PM +0800, Lai Jiangshan wrote: > I think it would be better if the tasklet developers were CCed in this patchset.
Sure.
> 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.
So, I'm not sold on this argument for two reasons.
1. cancel_work() and friends have always indicated whether the work item was queued at the time of cancel and ignored whether someone tried to queue the work item while cancelation was in progress. While it is important to make transition from tasklet reasonably easy, it's a lot more important to keep the interface consistent with the rest of workqueue.
Possible confusions in transitioning from tasklet are temporary. If we introduce workqueue interfaces which aren't congruent with the rest of workqueue, those confusiongs are going to last a lot longer.
2. Work items don't count how many times they're queued. The only thing they guarantee is that they will get executed at least once after the latest queueing, which means that the users usually have to have some type of state tracking - whether that's a queue of operations to perform or some flags tracking what needs to be done.
It's possible to write and there likely are examples of code that depends on work item never executing spuriously. However, tasklets are usually used in the fashion of irq handlers and they almost always have something else tracking what needs to be done. I spot checked a dozen tasklet users and couldn't find anyone who doesn't. While unlikely, if there are usages like that, it isn't all that difficult to update them so that they don't break. Again, possibly a bit of conversion pain but it shouldn't be too much and we'd be better off this way in the long term.
Thanks.
-- tejun
| |