[PATCH 0/2] do not select KALLSYMS_ALL

[PATCH 0/2] do not select KALLSYMS_ALL

[Artem Bityutskiy]

UBI and UBIFS have an option to compile-in debugging support. When we enable
debugging, compile-in various assertions, self-check functions and test modes.
We also compile in additional verbose error messages, flash dumps and stack
dumps.

When we have debugging support enabled, we want to have nice stackdumps, so
we selected KALLSYMS_ALL. But this causes various problems like unmet direct
dependencies, reported by Randy:

warning: (UBIFS_FS_DEBUG && LOCKDEP && LATENCYTOP) selects KALLSYMS_ALL which
has unmet direct dependencies (DEBUG_KERNEL && KALLSYMS)

I quickly looked at KALLSYMS_ALL and it is not immediately clear what exactly
is the difference between KALLSYMS and KALLSYMS_ALL. And there is this
CONFIG_KALLSYMS_EXTRA_PASS symbol, which is declared "temporary workaround"
in the Kconfig help. This shows that this stuff is messy.

I personally think KALLSYMS_ALL should be just merged with KALLSYMS and
disappear - we should have only one option. CONFIG_KALLSYMS_EXTRA_PASS should
die as well.

But I don't want to delve into that, I choose to just forget about KALLSYMS_ALL
and select KALLSYMS in UBI/UBIFS which seems to be enough for us.

Artem.

[PATCH 1/2] UBIFS: do not select KALLSYMS_ALL

[Artem Bityutskiy]

From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

All UBIFS needs is to make sure we stacktraces when UBIFS debugging
is enabled. It is enough to select KALLSYMS for this, KALLSYMS_ALL
is not necessary. Moreover, Randy Dunlap reported that UBIFS causes
the following Kconfig dependency warning:

warning: (UBIFS_FS_DEBUG && LOCKDEP && LATENCYTOP) selects KALLSYMS_ALL
which has unmet direct dependencies (DEBUG_KERNEL && KALLSYMS)

The reason is that KALLSYMS_ALL requires DEBUG_KERNEL and KALLSYMS, so
ideally, to select KALLSYMS_ALL we'd need to select DEBUG_KERNEL and
KALLSYMS first.

This seems to be too much to select. The easiest way to go is to forget
about KALLSYMS_ALL and just select KALLSYMS when UBIFS debugging is
enabled - that should be enough for stackdumps.

Reported-by: Randy Dunlap <randy.dunlap@oracle.com>
Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 fs/ubifs/Kconfig |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/ubifs/Kconfig b/fs/ubifs/Kconfig
index d744090..f8b0160 100644
--- a/fs/ubifs/Kconfig
+++ b/fs/ubifs/Kconfig
@@ -47,7 +47,7 @@ config UBIFS_FS_DEBUG
 	bool "Enable debugging support"
 	depends on UBIFS_FS
 	select DEBUG_FS
-	select KALLSYMS_ALL
+	select KALLSYMS
 	help
 	  This option enables UBIFS debugging support. It makes sure various
 	  assertions, self-checks, debugging messages and test modes are compiled
-- 
1.7.2.3

Re: [PATCH 1/2] UBIFS: do not select KALLSYMS_ALL

[Randy Dunlap]

On Wed, 30 Mar 2011 11:40:15 +0300 Artem Bityutskiy wrote:

