Please revert "debug: Make CONFIG_EXPERT select CONFIG_DEBUG_KERNEL to unhide debug options"

DISCUSSION Adrian Bunk 6 participants 17 messages Sep 30, 2011 – Oct 21, 2011

Share & tools

Adrian Bunk

author

2011-09-30 13:50 UTC #0 lore raw

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.

Josh Triplett

2011-09-30 15:10 UTC #1 lore raw

Quoted 20 lines · Adrian Bunk

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.

Quoted 4 lines

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 <[email protected]>, which explicitly recommended this approach.

- Josh Triplett

Frederic Weisbecker

2011-09-30 15:25 UTC #2 lore raw

Quoted 43 lines · Josh Triplett

Quoted 20 lines · Adrian Bunk
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.
Quoted 4 lines
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

Quoted 8 lines

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.

Frederic Weisbecker

2011-09-30 15:32 UTC #3 lore raw

2011/9/30 Frederic Weisbecker <[email protected]>:

Quoted 61 lines · Josh Triplett

Quoted 43 lines · Adrian Bunk
Quoted 20 lines
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.
Quoted 4 lines
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
Quoted 8 lines
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

Quoted 9 lines

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

Adrian Bunk

author

2011-09-30 15:54 UTC #4 lore raw

In reply to Frederic Weisbecker

Quoted 7 lines · Frederic Weisbecker

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

Signature · 4 lines

       "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

Adrian Bunk

author

2011-09-30 15:50 UTC #5 lore raw

In reply to Josh Triplett

Quoted 24 lines · Josh Triplett

On Fri, Sep 30, 2011 at 04:42:45PM +0300, Adrian Bunk wrote:
...
Quoted 4 lines
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

Quoted 7 lines

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

Quoted 4 lines

...
Also see Ingo Molnar's mail with Message-Id <[email protected]>, 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.

Quoted 1 line

- Josh Triplett

cu
Adrian

[1] http://lkml.org/lkml/2011/6/6/641

Signature · 4 lines

       "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

Ingo Molnar

2011-10-10 07:31 UTC #6 lore raw

Quoted 29 lines · Adrian Bunk

Quoted 28 lines · Josh Triplett
On Fri, Sep 30, 2011 at 04:42:45PM +0300, Adrian Bunk wrote:
...
Quoted 4 lines
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:

Quoted 6 lines

Quoted 3 lines
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:

Quoted 6 lines

config DEBUG_BUGVERBOSE
(quoted diff) +1 -1
-	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.

Quoted 1 line

The crazy select added in commit f505c553 is wrong.

Why? Your original message does not explain it.

Thanks,

Ingo

Adrian Bunk

author

2011-10-10 08:48 UTC #7 lore raw

Quoted 9 lines · Ingo Molnar

* Adrian Bunk <[email protected]> wrote:
...
I think you are wrong not just about that detail but about the whole premise of your complaint as well:
Quoted 4 lines
config DEBUG_BUGVERBOSE
(quoted diff) +1 -1
-	bool "Verbose BUG() reporting (adds 70K)" if DEBUG_KERNEL && EXPERT
+	bool "Verbose BUG() reporting (adds 70K)" if EXPERT

Quoted 6 lines

Quoted 2 lines
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.

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 1 line

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]

Quoted 3 lines

Quoted 1 line
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.

Quoted 3 lines

Thanks,
Ingo

cu
Adrian

[1] ignoring the buggy code using "#ifdef CONFIG_DEBUG_KERNEL"

Signature · 4 lines

       "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

Mike Galbraith

2011-10-10 09:44 UTC #8 lore raw

Quoted 3 lines · Adrian Bunk

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

Ingo Molnar

2011-10-10 10:23 UTC #9 lore raw

In reply to Adrian Bunk

Quoted 24 lines · Adrian Bunk

Quoted 9 lines · Ingo Molnar
* Adrian Bunk <[email protected]> wrote:
...
I think you are wrong not just about that detail but about the whole premise of your complaint as well:
Quoted 4 lines
config DEBUG_BUGVERBOSE
(quoted diff) +1 -1
-	bool "Verbose BUG() reporting (adds 70K)" if DEBUG_KERNEL && EXPERT
+	bool "Verbose BUG() reporting (adds 70K)" if EXPERT
Quoted 6 lines
Quoted 2 lines
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.

Quoted 6 lines

Quoted 2 lines
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.

Quoted 8 lines

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.

Quoted 8 lines

Again, this part of the discussion is unrelated to commit f505c553.
Quoted 3 lines
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.

Quoted 2 lines

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.

Quoted 4 lines

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.

Quoted 8 lines

Quoted 1 line
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.

Quoted 4 lines

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.

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
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

Adrian Bunk

author

2011-10-10 12:13 UTC #10 lore raw

Quoted 41 lines · Ingo Molnar

