Re: [PATCH v5 0/2] printk: Console owner and waiter logic cleanup

[Steven Rostedt]

On Tue, 16 Jan 2018 13:47:16 +0900
Sergey Senozhatsky <sergey.senozhatsky.work@xxxxxxxxx> wrote:

> From: Sergey Senozhatsky <sergey.senozhatsky@xxxxxxxxx>
> Subject: [PATCH] printk: never set console_may_schedule in console_trylock()
> 
> This patch, basically, reverts commit 6b97a20d3a79 ("printk:
> set may_schedule for some of console_trylock() callers").
> That commit was a mistake, it introduced a big dependency
> on the scheduler, by enabling preemption under console_sem
> in printk()->console_unlock() path, which is rather too
> critical. The patch did not significantly reduce the
> possibilities of printk() lockups, but made it possible to
> stall printk(), as has been reported by Tetsuo Handa [1].
> 
> Another issues is that preemption under console_sem also
> messes up with Steven Rostedt's hand off scheme, by making
> it possible to sleep with console_sem both in console_unlock()
> and in vprintk_emit(), after acquiring the console_sem
> ownership (anywhere between printk_safe_exit_irqrestore() in
> console_trylock_spinning() and printk_safe_enter_irqsave()
> in console_unlock()). This makes hand off less likely and,
> at the same time, may result in a significant amount of
> pending logbuf messages. Preempted console_sem owner makes
> it impossible for other CPUs to emit logbuf messages, but
> does not make it impossible for other CPUs to append new
> messages to the logbuf.
> 
> Reinstate the old behavior and make printk() non-preemptible.
> Should any printk() lockup reports arrive they must be handled
> in a different way.
> 
> [1] https://marc.info/?l=linux-mm&m=145692016122716

Especially since Konstantin is working on pulling in all LKML archives,
the above should be denoted as:

 Link: http://lkml.kernel.org/r/201603022101.CAH73907.OVOOMFHFFtQJSL%20()%20I-love%20!%20SAKURA%20!%20ne%20!%20jp

Although the above is for linux-mm and not LKML (it still works), I
should ask Konstantin if he will be pulling in any of the other
archives. Perhaps have both? (in case marc.info goes away).

> Fixes: 6b97a20d3a79 ("printk: set may_schedule for some of console_trylock() callers")

Should we Cc stable@xxxxxxxxxxxxxxx?

> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@xxxxxxxxx>
> Reported-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
> ---
>  kernel/printk/printk.c | 22 ++++++++--------------
>  1 file changed, 8 insertions(+), 14 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index ffe05024c622..9cb943c90d98 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1895,6 +1895,12 @@ asmlinkage int vprintk_emit(int facility, int level,
>  
>  	/* If called from the scheduler, we can not call up(). */
>  	if (!in_sched) {
> +		/*
> +		 * Disable preemption to avoid being preempted while holding
> +		 * console_sem which would prevent anyone from printing to
> +		 * console
> +		 */
> +		preempt_disable();
>  		/*
>  		 * Try to acquire and then immediately release the console
>  		 * semaphore.  The release will print out buffers and wake up
> @@ -1902,6 +1908,7 @@ asmlinkage int vprintk_emit(int facility, int level,
>  		 */
>  		if (console_trylock_spinning())
>  			console_unlock();
> +		preempt_enable();
>  	}
>  
>  	return printed_len;
> @@ -2229,20 +2236,7 @@ int console_trylock(void)
>  		return 0;
>  	}
>  	console_locked = 1;
> -	/*
> -	 * When PREEMPT_COUNT disabled we can't reliably detect if it's
> -	 * safe to schedule (e.g. calling printk while holding a spin_lock),
> -	 * because preempt_disable()/preempt_enable() are just barriers there
> -	 * and preempt_count() is always 0.
> -	 *
> -	 * RCU read sections have a separate preemption counter when
> -	 * PREEMPT_RCU enabled thus we must take extra care and check
> -	 * rcu_preempt_depth(), otherwise RCU read sections modify
> -	 * preempt_count().
> -	 */
> -	console_may_schedule = !oops_in_progress &&
> -			preemptible() &&
> -			!rcu_preempt_depth();
> +	console_may_schedule = 0;
>  	return 1;
>  }
>  EXPORT_SYMBOL(console_trylock);

Reviewed-by: Steven Rostedt (VMware) <rostedt@xxxxxxxxxxx>

Thanks Sergey!

-- Steve