> From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> 
> All UBIFS needs is to make sure we stacktraces when UBIFS debugging
> is enabled. It is enough to select KALLSYMS for this, KALLSYMS_ALL
> is not necessary. Moreover, Randy Dunlap reported that UBIFS causes
> the following Kconfig dependency warning:
> 
> warning: (UBIFS_FS_DEBUG && LOCKDEP && LATENCYTOP) selects KALLSYMS_ALL
> which has unmet direct dependencies (DEBUG_KERNEL && KALLSYMS)
> 
> The reason is that KALLSYMS_ALL requires DEBUG_KERNEL and KALLSYMS, so
> ideally, to select KALLSYMS_ALL we'd need to select DEBUG_KERNEL and
> KALLSYMS first.
> 
> This seems to be too much to select. The easiest way to go is to forget
> about KALLSYMS_ALL and just select KALLSYMS when UBIFS debugging is
> enabled - that should be enough for stackdumps.
> 
> Reported-by: Randy Dunlap <randy.dunlap@oracle.com>
> Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> ---
>  fs/ubifs/Kconfig |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/ubifs/Kconfig b/fs/ubifs/Kconfig
> index d744090..f8b0160 100644
> --- a/fs/ubifs/Kconfig
> +++ b/fs/ubifs/Kconfig
> @@ -47,7 +47,7 @@ config UBIFS_FS_DEBUG
>  	bool "Enable debugging support"
>  	depends on UBIFS_FS
>  	select DEBUG_FS
> -	select KALLSYMS_ALL
> +	select KALLSYMS
>  	help
>  	  This option enables UBIFS debugging support. It makes sure various
>  	  assertions, self-checks, debugging messages and test modes are compiled
> -- 

Acked-by: Randy Dunlap <randy.dunlap@oracle.com>

Thanks.

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH 1/2] UBIFS: do not select KALLSYMS_ALL

[Artem Bityutskiy]

On Wed, 2011-03-30 at 15:41 -0700, Randy Dunlap wrote:
> > diff --git a/fs/ubifs/Kconfig b/fs/ubifs/Kconfig
> > index d744090..f8b0160 100644
> > --- a/fs/ubifs/Kconfig
> > +++ b/fs/ubifs/Kconfig
> > @@ -47,7 +47,7 @@ config UBIFS_FS_DEBUG
> >  	bool "Enable debugging support"
> >  	depends on UBIFS_FS
> >  	select DEBUG_FS
> > -	select KALLSYMS_ALL
> > +	select KALLSYMS
> >  	help
> >  	  This option enables UBIFS debugging support. It makes sure various
> >  	  assertions, self-checks, debugging messages and test modes are compiled
> > -- 
> 
> Acked-by: Randy Dunlap <randy.dunlap@oracle.com>

Pushed to UBI / UBIFS trees. These will show-up in linux-next a bit
later (around -rc2, if it will be usable enough), and then I'll send
this to Linus.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

[PATCH 2/2] UBI: do not select KALLSYMS_ALL

[Artem Bityutskiy]

From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

All UBI needs is to make sure we stacktraces when UBI debugging
is enabled. It is enough to select KALLSYMS for this, KALLSYMS_ALL
is not necessary.

And the current Kconfig line we have:

select KALLSYMS_ALL if KALLSYMS && DEBUG_KERNEL

is just too complex to be sane and right. But this "if" part there
is needed to prevent "unmet direct dependency" warnings, because
KALLSYMS_ALL depends on KALLSYMS and DEBUG_KERNEL, so we cannot
just select KALLSYMS_ALL.

Anyway, this feels messy, and we do not seem to really need KALLSYMS_ALL,
so select KALLSYMS instead.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 drivers/mtd/ubi/Kconfig |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/ubi/Kconfig b/drivers/mtd/ubi/Kconfig
index 6abeb4f..4dcc752 100644
--- a/drivers/mtd/ubi/Kconfig
+++ b/drivers/mtd/ubi/Kconfig
@@ -56,7 +56,7 @@ config MTD_UBI_DEBUG
 	bool "UBI debugging"
 	depends on SYSFS
 	select DEBUG_FS
-	select KALLSYMS_ALL if KALLSYMS && DEBUG_KERNEL
+	select KALLSYMS
 	help
 	  This option enables UBI debugging.
 
-- 
1.7.2.3

Re: [PATCH 2/2] UBI: do not select KALLSYMS_ALL

[Randy Dunlap]

On Wed, 30 Mar 2011 11:40:16 +0300 Artem Bityutskiy wrote:

