Re: [RFC PATCH v1 20/25] serial: 8250: implement write_atomic
On Fri 2019-03-08 05:05:12, John Ogness wrote: > On 2019-02-27, Petr Mladek wrote: > Implement a non-sleeping NMI-safe write_atomic console function in > order to support emergency printk messages. > >>> > >>> 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. > > > > Who says _never_? I agree that it is not reasonable. But the > > history shows that it happens. > > Right, which is why it would need to become policy. > > The emergency messages (aka write_atomic) introduce a new requirement to > the kernel because this callback must be callable from any context. The > console drivers must have some way of synchronizing. The CPU-reentrant > spin lock is the only solution I am aware of. I am not in position to decide if this requirement is acceptable or not. We would need an opinion from Linus at minimum. But it is not acceptable from my point of view. Note that many spinlocks might be safely used in NMI in principle. You want to introduce a single spinlock just because printk() could be called anywhere. Most other subsystems do not have this problem because they a more self-contained. > If the ringbuffer was fully lockless Exactly, I think that a solution would be fully lockless logbuffer. > we should be able to have per-console CPU-reentrant spin locks > as long as the ordering is preserved, which I expect shouldn't > be a problem. If any NMI context needed a spin lock for its own > purposes, it would need to use the CPU-reentrant spin lock of > the first console so as to preserve the ordering in case of a panic. IMHO, this is not acceptable. It would be nice to have direct output from NMI but the cost looks to high here. A solution would be to defer the console to irq_work or so. We might also consider using trylock in NMI and have the irq_work just as a fallback. Best Regars, Petr
Re: [RFC PATCH v1 20/25] serial: 8250: implement write_atomic
On 2019-03-08, John Ogness wrote: > If the ringbuffer was fully lockless, we should be able to have > per-console CPU-reentrant spin locks as long as the ordering is > preserved, which I expect shouldn't be a problem. If any NMI context > needed a spin lock for its own purposes, it would need to use the > CPU-reentrant spin lock of the first console so as to preserve the > ordering in case of a panic. This point is garbage. Sorry. I do not see how we could safely have multiple CPU-reentrant spin locks. Example of a deadlock: CPU0 CPU1 printkprintk console2.lock console1.lock NMI NMI printkprintk console1.lock console2.lock >> ... it should not be a common lock for the ring buffer and all >> consoles. > > If the ring buffer becomes fully lockless, then we could move to > per-console CPU-reentrant spin locks. A fully lockless ring buffer will reduce the scope of the one, global CPU-reentrant spin lock. But I do not see how we can safely have multiple of these. If it is part of printk, it is already implicitly on every line of code. John Ogness
Re: [RFC PATCH v1 20/25] serial: 8250: implement write_atomic
On 2019-02-27, Petr Mladek wrote: Implement a non-sleeping NMI-safe write_atomic console function in order to support emergency printk messages. >>> >>> 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. > > Who says _never_? I agree that it is not reasonable. But the > history shows that it happens. Right, which is why it would need to become policy. The emergency messages (aka write_atomic) introduce a new requirement to the kernel because this callback must be callable from any context. The console drivers must have some way of synchronizing. The CPU-reentrant spin lock is the only solution I am aware of. > In principle, there is nothing wrong in using spinlock in NMI > when it is used only in NMI. The CPU-reentrant spin lock _will_ be used in NMI context and potentially could be used from any line of NMI code (if, for example, a panic is triggered). The problem is when you have 2 different spin locks in NMI context and their ordering cannot be guaranteed. And since I am introducing an implicit spin lock that potentially could be locked from any line of code, any explicit use of a spin lock in NMI could would really be adding a 2nd spin lock and thus deadlock potential. If the ringbuffer was fully lockless, we should be able to have per-console CPU-reentrant spin locks as long as the ordering is preserved, which I expect shouldn't be a problem. If any NMI context needed a spin lock for its own purposes, it would need to use the CPU-reentrant spin lock of the first console so as to preserve the ordering in case of a panic. >>>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! > > Sure. But it should not be a common lock for the ring buffer and > all consoles. As long as the ring buffer requires a CPU-reentrant spin lock, I expect that it _must_ be a common lock for all. Consider the situation that the ring buffer writer code causes a panic. I think it is beneficial if at least 1 level of printk recursion is supported so that even these backtraces make it out on the emergency consoles. If the ring buffer becomes fully lockless, then we could move to per-console CPU-reentrant spin locks. John Ogness
Re: [RFC PATCH v1 20/25] serial: 8250: implement write_atomic
On Wed 2019-02-27 11:32:05, John Ogness wrote: > On 2019-02-27, Petr Mladek 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. Who says _never_? I agree that it is not reasonable. But the history shows that it happens. In principle, there is nothing wrong in using spinlock in NMI when it is used only in NMI. > > 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. And I already wrote that they are not added by default and they does not solve kmsg interface. Also we might need to provide a userspace support in advance. We could not release kernel that will make logs hard to read without post processing. At least I do not have the balls to do so. > > 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.430053...@infradead.org > > 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. The synchronization was added just for the thread. I am not sure if Peter is using it in the real life. > > 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! Sure. But it should not be a common lock for the ring buffer and all consoles. Also there are still open questions with NMI and the direct console handling itself. Best Regards, Petr
Re: [RFC PATCH v1 20/25] serial: 8250: implement write_atomic
On 2019-02-27, Petr Mladek 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.430053...@infradead.org 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@linutronix.de
Re: [RFC PATCH v1 20/25] serial: 8250: implement write_atomic
On Tue 2019-02-12 15:29:58, John Ogness 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. It means that it is not completely save when NMIs happen on more CPUs in parallel, for example, when calling nmi_trigger_cpumask_backtrace(). OK, it would be safe when prb_lock() is the only lock taken in the NMI handler. But printk() should not make such limitation to the rest of the system. Not to say, that we would most likely need to add a lock back into nmi_cpu_backtrace() to keep the output sane. 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.430053...@infradead.org 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()). 2. I am afraid that we need to add some locking between CPUs to avoid mixing characters from directly printed messages. This would be safe everywhere expect in NMI. Then we could either risk ignoring the lock in NMI (there should be few messages anyway, the backtraces would get synchronized another way). Or we might need a compromise between handling console using the current owner and offload. Best Regards, Petr