Quoted 24 lines · Adrian Bunk
Quoted 9 lines · Ingo Molnar
* Adrian Bunk <[email protected]> wrote:
...
I think you are wrong not just about that detail but about the whole premise of your complaint as well:
Quoted 4 lines
config DEBUG_BUGVERBOSE
(quoted diff) +1 -1
-	bool "Verbose BUG() reporting (adds 70K)" if DEBUG_KERNEL && EXPERT
+	bool "Verbose BUG() reporting (adds 70K)" if EXPERT
Quoted 6 lines
Quoted 2 lines
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.
Quoted 6 lines
Quoted 2 lines
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

Quoted 8 lines

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.

Quoted 24 lines

Quoted 8 lines
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.
Quoted 8 lines
Again, this part of the discussion is unrelated to commit f505c553.
Quoted 3 lines
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.

Quoted 2 lines

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.

Quoted 22 lines

Quoted 2 lines
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

Quoted 4 lines

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

Quoted 10 lines

...
and yes, it should be fixed: CONFIG_DEBUG_KERNEL is a container Kconfig option which should not have functional side-effects in itself.
Quoted 5 lines
Quoted 1 line
This is a pretty easy model to think about.
There are many options in the kernel that only enable seeing other config options.

Quoted 7 lines

Quoted 3 lines
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.

Quoted 7 lines

Quoted 4 lines
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.

Quoted 10 lines

Quoted 6 lines
Quoted 3 lines
Quoted 1 line
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.

Quoted 2 lines

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.

Quoted 9 lines

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.

Quoted 7 lines

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.

Quoted 3 lines

Thanks,
Ingo

cu
Adrian

Signature · 4 lines

       "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

Ingo Molnar

2011-10-12 08:39 UTC #11 lore raw

Quoted 46 lines · Adrian Bunk

Quoted 45 lines · Ingo Molnar
Quoted 24 lines · Adrian Bunk
Quoted 9 lines · Ingo Molnar
* Adrian Bunk <[email protected]> wrote:
...
I think you are wrong not just about that detail but about the whole premise of your complaint as well:
Quoted 4 lines
config DEBUG_BUGVERBOSE
(quoted diff) +1 -1
-	bool "Verbose BUG() reporting (adds 70K)" if DEBUG_KERNEL && EXPERT
+	bool "Verbose BUG() reporting (adds 70K)" if EXPERT
Quoted 6 lines
Quoted 2 lines
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.
Quoted 6 lines
Quoted 2 lines
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:

Quoted 7 lines

Quoted 4 lines
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.

Quoted 2 lines

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:

Quoted 6 lines

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

Adrian Bunk

author

2011-10-20 21:41 UTC #12 lore raw

Quoted 54 lines · Ingo Molnar

Quoted 51 lines · Adrian Bunk
Quoted 49 lines · Ingo Molnar
Quoted 24 lines · Adrian Bunk
Quoted 9 lines · Ingo Molnar
* Adrian Bunk <[email protected]> wrote:
...
I think you are wrong not just about that detail but about the whole premise of your complaint as well:
Quoted 4 lines
config DEBUG_BUGVERBOSE
(quoted diff) +1 -1
-	bool "Verbose BUG() reporting (adds 70K)" if DEBUG_KERNEL && EXPERT
+	bool "Verbose BUG() reporting (adds 70K)" if EXPERT
Quoted 6 lines
Quoted 2 lines
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.
Quoted 6 lines
Quoted 2 lines
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.

Quoted 7 lines

Quoted 2 lines
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).

Quoted 4 lines

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.

Quoted 5 lines

Quoted 2 lines
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.

Quoted 15 lines

It should arguably not explode the options to *all* drivers of the kernel:
Quoted 6 lines
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)

Quoted 5 lines

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

Quoted 3 lines

Thanks,
Ingo

cu
Adrian

Signature · 4 lines

       "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

Michal Marek

2011-10-21 08:19 UTC #13 lore raw

Quoted 3 lines · Adrian Bunk

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

Adrian Bunk

author

2011-10-21 09:22 UTC #14 lore raw

Quoted 7 lines · Michal Marek

Quoted 3 lines · Adrian Bunk
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.)

Quoted 1 line

Michal

cu
Adrian

Signature · 4 lines

       "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

Michal Marek

2011-10-21 12:37 UTC #15 lore raw

Quoted 22 lines · Adrian Bunk

Quoted 7 lines · Michal Marek
Quoted 3 lines · Adrian Bunk
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

Quoted 5 lines

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

Adrian Bunk

author

2011-10-21 16:12 UTC #16 lore raw

Quoted 26 lines · Michal Marek

Quoted 25 lines · Adrian Bunk
Quoted 7 lines · Michal Marek
Quoted 3 lines · Adrian Bunk
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.

Quoted 6 lines

Quoted 2 lines
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.

Quoted 1 line

Michal

cu
Adrian

Signature · 4 lines

       "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