[PATCH 2/2] enable dumpstack() to printk log level

[PATCH 2/2] enable dumpstack() to printk log level

[he, bo]

From: he bo <bo.he@intel.com>

Function dump_stack calls printk with default log level.
At some scenarios, we need dump the logs at special log level.
For example, Android aplog saves log at a low level to reduce
log storage. With __might_sleep checking, it just prints out
the warning source line with KERNE_ERR and Android saves it to aplog,
but later dump_stack log is not saved. We couldn’t see the
important information when developers check it.
the patch adds function dump_stack_log_lvl to support this capability.

Signed-off-by: he, bo <bo.he@intel.com>
Reviewed-by: zhang, yanmin <yanmin.zhang@intel.com>
---
 arch/x86/kernel/dumpstack.c |   19 ++++++++++++++-----
 include/linux/printk.h      |    1 +
 kernel/sched/core.c         |    2 +-
 lib/dump_stack.c            |    7 ++++++-
 4 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index 4025fe4..7b2160c 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -177,20 +177,29 @@ void show_stack(struct task_struct *task, unsigned long *sp)
 }
 
 /*
- * The architecture-independent dump_stack generator
+ * The architecture-independent dump_stack_log_lvl generator
  */
-void dump_stack(void)
+void dump_stack_log_lvl(char *log_lvl)
 {
 	unsigned long bp;
 	unsigned long stack;
 
 	bp = stack_frame(current, NULL);
-	printk("Pid: %d, comm: %.20s %s %s %.*s\n",
-		current->pid, current->comm, print_tainted(),
+	printk("%sPid: %d, comm: %.20s %s %s %.*s\n",
+		log_lvl, current->pid, current->comm, print_tainted(),
 		init_utsname()->release,
 		(int)strcspn(init_utsname()->version, " "),
 		init_utsname()->version);
-	show_trace(NULL, NULL, &stack, bp);
+	show_trace_log_lvl(NULL, NULL, &stack, bp, log_lvl);
+}
+EXPORT_SYMBOL(dump_stack_log_lvl);
+
+/*
+ * The architecture-independent dump_stack generator
+ */
+void dump_stack(void)
+{
+	dump_stack_log_lvl("");
 }
 EXPORT_SYMBOL(dump_stack);
 
diff --git a/include/linux/printk.h b/include/linux/printk.h
index 0525927..2a13dd8 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -157,6 +157,7 @@ static inline void setup_log_buf(int early)
 #endif
 
 extern void dump_stack(void) __cold;
+extern void dump_stack_log_lvl(char *log_lvl) __cold;
 
 #ifndef pr_fmt
 #define pr_fmt(fmt) fmt
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 503d642..ab0dcdf 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -7060,7 +7060,7 @@ void __might_sleep(const char *file, int line, int preempt_offset)
 	debug_show_held_locks(current);
 	if (irqs_disabled())
 		print_irqtrace_events(current);
-	dump_stack();
+	dump_stack_log_lvl(KERN_ERR);
 }
 EXPORT_SYMBOL(__might_sleep);
 #endif
diff --git a/lib/dump_stack.c b/lib/dump_stack.c
index 53bff4c..d44ca5e 100644
--- a/lib/dump_stack.c
+++ b/lib/dump_stack.c
@@ -11,5 +11,10 @@ void dump_stack(void)
 	printk(KERN_NOTICE
 		"This architecture does not implement dump_stack()\n");
 }
-
 EXPORT_SYMBOL(dump_stack);
+
+void dump_stack_log_lvl(char *log_lvl)
+{
+	dump_stack();
+}
+EXPORT_SYMBOL(dump_stack_log_lvl);
-- 
1.7.1

Re: [PATCH 2/2] enable dumpstack() to printk log level

[Ingo Molnar]

* he, bo <bo.he@intel.com> wrote:

