On Wed 2018-06-06 14:10:29, Sergey Senozhatsky wrote:
> On (06/05/18 14:47), Petr Mladek wrote:
> [..]
> > Grr, the ABBA deadlock is still there. NMIs are not sent to the other
> > CPUs atomically. Even if we detect that logbuf_lock is available
> > in printk_nmi_enter() on some CPUs, it might still get locked on
> > another CPU before the other CPU gets NMI.
>
> Can we do something about "B"? :) I mean - any chance we can rework
> locking in nmi_cpu_backtrace()?
I can't think of any possibility at the moment. Well, it does not mean
that it does not exist.
The irony is that we need the extra lock in nmi_cpu_backtrace() only
because we try to store the messages into the common log buffer.
If we always use the per-CPU buffers in NMI then the lock would
not cause any problems but it also won't be necessary.
> > By other words, any check in printk_safe_enter() is racy and not
> > sufficient
>
> I suppose you meant printk_nmi_enter().
Yup, I am sorry for confusion.
> > => I suggest to revert the commit 719f6a7040f1bdaf96fcc70
> > "printk: Use the main logbuf in NMI when logbuf_lock is available"
> > for-4.18 and stable until we get a better solution.
>
> Just random thoughts.
>
> May be we need to revert it, but let's not "panic". I think [but don't
> insist on it] that the patch in question is *probably* "good enough". It
> addresses a bug report after all.
It was a problem reported by me. I found it when testing other changes.
The patch improved the situation definitely. The question is if it is
enough in practice.
> How often do we have arch_trigger_cpumask_backtrace() on all CPUs these
> days? I tend to think that it used to be much more popular in the past,
> because we had a loops_per_jiffy based spin_lock lockup detection which
> would trigger NMI backtracase, but this functionality has gone, see
> bc88c10d7e6900916f5e1ba3829d66a9de92b633 for details.
AFAIK, we (SUSE) started to work on the printk/NMI deadlocks because
it was too risky to use sysrq-l on systems with many CPUs. It means that
the use case is still there.
Hmm, the original problem was that any interrupted logbuf_lock owner
caused the deadlock. It is _different now_ because the interrupted
logbuf_lock owner must block another CPU via the lock in
nmi_cpu_backtrace().
I actually wonder why this patch actually helped so much in my test
(while true; do echo l >/proc/sysrq-trigger; done). My guess is
that it increased a lot the number of CPUs using PRINTK_NMI_CONTEXT
(per-CPU buffers). Therefore the deadlock was less likely.
On the other hand, the original problem was basically back
without this patch.
> I'm not saying that the race condition that you found is
> unrealistic, I'm just saying that _it seems_ that
> nmi_panic()->printk() on a single CPU is more common
> now, so having that nmi_printk()->printk_deferred() might be quite
> valuable at the end of the day.
I see the point. I need to think about it.
> May be I'm wrong!
>
> > The only safe solution seems to be a trylock() in NMI in
> > vprintk_emit() and fallback to vprintk_safe() when the lock
> > is not taken. I mean something like:
> >
> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > index 247808333ba4..4a5a0bf221b3 100644
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -1845,7 +1845,13 @@ asmlinkage int vprintk_emit(int facility, int level,
> > printk_delay();
> >
> > /* This stops the holder of console_sem just where we want him */
> > - logbuf_lock_irqsave(flags);
> > + printk_safe_enter_irqsave(flags);
> > + if (in_nmi() && !raw_spin_trylock(&logbuf_lock)) {
> > + vprintk_nmi(fmt, args);
> > + printk_safe_exit_irqrestore(flags);
> > + return;
> > + } else
> > + raw_spin_lock(&logbuf_lock);
> > /*
> > * The printf needs to come first; we need the syslog
> > * prefix which might be passed-in as a parameter.
>
> I need some time to think about it.
The bad thing about this solution is that different messages might
be handled different way during a single nmi_cpu_backtrace().
The result might be mixed backtraces in the log.
> > Sigh, this looks like a material for-4.19.
>
> Agreed.
>
> > We might need to revisit if printk_context still makes sense, ...
> What do you mean by this?
My dream was to select the right vprintk implementation according
to printk_context on a single place (vprintk_func()). If this is not
usable for NMI, we might need to think about another solution.
But this is premature. I believe the printk_context will most
likely make still sense.
I need to think more about it.
Best Regards,
Petr