> From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> 
> All UBI needs is to make sure we stacktraces when UBI debugging
> is enabled. It is enough to select KALLSYMS for this, KALLSYMS_ALL
> is not necessary.
> 
> And the current Kconfig line we have:
> 
> select KALLSYMS_ALL if KALLSYMS && DEBUG_KERNEL
> 
> is just too complex to be sane and right. But this "if" part there
> is needed to prevent "unmet direct dependency" warnings, because
> KALLSYMS_ALL depends on KALLSYMS and DEBUG_KERNEL, so we cannot
> just select KALLSYMS_ALL.
> 
> Anyway, this feels messy, and we do not seem to really need KALLSYMS_ALL,
> so select KALLSYMS instead.
> 
> Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> ---
>  drivers/mtd/ubi/Kconfig |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mtd/ubi/Kconfig b/drivers/mtd/ubi/Kconfig
> index 6abeb4f..4dcc752 100644
> --- a/drivers/mtd/ubi/Kconfig
> +++ b/drivers/mtd/ubi/Kconfig
> @@ -56,7 +56,7 @@ config MTD_UBI_DEBUG
>  	bool "UBI debugging"
>  	depends on SYSFS
>  	select DEBUG_FS
> -	select KALLSYMS_ALL if KALLSYMS && DEBUG_KERNEL
> +	select KALLSYMS
>  	help
>  	  This option enables UBI debugging.
>  
> -- 

Acked-by: Randy Dunlap <randy.dunlap@oracle.com>

Thanks.

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH 0/2] do not select KALLSYMS_ALL

[Paulo Marques]

Artem Bityutskiy wrote:
> [...]
> I personally think KALLSYMS_ALL should be just merged with KALLSYMS and
> disappear - we should have only one option. CONFIG_KALLSYMS_EXTRA_PASS should
> die as well.

That sounds a little too extreme...

KALLSYMS is useful for most kernels, since it provides nice readable
stack dumps for panics and BUG's.

KALLSYMS_ALL adds a lot of extra symbols that can be useful mostly to
development kernels and shouldn't be used to add unnecessary bloat to
user kernels.

Now as for CONFIG_KALLSYMS_EXTRA_PASS: to build the kallsyms table, the
build process first links a kernel image with an empty kallsyms table
and use that to fetch information for all the symbols.

It then uses that information to build the table with the right size,
and links it again. If everything goes ok, this new version as all the
symbols in the correct places and the final table can be built with the
correct addresses.

The final linking should produce the same result as only the data on the
kallsyms table changed, but not its size.

However, there have been bugs in the past with section alignments and
symbol reordering for symbols with the same address, etc., etc. that
make this final table not have the exact same size, and the build fails
with an inconsistent kallsyms data message. At this point, the user can
turn on the CONFIG_KALLSYMS_EXTRA_PASS and temporarily solve the problem
while the developers find the correct fix. Without this option, in this
situation the kernel would simply fail the compilation.

All this has been stable for a while and this option hasn't been needed
recently (AFAIK), but if there is some bug in some new binutils or
something, the option might be needed again.

-- 
Paulo Marques - www.grupopie.com

"Who is general Failure and why is he reading my disk?"

Re: [PATCH 0/2] do not select KALLSYMS_ALL

[Artem Bityutskiy]

On Thu, 2011-03-31 at 15:18 +0100, Paulo Marques wrote:
> Artem Bityutskiy wrote:
> > [...]
> > I personally think KALLSYMS_ALL should be just merged with KALLSYMS and
> > disappear - we should have only one option. CONFIG_KALLSYMS_EXTRA_PASS should
> > die as well.
> 
> That sounds a little too extreme...
> 
> KALLSYMS is useful for most kernels, since it provides nice readable
> stack dumps for panics and BUG's.
> 
> KALLSYMS_ALL adds a lot of extra symbols that can be useful mostly to
> development kernels and shouldn't be used to add unnecessary bloat to
> user kernels.

OK, thanks.

