Re: [PATCH v2 4/5] workqueue: Show all busy workers in stall diagnostics

[Jiri Slaby]

On 05. 03. 26, 17:15, Breno Leitao wrote:
> show_cpu_pool_hog() only prints workers whose task is currently running
> on the CPU (task_is_running()).  This misses workers that are busy
> processing a work item but are sleeping or blocked — for example, a
> worker that clears PF_WQ_WORKER and enters wait_event_idle().  Such a
> worker still occupies a pool slot and prevents progress, yet produces
> an empty backtrace section in the watchdog output.
>
> This is happening on real arm64 systems, where
> toggle_allocation_gate() IPIs every single CPU in the machine (which
> lacks NMI), causing workqueue stalls that show empty backtraces because
> toggle_allocation_gate() is sleeping in wait_event_idle().
>
> Remove the task_is_running() filter so every in-flight worker in the
> pool's busy_hash is dumped.  The busy_hash is protected by pool->lock,
> which is already held.
>
> Signed-off-by: Breno Leitao <leitao@xxxxxxxxxx>
> ---
>   kernel/workqueue.c | 28 +++++++++++++---------------
>   1 file changed, 13 insertions(+), 15 deletions(-)
>
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 56d8af13843f8..09b9ad78d566c 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -7583,9 +7583,9 @@ MODULE_PARM_DESC(panic_on_stall_time, "Panic if stall exceeds this many seconds
>   
>   /*
>    * Show workers that might prevent the processing of pending work items.
> - * The only candidates are CPU-bound workers in the running state.
> - * Pending work items should be handled by another idle worker
> - * in all other situations.
> + * A busy worker that is not running on the CPU (e.g. sleeping in
> + * wait_event_idle() with PF_WQ_WORKER cleared) can stall the pool just as
> + * effectively as a CPU-bound one, so dump every in-flight worker.
>    */
>   static void show_cpu_pool_hog(struct worker_pool *pool)
>   {
> @@ -7596,19 +7596,17 @@ static void show_cpu_pool_hog(struct worker_pool *pool)
>   	raw_spin_lock_irqsave(&pool->lock, irq_flags);
>   
>   	hash_for_each(pool->busy_hash, bkt, worker, hentry) {
> -		if (task_is_running(worker->task)) {

We see dumps from non-existent cpus on 7.0 like:
  BUG: workqueue lockup - pool cpus=144 node=0 flags=0x4 nice=0 stuck 
for 168224s!
...
  Showing busy workqueues and worker pools:
  workqueue rcu_gp: flags=0x108
    pwq 578: cpus=144 node=0 flags=0x4 nice=0 active=3 refcnt=4
in:
  https://bugzilla.suse.com/show_bug.cgi?id=1263947
?

Can this (or other patch from the series) cause this? Should there be 
something like cpu_online() instead of task_is_running() somewhere?

> -			/*
> -			 * Defer printing to avoid deadlocks in console
> -			 * drivers that queue work while holding locks
> -			 * also taken in their write paths.
> -			 */
> -			printk_deferred_enter();
> +		/*
> +		 * Defer printing to avoid deadlocks in console
> +		 * drivers that queue work while holding locks
> +		 * also taken in their write paths.
> +		 */
> +		printk_deferred_enter();
>   
> -			pr_info("pool %d:\n", pool->id);
> -			sched_show_task(worker->task);
> +		pr_info("pool %d:\n", pool->id);
> +		sched_show_task(worker->task);
>   
> -			printk_deferred_exit();
> -		}
> +		printk_deferred_exit();
>   	}
>   
>   	raw_spin_unlock_irqrestore(&pool->lock, irq_flags);



thanks,
--
js
suse labs