[PATCH 1/2] Kconfig: improve KALLSYMS_ALL documentation

[PATCH 1/2] Kconfig: improve KALLSYMS_ALL documentation

[Artem Bityutskiy]

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

Dumb users like myself are not able to grasp from the existing KALLSYMS_ALL
documentation that this option is not what they need. Improve the help
message and make it clearer that KALLSYMS is enough in the majority of
use cases, and KALLSYMS_ALL should really be used very rarely.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Cc: Paulo Marques <pmarques@grupopie.com>
Cc: Michal Marek <mmarek@suse.cz>

---
 init/Kconfig |   18 ++++++++++++------
 1 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/init/Kconfig b/init/Kconfig
index 56240e7..dae5297 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -968,12 +968,18 @@ config KALLSYMS_ALL
 	bool "Include all symbols in kallsyms"
 	depends on DEBUG_KERNEL && KALLSYMS
 	help
-	   Normally kallsyms only contains the symbols of functions, for nicer
-	   OOPS messages.  Some debuggers can use kallsyms for other
-	   symbols too: say Y here to include all symbols, if you need them 
-	   and you don't care about adding 300k to the size of your kernel.
-
-	   Say N.
+	   Normally kallsyms only contains the symbols of functions for nicer
+	   OOPS messages and backtraces(i.e., symbols from the text and inittext
+	   sections). This is sufficient for most cases. And only in very rare
+	   cases (e.g., when a debugger is used) all symbols are required (e.g.,
+	   names of variables from the data sections, etc).
+
+	   This option makes sure that all symbols are loaded into the kernel
+	   image (i.e., symbols from all sections) in cost of increased kernel
+	   size (depending on the kernel configuration, it may be 300KiB or
+	   something like this).
+
+	   Say N unless you really need all symbols.
 
 config KALLSYMS_EXTRA_PASS
 	bool "Do an extra kallsyms pass"
-- 
1.7.2.3

[PATCH 2/2] kbuild: move KALLSYMS_EXTRA_PASS from Kconfig to Makefile

[Artem Bityutskiy]

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

At the moment we have the CONFIG_KALLSYMS_EXTRA_PASS Kconfig switch,
which users can enable or disable while configuring the kernel. This
option is then used by 'make' to determine whether an extra kallsyms
pass is needed or not.

However, this approach is not nice and confusing, and this patch moves
CONFIG_KALLSYMS_EXTRA_PASS from Kconfig to Makefile instead. The
rationale is below.

1. CONFIG_KALLSYMS_EXTRA_PASS is really about the build time, not
   run-time. There is no real need for it to be in Kconfig. It is
   just an additional work-around which should be used only in rare
   cases, when someone breaks kallsyms, so Kbuild/Makefile is much
   better place for this option.
2. Grepping CONFIG_KALLSYMS_EXTRA_PASS shows that many defconfigs have
   it enabled, probably not because they try to work-around a kallsyms
   bug, but just because the Kconfig help text is confusing and does
   not really make it clear that this option should not be used unless
   except when kallsyms is broken.
3. And since many people have CONFIG_KALLSYMS_EXTRA_PASS enabled in
   their Kconfig, we do might fail to notice kallsyms bugs in time. E.g.,
   many testers use "make allyesconfig" to test builds, which will enable
   CONFIG_KALLSYMS_EXTRA_PASS and kallsyms breakage will not be noticed.

To address that, this patch:

1. Kills CONFIG_KALLSYMS_EXTRA_PASS
2. Changes Makefile so that people can use "make KALLSYMS_EXTRA_PASS=1"
   to enable the extra pass if needed.
3. By default KALLSYMS_EXTRA_PASS is disabled and if kallsyms has issues,
   "make" should print a warning and suggest using KALLSYMS_EXTRA_PASS
4. Add "make help" entry for KALLSYMS_EXTRA_PASS

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Cc: Paulo Marques <pmarques@grupopie.com>
Cc: Michal Marek <mmarek@suse.cz>
---
 Makefile     |   13 ++++++++-----
 init/Kconfig |   12 ------------
 2 files changed, 8 insertions(+), 17 deletions(-)

