linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] printk: rename vprintk_func to vprintk
@ 2021-03-23 14:42 Rasmus Villemoes
  2021-03-25  0:45 ` Steven Rostedt
  2021-03-30 12:32 ` Petr Mladek
  0 siblings, 2 replies; 5+ messages in thread
From: Rasmus Villemoes @ 2021-03-23 14:42 UTC (permalink / raw)
  To: Petr Mladek, Sergey Senozhatsky, Steven Rostedt, John Ogness
  Cc: Rasmus Villemoes, linux-kernel

The printk code is already hard enough to understand. Remove an
unnecessary indirection by renaming vprintk_func to vprintk (adding
the asmlinkage annotation), and removing the vprintk definition from
printk.c. That way, printk is implemented in terms of vprintk as one
would expect, and there's no "vprintk_func, what's that? Some function
pointer that gets set where?"

The declaration of vprintk in linux/printk.h already has the
__printf(1,0) attribute, there's no point repeating that with the
definition - it's for diagnostics in callers.

linux/printk.h already contains a static inline {return 0;} definition
of vprintk when !CONFIG_PRINTK.

Since the corresponding stub definition of vprintk_func was not marked
"static inline", any translation unit including internal.h would get a
definition of vprintk_func - it just so happens that for
!CONFIG_PRINTK, there is precisely one such TU, namely printk.c. Had
there been more, it would be a link error; now it's just a silly waste
of a few bytes of .text, which one must assume are rather precious to
anyone disabling PRINTK.

$ objdump -dr kernel/printk/printk.o
00000330 <vprintk_func>:
 330:   31 c0                   xor    %eax,%eax
 332:   c3                      ret
 333:   8d b4 26 00 00 00 00    lea    0x0(%esi,%eiz,1),%esi
 33a:   8d b6 00 00 00 00       lea    0x0(%esi),%esi

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 kernel/printk/internal.h    | 3 ---
 kernel/printk/printk.c      | 8 +-------
 kernel/printk/printk_safe.c | 3 ++-
 3 files changed, 3 insertions(+), 11 deletions(-)

diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
index 3a8fd491758c..1c7554f0e71b 100644
--- a/kernel/printk/internal.h
+++ b/kernel/printk/internal.h
@@ -21,7 +21,6 @@ int vprintk_store(int facility, int level,
 
 __printf(1, 0) int vprintk_default(const char *fmt, va_list args);
 __printf(1, 0) int vprintk_deferred(const char *fmt, va_list args);
-__printf(1, 0) int vprintk_func(const char *fmt, va_list args);
 void __printk_safe_enter(void);
 void __printk_safe_exit(void);
 
@@ -56,8 +55,6 @@ void defer_console_output(void);
 
 #else
 
-__printf(1, 0) int vprintk_func(const char *fmt, va_list args) { return 0; }
-
 /*
  * In !PRINTK builds we still export logbuf_lock spin_lock, console_sem
  * semaphore and some of console functions (console_unlock()/etc.), so
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 575a34b88936..458707a06124 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2104,12 +2104,6 @@ asmlinkage int vprintk_emit(int facility, int level,
 }
 EXPORT_SYMBOL(vprintk_emit);
 
-asmlinkage int vprintk(const char *fmt, va_list args)
-{
-	return vprintk_func(fmt, args);
-}
-EXPORT_SYMBOL(vprintk);
-
 int vprintk_default(const char *fmt, va_list args)
 {
 	return vprintk_emit(0, LOGLEVEL_DEFAULT, NULL, fmt, args);
@@ -2143,7 +2137,7 @@ asmlinkage __visible int printk(const char *fmt, ...)
 	int r;
 
 	va_start(args, fmt);
-	r = vprintk_func(fmt, args);
+	r = vprintk(fmt, args);
 	va_end(args);
 
 	return r;
diff --git a/kernel/printk/printk_safe.c b/kernel/printk/printk_safe.c
index 2e9e3ed7d63e..87d2e86af122 100644
--- a/kernel/printk/printk_safe.c
+++ b/kernel/printk/printk_safe.c
@@ -367,7 +367,7 @@ void __printk_safe_exit(void)
 	this_cpu_dec(printk_context);
 }
 
-__printf(1, 0) int vprintk_func(const char *fmt, va_list args)
+asmlinkage int vprintk(const char *fmt, va_list args)
 {
 #ifdef CONFIG_KGDB_KDB
 	/* Allow to pass printk() to kdb but avoid a recursion. */
@@ -420,3 +420,4 @@ void __init printk_safe_init(void)
 	/* Flush pending messages that did not have scheduled IRQ works. */
 	printk_safe_flush();
 }
