[RFC 0/4] CONFIG_STABLE to switch off development checks

[RFC 0/4] CONFIG_STABLE to switch off development checks

[clameter]

A while back we talked about having the capability of switching off checks
like the one for kmalloc(0) for stable kernel releases. This is a first stab
at such functionality. It adds #ifdef CONFIG_STABLE for now. Maybe we can
come up with some better way to handle it later. There should alsol be some
way to set CONFIG_STABLE from the Makefile.

CONFIG_STABLE switches off

- kmalloc(0) check in both slab allocators
- SLUB banner
- Makes SLUB tolerate object corruption like SLAB (not sure if we really want
  to go down this route. See patch)

-- 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[RFC 1/4] CONFIG_STABLE: Define it

[clameter]

[-- Attachment #1: stable_init --]
[-- Type: text/plain, Size: 964 bytes --]

Introduce CONFIG_STABLE to control checks only useful for development.

Signed-off-by: Christoph Lameter <clameter@sgi.com>

---
 init/Kconfig |    7 +++++++
 1 file changed, 7 insertions(+)

Index: slub/init/Kconfig
===================================================================
--- slub.orig/init/Kconfig	2007-05-30 16:35:05.000000000 -0700
+++ slub/init/Kconfig	2007-05-30 16:35:45.000000000 -0700
@@ -65,6 +65,13 @@ endmenu
 
 menu "General setup"
 
+config STABLE
+	bool "Stable kernel"
+	help
+	  If the kernel is configured to be a stable kernel then various
+	  checks that are only of interest to kernel development will be
+	  omitted.
+
 config LOCALVERSION
 	string "Local version - append to kernel release"
 	help

-- 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[RFC 2/4] CONFIG_STABLE: Switch off kmalloc(0) tests in slab allocators

[clameter]

[-- Attachment #1: stable_kmalloc_zero --]
[-- Type: text/plain, Size: 1674 bytes --]

We do not want kmalloc(0) to trigger stackdumps if this is a stable
kernel. kmalloc(0) is currently harmless.

Signed-off-by: Christoph Lameter <clameter@sgi.com>

---
 include/linux/slub_def.h |    2 ++
 mm/slab.c                |    2 ++
 2 files changed, 4 insertions(+)

Index: slub/include/linux/slub_def.h
===================================================================
--- slub.orig/include/linux/slub_def.h	2007-05-30 16:35:05.000000000 -0700
+++ slub/include/linux/slub_def.h	2007-05-30 16:37:39.000000000 -0700
@@ -74,6 +74,7 @@ extern struct kmem_cache kmalloc_caches[
  */
 static inline int kmalloc_index(size_t size)
 {
+#ifndef CONFIG_STABLE
 	/*
 	 * We should return 0 if size == 0 (which would result in the
 	 * kmalloc caller to get NULL) but we use the smallest object
@@ -81,6 +82,7 @@ static inline int kmalloc_index(size_t s
 	 * we can discover locations where we do 0 sized allocations.
 	 */
 	WARN_ON_ONCE(size == 0);
+#endif
 
 	if (size > KMALLOC_MAX_SIZE)
 		return -1;
Index: slub/mm/slab.c
===================================================================
--- slub.orig/mm/slab.c	2007-05-30 16:35:05.000000000 -0700
+++ slub/mm/slab.c	2007-05-30 16:37:39.000000000 -0700
@@ -774,7 +774,9 @@ static inline struct kmem_cache *__find_
 	 */
 	BUG_ON(malloc_sizes[INDEX_AC].cs_cachep == NULL);
 #endif
+#ifndef CONFIG_STABLE
 	WARN_ON_ONCE(size == 0);
+#endif
 	while (size > csizep->cs_size)
 		csizep++;
 

-- 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[RFC 3/4] CONFIG_STABLE: Switch off SLUB banner

[clameter]

[-- Attachment #1: stable_no_slub_banner --]
[-- Type: text/plain, Size: 1134 bytes --]

The one line that SLUB prints on bootup is useful for debugging but I do not
think that we would like to have it on in stable kernels.

Signed-off-by: Christoph Lameter <clameter@sgi.com>

---
 mm/slub.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Index: slub/mm/slub.c
===================================================================
--- slub.orig/mm/slub.c	2007-05-30 16:40:04.000000000 -0700
+++ slub/mm/slub.c	2007-05-30 16:40:40.000000000 -0700
@@ -2518,12 +2518,13 @@ void __init kmem_cache_init(void)
 
 	kmem_size = offsetof(struct kmem_cache, cpu_slab) +
 				nr_cpu_ids * sizeof(struct page *);
-
+#ifndef CONFIG_STABLE
 	printk(KERN_INFO "SLUB: Genslabs=%d, HWalign=%d, Order=%d-%d, MinObjects=%d,"
 		" Processors=%d, Nodes=%d\n",
 		KMALLOC_SHIFT_HIGH, cache_line_size(),
 		slub_min_order, slub_max_order, slub_min_objects,
 		nr_cpu_ids, nr_node_ids);
+#endif
 }
 
 /*

-- 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[RFC 4/4] CONFIG_STABLE: SLUB: Prefer object corruption over failure

[clameter]

[-- Attachment #1: stable_slub_robust --]
[-- Type: text/plain, Size: 3556 bytes --]

I am not sure if this is the right thing to do .... I suspect there may be
people on both side of the issue.

SLUB places its free pointer in the first word of an object. There it is very
vulnerable since write after frees usually occur to the first word. If
objects are tighly packed then the writes after the object boundary will
also immediately hit free pointer. A corrupted free pointer typically
leads to a deferencing of a pointer to nowhere and the system will
stop with a bad pointer dereference.

While this is good for development (we catch lots of cases that would
otherwise corrupt slab objects) it is desirable for stable releases that
SLUB behaves more like SLAB: Tolerate object corruption in order to let
the system continue its work.

This patch produces that type of SLAB behavior in SLUB for CONFIG_STABLE by
moving the free pointer to the second word of the object. The first word
can then be overwritten and the SLUB will continue without noticing (unless
we boot with slub_debug).

Signed-off-by: Christoph Lameter <clameter@sgi.com>

---
 mm/slub.c |   29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

Index: slub/mm/slub.c
===================================================================
--- slub.orig/mm/slub.c	2007-05-30 16:40:40.000000000 -0700
+++ slub/mm/slub.c	2007-05-30 16:41:57.000000000 -0700
@@ -374,7 +374,7 @@ static struct track *get_track(struct km
 {
 	struct track *p;
 
-	if (s->offset)
+	if (s->offset > s->objsize)
 		p = object + s->offset + sizeof(void *);
 	else
 		p = object + s->inuse;
@@ -387,7 +387,7 @@ static void set_track(struct kmem_cache 
 {
 	struct track *p;
 
-	if (s->offset)
+	if (s->offset > s->objsize)
 		p = object + s->offset + sizeof(void *);
 	else
 		p = object + s->inuse;
@@ -484,7 +484,7 @@ static void print_trailer(struct kmem_ca
 		print_section("Redzone", p + s->objsize,
 			s->inuse - s->objsize);
 
-	if (s->offset)
+	if (s->offset > s->objsize)
 		off = s->offset + sizeof(void *);
 	else
 		off = s->inuse;
@@ -618,7 +618,7 @@ static int check_pad_bytes(struct kmem_c
 {
 	unsigned long off = s->inuse;	/* The end of info */
 
-	if (s->offset)
+	if (s->offset > s->objsize)
 		/* Freepointer is placed after the object. */
 		off += sizeof(void *);
 
@@ -696,7 +696,7 @@ static int check_object(struct kmem_cach
 		check_pad_bytes(s, page, p);
 	}
 
-	if (!s->offset && active)
+	if (s->offset < s->objsize && active)
 		/*
 		 * Object and freepointer overlap. Cannot check
 		 * freepointer while object is allocated.
@@ -1947,6 +1947,25 @@ static int calculate_sizes(struct kmem_c
 	 */
 	size = ALIGN(size, sizeof(void *));
 
+
+#ifdef CONFIG_STABLE
+	if (size >= 2*sizeof(void *)) {
+		/*
+		 * For SLUB robustness we use the second word. The first word
+		 * is likely to be corrupted by write after the object end or
+		 * write after free. This means we do not fail because of
+		 * a corrupted free pointer. We continue with the corrupted
+		 * object like SLAB.
+		 */
+		s->offset = sizeof(void *);
+	} else
+#endif
+	/*
+	 * Object is too small to push back the free pointer a word. Or this is
+	 * not a release kernel. We prefer failures over object corruption.
+	 */
+	s->offset = 0;
+
 #ifdef CONFIG_SLUB_DEBUG
 	/*
 	 * If we are Redzoning then check if there is some space between the

-- 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC 1/4] CONFIG_STABLE: Define it

[young dave]

Hi Christoph,

>Introduce CONFIG_STABLE to control checks only useful for development.

What about control checks only as SLUB_DEBUG is set?

Regards
dave

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC 1/4] CONFIG_STABLE: Define it

[Christoph Lameter]

On Thu, 31 May 2007, young dave wrote:

> Hi Christoph,
> 
> > Introduce CONFIG_STABLE to control checks only useful for development.
> 
> What about control checks only as SLUB_DEBUG is set?

Debug code is always included in all builds unless it is an embedded 
system. Debug code is kept out of the hot path.

Disabling SLUB_DEBUG should only be done for embedded systems. That is why 
the option is in CONFIG_EMBEDDED.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC 1/4] CONFIG_STABLE: Define it

[Stefan Richter]

> --- slub.orig/init/Kconfig	2007-05-30 16:35:05.000000000 -0700
> +++ slub/init/Kconfig	2007-05-30 16:35:45.000000000 -0700
> @@ -65,6 +65,13 @@ endmenu
>  
>  menu "General setup"
>  
> +config STABLE
> +	bool "Stable kernel"
> +	help
> +	  If the kernel is configured to be a stable kernel then various
> +	  checks that are only of interest to kernel development will be
> +	  omitted.
> +
>  config LOCALVERSION
>  	string "Local version - append to kernel release"
>  	help

a) Why in Kconfig, why not in Makefile?

b) Of course nobody wants STABLE=n. :-)  How about:

config RELEASE
	bool "Build for release"
	help
	  If the kernel is declared as a release build here, then
	  various checks that are only of interest to kernel development
	  will be omitted.

c) A drawback of this general option is, it's hard to tell what will be
omitted in particular.
-- 
Stefan Richter
-=====-=-=== -=-= =====
http://arcgraph.de/sr/

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC 1/4] CONFIG_STABLE: Define it

[David Miller, Stefan Richter]

> b) Of course nobody wants STABLE=n. :-)  How about:
> 
> config RELEASE
> 	bool "Build for release"
> 	help
> 	  If the kernel is declared as a release build here, then
> 	  various checks that are only of interest to kernel development
> 	  will be omitted.

Agreed :-)

> 
> c) A drawback of this general option is, it's hard to tell what will be
>    omitted in particular.

In that sense it is similar to EMBEDDED, but I still think there
is high value to this, I can already think of several things I
want to put under this which are only noise I want to see during
development periods.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC 1/4] CONFIG_STABLE: Define it

[Stefan Richter]

>>  menu "General setup"
>>  
>> +config STABLE
>> +	bool "Stable kernel"
[...]
> a) Why in Kconfig, why not in Makefile?
> 
> b) Of course nobody wants STABLE=n. :-)  How about:
> 
> config RELEASE
> 	bool "Build for release"
> 	help
> 	  If the kernel is declared as a release build here, then
> 	  various checks that are only of interest to kernel development
> 	  will be omitted.

PS:  Also, it could be reversed (e.g. config TESTBUILD) and put into the
Kernel Hacking submenu.

> c) A drawback of this general option is, it's hard to tell what will be
> omitted in particular.

-- 
Stefan Richter
-=====-=-=== -=-= =====
http://arcgraph.de/sr/

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC 2/4] CONFIG_STABLE: Switch off kmalloc(0) tests in slab allocators

[Zach Brown]

> +#ifndef CONFIG_STABLE
>  	/*
>  	 * We should return 0 if size == 0 (which would result in the
>  	 * kmalloc caller to get NULL) but we use the smallest object
> @@ -81,6 +82,7 @@ static inline int kmalloc_index(size_t s
>  	 * we can discover locations where we do 0 sized allocations.
>  	 */
>  	WARN_ON_ONCE(size == 0);
> +#endif

> +#ifndef CONFIG_STABLE
>  	WARN_ON_ONCE(size == 0);
> +#endif

I wonder if there wouldn't be value in making a WARN_*() variant that
contained the ifdef internally so we could lose these tedious
surrounding ifdefs in call sites.  WARN_DEVELOPER_WHEN(), or something.
I don't care what it's called.  

- z 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC 1/4] CONFIG_STABLE: Define it

[Andrew Morton]

On Wed, 30 May 2007 17:20:48 -0700
clameter@sgi.com wrote:

> Introduce CONFIG_STABLE to control checks only useful for development.
> 
> Signed-off-by: Christoph Lameter <clameter@sgi.com>
> 
> ---
>  init/Kconfig |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> Index: slub/init/Kconfig
> ===================================================================
> --- slub.orig/init/Kconfig	2007-05-30 16:35:05.000000000 -0700
> +++ slub/init/Kconfig	2007-05-30 16:35:45.000000000 -0700
> @@ -65,6 +65,13 @@ endmenu
>  
>  menu "General setup"
>  
> +config STABLE
> +	bool "Stable kernel"
> +	help
> +	  If the kernel is configured to be a stable kernel then various
> +	  checks that are only of interest to kernel development will be
> +	  omitted.
> +
>  config LOCALVERSION
>  	string "Local version - append to kernel release"
>  	help

OK, but I think it'd be better if this knob was at line 6 of ./Makefile so
that Linus remembers to turn it on and off at appropriate times.  Also, I
suspect that we want it available within Kconfig expressions as well as
within cpp expressions.

So something like this:

diff -puN Makefile~a Makefile
--- a/Makefile~a
+++ a/Makefile
@@ -3,6 +3,7 @@ PATCHLEVEL = 6
 SUBLEVEL = 22
 EXTRAVERSION = -rc3
 NAME = Jeff Thinks I Should Change This, But To What?
+DEVEL_KERNEL = 1
 
 # *DOCUMENTATION*
 # To see a list of typical targets execute "make help"
@@ -320,7 +321,7 @@ AFLAGS          := -D__ASSEMBLY__
 KERNELRELEASE = $(shell cat include/config/kernel.release 2> /dev/null)
 KERNELVERSION = $(VERSION).$(PATCHLEVEL).$(SUBLEVEL)$(EXTRAVERSION)
 
-export VERSION PATCHLEVEL SUBLEVEL KERNELRELEASE KERNELVERSION
+export VERSION PATCHLEVEL SUBLEVEL KERNELRELEASE KERNELVERSION DEVEL_KERNEL
 export ARCH CONFIG_SHELL HOSTCC HOSTCFLAGS CROSS_COMPILE AS LD CC
 export CPP AR NM STRIP OBJCOPY OBJDUMP MAKE AWK GENKSYMS PERL UTS_MACHINE
 export HOSTCXX HOSTCXXFLAGS LDFLAGS_MODULE CHECK CHECKFLAGS
diff -puN scripts/kconfig/symbol.c~a scripts/kconfig/symbol.c
--- a/scripts/kconfig/symbol.c~a
+++ a/scripts/kconfig/symbol.c
@@ -68,6 +68,15 @@ void sym_init(void)
 	if (p)
 		sym_add_default(sym, p);
 
+	sym = sym_lookup("DEVEL_KERNEL", 0);
+	sym->type = S_BOOLEAN;
+	sym->flags |= SYMBOL_AUTO;
+	p = getenv("DEVEL_KERNEL");
+	if (p && atoi(p))
+		sym_add_default(sym, "y");
+	else
+		sym_add_default(sym, "n");
+
 	sym = sym_lookup("UNAME_RELEASE", 0);
 	sym->type = S_STRING;
 	sym->flags |= SYMBOL_AUTO;
_


With the following behaviour:

DEVEL_KERNEL = 0 in Makefile:

	DEVEL_KERNEL=n in Kconfig
	CONFIG_DEVEL_KERNEL is not set in cpp

DEVEL_KERNEL = 1 in Makefile:

	DEVEL_KERNEL=y in Kconfig
	CONFIG_DEVEL_KERNEL is set in cpp

however the above patch doesn't do this correctly and I got bored of
fiddling with it.  Help?
 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC 1/4] CONFIG_STABLE: Define it

[Christoph Lameter]

On Thu, 31 May 2007, Andrew Morton wrote:

> however the above patch doesn't do this correctly and I got bored of
> fiddling with it.  Help?

Will get something like what you described into the next release if 
no one else has anything to add.



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC 1/4] CONFIG_STABLE: Define it

[Sam Ravnborg]

> 
> So something like this:
> 
> diff -puN Makefile~a Makefile
> --- a/Makefile~a
> +++ a/Makefile
> @@ -3,6 +3,7 @@ PATCHLEVEL = 6
>  SUBLEVEL = 22
>  EXTRAVERSION = -rc3
>  NAME = Jeff Thinks I Should Change This, But To What?
> +DEVEL_KERNEL = 1

Could we name this: KERNELDEVEL to fit with current naming convention?
Alternative: KERNEL_DEVEL

Maybe a little comment that this is mirrored as a CONFIG_ symbol?

>  
>  # *DOCUMENTATION*
>  # To see a list of typical targets execute "make help"
> @@ -320,7 +321,7 @@ AFLAGS          := -D__ASSEMBLY__
>  KERNELRELEASE = $(shell cat include/config/kernel.release 2> /dev/null)
>  KERNELVERSION = $(VERSION).$(PATCHLEVEL).$(SUBLEVEL)$(EXTRAVERSION)
>  
> -export VERSION PATCHLEVEL SUBLEVEL KERNELRELEASE KERNELVERSION
> +export VERSION PATCHLEVEL SUBLEVEL KERNELRELEASE KERNELVERSION DEVEL_KERNEL
>  export ARCH CONFIG_SHELL HOSTCC HOSTCFLAGS CROSS_COMPILE AS LD CC
>  export CPP AR NM STRIP OBJCOPY OBJDUMP MAKE AWK GENKSYMS PERL UTS_MACHINE
>  export HOSTCXX HOSTCXXFLAGS LDFLAGS_MODULE CHECK CHECKFLAGS
> diff -puN scripts/kconfig/symbol.c~a scripts/kconfig/symbol.c
> --- a/scripts/kconfig/symbol.c~a
> +++ a/scripts/kconfig/symbol.c
> @@ -68,6 +68,15 @@ void sym_init(void)
>  	if (p)
>  		sym_add_default(sym, p);
>  
> +	sym = sym_lookup("DEVEL_KERNEL", 0);
> +	sym->type = S_BOOLEAN;
> +	sym->flags |= SYMBOL_AUTO;
> +	p = getenv("DEVEL_KERNEL");
> +	if (p && atoi(p))
> +		sym_add_default(sym, "y");
> +	else
> +		sym_add_default(sym, "n");
> +

		sym_set_tristate_value(sym, yes);
	else
		sym_set_tristate_value(sym, no);

should do the trick (untested).

	Sam

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC 2/4] CONFIG_STABLE: Switch off kmalloc(0) tests in slab allocators