diff --git a/Makefile b/Makefile
index ba7a55c..b66a43a 100644
--- a/Makefile
+++ b/Makefile
@@ -797,15 +797,16 @@ ifdef CONFIG_KALLSYMS
 # o The correct .tmp_kallsyms2.o is linked into the final vmlinux.
 # o Verify that the System.map from vmlinux matches the map from
 #   .tmp_vmlinux2, just in case we did not generate kallsyms correctly.
-# o If CONFIG_KALLSYMS_EXTRA_PASS is set, do an extra pass using
+# o If 'make KALLSYMS_EXTRA_PASS=1" was used, do an extra pass using
 #   .tmp_vmlinux3 and .tmp_kallsyms3.o.  This is only meant as a
 #   temporary bypass to allow the kernel to be built while the
 #   maintainers work out what went wrong with kallsyms.
 
-ifdef CONFIG_KALLSYMS_EXTRA_PASS
-last_kallsyms := 3
-else
 last_kallsyms := 2
+ifeq ("$(origin KALLSYMS_EXTRA_PASS)", "command line")
+	ifneq (KALLSYMS_EXTRA_PASS,0)
+		last_kallsyms := 3
+	endif
 endif
 
 kallsyms.o := .tmp_kallsyms$(last_kallsyms).o
@@ -816,7 +817,8 @@ define verify_kallsyms
 	  $(cmd_sysmap) .tmp_vmlinux$(last_kallsyms) .tmp_System.map
 	$(Q)cmp -s System.map .tmp_System.map ||                             \
 		(echo Inconsistent kallsyms data;                            \
-		 echo Try setting CONFIG_KALLSYMS_EXTRA_PASS;                \
+		 echo This is a bug - please report about it;                \
+		 echo Try "make KALLSYMS_EXTRA_PASS=1" as a workaround;      \
 		 rm .tmp_kallsyms* ; /bin/false )
 endef
 
@@ -1268,6 +1270,7 @@ help:
 	@echo  '  make C=1   [targets] Check all c source with $$CHECK (sparse by default)'
 	@echo  '  make C=2   [targets] Force check of all c source with $$CHECK'
 	@echo  '  make W=1   [targets] Enable extra gcc checks'
+	@echo  '  make KALLSYMS_EXTRA_PASS=1 [targets] Temporary workaround for kallsyms bugs'
 	@echo  ''
 	@echo  'Execute "make" or "make all" to build all targets marked with [*] '
 	@echo  'For further info see the ./README file'
diff --git a/init/Kconfig b/init/Kconfig
index dae5297..8c16c19 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -981,18 +981,6 @@ config KALLSYMS_ALL
 
 	   Say N unless you really need all symbols.
 
-config KALLSYMS_EXTRA_PASS
-	bool "Do an extra kallsyms pass"
-	depends on KALLSYMS
-	help
-	   If kallsyms is not working correctly, the build will fail with
-	   inconsistent kallsyms data.  If that occurs, log a bug report and
-	   turn on KALLSYMS_EXTRA_PASS which should result in a stable build.
-	   Always say N here unless you find a bug in kallsyms, which must be
-	   reported.  KALLSYMS_EXTRA_PASS is only a temporary workaround while
-	   you wait for kallsyms to be fixed.
-
-
 config HOTPLUG
 	bool "Support for hot-pluggable devices" if EXPERT
 	default y
-- 
1.7.2.3

Re: [PATCH 2/2] kbuild: move KALLSYMS_EXTRA_PASS from Kconfig to Makefile

[Michal Marek]