+EXPORT_SYMBOL(vprintk);
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] printk: rename vprintk_func to vprintk
  2021-03-23 14:42 [PATCH] printk: rename vprintk_func to vprintk Rasmus Villemoes
@ 2021-03-25  0:45 ` Steven Rostedt
  2021-03-30 12:32 ` Petr Mladek
  1 sibling, 0 replies; 5+ messages in thread
From: Steven Rostedt @ 2021-03-25  0:45 UTC (permalink / raw)
  To: Rasmus Villemoes
  Cc: Petr Mladek, Sergey Senozhatsky, John Ogness, linux-kernel

On Tue, 23 Mar 2021 15:42:01 +0100
Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> The printk code is already hard enough to understand. Remove an
> unnecessary indirection by renaming vprintk_func to vprintk (adding
> the asmlinkage annotation), and removing the vprintk definition from
> printk.c. That way, printk is implemented in terms of vprintk as one
> would expect, and there's no "vprintk_func, what's that? Some function
> pointer that gets set where?"
> 
> The declaration of vprintk in linux/printk.h already has the
> __printf(1,0) attribute, there's no point repeating that with the
> definition - it's for diagnostics in callers.
> 
> linux/printk.h already contains a static inline {return 0;} definition
> of vprintk when !CONFIG_PRINTK.
> 
> Since the corresponding stub definition of vprintk_func was not marked
> "static inline", any translation unit including internal.h would get a
> definition of vprintk_func - it just so happens that for
> !CONFIG_PRINTK, there is precisely one such TU, namely printk.c. Had
> there been more, it would be a link error; now it's just a silly waste
> of a few bytes of .text, which one must assume are rather precious to
> anyone disabling PRINTK.

I'm guessing that at commit: 42a0bb3f7138 ("printk/nmi: generic
solution for safe printk in NMI"), the special name for vprintk_func()
became obsolete.

Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve


