[RFC PATCH] workqueue: introduce WORK_STRUCT_TIMER_PENDING_BIT.

[RFC PATCH] workqueue: introduce WORK_STRUCT_TIMER_PENDING_BIT.

[Imran Khan]

Currently WORK_STRUCT_PENDING_BIT can indicate a work_struct that is
already made it to work list of a worker_pool or a work_struct that has
not yet made it there because it comes from a delayed_work whose timer
has not expired yet.

Use WORK_STRUCT_TIMER_PENDING_BIT to distinguish between these two
states of work_struct.

Signed-off-by: Imran Khan <imran.f.khan@oracle.com>
---
I have kept the patch as RFC, because this change is not fixing a bug.
It makes WORK_STRUCT_PENDING_BIT less ambiguous and helps in debugging,
by indicating where one should look for this pending work.

 include/linux/workqueue.h | 1 +
 kernel/workqueue.c        | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index b0dc957c3e560..2d327d9e9b53a 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -27,6 +27,7 @@ enum work_bits {
 	WORK_STRUCT_INACTIVE_BIT,	/* work item is inactive */
 	WORK_STRUCT_PWQ_BIT,		/* data points to pwq */
 	WORK_STRUCT_LINKED_BIT,		/* next work is linked to this one */
+	WORK_STRUCT_TIMER_PENDING_BIT,	/* timer pending for delayed work */
 #ifdef CONFIG_DEBUG_OBJECTS_WORK
 	WORK_STRUCT_STATIC_BIT,		/* static initializer (debugobjects) */
 #endif
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index eb71e3d9d16dd..454382a9a1585 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2481,6 +2481,7 @@ void delayed_work_timer_fn(struct timer_list *t)
 {
 	struct delayed_work *dwork = from_timer(dwork, t, timer);
 
+	clear_bit(WORK_STRUCT_TIMER_PENDING_BIT, work_data_bits(&dwork->work));
 	/* should have been called from irqsafe timer with irq already off */
 	__queue_work(dwork->cpu, dwork->wq, &dwork->work);
 }
@@ -2511,6 +2512,7 @@ static void __queue_delayed_work(int cpu, struct workqueue_struct *wq,
 	dwork->wq = wq;
 	dwork->cpu = cpu;
 	timer->expires = jiffies + delay;
+	set_bit(WORK_STRUCT_TIMER_PENDING_BIT, work_data_bits(work));
 
 	if (housekeeping_enabled(HK_TYPE_TIMER)) {
 		/* If the current cpu is a housekeeping cpu, use it. */

base-commit: 8155b4ef3466f0e289e8fcc9e6e62f3f4dceeac2
-- 
2.34.1

Re: [RFC PATCH] workqueue: introduce WORK_STRUCT_TIMER_PENDING_BIT.

[Tejun Heo]

Hello,

On Mon, Dec 30, 2024 at 05:45:00PM +1100, Imran Khan wrote:
> Currently WORK_STRUCT_PENDING_BIT can indicate a work_struct that is
> already made it to work list of a worker_pool or a work_struct that has
> not yet made it there because it comes from a delayed_work whose timer
> has not expired yet.
> 
> Use WORK_STRUCT_TIMER_PENDING_BIT to distinguish between these two
> states of work_struct.
> 
> Signed-off-by: Imran Khan <imran.f.khan@oracle.com>
> ---
> I have kept the patch as RFC, because this change is not fixing a bug.
> It makes WORK_STRUCT_PENDING_BIT less ambiguous and helps in debugging,
> by indicating where one should look for this pending work.

But can't you already tell this by looking at the work struct? I'm not sure
about adding more atomic ops for redundant information.

Thanks.

-- 
tejun

Re: [RFC PATCH] workqueue: introduce WORK_STRUCT_TIMER_PENDING_BIT.

[imran.f.khan]

Hello Tejun,
Thanks a lot for looking into this.
On 4/1/2025 9:18 am, Tejun Heo wrote:
> Hello,
> 
> On Mon, Dec 30, 2024 at 05:45:00PM +1100, Imran Khan wrote:
>> Currently WORK_STRUCT_PENDING_BIT can indicate a work_struct that is
>> already made it to work list of a worker_pool or a work_struct that has
>> not yet made it there because it comes from a delayed_work whose timer
>> has not expired yet.
>>
>> Use WORK_STRUCT_TIMER_PENDING_BIT to distinguish between these two
>> states of work_struct.
>>
>> Signed-off-by: Imran Khan <imran.f.khan@oracle.com>
>> ---
>> I have kept the patch as RFC, because this change is not fixing a bug.
>> It makes WORK_STRUCT_PENDING_BIT less ambiguous and helps in debugging,
>> by indicating where one should look for this pending work.
> 
> But can't you already tell this by looking at the work struct? 

Yes this can also be inferred by looking at pwq information set by insert_work.
If there is some other way, could you please let me know.

But this path (__queue_work->insert_work)is taken for normal works as well and
I thought that having a dedicated bit in the work_struct to indicate that this
work comes from a dwork, whose timer has not yet expired, would make debugging
easier.
Further in cases where dwork is part of another object and there is a possibilty
that some data corruption issue might be messing up the dwork or dwork.work,
we would have an extra piece of information to check against.

I'm not sure
> about adding more atomic ops for redundant information.
> 

I understand your concern if this extra piece of information is not
worth the overhead.

Thanks,
Imran

> Thanks.
>