On 4.4.2011 12:40, Artem Bityutskiy wrote:
> From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> 
> At the moment we have the CONFIG_KALLSYMS_EXTRA_PASS Kconfig switch,
> which users can enable or disable while configuring the kernel. This
> option is then used by 'make' to determine whether an extra kallsyms
> pass is needed or not.
> 
> However, this approach is not nice and confusing, and this patch moves
> CONFIG_KALLSYMS_EXTRA_PASS from Kconfig to Makefile instead. The
> rationale is below.
> 
> 1. CONFIG_KALLSYMS_EXTRA_PASS is really about the build time, not
>    run-time. There is no real need for it to be in Kconfig. It is
>    just an additional work-around which should be used only in rare
>    cases, when someone breaks kallsyms, so Kbuild/Makefile is much
>    better place for this option.
> 2. Grepping CONFIG_KALLSYMS_EXTRA_PASS shows that many defconfigs have
>    it enabled, probably not because they try to work-around a kallsyms
>    bug, but just because the Kconfig help text is confusing and does
>    not really make it clear that this option should not be used unless
>    except when kallsyms is broken.
> 3. And since many people have CONFIG_KALLSYMS_EXTRA_PASS enabled in
>    their Kconfig, we do might fail to notice kallsyms bugs in time. E.g.,
>    many testers use "make allyesconfig" to test builds, which will enable
>    CONFIG_KALLSYMS_EXTRA_PASS and kallsyms breakage will not be noticed.
> 
> To address that, this patch:
> 
> 1. Kills CONFIG_KALLSYMS_EXTRA_PASS
> 2. Changes Makefile so that people can use "make KALLSYMS_EXTRA_PASS=1"
>    to enable the extra pass if needed.
> 3. By default KALLSYMS_EXTRA_PASS is disabled and if kallsyms has issues,
>    "make" should print a warning and suggest using KALLSYMS_EXTRA_PASS
> 4. Add "make help" entry for KALLSYMS_EXTRA_PASS
> 
> Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> Cc: Paulo Marques <pmarques@grupopie.com>
> Cc: Michal Marek <mmarek@suse.cz>
> ---
>  Makefile     |   13 ++++++++-----
>  init/Kconfig |   12 ------------
>  2 files changed, 8 insertions(+), 17 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index ba7a55c..b66a43a 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -797,15 +797,16 @@ ifdef CONFIG_KALLSYMS
>  # o The correct .tmp_kallsyms2.o is linked into the final vmlinux.
>  # o Verify that the System.map from vmlinux matches the map from
>  #   .tmp_vmlinux2, just in case we did not generate kallsyms correctly.
> -# o If CONFIG_KALLSYMS_EXTRA_PASS is set, do an extra pass using
> +# o If 'make KALLSYMS_EXTRA_PASS=1" was used, do an extra pass using
>  #   .tmp_vmlinux3 and .tmp_kallsyms3.o.  This is only meant as a
>  #   temporary bypass to allow the kernel to be built while the
>  #   maintainers work out what went wrong with kallsyms.
>  
> -ifdef CONFIG_KALLSYMS_EXTRA_PASS
> -last_kallsyms := 3
> -else
>  last_kallsyms := 2
> +ifeq ("$(origin KALLSYMS_EXTRA_PASS)", "command line")

I would drop the origin check here. We use it for single-letter
variables that could be used outside the kernel with a different
meaning, but if you export KALLSYMS_EXTRA_PASS, then you certainly mean
the kernel's KALLSYMS_EXTRA_PASS.

Michal

Re: [PATCH 2/2] kbuild: move KALLSYMS_EXTRA_PASS from Kconfig to Makefile

[Artem Bityutskiy]

On Mon, 2011-04-04 at 14:17 +0200, Michal Marek wrote:
> > -ifdef CONFIG_KALLSYMS_EXTRA_PASS
> > -last_kallsyms := 3
> > -else
> >  last_kallsyms := 2
> > +ifeq ("$(origin KALLSYMS_EXTRA_PASS)", "command line")
> 
> I would drop the origin check here. We use it for single-letter
> variables that could be used outside the kernel with a different
> meaning, but if you export KALLSYMS_EXTRA_PASS, then you certainly mean
> the kernel's KALLSYMS_EXTRA_PASS.

Sure, I'll wait if there are more requests and re-send. There is a
missed white-space in the first patch as well which I'll fix.

Thanks!

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

Re: [PATCH 2/2] kbuild: move KALLSYMS_EXTRA_PASS from Kconfig to Makefile

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 447 bytes --]

On Mon, 04 Apr 2011 13:40:51 +0300, Artem Bityutskiy said:

> 3. By default KALLSYMS_EXTRA_PASS is disabled and if kallsyms has issues,
>    "make" should print a warning and suggest using KALLSYMS_EXTRA_PASS

