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

[Petr Mladek <pmladek@suse.com>]

On Wed 2026-03-25 14:19:27, Petr Mladek wrote:
> 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.

Grrr, I have sent a wrong version of the patch. I forgot to amend
last changes and refresh it. It is actually quite different.

Anyway, this is what I wanted to send:

From e7ab783f9a1dc5c0e8bd3b1a4bf16dd8856a59e5 Mon Sep 17 00:00:00 2001
From: Petr Mladek <pmladek@suse.com>
Date: Wed, 25 Mar 2026 13:34:18 +0100
Subject: [PATCH v2] 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 | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index ff97b705f25e..eda756556341 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -7702,13 +7702,14 @@ 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.
+		 * Do a lockless check first to do not disturb the system.
+		 *
+		 * Prevent false positives by double checking the timestamp
+		 * under pool->lock. The lock makes sure that the check reads
+		 * an updated pool->last_progress_ts when this CPU saw
+		 * an already updated pool->worklist above. It seems better
+		 * than adding another barrier into __queue_work() which
+		 * is a hotter path.
 		 */
 		if (time_after(now, ts + thresh)) {
 			scoped_guard(raw_spinlock_irqsave, &pool->lock) {
-- 
2.53.0