Messages in this thread | | | Date | Thu, 2 Mar 2017 14:35:09 +0900 | From | Sergey Senozhatsky <> | Subject | Re: [RFC PATCH] printk: Make functions of pr_<level> macros |
| |
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
| |