If the Makefile has enough information to print the warning, why can't it go
ahead and do the extra pass itself instead of issuing the warning?

(I'm obviously missing something here, as I'm foolishly posting to lkml before
caffeine ;)


[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]

Re: [PATCH 2/2] kbuild: move KALLSYMS_EXTRA_PASS from Kconfig to Makefile

[Artem Bityutskiy]

On Mon, 2011-04-04 at 08:45 -0400, Valdis.Kletnieks@vt.edu wrote:
> On Mon, 04 Apr 2011 13:40:51 +0300, Artem Bityutskiy said:
> 
> > 3. By default KALLSYMS_EXTRA_PASS is disabled and if kallsyms has issues,
> >    "make" should print a warning and suggest using KALLSYMS_EXTRA_PASS
> 
> If the Makefile has enough information to print the warning, why can't it go
> ahead and do the extra pass itself instead of issuing the warning?
> 
> (I'm obviously missing something here, as I'm foolishly posting to lkml before
> caffeine ;)

Good question, I thought to force people to send a bugreport.

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

Re: [PATCH 2/2] kbuild: move KALLSYMS_EXTRA_PASS from Kconfig to Makefile

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 1061 bytes --]

On Mon, 04 Apr 2011 15:47:10 +0300, Artem Bityutskiy said:
> On Mon, 2011-04-04 at 08:45 -0400, Valdis.Kletnieks@vt.edu wrote:
> > On Mon, 04 Apr 2011 13:40:51 +0300, Artem Bityutskiy said:
> >
> > > 3. By default KALLSYMS_EXTRA_PASS is disabled and if kallsyms has issues,
> > >    "make" should print a warning and suggest using KALLSYMS_EXTRA_PASS
> >
> > If the Makefile has enough information to print the warning, why can't it go
> > ahead and do the extra pass itself instead of issuing the warning?
> >
> > (I'm obviously missing something here, as I'm foolishly posting to lkml before
> > caffeine ;)
>
> Good question, I thought to force people to send a bugreport.

When was the last time we actually saw such a bug report?  I'm suspecting that
all the affected people just set the variable and get on with their lives.  So
we should either just make it automatic if that's what people are going to do,
or we rip the entire "EXTRA_PASS' stuff out forcibly and *make* people fix the
actual underlying issue that causes the extra pass to be needed...


[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]

Re: [PATCH 2/2] kbuild: move KALLSYMS_EXTRA_PASS from Kconfig to Makefile

[Artem Bityutskiy]

On Mon, 2011-04-04 at 11:22 -0400, Valdis.Kletnieks@vt.edu wrote:
> On Mon, 04 Apr 2011 15:47:10 +0300, Artem Bityutskiy said:
> > On Mon, 2011-04-04 at 08:45 -0400, Valdis.Kletnieks@vt.edu wrote:
> > > On Mon, 04 Apr 2011 13:40:51 +0300, Artem Bityutskiy said:
> > >
> > > > 3. By default KALLSYMS_EXTRA_PASS is disabled and if kallsyms has issues,
> > > >    "make" should print a warning and suggest using KALLSYMS_EXTRA_PASS
> > >
> > > If the Makefile has enough information to print the warning, why can't it go
> > > ahead and do the extra pass itself instead of issuing the warning?
> > >
> > > (I'm obviously missing something here, as I'm foolishly posting to lkml before
> > > caffeine ;)
> >
> > Good question, I thought to force people to send a bugreport.
> 
> When was the last time we actually saw such a bug report?  I'm suspecting that
> all the affected people just set the variable and get on with their lives.  So
> we should either just make it automatic if that's what people are going to do,
> or we rip the entire "EXTRA_PASS' stuff out forcibly and *make* people fix the
> actual underlying issue that causes the extra pass to be needed...

I do not know when this bug was last seen, but I'd not remove this
"extra pass" work around so far. WRT automatic fix-up - I do not have
strong opinion here, but Paulo Marques pointed in another thread that
due to various alignment issues the bug is tricky and may come back. So
probably removing working code is not very wise. Thus, it seems the way
I proposed in this patch is a good balance.

