Re: [RFC PATCH v1 20/25] serial: 8250: implement write_atomic

[John Ogness]

On 2019-02-27, Petr Mladek <pmladek@xxxxxxxx> wrote:
>> Implement a non-sleeping NMI-safe write_atomic console function in
>> order to support emergency printk messages.
>
> It uses console_atomic_lock() added in 18th patch. That one uses
> prb_lock() added by 2nd patch.
>
> Now, prb_lock() allows recursion on the same CPU. But it still needs
> to wait until it is released on another CPU.
>
> [...]
>
> OK, it would be safe when prb_lock() is the only lock taken
> in the NMI handler.

Which is the case. As I wrote to you already [0], NMI contexts are
_never_ allowed to do things that rely on waiting forever for other
CPUs. I could not find any instances where that is the
case. nmi_cpu_backtrace() used to do this, but it does not anymore.

> But printk() should not make such limitation
> to the rest of the system.

That is something we have to decide. It is the one factor that makes
prb_lock() feel a hell of a lot like BKL.

> Not to say, that we would most
> likely need to add a lock back into nmi_cpu_backtrace()
> to keep the output sane.

No. That is why CPU-IDs were added to the output. It is quite sane and
easy to read.

> Peter Zijlstra several times talked about fully lockless
> consoles. He is using the early console for debugging, see
> the patchset
> https://lkml.kernel.org/r/20170928121823.430053219@xxxxxxxxxxxxx

That is an interesting thread to quote. In that thread Peter actually
wrote the exact implementation of prb_lock() as the method to
synchronize access to the serial console.

> I am not sure if it is always possible. I personally see
> the following way:
>
>    1. Make the printk ring buffer fully lockless. Then we reduce
>       the problem only to console locking. And we could
>       have a per-console-driver lock (no the big lock like
>       prb_lock()).

A fully lockless ring buffer is an option. But as you said, it only
reduces the window, which is why I decided it is not so important (at
least for now). Creating a per-console-driver lock would probably be a
good idea anyway as long as we can guarantee the ordering (which
shouldn't be a problem as long as emergency console ordering remains
fixed and emergency writers always follow that ordering).

>    2. I am afraid that we need to add some locking between CPUs
>       to avoid mixing characters from directly printed messages.

That is exactly what console_atomic_lock() (actually prb_lock) is!

John Ogness

[0] https://lkml.kernel.org/r/87pnrvs707.fsf@xxxxxxxxxxxxx