> --- a/arch/x86/kernel/dumpstack.c
> +++ b/arch/x86/kernel/dumpstack.c
> @@ -177,20 +177,29 @@ void show_stack(struct task_struct *task, unsigned long *sp)
>  }
>  
>  /*
> - * The architecture-independent dump_stack generator
> + * The architecture-independent dump_stack_log_lvl generator

I guess it wants to say "architecture-dependent"?

> +	show_trace_log_lvl(NULL, NULL, &stack, bp, log_lvl);
> +}
> +EXPORT_SYMBOL(dump_stack_log_lvl);

this should be a _GPL export.

> +}
> +EXPORT_SYMBOL(dump_stack_log_lvl);

Ditto.

Looks useful otherwise.

Thanks,

	Ingo

Re: [PATCH 2/2] enable dumpstack() to printk log level

[he, bo]

On Fri, 2012-03-23 at 13:22 +0100, Ingo Molnar wrote:
> * he, bo <bo.he@intel.com> wrote:
> 
> > --- a/arch/x86/kernel/dumpstack.c
> > +++ b/arch/x86/kernel/dumpstack.c
> > @@ -177,20 +177,29 @@ void show_stack(struct task_struct *task, unsigned long *sp)
> >  }
Ingo,

Thanks for your kind comments. It's the 1st time for me to send patches to LKML.
There are 2 patches. I sent the 1st one to a wrong address. I will resend the 2 patches.

> >  
> >  /*
> > - * The architecture-independent dump_stack generator
> > + * The architecture-independent dump_stack_log_lvl generator
> 
> I guess it wants to say "architecture-dependent"?
Right. I will change it to architecture-dependent.

> 
> > +	show_trace_log_lvl(NULL, NULL, &stack, bp, log_lvl);
> > +}
> > +EXPORT_SYMBOL(dump_stack_log_lvl);
> 
> this should be a _GPL export.
Yes. I will change it.

> 
> > +}
> > +EXPORT_SYMBOL(dump_stack_log_lvl);
Yes. I will change it.

[PATCH_V2 1/2] fix the bug that printk can't support printk(KERN_LEVEL)

[he, bo]

From: he bo <bo.he@intel.com>

Usually, there is a special scenario that developer wants to
call printk to just set up a log level (might be transferred
here as a parameter from upper level), then, later calling
of printk prints out real string with the same log level
continuously.

Current function vprintk has an issue to support this capability.
When the whole string in one calling to printk is just a log level,
it ignores it.

Signed-off-by: he, bo <bo.he@intel.com>
Reviewed-by: Zhang, Yanmin <yanmin.zhang@intel.com>
---
 kernel/printk.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/kernel/printk.c b/kernel/printk.c
index b663c2c..473afdb 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -909,7 +909,7 @@ asmlinkage int vprintk(const char *fmt, va_list args)
 	 * Copy the output into log_buf. If the caller didn't provide
 	 * the appropriate log prefix, we insert them here
 	 */
-	for (; *p; p++) {
+	for (; plen || *p; p++) {
 		if (new_text_line) {
 			new_text_line = 0;
 
@@ -920,6 +920,7 @@ asmlinkage int vprintk(const char *fmt, va_list args)
 				for (i = 0; i < plen; i++)
 					emit_log_char(printk_buf[i]);
 				printed_len += plen;
+				plen = 0;
 			} else {
 				/* Add log prefix */
 				emit_log_char('<');
@@ -946,10 +947,10 @@ asmlinkage int vprintk(const char *fmt, va_list args)
 				printed_len += tlen;
 			}
 
-			if (!*p)
-				break;
 		}
 
+		if (!*p)
+			break;
 		emit_log_char(*p);
 		if (*p == '\n')
 			new_text_line = 1;
-- 
1.7.1

Re: [PATCH_V2 1/2] fix the bug that printk can't support printk(KERN_LEVEL)

[Yanmin Zhang]

On Mon, 2012-03-26 at 09:36 +0800, he, bo wrote:
> From: he bo <bo.he@intel.com>
> 
> Usually, there is a special scenario that developer wants to
> call printk to just set up a log level (might be transferred
> here as a parameter from upper level), then, later calling
> of printk prints out real string with the same log level
> continuously.
The patch fixes a real issue. See the function.

static void print_trace_address(void *data, unsigned long addr, int reliable)
{
        touch_nmi_watchdog();
        printk(data);		<============= Here is to print the log level only
        printk_address(addr, reliable);
}

Without the patch, printk(data) doesn't emit the log level,
so printk_address would use the default log level.

Besides above real issue, developer might write below codes to debug issues.
printk("%s", log_level);
for (i = 0; i < fields; i++) {
	printk("one field");
}
I did so years ago and felt weird why the log level was incorrect. :)

