CONFIG_STABLE to switch off development checks

Introduce CONFIG_STABLE to control checks only useful for development.

Hi Christoph,

Quoted 1 line

Introduce CONFIG_STABLE to control checks only useful for development.

What about control checks only as SLUB_DEBUG is set?

Quoted 4 lines · young dave

Hi Christoph,

Quoted 1 line

Introduce CONFIG_STABLE to control checks only useful for development.

Quoted 1 line

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.

Quoted 2 lines

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.

init/Kconfig +0 -0

@@ -65,6 +65,13 @@ endmenu
 
 menu "General setup"
 

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.

Quoted 4 lines

menu "General setup"

init/Kconfig +2 -0

+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.

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

Introduce CONFIG_STABLE to control checks only useful for development.

Signed-off-by Christoph Lameter <[email protected]>

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

Index: slub/init/Kconfig
===================================================================

init/Kconfig +0 -0

@@ -65,6 +65,13 @@ endmenu
 
 menu "General setup"
 

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

Makefile renamed from Makefile~a +1 -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

scripts/kconfig/symbol.c renamed from scripts/kconfig/symbol.c~a +0 -0

@@ -68,6 +68,15 @@ void sym_init(void)
 	if (p)
 		sym_add_default(sym, p);

scripts/kconfig/symbol.c void sym_init(void) · line 79

Quoted 9 lines

scripts/kconfig/symbol.c renamed from scripts/kconfig/symbol.c~a +9 -0

+	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).

Quoted 14 lines · Sam Ravnborg

scripts/kconfig/symbol.c void sym_init(void) · line 79

Quoted 9 lines

scripts/kconfig/symbol.c renamed from scripts/kconfig/symbol.c~a +9 -0

+	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);

Quoted 1 line

should do the trick (untested).

Odd. What's the third state ? Undefined?

Quoted 15 lines · Dave Jones

Quoted 14 lines · Sam Ravnborg

scripts/kconfig/symbol.c void sym_init(void) · line 79

Quoted 9 lines

scripts/kconfig/symbol.c renamed from scripts/kconfig/symbol.c~a +9 -0

+	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);

Quoted 3 lines

Quoted 1 line

should do the trick (untested).

Odd. What's the third state ? Undefined?

no, mod, yes
Representing: no, module, yes as the three config choices.

Quoted 16 lines · Sam Ravnborg

Quoted 15 lines · Dave Jones

Quoted 14 lines · Sam Ravnborg

scripts/kconfig/symbol.c void sym_init(void) · line 79

Quoted 9 lines

scripts/kconfig/symbol.c renamed from scripts/kconfig/symbol.c~a +9 -0

+	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);

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

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 ?

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?

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

init/Kconfig endmenu · line 74

Quoted 13 lines · On 5/31/07, [email protected] <[email protected]> wrote:

Introduce CONFIG_STABLE to control checks only useful for development.

Signed-off-by Christoph Lameter <[email protected]>

[...]

menu "General setup"

init/Kconfig +7 -0

+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 ...

Quoted 25 lines · Satyam Sharma

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

init/Kconfig endmenu · line 74

Quoted 13 lines · On 5/31/07, [email protected] <[email protected]> wrote:

Introduce CONFIG_STABLE to control checks only useful for development.

Signed-off-by Christoph Lameter <[email protected]>

[...]

menu "General setup"

init/Kconfig +7 -0

+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

Quoted 3 lines

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.

Quoted 32 lines · Chris Snook

Quoted 14 lines · Satyam Sharma

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

Quoted 10 lines · On 5/31/07, [email protected] <[email protected]> wrote:

Introduce CONFIG_STABLE to control checks only useful for development.

Signed-off-by Christoph Lameter <[email protected]>

[...]

menu "General setup"

init/Kconfig +4 -0

+config STABLE
+       bool "Stable kernel"
+       help
+         If the kernel is configured to be a stable kernel then 

various

Quoted 1 line

Quoted 1 line

+ checks that are only of interest to kernel development

will be

Quoted 13 lines

init/Kconfig endmenu · line 74

Quoted 2 lines

init/Kconfig +2 -0

+         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 ...

Quoted 5 lines

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.

Quoted 3 lines

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?

Quoted 43 lines · Satyam Sharma

Quoted 41 lines · Chris Snook

Quoted 14 lines · Satyam Sharma

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

Quoted 10 lines · On 5/31/07, [email protected] <[email protected]> wrote:

Introduce CONFIG_STABLE to control checks only useful for development.

Signed-off-by Christoph Lameter <[email protected]>

[...]

menu "General setup"

init/Kconfig +4 -0

+config STABLE
+       bool "Stable kernel"
+       help
+         If the kernel is configured to be a stable kernel then

various

Quoted 1 line

Quoted 1 line

+ checks that are only of interest to kernel development

will be

Quoted 10 lines

init/Kconfig endmenu · line 74

Quoted 2 lines

init/Kconfig +2 -0

+         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

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 50 lines · Chris Snook

Quoted 7 lines · Satyam Sharma

Quoted 6 lines · Chris Snook

Quoted 5 lines · Satyam Sharma

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

Quoted 1 line · On 5/31/07, [email protected] <[email protected]> wrote:

Introduce CONFIG_STABLE to control checks only useful for

development.

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

init/Kconfig endmenu

Quoted 9 lines

Signed-off-by Christoph Lameter <[email protected]>

[...]

menu "General setup"

init/Kconfig +4 -0

+config STABLE
+       bool "Stable kernel"
+       help
+         If the kernel is configured to be a stable kernel then

various

Quoted 8 lines

Quoted 8 lines

Quoted 1 line

Quoted 1 line

+ checks that are only of interest to kernel development

will be

Quoted 6 lines

init/Kconfig endmenu · line 74

Quoted 2 lines

init/Kconfig +2 -0

+         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

Quoted 21 lines

Quoted 8 lines

Quoted 7 lines

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 ...

Quoted 9 lines

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.

Quoted 3 lines

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.

Quoted 6 lines

Introduce CONFIG_STABLE to control checks only useful for development.

Signed-off-by Christoph Lameter <[email protected]>

[...]

menu "General setup"

init/Kconfig endmenu · line 74

Quoted 7 lines

init/Kconfig +7 -0

+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.
+

Quoted 2 lines

init/Kconfig endmenu · line 69

Quoted 2 lines

init/Kconfig +2 -0

+config STABLE
+       bool "Stable kernel"

Quoted 2 lines

Quoted 2 lines

init/Kconfig endmenu · line 69

Quoted 2 lines

init/Kconfig +2 -0

+config STABLE
+       bool "Stable kernel"

PS: Imagine the headlines at Slashdot, CNET et al when this gets in.

Quoted 9 lines

include/linux/slub_def.h +2 -0

+#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

mm/slab.c static inline struct kmem_cache *__find_ · line 779

Quoted 3 lines

mm/slab.c +2 -0

+#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.

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).

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).

Quoted 6 lines

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.

Quoted 6 lines

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.

Quoted 7 lines

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.

Quoted 4 lines

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?

Quoted 9 lines · Christoph Lameter

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.

Quoted 4 lines · Dave Kleikamp

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.

Quoted 4 lines

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.