Received-SPF: pass (google.com: domain of linux-kernel+bounces-76161-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Sender: Tejun Heo <htejun@gmail.com>
Date: Wed, 21 Feb 2024 22:56:52 -1000
From: Tejun Heo <tj@kernel.org>
To: Lai Jiangshan <jiangshanlai@gmail.com>
Cc: torvalds@linux-foundation.org, linux-kernel@vger.kernel.org,
	allen.lkml@gmail.com, kernel-team@meta.com,
	Thomas Gleixner <tglx@linutronix.de>
Subject: Re: [PATCHSET v2 wq/6.10] workqueue: Implement disable/enable_work()
Message-ID: <ZdcMVA_IDptAPsrW@slm.duckdns.org>
References: <20240221174333.700197-1-tj@kernel.org>
 <CAJhGHyCJS7Pb_5dwTQtcZ25yOVzxFULJEYT4o3id_3xdj32EYA@mail.gmail.com>
 <CAJhGHyB-i-1S3ToYYKtri0Zz1ERikctGQHSjr=tbkD-WXHJe8g@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAJhGHyB-i-1S3ToYYKtri0Zz1ERikctGQHSjr=tbkD-WXHJe8g@mail.gmail.com>

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