[Andi Kleen]

Zach Brown <zach.brown@oracle.com> writes:

> > +#ifndef CONFIG_STABLE
> >  	/*
> >  	 * We should return 0 if size == 0 (which would result in the
> >  	 * kmalloc caller to get NULL) but we use the smallest object
> > @@ -81,6 +82,7 @@ static inline int kmalloc_index(size_t s
> >  	 * we can discover locations where we do 0 sized allocations.
> >  	 */
> >  	WARN_ON_ONCE(size == 0);
> > +#endif
> 
> > +#ifndef CONFIG_STABLE
> >  	WARN_ON_ONCE(size == 0);
> > +#endif
> 
> I wonder if there wouldn't be value in making a WARN_*() variant that
> contained the ifdef internally so we could lose these tedious
> surrounding ifdefs in call sites.  WARN_DEVELOPER_WHEN(), or something.
> I don't care what it's called.  

Networking has had NETDEBUG(codeblock) for this. Perhaps something
similar would be useful (DEVELOPMENT(codeblock)) in addition
to the special WARN/BUG_ONs

-Andi

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC 0/4] CONFIG_STABLE to switch off development checks

[Jeremy Fitzhardinge]

clameter@sgi.com wrote:
> A while back we talked about having the capability of switching off checks
> like the one for kmalloc(0) for stable kernel releases. This is a first stab
> at such functionality. It adds #ifdef CONFIG_STABLE for now. Maybe we can
> come up with some better way to handle it later. There should alsol be some
> way to set CONFIG_STABLE from the Makefile.
>
> CONFIG_STABLE switches off
>
> - kmalloc(0) check in both slab allocators
> - SLUB banner
> - Makes SLUB tolerate object corruption like SLAB (not sure if we really want
>   to go down this route. See patch)
>   