But anyway, if Michal Marek requests to change this, I'll do this (he is
the Maintainer).

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

Re: [PATCH 2/2] kbuild: move KALLSYMS_EXTRA_PASS from Kconfig to Makefile

[Paulo Marques]

Artem Bityutskiy wrote:
> On Mon, 2011-04-04 at 11:22 -0400, Valdis.Kletnieks@vt.edu wrote:
>> [...]
>> When was the last time we actually saw such a bug report?  I'm suspecting that
>> all the affected people just set the variable and get on with their lives.  So
>> we should either just make it automatic if that's what people are going to do,
>> or we rip the entire "EXTRA_PASS' stuff out forcibly and *make* people fix the
>> actual underlying issue that causes the extra pass to be needed...
> 
> I do not know when this bug was last seen, but I'd not remove this
> "extra pass" work around so far. WRT automatic fix-up - I do not have
> strong opinion here, but Paulo Marques pointed in another thread that
> due to various alignment issues the bug is tricky and may come back. So
> probably removing working code is not very wise. Thus, it seems the way
> I proposed in this patch is a good balance.
> 
> But anyway, if Michal Marek requests to change this, I'll do this (he is
> the Maintainer).

Doing a search on a mail archive for "Inconsistent kallsyms data"
results in a dozen or so threads, and the last one seems to be on alpha
in 2008.

Not very frequent for certain, but not in the "never been seen" category
either.

-- 
Paulo Marques - www.grupopie.com

"All I ask is a chance to prove that money can't make me happy."

Re: [PATCH 2/2] kbuild: move KALLSYMS_EXTRA_PASS from Kconfig to Makefile

[Artem Bityutskiy]

On Mon, 2011-04-04 at 08:45 -0400, Valdis.Kletnieks@vt.edu wrote:
> If the Makefile has enough information to print the warning, why can't it go
> ahead and do the extra pass itself instead of issuing the warning?

Actually I need to dig and check if make can really always spot all
errors.

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

Re: [PATCH 1/2] Kconfig: improve KALLSYMS_ALL documentation

[Randy Dunlap]

On Mon,  4 Apr 2011 13:40:50 +0300 Artem Bityutskiy wrote:

> From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> 
> Dumb users like myself are not able to grasp from the existing KALLSYMS_ALL
> documentation that this option is not what they need. Improve the help
> message and make it clearer that KALLSYMS is enough in the majority of
> use cases, and KALLSYMS_ALL should really be used very rarely.
> 
> Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> Cc: Paulo Marques <pmarques@grupopie.com>
> Cc: Michal Marek <mmarek@suse.cz>
> 
> ---
>  init/Kconfig |   18 ++++++++++++------
>  1 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/init/Kconfig b/init/Kconfig
> index 56240e7..dae5297 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -968,12 +968,18 @@ config KALLSYMS_ALL
>  	bool "Include all symbols in kallsyms"
>  	depends on DEBUG_KERNEL && KALLSYMS
>  	help
> -	   Normally kallsyms only contains the symbols of functions, for nicer
> -	   OOPS messages.  Some debuggers can use kallsyms for other
> -	   symbols too: say Y here to include all symbols, if you need them 
> -	   and you don't care about adding 300k to the size of your kernel.
> -
> -	   Say N.
> +	   Normally kallsyms only contains the symbols of functions for nicer
> +	   OOPS messages and backtraces(i.e., symbols from the text and inittext

add space:                   backtraces (i.e.,

> +	   sections). This is sufficient for most cases. And only in very rare
> +	   cases (e.g., when a debugger is used) all symbols are required (e.g.,
> +	   names of variables from the data sections, etc).
> +
> +	   This option makes sure that all symbols are loaded into the kernel
> +	   image (i.e., symbols from all sections) in cost of increased kernel
> +	   size (depending on the kernel configuration, it may be 300KiB or
> +	   something like this).
> +
> +	   Say N unless you really need all symbols.
>  
>  config KALLSYMS_EXTRA_PASS
>  	bool "Do an extra kallsyms pass"
> -- 
> 1.7.2.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


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