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

[Song Liu]

On Wed, Mar 25, 2026 at 7:12 AM Petr Mladek <pmladek@xxxxxxxx> wrote:
[...]
> Anyway, this is what I wanted to send:
>
> From e7ab783f9a1dc5c0e8bd3b1a4bf16dd8856a59e5 Mon Sep 17 00:00:00 2001
> From: Petr Mladek <pmladek@xxxxxxxx>
> 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@xxxxxxxx>
> ---
>  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.
>                  */

Looks good to me. Thanks for improving this comment!

Tejun,

If we want this as a separate commit, please add

Acked-by: Song Liu <song@xxxxxxxxxx>

Or we can fold this in with Petr's Signed-off-by and/or
Co-developed-by.

Thanks,
Song