Re: [PATCH v2] workqueue: Fix false positive stall reports

[Petr Mladek <pmladek@suse.com>]

On Tue 2026-03-24 11:22:11, Song Liu wrote:
> On Tue, Mar 24, 2026 at 3:01 AM Petr Mladek <pmladek@suse.com> wrote:
> [...]
> > This explains why taking the lock is needed.
> >
> > > > > + Since
> > > > > +              * __queue_work() is a much hotter path than the timer
> > > > > +              * function, we handle false positive here by reading
> > > > > +              * last_progress_ts again with pool->lock held.
> >
> > But this is confusing. It says that __queue_work() is a much hotter path
> > but it already takes pool->lock. The sentence makes a feeling that
> > the watchdog patch is less hot. Then it is weird why the watchdog
> > path ignores the lock by default.
> 
> This comment primarily concerns the cost of making the read lockless.
> To do that, we need to add a memory barrier in __queue_work(),
> which will slow down the hot path. __queue_work() does take
> pool->lock, but in most cases,  __queue_work() only takes the lock
> in local pool, which is faster than taking all the pool->locks from the
> watchdog timer.
> 
> That said, I am open to other suggestions to get rid of this false
> positive.

See below a patch which tries to improve the comment.

Honestly, I am not sure if it is better than the original. It probably
better answers the questions which I had. But it might open another
questions. I feel that I have lost a detached view because I already
know the details. Also it is hard to mention everything and keep it
"short".

Feel free to take it, ignore it, or squash it into the original
commit.

From c65eeaac600f069c7f3398c87186f73f9c437735 Mon Sep 17 00:00:00 2001
From: Petr Mladek <pmladek@suse.com>
Date: Wed, 25 Mar 2026 13:34:18 +0100
Subject: [PATCH] workqueue: Better describe stall check

Try to be more explicit why the workqueue watchdog does not take
pool->lock by default. Spin locks are full memory barriers which
delay anything. Obviously, they would primary delay operations
on the related worker pools.

Explain why it is enough to prevent the false positive by re-checking
the timestamp under the pool->lock.

Finally, make it clear what would be the alternative solution in
__queue_work() which is a hotter path.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 kernel/workqueue.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index ff97b705f25e..e3e97d59d2f4 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -7702,13 +7702,13 @@ static void wq_watchdog_timer_fn(struct timer_list *unused)
 		/*
 		 * Did we stall?
 		 *
-		 * Do a lockless check first. On weakly ordered
-		 * architectures, the lockless check can observe a
-		 * reordering between worklist insert_work() and
-		 * last_progress_ts update from __queue_work(). Since
-		 * __queue_work() is a much hotter path than the timer
-		 * function, we handle false positive here by reading
-		 * last_progress_ts again with pool->lock held.
+		 * Try to block or slow down queue_work() as less as possible
+		 * because it might be used in hot paths. Just prevent false
+		 * positives.
+		 *
+		 * Do a lockless check first. The spin_lock() makes sure
+		 * that the re-check reads an updated pool->last_progress_ts
+		 * when this CPU saw an already updated pool->worklist.
 		 */
 		if (time_after(now, ts + thresh)) {
 			scoped_guard(raw_spinlock_irqsave, &pool->lock) {
-- 
2.53.0