From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1753152AbdCBFlv (ORCPT <rfc822;w@1wt.eu>);
        Thu, 2 Mar 2017 00:41:51 -0500
Received: from mail-pg0-f65.google.com ([74.125.83.65]:33793 "EHLO
        mail-pg0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1750771AbdCBFlt (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Thu, 2 Mar 2017 00:41:49 -0500
Date: Thu, 2 Mar 2017 14:35:09 +0900
From: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
To: Joe Perches <joe@perches.com>
Cc: Petr Mladek <pmladek@suse.com>, Steven Rostedt <rostedt@goodmis.org>,
        linux-kernel@vger.kernel.org,
        Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>,
        Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Subject: Re: [RFC PATCH] printk: Make functions of pr_<level> macros
Message-ID: <20170302053509.GA398@jagdpanzerIV.localdomain>
References: <f75a04ab9b2abdb25278a71342377d8359166938.1488337468.git.joe@perches.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <f75a04ab9b2abdb25278a71342377d8359166938.1488337468.git.joe@perches.com>
User-Agent: Mutt/1.8.0 (2017-02-23)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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