> 
> $ objdump -dr kernel/printk/printk.o
> 00000330 <vprintk_func>:
>  330:   31 c0                   xor    %eax,%eax
>  332:   c3                      ret
>  333:   8d b4 26 00 00 00 00    lea    0x0(%esi,%eiz,1),%esi
>  33a:   8d b6 00 00 00 00       lea    0x0(%esi),%esi
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  kernel/printk/internal.h    | 3 ---
>  kernel/printk/printk.c      | 8 +-------
>  kernel/printk/printk_safe.c | 3 ++-
>  3 files changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
> index 3a8fd491758c..1c7554f0e71b 100644
> --- a/kernel/printk/internal.h
> +++ b/kernel/printk/internal.h
> @@ -21,7 +21,6 @@ int vprintk_store(int facility, int level,
>  
>  __printf(1, 0) int vprintk_default(const char *fmt, va_list args);
>  __printf(1, 0) int vprintk_deferred(const char *fmt, va_list args);
> -__printf(1, 0) int vprintk_func(const char *fmt, va_list args);
>  void __printk_safe_enter(void);
>  void __printk_safe_exit(void);
>  
> @@ -56,8 +55,6 @@ void defer_console_output(void);
>  
>  #else
>  
> -__printf(1, 0) int vprintk_func(const char *fmt, va_list args) { return 0; }
> -
>  /*
>   * In !PRINTK builds we still export logbuf_lock spin_lock, console_sem
>   * semaphore and some of console functions (console_unlock()/etc.), so
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 575a34b88936..458707a06124 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2104,12 +2104,6 @@ asmlinkage int vprintk_emit(int facility, int level,
>  }
>  EXPORT_SYMBOL(vprintk_emit);
>  
> -asmlinkage int vprintk(const char *fmt, va_list args)
> -{
> -	return vprintk_func(fmt, args);
> -}
> -EXPORT_SYMBOL(vprintk);
> -
>  int vprintk_default(const char *fmt, va_list args)
>  {
>  	return vprintk_emit(0, LOGLEVEL_DEFAULT, NULL, fmt, args);
> @@ -2143,7 +2137,7 @@ asmlinkage __visible int printk(const char *fmt, ...)
>  	int r;
>  
>  	va_start(args, fmt);
> -	r = vprintk_func(fmt, args);
> +	r = vprintk(fmt, args);
>  	va_end(args);
>  
>  	return r;
> diff --git a/kernel/printk/printk_safe.c b/kernel/printk/printk_safe.c
> index 2e9e3ed7d63e..87d2e86af122 100644
> --- a/kernel/printk/printk_safe.c
> +++ b/kernel/printk/printk_safe.c
> @@ -367,7 +367,7 @@ void __printk_safe_exit(void)
>  	this_cpu_dec(printk_context);
>  }
>  
> -__printf(1, 0) int vprintk_func(const char *fmt, va_list args)
> +asmlinkage int vprintk(const char *fmt, va_list args)
>  {
>  #ifdef CONFIG_KGDB_KDB
>  	/* Allow to pass printk() to kdb but avoid a recursion. */
> @@ -420,3 +420,4 @@ void __init printk_safe_init(void)
>  	/* Flush pending messages that did not have scheduled IRQ works. */
>  	printk_safe_flush();
>  }
> +EXPORT_SYMBOL(vprintk);


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] printk: rename vprintk_func to vprintk
  2021-03-23 14:42 [PATCH] printk: rename vprintk_func to vprintk Rasmus Villemoes
  2021-03-25  0:45 ` Steven Rostedt
@ 2021-03-30 12:32 ` Petr Mladek
  2021-03-30 12:59   ` John Ogness
  1 sibling, 1 reply; 5+ messages in thread
From: Petr Mladek @ 2021-03-30 12:32 UTC (permalink / raw)
  To: Rasmus Villemoes, John Ogness
  Cc: Sergey Senozhatsky, Steven Rostedt, linux-kernel

On Tue 2021-03-23 15:42:01, Rasmus Villemoes wrote:
> The printk code is already hard enough to understand. Remove an
> unnecessary indirection by renaming vprintk_func to vprintk (adding
> the asmlinkage annotation), and removing the vprintk definition from
> printk.c. That way, printk is implemented in terms of vprintk as one
> would expect, and there's no "vprintk_func, what's that? Some function
> pointer that gets set where?"
> 
> The declaration of vprintk in linux/printk.h already has the
> __printf(1,0) attribute, there's no point repeating that with the
> definition - it's for diagnostics in callers.
> 
> linux/printk.h already contains a static inline {return 0;} definition
> of vprintk when !CONFIG_PRINTK.
> 
> Since the corresponding stub definition of vprintk_func was not marked
> "static inline", any translation unit including internal.h would get a
> definition of vprintk_func - it just so happens that for
> !CONFIG_PRINTK, there is precisely one such TU, namely printk.c. Had
> there been more, it would be a link error; now it's just a silly waste
> of a few bytes of .text, which one must assume are rather precious to
> anyone disabling PRINTK.
> 
> $ objdump -dr kernel/printk/printk.o
> 00000330 <vprintk_func>:
>  330:   31 c0                   xor    %eax,%eax
>  332:   c3                      ret
>  333:   8d b4 26 00 00 00 00    lea    0x0(%esi,%eiz,1),%esi
>  33a:   8d b6 00 00 00 00       lea    0x0(%esi),%esi
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>

Nice clean up!

Reviewed-by: Petr Mladek <pmladek@suse.com>

John,

it conflicts with the patchset removing printk safe buffers[1].
Would you prefer to queue this into the patchset?
Or should I push it into printk/linux.git, printk-rework and you would
base v2 on top of it?

Best Regards,
Petr

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] printk: rename vprintk_func to vprintk
  2021-03-30 12:32 ` Petr Mladek
@ 2021-03-30 12:59   ` John Ogness
  2021-03-30 14:33     ` Petr Mladek
  0 siblings, 1 reply; 5+ messages in thread
From: John Ogness @ 2021-03-30 12:59 UTC (permalink / raw)
  To: Petr Mladek, Rasmus Villemoes
  Cc: Sergey Senozhatsky, Steven Rostedt, linux-kernel

