Re: [PATCH] printk: Add best-effort printk() buffering.

[Joe Perches]

On Wed, 2017-05-03 at 15:21 +0900, Tetsuo Handa wrote:
> Joe Perches wrote:
> > On Sun, 2017-04-30 at 22:54 +0900, Tetsuo Handa wrote:
> > > Sometimes we want to printk() multiple lines in a group without being
> > > disturbed by concurrent printk() from interrupts and/or other threads.
> > > For example, mixed printk() output of multiple thread's dump makes it
> > > hard to interpret.
> > > 
> > > This patch introduces fixed-sized statically allocated buffers for
> > > buffering printk() output for each thread/context in best effort
> > > (i.e. up to PAGE_SIZE bytes, up to 16 concurrent printk() users).
> > 
> > []
> > > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > 
> > []
> > > +#define MAX_PRINTK_BUFFERS 16
> > > +static struct printk_buffer {
> > > +	unsigned long context; /* printk_context() */
> > > +	unsigned int nested;
> > > +	unsigned int used; /* Valid bytes in buf[]. */
> > > +	char buf[PAGE_SIZE];
> > > +} printk_buffers[MAX_PRINTK_BUFFERS];
> > 
> > Perhaps these buffers could be acquired by
> > alloc_page rather than be static structures and
> > the sizeof buf[PAGE_SIZE] should be reduced by
> > 	sizeof(unsigned long) +
> > 	sizeof(unsigned int) +
> > 	sizeof(unsigned int)
> > so that struct printk_buffers is exactly
> > PAGE_SIZE.
> 
> When should the buffers be allocated? If upon boot, there will be little
> difference. If the first time each buffer is needed, we introduce a risk
> of failing to allocate memory using alloc_page(GFP_ATOMIC | __GFP_NOWARN)
> and a risk of stack overflow during alloc_page() because printk() has to be
> prepared for being called from stack-tight situation.

This is supposed to be best effort anyway.

> Also, while dynamic allocation can allow linked list of the buffer, we
> will need to introduce a lock for traversing the list, which might become
> more expensive than walking fixed-sized array of the buffer.

Shouldn't matter as this is supposed to be best effort
and any printk is already quite expensive and not fast-path.

> We could avoid list traversal by passing "struct printk_buffer" argument,
> but since there are so many functions which expect pr_cont() behavior,
> scattering "struct printk_buffer" argument is a big patch.

There would be possible to add pid instead.

> Thus, I think fixed-sized statically allocated buffers is the most
> reasonable choice. Using a CONFIG_ option for controlling how many pages
> should be allocated for "struct printk_buffer" might make sense for systems
> with little RAM.

Simpler, but not better.