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

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

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


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.

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

[Josh Triplett]

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

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

[Frederic Weisbecker]

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.

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

[Frederic Weisbecker]

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

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

[Adrian Bunk]

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

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

[Adrian Bunk]

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

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

[Ingo Molnar]

* 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

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

[Adrian Bunk]

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

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

[Mike Galbraith]

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

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

[Ingo Molnar]

* 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

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

[Adrian Bunk]

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

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

[Ingo Molnar]

* 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

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

[Adrian Bunk]

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

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

[Michal Marek]

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

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

[Adrian Bunk]

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

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

[Michal Marek]

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

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

[Adrian Bunk]

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