Re: [RFC PATCH] printk: Make functions of pr_<level> macros

[Sergey Senozhatsky]

Hello Joe,

On (02/28/17 19:17), Joe Perches wrote:
> Can save the space that the KERN_<LEVEL> headers require.
> 
> The biggest negative here is the %pV use which needs
> recursion and adds stack depth.
> 
> $ size vmlinux.o* (defconfig, x86-64)
>    text     data     bss      dec     hex  filename
> 12586135  1909841  777528 15273504  e90e20 vmlinux.o.new
> 12590348  1909841  777528 15277717  e91e95 vmlinux.o.old

interesting. 4K.

[..]
> +#define define_pr_func(func, level)			\
> +asmlinkage __visible int func(const char *fmt, ...)	\
> +{							\
> +	va_list args;					\
> +	int r;						\
> +	struct va_format vaf;				\
> +							\
> +	va_start(args, fmt);				\
> +	vaf.fmt = fmt;					\
> +	vaf.va = &args;					\
> +							\
> +	r = printk(level "%pV", &vaf);			\
> +							\
> +	va_end(args);					\
> +							\
> +	return r;					\
> +}							\

hm. that's really hacky (which is a compliment) and a bit complicated.
my quick thought was to tweak vprintk_emit() for 'facility != 0' so it
could get loglevel (and adjust lflags) from the passed level, not from
the text, and then do something like this

#define define_pr_func(func, level) asmlinkage __visible int func(const char *fmt, ...)
{
	va_start(args, fmt);
	r = vprintk_emit(level[0], level[1], NULL, 0, fmt, args);
	va_end();
}

but this won't do the trick. because func()->vprintk_emit() shortcut
disables the printk-safe mechanism:
	func()->printk()->vprintk_func()->this_cpu(printk_context)::print()

this *probably* and *may be* works for dev_printk() /* assuming that
dev_printk() is never called fomr under sched/sempahore/etc locks */,
but for printk() in general it's a huge no-no-no.

so I see that you did the very same thing in 99bcf217183e02ebae for
dev_printk(), which is, once again, probably ok for dev_print(). and
now the question is do we want to add %pV recursion to non-dev
printk-s, and I'm, frankly, don't have any answers at the moment.
sorry.

there are already %pV pr_foo() calls in the kernel. and we even have %pV in
OOM path. now those calls are going to be %pV->%pV = recursion depth 2.

for example I can easily do this thing:

[   38.006766] WARNING: CPU: 2 PID: 368 at lib/vsprintf.c:1901 format_decode+0x226/0x2fd
[   38.006766] Please remove unsupported %] in format string
[..]
[   38.006783] Call Trace:
[   38.006783]  dump_stack+0x68/0x92
[   38.006784]  __warn+0xc2/0xdd
[   38.006784]  warn_slowpath_fmt+0x4b/0x53
[   38.006785]  ? __lock_acquire+0x2ac/0x1501
[   38.006785]  format_decode+0x226/0x2fd
[   38.006786]  vsnprintf+0x89/0x3b7
[   38.006786]  pointer+0x1c3/0x378
[   38.006787]  vsnprintf+0x22d/0x3b7
[   38.006787]  pointer+0x1c3/0x378
[   38.006788]  vsnprintf+0x22d/0x3b7
[   38.006788]  vscnprintf+0xd/0x26
[   38.006788]  vprintk_emit+0x81/0x294
[   38.006789]  ? 0xffffffffa0096000
[   38.006789]  vprintk_default+0x1d/0x1f
[   38.006790]  vprintk_func+0x6c/0x73
[   38.006790]  ? 0xffffffffa0096000
[   38.006791]  printk+0x43/0x4b
[   38.006791]  pr_cont+0x56/0x5e


I wonder if it can get any worse than that. I sort of suspect
it can. in trivial case, dump_stack() can add at least one more
level of recursion by using pr_{err, etc.} instead of corresponding
printk(KERN_{ERR, etc}.

can it be even worse? um... NMI?
any thoughts?

dunno, at the moment I'm not really comfortable with %pV recursion
for every pr_foo() call.

	-ss