Re: [PATCH] printk: inject caller information into the body of message

[Sergey Senozhatsky]

On (09/19/18 20:02), Tetsuo Handa wrote:
> I'm inclined to propose a simple one shown below, similar to just having
> several "struct cont" for concurrent printk() users.

Tetsuo, thanks for the patch.

> What Linus has commented is that implicit context is bad, and below one
> uses explicit context.
> After almost all users are converted to use below one, we might be able
> to get rid of KERN_CONT support.

The good thing about cont buffer is that we flush it on panic. E.g.
core/arch early boot stage can do:

	pr_cont("going to call early_init_foo()...");
	early_init_foo();
	pr_cont("OK\n");

should early_init_foo() panic the system we will have
"going to call early_init_foo()" on the serial console. This can
be addressed if you'd iterate printk_buffers[] in flush_on_panic().

> +#define MAX_PRINTK_BUFFERS 16
> +static struct printk_buffer printk_buffers[MAX_PRINTK_BUFFERS];

Well, hmm, maybe. Now can we have a problem of either too-small or too-large
MAX_PRINTK_BUFFERS. 16 buffers on a 4 CPU arm board most probably will just
waste some memory. At the same time we probably don't want to have NR_CPUS
buffers. The fallback to "regular printk" is still a bit troubling - technically
there may be cases when we don't fix anything.

So, overall, I'm not against your patch. There are some pros and cons,
however.

pr_line() patch seems to be simpler [probably] and smaller [definitely].
The only problem, as you have mentioned, is that people may miscalculate
the size of the buffer, which won't crash us or anything; people can overshot
even a LOG_LINE_MAX buffer. So probably I'm not completely sold on having
a fixed size printk_buffers[].

May be all we want at the end is to drop explicit buffer API and have just
two options in pr_line:

 DEFINE_PR_LINE()	-- 80-bytes (or 256) pr_line // implicit buffer
 DEFINE_PR_LINE_HUGE()	-- 1024-bytes pr_line        // implicit buffer

So, no explicit buffers, just "a normal" pr_line or "a huge" pr_line.
And no "normal printk" fallback; buffered printk line stays buffered.

The 80-bytes limit can be lifted to, say, 256-bytes.

Tetsuo, do you still want to have a fixed size array of printk buffers?

What do others think?


BTW, Tetsuo, I have addressed your pr_line suggestions/corrections.
Couldn't send the patch or reply to emails because I was offline for
a week due to personal reasons; but I can send it now - it does not
have DEFINE_PR_LINE_HUGE() macro. Just a previous version with
corrections which you have pointed out.

	-ss