Perhaps I missed it, but what's the rationale for complaining about
0-sized allocations?  They seem like a perfectly reasonable thing to me;
they turn up at the boundary conditions of many algorithms, and avoiding
them just cruds up the callsites to make them go through hoops to avoid
allocation. 

Why not just do a 1 byte allocation instead, and be done with it?  Any
non-constant-sized allocation will potentially have to deal with this
case, so it seems to me we could just put the fix in common code (and
use an inline wrapper to avoid it when dealing with constant non-zero
sized allocations).

    J

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC 1/4] CONFIG_STABLE: Define it

[Dave Jones]

On Thu, May 31, 2007 at 11:30:46PM +0200, Sam Ravnborg wrote:
 > > +	sym = sym_lookup("DEVEL_KERNEL", 0);
 > > +	sym->type = S_BOOLEAN;
 > > +	sym->flags |= SYMBOL_AUTO;
 > > +	p = getenv("DEVEL_KERNEL");
 > > +	if (p && atoi(p))
 > > +		sym_add_default(sym, "y");
 > > +	else
 > > +		sym_add_default(sym, "n");
 > > +
 > 
 > 		sym_set_tristate_value(sym, yes);
 > 	else
 > 		sym_set_tristate_value(sym, no);
 > 
 > should do the trick (untested).