> Now as for CONFIG_KALLSYMS_EXTRA_PASS: to build the kallsyms table, the
> build process first links a kernel image with an empty kallsyms table
> and use that to fetch information for all the symbols.
> 
> It then uses that information to build the table with the right size,
> and links it again. If everything goes ok, this new version as all the
> symbols in the correct places and the final table can be built with the
> correct addresses.
> 
> The final linking should produce the same result as only the data on the
> kallsyms table changed, but not its size.
> 
> However, there have been bugs in the past with section alignments and
> symbol reordering for symbols with the same address, etc., etc. that
> make this final table not have the exact same size, and the build fails
> with an inconsistent kallsyms data message. At this point, the user can
> turn on the CONFIG_KALLSYMS_EXTRA_PASS and temporarily solve the problem
> while the developers find the correct fix. Without this option, in this
> situation the kernel would simply fail the compilation.
> 
> All this has been stable for a while and this option hasn't been needed
> recently (AFAIK), but if there is some bug in some new binutils or
> something, the option might be needed again.

Thanks for explanation!

But... why on earth this option is in Kconfig then, if this is only
about extra pass during the kernel _compilation_ ? This and the vague
help message in Kconfig help section are very misleading. This should
not be in Kconfig at all then, it should be purely a Makefile thing!

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH 0/2] do not select KALLSYMS_ALL

[Artem Bityutskiy]

On Thu, 2011-03-31 at 15:18 +0100, Paulo Marques wrote:
> Artem Bityutskiy wrote:
> > [...]
> > I personally think KALLSYMS_ALL should be just merged with KALLSYMS and
> > disappear - we should have only one option. CONFIG_KALLSYMS_EXTRA_PASS should
> > die as well.
> 
> That sounds a little too extreme...
> 
> KALLSYMS is useful for most kernels, since it provides nice readable
> stack dumps for panics and BUG's.
> 
> KALLSYMS_ALL adds a lot of extra symbols that can be useful mostly to
> development kernels and shouldn't be used to add unnecessary bloat to
> user kernels.

Well, ok, I've measured how much is this "a lot". On an embedded arm
platform this makes the kernel only 1.5% larger.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH 0/2] do not select KALLSYMS_ALL

[Artem Bityutskiy]

On Fri, 2011-04-01 at 17:56 +0300, Artem Bityutskiy wrote:
> Well, ok, I've measured how much is this "a lot". On an embedded arm
> platform this makes the kernel only 1.5% larger. 

But in absolute numbers this is 70KiB in my case, which is indeed
considerable amount. So, I agree that it makes sense to keep this as a
separate option.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH 0/2] do not select KALLSYMS_ALL

[Ricard Wanderlof]

On Fri, 1 Apr 2011, Artem Bityutskiy wrote:

> > On Fri, 2011-04-01 at 17:56 +0300, Artem Bityutskiy wrote:
> > Well, ok, I've measured how much is this "a lot". On an embedded arm
> > platform this makes the kernel only 1.5% larger. 
>
> But in absolute numbers this is 70KiB in my case, which is indeed
> considerable amount. So, I agree that it makes sense to keep this as a
> separate option.

Yes there are platforms where space is tight and 70 KiB could make or 
break a given setup.

/Ricard
-- 
Ricard Wolf Wanderlöf                           ricardw(at)axis.com
Axis Communications AB, Lund, Sweden            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30

Re: [PATCH 0/2] do not select KALLSYMS_ALL

[Paulo Marques]

Artem Bityutskiy wrote:
> On Fri, 2011-04-01 at 17:56 +0300, Artem Bityutskiy wrote:
>> Well, ok, I've measured how much is this "a lot". On an embedded arm
>> platform this makes the kernel only 1.5% larger. 
> 
> But in absolute numbers this is 70KiB in my case, which is indeed
> considerable amount. So, I agree that it makes sense to keep this as a
> separate option.

Yes, and this depends a lot on the kernel configuration options you've
selected (and that 1.5% of the kernel size, might be 50%~100% of the
kallsyms table).

