Sender: Tejun Heo <htejun@gmail.com>
Date:   Thu, 2 Feb 2023 13:45:05 -1000
From:   Tejun Heo <tj@kernel.org>
To:     Petr Mladek <pmladek@suse.com>
Cc:     Lai Jiangshan <jiangshanlai@gmail.com>,
        Michal Koutny <mkoutny@suse.com>, linux-kernel@vger.kernel.org
Subject: Re: [RFC 5/5] workqueue: Print backtraces from CPUs with hung CPU
 bound workqueues
Message-ID: <Y9xLAdc9ODyEO+RQ@slm.duckdns.org>
References: <20230201134543.13687-1-pmladek@suse.com>
 <20230201134543.13687-6-pmladek@suse.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230201134543.13687-6-pmladek@suse.com>
Precedence: bulk

Hello,

I generally really like it.

> +static bool show_pool_suspicious_workers(struct worker_pool *pool, bool shown_title)

Maybe the name can be a bit more specific? show_cpu_pool_hog()?

> +{
> +	bool shown_any = false;
> +	struct worker *worker;
> +	unsigned long flags;
> +	int bkt;
> +
> +	raw_spin_lock_irqsave(&pool->lock, flags);
> +
> +	if (pool->cpu < 0)
> +		goto out;

This can be tested before grabbling the lock.

> +	if (!per_cpu(wq_watchdog_hung_cpu, pool->cpu))
> +		goto out;

Given that the state is per-pool, would it make sense to mark this on the
pool instead?

> +	if (list_empty(&pool->worklist))
> +		goto out;

This condition isn't really necessary, right?

> +	hash_for_each(pool->busy_hash, bkt, worker, hentry) {
> +		if (!task_is_running(worker->task))
> +			continue;
> +
> +		if (!shown_title) {
> +			pr_info("Showing backtraces of running workers in stuck CPU-bound worker pools:\n");
> +			shown_title = true;
> +		}
> +
> +		shown_any = true;
> +		pr_info("pool %d:\n", pool->id);
> +		sched_show_task(worker->task);
> +	}
> +out:
> +	raw_spin_unlock_irqrestore(&pool->lock, flags);
> +	return shown_any;
> +}
> +
> +static void show_suspicious_workers(void)
> +{
> +	struct worker_pool *pool;
> +	bool shown_title = false;
> +	int pi;
> +
> +	rcu_read_lock();
> +
> +	for_each_pool(pool, pi) {
> +		bool shown;
> +
> +		shown = show_pool_suspicious_workers(pool, shown_title);
> +		if (shown)
> +			shown_title = shown;

Maybe, move shown to the outer scope and:

	shown |= show_pool_suspicious_workers(pool, show);

> +	}
> +
> +	rcu_read_unlock();
> +}
>  
>  static void wq_watchdog_reset_touched(void)
>  {

Thanks.

-- 
tejun