Re: [PATCH] printk: make printk_safe_flush safe in NMI context by skipping flushing

[Petr Mladek]

On Fri 2018-06-01 14:17:54, Hoeun Ryu wrote:
> 
> > -----Original Message-----
> > From: Petr Mladek [mailto:pmladek@xxxxxxxx]
> > Sent: Wednesday, May 30, 2018 5:32 PM
> > To: Sergey Senozhatsky <sergey.senozhatsky.work@xxxxxxxxx>
> > Cc: Hoeun Ryu <hoeun.ryu@xxxxxxxxxxx>; Sergey Senozhatsky
> > <sergey.senozhatsky@xxxxxxxxx>; Steven Rostedt <rostedt@xxxxxxxxxxx>;
> > Hoeun Ryu <hoeun.ryu@xxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx
> > Subject: Re: [PATCH] printk: make printk_safe_flush safe in NMI context by
> > skipping flushing
> > 
> > On Tue 2018-05-29 21:13:15, Sergey Senozhatsky wrote:
> > > On (05/29/18 11:51), Hoeun Ryu wrote:
> > > >  Make printk_safe_flush() safe in NMI context.
> > > > nmi_trigger_cpumask_backtrace() can be called in NMI context. For
> > example the
> > > > function is called in watchdog_overflow_callback() if the flag of
> > hardlockup
> > > > backtrace (sysctl_hardlockup_all_cpu_backtrace) is true and
> > > > watchdog_overflow_callback() function is called in NMI context on some
> > > > architectures.
> > > >  Calling printk_safe_flush() in nmi_trigger_cpumask_backtrace()
> > eventually tries
> > > > to lock logbuf_lock in vprintk_emit() but the logbuf_lock can be
> > already locked in
> > > > preempted contexts (task or irq in this case) or by other CPUs and it
> > may cause
> > 
> > The sentence "logbuf_lock can be already locked in preempted contexts"
> > does not
> > make much sense. It is a spin lock. It means that both interrupts and
> > preemption are disabled.
> > 
> 
> I'd like to say that the preempting context is NMI,
> so the preempted contexts could be task/irq/bh contexts on the same CPU.

Good point!

> > I would change it to something like:
> > 
> > "Calling printk_safe_flush() in nmi_trigger_cpumask_backtrace() eventually
> > tries
> > to lock logbuf_lock in vprintk_emit() that might be already be part
> > of a soft- or hard-lockup on another CPU."
> > 
> 
> It looks more clear.
> But I'd modify "be part of a soft- or hard-lockup on another CPU." to
> "be part of another non-nmi context on the same CPU or a soft- or
> hard-lockup on another CPU."
> 
> How about it?

Looks fine to me.

Best Regards,
Petr