> 
> Current function vprintk has an issue to support this capability.
> When the whole string in one calling to printk is just a log level,
> it ignores it.
> 
> Signed-off-by: he, bo <bo.he@intel.com>
> Reviewed-by: Zhang, Yanmin <yanmin.zhang@intel.com>
> ---
>  kernel/printk.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/printk.c b/kernel/printk.c
> index b663c2c..473afdb 100644
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -909,7 +909,7 @@ asmlinkage int vprintk(const char *fmt, va_list args)
>  	 * Copy the output into log_buf. If the caller didn't provide
>  	 * the appropriate log prefix, we insert them here
>  	 */
> -	for (; *p; p++) {
> +	for (; plen || *p; p++) {
>  		if (new_text_line) {
>  			new_text_line = 0;
>  
> @@ -920,6 +920,7 @@ asmlinkage int vprintk(const char *fmt, va_list args)
>  				for (i = 0; i < plen; i++)
>  					emit_log_char(printk_buf[i]);
>  				printed_len += plen;
> +				plen = 0;
>  			} else {
>  				/* Add log prefix */
>  				emit_log_char('<');
> @@ -946,10 +947,10 @@ asmlinkage int vprintk(const char *fmt, va_list args)
>  				printed_len += tlen;
>  			}
>  
> -			if (!*p)
> -				break;
>  		}
>  
> +		if (!*p)
> +			break;
>  		emit_log_char(*p);
>  		if (*p == '\n')
>  			new_text_line = 1;

Re: [PATCH_V2 1/2] fix the bug that printk can't support printk(KERN_LEVEL)

[he, bo]

On Mon, 2012-03-26 at 09:36 +0800, he, bo wrote:
> From: he bo <bo.he@intel.com>
> 
> Usually, there is a special scenario that developer wants to
> call printk to just set up a log level (might be transferred
> here as a parameter from upper level), then, later calling
> of printk prints out real string with the same log level
> continuously.
> 
> Current function vprintk has an issue to support this capability.
> When the whole string in one calling to printk is just a log level,
> it ignores it.
> 
> Signed-off-by: he, bo <bo.he@intel.com>
> Reviewed-by: Zhang, Yanmin <yanmin.zhang@intel.com>
> ---
>  kernel/printk.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
Ingo,

What's your opinion about the 2 patches? The 1st patch fixes an issue
and 2nd patch enhances dump_stack with log level.

https://lkml.org/lkml/2012/3/25/152
https://lkml.org/lkml/2012/3/25/153

Thanks,
Bo

> 
> diff --git a/kernel/printk.c b/kernel/printk.c
> index b663c2c..473afdb 100644
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -909,7 +909,7 @@ asmlinkage int vprintk(const char *fmt, va_list args)
>  	 * Copy the output into log_buf. If the caller didn't provide
>  	 * the appropriate log prefix, we insert them here
>  	 */
> -	for (; *p; p++) {
> +	for (; plen || *p; p++) {
>  		if (new_text_line) {
>  			new_text_line = 0;
>  
> @@ -920,6 +920,7 @@ asmlinkage int vprintk(const char *fmt, va_list args)
>  				for (i = 0; i < plen; i++)
>  					emit_log_char(printk_buf[i]);
>  				printed_len += plen;
> +				plen = 0;
>  			} else {
>  				/* Add log prefix */
>  				emit_log_char('<');
> @@ -946,10 +947,10 @@ asmlinkage int vprintk(const char *fmt, va_list args)
>  				printed_len += tlen;
>  			}
>  
> -			if (!*p)
> -				break;
>  		}
>  
> +		if (!*p)
> +			break;
>  		emit_log_char(*p);
>  		if (*p == '\n')
>  			new_text_line = 1;

[PATCH_V2 2/2] enable dumpstack() to printk log level

[he, bo]

From: he bo <bo.he@intel.com>

Function dump_stack calls printk with default log level.
At some scenarios, we need dump the logs at special log level.
For example, Android aplog saves log at a low level to reduce
log storage. With __might_sleep checking, it just prints out
the warning source line with KERNE_ERR and Android saves it to aplog,
but later dump_stack log is not saved. We couldn’t see the
important information when developers check it.
the patch adds function dump_stack_log_lvl to support this capability.

Signed-off-by: he, bo <bo.he@intel.com>
Reviewed-by: zhang, yanmin <yanmin.zhang@intel.com>
---
 arch/x86/kernel/dumpstack.c |   19 ++++++++++++++-----
 include/linux/printk.h      |    1 +
 kernel/sched/core.c         |    2 +-
 lib/dump_stack.c            |    7 ++++++-
 4 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index 4025fe4..2ffa890 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -177,20 +177,29 @@ void show_stack(struct task_struct *task, unsigned long *sp)
 }
 
 /*
- * The architecture-independent dump_stack generator
+ * The architecture-dependent dump_stack_log_lvl generator
  */
-void dump_stack(void)
+void dump_stack_log_lvl(char *log_lvl)
 {
 	unsigned long bp;
 	unsigned long stack;
 
 	bp = stack_frame(current, NULL);
-	printk("Pid: %d, comm: %.20s %s %s %.*s\n",
-		current->pid, current->comm, print_tainted(),
+	printk("%sPid: %d, comm: %.20s %s %s %.*s\n",
+		log_lvl, current->pid, current->comm, print_tainted(),
 		init_utsname()->release,
 		(int)strcspn(init_utsname()->version, " "),
 		init_utsname()->version);
-	show_trace(NULL, NULL, &stack, bp);
+	show_trace_log_lvl(NULL, NULL, &stack, bp, log_lvl);
+}
+EXPORT_SYMBOL_GPL(dump_stack_log_lvl);
+
+/*
+ * The architecture-dependent dump_stack generator
+ */
+void dump_stack(void)
+{
+	dump_stack_log_lvl("");
 }
 EXPORT_SYMBOL(dump_stack);
 
diff --git a/include/linux/printk.h b/include/linux/printk.h
index 0525927..2a13dd8 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -157,6 +157,7 @@ static inline void setup_log_buf(int early)
 #endif
 
 extern void dump_stack(void) __cold;
+extern void dump_stack_log_lvl(char *log_lvl) __cold;
 
 #ifndef pr_fmt
 #define pr_fmt(fmt) fmt
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 503d642..ab0dcdf 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -7060,7 +7060,7 @@ void __might_sleep(const char *file, int line, int preempt_offset)
 	debug_show_held_locks(current);
 	if (irqs_disabled())
 		print_irqtrace_events(current);
-	dump_stack();
+	dump_stack_log_lvl(KERN_ERR);
 }
 EXPORT_SYMBOL(__might_sleep);
 #endif
diff --git a/lib/dump_stack.c b/lib/dump_stack.c
index 53bff4c..fd14940 100644
--- a/lib/dump_stack.c
+++ b/lib/dump_stack.c
@@ -11,5 +11,10 @@ void dump_stack(void)
 	printk(KERN_NOTICE
 		"This architecture does not implement dump_stack()\n");
 }
-
 EXPORT_SYMBOL(dump_stack);
+
+void dump_stack_log_lvl(char *log_lvl)
+{
+	dump_stack();
+}
+EXPORT_SYMBOL_GPL(dump_stack_log_lvl);
-- 
1.7.1