On 2021-03-30, Petr Mladek <pmladek@suse.com> wrote:
> On Tue 2021-03-23 15:42:01, Rasmus Villemoes wrote:
>> The printk code is already hard enough to understand. Remove an
>> unnecessary indirection by renaming vprintk_func to vprintk (adding
>> the asmlinkage annotation), and removing the vprintk definition from
>> printk.c. That way, printk is implemented in terms of vprintk as one
>> would expect, and there's no "vprintk_func, what's that? Some function
>> pointer that gets set where?"
>> 
>> The declaration of vprintk in linux/printk.h already has the
>> __printf(1,0) attribute, there's no point repeating that with the
>> definition - it's for diagnostics in callers.
>> 
>> linux/printk.h already contains a static inline {return 0;} definition
>> of vprintk when !CONFIG_PRINTK.
>> 
>> Since the corresponding stub definition of vprintk_func was not marked
>> "static inline", any translation unit including internal.h would get a
>> definition of vprintk_func - it just so happens that for
>> !CONFIG_PRINTK, there is precisely one such TU, namely printk.c. Had
>> there been more, it would be a link error; now it's just a silly waste
>> of a few bytes of .text, which one must assume are rather precious to
>> anyone disabling PRINTK.
>> 
>> $ objdump -dr kernel/printk/printk.o
>> 00000330 <vprintk_func>:
>>  330:   31 c0                   xor    %eax,%eax
>>  332:   c3                      ret
>>  333:   8d b4 26 00 00 00 00    lea    0x0(%esi,%eiz,1),%esi
>>  33a:   8d b6 00 00 00 00       lea    0x0(%esi),%esi
>> 
>> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>
> Nice clean up!
>
> Reviewed-by: Petr Mladek <pmladek@suse.com>
>
> John,
>
> it conflicts with the patchset removing printk safe buffers[1].
> Would you prefer to queue this into the patchset?
> Or should I push it into printk/linux.git, printk-rework and you would
> base v2 on top of it?

Please push it to printk-rework. I will base my v2 on top of it.

Thanks.

John

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] printk: rename vprintk_func to vprintk
  2021-03-30 12:59   ` John Ogness
@ 2021-03-30 14:33     ` Petr Mladek
  0 siblings, 0 replies; 5+ messages in thread
From: Petr Mladek @ 2021-03-30 14:33 UTC (permalink / raw)
  To: John Ogness
  Cc: Rasmus Villemoes, Sergey Senozhatsky, Steven Rostedt, linux-kernel

On Tue 2021-03-30 14:59:31, John Ogness wrote:
> On 2021-03-30, Petr Mladek <pmladek@suse.com> wrote:
> > On Tue 2021-03-23 15:42:01, Rasmus Villemoes wrote:
> >> The printk code is already hard enough to understand. Remove an
> >> unnecessary indirection by renaming vprintk_func to vprintk (adding
> >> the asmlinkage annotation), and removing the vprintk definition from
> >> printk.c. That way, printk is implemented in terms of vprintk as one
> >> would expect, and there's no "vprintk_func, what's that? Some function
> >> pointer that gets set where?"
> >> 
> >> The declaration of vprintk in linux/printk.h already has the
> >> __printf(1,0) attribute, there's no point repeating that with the
> >> definition - it's for diagnostics in callers.
> >> 
> >> linux/printk.h already contains a static inline {return 0;} definition
> >> of vprintk when !CONFIG_PRINTK.
> >> 
> >> Since the corresponding stub definition of vprintk_func was not marked
> >> "static inline", any translation unit including internal.h would get a
> >> definition of vprintk_func - it just so happens that for
> >> !CONFIG_PRINTK, there is precisely one such TU, namely printk.c. Had
> >> there been more, it would be a link error; now it's just a silly waste
> >> of a few bytes of .text, which one must assume are rather precious to
> >> anyone disabling PRINTK.
> >> 
> >> $ objdump -dr kernel/printk/printk.o
> >> 00000330 <vprintk_func>:
> >>  330:   31 c0                   xor    %eax,%eax
> >>  332:   c3                      ret
> >>  333:   8d b4 26 00 00 00 00    lea    0x0(%esi,%eiz,1),%esi
> >>  33a:   8d b6 00 00 00 00       lea    0x0(%esi),%esi
> >> 
> >> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> >
> > Nice clean up!
> >
> > Reviewed-by: Petr Mladek <pmladek@suse.com>
> >
> > John,
> >
> > it conflicts with the patchset removing printk safe buffers[1].
> > Would you prefer to queue this into the patchset?
> > Or should I push it into printk/linux.git, printk-rework and you would
> > base v2 on top of it?
> 
> Please push it to printk-rework. I will base my v2 on top of it.

The patch is committed in printk/linux.git, branch printk-rework.
It is queued for 5.13.

Best Regards,
Petr

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-03-30 14:34 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-23 14:42 [PATCH] printk: rename vprintk_func to vprintk Rasmus Villemoes
2021-03-25  0:45 ` Steven Rostedt
2021-03-30 12:32 ` Petr Mladek
2021-03-30 12:59   ` John Ogness
2021-03-30 14:33     ` Petr Mladek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).