Re: [PATCH] stop ==== emergency

Re: [PATCH] stop ==== emergency

[Pete Zaitcev]

On Fri, 3 Feb 2006 21:37:09 +0000 (GMT), Hugh Dickins <hugh@veritas.com> wrote:

> +++ linux/arch/i386/kernel/traps.c	2006-02-03 09:59:37.000000000 +0000
> @@ -166,7 +166,8 @@ static void show_trace_log_lvl(struct ta
>  		stack = (unsigned long*)context->previous_esp;
>  		if (!stack)
>  			break;
> -		printk(KERN_EMERG " =======================\n");
> +		printk(log_lvl);
> +		printk(" =======================\n");
>  	}

This is wrong, Hugh. What do you think the priority of the second printk?
It's not log_lvl, that's for sure.

-- Pete

Re: [PATCH] stop ==== emergency

[Hugh Dickins]

On Sun, 5 Feb 2006, Pete Zaitcev wrote:
> On Fri, 3 Feb 2006 21:37:09 +0000 (GMT), Hugh Dickins <hugh@veritas.com> wrote:
> 
> > +++ linux/arch/i386/kernel/traps.c	2006-02-03 09:59:37.000000000 +0000
> > @@ -166,7 +166,8 @@ static void show_trace_log_lvl(struct ta
> >  		stack = (unsigned long*)context->previous_esp;
> >  		if (!stack)
> >  			break;
> > -		printk(KERN_EMERG " =======================\n");
> > +		printk(log_lvl);
> > +		printk(" =======================\n");
> >  	}
> 
> This is wrong, Hugh. What do you think the priority of the second printk?
> It's not log_lvl, that's for sure.

Are you sure?  I've not delved into the printk code itself, but this
does follow the same pattern as in show_stack_log_lvl itself e.g. its
"Call Trace:\n" line.  (I am assuming print_context_stack ends with a
newline, as it does.)

Hugh

Re: [PATCH] stop ==== emergency

[Jean Delvare]

Hi Hugh, Pete,

> > This is wrong, Hugh. What do you think the priority of the second printk?
> > It's not log_lvl, that's for sure.
> 
> Are you sure?  I've not delved into the printk code itself, but this
> does follow the same pattern as in show_stack_log_lvl itself e.g. its
> "Call Trace:\n" line.  (I am assuming print_context_stack ends with a
> newline, as it does.)

The code was correct (and was applied already as far as I can see.)
However, given that printk calls aren't exactly cheap, don't we want to
merge them where possible?


Merge a few printk calls in i386 traps.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
---
 arch/i386/kernel/traps.c |   12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

--- linux-2.6.16-rc2.orig/arch/i386/kernel/traps.c	2006-02-06 07:50:57.000000000 +0100
+++ linux-2.6.16-rc2/arch/i386/kernel/traps.c	2006-02-06 19:42:10.000000000 +0100
@@ -114,8 +114,7 @@
 
 static void print_addr_and_symbol(unsigned long addr, char *log_lvl)
 {
-	printk(log_lvl);
-	printk(" [<%08lx>] ", addr);
+	printk("%s [<%08lx>] ", log_lvl, addr);
 	print_symbol("%s", addr);
 	printk("\n");
 }
@@ -166,8 +165,7 @@
 		stack = (unsigned long*)context->previous_esp;
 		if (!stack)
 			break;
-		printk(log_lvl);
-		printk(" =======================\n");
+		printk("%s =======================\n", log_lvl);
 	}
 }
 
@@ -196,14 +194,12 @@
 			break;
 		if (i && ((i % 8) == 0)) {
 			printk("\n");
-			printk(log_lvl);
-			printk("       ");
+			printk("%s       ", log_lvl);
 		}
 		printk("%08lx ", *stack++);
 	}
 	printk("\n");
-	printk(log_lvl);
-	printk("Call Trace:\n");
+	printk("%sCall Trace:\n", log_lvl);
 	show_trace_log_lvl(task, esp, log_lvl);
 }
 

More merges are possible, but I'm not sure how far we want to go.

Thanks,
-- 
Jean Delvare

Re: [PATCH] stop ==== emergency

[Hugh Dickins]

On Mon, 6 Feb 2006, Jean Delvare wrote:
> 
> However, given that printk calls aren't exactly cheap, don't we want to
> merge them where possible?
> 
> --- linux-2.6.16-rc2.orig/arch/i386/kernel/traps.c	2006-02-06 07:50:57.000000000 +0100
> +++ linux-2.6.16-rc2/arch/i386/kernel/traps.c	2006-02-06 19:42:10.000000000 +0100
> @@ -114,8 +114,7 @@
>  
>  static void print_addr_and_symbol(unsigned long addr, char *log_lvl)
>  {
> -	printk(log_lvl);
> -	printk(" [<%08lx>] ", addr);
> +	printk("%s [<%08lx>] ", log_lvl, addr);
> ....
> 
> More merges are possible, but I'm not sure how far we want to go.

I assumed, as I guess Chuck who put in all the printk(log_lvl)s did,
that they wouldn't be deciphered right if as separate args.  But a
glance at vprintk suggests you're right, they're interpreted after
expanding into printk_buf.

But my own interest in minimizing printk calls is rather low at the
moment; and they're hardly time-critical, are they? fast paths won't
be fast if they're doing printk's, and I hope things haven't got so
bad that we need dump_stack to be fast!  Waste more space? expect so.

More important would be: how's the SMP printk behaviour these days?
Does separating log_lvl from message increase the likelihood that the
log_lvls might be misapplied if different CPUs print at the same time?
If so, then I'd say your changes are very important - but please send
them to Andrew rather than to me.

Hugh

Re: [PATCH] stop ==== emergency

[Pete Zaitcev]

On Mon, 6 Feb 2006 21:11:11 +0000 (GMT), Hugh Dickins <hugh@veritas.com> wrote:

> But my own interest in minimizing printk calls is rather low at the
> moment; and they're hardly time-critical, are they?

Absolutely, saving time by mering printks is rather stupid. That said,
I'm totally sick of "<4>" all over the place. You might not see it on
your VGA, but it leakes everywhere else and is annoying. As far as
I am concerned, every printk must end with a newline and this is why
I disagreed with your  printk(log_lvl);

-- Pete

Re: [PATCH] stop ==== emergency

[Hugh Dickins]

On Mon, 6 Feb 2006, Pete Zaitcev wrote:
> As far as I am concerned, every printk must end with a newline

You're out of luck, I'm afraid.
Hugh

[PATCH] stop ==== emergency

[Hugh Dickins]

dump_stack on page allocation failure presently has an irritating habit
of shouting just "====" at everyone: please stop it.

Signed-off-by: Hugh Dickins <hugh@veritas.com>
---

 arch/i386/kernel/traps.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletion(-)

--- 2.6.16-rc2/arch/i386/kernel/traps.c	2006-02-03 09:31:53.000000000 +0000
+++ linux/arch/i386/kernel/traps.c	2006-02-03 09:59:37.000000000 +0000
@@ -166,7 +166,8 @@ static void show_trace_log_lvl(struct ta
 		stack = (unsigned long*)context->previous_esp;
 		if (!stack)
 			break;
-		printk(KERN_EMERG " =======================\n");
+		printk(log_lvl);
+		printk(" =======================\n");
 	}
 }