Odd. What's the third state ? Undefined?

	Dave

-- 
http://www.codemonkey.org.uk

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC 1/4] CONFIG_STABLE: Define it

[Dave Jones]

On Wed, May 30, 2007 at 05:49:56PM -0700, Christoph Lameter wrote:
 > On Thu, 31 May 2007, young dave wrote:
 > 
 > > Hi Christoph,
 > > 
 > > > Introduce CONFIG_STABLE to control checks only useful for development.
 > > 
 > > What about control checks only as SLUB_DEBUG is set?
 > 
 > Debug code is always included in all builds unless it is an embedded 
 > system. Debug code is kept out of the hot path.
 > 
 > Disabling SLUB_DEBUG should only be done for embedded systems. That is why 
 > the option is in CONFIG_EMBEDDED.

Something I'd really love to have is a CONFIG option to decide if
slub_debug is set or not by default.  The reasoning behind this is that during
development of each Fedora release, I used to leave SLAB_DEBUG=y for
months on end and catch all kinds of nasties.

Now that I've switched it over to using slub, I ended up adding the
ugly patch below, because otherwise, no-one would ever run with
slub_debug and we'd miss out on all those lovely bugs.
(I have 'make release' and 'make debug' targets which enable/disable
 this [and other] patches in the Fedora kernel).

(Patch for illustration only, obviously not for applying).

Unless someone beats me to it, I'll hack up a CONFIG option around
this. Having that turned on if !CONFIG_STABLE would also be a win I think.

	Dave


--- linux-2.6/mm/slub.c~	2007-05-27 21:48:42.000000000 -0400
+++ linux-2.6/mm/slub.c	2007-05-27 21:51:22.000000000 -0400
@@ -323,7 +323,7 @@ static inline int slab_index(void *p, st
 /*
  * Debug settings:
  */
-static int slub_debug;
+static int slub_debug = DEBUG_DEFAULT_FLAGS;
 
 static char *slub_debug_slabs;
 


-- 
http://www.codemonkey.org.uk

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC 1/4] CONFIG_STABLE: Define it

[Christoph Lameter]

On Fri, 1 Jun 2007, Dave Jones wrote:

>  > Disabling SLUB_DEBUG should only be done for embedded systems. That is why 
>  > the option is in CONFIG_EMBEDDED.
> 
> Something I'd really love to have is a CONFIG option to decide if
> slub_debug is set or not by default.  The reasoning behind this is that during
> development of each Fedora release, I used to leave SLAB_DEBUG=y for
> months on end and catch all kinds of nasties.

So slub_debug as a boot parameter is not enough.

> Now that I've switched it over to using slub, I ended up adding the
> ugly patch below, because otherwise, no-one would ever run with
> slub_debug and we'd miss out on all those lovely bugs.

Oh. No worry. By default slub puts its free pointer in the most dangerous 
area. In my experience it will bug immediately if there is something 
wrong. The mode of operations that I had in mind for development was to 
run until we crash somewhere. Then reboot with slub_debug to get the 
lovely report on who did it.

> (I have 'make release' and 'make debug' targets which enable/disable
>  this [and other] patches in the Fedora kernel).
> 
> (Patch for illustration only, obviously not for applying).

Hummm..... I need to think about this one.
 
> Unless someone beats me to it, I'll hack up a CONFIG option around
> this. Having that turned on if !CONFIG_STABLE would also be a win I think.

Doing so will impair performance testing. Memory use will be changed due 
to the growth of all the objects etc etc. Generally I think running 
with slub_debug by default is overkill. 

Having said that you can do even more if you would run

slabinfo -v

to validate object from cron. That way you can check up on all slab 
objects.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC 0/4] CONFIG_STABLE to switch off development checks

[Christoph Lameter]

On Fri, 1 Jun 2007, Jeremy Fitzhardinge wrote:

> Perhaps I missed it, but what's the rationale for complaining about
> 0-sized allocations?  They seem like a perfectly reasonable thing to me;
> they turn up at the boundary conditions of many algorithms, and avoiding
> them just cruds up the callsites to make them go through hoops to avoid
> allocation. 

Hmmm... We got there because SLUB initially return NULL for kmalloc(0). 
Rationale: The user did not request any memory so we wont give him 
any.

That (to my surprise) caused some strange behavior of code and so we then 
decided to keep SLAB behavior and return the smallest available object 
size and put a warning in there. At some later point we plan to switch
to returning NULL for kmalloc(0).
 
> Why not just do a 1 byte allocation instead, and be done with it?  Any
> non-constant-sized allocation will potentially have to deal with this
> case, so it seems to me we could just put the fix in common code (and
> use an inline wrapper to avoid it when dealing with constant non-zero
> sized allocations).

The smallest allocation that SLUB can do is 8 bytes (SLAB 32). We are
giving the user the smallest object that we can allocate. We could just 
remove the warning and would have the behavior that you want.

But now allocating code gets memory although none was requested. The 
object can be modified without us being able to check.

By returning NULL any use of the returned pointer would BUG.

An allocation of zero bytes usually indicates that the code is not dealing 
with a special case. Later code may operate on the allocated object. I 
think its clearer and cleaner if code would deal with that special case 
explicitly. We have seen a series of code pieces that do uncomfortably 
looking operations on structures with no objects.


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC 0/4] CONFIG_STABLE to switch off development checks

[Jeremy Fitzhardinge]

Christoph Lameter wrote:
> Hmmm... We got there because SLUB initially return NULL for kmalloc(0). 
> Rationale: The user did not request any memory so we wont give him 
> any.
>
> That (to my surprise) caused some strange behavior of code and so we then 
> decided to keep SLAB behavior and return the smallest available object 
> size and put a warning in there. At some later point we plan to switch
> to returning NULL for kmalloc(0).
>   

Unfortunately, returning NULL is indistinguishable from ENOMEM, so the
caller would have to check to see how much it asked for before deciding
to really fail, which doesn't help things much.

Or does it (should it) return ERRPTR(-ENOMEM)?  Bit of a major API
change if not.

>> Why not just do a 1 byte allocation instead, and be done with it?  Any
>> non-constant-sized allocation will potentially have to deal with this
>> case, so it seems to me we could just put the fix in common code (and
>> use an inline wrapper to avoid it when dealing with constant non-zero
>> sized allocations).
>>     
>
> The smallest allocation that SLUB can do is 8 bytes (SLAB 32). We are
> giving the user the smallest object that we can allocate. We could just 
> remove the warning and would have the behavior that you want.
>   

I think that's reasonable.  0-sized allocations seem to be relatively
rare anyway, so its not like we're going to be filling the heap with
these things.

I'm getting a couple of these warnings out of my code, but I've been
hesitating about fixing them because I can't see it resulting in any
improvement, either locally or globally - it would just be to shut the
warning up.

> But now allocating code gets memory although none was requested. The 
> object can be modified without us being able to check.
>   

Yes, that's a problem, but maybe heap debugging would help (ie, if
enabled make sure the "zero-sized allocation" pattern is undisturbed). 
Or return a pointer to a specific non-NULL unmapped address, though that
would need special handling on the free side.  Also, callers may get
upset if they get non-unique non-NULL addresses back from allocation.

> By returning NULL any use of the returned pointer would BUG.
>   

