Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1590852 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC PATCH] printk: Make functions of pr_<level> macros Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-03-02 06:50 +0100
    Re: [RFC PATCH] printk: Make functions of pr_<level> macros Joe Perches <joe@perches.com> - 2017-03-02 07:00 +0100

#1590852 — 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

[toc] | [next] | [standalone]

#1590854

On Thu, 2017-03-02 at 14:35 +0900, Sergey Senozhatsky wrote:
> 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.

Yeah, more when more calls are converted,

Maybe more like 12k, still the goal is to 
create singletons for the pr_fmt prefixes
and whatever __func__ uses that are most
common via a SOH + flag and using
__builtin_return_address where possible and
appropriate.  That could shrink another 10k
or so.

> [..]
> > +#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()

That was what I had done originally a while ago
https://lkml.org/lkml/2016/6/23/652

Now the with "safe" version, it's a bit more complicated.

[stack depth can be high, ~400 bytes per recursion]

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

Me neither really.  I'd much prefer a direct vprintk_emit.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web