[patch] Printk kernel version in WARN_ON

[patch] Printk kernel version in WARN_ON

[Arjan van de Ven]

Hi,

today, all oopses contain a version number of the kernel, which is nice
because the people who actually do bother to read the oops get this
vital bit of information always without having to ask the reporter in
another round trip.

However, WARN_ON() right now lacks this information; the patch below
adds this. This information is essential for getting people to use
their time effectively when looking at these things; in addition, it's
essential for tools that try to collect statistics about defects.

Please consider, maybe even for 2.6.24 since its so simple and
important for long term quality

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>

--- linux-2.6.24-rc3/include/asm-generic/bug.h.org	2007-11-17 09:55:00.000000000 -0800
+++ linux-2.6.24-rc3/include/asm-generic/bug.h	2007-11-17 10:11:23.000000000 -0800
@@ -2,6 +2,7 @@
 #define _ASM_GENERIC_BUG_H
 
 #include <linux/compiler.h>
+#include <linux/utsrelease.h>
 
 #ifdef CONFIG_BUG
 
@@ -35,8 +36,8 @@ struct bug_entry {
 #define WARN_ON(condition) ({						\
 	int __ret_warn_on = !!(condition);				\
 	if (unlikely(__ret_warn_on)) {					\
-		printk("WARNING: at %s:%d %s()\n", __FILE__,		\
-			__LINE__, __FUNCTION__);			\
+		printk("WARNING: at %s:%d %s()  (%s)\n", __FILE__,	\
+			__LINE__, __FUNCTION__, UTS_RELEASE);		\
 		dump_stack();						\
 	}								\
 	unlikely(__ret_warn_on);					\


-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [patch] Printk kernel version in WARN_ON

[Andrew Morton]

On Sat, 17 Nov 2007 10:15:52 -0800 Arjan van de Ven <arjan@infradead.org> wrote:

> @@ -35,8 +36,8 @@ struct bug_entry {
>  #define WARN_ON(condition) ({						\
>  	int __ret_warn_on = !!(condition);				\
>  	if (unlikely(__ret_warn_on)) {					\
> -		printk("WARNING: at %s:%d %s()\n", __FILE__,		\
> -			__LINE__, __FUNCTION__);			\
> +		printk("WARNING: at %s:%d %s()  (%s)\n", __FILE__,	\
> +			__LINE__, __FUNCTION__, UTS_RELEASE);		\
>  		dump_stack();						\
>  	}								\
>  	unlikely(__ret_warn_on);					\

that made our 1100-odd WARN_ON sites fatter.

I suppose sometime we should optimise WARN_ON like we did BUG_ON.

Re: [patch] Printk kernel version in WARN_ON

[Arjan van de Ven]

On Sat, 17 Nov 2007 10:27:20 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Sat, 17 Nov 2007 10:15:52 -0800 Arjan van de Ven
> <arjan@infradead.org> wrote:
> 
> > @@ -35,8 +36,8 @@ struct bug_entry {
> >  #define WARN_ON(condition)
> > ({						\ int
> > __ret_warn_on = !!(condition);				\ if
> > (unlikely(__ret_warn_on)) {					\
> > -		printk("WARNING: at %s:%d %s()\n",
> > __FILE__,		\
> > -			__LINE__,
> > __FUNCTION__);			\
> > +		printk("WARNING: at %s:%d %s()  (%s)\n",
> > __FILE__,	\
> > +			__LINE__, __FUNCTION__,
> > UTS_RELEASE);		\
> > dump_stack();
> > \ }
> > \ unlikely(__ret_warn_on);					\
> 
> that made our 1100-odd WARN_ON sites fatter.

by ... not too much at least, gcc ought to be quite good at merging
same-strings into one, so it's just one extra pointer argument



-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [patch] Printk kernel version in WARN_ON

[Andrew Morton]

On Sat, 17 Nov 2007 10:39:47 -0800 Arjan van de Ven <arjan@infradead.org> wrote:

> On Sat, 17 Nov 2007 10:27:20 -0800
> Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > On Sat, 17 Nov 2007 10:15:52 -0800 Arjan van de Ven
> > <arjan@infradead.org> wrote:
> > 
> > > @@ -35,8 +36,8 @@ struct bug_entry {
> > >  #define WARN_ON(condition)
> > > ({						\ int
> > > __ret_warn_on = !!(condition);				\ if
> > > (unlikely(__ret_warn_on)) {					\
> > > -		printk("WARNING: at %s:%d %s()\n",
> > > __FILE__,		\
> > > -			__LINE__,
> > > __FUNCTION__);			\
> > > +		printk("WARNING: at %s:%d %s()  (%s)\n",
> > > __FILE__,	\
> > > +			__LINE__, __FUNCTION__,
> > > UTS_RELEASE);		\
> > > dump_stack();
> > > \ }
> > > \ unlikely(__ret_warn_on);					\
> > 
> > that made our 1100-odd WARN_ON sites fatter.
> 
> by ... not too much at least, gcc ought to be quite good at merging
> same-strings into one, so it's just one extra pointer argument
> 

I think I knew that.  At 1000 callsites.

Re: [patch] Printk kernel version in WARN_ON

[Sam Ravnborg]

On Sat, Nov 17, 2007 at 10:15:52AM -0800, Arjan van de Ven wrote:
> Hi,
> 
> today, all oopses contain a version number of the kernel, which is nice
> because the people who actually do bother to read the oops get this
> vital bit of information always without having to ask the reporter in
> another round trip.
> 
> However, WARN_ON() right now lacks this information; the patch below
> adds this. This information is essential for getting people to use
> their time effectively when looking at these things; in addition, it's
> essential for tools that try to collect statistics about defects.
> 
> Please consider, maybe even for 2.6.24 since its so simple and
> important for long term quality

With this change we will see zillions of files being rebuild each
time we pick up another kernel version from git and friends.

For me it looks like this right now:
#define UTS_RELEASE "2.6.24-rc2-g99fee6d7-dirty"

committing my local changes made it look like:
#define UTS_RELEASE "2.6.24-rc2-g99fee6d7"

The above change will trigger a rebuild of all files
that reference UTS_RELEASE as will all WARN_ON users.

And this is with the default configuration.

So if we want this then we want to push that change to a
seperate function so we rebuild less files for simple changes.

	Sam

Re: [patch] Printk kernel version in WARN_ON

[Arjan van de Ven]

On Sat, 17 Nov 2007 10:46:52 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:

> > by ... not too much at least, gcc ought to be quite good at merging
> > same-strings into one, so it's just one extra pointer argument
> > 
> 
> I think I knew that.  At 1000 callsites.

ok so how about putting the same into dump_stack() instead? (see below)
added bonus is that it's now present for all dumps that use
dump_stack(), not just WARN_ON()
(the format I copied from the exact line used by oopses)



Subject: printk kernel version in WARN_ON and other dump_stack users
From: Arjan van de Ven <arjan@linux.intel.com>

today, all oopses contain a version number of the kernel, which is nice
because the people who actually do bother to read the oops get this
vital bit of information always without having to ask the reporter in
another round trip.

However, WARN_ON() and many other dump_stack() users right now lack this 
information; the patch below adds this. This information is essential for 
getting people to use their time effectively when looking at these things;
in addition, it's essential for tools that try to collect statistics about defects.

Please consider, maybe even for 2.6.24 since its so simple and
important for long term quality processes

The code is identical between 32/64 bit; a lot of this code should be unified over time,
the patch keeps the identical-ness in tact.

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>



--- linux-2.6.24-rc3/arch/x86/kernel/traps_32.c.org	2007-11-17 11:26:17.000000000 -0800
+++ linux-2.6.24-rc3/arch/x86/kernel/traps_32.c	2007-11-17 11:29:12.000000000 -0800
@@ -283,6 +283,11 @@ void dump_stack(void)
 {
 	unsigned long stack;
 
+	printk("Pid: %d, comm: %.20s %s %s %.*s\n",
+		current->pid, current->comm, print_tainted(),
+		init_utsname()->release,
+		(int)strcspn(init_utsname()->version, " "),
+		init_utsname()->version);
 	show_trace(current, NULL, &stack);
 }
 
--- linux-2.6.24-rc3/arch/x86/kernel/traps_64.c.org	2007-11-17 11:26:25.000000000 -0800
+++ linux-2.6.24-rc3/arch/x86/kernel/traps_64.c	2007-11-17 11:29:22.000000000 -0800
@@ -400,6 +400,12 @@ void show_stack(struct task_struct *tsk,
 void dump_stack(void)
 {
 	unsigned long dummy;
+
+	printk("Pid: %d, comm: %.20s %s %s %.*s\n",
+		current->pid, current->comm, print_tainted(),
+		init_utsname()->release,
+		(int)strcspn(init_utsname()->version, " "),
+		init_utsname()->version);
 	show_trace(NULL, NULL, &dummy);
 }
 


-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [patch] Printk kernel version in WARN_ON

[Sam Ravnborg]

On Sat, Nov 17, 2007 at 11:35:01AM -0800, Arjan van de Ven wrote:
> On Sat, 17 Nov 2007 10:46:52 -0800
> Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > > by ... not too much at least, gcc ought to be quite good at merging
> > > same-strings into one, so it's just one extra pointer argument
> > > 
> > 
> > I think I knew that.  At 1000 callsites.
> 
> ok so how about putting the same into dump_stack() instead? (see below)
> added bonus is that it's now present for all dumps that use
> dump_stack(), not just WARN_ON()
> (the format I copied from the exact line used by oopses)

This solved the "zillion files being rebuild" issue I mentioned.
So from that angle it is better.

And I notice you use the namespace aware helpers to access the
kernelrelease string - I assume this is better than direct use
of UTS_RELEASE.

	Sam

Re: [patch] Printk kernel version in WARN_ON

[Denys Vlasenko]

On Saturday 17 November 2007 10:15, Arjan van de Ven wrote:
> Hi,
>
>  #define WARN_ON(condition) ({						\
>  	int __ret_warn_on = !!(condition);				\
>  	if (unlikely(__ret_warn_on)) {					\
> -		printk("WARNING: at %s:%d %s()\n", __FILE__,		\
> -			__LINE__, __FUNCTION__);			\
> +		printk("WARNING: at %s:%d %s()  (%s)\n", __FILE__,	\
> +			__LINE__, __FUNCTION__, UTS_RELEASE);		\
>  		dump_stack();						\
>  	}								\
>  	unlikely(__ret_warn_on);					\

We have ~700 WARN_ONs in the tree. Adding UTS_RELEASE to printk
grows every one of them by at least 5 bytes.

I think it makes sense to move printk out-of-line, to

void print_WARN_ON_warning(const char *file, int line, const char *func);

This will save at least 10 bytes per WARN_ON.
--
vda

Re: [patch] Printk kernel version in WARN_ON

[Ingo Molnar]

* Arjan van de Ven <arjan@infradead.org> wrote:

> ok so how about putting the same into dump_stack() instead? (see 
> below) added bonus is that it's now present for all dumps that use 
> dump_stack(), not just WARN_ON() (the format I copied from the exact 
> line used by oopses)

nice! I did things like this in -rt because it really helps to know 
which process does a WARN_ON() or raw dump_stack().

> Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>

Acked-by: Ingo Molnar <mingo@elte.hu>

unless objections we'll put this into the x86 git tree.

	Ingo

Re: [patch] Printk kernel version in WARN_ON

[Andrew Morton]

On Sun, 18 Nov 2007 01:42:18 +0100 Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Arjan van de Ven <arjan@infradead.org> wrote:
> 
> > ok so how about putting the same into dump_stack() instead? (see 
> > below) added bonus is that it's now present for all dumps that use 
> > dump_stack(), not just WARN_ON() (the format I copied from the exact 
> > line used by oopses)
> 
> nice! I did things like this in -rt because it really helps to know 
> which process does a WARN_ON() or raw dump_stack().
> 
> > Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
> 
> Acked-by: Ingo Molnar <mingo@elte.hu>
> 
> unless objections we'll put this into the x86 git tree.
> 

Should be done for all architectures, methinks.

If so, an appropriate way to do that would be to do
s/dump_stack/arch_dump_stack/ and do a single all-arch implementation of
dump_stack().  (Where we might add new goodies in the future).

Problem is that this will add a new an pointless entry to all the stack
dumps, unless the arch_dump_stack() implementation is smart enough to skip the
innermost frame.

Re: [patch] Printk kernel version in WARN_ON

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> Should be done for all architectures, methinks.
> 
> If so, an appropriate way to do that would be to do 
> s/dump_stack/arch_dump_stack/ and do a single all-arch implementation 
> of dump_stack().  (Where we might add new goodies in the future).

i agree we can clean this up - but this is a single-line thing that is 
very useful for QA so i think utility warrants .24 inclusion. The oops 
printouts are not generalized anyway.

> Problem is that this will add a new an pointless entry to all the 
> stack dumps, unless the arch_dump_stack() implementation is smart 
> enough to skip the innermost frame.

x86 can skip stackframes via stacktrace.c.

	Ingo

Re: [patch] Printk kernel version in WARN_ON

[Arjan van de Ven]

On Sat, 17 Nov 2007 16:57:19 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:

> 
> Should be done for all architectures, methinks.
> 
> If so, an appropriate way to do that would be to do
> s/dump_stack/arch_dump_stack/ and do a single all-arch implementation
> of dump_stack().  (Where we might add new goodies in the future).
> 
> Problem is that this will add a new an pointless entry to all the
> stack dumps, unless the arch_dump_stack() implementation is smart
> enough to skip the innermost frame.

it also adds a stackframe to the "oh my god I'm low on stack space lets
get a dump out" codepath ;(

> 


-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [patch] Printk kernel version in WARN_ON

[Pavel Machek]

On Sat 2007-11-17 20:42:40, Sam Ravnborg wrote:
> On Sat, Nov 17, 2007 at 11:35:01AM -0800, Arjan van de Ven wrote:
> > On Sat, 17 Nov 2007 10:46:52 -0800
> > Andrew Morton <akpm@linux-foundation.org> wrote:
> > 
> > > > by ... not too much at least, gcc ought to be quite good at merging
> > > > same-strings into one, so it's just one extra pointer argument
> > > > 
> > > 
> > > I think I knew that.  At 1000 callsites.
> > 
> > ok so how about putting the same into dump_stack() instead? (see below)
> > added bonus is that it's now present for all dumps that use
> > dump_stack(), not just WARN_ON()
> > (the format I copied from the exact line used by oopses)
> 
> This solved the "zillion files being rebuild" issue I mentioned.
> So from that angle it is better.
> 
> And I notice you use the namespace aware helpers to access the
> kernelrelease string - I assume this is better than direct use
> of UTS_RELEASE.

I'm not sure... namespace-aware kernel release seems like madness to
me. This stackdump is from 2.6.10, but the other stackdump is from
2.6.20 running at the same machine at the same time?! Why do namespace
helpers touch UTS_RELEASE.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html