Well, actually it would stomp usermode memory, but we generally assume
there's nothing mapped at 0.  Or there are no bugs.

> An allocation of zero bytes usually indicates that the code is not dealing 
> with a special case. Later code may operate on the allocated object. I 
> think its clearer and cleaner if code would deal with that special case 
> explicitly. We have seen a series of code pieces that do uncomfortably 
> looking operations on structures with no objects.
>   

I disagree.  There are plenty of boundary conditions where 0 is not
really a special case, and making it a special case just complicates
things.  I think at least some of the patches posted to silence this
warning have been generally negative for code quality.  If we were
seeing lots of zero-sized allocations then that might indicate something
is amiss, but it seems to me that there's just a scattered handful.

I agree that it's always a useful debugging aid to make sure that
allocated regions are not over-run, but 0-sized allocations are not
special in this regard.

    J

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC 1/4] CONFIG_STABLE: Define it

[Sam Ravnborg]

On Fri, Jun 01, 2007 at 02:02:00PM -0400, Dave Jones wrote:
> On Thu, May 31, 2007 at 11:30:46PM +0200, Sam Ravnborg wrote:
>  > > +	sym = sym_lookup("DEVEL_KERNEL", 0);
>  > > +	sym->type = S_BOOLEAN;
>  > > +	sym->flags |= SYMBOL_AUTO;
>  > > +	p = getenv("DEVEL_KERNEL");
>  > > +	if (p && atoi(p))
>  > > +		sym_add_default(sym, "y");
>  > > +	else
>  > > +		sym_add_default(sym, "n");
>  > > +
>  > 
>  > 		sym_set_tristate_value(sym, yes);
>  > 	else
>  > 		sym_set_tristate_value(sym, no);
>  > 
>  > should do the trick (untested).
> 
> Odd. What's the third state ? Undefined?
no, mod, yes
Representing: no, module, yes as the three config choices.

	Sam

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC 1/4] CONFIG_STABLE: Define it

[Sam Ravnborg]

> 
> With the following behaviour:
> 
> DEVEL_KERNEL = 0 in Makefile:
> 
> 	DEVEL_KERNEL=n in Kconfig
> 	CONFIG_DEVEL_KERNEL is not set in cpp
> 
> DEVEL_KERNEL = 1 in Makefile:
> 
> 	DEVEL_KERNEL=y in Kconfig
> 	CONFIG_DEVEL_KERNEL is set in cpp
> 
> however the above patch doesn't do this correctly and I got bored of
> fiddling with it.  Help?

My first try below.
It does the kconfig stuff as expected.
But the CONFIG_KERNEL_DEVEL is NOT updated unless you
touch .config (or in other ways change the config).

I did not see an easy way to fix that - Roman?

I only had to add SYMBOL_VALID as falg to get it working - but it 
took me a while to figure out. Somehow all the comments describing the
data structures for kconfig has got lost.

	Sam

diff --git a/Makefile b/Makefile
index 562a909..362668c 100644
--- a/Makefile
+++ b/Makefile
@@ -3,6 +3,7 @@ PATCHLEVEL = 6
 SUBLEVEL = 22
 EXTRAVERSION = -rc3
 NAME = Jeff Thinks I Should Change This, But To What?
+KERNEL_DEVEL = 
 
 # *DOCUMENTATION*
 # To see a list of typical targets execute "make help"
@@ -320,7 +321,7 @@ AFLAGS          := -D__ASSEMBLY__
 KERNELRELEASE = $(shell cat include/config/kernel.release 2> /dev/null)
 KERNELVERSION = $(VERSION).$(PATCHLEVEL).$(SUBLEVEL)$(EXTRAVERSION)
 
-export VERSION PATCHLEVEL SUBLEVEL KERNELRELEASE KERNELVERSION
+export VERSION PATCHLEVEL SUBLEVEL KERNELRELEASE KERNELVERSION KERNEL_DEVEL
 export ARCH CONFIG_SHELL HOSTCC HOSTCFLAGS CROSS_COMPILE AS LD CC
 export CPP AR NM STRIP OBJCOPY OBJDUMP MAKE AWK GENKSYMS PERL UTS_MACHINE
 export HOSTCXX HOSTCXXFLAGS LDFLAGS_MODULE CHECK CHECKFLAGS
diff --git a/arch/i386/Kconfig b/arch/i386/Kconfig
index 8770a5d..5373d58 100644
--- a/arch/i386/Kconfig
+++ b/arch/i386/Kconfig
@@ -91,6 +91,14 @@ source "init/Kconfig"
 
 menu "Processor type and features"
 
+config MY_KERNEL_DEVEL
+	bool "Needs Kernel devel"
+	depends on KERNEL_DEVEL
+
+config MY_KERNEL_DEVEL2
+	bool "Do not need kernel devel"
+	depends on !KERNEL_DEVEL
+
 source "kernel/time/Kconfig"
 
 config SMP
diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
index c35dcc5..fb4d5b8 100644
--- a/scripts/kconfig/symbol.c
+++ b/scripts/kconfig/symbol.c
@@ -68,6 +68,15 @@ void sym_init(void)
 	if (p)
 		sym_add_default(sym, p);
 
+	sym = sym_lookup("KERNEL_DEVEL", 0);
+	sym->type = S_BOOLEAN;
+	sym->flags |= SYMBOL_VALID|SYMBOL_AUTO;
+	p = getenv("KERNEL_DEVEL");
+	if (p && atoi(p))
+		sym_add_default(sym, "y");
+	else
+		sym_add_default(sym, "n");
+
 	sym = sym_lookup("UNAME_RELEASE", 0);
 	sym->type = S_STRING;
 	sym->flags |= SYMBOL_AUTO;

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC 1/4] CONFIG_STABLE: Define it

[Dave Jones]

On Fri, Jun 01, 2007 at 10:22:02PM +0200, Sam Ravnborg wrote:
 > On Fri, Jun 01, 2007 at 02:02:00PM -0400, Dave Jones wrote:
 > > On Thu, May 31, 2007 at 11:30:46PM +0200, Sam Ravnborg wrote:
 > >  > > +	sym = sym_lookup("DEVEL_KERNEL", 0);
 > >  > > +	sym->type = S_BOOLEAN;
 > >  > > +	sym->flags |= SYMBOL_AUTO;
 > >  > > +	p = getenv("DEVEL_KERNEL");
 > >  > > +	if (p && atoi(p))
 > >  > > +		sym_add_default(sym, "y");
 > >  > > +	else
 > >  > > +		sym_add_default(sym, "n");
 > >  > > +
 > >  > 
 > >  > 		sym_set_tristate_value(sym, yes);
 > >  > 	else
 > >  > 		sym_set_tristate_value(sym, no);
 > >  > 
 > >  > should do the trick (untested).
 > > 
 > > Odd. What's the third state ? Undefined?
 > no, mod, yes
 > Representing: no, module, yes as the three config choices.

Now I'm even more puzzled.  Why would 'DEVEL_KERNEL' need
to be modular ?

		Dave

-- 
http://www.codemonkey.org.uk

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC 1/4] CONFIG_STABLE: Define it

[Sam Ravnborg]

