Received-SPF: pass (google.com: domain of linux-kernel+bounces-73450-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240216180559.208276-1-tj@kernel.org>
In-Reply-To: <20240216180559.208276-1-tj@kernel.org>
From: Allen <allen.lkml@gmail.com>
Date: Tue, 20 Feb 2024 09:33:08 -0800
Message-ID: <CAOMdWS+pkf_HeZ2+xcsZvFb0Jwrdd2NyfMNFOje2AcHySei0yA@mail.gmail.com>
Subject: Re: [PATCHSET wq/for-6.9,6.10] workqueue: Implement disable/enable_work()
To: Tejun Heo <tj@kernel.org>
Cc: jiangshanlai@gmail.com, torvalds@linux-foundation.org, 
	linux-kernel@vger.kernel.org, kernel-team@meta.com
Content-Type: text/plain; charset="UTF-8"

>
> 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().
>
> - tasklet_disable() keeps disable depth so that a tasklet can be disabled
>   and re-enabled by multiple parties at the same time. Workqueue is updated
>   to carry 16bit disable count in work->data while the work item is not
>   queued. When non-zero, attempts to queue the work item fail.
>
> - The cancel_work_sync() path used WORK_OFFQ_CANCELING to synchronize
>   multiple cancel_sync attempts. This added a completely separate wait
>   mechanism which wasn't very congruent with the rest of workqueue. This was
>   because the canceling state was carried in a way which couldn't
>   accommodate multiple concurrent uses. This mechanism is replaced by
>   disable - cancel_sync now simply disables the work item, flushes and
>   re-enables it.
>
> - There is a wart in how tasklet_disable/enable() works. When a tasklet is
>   disabled, if the tasklet is queued, it keeps the softirq constantly raised
>   until the tasklet is re-enabled and executed. This makes disabling
>   unnecessarily expensive and brittle. The only busy looping workqueue's
>   implementation does is on the party that's trying to cancel_sync or
>   disable_sync to wait for the completion of the currently executing
>   instance, which should be safe as long as it's from process and BH
>   contexts.
>
> - A disabled tasklet remembers whether it was queued while disabled and
>   starts executing when re-enabled. It turns out doing this with work items
>   is challenging as there are a lot more to remember and the synchronization
>   becomes more complicated too. Looking at the use cases and given the
>   continuity from how cancel_work_sync() works, it seems better to just
>   ignore queueings which happen while a work item is disabled and require
>   the users to explicitly queue the work item after re-enabling as
>   necessary. Most users should be able to re-enqueue unconditionally after
>   enabling.
>
> This patchset is on top of wq/for-6.9 and contains the following 17 patches:
>
>  0001-workqueue-Cosmetic-changes.patch
>  0002-workqueue-Use-rcu_read_lock_any_held-instead-of-rcu_.patch
>  0003-workqueue-Rename-__cancel_work_timer-to-__cancel_tim.patch
>  0004-workqueue-Reorganize-flush-and-cancel-_sync-function.patch
>  0005-workqueue-Use-variable-name-irq_flags-for-saving-loc.patch
>  0006-workqueue-Introduce-work_cancel_flags.patch
>  0007-workqueue-Clean-up-enum-work_bits-and-related-consta.patch
>  0008-workqueue-Factor-out-work_grab_pending-from-__cancel.patch
>  0009-workqueue-Remove-clear_work_data.patch
>  0010-workqueue-Make-flags-handling-consistent-across-set_.patch
>  0011-workqueue-Preserve-OFFQ-bits-in-cancel-_sync-paths.patch
>  0012-workqueue-Implement-disable-enable-for-delayed-work-.patch
>  0013-workqueue-Remove-WORK_OFFQ_CANCELING.patch
>  0014-workqueue-Remember-whether-a-work-item-was-on-a-BH-w.patch
>  0015-workqueue-Update-how-start_flush_work-is-called.patch
>  0016-workqueue-Allow-cancel_work_sync-and-disable_work-fr.patch
>  0017-r8152-Convert-from-tasklet-to-BH-workqueue.patch
>
> 0001-0010 are cleanup and prep patches with the only functional change being
> the use of rcu_read_lock_any_held() instead of rcu_read_lock() in 0002. I'll
> apply them to wq/for-6.9 unless there are objections. I thought about making
> these a separate patch series but the cleanups make more sense as a part of
> this series.
>
> For the rest of the series, given how many invasive workqueue changes are
> already queued for v6.9 and the subtle nature of these patches, I think it'd
> be best to defer them to the one after that so that we can use v6.9 as an
> intermediate verification point.
>
> 0011-0012 implement disable_work() and enable_work(). At this stage, all
> disable[_sync]_work and enable_work operations might_sleep(). disable_work()
> and enable_work() due to CANCELING synchronization described above.
> disable_work_sync() also needs to wait for the in-flight work item to finish
> which requires blocking.
>
> 0013 replaces CANCELING with internal use of disble/enable. This removes one
> ugliness from workqueue code and allows disable_work() and enable_work() to
> be used from atomic contexts.
>
> 0014-0016 implement busy-wait for BH work items when they're being canceled
> thus allowing cancel_work_sync() and disable_work_sync() to be called from
> atomic contexts for them. This makes workqueue interface a superset of
> tasklet and also makes BH workqueues easier to live with.
>
> 0017 converts drivers/net/r8152.c from tasklet to BH workqueue as a
> demonstration. It seems to work fine.
>
> The patchset is on top of wq/for-6.9 fd0a68a2337b ("workqueue, irq_work:
> Build fix for !CONFIG_IRQ_WORK") and also available in the following git
> branch:
>
>  git://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git disable_work-v1
>

 Thank you. Rebasing all the conversion patches on top of these changes.
I should have the entire set going out in a day or two for review.

- Allen
> diffstat follows. Thanks.
>
>  drivers/net/usb/r8152.c   |   44 ++--
>  include/linux/workqueue.h |   69 ++++---
>  kernel/workqueue.c        |  623 ++++++++++++++++++++++++++++++++++++++++----------------------------
>  3 files changed, 441 insertions(+), 295 deletions(-)
>
> --
> tejun


-- 
       - Allen