Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 6 Oct 2021 11:19:01 +0200
From:   Petr Mladek <pmladek@suse.com>
To:     Johan Hovold <johan@kernel.org>
Cc:     Tejun Heo <tj@kernel.org>, Lai Jiangshan <jiangshanlai@gmail.com>,
        Sergey Senozhatsky <senozhatsky@chromium.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        John Ogness <john.ogness@linutronix.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Fabio Estevam <festevam@denx.de>, linux-serial@vger.kernel.org,
        linux-kernel@vger.kernel.org, stable@vger.kernel.org
Subject: Re: [PATCH] workqueue: fix state-dump console deadlock
Message-ID: <YV1qBZiXx/IADcb6@alley>
References: <YV1Z8JslFiBSFGJF@hovoldconsulting.com>
 <20211006081115.20451-1-johan@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211006081115.20451-1-johan@kernel.org>
Precedence: bulk

On Wed 2021-10-06 10:11:15, Johan Hovold wrote:
> Console drivers often queue work while holding locks also taken in their
> console write paths, something which can lead to deadlocks on SMP when
> dumping workqueue state (e.g. sysrq-t or on suspend failures).
> 
> For serial console drivers this could look like:
> 
> 	CPU0				CPU1
> 	----				----
> 
> 	show_workqueue_state();
> 	  lock(&pool->lock);		<IRQ>
> 	  				  lock(&port->lock);
> 					  schedule_work();
> 					    lock(&pool->lock);
> 	  printk();
> 	    lock(console_owner);
> 	    lock(&port->lock);
> 
> where workqueues are, for example, used to push data to the line
> discipline, process break signals and handle modem-status changes. Line
> disciplines and serdev drivers can also queue work on write-wakeup
> notifications, etc.
> 
> Reworking every console driver to avoid queuing work while holding locks
> also taken in their write paths would complicate drivers and is neither
> desirable or feasible.
> 
> Instead use the deferred-printk mechanism to avoid printing while
> holding pool locks when dumping workqueue state.
> 
> Note that there are a few WARN_ON() assertions in the workqueue code
> which could potentially also trigger a deadlock. Hopefully the ongoing
> printk rework will provide a general solution for this eventually.
> 
> This was originally reported after a lockdep splat when executing
> sysrq-t with the imx serial driver.
> 
> Fixes: 3494fc30846d ("workqueue: dump workqueues on sysrq-t")
> Cc: stable@vger.kernel.org	# 4.0
> Reported-by: Fabio Estevam <festevam@denx.de>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  kernel/workqueue.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 33a6b4a2443d..fded64b48b96 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -4830,8 +4830,16 @@ void show_workqueue_state(void)
>  
>  		for_each_pwq(pwq, wq) {
>  			raw_spin_lock_irqsave(&pwq->pool->lock, flags);
> -			if (pwq->nr_active || !list_empty(&pwq->inactive_works))
> +			if (pwq->nr_active || !list_empty(&pwq->inactive_works)) {
> +				/*
> +				 * Defer printing to avoid deadlocks in console
> +				 * drivers that queue work while holding locks
> +				 * also taken in their write paths.
> +				 */
> +				printk_deferred_enter();
>  				show_pwq(pwq);
> +				printk_deferred_exit();
> +			}
>  			raw_spin_unlock_irqrestore(&pwq->pool->lock, flags);
>  			/*
>  			 * We could be printing a lot from atomic context, e.g.

This handles only one printk() caller. But there are many more callers
under pool->lock, for example in the next for-cycle in this function:

	for_each_pool(pool, pi) {
		raw_spin_lock_irqsave(&pool->lock, flags);
[...]
		pr_info("pool %d:", pool->id);
		pr_cont_pool_info(pool);
		pr_cont(" hung=%us workers=%d",

And this is the problem with printk_deferred() and printk_deferred_enter().
It is a "catch a mole" approach. It might end up with switching half
of the kernel into printk_deferred().

John Ogness is working on a generic solution where any printk() will
be deferred out of box. consoles will be called from a dedicated
kthreads.

John has already worked on reworking printk() two years or so. It gets
slowly because we need to be careful. Also we started with
implementing lockless ringbuffer which was a big challenge. Anyway, there
is a stable progress. The lockless ringbuffer is done. And the
kthreads are the very next step.

printk_deferred() is currently used only in the scheduler code where
the deadlocks really happened in the past. printk_deferred_enter()
is used only in printk() because it would be otherwise hard to debug
and lockdep would always report problems there.

From this perspective, I suggest to ignore this possible deadlock if
they do not happen in the real life.

If you really want to avoid the lockdep report. Alternative and
probably easier workaround is to temporary disable lockdep around
queuing the work in the console code. I do not see any reason
why workqueue code would call back to console code directly.
So the only source of a possible deadlock is the printk() path.
But I think that it is not worth it. It is better to concentrate
on the printk() rework.

Best Regards,
Petr