On Fri, Jun 01, 2007 at 04:30:06PM -0400, Dave Jones wrote:
> On Fri, Jun 01, 2007 at 10:22:02PM +0200, Sam Ravnborg wrote:
>  > On Fri, Jun 01, 2007 at 02:02:00PM -0400, Dave Jones wrote:
>  > > On Thu, May 31, 2007 at 11:30:46PM +0200, Sam Ravnborg wrote:
>  > >  > > +	sym = sym_lookup("DEVEL_KERNEL", 0);
>  > >  > > +	sym->type = S_BOOLEAN;
>  > >  > > +	sym->flags |= SYMBOL_AUTO;
>  > >  > > +	p = getenv("DEVEL_KERNEL");
>  > >  > > +	if (p && atoi(p))
>  > >  > > +		sym_add_default(sym, "y");
>  > >  > > +	else
>  > >  > > +		sym_add_default(sym, "n");
>  > >  > > +
>  > >  > 
>  > >  > 		sym_set_tristate_value(sym, yes);
>  > >  > 	else
>  > >  > 		sym_set_tristate_value(sym, no);
>  > >  > 
>  > >  > should do the trick (untested).
>  > > 
>  > > Odd. What's the third state ? Undefined?
>  > no, mod, yes
>  > Representing: no, module, yes as the three config choices.
> 
> Now I'm even more puzzled.  Why would 'DEVEL_KERNEL' need
> to be modular ?
The same type is used to represent a boolean and a tristate
within kconfig:

typedef enum tristate {
	no, mod, yes
} tristate;

And in the cases where the config symbol is of type 'bool' then
the value 'mod' is not used.

	Sam

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC 0/4] CONFIG_STABLE to switch off development checks

[Christoph Lameter]

On Fri, 1 Jun 2007, Jeremy Fitzhardinge wrote:

> > An allocation of zero bytes usually indicates that the code is not dealing 
> > with a special case. Later code may operate on the allocated object. I 
> > think its clearer and cleaner if code would deal with that special case 
> > explicitly. We have seen a series of code pieces that do uncomfortably 
> > looking operations on structures with no objects.
> >   
> 
> I disagree.  There are plenty of boundary conditions where 0 is not
> really a special case, and making it a special case just complicates
> things.  I think at least some of the patches posted to silence this
> warning have been generally negative for code quality.  If we were
> seeing lots of zero-sized allocations then that might indicate something
> is amiss, but it seems to me that there's just a scattered handful.
> 
> I agree that it's always a useful debugging aid to make sure that
> allocated regions are not over-run, but 0-sized allocations are not
> special in this regard.

Still insisting on it even after the discovery of the cpuset kmalloc(0) issue?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC 0/4] CONFIG_STABLE to switch off development checks

[Jeremy Fitzhardinge]

Christoph Lameter wrote:
>> I disagree.  There are plenty of boundary conditions where 0 is not
>> really a special case, and making it a special case just complicates
>> things.  I think at least some of the patches posted to silence this
>> warning have been generally negative for code quality.  If we were
>> seeing lots of zero-sized allocations then that might indicate something
>> is amiss, but it seems to me that there's just a scattered handful.
>>
>> I agree that it's always a useful debugging aid to make sure that
>> allocated regions are not over-run, but 0-sized allocations are not
>> special in this regard.
>>     
>
> Still insisting on it even after the discovery of the cpuset kmalloc(0) issue?
>   

Sure. That was a normal buffer-overrun bug. There's nothing special
about 0-sized allocations.

J

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC 0/4] CONFIG_STABLE to switch off development checks

[Dave Kleikamp]

On Fri, 2007-06-01 at 11:58 -0700, Jeremy Fitzhardinge wrote:
> Christoph Lameter wrote:
> > Hmmm... We got there because SLUB initially return NULL for kmalloc(0). 
> > Rationale: The user did not request any memory so we wont give him 
> > any.
> >
> > That (to my surprise) caused some strange behavior of code and so we then 
> > decided to keep SLAB behavior and return the smallest available object 
> > size and put a warning in there. At some later point we plan to switch
> > to returning NULL for kmalloc(0).
> >   
> 
> Unfortunately, returning NULL is indistinguishable from ENOMEM, so the
> caller would have to check to see how much it asked for before deciding
> to really fail, which doesn't help things much.
> 
> Or does it (should it) return ERRPTR(-ENOMEM)?  Bit of a major API
> change if not.

I'm on Christoph's side here.  I don't think it makes sense for any code
to ask to allocate zero bytes of memory and expect valid memory to be
returned.

Would a compromise be to return a pointer to some known invalid region?
This way the kmalloc(0) call would appear successful to the caller, but
any access to the memory would result in an exception.

Just my 2 cents,
Shaggy
-- 
David Kleikamp
IBM Linux Technology Center

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC 0/4] CONFIG_STABLE to switch off development checks

[Jeremy Fitzhardinge]

Dave Kleikamp wrote:
> I'm on Christoph's side here.  I don't think it makes sense for any code
> to ask to allocate zero bytes of memory and expect valid memory to be
> returned.
>   

Yes, everyone agrees on that.  If you do kmalloc(0), its never OK to
dereference the result.  The question is whether kmalloc(0) should complain.

> Would a compromise be to return a pointer to some known invalid region?
> This way the kmalloc(0) call would appear successful to the caller, but
> any access to the memory would result in an exception.
>   

Yes, that's what Christoph has posted.  I'm slightly concerned about
kmalloc() returning the same non-NULL address multiple times, but it
seems sound otherwise.

    J

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC 0/4] CONFIG_STABLE to switch off development checks

[Dave Kleikamp]

On Sat, 2007-06-02 at 09:28 -0700, Jeremy Fitzhardinge wrote:
> Dave Kleikamp wrote:
> > I'm on Christoph's side here.  I don't think it makes sense for any code
> > to ask to allocate zero bytes of memory and expect valid memory to be
> > returned.
> >   
> 
> Yes, everyone agrees on that.  If you do kmalloc(0), its never OK to
> dereference the result.  The question is whether kmalloc(0) should complain.

Yeah, I see that you aren't necessarily asking for valid memory, just
something that appears valid.  I'm still of the mind that if code is
asking for a zero-length allocation, it's raising a flag that it's not
taking some corner case into account.  But I think I'm just
regurgitating what Christoph is arguing.

> > Would a compromise be to return a pointer to some known invalid region?
> > This way the kmalloc(0) call would appear successful to the caller, but
> > any access to the memory would result in an exception.
> >   
> 
> Yes, that's what Christoph has posted.

Oh.  I went back and re-read the thread and it looks like you proposed
this already.  I don't see where Christoph did, or agreed, but maybe I
missed something.

> I'm slightly concerned about
> kmalloc() returning the same non-NULL address multiple times, but it
> seems sound otherwise.

If the caller is asking for 0 bytes, it shouldn't be doing anything with
the returned address except checking for a NULL return.  But then, it's
hard to predict everything that calling code might be doing, such as
allocating buffers and creating a hash based on their addresses.  Of
course, if there's code that would have a problem with it, I think it's
a further argument that it would be better off avoiding the calling
kmalloc(0) in the first place.

Shaggy
-- 
David Kleikamp
IBM Linux Technology Center

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC 1/4] CONFIG_STABLE: Define it

[Satyam Sharma]

[ Just cleaning up my inbox, and stumbled across this thread ... ]


On 5/31/07, clameter@sgi.com <clameter@sgi.com> wrote:
> Introduce CONFIG_STABLE to control checks only useful for development.
>
> Signed-off-by: Christoph Lameter <clameter@sgi.com>
> [...]
>  menu "General setup"
>
> +config STABLE
> +       bool "Stable kernel"
> +       help
> +         If the kernel is configured to be a stable kernel then various
> +         checks that are only of interest to kernel development will be
> +         omitted.
> +


