Re: [RFC][PATCHv5 3/7] printk: introduce per-cpu safe_print seq buffer

[Petr Mladek]

On Mon 2016-12-12 23:12:30, Sergey Senozhatsky wrote:
> On (12/12/16 14:54), Petr Mladek wrote:
> > On Sat 2016-12-10 12:10:22, Sergey Senozhatsky wrote:
> > > On (12/09/16 17:46), Petr Mladek wrote:
> > > > > -/*
> > > > > - * Safe printk() for NMI context. It uses a per-CPU buffer to
> > > > > - * store the message. NMIs are not nested, so there is always only
> > > > > - * one writer running. But the buffer might get flushed from another
> > > > > - * CPU, so we need to be careful.
> > > > > - */
> > > > 
> > > > We should keep/create a good description here because the function
> > > > has a non-trivial code. What about something like?
> > > > 
> > > 
> > > which is really not related to this patch set.
> > 
> > I am sorry but I do not understand. This patch removes description
> > that explained constrains of a rather complex code. In fact, the
> > constrains has changed because we started using the function also
> > in other context. When will be the right time/patchset to explain
> > it?
> 
> but I didn't remove it.
> 
> $ grep -A3 -B3 'But the buffer might get flushed from another' kernel/printk/printk_safe.c
> 
> /*
>  * Safe printk() for NMI context. It uses a per-CPU buffer to
>  * store the message. NMIs are not nested, so there is always only
>  * one writer running. But the buffer might get flushed from another
>  * CPU, so we need to be careful.
>  */
> static int vprintk_safe_nmi(const char *fmt, va_list args)

I know, it is moved to the caller of the complex function.
And the description of the other new caller explicitly talks
about printk() recursion (nesting). It opens question if
it is still safe and there is no single note about it.
Also there is no explanation why we need the other buffer
at all.


> > > > > +#ifdef CONFIG_PRINTK_NMI
> > > > > +/*
> > > > > + * Safe printk() for NMI context. It uses a per-CPU buffer to
> > > > > + * store the message. NMIs are not nested, so there is always only
> > > > > + * one writer running. But the buffer might get flushed from another
> > > > > + * CPU, so we need to be careful.
> > > > > + */
> > > > 
> > > > Hmm, I wanted to describe why we need another per-CPU buffer in NMI
> > > > and I am not sure that we really need it.
> > > 
> > > NMI-printk can interrupt safe-printk's vsnprintf() in the middle of
> > > the "while (*fmt)" loop: safe-priNMI-PRINTK
> > 
> > But this already happens when any of the WARNs is triggered
> > inside vsnprintf(). Either this is safe or we are in
> > trouble.
> 
> the point was that when printk-safe resumes after being interrupted
> by NMI-printk it continues printing from the offset at which it has
> been interrupted, writing over the lines that were sprintf-d by NMI
> printk; because NMI-printk used the same buffer offset `s->len'. so
> at least part of NMI-printk message will be lost.

Yes, I wrote this in the previous mail as well. I remember that
I already thought about this problem when working on the original
NMI implementation and I forgot it. This is what comments are for.
Even authors forget details and they do not want to get into
the same cycles again and again.

I understand that you are tired with respining the patchset.
But hey, updating comments is easy. And if people only ask
to add some comments, it means that it is most likely
the last round and all is almost done.

I do not know. Maybe you take my comments as criticism.
But it is not meant like this. I only want to safe some
work me and other people in the future.

Best Regards,
Petr