Re: [PATCH RFC 3/3] workqueue: dump the last woken worker for stalled pools

[Petr Mladek <pmladek@suse.com>]

On Tue 2026-06-16 09:44:41, Breno Leitao wrote:
> To identify the task most likely responsible for a stall, add
> last_woken_worker (L: pool->lock) to worker_pool and record it in
> kick_pool() just before wake_up_process().  This captures the idle
> worker that was kicked to take over when the last running worker went to
> sleep; if the pool is now stuck with no running worker, that task is the
> prime suspect and its backtrace is dumped by show_pool_no_running_worker().
> 
> Using struct worker * rather than struct task_struct * avoids any
> lifetime concern: workers are only destroyed via set_worker_dying()
> which requires pool->lock, and set_worker_dying() clears
> last_woken_worker when the dying worker matches.
> show_cpu_pool_busy_workers() holds pool->lock while calling
> sched_show_task(), so last_woken_worker is either NULL or points to a
> live worker with a valid task.  More precisely, set_worker_dying() clears
> last_woken_worker before setting WORKER_DIE, so a non-NULL
> last_woken_worker means the kthread has not yet exited and worker->task
> is still alive.
> 
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -226,6 +226,7 @@ struct worker_pool {
>  						/* L: hash of busy workers */
>  
>  	struct worker		*manager;	/* L: purely informational */
> +	struct worker		*last_woken_worker; /* L: last worker woken by kick_pool() */

I thought more about it. The "last_woken_worker" and "manager" are
related and they might eventually duplicate the information.

If I get it correctly then kick_pool() wakes a worker when needed.
The last worker becomes a "manager" and tries to create a new
worker.

IMHO, in most situations "manager" will have the same value as
"last_woken_worker". But it is not guaranteed because "pool->lock"
is not taken all the time.

There are two questions:

1. Do we need both values?

IMHO, we do:

  + "last_woken_worker" is the last woken worker. It is supposed to
    guarantee the forward progress. The backtrace is interesting
    because it can never get scheduled.

  + "manager" is the last "idle" worker which is actively trying to
    create a new worker. It is supposed to guarantee forward progress
    too. IMHO, it usually will be the "last_woken_worker" but it is
    not guaranteed as mentioned above.

2. Should we print backtrace of both?

Probably not both at the same time:

  + We should print "manager" when it is set. It is set when a new
    worker has to be created. And the "manager" is responsible for
    the forward progress, definitely.

   + We should print "last_woken_worker" when "manager" is not set.
     It is the only clue. And it likely got stuck for some reasons.

   + IMHO, "last_woken_worker" need not be printed when "manager"
     is set even when it is a different worker. The "manager" is
     the really responsible worker. And "last_woken_worker" likely
     just started processing work items because it somehow raced with
     the manager.

Does this make sense, please?

We could also add the "manager" printing in a separate patch later.
This patch is a good step forward. Feel free to use:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr