Sender: Tejun Heo <htejun@gmail.com>
Date:   Thu, 2 Feb 2023 13:30:39 -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 2/5] workqueue: Warn when a new worker could not be created
Message-ID: <Y9xHnwG39IHN/BBu@slm.duckdns.org>
References: <20230201134543.13687-1-pmladek@suse.com>
 <20230201134543.13687-3-pmladek@suse.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230201134543.13687-3-pmladek@suse.com>
Precedence: bulk

Hello,

On Wed, Feb 01, 2023 at 02:45:40PM +0100, Petr Mladek wrote:
> +static __printf(2, 3) __cold
> +void __print_create_worker_failure(long err, const char *fmt, ...)
> +{
> +	spin_lock_irq(&create_worker_failed_lock);
> +
> +	/*
> +	 * Report potentially repeated failures only once during a stall.
> +	 * Otherwise, it might be noisy. Also slow serial console drivers
> +	 * touch watchdogs so that more frequent messages would prevent
> +	 * reaching the watchdog thresh.
> +	 */
> +	if (!create_worker_failed) {
> +		va_list args;
> +
> +		va_start(args, fmt);
> +		vprintk(fmt, args);
> +		va_end(args);
> +	}
> +
> +	create_worker_failed++;
> +
> +	spin_unlock_irq(&create_worker_failed_lock);
> +}

That's pretty elaborate. Why not just use printk_ratelimited()?

> @@ -1931,12 +1992,16 @@ static struct worker *create_worker(struct worker_pool *pool)
>  
>  	/* ID is needed to determine kthread name */
>  	id = ida_alloc(&pool->worker_ida, GFP_KERNEL);
> -	if (id < 0)
> +	if (id < 0) {
> +		print_create_worker_failure("workqueue: Failed to allocate a pool ID", id);
                                                                             ^
									  worker ID?

> @@ -5880,8 +5950,10 @@ static void wq_watchdog_timer_fn(struct timer_list *unused)
>  
>  	rcu_read_unlock();
>  
> -	if (lockup_detected)
> +	if (lockup_detected) {
> +		print_create_worker_failed_num();
>  		show_all_workqueues();
> +	}

I'd just use printk_ratelimited() and leave it at that.

Thanks.

-- 
tejun