* Please revert "debug: Make CONFIG_EXPERT select CONFIG_DEBUG_KERNEL to unhide debug options" @ 2011-09-30 13:42 Adrian Bunk 2011-09-30 15:05 ` Josh Triplett 0 siblings, 1 reply; 17+ messages in thread From: Adrian Bunk @ 2011-09-30 13:42 UTC (permalink / raw) To: Linus Torvalds Cc: linux-kernel, Josh Triplett, Frederic Weisbecker, Sam Ravnborg I was just wondering why I was asked about all the debug options when I tried 3.1-rc8, and that was due to commit f505c553 (debug: Make CONFIG_EXPERT select CONFIG_DEBUG_KERNEL to unhide debug options), the full commit is below. It is wrong, and the author does not seem to understand how Kconfig works. The commit message is: Several debugging options currently default to y, such as CONFIG_DEBUG_BUGVERBOSE and CONFIG_DEBUG_RODATA. Embedded users might want to turn those options off to save space; however, turning them off requires turning on CONFIG_DEBUG_KERNEL to unhide them. Since CONFIG_DEBUG_KERNEL exists specifically to unhide debugging options, and CONFIG_EXPERT exists specifically to unhide options potentially needed by experts and/or embedded users, make CONFIG_EXPERT automatically imply CONFIG_DEBUG_KERNEL. Let me point at the obvious fact that both CONFIG_DEBUG_BUGVERBOSE and CONFIG_DEBUG_RODATA do depend on DEBUG_KERNEL, and are contrary to the claim of the author of this patch never enabled with CONFIG_DEBUG_KERNEL=n. [1] Linus, please revert this commit. Thanks Adrian [1] Select abuse would be an exception, but that doesn't seem to be the case for these options. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Please revert "debug: Make CONFIG_EXPERT select CONFIG_DEBUG_KERNEL to unhide debug options" 2011-09-30 13:42 Please revert "debug: Make CONFIG_EXPERT select CONFIG_DEBUG_KERNEL to unhide debug options" Adrian Bunk @ 2011-09-30 15:05 ` Josh Triplett 2011-09-30 15:25 ` Frederic Weisbecker 2011-09-30 15:50 ` Adrian Bunk 0 siblings, 2 replies; 17+ messages in thread From: Josh Triplett @ 2011-09-30 15:05 UTC (permalink / raw) To: Adrian Bunk Cc: Linus Torvalds, linux-kernel, Frederic Weisbecker, Sam Ravnborg On Fri, Sep 30, 2011 at 04:42:45PM +0300, Adrian Bunk wrote: > I was just wondering why I was asked about all the debug options when I > tried 3.1-rc8, and that was due to commit f505c553 (debug: Make > CONFIG_EXPERT select CONFIG_DEBUG_KERNEL to unhide debug options), > the full commit is below. > > It is wrong, and the author does not seem to understand how Kconfig works. > > The commit message is: > > Several debugging options currently default to y, such as > CONFIG_DEBUG_BUGVERBOSE and CONFIG_DEBUG_RODATA. Embedded users > might want to turn those options off to save space; however, > turning them off requires turning on CONFIG_DEBUG_KERNEL to > unhide them. Since CONFIG_DEBUG_KERNEL exists specifically to > unhide debugging options, and CONFIG_EXPERT exists specifically > to unhide options potentially needed by experts and/or embedded > users, make CONFIG_EXPERT automatically imply > CONFIG_DEBUG_KERNEL. > > > Let me point at the obvious fact that both CONFIG_DEBUG_BUGVERBOSE and > CONFIG_DEBUG_RODATA do depend on DEBUG_KERNEL, and are contrary to the > claim of the author of this patch never enabled with > CONFIG_DEBUG_KERNEL=n. [1] Not true: ~/src/linux-2.6$ rm .config ~/src/linux-2.6$ make allnoconfig HOSTCC scripts/basic/fixdep HOSTCC scripts/kconfig/conf.o HOSTCC scripts/kconfig/zconf.tab.o HOSTLD scripts/kconfig/conf scripts/kconfig/conf --allnoconfig Kconfig # # configuration written to .config # ~/src/linux-2.6$ grep BUGVERBOSE .config CONFIG_DEBUG_BUGVERBOSE=y ~/src/linux-2.6$ grep DEBUG_KERNEL .config # CONFIG_DEBUG_KERNEL is not set DEBUG_BUGVERBOSE does not depend on DEBUG_KERNEL; it just only shows up with DEBUG_KERNEL (and EXPERT) set. The *description* has a conditional on DEBUG_KERNEL and EXPERT: config DEBUG_BUGVERBOSE bool "Verbose BUG() reporting (adds 70K)" if DEBUG_KERNEL && EXPERT That doesn't mean the same thing as "depends on DEBUG_KERNEL". Also see Ingo Molnar's mail with Message-Id <20110605093445.GA19927@elte.hu>, which explicitly recommended this approach. - Josh Triplett ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Please revert "debug: Make CONFIG_EXPERT select CONFIG_DEBUG_KERNEL to unhide debug options" 2011-09-30 15:05 ` Josh Triplett @ 2011-09-30 15:25 ` Frederic Weisbecker 2011-09-30 15:32 ` Frederic Weisbecker 2011-09-30 15:54 ` Adrian Bunk 2011-09-30 15:50 ` Adrian Bunk 1 sibling, 2 replies; 17+ messages in thread From: Frederic Weisbecker @ 2011-09-30 15:25 UTC (permalink / raw) To: Josh Triplett; +Cc: Adrian Bunk, Linus Torvalds, linux-kernel, Sam Ravnborg On Fri, Sep 30, 2011 at 08:05:19AM -0700, Josh Triplett wrote: > On Fri, Sep 30, 2011 at 04:42:45PM +0300, Adrian Bunk wrote: > > I was just wondering why I was asked about all the debug options when I > > tried 3.1-rc8, and that was due to commit f505c553 (debug: Make > > CONFIG_EXPERT select CONFIG_DEBUG_KERNEL to unhide debug options), > > the full commit is below. > > > > It is wrong, and the author does not seem to understand how Kconfig works. > > > > The commit message is: > > > > Several debugging options currently default to y, such as > > CONFIG_DEBUG_BUGVERBOSE and CONFIG_DEBUG_RODATA. Embedded users > > might want to turn those options off to save space; however, > > turning them off requires turning on CONFIG_DEBUG_KERNEL to > > unhide them. Since CONFIG_DEBUG_KERNEL exists specifically to > > unhide debugging options, and CONFIG_EXPERT exists specifically > > to unhide options potentially needed by experts and/or embedded > > users, make CONFIG_EXPERT automatically imply > > CONFIG_DEBUG_KERNEL. > > > > > > Let me point at the obvious fact that both CONFIG_DEBUG_BUGVERBOSE and > > CONFIG_DEBUG_RODATA do depend on DEBUG_KERNEL, and are contrary to the > > claim of the author of this patch never enabled with > > CONFIG_DEBUG_KERNEL=n. [1] > > Not true: > > ~/src/linux-2.6$ rm .config > ~/src/linux-2.6$ make allnoconfig > HOSTCC scripts/basic/fixdep > HOSTCC scripts/kconfig/conf.o > HOSTCC scripts/kconfig/zconf.tab.o > HOSTLD scripts/kconfig/conf > scripts/kconfig/conf --allnoconfig Kconfig > # > # configuration written to .config > # > ~/src/linux-2.6$ grep BUGVERBOSE .config > CONFIG_DEBUG_BUGVERBOSE=y > ~/src/linux-2.6$ grep DEBUG_KERNEL .config > # CONFIG_DEBUG_KERNEL is not set > > DEBUG_BUGVERBOSE does not depend on DEBUG_KERNEL; it just only shows up > with DEBUG_KERNEL (and EXPERT) set. The *description* has a conditional > on DEBUG_KERNEL and EXPERT: > > config DEBUG_BUGVERBOSE > bool "Verbose BUG() reporting (adds 70K)" if DEBUG_KERNEL && EXPERT > > That doesn't mean the same thing as "depends on DEBUG_KERNEL". Indeed. But for DEBUG_RODATA it is the case. That said given the issue described with DEBUG_BUGVERSBOSE, it makes only the changelog buggy, not the patch. Also the patch was supposed to have a broader cleanup effect: https://lkml.org/lkml/2011/6/6/641 But we applied an earlier version by accident. All in one I think we chose a wrong way to fix the issue. It's annoying to have all the configs that are only visible with CONFIG_EXPERT spread all around in random config menu. Anything that has "if CONFIG_EXPERT" should probably be moved under the CONFIG_EXPERT menu so that it's visible and found right away. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Please revert "debug: Make CONFIG_EXPERT select CONFIG_DEBUG_KERNEL to unhide debug options" 2011-09-30 15:25 ` Frederic Weisbecker @ 2011-09-30 15:32 ` Frederic Weisbecker 2011-09-30 15:54 ` Adrian Bunk 1 sibling, 0 replies; 17+ messages in thread From: Frederic Weisbecker @ 2011-09-30 15:32 UTC (permalink / raw) To: Josh Triplett; +Cc: Adrian Bunk, Linus Torvalds, linux-kernel, Sam Ravnborg 2011/9/30 Frederic Weisbecker <fweisbec@gmail.com>: > On Fri, Sep 30, 2011 at 08:05:19AM -0700, Josh Triplett wrote: >> On Fri, Sep 30, 2011 at 04:42:45PM +0300, Adrian Bunk wrote: >> > I was just wondering why I was asked about all the debug options when I >> > tried 3.1-rc8, and that was due to commit f505c553 (debug: Make >> > CONFIG_EXPERT select CONFIG_DEBUG_KERNEL to unhide debug options), >> > the full commit is below. >> > >> > It is wrong, and the author does not seem to understand how Kconfig works. >> > >> > The commit message is: >> > >> > Several debugging options currently default to y, such as >> > CONFIG_DEBUG_BUGVERBOSE and CONFIG_DEBUG_RODATA. Embedded users >> > might want to turn those options off to save space; however, >> > turning them off requires turning on CONFIG_DEBUG_KERNEL to >> > unhide them. Since CONFIG_DEBUG_KERNEL exists specifically to >> > unhide debugging options, and CONFIG_EXPERT exists specifically >> > to unhide options potentially needed by experts and/or embedded >> > users, make CONFIG_EXPERT automatically imply >> > CONFIG_DEBUG_KERNEL. >> > >> > >> > Let me point at the obvious fact that both CONFIG_DEBUG_BUGVERBOSE and >> > CONFIG_DEBUG_RODATA do depend on DEBUG_KERNEL, and are contrary to the >> > claim of the author of this patch never enabled with >> > CONFIG_DEBUG_KERNEL=n. [1] >> >> Not true: >> >> ~/src/linux-2.6$ rm .config >> ~/src/linux-2.6$ make allnoconfig >> HOSTCC scripts/basic/fixdep >> HOSTCC scripts/kconfig/conf.o >> HOSTCC scripts/kconfig/zconf.tab.o >> HOSTLD scripts/kconfig/conf >> scripts/kconfig/conf --allnoconfig Kconfig >> # >> # configuration written to .config >> # >> ~/src/linux-2.6$ grep BUGVERBOSE .config >> CONFIG_DEBUG_BUGVERBOSE=y >> ~/src/linux-2.6$ grep DEBUG_KERNEL .config >> # CONFIG_DEBUG_KERNEL is not set >> >> DEBUG_BUGVERBOSE does not depend on DEBUG_KERNEL; it just only shows up >> with DEBUG_KERNEL (and EXPERT) set. The *description* has a conditional >> on DEBUG_KERNEL and EXPERT: >> >> config DEBUG_BUGVERBOSE >> bool "Verbose BUG() reporting (adds 70K)" if DEBUG_KERNEL && EXPERT >> >> That doesn't mean the same thing as "depends on DEBUG_KERNEL". > > Indeed. > > But for DEBUG_RODATA it is the case. That said given the issue described > with DEBUG_BUGVERSBOSE, it makes only the changelog buggy, not the patch. > > Also the patch was supposed to have a broader cleanup effect: > https://lkml.org/lkml/2011/6/6/641 > > But we applied an earlier version by accident. > > All in one I think we chose a wrong way to fix the issue. It's annoying to > have all the configs that are only visible with CONFIG_EXPERT spread all > around in random config menu. > > Anything that has "if CONFIG_EXPERT" should probably be moved under the CONFIG_EXPERT > menu so that it's visible and found right away. > But now I realize there are too many things all around that have "if EXPERT". So I guess we need to continue that way :) ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Please revert "debug: Make CONFIG_EXPERT select CONFIG_DEBUG_KERNEL to unhide debug options" 2011-09-30 15:25 ` Frederic Weisbecker 2011-09-30 15:32 ` Frederic Weisbecker @ 2011-09-30 15:54 ` Adrian Bunk 1 sibling, 0 replies; 17+ messages in thread From: Adrian Bunk @ 2011-09-30 15:54 UTC (permalink / raw) To: Frederic Weisbecker Cc: Josh Triplett, Linus Torvalds, linux-kernel, Sam Ravnborg, Ingo Molnar On Fri, Sep 30, 2011 at 05:25:23PM +0200, Frederic Weisbecker wrote: >... > All in one I think we chose a wrong way to fix the issue. It's annoying to > have all the configs that are only visible with CONFIG_EXPERT spread all > around in random config menu. > > Anything that has "if CONFIG_EXPERT" should probably be moved under the CONFIG_EXPERT > menu so that it's visible and found right away. No, it wouldn't be correct to have tons of options from different drivers smashed at one place, instead of keeping them at the subsystem where they belong. cu Adrian -- "Is there not promise of rain?" Ling Tan asked suddenly out of the darkness. There had been need of rain for many days. "Only a promise," Lao Er said. Pearl S. Buck - Dragon Seed ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Please revert "debug: Make CONFIG_EXPERT select CONFIG_DEBUG_KERNEL to unhide debug options" 2011-09-30 15:05 ` Josh Triplett 2011-09-30 15:25 ` Frederic Weisbecker @ 2011-09-30 15:50 ` Adrian Bunk 2011-10-10 7:29 ` Ingo Molnar 1 sibling, 1 reply; 17+ messages in thread From: Adrian Bunk @ 2011-09-30 15:50 UTC (permalink / raw) To: Josh Triplett Cc: Linus Torvalds, linux-kernel, Frederic Weisbecker, Sam Ravnborg, Ingo Molnar On Fri, Sep 30, 2011 at 08:05:19AM -0700, Josh Triplett wrote: > On Fri, Sep 30, 2011 at 04:42:45PM +0300, Adrian Bunk wrote: >... > > Let me point at the obvious fact that both CONFIG_DEBUG_BUGVERBOSE and > > CONFIG_DEBUG_RODATA do depend on DEBUG_KERNEL, and are contrary to the > > claim of the author of this patch never enabled with > > CONFIG_DEBUG_KERNEL=n. [1] > > Not true: > > ~/src/linux-2.6$ rm .config > ~/src/linux-2.6$ make allnoconfig > HOSTCC scripts/basic/fixdep > HOSTCC scripts/kconfig/conf.o > HOSTCC scripts/kconfig/zconf.tab.o > HOSTLD scripts/kconfig/conf > scripts/kconfig/conf --allnoconfig Kconfig > # > # configuration written to .config > # > ~/src/linux-2.6$ grep BUGVERBOSE .config > CONFIG_DEBUG_BUGVERBOSE=y > ~/src/linux-2.6$ grep DEBUG_KERNEL .config > # CONFIG_DEBUG_KERNEL is not set > > DEBUG_BUGVERBOSE does not depend on DEBUG_KERNEL; it just only shows up > with DEBUG_KERNEL (and EXPERT) set. The *description* has a conditional > on DEBUG_KERNEL and EXPERT: > > config DEBUG_BUGVERBOSE > bool "Verbose BUG() reporting (adds 70K)" if DEBUG_KERNEL && EXPERT >... Apologies, I was wrong on DEBUG_BUGVERBOSE (but for DEBUG_RODATA I was right). >... > Also see Ingo Molnar's mail with Message-Id > <20110605093445.GA19927@elte.hu>, which explicitly recommended this > approach. Ingo is an expert in a gazillion areas of the kernel and knows more about these than mere mortals like me will ever know. Kconfig is (the?) one thing where he in not a good advisor. Looking at the patch Frederic points at [1]: config DEBUG_BUGVERBOSE - bool "Verbose BUG() reporting (adds 70K)" if DEBUG_KERNEL && EXPERT + bool "Verbose BUG() reporting (adds 70K)" if EXPERT This part of the patch would have been the correct and complete solution for DEBUG_BUGVERBOSE. The crazy select added in commit f505c553 is wrong. > - Josh Triplett cu Adrian [1] http://lkml.org/lkml/2011/6/6/641 -- "Is there not promise of rain?" Ling Tan asked suddenly out of the darkness. There had been need of rain for many days. "Only a promise," Lao Er said. Pearl S. Buck - Dragon Seed ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Please revert "debug: Make CONFIG_EXPERT select CONFIG_DEBUG_KERNEL to unhide debug options" 2011-09-30 15:50 ` Adrian Bunk @ 2011-10-10 7:29 ` Ingo Molnar 2011-10-10 8:48 ` Adrian Bunk 0 siblings, 1 reply; 17+ messages in thread From: Ingo Molnar @ 2011-10-10 7:29 UTC (permalink / raw) To: Adrian Bunk Cc: Josh Triplett, Linus Torvalds, linux-kernel, Frederic Weisbecker, Sam Ravnborg * Adrian Bunk <bunk@stusta.de> wrote: > On Fri, Sep 30, 2011 at 08:05:19AM -0700, Josh Triplett wrote: > > On Fri, Sep 30, 2011 at 04:42:45PM +0300, Adrian Bunk wrote: > >... > > > Let me point at the obvious fact that both CONFIG_DEBUG_BUGVERBOSE and > > > CONFIG_DEBUG_RODATA do depend on DEBUG_KERNEL, and are contrary to the > > > claim of the author of this patch never enabled with > > > CONFIG_DEBUG_KERNEL=n. [1] > > > > Not true: > > > > ~/src/linux-2.6$ rm .config > > ~/src/linux-2.6$ make allnoconfig > > HOSTCC scripts/basic/fixdep > > HOSTCC scripts/kconfig/conf.o > > HOSTCC scripts/kconfig/zconf.tab.o > > HOSTLD scripts/kconfig/conf > > scripts/kconfig/conf --allnoconfig Kconfig > > # > > # configuration written to .config > > # > > ~/src/linux-2.6$ grep BUGVERBOSE .config > > CONFIG_DEBUG_BUGVERBOSE=y > > ~/src/linux-2.6$ grep DEBUG_KERNEL .config > > # CONFIG_DEBUG_KERNEL is not set > > > > DEBUG_BUGVERBOSE does not depend on DEBUG_KERNEL; it just only shows up > > with DEBUG_KERNEL (and EXPERT) set. The *description* has a conditional > > on DEBUG_KERNEL and EXPERT: > > > > config DEBUG_BUGVERBOSE > > bool "Verbose BUG() reporting (adds 70K)" if DEBUG_KERNEL && EXPERT > >... > > Apologies, I was wrong on DEBUG_BUGVERBOSE > (but for DEBUG_RODATA I was right). I think you are wrong not just about that detail but about the whole premise of your complaint as well: > config DEBUG_BUGVERBOSE > - bool "Verbose BUG() reporting (adds 70K)" if DEBUG_KERNEL && EXPERT > + bool "Verbose BUG() reporting (adds 70K)" if EXPERT > > This part of the patch would have been the correct and complete > solution for DEBUG_BUGVERBOSE. Not really - it's a debugging-only option and when i turn on CONFIG_DEBUG_KERNEL I expect to have full config control over all debug options - with sane defaults provided. I definitely don't want a debugging option to depend on CONFIG_EXPERT. CONFIG_EXPERT is a superset to all that: it implies config control over all debug options *and* over many other kernel components as well. This is a pretty easy model to think about. > The crazy select added in commit f505c553 is wrong. Why? Your original message does not explain it. Thanks, Ingo ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Please revert "debug: Make CONFIG_EXPERT select CONFIG_DEBUG_KERNEL to unhide debug options" 2011-10-10 7:29 ` Ingo Molnar @ 2011-10-10 8:48 ` Adrian Bunk 2011-10-10 9:44 ` Mike Galbraith 2011-10-10 10:21 ` Ingo Molnar 0 siblings, 2 replies; 17+ messages in thread From: Adrian Bunk @ 2011-10-10 8:48 UTC (permalink / raw) To: Ingo Molnar Cc: Josh Triplett, Linus Torvalds, linux-kernel, Frederic Weisbecker, Sam Ravnborg On Mon, Oct 10, 2011 at 09:29:48AM +0200, Ingo Molnar wrote: > * Adrian Bunk <bunk@stusta.de> wrote: >... > I think you are wrong not just about that detail but about the whole > premise of your complaint as well: > > > config DEBUG_BUGVERBOSE > > - bool "Verbose BUG() reporting (adds 70K)" if DEBUG_KERNEL && EXPERT > > + bool "Verbose BUG() reporting (adds 70K)" if EXPERT > > > > This part of the patch would have been the correct and complete > > solution for DEBUG_BUGVERBOSE. > > Not really - it's a debugging-only option and when i turn on > CONFIG_DEBUG_KERNEL I expect to have full config control over all > debug options - with sane defaults provided. Then you would have to remove the dependency on EXPERT from the prompt, and allow unsetting DEBUG_BUGVERBOSE with EXPERT=n, DEBUG_KERNEL=y. Note that this is completely unrelated to the commit we are discussing, since commit f505c553 has no effect in the EXPERT=n case you are discussing here. > I definitely don't want a debugging option to depend on > CONFIG_EXPERT. DEBUG_BUGVERBOSE does not depend on EXPERT. But EXPERT is currently required for disabling it. This part of our discussion boils down to the following options: 1. DEBUG_KERNEL allows a user to enable additional debugging 2. DEBUG_KERNEL gives a user full control over all debug options Traditionally it was 1., to tell a user reporting a bug that he should enable additional debugging in his kernel - without him accidentally disabling useful debugging. Again, this part of the discussion is unrelated to commit f505c553. > CONFIG_EXPERT is a superset to all that: it implies config control > over all debug options *and* over many other kernel components as > well. I don't see the advantage that EXPERT=y is now unconditionally enabling the DEBUG_KERNEL knob. When I am an "expert", why can't I still decide myself globally if I want to enable more debugging or not? Another problem is that we have code using "#ifdef CONFIG_DEBUG_KERNEL" so enabling EXPERT for making your kernel smaller can make your kernel bigger, but such usages should anyway get own config options. > This is a pretty easy model to think about. There are many options in the kernel that only enable seeing other config options. With that model, shouldn't EXPERT also have select MISC_FILESYSTEMS and many other similar select's? The point I am trying to make here is that there are many convenience config options that only exist to allow a user to hide a bunch of options he is not interested in, and DEBUG_KERNEL is one of them. [1] > > The crazy select added in commit f505c553 is wrong. > > Why? Your original message does not explain it. It is not needed for what it was claimed it was needed for, and it is a bad idea in general. If I didn't explain that properly that was my fault, I hope the explanation in this email is better. > Thanks, > > Ingo cu Adrian [1] ignoring the buggy code using "#ifdef CONFIG_DEBUG_KERNEL" -- "Is there not promise of rain?" Ling Tan asked suddenly out of the darkness. There had been need of rain for many days. "Only a promise," Lao Er said. Pearl S. Buck - Dragon Seed ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Please revert "debug: Make CONFIG_EXPERT select CONFIG_DEBUG_KERNEL to unhide debug options" 2011-10-10 8:48 ` Adrian Bunk @ 2011-10-10 9:44 ` Mike Galbraith 2011-10-10 10:21 ` Ingo Molnar 1 sibling, 0 replies; 17+ messages in thread From: Mike Galbraith @ 2011-10-10 9:44 UTC (permalink / raw) To: Adrian Bunk Cc: Ingo Molnar, Josh Triplett, Linus Torvalds, linux-kernel, Frederic Weisbecker, Sam Ravnborg On Mon, 2011-10-10 at 11:48 +0300, Adrian Bunk wrote: > DEBUG_BUGVERBOSE does not depend on EXPERT. > > But EXPERT is currently required for disabling it. Why would anyone other than poor embedded sods turn that off? -Mike ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Please revert "debug: Make CONFIG_EXPERT select CONFIG_DEBUG_KERNEL to unhide debug options" 2011-10-10 8:48 ` Adrian Bunk 2011-10-10 9:44 ` Mike Galbraith @ 2011-10-10 10:21 ` Ingo Molnar 2011-10-10 12:13 ` Adrian Bunk 1 sibling, 1 reply; 17+ messages in thread From: Ingo Molnar @ 2011-10-10 10:21 UTC (permalink / raw) To: Adrian Bunk, Arjan van de Ven Cc: Josh Triplett, Linus Torvalds, linux-kernel, Frederic Weisbecker, Sam Ravnborg * Adrian Bunk <bunk@stusta.de> wrote: > On Mon, Oct 10, 2011 at 09:29:48AM +0200, Ingo Molnar wrote: > > * Adrian Bunk <bunk@stusta.de> wrote: > >... > > I think you are wrong not just about that detail but about the whole > > premise of your complaint as well: > > > > > config DEBUG_BUGVERBOSE > > > - bool "Verbose BUG() reporting (adds 70K)" if DEBUG_KERNEL && EXPERT > > > + bool "Verbose BUG() reporting (adds 70K)" if EXPERT > > > > > > This part of the patch would have been the correct and complete > > > solution for DEBUG_BUGVERBOSE. > > > > Not really - it's a debugging-only option and when i turn on > > CONFIG_DEBUG_KERNEL I expect to have full config control over all > > debug options - with sane defaults provided. > > Then you would have to remove the dependency on EXPERT from the prompt, > and allow unsetting DEBUG_BUGVERBOSE with EXPERT=n, DEBUG_KERNEL=y. > > Note that this is completely unrelated to the commit we are discussing, > since commit f505c553 has no effect in the EXPERT=n case you are > discussing here. > > > I definitely don't want a debugging option to depend on > > CONFIG_EXPERT. > > DEBUG_BUGVERBOSE does not depend on EXPERT. > > But EXPERT is currently required for disabling it. Correct - that's a further variation. In the case of debug options that we *really* don't want normal users to disable we do something like this: config DEBUG_BUGVERBOSE bool "Verbose BUG() reporting (adds 70K)" if DEBUG_KERNEL && EXPERT Commit f505c553 ("debug: Make CONFIG_EXPERT select CONFIG_DEBUG_KERNEL to unhide debug options") allows this line to be further simplified into: bool "Verbose BUG() reporting (adds 70K)" if EXPERT ... but this was not the main purpose of the commit - nor is this simplification strictly necessary. > This part of our discussion boils down to the following options: > 1. DEBUG_KERNEL allows a user to enable additional debugging > 2. DEBUG_KERNEL gives a user full control over all debug options > > Traditionally it was 1., to tell a user reporting a bug that he should > enable additional debugging in his kernel - without him accidentally > disabling useful debugging. > > Again, this part of the discussion is unrelated to commit f505c553. > > > CONFIG_EXPERT is a superset to all that: it implies config control > > over all debug options *and* over many other kernel components as > > well. > > I don't see the advantage that EXPERT=y is now unconditionally enabling > the DEBUG_KERNEL knob. You don't see it - Josh and me saw an advantage and we explained it in the commit and in the discussion: when full fine-grained configurability is desired then it's useful to extend that to all the debugging options as well - instead of requiring an explicit EXPERT line on all default-enabled debug feature options that might have kernel size (or other expert configuration) relevance. It's a somewhat simpler, more robust and more maintainable Kconfig option hierarchy. > When I am an "expert", why can't I still decide myself globally if > I want to enable more debugging or not? The (valid) observation from Josh was that enabling CONFIG_EXPERT is more than enough to signal that unusual, highly expert configuration is sought. Any additional knobs are just unnecessary obfuscation of that primary intent. So the point of the change is to extend the scope of CONFIG_EXPERT to more kernel subsystem config options. The only side effect of the change that i can see, beyond open-coded use of CONFIG_DEBUG_KERNEL, is that DEBUG_KERNEL hidden options like this: FOO bool "debug option foo" default y depends on DEBUG_KERNEL ... will now become visible if CONFIG_EXPERT is enabled. This is similar to how new CONFIG_EXPERT options become visible so not unusual or hard to handle for the expert configurator. Normal users should not need to enable CONFIG_EXPERT. > Another problem is that we have code using "#ifdef > CONFIG_DEBUG_KERNEL" so enabling EXPERT for making your kernel > smaller can make your kernel bigger, but such usages should anyway > get own config options. Doesn't seem to be a significant issue, only some architectures have a low number of such hacks: arch/blackfin/include/asm/context.S:#ifdef CONFIG_DEBUG_KERNEL arch/blackfin/include/asm/entry.h:# ifndef CONFIG_DEBUG_KERNEL arch/blackfin/include/asm/entry.h:# else /* CONFIG_DEBUG_KERNEL */ arch/blackfin/include/asm/entry.h:# endif /* CONFIG_DEBUG_KERNEL */ arch/parisc/mm/init.c:#ifdef CONFIG_DEBUG_KERNEL /* double-sanity-check paranoia */ arch/powerpc/kernel/sysfs.c:#ifdef CONFIG_DEBUG_KERNEL arch/powerpc/kernel/sysfs.c:#endif /* CONFIG_DEBUG_KERNEL */ arch/powerpc/kernel/sysfs.c:#ifdef CONFIG_DEBUG_KERNEL arch/powerpc/kernel/sysfs.c:#endif /* CONFIG_DEBUG_KERNEL */ and yes, it should be fixed: CONFIG_DEBUG_KERNEL is a container Kconfig option which should not have functional side-effects in itself. > > This is a pretty easy model to think about. > > There are many options in the kernel that only enable seeing other > config options. > > With that model, shouldn't EXPERT also have > select MISC_FILESYSTEMS > and many other similar select's? Yes, adding that select would probably make sense, if the vfs folks agreed too. Basically CONFIG_EXPERT wants to imply 'full configurability of the kernel' - within sensible limits. > The point I am trying to make here is that there are many > convenience config options that only exist to allow a user to hide > a bunch of options he is not interested in, and DEBUG_KERNEL is one > of them. [1] Yes, and the correct answer to that observation is to increase convenience, not to reduce it. > > > The crazy select added in commit f505c553 is wrong. > > > > Why? Your original message does not explain it. > > It is not needed for what it was claimed it was needed for, > and it is a bad idea in general. Nonsense, what the commit changelog says is mostly accurate: before the change it was not possible to disable DEBUG_BUGVERBOSE under CONFIG_EXPERT, if DEBUG_KERNEL was disabled. DEBUG_BUGVERBOSE is by far the biggest size impact debug option of the ones that are enabled by default. The changelog is technically not correct for RODATA (which depends on DEBUG_KERNEL in a depends line) but it's certainly true in spirit, IMO DEBUG_RODATA should be turned into an always-enabled debug option like: config DEBUG_RODATA bool "Write protect kernel read-only data structures" if DEBUG_KERNEL default y ---help--- Arjan, Linus, do you have objections against doing that? Adrian, you had valid points, but i have the impression that you got lost in details and are missing the bigger picture and the revert you suggested looks like a step backwards to me. The commit itself is no big deal, it's a small convenience change - but i preferred if it all moved in the right direction ... Thanks, Ingo ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Please revert "debug: Make CONFIG_EXPERT select CONFIG_DEBUG_KERNEL to unhide debug options" 2011-10-10 10:21 ` Ingo Molnar @ 2011-10-10 12:13 ` Adrian Bunk 2011-10-12 8:38 ` Ingo Molnar 0 siblings, 1 reply; 17+ messages in thread From: Adrian Bunk @ 2011-10-10 12:13 UTC (permalink / raw) To: Ingo Molnar Cc: Arjan van de Ven, Josh Triplett, Linus Torvalds, linux-kernel, Frederic Weisbecker, Sam Ravnborg On Mon, Oct 10, 2011 at 12:21:21PM +0200, Ingo Molnar wrote: > > * Adrian Bunk <bunk@stusta.de> wrote: > > > On Mon, Oct 10, 2011 at 09:29:48AM +0200, Ingo Molnar wrote: > > > * Adrian Bunk <bunk@stusta.de> wrote: > > >... > > > I think you are wrong not just about that detail but about the whole > > > premise of your complaint as well: > > > > > > > config DEBUG_BUGVERBOSE > > > > - bool "Verbose BUG() reporting (adds 70K)" if DEBUG_KERNEL && EXPERT > > > > + bool "Verbose BUG() reporting (adds 70K)" if EXPERT > > > > > > > > This part of the patch would have been the correct and complete > > > > solution for DEBUG_BUGVERBOSE. > > > > > > Not really - it's a debugging-only option and when i turn on > > > CONFIG_DEBUG_KERNEL I expect to have full config control over all > > > debug options - with sane defaults provided. > > > > Then you would have to remove the dependency on EXPERT from the prompt, > > and allow unsetting DEBUG_BUGVERBOSE with EXPERT=n, DEBUG_KERNEL=y. > > > > Note that this is completely unrelated to the commit we are discussing, > > since commit f505c553 has no effect in the EXPERT=n case you are > > discussing here. > > > > > I definitely don't want a debugging option to depend on > > > CONFIG_EXPERT. > > > > DEBUG_BUGVERBOSE does not depend on EXPERT. > > > > But EXPERT is currently required for disabling it. > > Correct - that's a further variation. In the case of debug options > that we *really* don't want normal users to disable we do something > like this: > > config DEBUG_BUGVERBOSE > bool "Verbose BUG() reporting (adds 70K)" if DEBUG_KERNEL && EXPERT > > Commit f505c553 ("debug: Make CONFIG_EXPERT select > CONFIG_DEBUG_KERNEL to unhide debug options") allows this line to be > further simplified into: > > bool "Verbose BUG() reporting (adds 70K)" if EXPERT > > ... but this was not the main purpose of the commit - nor is this > simplification strictly necessary. You do not need commit f505c553 for that, the dependency of this prompt on DEBUG_KERNEL should be removed in any case. > > This part of our discussion boils down to the following options: > > 1. DEBUG_KERNEL allows a user to enable additional debugging > > 2. DEBUG_KERNEL gives a user full control over all debug options > > > > Traditionally it was 1., to tell a user reporting a bug that he should > > enable additional debugging in his kernel - without him accidentally > > disabling useful debugging. > > > > Again, this part of the discussion is unrelated to commit f505c553. > > > > > CONFIG_EXPERT is a superset to all that: it implies config control > > > over all debug options *and* over many other kernel components as > > > well. > > > > I don't see the advantage that EXPERT=y is now unconditionally enabling > > the DEBUG_KERNEL knob. > > You don't see it - Josh and me saw an advantage and we explained it > in the commit and in the discussion: when full fine-grained > configurability is desired then it's useful to extend that to all the > debugging options as well - instead of requiring an explicit EXPERT > line on all default-enabled debug feature options that might have > kernel size (or other expert configuration) relevance. > > It's a somewhat simpler, more robust and more maintainable Kconfig > option hierarchy. This is not true: Commit f505c553 does not allow you to remove any EXPERT dependencies anywhere (unless you could remove them already without that commit). The reason is simple: Any kind of dependencies on EXPERT are a nop with EXPERT=y, and commit f505c553 only makes any difference with EXPERT=y. > > When I am an "expert", why can't I still decide myself globally if > > I want to enable more debugging or not? > > The (valid) observation from Josh was that enabling CONFIG_EXPERT > is more than enough to signal that unusual, highly expert > configuration is sought. > > Any additional knobs are just unnecessary obfuscation of that primary > intent. > > So the point of the change is to extend the scope of CONFIG_EXPERT to > more kernel subsystem config options. > > The only side effect of the change that i can see, beyond open-coded > use of CONFIG_DEBUG_KERNEL, is that DEBUG_KERNEL hidden options like > this: > > FOO > bool "debug option foo" > default y > depends on DEBUG_KERNEL > > ... will now become visible if CONFIG_EXPERT is enabled. This is > similar to how new CONFIG_EXPERT options become visible so not > unusual or hard to handle for the expert configurator. Normal users > should not need to enable CONFIG_EXPERT. We do have an existing global "I don't need additional debugging" knob, that is DEBUG_KERNEL=n. Every user (no matter how EXPERT is set) can turn this on. Why do you want to make life harder for people with EXPERT=y by not allowing them to turn this off if they want to? >... > and yes, it should be fixed: CONFIG_DEBUG_KERNEL is a container > Kconfig option which should not have functional side-effects in > itself. > > > > This is a pretty easy model to think about. > > > > There are many options in the kernel that only enable seeing other > > config options. > > > > With that model, shouldn't EXPERT also have > > select MISC_FILESYSTEMS > > and many other similar select's? > > Yes, adding that select would probably make sense, if the vfs folks > agreed too. Basically CONFIG_EXPERT wants to imply 'full > configurability of the kernel' - within sensible limits. When configuring his kernel, a user set MISC_FILESYSTEMS=n. Now he sets EXPERT=y and runs "make oldconfig". Why would it make sense that he is now asked for each of these filesystems whether he wants to enable it? Noone was ever hiding MISC_FILESYSTEMS=y from him, and he already said explicitely that he does not want it. > > The point I am trying to make here is that there are many > > convenience config options that only exist to allow a user to hide > > a bunch of options he is not interested in, and DEBUG_KERNEL is one > > of them. [1] > > Yes, and the correct answer to that observation is to increase > convenience, not to reduce it. Convenience is to be able to disable a set of options a user is not interested in by disabling one option. > > > > The crazy select added in commit f505c553 is wrong. > > > > > > Why? Your original message does not explain it. > > > > It is not needed for what it was claimed it was needed for, > > and it is a bad idea in general. > > Nonsense, what the commit changelog says is mostly accurate: before > the change it was not possible to disable DEBUG_BUGVERBOSE under > CONFIG_EXPERT, if DEBUG_KERNEL was disabled. It fixes this problem (I already admitted that I was wrong for DEBUG_BUGVERBOSE), but it is the wrong fix. > DEBUG_BUGVERBOSE is by far the biggest size impact debug option of > the ones that are enabled by default. config DEBUG_BUGVERBOSE - bool "Verbose BUG() reporting (adds 70K)" if DEBUG_KERNEL && EXPERT + bool "Verbose BUG() reporting (adds 70K)" if EXPERT would have fixed this bug. > The changelog is technically not correct for RODATA (which depends on > DEBUG_KERNEL in a depends line) but it's certainly true in spirit, > IMO DEBUG_RODATA should be turned into an always-enabled debug option > like: > > config DEBUG_RODATA > bool "Write protect kernel read-only data structures" if DEBUG_KERNEL > default y > ---help--- With that, you would have to *enable* DEBUG_KERNEL for being able to *disable* this debug option. What we are discussing is a commit that lets EXPERT select DEBUG_KERNEL, to fix the problem that when enabling EXPERT one was not able to disable such an option. config DEBUG_RODATA bool "Write protect kernel read-only data structures" if EXPERT default y would be the correct solution. > Arjan, Linus, do you have objections against doing that? > > Adrian, you had valid points, but i have the impression that you got > lost in details and are missing the bigger picture and the revert you > suggested looks like a step backwards to me. The commit itself is no > big deal, it's a small convenience change - but i preferred if it all > moved in the right direction ... But it moves it to the wrong direction... I have a pretty clear view of the bigger picture: Enabling DEBUG_KERNEL allows to enable additional debugging in the kernel. Enabling EXPERT gives additional choices, often of the "fiddle with this only if you *really* know what you are doing" type. None of these two options implies the other, and it sounds rather strange to me that enabling an option mostly intended for decreasing the kernel size now automatically enables an option that enables options for increasing the kernel size. > Thanks, > > Ingo cu Adrian -- "Is there not promise of rain?" Ling Tan asked suddenly out of the darkness. There had been need of rain for many days. "Only a promise," Lao Er said. Pearl S. Buck - Dragon Seed ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Please revert "debug: Make CONFIG_EXPERT select CONFIG_DEBUG_KERNEL to unhide debug options" 2011-10-10 12:13 ` Adrian Bunk @ 2011-10-12 8:38 ` Ingo Molnar 2011-10-20 21:41 ` Adrian Bunk 0 siblings, 1 reply; 17+ messages in thread From: Ingo Molnar @ 2011-10-12 8:38 UTC (permalink / raw) To: Adrian Bunk Cc: Arjan van de Ven, Josh Triplett, Linus Torvalds, linux-kernel, Frederic Weisbecker, Sam Ravnborg * Adrian Bunk <bunk@stusta.de> wrote: > On Mon, Oct 10, 2011 at 12:21:21PM +0200, Ingo Molnar wrote: > > > > * Adrian Bunk <bunk@stusta.de> wrote: > > > > > On Mon, Oct 10, 2011 at 09:29:48AM +0200, Ingo Molnar wrote: > > > > * Adrian Bunk <bunk@stusta.de> wrote: > > > >... > > > > I think you are wrong not just about that detail but about the whole > > > > premise of your complaint as well: > > > > > > > > > config DEBUG_BUGVERBOSE > > > > > - bool "Verbose BUG() reporting (adds 70K)" if DEBUG_KERNEL && EXPERT > > > > > + bool "Verbose BUG() reporting (adds 70K)" if EXPERT > > > > > > > > > > This part of the patch would have been the correct and complete > > > > > solution for DEBUG_BUGVERBOSE. > > > > > > > > Not really - it's a debugging-only option and when i turn on > > > > CONFIG_DEBUG_KERNEL I expect to have full config control over all > > > > debug options - with sane defaults provided. > > > > > > Then you would have to remove the dependency on EXPERT from the prompt, > > > and allow unsetting DEBUG_BUGVERBOSE with EXPERT=n, DEBUG_KERNEL=y. > > > > > > Note that this is completely unrelated to the commit we are discussing, > > > since commit f505c553 has no effect in the EXPERT=n case you are > > > discussing here. > > > > > > > I definitely don't want a debugging option to depend on > > > > CONFIG_EXPERT. > > > > > > DEBUG_BUGVERBOSE does not depend on EXPERT. > > > > > > But EXPERT is currently required for disabling it. > > > > Correct - that's a further variation. In the case of debug options > > that we *really* don't want normal users to disable we do something > > like this: > > > > config DEBUG_BUGVERBOSE > > bool "Verbose BUG() reporting (adds 70K)" if DEBUG_KERNEL && EXPERT > > > > Commit f505c553 ("debug: Make CONFIG_EXPERT select > > CONFIG_DEBUG_KERNEL to unhide debug options") allows this line to be > > further simplified into: > > > > bool "Verbose BUG() reporting (adds 70K)" if EXPERT > > > > ... but this was not the main purpose of the commit - nor is this > > simplification strictly necessary. > > You do not need commit f505c553 for that, the dependency of this > prompt on DEBUG_KERNEL should be removed in any case. An opt-out model is more maintainable here than an opt-in method. People will get debug dependencies right most of the time - but getting debug *and* CONFIG_EXPERT interactions right is on the backburner generally. So we are better off if CONFIG_EXPERT simply implies (selects) CONFIG_KERNEL_DEBUG - makes CONFIG_EXPERT an invariant as far as debugging features are concerned and reduces/eliminates the trickle of avoidable CONFIG_EXPERT tweaking patches in lib/Kconfig.debug. > Why do you want to make life harder for people with EXPERT=y by not > allowing them to turn this off if they want to? It's simpler to have one flat CONFIG_EXPERT=y option to gain broad expert-configurability of core debug functionality of the kernel. It should arguably not explode the options to *all* drivers of the kernel: > When configuring his kernel, a user set MISC_FILESYSTEMS=n. > > Now he sets EXPERT=y and runs "make oldconfig". > > Why would it make sense that he is now asked for each of these > filesystems whether he wants to enable it? I agree with you that filesystems are more like drivers here and should probably not be selected by CONFIG_EXPERT. It's up to the VFS folks whether they consider experts to be frequent requestors. (probably not) But the important case here is the situation outlined in the changelog, that a default-y core kernel option such as BUGVERBOSE unconditionally *adds* code. Core kernel debugging code is an area that by its nature has and is bound to have such options - MISC_FILESYSTEMS probably not. Thanks, Ingo ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Please revert "debug: Make CONFIG_EXPERT select CONFIG_DEBUG_KERNEL to unhide debug options" 2011-10-12 8:38 ` Ingo Molnar @ 2011-10-20 21:41 ` Adrian Bunk 2011-10-21 8:19 ` Michal Marek 0 siblings, 1 reply; 17+ messages in thread From: Adrian Bunk @ 2011-10-20 21:41 UTC (permalink / raw) To: Ingo Molnar Cc: Arjan van de Ven, Josh Triplett, Linus Torvalds, linux-kernel, Frederic Weisbecker, Sam Ravnborg, Michal Marek On Wed, Oct 12, 2011 at 10:38:01AM +0200, Ingo Molnar wrote: > > * Adrian Bunk <bunk@stusta.de> wrote: > > > On Mon, Oct 10, 2011 at 12:21:21PM +0200, Ingo Molnar wrote: > > > > > > * Adrian Bunk <bunk@stusta.de> wrote: > > > > > > > On Mon, Oct 10, 2011 at 09:29:48AM +0200, Ingo Molnar wrote: > > > > > * Adrian Bunk <bunk@stusta.de> wrote: > > > > >... > > > > > I think you are wrong not just about that detail but about the whole > > > > > premise of your complaint as well: > > > > > > > > > > > config DEBUG_BUGVERBOSE > > > > > > - bool "Verbose BUG() reporting (adds 70K)" if DEBUG_KERNEL && EXPERT > > > > > > + bool "Verbose BUG() reporting (adds 70K)" if EXPERT > > > > > > > > > > > > This part of the patch would have been the correct and complete > > > > > > solution for DEBUG_BUGVERBOSE. > > > > > > > > > > Not really - it's a debugging-only option and when i turn on > > > > > CONFIG_DEBUG_KERNEL I expect to have full config control over all > > > > > debug options - with sane defaults provided. > > > > > > > > Then you would have to remove the dependency on EXPERT from the prompt, > > > > and allow unsetting DEBUG_BUGVERBOSE with EXPERT=n, DEBUG_KERNEL=y. > > > > > > > > Note that this is completely unrelated to the commit we are discussing, > > > > since commit f505c553 has no effect in the EXPERT=n case you are > > > > discussing here. > > > > > > > > > I definitely don't want a debugging option to depend on > > > > > CONFIG_EXPERT. > > > > > > > > DEBUG_BUGVERBOSE does not depend on EXPERT. > > > > > > > > But EXPERT is currently required for disabling it. > > > > > > Correct - that's a further variation. In the case of debug options > > > that we *really* don't want normal users to disable we do something > > > like this: > > > > > > config DEBUG_BUGVERBOSE > > > bool "Verbose BUG() reporting (adds 70K)" if DEBUG_KERNEL && EXPERT > > > > > > Commit f505c553 ("debug: Make CONFIG_EXPERT select > > > CONFIG_DEBUG_KERNEL to unhide debug options") allows this line to be > > > further simplified into: > > > > > > bool "Verbose BUG() reporting (adds 70K)" if EXPERT > > > > > > ... but this was not the main purpose of the commit - nor is this > > > simplification strictly necessary. > > > > You do not need commit f505c553 for that, the dependency of this > > prompt on DEBUG_KERNEL should be removed in any case. > > An opt-out model is more maintainable here than an opt-in method. > People will get debug dependencies right most of the time - but > getting debug *and* CONFIG_EXPERT interactions right is on the > backburner generally. The problem gets slightly shifted now, I am not seeing how it gets substancially better - at the expense of making it worse for kconfig users. Ingo, would you accept if I would go through the Kconfig files and monitor future changes to Kconfig files in the kernel (or if Michal does it, I don't insist that it has to be me if someone else wants to do it)? Kconfig is the one (and only) area in the kernel where I know more than you, and tryng to get it all right in the kernel was something I was already trying a few years ago, before clashes with you over kconfig fixes resulted in nasty arguments and were part of the reason for me to leave kernel development (and although it was not exactly planned, the latter was in hindsight a good thing for everyone). > So we are better off if CONFIG_EXPERT simply implies (selects) > CONFIG_KERNEL_DEBUG - makes CONFIG_EXPERT an invariant as far as > debugging features are concerned and reduces/eliminates the trickle > of avoidable CONFIG_EXPERT tweaking patches in lib/Kconfig.debug. No, as I already explained commit f505c553 does not allow that at all. Let me repeat my explanation: Any kind of dependencies on EXPERT are a nop with EXPERT=y, and commit f505c553 only makes any difference with EXPERT=y. > > Why do you want to make life harder for people with EXPERT=y by not > > allowing them to turn this off if they want to? > > It's simpler to have one flat CONFIG_EXPERT=y option to gain broad > expert-configurability of core debug functionality of the kernel. It still sounds awkward that an option that is mostly intended for users wanting to make their kernels smaller is now forcibly showing debug options. > It should arguably not explode the options to *all* drivers of the > kernel: > > > When configuring his kernel, a user set MISC_FILESYSTEMS=n. > > > > Now he sets EXPERT=y and runs "make oldconfig". > > > > Why would it make sense that he is now asked for each of these > > filesystems whether he wants to enable it? > > I agree with you that filesystems are more like drivers here and > should probably not be selected by CONFIG_EXPERT. It's up to the VFS > folks whether they consider experts to be frequent requestors. > (probably not) > > But the important case here is the situation outlined in the > changelog, that a default-y core kernel option such as BUGVERBOSE > unconditionally *adds* code. Core kernel debugging code is an area > that by its nature has and is bound to have such options - > MISC_FILESYSTEMS probably not. There was a bug in the dependencies of DEBUG_BUGVERBOSE, there is no discussion about that. You don't need the big hammer of commit f505c553 for that, the correct fix is to fix this one option with config DEBUG_BUGVERBOSE - bool "Verbose BUG() reporting (adds 70K)" if DEBUG_KERNEL && EXPERT + bool "Verbose BUG() reporting (adds 70K)" if EXPERT Can you confirm that this patch alone fixes the DEBUG_BUGVERBOSE bug? Let me make a related example: My vsyscall=native patch fixes the UML problem I ran into. But it is not considered the correct fix by you and others, and will be reverted after a correct fix is available. Both for DEBUG_BUGVERBOSE and my UML problem there are big hammers that also fix the problems but are not the best solutions, and there are correct fixes (for the vsyscall problem I cannot claim knowledge on what the correct fix is, for DEBUG_BUGVERBOSE I can). > Thanks, > > Ingo cu Adrian -- "Is there not promise of rain?" Ling Tan asked suddenly out of the darkness. There had been need of rain for many days. "Only a promise," Lao Er said. Pearl S. Buck - Dragon Seed ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Please revert "debug: Make CONFIG_EXPERT select CONFIG_DEBUG_KERNEL to unhide debug options" 2011-10-20 21:41 ` Adrian Bunk @ 2011-10-21 8:19 ` Michal Marek 2011-10-21 9:22 ` Adrian Bunk 0 siblings, 1 reply; 17+ messages in thread From: Michal Marek @ 2011-10-21 8:19 UTC (permalink / raw) To: Adrian Bunk Cc: Ingo Molnar, Arjan van de Ven, Josh Triplett, Linus Torvalds, linux-kernel, Frederic Weisbecker, Sam Ravnborg On 20.10.2011 23:41, Adrian Bunk wrote: > Ingo, would you accept if I would go through the Kconfig files and > monitor future changes to Kconfig files in the kernel (or if Michal does > it, I don't insist that it has to be me if someone else wants to do it)? What kind of changes do you have in mind? Sorry, I haven't followed the whole thread. Michal ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Please revert "debug: Make CONFIG_EXPERT select CONFIG_DEBUG_KERNEL to unhide debug options" 2011-10-21 8:19 ` Michal Marek @ 2011-10-21 9:22 ` Adrian Bunk 2011-10-21 12:37 ` Michal Marek 0 siblings, 1 reply; 17+ messages in thread From: Adrian Bunk @ 2011-10-21 9:22 UTC (permalink / raw) To: Michal Marek Cc: Ingo Molnar, Arjan van de Ven, Josh Triplett, Linus Torvalds, linux-kernel, Frederic Weisbecker, Sam Ravnborg On Fri, Oct 21, 2011 at 10:19:29AM +0200, Michal Marek wrote: > On 20.10.2011 23:41, Adrian Bunk wrote: > > Ingo, would you accept if I would go through the Kconfig files and > > monitor future changes to Kconfig files in the kernel (or if Michal does > > it, I don't insist that it has to be me if someone else wants to do it)? > > What kind of changes do you have in mind? Sorry, I haven't followed the > whole thread. Checking the correctness and making things more robust. An example: config KVM tristate "Kernel-based Virtual Machine (KVM) support" ... # for TASKSTATS/TASK_DELAY_ACCT: depends on NET ... select TASKSTATS select TASK_DELAY_ACCT That breaks if anyone touches the dependencies of TASKSTATS or TASK_DELAY_ACCT. It should be solved better, my first thought would be introducing something like a TASK_DELAY_ACCT_AVAILABLE variable. (I am also not sure if what KVM is doing here is the best possible solution or if there's an in-kernel interface that should be factored out instead - but that is a secondary discussion that might start when you look at such cross-subsystem select's.) > Michal cu Adrian -- "Is there not promise of rain?" Ling Tan asked suddenly out of the darkness. There had been need of rain for many days. "Only a promise," Lao Er said. Pearl S. Buck - Dragon Seed ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Please revert "debug: Make CONFIG_EXPERT select CONFIG_DEBUG_KERNEL to unhide debug options" 2011-10-21 9:22 ` Adrian Bunk @ 2011-10-21 12:37 ` Michal Marek 2011-10-21 16:12 ` Adrian Bunk 0 siblings, 1 reply; 17+ messages in thread From: Michal Marek @ 2011-10-21 12:37 UTC (permalink / raw) To: Adrian Bunk Cc: Ingo Molnar, Arjan van de Ven, Josh Triplett, Linus Torvalds, linux-kernel, Frederic Weisbecker, Sam Ravnborg On 21.10.2011 11:22, Adrian Bunk wrote: > On Fri, Oct 21, 2011 at 10:19:29AM +0200, Michal Marek wrote: >> On 20.10.2011 23:41, Adrian Bunk wrote: >>> Ingo, would you accept if I would go through the Kconfig files and >>> monitor future changes to Kconfig files in the kernel (or if Michal does >>> it, I don't insist that it has to be me if someone else wants to do it)? >> >> What kind of changes do you have in mind? Sorry, I haven't followed the >> whole thread. > > Checking the correctness and making things more robust. > > An example: > > config KVM > tristate "Kernel-based Virtual Machine (KVM) support" > ... > # for TASKSTATS/TASK_DELAY_ACCT: > depends on NET > ... > select TASKSTATS > select TASK_DELAY_ACCT > > That breaks if anyone touches the dependencies of TASKSTATS > or TASK_DELAY_ACCT. > > It should be solved better, my first thought would be introducing > something like a TASK_DELAY_ACCT_AVAILABLE variable. Right, but that's a deficiency of the kconfig solver. Catalin Marinas has patched it to at least print a warning if a dependency of a select-ed symbol is not set. Ideally, we should get a better solver. Michal Michal ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Please revert "debug: Make CONFIG_EXPERT select CONFIG_DEBUG_KERNEL to unhide debug options" 2011-10-21 12:37 ` Michal Marek @ 2011-10-21 16:12 ` Adrian Bunk 0 siblings, 0 replies; 17+ messages in thread From: Adrian Bunk @ 2011-10-21 16:12 UTC (permalink / raw) To: Michal Marek Cc: Ingo Molnar, Arjan van de Ven, Josh Triplett, Linus Torvalds, linux-kernel, Frederic Weisbecker, Sam Ravnborg On Fri, Oct 21, 2011 at 02:37:13PM +0200, Michal Marek wrote: > On 21.10.2011 11:22, Adrian Bunk wrote: > > On Fri, Oct 21, 2011 at 10:19:29AM +0200, Michal Marek wrote: > >> On 20.10.2011 23:41, Adrian Bunk wrote: > >>> Ingo, would you accept if I would go through the Kconfig files and > >>> monitor future changes to Kconfig files in the kernel (or if Michal does > >>> it, I don't insist that it has to be me if someone else wants to do it)? > >> > >> What kind of changes do you have in mind? Sorry, I haven't followed the > >> whole thread. > > > > Checking the correctness and making things more robust. > > > > An example: > > > > config KVM > > tristate "Kernel-based Virtual Machine (KVM) support" > > ... > > # for TASKSTATS/TASK_DELAY_ACCT: > > depends on NET > > ... > > select TASKSTATS > > select TASK_DELAY_ACCT > > > > That breaks if anyone touches the dependencies of TASKSTATS > > or TASK_DELAY_ACCT. > > > > It should be solved better, my first thought would be introducing > > something like a TASK_DELAY_ACCT_AVAILABLE variable. > > Right, but that's a deficiency of the kconfig solver. Catalin Marinas > has patched it to at least print a warning if a dependency of a > select-ed symbol is not set. Ideally, we should get a better solver. I have heared that claim already 4-5 years ago, but I don't think it is true. Do you have a semantics how a better solver could handle all cases 100% correctly? Take these dependencies as an example: Originally (commit c9aaa895) only the select of TASK_DELAY_ACCT was added, let's look at that initial state. The select of TASKSTATS and the dependency on NET were later discovered by randconfig testings, your solver would have to handle that automatically. The dependency on NET is outside of people doing randconfig runs of little interest, since NET=y is set basically everywhere. The dependency of TASK_DELAY_ACCT on TASKSTATS is more interesting: You can automatically deduct that KVM select's TASK_DELAY_ACCT and TASK_DELAY_ACCT depends on TASKSTATS, and therefore KVM needs to automatically get a dependency on TASKSTATS. That fixes all build problems. For people trying to enable KVM in their kernel that would be a disaster. And you cannot blindly enable all dependencies, think e.g. of some code depending on 64BIT=n. I came to the conclusion that there is not "a deficiency of the kconfig solver", but that select'ing of options with dependencies is an evil thing that should be avoided whenever possible, and needs manual attention when it is unavoidable. > Michal cu Adrian -- "Is there not promise of rain?" Ling Tan asked suddenly out of the darkness. There had been need of rain for many days. "Only a promise," Lao Er said. Pearl S. Buck - Dragon Seed ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2011-10-21 16:12 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-09-30 13:42 Please revert "debug: Make CONFIG_EXPERT select CONFIG_DEBUG_KERNEL to unhide debug options" Adrian Bunk 2011-09-30 15:05 ` Josh Triplett 2011-09-30 15:25 ` Frederic Weisbecker 2011-09-30 15:32 ` Frederic Weisbecker 2011-09-30 15:54 ` Adrian Bunk 2011-09-30 15:50 ` Adrian Bunk 2011-10-10 7:29 ` Ingo Molnar 2011-10-10 8:48 ` Adrian Bunk 2011-10-10 9:44 ` Mike Galbraith 2011-10-10 10:21 ` Ingo Molnar 2011-10-10 12:13 ` Adrian Bunk 2011-10-12 8:38 ` Ingo Molnar 2011-10-20 21:41 ` Adrian Bunk 2011-10-21 8:19 ` Michal Marek 2011-10-21 9:22 ` Adrian Bunk 2011-10-21 12:37 ` Michal Marek 2011-10-21 16:12 ` Adrian Bunk
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.