"A programmer who uses assertions during testing and turns them off
during production is like a sailor who wears a life vest while drilling
on shore and takes it off at sea."
                                                - Tony Hoare


Probably you meant to turn off debug _output_ (and not _checks_)
with this config option? But we already have CONFIG_FOO_DEBUG_BAR
for those situations ...

Satyam

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC 1/4] CONFIG_STABLE: Define it

[Chris Snook]

Satyam Sharma wrote:
> [ Just cleaning up my inbox, and stumbled across this thread ... ]
> 
> 
> On 5/31/07, clameter@sgi.com <clameter@sgi.com> wrote:
>> Introduce CONFIG_STABLE to control checks only useful for development.
>>
>> Signed-off-by: Christoph Lameter <clameter@sgi.com>
>> [...]
>>  menu "General setup"
>>
>> +config STABLE
>> +       bool "Stable kernel"
>> +       help
>> +         If the kernel is configured to be a stable kernel then various
>> +         checks that are only of interest to kernel development will be
>> +         omitted.
>> +
> 
> 
> "A programmer who uses assertions during testing and turns them off
> during production is like a sailor who wears a life vest while drilling
> on shore and takes it off at sea."
>                                                - Tony Hoare
> 
> 
> Probably you meant to turn off debug _output_ (and not _checks_)
> with this config option? But we already have CONFIG_FOO_DEBUG_BAR
> for those situations ...

There are plenty of validation and debugging features in the kernel that go WAY 
beyond mere assertions, often imposing significant overhead (particularly when 
you scale up) or creating interfaces you'd never use unless you were doing 
kernel development work.  You really do want these features completely removed 
from production kernels.

The point of this is not to remove one-line WARN_ON and BUG_ON checks (though we 
might remove a few from fast paths), but rather to disable big chunks of 
debugging code that don't implement anything visible to a production workload.

	-- Chris

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC 1/4] CONFIG_STABLE: Define it

[Satyam Sharma]

On 7/20/07, Chris Snook <csnook@redhat.com> wrote:
> Satyam Sharma wrote:
> > [ Just cleaning up my inbox, and stumbled across this thread ... ]
> >
> >
> > On 5/31/07, clameter@sgi.com <clameter@sgi.com> wrote:
> >> Introduce CONFIG_STABLE to control checks only useful for development.
> >>
> >> Signed-off-by: Christoph Lameter <clameter@sgi.com>
> >> [...]
> >>  menu "General setup"
> >>
> >> +config STABLE
> >> +       bool "Stable kernel"
> >> +       help
> >> +         If the kernel is configured to be a stable kernel then various
> >> +         checks that are only of interest to kernel development will be
> >> +         omitted.
> >> +
> >
> >
> > "A programmer who uses assertions during testing and turns them off
> > during production is like a sailor who wears a life vest while drilling
> > on shore and takes it off at sea."
> >                                                - Tony Hoare
> >
> >
> > Probably you meant to turn off debug _output_ (and not _checks_)
> > with this config option? But we already have CONFIG_FOO_DEBUG_BAR
> > for those situations ...
>
> There are plenty of validation and debugging features in the kernel that go WAY
> beyond mere assertions, often imposing significant overhead (particularly when
> you scale up) or creating interfaces you'd never use unless you were doing
> kernel development work.  You really do want these features completely removed
> from production kernels.

As for entire such "development/debugging-related features", most (all, really)
should anyway have their own config options.

> The point of this is not to remove one-line WARN_ON and BUG_ON checks (though we
> might remove a few from fast paths), but rather to disable big chunks of
> debugging code that don't implement anything visible to a production workload.

Oh yes, but it's still not clear to me why or how a kernel-wide "CONFIG_STABLE"
or "CONFIG_RELEASE" would help ... what's wrong with finer granularity
"CONFIG_xxx_DEBUG_xxx" kind of knobs?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC 1/4] CONFIG_STABLE: Define it

[Chris Snook]

Satyam Sharma wrote:
> On 7/20/07, Chris Snook <csnook@redhat.com> wrote:
>> Satyam Sharma wrote:
>> > [ Just cleaning up my inbox, and stumbled across this thread ... ]
>> >
>> >
>> > On 5/31/07, clameter@sgi.com <clameter@sgi.com> wrote:
>> >> Introduce CONFIG_STABLE to control checks only useful for development.
>> >>
>> >> Signed-off-by: Christoph Lameter <clameter@sgi.com>
>> >> [...]
>> >>  menu "General setup"
>> >>
>> >> +config STABLE
>> >> +       bool "Stable kernel"
>> >> +       help
>> >> +         If the kernel is configured to be a stable kernel then 
>> various
>> >> +         checks that are only of interest to kernel development 
>> will be
>> >> +         omitted.
>> >> +
>> >
>> >
>> > "A programmer who uses assertions during testing and turns them off
>> > during production is like a sailor who wears a life vest while drilling
>> > on shore and takes it off at sea."
>> >                                                - Tony Hoare
>> >
>> >
>> > Probably you meant to turn off debug _output_ (and not _checks_)
>> > with this config option? But we already have CONFIG_FOO_DEBUG_BAR
>> > for those situations ...
>>
>> There are plenty of validation and debugging features in the kernel 
>> that go WAY
>> beyond mere assertions, often imposing significant overhead 
>> (particularly when
>> you scale up) or creating interfaces you'd never use unless you were 
>> doing
>> kernel development work.  You really do want these features completely 
>> removed
>> from production kernels.
> 
> As for entire such "development/debugging-related features", most (all, 
> really)
> should anyway have their own config options.

They do.  With kconfig dependencies, we can ensure that those config options are 
off when CONFIG_STABLE is set.  That way you only have to set one option to 
ensure that all these expensive checks are disabled.

>> The point of this is not to remove one-line WARN_ON and BUG_ON checks 
>> (though we
>> might remove a few from fast paths), but rather to disable big chunks of
>> debugging code that don't implement anything visible to a production 
>> workload.
> 
> Oh yes, but it's still not clear to me why or how a kernel-wide 
> "CONFIG_STABLE"
> or "CONFIG_RELEASE" would help ... what's wrong with finer granularity
> "CONFIG_xxx_DEBUG_xxx" kind of knobs?

With kconfig dependencies, we can keep the fine granularity, but not have to 
spend a half hour digging through the configuration to make sure we have a 
production-suitable kernel.

	-- Chris

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC 1/4] CONFIG_STABLE: Define it

[Satyam Sharma]

On 7/20/07, Chris Snook <csnook@redhat.com> wrote:
> Satyam Sharma wrote:
> > On 7/20/07, Chris Snook <csnook@redhat.com> wrote:
> >> Satyam Sharma wrote:
> >> > [ Just cleaning up my inbox, and stumbled across this thread ... ]
> >> >
> >> >
> >> > On 5/31/07, clameter@sgi.com <clameter@sgi.com> wrote:
> >> >> Introduce CONFIG_STABLE to control checks only useful for development.
> >> >>
> >> >> Signed-off-by: Christoph Lameter <clameter@sgi.com>
> >> >> [...]
> >> >>  menu "General setup"
> >> >>
> >> >> +config STABLE
> >> >> +       bool "Stable kernel"
> >> >> +       help
> >> >> +         If the kernel is configured to be a stable kernel then
> >> various
> >> >> +         checks that are only of interest to kernel development
> >> will be
> >> >> +         omitted.
> >> >> +
> >> >
> >> >
> >> > "A programmer who uses assertions during testing and turns them off
> >> > during production is like a sailor who wears a life vest while drilling
> >> > on shore and takes it off at sea."
> >> >                                                - Tony Hoare
> >> >
> >> >
> >> > Probably you meant to turn off debug _output_ (and not _checks_)
> >> > with this config option? But we already have CONFIG_FOO_DEBUG_BAR
> >> > for those situations ...
> >>
> >> There are plenty of validation and debugging features in the kernel
> >> that go WAY
> >> beyond mere assertions, often imposing significant overhead
> >> (particularly when
> >> you scale up) or creating interfaces you'd never use unless you were
> >> doing
> >> kernel development work.  You really do want these features completely
> >> removed
> >> from production kernels.
> >
> > As for entire such "development/debugging-related features", most (all,
> > really)
> > should anyway have their own config options.
>
> They do.  With kconfig dependencies, we can ensure that those config options are
> off when CONFIG_STABLE is set.  That way you only have to set one option to
> ensure that all these expensive checks are disabled.

