Received-SPF: pass (google.com: domain of linux-kernel+bounces-73729-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240216180559.208276-1-tj@kernel.org> <ZdUK1jhMUSyC7yUj@slm.duckdns.org>
In-Reply-To: <ZdUK1jhMUSyC7yUj@slm.duckdns.org>
From: Allen <allen.lkml@gmail.com>
Date: Tue, 20 Feb 2024 13:38:18 -0800
Message-ID: <CAOMdWSKrcRFUsFc3a0+2Js5hOA92xeQSkS06oL9VgSyZpDrO8g@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"

Tejun

> >  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.
>
> Lai, would you mind reviewing patches 0001-00010? If you're okay, I'll apply
> them to wq/for-6.9 and then post the v2 patchset with the rest of the
> patches updated to reflect your reviews.
>

 In addition to the above features, would it make sense to introduce a "count"
variable in struct work_struct?(I am not an expert with workqueue
internals). Or perhaps
we could use the existing "data" variable.

struct tasklet_struct has a variable count, which is referenced in
several drivers, Ex:

from drivers/gpu/drm/i915/i915_tasklet.h
 28 static inline bool __tasklet_is_enabled(const struct tasklet_struct *t)
 29 {
 30         return !atomic_read(&t->count);
 31 }

Also, there are several helper functions that use the "state"
variable, tasklet_lock() which calls tasklet_trylock().
I was thinking of adding/introducing these helper functions. Do let me
know if it makes sense.

Thanks.