On Fri 2019-02-22 16:06:26, John Ogness wrote: > On 2019-02-22, Petr Mladek <pmladek@xxxxxxxx> wrote: > >>>> + rbuf = prb_reserve(&h, &sprint_rb, PRINTK_SPRINT_MAX); > >>> > >>> The second ring buffer for temporary buffers is really interesting > >>> idea. > >>> > >>> Well, it brings some questions. For example, how many users might > >>> need a reservation in parallel. Or if the nested use might cause > >>> some problems when we decide to use printk-specific ring buffer > >>> implementation. I still have to think about it. > >> > >> Keep in mind that it is only used by the writers, which have the > >> prb_cpulock. Typically there would only be 2 max users: a non-NMI > >> writer that was interrupted during the reserve/commit window and the > >> interrupting NMI that does printk. The only exception would be if the > >> printk-code code itself triggers a BUG_ON or WARN_ON within the > >> reserve/commit window. Then you will have an additional user per > >> recursion level. > > > > I am not sure it is worth to call the ring buffer machinery just > > to handle 2-3 buffers. > > It may be slightly overkill, but: > > 1. We have the prb_cpulock at this point anyway, so it will be > fast. (Both ring buffers share the same prb_cpulock.) I am still not persuaded that we really need the lock. The implementation looks almost ready for a fully lockless writers. But I might be wrong. The lock might be fine when it makes the code easier and does not bring any deadlocks. > 2. Getting a safe buffer is just 1 line of code: prb_reserve() The problem is how complicated code is hidden behind this 1 line of code. > 3. Why should we waste _any_ lines of code implementing the handling of > these special 3-4 buffers? It might be worth if it makes the code more strighforward and less prone to bugs. > > Well, it might be just my mental block. We need to be really careful > > to avoid infinite recursion when storing messages into the log > > buffer. > > The recursion works well. I inserted a triggerable BUG_ON() in > vprintk_emit() _within_ the reserve/commit window and I see a clean > backtrace on the emergency console. Have you tested all possible error situations that might happen? Testing helps a lot. But the real life often brings surprises. Best Regards, Petr