Oh, so you mean use this (the negation of this, actually) as a universal
kconfig dependency of all other such development/debugging related stuff?
Hmm, the name is quite misleading in that case.

Anyway, what surprised me was 4/4 in this patchset. Funny that we wouldn't
want to corrupt memory / trash hard disks / follow invalid pointers on a
developers testbox, but (knowingly) want to do that on a production website
running Google.com's website :-)

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC 1/4] CONFIG_STABLE: Define it

[Chris Snook]

Satyam Sharma wrote:
> On 7/20/07, Chris Snook <csnook@redhat.com> wrote:
>> Satyam Sharma wrote:
>> > On 7/20/07, Chris Snook <csnook@redhat.com> wrote:
>> >> Satyam Sharma wrote:
>> >> > [ Just cleaning up my inbox, and stumbled across this thread ... ]
>> >> >
>> >> >
>> >> > On 5/31/07, clameter@sgi.com <clameter@sgi.com> wrote:
>> >> >> Introduce CONFIG_STABLE to control checks only useful for 
>> development.
>> >> >>
>> >> >> Signed-off-by: Christoph Lameter <clameter@sgi.com>
>> >> >> [...]
>> >> >>  menu "General setup"
>> >> >>
>> >> >> +config STABLE
>> >> >> +       bool "Stable kernel"
>> >> >> +       help
>> >> >> +         If the kernel is configured to be a stable kernel then
>> >> various
>> >> >> +         checks that are only of interest to kernel development
>> >> will be
>> >> >> +         omitted.
>> >> >> +
>> >> >
>> >> >
>> >> > "A programmer who uses assertions during testing and turns them off
>> >> > during production is like a sailor who wears a life vest while 
>> drilling
>> >> > on shore and takes it off at sea."
>> >> >                                                - Tony Hoare
>> >> >
>> >> >
>> >> > Probably you meant to turn off debug _output_ (and not _checks_)
>> >> > with this config option? But we already have CONFIG_FOO_DEBUG_BAR
>> >> > for those situations ...
>> >>
>> >> There are plenty of validation and debugging features in the kernel
>> >> that go WAY
>> >> beyond mere assertions, often imposing significant overhead
>> >> (particularly when
>> >> you scale up) or creating interfaces you'd never use unless you were
>> >> doing
>> >> kernel development work.  You really do want these features completely
>> >> removed
>> >> from production kernels.
>> >
>> > As for entire such "development/debugging-related features", most (all,
>> > really)
>> > should anyway have their own config options.
>>
>> They do.  With kconfig dependencies, we can ensure that those config 
>> options are
>> off when CONFIG_STABLE is set.  That way you only have to set one 
>> option to
>> ensure that all these expensive checks are disabled.
> 
> Oh, so you mean use this (the negation of this, actually) as a universal
> kconfig dependency of all other such development/debugging related stuff?
> Hmm, the name is quite misleading in that case.

There are many different ways you can use it.  If I'm writing a configurable 
feature, I could make it depend on !CONFIG_STABLE, or I could ifdef my code out 
if CONFIG_STABLE is set, unless a more granular option is also set.  The 
maintainer of the code that uses the config option has a lot of flexibility, at 
least until we start enforcing standards.

	-- Chris

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC 1/4] CONFIG_STABLE: Define it

[Stefan Richter]

(I missed the original post, hence am replying to te reply...)
> On 5/31/07, clameter@sgi.com <clameter@sgi.com> wrote:
>> Introduce CONFIG_STABLE to control checks only useful for development.
>>
>> Signed-off-by: Christoph Lameter <clameter@sgi.com>
>> [...]
>>  menu "General setup"
>>
>> +config STABLE
>> +       bool "Stable kernel"
>> +       help
>> +         If the kernel is configured to be a stable kernel then various
>> +         checks that are only of interest to kernel development will be
>> +         omitted.
>> +

Didn't we talk about the wording and the logic some time ago?  Your
option looks like a magic switch that suddenly improves kernel
stability, hence everyone will switch it on.

How about this:

config BUILD_FOR_RELEASE
	bool "Build for release"
	help
	  If the kernel is configured as a release build, various checks
	  that are only of interest to kernel development will be
	  omitted.

	  If unsure, say Y.

Or this:

config BUILD_FOR_TESTING
	bool "Build for testing"
	help
	  If the kernel is configured as a test build, various checks
	  useful for testing of pre-releases will be activated.

	  If unsure, say N.
-- 
Stefan Richter
-=====-=-=== -=== =-=--
http://arcgraph.de/sr/

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC 1/4] CONFIG_STABLE: Define it

[Stefan Richter]

>>> +config STABLE
>>> +       bool "Stable kernel"

PS:  Imagine the headlines at Slashdot, CNET et al when this gets in.
-- 
Stefan Richter
-=====-=-=== -=== =-=--
http://arcgraph.de/sr/

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC 1/4] CONFIG_STABLE: Define it

[Stefan Richter]

Chris Snook wrote:
> There are many different ways you can use it.  If I'm writing a configurable 
> feature, I could make it depend on !CONFIG_STABLE, or I could ifdef my code out 
> if CONFIG_STABLE is set, unless a more granular option is also set.  The 
> maintainer of the code that uses the config option has a lot of flexibility,

In other words, nobody will ever know what this config option really does.

> at least until we start enforcing standards.
-- 
Stefan Richter
-=====-=-=== -=== =-=--
http://arcgraph.de/sr/

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC 1/4] CONFIG_STABLE: Define it

[Chuck Ebbert]

On 07/20/2007 12:36 PM, Stefan Richter wrote:
>>>> +config STABLE
>>>> +       bool "Stable kernel"
> 
> PS:  Imagine the headlines at Slashdot, CNET et al when this gets in.

I think this really should be called CONFIG_EXTRA_CHECKS.
It's a lot clearer what that means...

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC 0/4] CONFIG_STABLE to switch off development checks

[Robert Hancock]

Dave Kleikamp wrote:
> I'm on Christoph's side here.  I don't think it makes sense for any code
> to ask to allocate zero bytes of memory and expect valid memory to be
> returned.
> 
> Would a compromise be to return a pointer to some known invalid region?
> This way the kmalloc(0) call would appear successful to the caller, but
> any access to the memory would result in an exception.

I would think returning 1 as the address would work here, it's not NULL 
but any access to that page should still oops..

-- 
Robert Hancock      Saskatoon, SK, Canada
To email, remove "nospam" from hancockr@nospamshaw.ca
Home Page: http://www.roberthancock.com/

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>