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

Steven Rostedt
2018-01-16T15:45:14Z

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

> From: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> 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@vger.kernel.org?

> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
> 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@goodmis.org>

Thanks Sergey!

-- Steve