IIRC, I had configurations where the KALLSYMS_ALL option was increasing
the kallsyms table from something like 150kB to above 500kB (before
compression). This was a long time ago though and I can't say exactly
what was the configuration that made this happen.

As for the CONFIG_KALLSYMS_EXTRA_PASS, I think the original idea (it was
not mine) was that it should be off by default and then the user would
need to turn it on when something went wrong.

With an automatic makefile mechanism, the problem would go unnoticed and
it just wouldn't be fixed, increasing the kernel compile time for
everyone who hits the same troublesome configuration.

-- 
Paulo Marques - www.grupopie.com

"As far as we know, our computer has never had an undetected error."
Weisert

Re: [PATCH 0/2] do not select KALLSYMS_ALL

[Artem Bityutskiy]

On Fri, 2011-04-01 at 16:34 +0100, Paulo Marques wrote:
> Artem Bityutskiy wrote:
> > On Fri, 2011-04-01 at 17:56 +0300, Artem Bityutskiy wrote:
> >> Well, ok, I've measured how much is this "a lot". On an embedded arm
> >> platform this makes the kernel only 1.5% larger. 
> > 
> > But in absolute numbers this is 70KiB in my case, which is indeed
> > considerable amount. So, I agree that it makes sense to keep this as a
> > separate option.
> 
> Yes, and this depends a lot on the kernel configuration options you've
> selected (and that 1.5% of the kernel size, might be 50%~100% of the
> kallsyms table).
> 
> IIRC, I had configurations where the KALLSYMS_ALL option was increasing
> the kallsyms table from something like 150kB to above 500kB (before
> compression). This was a long time ago though and I can't say exactly
> what was the configuration that made this happen.

Yes, thanks, I agree that having 2 separate options is OK.

> As for the CONFIG_KALLSYMS_EXTRA_PASS, I think the original idea (it was
> not mine) was that it should be off by default and then the user would
> need to turn it on when something went wrong.

Yes, but my point is that this feature is about the build and it does
not affect run-time, so it should not be in Kconfig.

> With an automatic makefile mechanism, the problem would go unnoticed and
> it just wouldn't be fixed, increasing the kernel compile time for
> everyone who hits the same troublesome configuration.

Yes, the build system may print a message:

Your symbols table is screwed, this is a bug, report about it. As a
temporary workaround use "make KALLSYMS_EXTRA_PASS=1"

Or something like that.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH 0/2] do not select KALLSYMS_ALL

[Paulo Marques]

Artem Bityutskiy wrote:
> On Fri, 2011-04-01 at 16:34 +0100, Paulo Marques wrote:
>>[...]
>> With an automatic makefile mechanism, the problem would go unnoticed and
>> it just wouldn't be fixed, increasing the kernel compile time for
>> everyone who hits the same troublesome configuration.
> 
> Yes, the build system may print a message:
> 
> Your symbols table is screwed, this is a bug, report about it. As a
> temporary workaround use "make KALLSYMS_EXTRA_PASS=1"
> 
> Or something like that.

Yes, I think that would work too, and it would even prevent the option
from being on for no good reason.

-- 
Paulo Marques - www.grupopie.com

"Nostalgia isn't what it used to be."

Re: [PATCH 0/2] do not select KALLSYMS_ALL

[Artem Bityutskiy]

On Fri, 2011-04-01 at 16:54 +0100, Paulo Marques wrote:
> Artem Bityutskiy wrote:
> > On Fri, 2011-04-01 at 16:34 +0100, Paulo Marques wrote:
> >>[...]
> >> With an automatic makefile mechanism, the problem would go unnoticed and
> >> it just wouldn't be fixed, increasing the kernel compile time for
> >> everyone who hits the same troublesome configuration.
> > 
> > Yes, the build system may print a message:
> > 
> > Your symbols table is screwed, this is a bug, report about it. As a
> > temporary workaround use "make KALLSYMS_EXTRA_PASS=1"
> > 
> > Or something like that.
> 
> Yes, I think that would work too, and it would even prevent the option
> from being on for no good reason.

I'll try to find some time and cook a patch.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)