Re: Fast LKM symbol resolution

Re: Fast LKM symbol resolution

[Alan Jenkins]

Here's my latest code using binary search for symbol resolution.  This
version has benefited from a little more testing (heh), in particular
running on a simulated ARM system.

    web: <http://github.com/sourcejedi/linux-2.6/commits/module-V2-beta1>

    git clone git://github.com/sourcejedi/linux-2.6.git module-V2-beta1

As before, it saves 0.3s boot time for a modular kernel on my netbook
(630 Mhz Celeron M).  On that system it represents something like 90%
of the maximum possible speedup.  It is claimed that slower systems
would benefit from a further speedup (e.g. using hash tables instead),
but we don't have any numbers to illustrate this yet.


Changes since the original patches were posted:

1) The sorted version of the exports is now generated in
arch-independent assembly language, as .tmp-exports-asm.S.  It uses
similar pseudo-instructions to those in the current .tmp-kallsyms.S.

Previously the sorted exports were generated in C, but the compiler
outputted them in reverse order.  This was made to work by reversing
the comparison used in binary search, but it was a brittle hack.

This means mod_export.h now includes two implementations of
EXPORT_SYMBOL; one for general use in C code, and one for use in
.tmp-exports-asm.S.

2) In vmlinux.lds.h, the unsorted exports are now discarded in the
DISCARDS macro.

Previouslly they were discarded before including the sections of
sorted exports.  This broke the ARM build by discarding some unrelated
symbols too early.  It appears that a) the position of /DISCARD/
directives is signficant; b) all /DISCARD/ directives are merged
together and applied at the point of the first /DISCARD/ directive.

3) Now lightly tested on ARM (using qemu)

To build on ARM it was necessary to remove EXPORT_ALIAS, used in
armksyms.c for the sole purpose of allowing out of tree floating point
emulation code to be loaded as a module.  I have posted this change on
the ARM list and received no comments either way.

4) Build-tested on blackfin (a MODULE_SYMBOL_PREFIX arch)

This required that MODULE_SYMBOL_PREFIX be defined via a Kconfig
option, CONFIG_HAVE_SYMBOL_PREFIX.  Up until now it has been defined
in <asm/module.h> - but this also includes C declarations, which made
it impossible to use in .tmp-exports-asm.S.

Alan

Re: Fast LKM symbol resolution

[Greg KH]

On Mon, Nov 02, 2009 at 04:52:47PM +0000, Alan Jenkins wrote:
> Here's my latest code using binary search for symbol resolution.  This
> version has benefited from a little more testing (heh), in particular
> running on a simulated ARM system.
> 
>     web: <http://github.com/sourcejedi/linux-2.6/commits/module-V2-beta1>
> 
>     git clone git://github.com/sourcejedi/linux-2.6.git module-V2-beta1

Care to post actual patches?

thanks,

greg k-h

[PATCH 0/10] module: Speed up symbol resolution during module loading (using binary search)

[Alan Jenkins]

GKH: Care to post actual patches?

Sure.

Changelog as per original post
<http://article.gmane.org/gmane.linux.kbuild.devel/3983>

Regards
Alan

Re: [PATCH 0/10] module: Speed up symbol resolution during module loading (using binary search)

[Greg KH]

On Tue, Nov 03, 2009 at 10:06:12AM +0000, Alan Jenkins wrote:
> GKH: Care to post actual patches?
> 
> Sure.
> 
> Changelog as per original post
> <http://article.gmane.org/gmane.linux.kbuild.devel/3983>

Nice stuff.  Becides the embedded people arguing among themselves, I see
no reason why you shouldn't submit this for inclusion in -next now.
Can you send this to Ingo for inclusion in the -tip tree?  Or should it
go through someone else's tree instead?

thanks,

greg k-h

Re: [PATCH 0/10] module: Speed up symbol resolution during module loading (using binary search)

[Rusty Russell]

On Wed, 4 Nov 2009 02:28:24 am Greg KH wrote:
> Can you send this to Ingo for inclusion in the -tip tree?  Or should it
> go through someone else's tree instead?

Gee, did the "module" prefix not supply sufficient hint? :)

I'd like an ack for the build and arm bits, but I'm not going to hold it
up if not.

Cheers,
Rusty.

[PATCH 01/10] ARM: use unified discard definition in linker script

[Alan Jenkins]

Commit 023bf6f "linker script: unify usage of discard definition"
changed the linker scripts for all architectures except for ARM.
I can find no discussion about this ommision, so here are the changes
for ARM.

These changes are exactly parallel to the ia64 case.

"ia64 is notable because it first throws away some ia64 specific
 subsections and then include the rest of the sections into the final
 image, so those sections must be discarded before the inclusion."

Not boot-tested.  In build testing, the modified linker script generated
an identical vmlinux file.

[I would like to be able to rely on this unified discard definition.
 I want to sort the kernel symbol tables to allow faster symbol
 resolution during module loading. The simplest way appears to be
 to generate sorted versions from vmlinux.o, link them in to vmlinux,
 _and discard the original unsorted tables_.

 This work is driven by my x86 netbook, but it is implemented at a
 generic level. It is possible it will benefit some ARM systems also.]

Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
Acked-by-without-testing: Tejun Heo <tj@kernel.org>
---
 arch/arm/kernel/vmlinux.lds.S |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/arch/arm/kernel/vmlinux.lds.S b/arch/arm/kernel/vmlinux.lds.S
index aecf87d..ec511d4 100644
--- a/arch/arm/kernel/vmlinux.lds.S
+++ b/arch/arm/kernel/vmlinux.lds.S
@@ -79,11 +79,11 @@ SECTIONS
 #endif
 	}
 
-	/DISCARD/ : {			/* Exit code and data		*/
-		EXIT_TEXT
-		EXIT_DATA
-		*(.exitcall.exit)
-		*(.discard)
+	/*
+	 * unwind exit sections must be discarded before the rest of the
+	 * unwind sections get included.
+	 */
+	/DISCARD/ : {
 		*(.ARM.exidx.exit.text)
 		*(.ARM.extab.exit.text)
 #ifndef CONFIG_HOTPLUG_CPU
@@ -271,6 +271,9 @@ SECTIONS
 	.stab.index 0 : { *(.stab.index) }
 	.stab.indexstr 0 : { *(.stab.indexstr) }
 	.comment 0 : { *(.comment) }
+
+	/* Default discards */
+	DISCARDS
 }
 
 /*
-- 
1.6.3.2

[PATCH 02/10] ARM: unexport symbols used to implement floating point emulation

[Alan Jenkins]

The Kconfigs for in-tree floating point emulation do not allow building
as modules. That leaves the Acorn FPEmulator module. I found two public
releases of this as a binary module for 2.1 and 2.2 kernels, optimized
for ARMV4.[1] If there is a resurgence of interest in this, the symbols
can always be re-exported.

This allows the EXPORT_SYMBOL_ALIAS() hack to be removed. The ulterior
motive here is that EXPORT_SYMBOL_ALIAS() makes it harder to sort the
resulting kernel symbol tables.  Sorted symbol tables will allow faster
symbol resolution during module loading.

Note that fp_send_sigs() and fp_printk() are simply aliases for existing
exports and add no obvious value.  Similarly fp_enter could easily be
renamed to kern_fp_enter at the point of definition. Therefore removing
EXPORT_SYMBOL_ALIAS will not serve as a material obstacle to re-adding
the exports should they be desired in future.

Build tested only.

[1] http://ftp.arm.linux.org.uk/pub/linux/arm/fpemulator/

Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
CC: Russell King <linux@arm.linux.org.uk>
---
 arch/arm/kernel/armksyms.c |   20 --------------------
 1 files changed, 0 insertions(+), 20 deletions(-)

diff --git a/arch/arm/kernel/armksyms.c b/arch/arm/kernel/armksyms.c
index 0e62770..8214bfe 100644
--- a/arch/arm/kernel/armksyms.c
+++ b/arch/arm/kernel/armksyms.c
@@ -48,27 +48,7 @@ extern void __aeabi_uidivmod(void);
 extern void __aeabi_ulcmp(void);
 
 extern void fpundefinstr(void);
-extern void fp_enter(void);
 
-/*
- * This has a special calling convention; it doesn't
- * modify any of the usual registers, except for LR.
- */
-#define EXPORT_CRC_ALIAS(sym) __CRC_SYMBOL(sym, "")
-
-#define EXPORT_SYMBOL_ALIAS(sym,orig)		\
- EXPORT_CRC_ALIAS(sym)				\
- static const struct kernel_symbol __ksymtab_##sym	\
-  __used __attribute__((section("__ksymtab"))) =	\
-    { (unsigned long)&orig, #sym };
-
-/*
- * floating point math emulator support.
- * These symbols will never change their calling convention...
- */
-EXPORT_SYMBOL_ALIAS(kern_fp_enter,fp_enter);
-EXPORT_SYMBOL_ALIAS(fp_printk,printk);
-EXPORT_SYMBOL_ALIAS(fp_send_sig,send_sig);
 
 EXPORT_SYMBOL(__backtrace);
 
-- 
1.6.3.2

[PATCH 03/10] module: extract __EXPORT_SYMBOL from module.h into mod_export.h

[Alan Jenkins]

The new header mod_export.h allows __EXPORT_SYMBOL to be used without
pulling in any function or variable declarations.  It will be used by
the build system to help sort the list of symbols exported by the
kernel.

Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
---
 include/linux/mod_export.h |   73 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/module.h     |   63 +-------------------------------------
 2 files changed, 74 insertions(+), 62 deletions(-)
 create mode 100644 include/linux/mod_export.h

diff --git a/include/linux/mod_export.h b/include/linux/mod_export.h
new file mode 100644
index 0000000..3d80057
--- /dev/null
+++ b/include/linux/mod_export.h
@@ -0,0 +1,73 @@
+#ifndef LINUX_MOD_EXPORT_H
+#define LINUX_MOD_EXPORT_H
+
+#include <linux/compiler.h>
+#include <asm/module.h>
+
+/* some toolchains uses a `_' prefix for all user symbols */
+#ifndef MODULE_SYMBOL_PREFIX
+#define MODULE_SYMBOL_PREFIX ""
+#endif
+
+struct kernel_symbol {
+	unsigned long value;
+	const char *name;
+};
+
+#ifdef CONFIG_MODULES
+#ifndef __GENKSYMS__
+#ifdef CONFIG_MODVERSIONS
+/* Mark the CRC weak since genksyms apparently decides not to
+ * generate a checksums for some symbols */
+#define __CRC_SYMBOL(sym, sec)					\
+	extern void *__crc_##sym __attribute__((weak));		\
+	static const unsigned long __kcrctab_##sym		\
+	__used							\
+	__attribute__((section("__kcrctab" sec), unused))	\
+	= (unsigned long) &__crc_##sym;
+#else
+#define __CRC_SYMBOL(sym, sec)
+#endif
+
+/* For every exported symbol, place a struct in the __ksymtab section */
+#define __EXPORT_SYMBOL(sym, sec)				\
+	extern typeof(sym) sym;					\
+	__CRC_SYMBOL(sym, sec)					\
+	static const char __kstrtab_##sym[]			\
+	__attribute__((section("__ksymtab_strings"), aligned(1))) \
+	= MODULE_SYMBOL_PREFIX #sym;                    	\
+	static const struct kernel_symbol __ksymtab_##sym	\
+	__used							\
+	__attribute__((section("__ksymtab" sec), unused))	\
+	= { (unsigned long)&sym, __kstrtab_##sym }
+
+#define EXPORT_SYMBOL(sym)					\
+	__EXPORT_SYMBOL(sym, "")
+
+#define EXPORT_SYMBOL_GPL(sym)					\
+	__EXPORT_SYMBOL(sym, "_gpl")
+
+#define EXPORT_SYMBOL_GPL_FUTURE(sym)				\
+	__EXPORT_SYMBOL(sym, "_gpl_future")
+
+#ifdef CONFIG_UNUSED_SYMBOLS
+#define EXPORT_UNUSED_SYMBOL(sym) __EXPORT_SYMBOL(sym, "_unused")
+#define EXPORT_UNUSED_SYMBOL_GPL(sym) __EXPORT_SYMBOL(sym, "_unused_gpl")
+#else
+#define EXPORT_UNUSED_SYMBOL(sym)
+#define EXPORT_UNUSED_SYMBOL_GPL(sym)
+#endif
+
+#endif /* __GENKSYMS__ */
+
+#else /* !CONFIG_MODULES */
+
+#define EXPORT_SYMBOL(sym)
+#define EXPORT_SYMBOL_GPL(sym)
+#define EXPORT_SYMBOL_GPL_FUTURE(sym)
+#define EXPORT_UNUSED_SYMBOL(sym)
+#define EXPORT_UNUSED_SYMBOL_GPL(sym)
+
+#endif /* CONFIG_MODULES */
+
+#endif /* LINUX_MOD_EXPORT_H */
diff --git a/include/linux/module.h b/include/linux/module.h
index 482efc8..b9e860a 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -15,6 +15,7 @@
 #include <linux/stringify.h>
 #include <linux/kobject.h>
 #include <linux/moduleparam.h>
+#include <linux/mod_export.h>
 #include <linux/tracepoint.h>
 
 #include <asm/local.h>
@@ -25,19 +26,8 @@
 /* Not Yet Implemented */
 #define MODULE_SUPPORTED_DEVICE(name)
 
-/* some toolchains uses a `_' prefix for all user symbols */
-#ifndef MODULE_SYMBOL_PREFIX
-#define MODULE_SYMBOL_PREFIX ""
-#endif
-
 #define MODULE_NAME_LEN MAX_PARAM_PREFIX_LEN
 
-struct kernel_symbol
-{
-	unsigned long value;
-	const char *name;
-};
-
 struct modversion_info
 {
 	unsigned long crc;
@@ -178,52 +168,6 @@ void *__symbol_get(const char *symbol);
 void *__symbol_get_gpl(const char *symbol);
 #define symbol_get(x) ((typeof(&x))(__symbol_get(MODULE_SYMBOL_PREFIX #x)))
 
-#ifndef __GENKSYMS__
-#ifdef CONFIG_MODVERSIONS
-/* Mark the CRC weak since genksyms apparently decides not to
- * generate a checksums for some symbols */
-#define __CRC_SYMBOL(sym, sec)					\
-	extern void *__crc_##sym __attribute__((weak));		\
-	static const unsigned long __kcrctab_##sym		\
-	__used							\
-	__attribute__((section("__kcrctab" sec), unused))	\
-	= (unsigned long) &__crc_##sym;
-#else
-#define __CRC_SYMBOL(sym, sec)
-#endif
-
-/* For every exported symbol, place a struct in the __ksymtab section */
-#define __EXPORT_SYMBOL(sym, sec)				\
-	extern typeof(sym) sym;					\
-	__CRC_SYMBOL(sym, sec)					\
-	static const char __kstrtab_##sym[]			\
-	__attribute__((section("__ksymtab_strings"), aligned(1))) \
-	= MODULE_SYMBOL_PREFIX #sym;                    	\
-	static const struct kernel_symbol __ksymtab_##sym	\
-	__used							\
-	__attribute__((section("__ksymtab" sec), unused))	\
-	= { (unsigned long)&sym, __kstrtab_##sym }
-
-#define EXPORT_SYMBOL(sym)					\
-	__EXPORT_SYMBOL(sym, "")
-
-#define EXPORT_SYMBOL_GPL(sym)					\
-	__EXPORT_SYMBOL(sym, "_gpl")
-
-#define EXPORT_SYMBOL_GPL_FUTURE(sym)				\
-	__EXPORT_SYMBOL(sym, "_gpl_future")
-
-
-#ifdef CONFIG_UNUSED_SYMBOLS
-#define EXPORT_UNUSED_SYMBOL(sym) __EXPORT_SYMBOL(sym, "_unused")
-#define EXPORT_UNUSED_SYMBOL_GPL(sym) __EXPORT_SYMBOL(sym, "_unused_gpl")
-#else
-#define EXPORT_UNUSED_SYMBOL(sym)
-#define EXPORT_UNUSED_SYMBOL_GPL(sym)
-#endif
-
-#endif
-
 enum module_state
 {
 	MODULE_STATE_LIVE,
@@ -541,11 +485,6 @@ extern void module_update_tracepoints(void);
 extern int module_get_iter_tracepoints(struct tracepoint_iter *iter);
 
 #else /* !CONFIG_MODULES... */
-#define EXPORT_SYMBOL(sym)
-#define EXPORT_SYMBOL_GPL(sym)
-#define EXPORT_SYMBOL_GPL_FUTURE(sym)
-#define EXPORT_UNUSED_SYMBOL(sym)
-#define EXPORT_UNUSED_SYMBOL_GPL(sym)
 
 /* Given an address, look for it in the exception tables. */
 static inline const struct exception_table_entry *
-- 
1.6.3.2

[PATCH 04/10] module: make MODULE_SYMBOL_PREFIX into a CONFIG option

[Alan Jenkins]

The next commit will require the use of MODULE_SYMBOL_PREFIX in
.tmp_exports-asm.S.  Currently it is mixed in with C structure
definitions in "asm/module.h".  Move the definition of this arch option
into Kconfig, so it can be easily accessed by any code.

This also lets modpost.c use the same definition.  Previously modpost
relied on a hardcoded list of architectures in mk_elfconfig.c.

A build test for blackfin, one of the two MODULE_SYMBOL_PREFIX archs,
showed the generated code was unchanged.  vmlinux was identical save
for build ids, and an apparently randomized suffix on a single "__key"
symbol in the kallsyms data).

Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
---
 arch/blackfin/Kconfig              |    1 +
 arch/blackfin/include/asm/module.h |    2 --
 arch/h8300/Kconfig                 |    1 +
 arch/h8300/include/asm/module.h    |    2 --
 include/linux/mod_export.h         |    6 ++----
 init/Kconfig                       |   11 +++++++++++
 scripts/mod/Makefile               |    2 +-
 scripts/mod/mk_elfconfig.c         |    9 ---------
 scripts/mod/modpost.c              |    4 ++++
 9 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/arch/blackfin/Kconfig b/arch/blackfin/Kconfig
index 9a01d44..6c99419 100644
--- a/arch/blackfin/Kconfig
+++ b/arch/blackfin/Kconfig
@@ -26,6 +26,7 @@ config BLACKFIN
 	select HAVE_KERNEL_BZIP2
 	select HAVE_KERNEL_LZMA
 	select HAVE_OPROFILE
+	select HAVE_SYMBOL_PREFIX
 	select ARCH_WANT_OPTIONAL_GPIOLIB
 
 config GENERIC_BUG
diff --git a/arch/blackfin/include/asm/module.h b/arch/blackfin/include/asm/module.h
index e3128df..81d8b90 100644
--- a/arch/blackfin/include/asm/module.h
+++ b/arch/blackfin/include/asm/module.h
@@ -1,8 +1,6 @@
 #ifndef _ASM_BFIN_MODULE_H
 #define _ASM_BFIN_MODULE_H
 
-#define MODULE_SYMBOL_PREFIX "_"
-
 #define Elf_Shdr        Elf32_Shdr
 #define Elf_Sym         Elf32_Sym
 #define Elf_Ehdr        Elf32_Ehdr
diff --git a/arch/h8300/Kconfig b/arch/h8300/Kconfig
index 9420648..cc03bbf 100644
--- a/arch/h8300/Kconfig
+++ b/arch/h8300/Kconfig
@@ -9,6 +9,7 @@ config H8300
 	bool
 	default y
 	select HAVE_IDE
+	select HAVE_SYMBOL_PREFIX
 
 config MMU
 	bool
diff --git a/arch/h8300/include/asm/module.h b/arch/h8300/include/asm/module.h
index de23231..8e46724 100644
--- a/arch/h8300/include/asm/module.h
+++ b/arch/h8300/include/asm/module.h
@@ -8,6 +8,4 @@ struct mod_arch_specific { };
 #define Elf_Sym Elf32_Sym
 #define Elf_Ehdr Elf32_Ehdr
 
-#define MODULE_SYMBOL_PREFIX "_"
-
 #endif /* _ASM_H8/300_MODULE_H */
diff --git a/include/linux/mod_export.h b/include/linux/mod_export.h
index 3d80057..56b817a 100644
--- a/include/linux/mod_export.h
+++ b/include/linux/mod_export.h
@@ -4,10 +4,8 @@
 #include <linux/compiler.h>
 #include <asm/module.h>
 
-/* some toolchains uses a `_' prefix for all user symbols */
-#ifndef MODULE_SYMBOL_PREFIX
-#define MODULE_SYMBOL_PREFIX ""
-#endif
+/* Some toolchains use a `_' prefix for all user symbols. */
+#define MODULE_SYMBOL_PREFIX CONFIG_SYMBOL_PREFIX
 
 struct kernel_symbol {
 	unsigned long value;
diff --git a/init/Kconfig b/init/Kconfig
index c7bac39..fe43d6d 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1171,6 +1171,17 @@ config MODULE_SRCVERSION_ALL
 
 endif # MODULES
 
+config HAVE_SYMBOL_PREFIX
+	bool
+	help
+	  Some arch toolchains use a `_' prefix for all user symbols.
+	  This option will be taken into account when loading modules.
+
+config SYMBOL_PREFIX
+	string
+	default "_" if HAVE_SYMBOL_PREFIX
+	default ""
+
 config INIT_ALL_POSSIBLE
 	bool
 	help
diff --git a/scripts/mod/Makefile b/scripts/mod/Makefile
index 11d69c3..ff954f8 100644
--- a/scripts/mod/Makefile
+++ b/scripts/mod/Makefile
@@ -8,7 +8,7 @@ modpost-objs	:= modpost.o file2alias.o sumversion.o
 $(obj)/modpost.o $(obj)/file2alias.o $(obj)/sumversion.o: $(obj)/elfconfig.h
 
 quiet_cmd_elfconfig = MKELF   $@
-      cmd_elfconfig = $(obj)/mk_elfconfig $(ARCH) < $< > $@
+      cmd_elfconfig = $(obj)/mk_elfconfig < $< > $@
 
 $(obj)/elfconfig.h: $(obj)/empty.o $(obj)/mk_elfconfig FORCE
 	$(call if_changed,elfconfig)
diff --git a/scripts/mod/mk_elfconfig.c b/scripts/mod/mk_elfconfig.c
index 6a96d47..639bca7 100644
--- a/scripts/mod/mk_elfconfig.c
+++ b/scripts/mod/mk_elfconfig.c
@@ -9,9 +9,6 @@ main(int argc, char **argv)
 	unsigned char ei[EI_NIDENT];
 	union { short s; char c[2]; } endian_test;
 
-	if (argc != 2) {
-		fprintf(stderr, "Error: no arch\n");
-	}
 	if (fread(ei, 1, EI_NIDENT, stdin) != EI_NIDENT) {
 		fprintf(stderr, "Error: input truncated\n");
 		return 1;
@@ -55,12 +52,6 @@ main(int argc, char **argv)
 	else
 		exit(1);
 
-	if ((strcmp(argv[1], "h8300") == 0)
-	    || (strcmp(argv[1], "blackfin") == 0))
-		printf("#define MODULE_SYMBOL_PREFIX \"_\"\n");
-	else
-		printf("#define MODULE_SYMBOL_PREFIX \"\"\n");
-
 	return 0;
 }
 
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 801a16a..3867481 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -15,8 +15,12 @@
 #include <stdio.h>
 #include <ctype.h>
 #include "modpost.h"
+#include "../../include/linux/autoconf.h"
 #include "../../include/linux/license.h"
 
+/* Some toolchains use a `_' prefix for all user symbols. */
+#define MODULE_SYMBOL_PREFIX CONFIG_SYMBOL_PREFIX
+
 /* Are we using CONFIG_MODVERSIONS? */
 int modversions = 0;
 /* Warn about undefined symbols? (do so if we have vmlinux) */
-- 
1.6.3.2

Re: [PATCH 04/10] module: make MODULE_SYMBOL_PREFIX into a CONFIG option

[Mike Frysinger]

On Tue, Nov 3, 2009 at 05:06, Alan Jenkins wrote:
> The next commit will require the use of MODULE_SYMBOL_PREFIX in
> .tmp_exports-asm.S.  Currently it is mixed in with C structure
> definitions in "asm/module.h".  Move the definition of this arch option
> into Kconfig, so it can be easily accessed by any code.
>
> This also lets modpost.c use the same definition.  Previously modpost
> relied on a hardcoded list of architectures in mk_elfconfig.c.

this should also let us push VMLINUX_SYMBOL() out of
arch/*/kernel/vmlinux.lds.S and into asm-generic/vmlinux.lds.h ...

> A build test for blackfin, one of the two MODULE_SYMBOL_PREFIX archs,
> showed the generated code was unchanged.  vmlinux was identical save
> for build ids, and an apparently randomized suffix on a single "__key"
> symbol in the kallsyms data).

when you get localized (static) namespace collisions, the linker
automatically does that

> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1171,6 +1171,17 @@ config MODULE_SRCVERSION_ALL
>
>  endif # MODULES
>
> +config HAVE_SYMBOL_PREFIX
> +       bool
> +       help
> +         Some arch toolchains use a `_' prefix for all user symbols.
> +         This option will be taken into account when loading modules.
> +
> +config SYMBOL_PREFIX
> +       string
> +       default "_" if HAVE_SYMBOL_PREFIX
> +       default ""

in practice, the symbol prefix is an underscore.  but there is no
technical limitation here -- the toolchain could use whatever prefix
they wanted

so if the Kconfig option was pushed to arch/*/Kconfig, we could drop
HAVE_SYMBOL_PREFIX and let the arch declare the exact SYMBOL_PREFIX
value itself
-mike

Re: [PATCH 04/10] module: make MODULE_SYMBOL_PREFIX into a CONFIG option

[Mike Frysinger]

On Tue, Nov 3, 2009 at 05:06, Alan Jenkins wrote:
> The next commit will require the use of MODULE_SYMBOL_PREFIX in
> .tmp_exports-asm.S.  Currently it is mixed in with C structure
> definitions in "asm/module.h".  Move the definition of this arch option
> into Kconfig, so it can be easily accessed by any code.
>
> This also lets modpost.c use the same definition.  Previously modpost
> relied on a hardcoded list of architectures in mk_elfconfig.c.

this should also let us push VMLINUX_SYMBOL() out of
arch/*/kernel/vmlinux.lds.S and into asm-generic/vmlinux.lds.h ...

> A build test for blackfin, one of the two MODULE_SYMBOL_PREFIX archs,
> showed the generated code was unchanged.  vmlinux was identical save
> for build ids, and an apparently randomized suffix on a single "__key"
> symbol in the kallsyms data).

when you get localized (static) namespace collisions, the linker
automatically does that

> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1171,6 +1171,17 @@ config MODULE_SRCVERSION_ALL
>
>  endif # MODULES
>
> +config HAVE_SYMBOL_PREFIX
> +       bool
> +       help
> +         Some arch toolchains use a `_' prefix for all user symbols.
> +         This option will be taken into account when loading modules.
> +
> +config SYMBOL_PREFIX
> +       string
> +       default "_" if HAVE_SYMBOL_PREFIX
> +       default ""

in practice, the symbol prefix is an underscore.  but there is no
technical limitation here -- the toolchain could use whatever prefix
they wanted

so if the Kconfig option was pushed to arch/*/Kconfig, we could drop
HAVE_SYMBOL_PREFIX and let the arch declare the exact SYMBOL_PREFIX
value itself
-mike

Re: [PATCH 04/10] module: make MODULE_SYMBOL_PREFIX into a CONFIG option

[Alan Jenkins]

Mike Frysinger wrote:
> On Tue, Nov 3, 2009 at 05:06, Alan Jenkins wrote:
>   
>> The next commit will require the use of MODULE_SYMBOL_PREFIX in
>> .tmp_exports-asm.S.  Currently it is mixed in with C structure
>> definitions in "asm/module.h".  Move the definition of this arch option
>> into Kconfig, so it can be easily accessed by any code.
>>
>> This also lets modpost.c use the same definition.  Previously modpost
>> relied on a hardcoded list of architectures in mk_elfconfig.c.
>>     
>
> this should also let us push VMLINUX_SYMBOL() out of
> arch/*/kernel/vmlinux.lds.S and into asm-generic/vmlinux.lds.h ...
>
>   
>> A build test for blackfin, one of the two MODULE_SYMBOL_PREFIX archs,
>> showed the generated code was unchanged.  vmlinux was identical save
>> for build ids, and an apparently randomized suffix on a single "__key"
>> symbol in the kallsyms data).
>>     
>
> when you get localized (static) namespace collisions, the linker
> automatically does that
>
>   
>> --- a/init/Kconfig
>> +++ b/init/Kconfig
>> @@ -1171,6 +1171,17 @@ config MODULE_SRCVERSION_ALL
>>
>>  endif # MODULES
>>
>> +config HAVE_SYMBOL_PREFIX
>> +       bool
>> +       help
>> +         Some arch toolchains use a `_' prefix for all user symbols.
>> +         This option will be taken into account when loading modules.
>> +
>> +config SYMBOL_PREFIX
>> +       string
>> +       default "_" if HAVE_SYMBOL_PREFIX
>> +       default ""
>>     
>
> in practice, the symbol prefix is an underscore.  but there is no
> technical limitation here -- the toolchain could use whatever prefix
> they wanted
>
> so if the Kconfig option was pushed to arch/*/Kconfig, we could drop
> HAVE_SYMBOL_PREFIX and let the arch declare the exact SYMBOL_PREFIX
> value itself
> -mike
>   

I don't think that's possible.

    #define VMLINUX_SYMBOL(_sym_) _##_sym_

I don't know any "unstringify" operation.  So I can't convert a string 
value of CONFIG_SYMBOL_PREFIX to the unquoted underscore we neeed for 
this macro.  The same applies for the SYM() macro I use.  Currently it 
assumes the prefix is a single underscore:

    #ifdef HAVE_SYMBOL_PREFIX
    #define __SYM(sym) _##sym
    #else
    #define __SYM(sym) sym
    #endif

If we positively want to keep the generality, I guess I should put 
MODULE_SYMBOL_PREFIX in a header file of it's own.  The disadvantage is 
that it makes it inaccessible to host programs again, like modpost 
(which currently hardcodes the list of affected architectures in 
mk_elfconfig.c).

Personally I favour "look, a small cleanup!" against "who knows what 
crazy things the next toolchain will do".  Of course I'm open to being 
outvoted by experience :).

Regards
Alan

Re: [PATCH 04/10] module: make MODULE_SYMBOL_PREFIX into a CONFIG option

[Mike Frysinger]

On Tue, Nov 3, 2009 at 07:16, Alan Jenkins wrote:
> Mike Frysinger wrote:
>> On Tue, Nov 3, 2009 at 05:06, Alan Jenkins wrote:
>>> The next commit will require the use of MODULE_SYMBOL_PREFIX in
>>> .tmp_exports-asm.S.  Currently it is mixed in with C structure
>>> definitions in "asm/module.h".  Move the definition of this arch option
>>> into Kconfig, so it can be easily accessed by any code.
>>>
>>> This also lets modpost.c use the same definition.  Previously modpost
>>> relied on a hardcoded list of architectures in mk_elfconfig.c.
>>
>> this should also let us push VMLINUX_SYMBOL() out of
>> arch/*/kernel/vmlinux.lds.S and into asm-generic/vmlinux.lds.h ...
>
> I don't think that's possible.
>
>   #define VMLINUX_SYMBOL(_sym_) _##_sym_
>
> I don't know any "unstringify" operation.  So I can't convert a string value
> of CONFIG_SYMBOL_PREFIX to the unquoted underscore we neeed for this macro.
>  The same applies for the SYM() macro I use.

let the build system do the unstringify operation.
qstrip = $(strip $(subst ",,$(1)))
CPPFLAGS_vmlinux.lds += -DVMLINUX_SYMBOL_PREFIX=$(call
qstrip,CONFIG_SYMBOL_PREFIX)

> If we positively want to keep the generality, I guess I should put
> MODULE_SYMBOL_PREFIX in a header file of it's own.  The disadvantage is that
> it makes it inaccessible to host programs again, like modpost (which
> currently hardcodes the list of affected architectures in mk_elfconfig.c).

having it in the arch Kconfig removes any and all possible
limitations, and it keeps the cruft out of the common init/Kconfig and
in the arch-specific Kconfig, and avoids a dead symbol
(HAVE_SYMBOL_PREFIX)
-mike

Re: [PATCH 04/10] module: make MODULE_SYMBOL_PREFIX into a CONFIG option

[Mike Frysinger]

On Tue, Nov 3, 2009 at 07:16, Alan Jenkins wrote:
> Mike Frysinger wrote:
>> On Tue, Nov 3, 2009 at 05:06, Alan Jenkins wrote:
>>> The next commit will require the use of MODULE_SYMBOL_PREFIX in
>>> .tmp_exports-asm.S.  Currently it is mixed in with C structure
>>> definitions in "asm/module.h".  Move the definition of this arch option
>>> into Kconfig, so it can be easily accessed by any code.
>>>
>>> This also lets modpost.c use the same definition.  Previously modpost
>>> relied on a hardcoded list of architectures in mk_elfconfig.c.
>>
>> this should also let us push VMLINUX_SYMBOL() out of
>> arch/*/kernel/vmlinux.lds.S and into asm-generic/vmlinux.lds.h ...
>
> I don't think that's possible.
>
>   #define VMLINUX_SYMBOL(_sym_) _##_sym_
>
> I don't know any "unstringify" operation.  So I can't convert a string value
> of CONFIG_SYMBOL_PREFIX to the unquoted underscore we neeed for this macro.
>  The same applies for the SYM() macro I use.

let the build system do the unstringify operation.
qstrip = $(strip $(subst ",,$(1)))
CPPFLAGS_vmlinux.lds += -DVMLINUX_SYMBOL_PREFIX=$(call
qstrip,CONFIG_SYMBOL_PREFIX)

> If we positively want to keep the generality, I guess I should put
> MODULE_SYMBOL_PREFIX in a header file of it's own.  The disadvantage is that
> it makes it inaccessible to host programs again, like modpost (which
> currently hardcodes the list of affected architectures in mk_elfconfig.c).

having it in the arch Kconfig removes any and all possible
limitations, and it keeps the cruft out of the common init/Kconfig and
in the arch-specific Kconfig, and avoids a dead symbol
(HAVE_SYMBOL_PREFIX)
-mike

Re: [PATCH 04/10] module: make MODULE_SYMBOL_PREFIX into a CONFIG option

[Paul Mundt]

On Tue, Nov 03, 2009 at 07:30:20AM -0500, Mike Frysinger wrote:
> On Tue, Nov 3, 2009 at 07:16, Alan Jenkins wrote:
> > Mike Frysinger wrote:
> >> On Tue, Nov 3, 2009 at 05:06, Alan Jenkins wrote:
> >>> The next commit will require the use of MODULE_SYMBOL_PREFIX in
> >>> .tmp_exports-asm.S. ??Currently it is mixed in with C structure
> >>> definitions in "asm/module.h". ??Move the definition of this arch option
> >>> into Kconfig, so it can be easily accessed by any code.
> >>>
> >>> This also lets modpost.c use the same definition. ??Previously modpost
> >>> relied on a hardcoded list of architectures in mk_elfconfig.c.
> >>
> >> this should also let us push VMLINUX_SYMBOL() out of
> >> arch/*/kernel/vmlinux.lds.S and into asm-generic/vmlinux.lds.h ...
> >
> > I don't think that's possible.
> >
> > ?? #define VMLINUX_SYMBOL(_sym_) _##_sym_
> >
> > I don't know any "unstringify" operation. ??So I can't convert a string value
> > of CONFIG_SYMBOL_PREFIX to the unquoted underscore we neeed for this macro.
> > ??The same applies for the SYM() macro I use.
> 
> let the build system do the unstringify operation.
> qstrip = $(strip $(subst ",,$(1)))
> CPPFLAGS_vmlinux.lds += -DVMLINUX_SYMBOL_PREFIX=$(call
> qstrip,CONFIG_SYMBOL_PREFIX)
> 
> > If we positively want to keep the generality, I guess I should put
> > MODULE_SYMBOL_PREFIX in a header file of it's own. ??The disadvantage is that
> > it makes it inaccessible to host programs again, like modpost (which
> > currently hardcodes the list of affected architectures in mk_elfconfig.c).
> 
> having it in the arch Kconfig removes any and all possible
> limitations, and it keeps the cruft out of the common init/Kconfig and
> in the arch-specific Kconfig, and avoids a dead symbol
> (HAVE_SYMBOL_PREFIX)

Having it in the Kconfig also makes it a nuisance for platforms that can
use -elf and -linux toolchains for the same tree for different platforms.
It would be nice to have this supported in such a way that we can just
set a flag from the Makefile and have a compiler test that determines
whether it is necessary or not.

Re: [PATCH 04/10] module: make MODULE_SYMBOL_PREFIX into a CONFIG option

[Mike Frysinger]

On Tue, Nov 3, 2009 at 08:29, Paul Mundt wrote:
> On Tue, Nov 03, 2009 at 07:30:20AM -0500, Mike Frysinger wrote:
>> On Tue, Nov 3, 2009 at 07:16, Alan Jenkins wrote:
>> > Mike Frysinger wrote:
>> >> On Tue, Nov 3, 2009 at 05:06, Alan Jenkins wrote:
>> >>> The next commit will require the use of MODULE_SYMBOL_PREFIX in
>> >>> .tmp_exports-asm.S. ??Currently it is mixed in with C structure
>> >>> definitions in "asm/module.h". ??Move the definition of this arch option
>> >>> into Kconfig, so it can be easily accessed by any code.
>> >>>
>> >>> This also lets modpost.c use the same definition. ??Previously modpost
>> >>> relied on a hardcoded list of architectures in mk_elfconfig.c.
>> >>
>> >> this should also let us push VMLINUX_SYMBOL() out of
>> >> arch/*/kernel/vmlinux.lds.S and into asm-generic/vmlinux.lds.h ...
>> >
>> > I don't think that's possible.
>> >
>> > ?? #define VMLINUX_SYMBOL(_sym_) _##_sym_
>> >
>> > I don't know any "unstringify" operation. ??So I can't convert a string value
>> > of CONFIG_SYMBOL_PREFIX to the unquoted underscore we neeed for this macro.
>> > ??The same applies for the SYM() macro I use.
>>
>> let the build system do the unstringify operation.
>> qstrip = $(strip $(subst ",,$(1)))
>> CPPFLAGS_vmlinux.lds += -DVMLINUX_SYMBOL_PREFIX=$(call
>> qstrip,CONFIG_SYMBOL_PREFIX)
>>
>> > If we positively want to keep the generality, I guess I should put
>> > MODULE_SYMBOL_PREFIX in a header file of it's own. ??The disadvantage is that
>> > it makes it inaccessible to host programs again, like modpost (which
>> > currently hardcodes the list of affected architectures in mk_elfconfig.c).
>>
>> having it in the arch Kconfig removes any and all possible
>> limitations, and it keeps the cruft out of the common init/Kconfig and
>> in the arch-specific Kconfig, and avoids a dead symbol
>> (HAVE_SYMBOL_PREFIX)
>
> Having it in the Kconfig also makes it a nuisance for platforms that can
> use -elf and -linux toolchains for the same tree for different platforms.
> It would be nice to have this supported in such a way that we can just
> set a flag from the Makefile and have a compiler test that determines
> whether it is necessary or not.

what arch is this an issue for ?  the only symbol prefixed arches are
Blackfin and H8300, and they dont provide toolchains that omit the
prefix.

if anything, the common build code could easily do:
ifeq ($(CONFIG_SYMBOL_PREFIX),)
CFLAGS += $(call cc-option,-fno-leading-underscore)
endif

trying to enable symbol prefix support dynamically based on the
toolchain is a bad idea and pretty fragile.  the arch-specific
assembly code would have to be all rewritten to wrap all C-visible
symbols with a macro like VMLINUX_SYMBOL().

i say let anyone who actually has such a system and wants to do such a
crazy ass thing put together a working arch first before we worry
about it.  the current code doesnt preclude dynamic hooking anyways
(manually adding -DCONFIG_xxx to CPPFLAGS).
-mike

Re: [PATCH 04/10] module: make MODULE_SYMBOL_PREFIX into a CONFIG option

[Mike Frysinger]

On Tue, Nov 3, 2009 at 08:29, Paul Mundt wrote:
> On Tue, Nov 03, 2009 at 07:30:20AM -0500, Mike Frysinger wrote:
>> On Tue, Nov 3, 2009 at 07:16, Alan Jenkins wrote:
>> > Mike Frysinger wrote:
>> >> On Tue, Nov 3, 2009 at 05:06, Alan Jenkins wrote:
>> >>> The next commit will require the use of MODULE_SYMBOL_PREFIX in
>> >>> .tmp_exports-asm.S. ??Currently it is mixed in with C structure
>> >>> definitions in "asm/module.h". ??Move the definition of this arch option
>> >>> into Kconfig, so it can be easily accessed by any code.
>> >>>
>> >>> This also lets modpost.c use the same definition. ??Previously modpost
>> >>> relied on a hardcoded list of architectures in mk_elfconfig.c.
>> >>
>> >> this should also let us push VMLINUX_SYMBOL() out of
>> >> arch/*/kernel/vmlinux.lds.S and into asm-generic/vmlinux.lds.h ...
>> >
>> > I don't think that's possible.
>> >
>> > ?? #define VMLINUX_SYMBOL(_sym_) _##_sym_
>> >
>> > I don't know any "unstringify" operation. ??So I can't convert a string value
>> > of CONFIG_SYMBOL_PREFIX to the unquoted underscore we neeed for this macro.
>> > ??The same applies for the SYM() macro I use.
>>
>> let the build system do the unstringify operation.
>> qstrip = $(strip $(subst ",,$(1)))
>> CPPFLAGS_vmlinux.lds += -DVMLINUX_SYMBOL_PREFIX=$(call
>> qstrip,CONFIG_SYMBOL_PREFIX)
>>
>> > If we positively want to keep the generality, I guess I should put
>> > MODULE_SYMBOL_PREFIX in a header file of it's own. ??The disadvantage is that
>> > it makes it inaccessible to host programs again, like modpost (which
>> > currently hardcodes the list of affected architectures in mk_elfconfig.c).
>>
>> having it in the arch Kconfig removes any and all possible
>> limitations, and it keeps the cruft out of the common init/Kconfig and
>> in the arch-specific Kconfig, and avoids a dead symbol
>> (HAVE_SYMBOL_PREFIX)
>
> Having it in the Kconfig also makes it a nuisance for platforms that can
> use -elf and -linux toolchains for the same tree for different platforms.
> It would be nice to have this supported in such a way that we can just
> set a flag from the Makefile and have a compiler test that determines
> whether it is necessary or not.

what arch is this an issue for ?  the only symbol prefixed arches are
Blackfin and H8300, and they dont provide toolchains that omit the
prefix.

if anything, the common build code could easily do:
ifeq ($(CONFIG_SYMBOL_PREFIX),)
CFLAGS += $(call cc-option,-fno-leading-underscore)
endif

trying to enable symbol prefix support dynamically based on the
toolchain is a bad idea and pretty fragile.  the arch-specific
assembly code would have to be all rewritten to wrap all C-visible
symbols with a macro like VMLINUX_SYMBOL().

i say let anyone who actually has such a system and wants to do such a
crazy ass thing put together a working arch first before we worry
about it.  the current code doesnt preclude dynamic hooking anyways
(manually adding -DCONFIG_xxx to CPPFLAGS).
-mike

Re: [PATCH 04/10] module: make MODULE_SYMBOL_PREFIX into a CONFIG option

[Paul Mundt]

On Tue, Nov 03, 2009 at 08:39:29AM -0500, Mike Frysinger wrote:
> On Tue, Nov 3, 2009 at 08:29, Paul Mundt wrote:
> > Having it in the Kconfig also makes it a nuisance for platforms that can
> > use -elf and -linux toolchains for the same tree for different platforms.
> > It would be nice to have this supported in such a way that we can just
> > set a flag from the Makefile and have a compiler test that determines
> > whether it is necessary or not.
> 
> what arch is this an issue for ?  the only symbol prefixed arches are
> Blackfin and H8300, and they dont provide toolchains that omit the
> prefix.
> 
No, those are the only symbol prefixed platforms enabled in the kernel at
present because neither one ships different toolchains.

The symbol prefixing itself is more an artifact of a -elf target
contrasted with a -linux one than anything "platform" specific. Thus, any
nommu platform using a bare metal or -elf toolchain can easily be used
for building the kernel if this can be supported in a clean way. As such,
a config option is not useful.

> trying to enable symbol prefix support dynamically based on the
> toolchain is a bad idea and pretty fragile.  the arch-specific
> assembly code would have to be all rewritten to wrap all C-visible
> symbols with a macro like VMLINUX_SYMBOL().
> 
There is nothing fragile about it, symbols are either prefixed or they
aren't. The common case for things like the syscall table obiously have
to be wrapped, but so what? C_SYMBOL_PREFIX() used to be the norm back in
the day, so it obiously worked well enough for the common case.

> i say let anyone who actually has such a system and wants to do such a
> crazy ass thing put together a working arch first before we worry
> about it.  the current code doesnt preclude dynamic hooking anyways
> (manually adding -DCONFIG_xxx to CPPFLAGS).

You talk about fragile bad ideas and then throw out defining Kconfig
variables from Makefiles? This simply has no place in the Kconfig space,
as it is now and always has been a toolchain property, not an
architectural/platform one.

The other thing you seem to have ignored is that pretty much everyone has
such a system, it's only crippled platforms like blackfin and h8300 that
don't support toolchains without the prefix.

Re: [PATCH 04/10] module: make MODULE_SYMBOL_PREFIX into a CONFIG option

[Mike Frysinger]

On Tue, Nov 3, 2009 at 08:46, Paul Mundt wrote:
> On Tue, Nov 03, 2009 at 08:39:29AM -0500, Mike Frysinger wrote:
>> On Tue, Nov 3, 2009 at 08:29, Paul Mundt wrote:
>> > Having it in the Kconfig also makes it a nuisance for platforms that can
>> > use -elf and -linux toolchains for the same tree for different platforms.
>> > It would be nice to have this supported in such a way that we can just
>> > set a flag from the Makefile and have a compiler test that determines
>> > whether it is necessary or not.
>>
>> what arch is this an issue for ?  the only symbol prefixed arches are
>> Blackfin and H8300, and they dont provide toolchains that omit the
>> prefix.
>
> No, those are the only symbol prefixed platforms enabled in the kernel at
> present because neither one ships different toolchains.

and most likely never will.  the Blackfin symbol prefix is every
where, userspace included.

> The symbol prefixing itself is more an artifact of a -elf target
> contrasted with a -linux one than anything "platform" specific. Thus, any
> nommu platform using a bare metal or -elf toolchain can easily be used
> for building the kernel if this can be supported in a clean way. As such,
> a config option is not useful.

which has no bearing on the Blackfin case as every toolchain target
can currently be used to build the kernel

>> trying to enable symbol prefix support dynamically based on the
>> toolchain is a bad idea and pretty fragile.  the arch-specific
>> assembly code would have to be all rewritten to wrap all C-visible
>> symbols with a macro like VMLINUX_SYMBOL().
>
> There is nothing fragile about it, symbols are either prefixed or they
> aren't. The common case for things like the syscall table obiously have
> to be wrapped, but so what? C_SYMBOL_PREFIX() used to be the norm back in
> the day, so it obiously worked well enough for the common case.

it worked well when it was the *common* case as you said.  when people
rarely use it (which is what happens today), things constantly break
because no one tests it, the usage is awkward, and it's an artifact
that shouldnt exist in the first place.

>> i say let anyone who actually has such a system and wants to do such a
>> crazy ass thing put together a working arch first before we worry
>> about it.  the current code doesnt preclude dynamic hooking anyways
>> (manually adding -DCONFIG_xxx to CPPFLAGS).
>
> You talk about fragile bad ideas and then throw out defining Kconfig
> variables from Makefiles? This simply has no place in the Kconfig space,
> as it is now and always has been a toolchain property, not an
> architectural/platform one.

my point was that it can easily be mixed.  i personally could care
less where the symbol is declared so long as it's declared just once.

> The other thing you seem to have ignored is that pretty much everyone has
> such a system, it's only crippled platforms like blackfin and h8300 that
> don't support toolchains without the prefix.

"cripple" is exactly the right word.  why in the world do you want to
cripple people that dont need it ?  attempting to support busted
toolchains by forcing even more symbol prefix crap throughout an arch
makes no sense at all.  use the -fno-leading-underscore gcc option if
you want to re-use a non-standard symbol prefixed elf compiler to
build an arch.
-mike

Re: [PATCH 04/10] module: make MODULE_SYMBOL_PREFIX into a CONFIG option

[Mike Frysinger]

On Tue, Nov 3, 2009 at 08:46, Paul Mundt wrote:
> On Tue, Nov 03, 2009 at 08:39:29AM -0500, Mike Frysinger wrote:
>> On Tue, Nov 3, 2009 at 08:29, Paul Mundt wrote:
>> > Having it in the Kconfig also makes it a nuisance for platforms that can
>> > use -elf and -linux toolchains for the same tree for different platforms.
>> > It would be nice to have this supported in such a way that we can just
>> > set a flag from the Makefile and have a compiler test that determines
>> > whether it is necessary or not.
>>
>> what arch is this an issue for ?  the only symbol prefixed arches are
>> Blackfin and H8300, and they dont provide toolchains that omit the
>> prefix.
>
> No, those are the only symbol prefixed platforms enabled in the kernel at
> present because neither one ships different toolchains.

and most likely never will.  the Blackfin symbol prefix is every
where, userspace included.

> The symbol prefixing itself is more an artifact of a -elf target
> contrasted with a -linux one than anything "platform" specific. Thus, any
> nommu platform using a bare metal or -elf toolchain can easily be used
> for building the kernel if this can be supported in a clean way. As such,
> a config option is not useful.

which has no bearing on the Blackfin case as every toolchain target
can currently be used to build the kernel

>> trying to enable symbol prefix support dynamically based on the
>> toolchain is a bad idea and pretty fragile.  the arch-specific
>> assembly code would have to be all rewritten to wrap all C-visible
>> symbols with a macro like VMLINUX_SYMBOL().
>
> There is nothing fragile about it, symbols are either prefixed or they
> aren't. The common case for things like the syscall table obiously have
> to be wrapped, but so what? C_SYMBOL_PREFIX() used to be the norm back in
> the day, so it obiously worked well enough for the common case.

it worked well when it was the *common* case as you said.  when people
rarely use it (which is what happens today), things constantly break
because no one tests it, the usage is awkward, and it's an artifact
that shouldnt exist in the first place.

>> i say let anyone who actually has such a system and wants to do such a
>> crazy ass thing put together a working arch first before we worry
>> about it.  the current code doesnt preclude dynamic hooking anyways
>> (manually adding -DCONFIG_xxx to CPPFLAGS).
>
> You talk about fragile bad ideas and then throw out defining Kconfig
> variables from Makefiles? This simply has no place in the Kconfig space,
> as it is now and always has been a toolchain property, not an
> architectural/platform one.

my point was that it can easily be mixed.  i personally could care
less where the symbol is declared so long as it's declared just once.

> The other thing you seem to have ignored is that pretty much everyone has
> such a system, it's only crippled platforms like blackfin and h8300 that
> don't support toolchains without the prefix.

"cripple" is exactly the right word.  why in the world do you want to
cripple people that dont need it ?  attempting to support busted
toolchains by forcing even more symbol prefix crap throughout an arch
makes no sense at all.  use the -fno-leading-underscore gcc option if
you want to re-use a non-standard symbol prefixed elf compiler to
build an arch.
-mike

Re: [PATCH 04/10] module: make MODULE_SYMBOL_PREFIX into a CONFIG option

[Paul Mundt]

On Tue, Nov 03, 2009 at 08:58:49AM -0500, Mike Frysinger wrote:
> On Tue, Nov 3, 2009 at 08:46, Paul Mundt wrote:
> > The other thing you seem to have ignored is that pretty much everyone has
> > such a system, it's only crippled platforms like blackfin and h8300 that
> > don't support toolchains without the prefix.
> 
> "cripple" is exactly the right word.  why in the world do you want to
> cripple people that dont need it ?  attempting to support busted
> toolchains by forcing even more symbol prefix crap throughout an arch
> makes no sense at all.  use the -fno-leading-underscore gcc option if
> you want to re-use a non-standard symbol prefixed elf compiler to
> build an arch.

My main consideration is for some SH-2 compilers where only bare metal
targets exist which could theoretically be used for the kernel, too. I've
avoided tying them in precisely because there wasn't a very clean way to
support the prefixed and non-prefixed case dynamically.

On the other hand, in those cases I don't think anyone actually cares
about the ABI, so having gcc not emit the prefix in the first place could
be a valid alternative, I'll give that a try, as that would simplify
things a fair bit.

[PATCH 05/10] kbuild: sort the list of symbols exported by the kernel (__ksymtab)

[Alan Jenkins]

modpost of vmlinux.o now extracts the ksymtab sections and outputs
sorted versions of them as .tmp_exports-asm.S.  These sorted sections
are linked into vmlinux and the original unsorted sections are
discarded.

This will allow modules to be loaded faster, resolving symbols using
binary search, without any increase in the memory needed for the
symbol tables.

This does not affect the building of modules, so hopefully it won't
affect compile times too much.

Minimally tested on ARM under QEMU emulator.
Build tested on blackfin; output of "size -A" unchanged.

Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
CC: Sam Ravnborg <sam@ravnborg.org>
---
 Makefile                          |   20 ++++++---
 include/asm-generic/vmlinux.lds.h |   39 ++++++++++++-----
 include/linux/mod_export.h        |   83 ++++++++++++++++++++++++++++++++++--
 init/Kconfig                      |    5 ++
 scripts/Makefile.modpost          |    2 +-
 scripts/mod/modpost.c             |   63 +++++++++++++++++++++++++++-
 6 files changed, 188 insertions(+), 24 deletions(-)

diff --git a/Makefile b/Makefile
index 00444a8..d7e4ed9 100644
--- a/Makefile
+++ b/Makefile
@@ -721,6 +721,8 @@ libs-y		:= $(libs-y1) $(libs-y2)
 #   +--< $(vmlinux-main)
 #   |    +--< driver/built-in.o mm/built-in.o + more
 #   |
+#   +-< .tmp_exports-asm.o (see comments regarding modpost of vmlinux.o)
+#   |
 #   +-< kallsyms.o (see description in CONFIG_KALLSYMS section)
 #
 # vmlinux version (uname -v) cannot be updated during normal
@@ -784,7 +786,6 @@ define rule_vmlinux__
 	$(verify_kallsyms)
 endef
 
-
 ifdef CONFIG_KALLSYMS
 # Generate section listing all symbols and add it into vmlinux $(kallsyms.o)
 # It's a three stage process:
@@ -844,13 +845,13 @@ quiet_cmd_kallsyms = KSYM    $@
 	$(call cmd,kallsyms)
 
 # .tmp_vmlinux1 must be complete except kallsyms, so update vmlinux version
-.tmp_vmlinux1: $(vmlinux-lds) $(vmlinux-all) FORCE
+.tmp_vmlinux1: $(vmlinux-lds) $(vmlinux-all) .tmp_exports-asm.o FORCE
 	$(call if_changed_rule,ksym_ld)
 
-.tmp_vmlinux2: $(vmlinux-lds) $(vmlinux-all) .tmp_kallsyms1.o FORCE
+.tmp_vmlinux2: $(vmlinux-lds) $(vmlinux-all) .tmp_exports-asm.o .tmp_kallsyms1.o FORCE
 	$(call if_changed,vmlinux__)
 
-.tmp_vmlinux3: $(vmlinux-lds) $(vmlinux-all) .tmp_kallsyms2.o FORCE
+.tmp_vmlinux3: $(vmlinux-lds) $(vmlinux-all) .tmp_exports-asm.o .tmp_kallsyms2.o FORCE
 	$(call if_changed,vmlinux__)
 
 # Needs to visit scripts/ before $(KALLSYMS) can be used.
@@ -882,7 +883,7 @@ define rule_vmlinux-modpost
 endef
 
 # vmlinux image - including updated kernel symbols
-vmlinux: $(vmlinux-lds) $(vmlinux-init) $(vmlinux-main) vmlinux.o $(kallsyms.o) FORCE
+vmlinux: $(vmlinux-lds) $(vmlinux-init) $(vmlinux-main) vmlinux.o .tmp_exports-asm.o $(kallsyms.o) FORCE
 ifdef CONFIG_HEADERS_CHECK
 	$(Q)$(MAKE) -f $(srctree)/Makefile headers_check
 endif
@@ -905,6 +906,12 @@ modpost-init := $(filter-out init/built-in.o, $(vmlinux-init))
 vmlinux.o: $(modpost-init) $(vmlinux-main) FORCE
 	$(call if_changed_rule,vmlinux-modpost)
 
+# The modpost of vmlinux.o above creates .tmp_exports-asm.S, a list of exported
+# symbols sorted by name.  This list is linked into vmlinux to replace the
+# original unsorted exports.  It allows symbols to be resolved efficiently
+# when loading modules.
+.tmp_exports-asm.S: vmlinux.o
+
 # The actual objects are generated when descending, 
 # make sure no implicit rule kicks in
 $(sort $(vmlinux-init) $(vmlinux-main)) $(vmlinux-lds): $(vmlinux-dirs) ;
@@ -1232,7 +1239,8 @@ endif # CONFIG_MODULES
 # Directories & files removed with 'make clean'
 CLEAN_DIRS  += $(MODVERDIR)
 CLEAN_FILES +=	vmlinux System.map \
-                .tmp_kallsyms* .tmp_version .tmp_vmlinux* .tmp_System.map
+                .tmp_kallsyms* .tmp_version .tmp_vmlinux* .tmp_System.map \
+                .tmp_exports-asm*
 
 # Directories & files removed with 'make mrproper'
 MRPROPER_DIRS  += include/config include2 usr/include include/generated
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index b6e818f..9feb474 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -254,76 +254,76 @@
 	/* Kernel symbol table: Normal symbols */			\
 	__ksymtab         : AT(ADDR(__ksymtab) - LOAD_OFFSET) {		\
 		VMLINUX_SYMBOL(__start___ksymtab) = .;			\
-		*(__ksymtab)						\
+		*(__ksymtab_sorted)					\
 		VMLINUX_SYMBOL(__stop___ksymtab) = .;			\
 	}								\
 									\
 	/* Kernel symbol table: GPL-only symbols */			\
 	__ksymtab_gpl     : AT(ADDR(__ksymtab_gpl) - LOAD_OFFSET) {	\
 		VMLINUX_SYMBOL(__start___ksymtab_gpl) = .;		\
-		*(__ksymtab_gpl)					\
+		*(__ksymtab_gpl_sorted)					\
 		VMLINUX_SYMBOL(__stop___ksymtab_gpl) = .;		\
 	}								\
 									\
 	/* Kernel symbol table: Normal unused symbols */		\
 	__ksymtab_unused  : AT(ADDR(__ksymtab_unused) - LOAD_OFFSET) {	\
 		VMLINUX_SYMBOL(__start___ksymtab_unused) = .;		\
-		*(__ksymtab_unused)					\
+		*(__ksymtab_unused_sorted)				\
 		VMLINUX_SYMBOL(__stop___ksymtab_unused) = .;		\
 	}								\
 									\
 	/* Kernel symbol table: GPL-only unused symbols */		\
 	__ksymtab_unused_gpl : AT(ADDR(__ksymtab_unused_gpl) - LOAD_OFFSET) { \
 		VMLINUX_SYMBOL(__start___ksymtab_unused_gpl) = .;	\
-		*(__ksymtab_unused_gpl)					\
+		*(__ksymtab_unused_gpl_sorted)				\
 		VMLINUX_SYMBOL(__stop___ksymtab_unused_gpl) = .;	\
 	}								\
 									\
 	/* Kernel symbol table: GPL-future-only symbols */		\
 	__ksymtab_gpl_future : AT(ADDR(__ksymtab_gpl_future) - LOAD_OFFSET) { \
 		VMLINUX_SYMBOL(__start___ksymtab_gpl_future) = .;	\
-		*(__ksymtab_gpl_future)					\
+		*(__ksymtab_gpl_future_sorted)				\
 		VMLINUX_SYMBOL(__stop___ksymtab_gpl_future) = .;	\
 	}								\
 									\
 	/* Kernel symbol table: Normal symbols */			\
 	__kcrctab         : AT(ADDR(__kcrctab) - LOAD_OFFSET) {		\
 		VMLINUX_SYMBOL(__start___kcrctab) = .;			\
-		*(__kcrctab)						\
+		*(__kcrctab_sorted)					\
 		VMLINUX_SYMBOL(__stop___kcrctab) = .;			\
 	}								\
 									\
 	/* Kernel symbol table: GPL-only symbols */			\
 	__kcrctab_gpl     : AT(ADDR(__kcrctab_gpl) - LOAD_OFFSET) {	\
 		VMLINUX_SYMBOL(__start___kcrctab_gpl) = .;		\
-		*(__kcrctab_gpl)					\
+		*(__kcrctab_gpl_sorted)					\
 		VMLINUX_SYMBOL(__stop___kcrctab_gpl) = .;		\
 	}								\
 									\
 	/* Kernel symbol table: Normal unused symbols */		\
 	__kcrctab_unused  : AT(ADDR(__kcrctab_unused) - LOAD_OFFSET) {	\
 		VMLINUX_SYMBOL(__start___kcrctab_unused) = .;		\
-		*(__kcrctab_unused)					\
+		*(__kcrctab_unused_sorted)				\
 		VMLINUX_SYMBOL(__stop___kcrctab_unused) = .;		\
 	}								\
 									\
 	/* Kernel symbol table: GPL-only unused symbols */		\
 	__kcrctab_unused_gpl : AT(ADDR(__kcrctab_unused_gpl) - LOAD_OFFSET) { \
 		VMLINUX_SYMBOL(__start___kcrctab_unused_gpl) = .;	\
-		*(__kcrctab_unused_gpl)					\
+		*(__kcrctab_unused_gpl_sorted)				\
 		VMLINUX_SYMBOL(__stop___kcrctab_unused_gpl) = .;	\
 	}								\
 									\
 	/* Kernel symbol table: GPL-future-only symbols */		\
 	__kcrctab_gpl_future : AT(ADDR(__kcrctab_gpl_future) - LOAD_OFFSET) { \
 		VMLINUX_SYMBOL(__start___kcrctab_gpl_future) = .;	\
-		*(__kcrctab_gpl_future)					\
+		*(__kcrctab_gpl_future_sorted)				\
 		VMLINUX_SYMBOL(__stop___kcrctab_gpl_future) = .;	\
 	}								\
 									\
 	/* Kernel symbol table: strings */				\
         __ksymtab_strings : AT(ADDR(__ksymtab_strings) - LOAD_OFFSET) {	\
-		*(__ksymtab_strings)					\
+		*(__ksymtab_strings_sorted)				\
 	}								\
 									\
 	/* __*init sections */						\
@@ -639,6 +639,23 @@
 	EXIT_DATA							\
 	EXIT_CALL							\
 	*(.discard)							\
+									\
+	/* 								\
+	 * Discard the original unsorted symbol tables.			\
+	 * In vmlinux they are replaced by sorted versions		\
+	 * generated by modpost -x.					\
+	 */								\
+	*(__ksymtab)							\
+	*(__ksymtab_gpl)						\
+	*(__ksymtab_unused)						\
+	*(__ksymtab_unused_gpl)						\
+	*(__ksymtab_gpl_future)						\
+	*(__kcrctab)							\
+	*(__kcrctab_gpl)						\
+	*(__kcrctab_unused)						\
+	*(__kcrctab_unused_gpl)						\
+	*(__kcrctab_gpl_future)						\
+	*(__ksymtab_strings)						\
 	}
 
 /**
diff --git a/include/linux/mod_export.h b/include/linux/mod_export.h
index 56b817a..3bb14e9 100644
--- a/include/linux/mod_export.h
+++ b/include/linux/mod_export.h
@@ -1,19 +1,35 @@
 #ifndef LINUX_MOD_EXPORT_H
 #define LINUX_MOD_EXPORT_H
 
+/*
+ * mod_export.h
+ *
+ * Define EXPORT_SYMBOL() and friends for kernel modules.
+ *
+ * Alternatively under __MODPOST_EXPORTS__, define __EXPORT_SYMBOL()
+ * in arch-independent assembly language.
+ */
+
+#ifndef __MODPOST_EXPORTS__
 #include <linux/compiler.h>
-#include <asm/module.h>
+#else
+#include <asm/bitsperlong.h>
+#include <linux/stringify.h>
+#endif
 
 /* Some toolchains use a `_' prefix for all user symbols. */
 #define MODULE_SYMBOL_PREFIX CONFIG_SYMBOL_PREFIX
 
+
+#ifndef __GENKSYMS__
+#ifndef __MODPOST_EXPORTS__
+#ifdef CONFIG_MODULES
+
 struct kernel_symbol {
 	unsigned long value;
 	const char *name;
 };
 
-#ifdef CONFIG_MODULES
-#ifndef __GENKSYMS__
 #ifdef CONFIG_MODVERSIONS
 /* Mark the CRC weak since genksyms apparently decides not to
  * generate a checksums for some symbols */
@@ -56,8 +72,6 @@ struct kernel_symbol {
 #define EXPORT_UNUSED_SYMBOL_GPL(sym)
 #endif
 
-#endif /* __GENKSYMS__ */
-
 #else /* !CONFIG_MODULES */
 
 #define EXPORT_SYMBOL(sym)
@@ -68,4 +82,63 @@ struct kernel_symbol {
 
 #endif /* CONFIG_MODULES */
 
+#else /* __MODPOST_EXPORTS__ */
+
+#if BITS_PER_LONG == 64
+#define PTR .quad
+#define ALGN .balign 8
+#else
+#define PTR .long
+#define ALGN .balign 4
+#endif
+
+/*
+ * We use CPP macros since they are more familiar than assembly macros.
+ * Note that CPP macros eat newlines, so each statement must be terminated
+ * by a semicolon.
+ */
+
+#ifdef CONFIG_HAVE_SYMBOL_PREFIX
+#define __SYM(sym) _##sym
+#else
+#define __SYM(sym) sym
+#endif
+
+#define SYM(sym) __SYM(sym)
+
+
+#ifdef CONFIG_MODVERSIONS
+#define __CRC_SYMBOL(sym, crcsec)				\
+	.globl SYM(__crc_##sym);				\
+	.weak SYM(__crc_##sym);					\
+	.pushsection crcsec, "a";				\
+	ALGN;							\
+	SYM(__kcrctab_##sym):					\
+	PTR SYM(__crc_##sym);					\
+	.popsection;
+#else
+#define __CRC_SYMBOL(sym, section)
+#endif
+
+#define __EXPORT_SYMBOL(sym, sec, strsec, crcsec)		\
+	.globl SYM(sym);					\
+								\
+	__CRC_SYMBOL(sym, crcsec)				\
+								\
+	.pushsection strsec, "a";				\
+	SYM(__kstrtab_##sym):					\
+	.asciz __stringify(SYM(sym));				\
+	.popsection;						\
+								\
+	.pushsection sec, "a";					\
+	ALGN;							\
+	SYM(__ksymtab_##sym):					\
+	PTR SYM(sym);						\
+	PTR SYM(__kstrtab_##sym);				\
+	.popsection;
+
+#endif /* __MODPOST_EXPORTS__ */
+
+#endif /* __GENKSYMS__ */
+
 #endif /* LINUX_MOD_EXPORT_H */
diff --git a/init/Kconfig b/init/Kconfig
index fe43d6d..7f4ddf6 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1100,6 +1100,11 @@ config BASE_SMALL
 	default 0 if BASE_FULL
 	default 1 if !BASE_FULL
 
+config HAVE_SYMBOL_PREFIX
+	bool
+	help
+	  Some arch toolchains use a `_' prefix for all user symbols.
+
 menuconfig MODULES
 	bool "Enable loadable module support"
 	help
diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
index 8f14c81..876a3c7 100644
--- a/scripts/Makefile.modpost
+++ b/scripts/Makefile.modpost
@@ -91,7 +91,7 @@ __modpost: $(modules:.ko=.o) FORCE
 	$(call cmd,modpost) $(wildcard vmlinux) $(filter-out FORCE,$^)
 
 quiet_cmd_kernel-mod = MODPOST $@
-      cmd_kernel-mod = $(modpost) $@
+      cmd_kernel-mod = $(modpost) -x .tmp_exports-asm.S $@
 
 vmlinux.o: FORCE
 	$(call cmd,kernel-mod)
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 3867481..404b69a 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -158,6 +158,7 @@ struct symbol {
 };
 
 static struct symbol *symbolhash[SYMBOL_HASH_SIZE];
+unsigned int symbolcount;
 
 /* This is based on the hash agorithm from gdbm, via tdb */
 static inline unsigned int tdb_hash(const char *name)
@@ -195,6 +196,7 @@ static struct symbol *new_symbol(const char *name, struct module *module,
 	unsigned int hash;
 	struct symbol *new;
 
+	symbolcount++;
 	hash = tdb_hash(name) % SYMBOL_HASH_SIZE;
 	new = symbolhash[hash] = alloc_symbol(name, 0, symbolhash[hash]);
 	new->module = module;
@@ -1980,6 +1982,58 @@ static void write_dump(const char *fname)
 	write_if_changed(&buf, fname);
 }
 
+static const char *section_names[] = {
+	[export_plain] 		= "",
+	[export_unused]		= "_unused",
+	[export_gpl]		= "_gpl",
+	[export_unused_gpl]	= "_unused_gpl",
+	[export_gpl_future]	= "_gpl_future",
+};
+
+static int compare_symbol_names(const void *a, const void *b)
+{
+	struct symbol *const *syma = a;
+	struct symbol *const *symb = b;
+
+	return strcmp((*syma)->name, (*symb)->name);
+}
+
+/* sort exported symbols and output using arch-independent assembly macros */
+static void write_exports(const char *fname)
+{
+	struct buffer buf = { };
+	struct symbol *sym, **symbols;
+	int i, n;
+
+	symbols = NOFAIL(malloc(sizeof(struct symbol *) * symbolcount));
+	n = 0;
+
+	for (i = 0; i < SYMBOL_HASH_SIZE; i++) {
+		for (sym = symbolhash[i]; sym; sym = sym->next)
+			symbols[n++] = sym;
+	}
+
+	qsort(symbols, n, sizeof(struct symbol *), compare_symbol_names);
+
+	buf_printf(&buf, "#define __MODPOST_EXPORTS__\n");
+	buf_printf(&buf, "#include <linux/mod_export.h>\n");
+	buf_printf(&buf, "\n");
+
+	for (i = 0; i < n; i++) {
+		sym = symbols[i];
+
+		buf_printf(&buf, "__EXPORT_SYMBOL(%s,"
+					" __ksymtab%s_sorted,"
+					" __ksymtab_strings_sorted,"
+					" __kcrctab%s_sorted)\n",
+					sym->name,
+					section_names[sym->export],
+					section_names[sym->export]);
+	}
+
+	write_if_changed(&buf, fname);
+}
+
 static void add_marker(struct module *mod, const char *name, const char *fmt)
 {
 	char *line = NULL;
@@ -2081,6 +2135,7 @@ int main(int argc, char **argv)
 	struct buffer buf = { };
 	char *kernel_read = NULL, *module_read = NULL;
 	char *dump_write = NULL;
+	char *exports_write = NULL;
 	char *markers_read = NULL;
 	char *markers_write = NULL;
 	int opt;
@@ -2088,7 +2143,7 @@ int main(int argc, char **argv)
 	struct ext_sym_list *extsym_iter;
 	struct ext_sym_list *extsym_start = NULL;
 
-	while ((opt = getopt(argc, argv, "i:I:e:cmsSo:awM:K:")) != -1) {
+	while ((opt = getopt(argc, argv, "i:I:e:cmsSo:awM:K:x:")) != -1) {
 		switch (opt) {
 		case 'i':
 			kernel_read = optarg;
@@ -2126,6 +2181,9 @@ int main(int argc, char **argv)
 		case 'w':
 			warn_unresolved = 1;
 			break;
+		case 'x':
+			exports_write = optarg;
+			break;
 			case 'M':
 				markers_write = optarg;
 				break;
@@ -2186,6 +2244,9 @@ int main(int argc, char **argv)
 		     "'make CONFIG_DEBUG_SECTION_MISMATCH=y'\n",
 		     sec_mismatch_count);
 
+	if (exports_write)
+		write_exports(exports_write);
+
 	if (markers_read)
 		read_markers(markers_read);
 
-- 
1.6.3.2

Re: [PATCH 05/10] kbuild: sort the list of symbols exported by the kernel (__ksymtab)

[Rusty Russell]

On Tue, 3 Nov 2009 08:36:17 pm Alan Jenkins wrote:
> +/*
> + * We use CPP macros since they are more familiar than assembly macros.
> + * Note that CPP macros eat newlines, so each statement must be terminated
> + * by a semicolon.
> + */
> +
> +#ifdef CONFIG_HAVE_SYMBOL_PREFIX
> +#define __SYM(sym) _##sym
> +#else
> +#define __SYM(sym) sym
> +#endif

Ideally, you would used MODULE_SYMBOL_PREFIX here, but of course it's a
string.  I don't think Kconfig can do arbitrary identifiers, so we can't
make CONFIG_SYMBOL_PREFIX empty or _.

Perhaps clarify it to a bool CONFIG_HAVE_MODULE_UNDERSCORE_PREFIX then,
since that's what you're assuming here?

Thanks,
Rusty.

Re: [PATCH 05/10] kbuild: sort the list of symbols exported by the kernel (__ksymtab)

[Alan Jenkins]

Rusty Russell wrote:
> On Tue, 3 Nov 2009 08:36:17 pm Alan Jenkins wrote:
>   
>> +/*
>> + * We use CPP macros since they are more familiar than assembly macros.
>> + * Note that CPP macros eat newlines, so each statement must be terminated
>> + * by a semicolon.
>> + */
>> +
>> +#ifdef CONFIG_HAVE_SYMBOL_PREFIX
>> +#define __SYM(sym) _##sym
>> +#else
>> +#define __SYM(sym) sym
>> +#endif
>>     
>
> Ideally, you would used MODULE_SYMBOL_PREFIX here, but of course it's a
> string.  I don't think Kconfig can do arbitrary identifiers, so we can't
> make CONFIG_SYMBOL_PREFIX empty or _.
>
> Perhaps clarify it to a bool CONFIG_HAVE_MODULE_UNDERSCORE_PREFIX then,
> since that's what you're assuming here?
>
> Thanks,
> Rusty.

I made the same assumption in patch 4.  The arch defines 
CONFIG_HAVE_SYMBOL_PREFIX, which then causes init/Kconfig to define 
CONFIG_SYMBOL_PREFIX="_".

Mike suggested that I hack kbuild instead, to do something like

unquote = ...
AFLAGS_.tmp_export-asm.o += -DSYMBOL_PREFIX=$(unquote CONFIG_SYMBOL_PREFIX)

I'm experimenting with the idea, but I haven't managed to get it working 
yet.

Alan

Re: [PATCH 05/10] kbuild: sort the list of symbols exported by the kernel (__ksymtab)

[Mike Frysinger]

On Wed, Nov 4, 2009 at 05:00, Alan Jenkins wrote:
> Rusty Russell wrote:
>> On Tue, 3 Nov 2009 08:36:17 pm Alan Jenkins wrote:
>>> +/*
>>> + * We use CPP macros since they are more familiar than assembly macros.
>>> + * Note that CPP macros eat newlines, so each statement must be
>>> terminated
>>> + * by a semicolon.
>>> + */
>>> +
>>> +#ifdef CONFIG_HAVE_SYMBOL_PREFIX
>>> +#define __SYM(sym) _##sym
>>> +#else
>>> +#define __SYM(sym) sym
>>> +#endif
>>>
>>
>> Ideally, you would used MODULE_SYMBOL_PREFIX here, but of course it's a
>> string.  I don't think Kconfig can do arbitrary identifiers, so we can't
>> make CONFIG_SYMBOL_PREFIX empty or _.
>>
>> Perhaps clarify it to a bool CONFIG_HAVE_MODULE_UNDERSCORE_PREFIX then,
>> since that's what you're assuming here?
>>
>> Thanks,
>> Rusty.
>
> I made the same assumption in patch 4.  The arch defines
> CONFIG_HAVE_SYMBOL_PREFIX, which then causes init/Kconfig to define
> CONFIG_SYMBOL_PREFIX="_".
>
> Mike suggested that I hack kbuild instead, to do something like
>
> unquote = ...
> AFLAGS_.tmp_export-asm.o += -DSYMBOL_PREFIX=$(unquote CONFIG_SYMBOL_PREFIX)
>
> I'm experimenting with the idea, but I haven't managed to get it working
> yet.

presumably you're having trouble with the preprocessor using the
define name literally instead of expanding it.  you can address this
two ways:
 - force gcc to expand it wit a few more levels
 - define the macro on the command line

here is the first way:
$ cat test.c
#include <stdio.h>
foo() { puts("foo"); }
_foo() { puts("_foo"); }
__foo() { puts("__foo"); }
#define __SYM(x) ___SYM(PFX, x) /* queue PFX */
#define ___SYM(pfx,x) ____SYM(pfx, x) /* expand PFX */
#define ____SYM(pfx,x) pfx##x /* paste them together */
main() { __SYM(foo)(); }
$ gcc test.c -DPFX=_ && ./a.out
_foo

and here is the second:
$ cat test.c
#include <stdio.h>
foo() { puts("foo"); }
_foo() { puts("_foo"); }
__foo() { puts("__foo"); }
main() { __SYM(foo)(); }
$ gcc test.c -D'__SYM(x)=_##x' && ./a.out
_foo

HTH
-mike

Re: [PATCH 05/10] kbuild: sort the list of symbols exported by the kernel (__ksymtab)

[Mike Frysinger]

On Wed, Nov 4, 2009 at 05:00, Alan Jenkins wrote:
> Rusty Russell wrote:
>> On Tue, 3 Nov 2009 08:36:17 pm Alan Jenkins wrote:
>>> +/*
>>> + * We use CPP macros since they are more familiar than assembly macros.
>>> + * Note that CPP macros eat newlines, so each statement must be
>>> terminated
>>> + * by a semicolon.
>>> + */
>>> +
>>> +#ifdef CONFIG_HAVE_SYMBOL_PREFIX
>>> +#define __SYM(sym) _##sym
>>> +#else
>>> +#define __SYM(sym) sym
>>> +#endif
>>>
>>
>> Ideally, you would used MODULE_SYMBOL_PREFIX here, but of course it's a
>> string.  I don't think Kconfig can do arbitrary identifiers, so we can't
>> make CONFIG_SYMBOL_PREFIX empty or _.
>>
>> Perhaps clarify it to a bool CONFIG_HAVE_MODULE_UNDERSCORE_PREFIX then,
>> since that's what you're assuming here?
>>
>> Thanks,
>> Rusty.
>
> I made the same assumption in patch 4.  The arch defines
> CONFIG_HAVE_SYMBOL_PREFIX, which then causes init/Kconfig to define
> CONFIG_SYMBOL_PREFIX="_".
>
> Mike suggested that I hack kbuild instead, to do something like
>
> unquote = ...
> AFLAGS_.tmp_export-asm.o += -DSYMBOL_PREFIX=$(unquote CONFIG_SYMBOL_PREFIX)
>
> I'm experimenting with the idea, but I haven't managed to get it working
> yet.

presumably you're having trouble with the preprocessor using the
define name literally instead of expanding it.  you can address this
two ways:
 - force gcc to expand it wit a few more levels
 - define the macro on the command line

here is the first way:
$ cat test.c
#include <stdio.h>
foo() { puts("foo"); }
_foo() { puts("_foo"); }
__foo() { puts("__foo"); }
#define __SYM(x) ___SYM(PFX, x) /* queue PFX */
#define ___SYM(pfx,x) ____SYM(pfx, x) /* expand PFX */
#define ____SYM(pfx,x) pfx##x /* paste them together */
main() { __SYM(foo)(); }
$ gcc test.c -DPFX=_ && ./a.out
_foo

and here is the second:
$ cat test.c
#include <stdio.h>
foo() { puts("foo"); }
_foo() { puts("_foo"); }
__foo() { puts("__foo"); }
main() { __SYM(foo)(); }
$ gcc test.c -D'__SYM(x)=_##x' && ./a.out
_foo

HTH
-mike

Re: [PATCH 05/10] kbuild: sort the list of symbols exported by the kernel (__ksymtab)

[Sam Ravnborg]

On Wed, Nov 04, 2009 at 10:00:50AM +0000, Alan Jenkins wrote:
> Rusty Russell wrote:
>> On Tue, 3 Nov 2009 08:36:17 pm Alan Jenkins wrote:
>>   
>>> +/*
>>> + * We use CPP macros since they are more familiar than assembly macros.
>>> + * Note that CPP macros eat newlines, so each statement must be terminated
>>> + * by a semicolon.
>>> + */
>>> +
>>> +#ifdef CONFIG_HAVE_SYMBOL_PREFIX
>>> +#define __SYM(sym) _##sym
>>> +#else
>>> +#define __SYM(sym) sym
>>> +#endif
>>>     
>>
>> Ideally, you would used MODULE_SYMBOL_PREFIX here, but of course it's a
>> string.  I don't think Kconfig can do arbitrary identifiers, so we can't
>> make CONFIG_SYMBOL_PREFIX empty or _.
>>
>> Perhaps clarify it to a bool CONFIG_HAVE_MODULE_UNDERSCORE_PREFIX then,
>> since that's what you're assuming here?
>>
>> Thanks,
>> Rusty.
>
> I made the same assumption in patch 4.  The arch defines  
> CONFIG_HAVE_SYMBOL_PREFIX, which then causes init/Kconfig to define  
> CONFIG_SYMBOL_PREFIX="_".
>
> Mike suggested that I hack kbuild instead, to do something like
>
> unquote = ...
> AFLAGS_.tmp_export-asm.o += -DSYMBOL_PREFIX=$(unquote CONFIG_SYMBOL_PREFIX)
>
> I'm experimenting with the idea, but I haven't managed to get it working  

Something like this:
unquote = $(patsubst "%",%,$($1))

AFLAGS_.tmp_export-asm.o += -DSYMBOL_PREFIX=$(call unquote,CONFIG_SYMBOL_PREFIX)

But the readability is low so we could be better off doing it direct:

AFLAGS_.tmp_export-asm.o += -DSYMBOL_PREFIX=$(patsubst "%",",$(CONFIG_SYMBOL_PREFIX))

	Sam

Re: [PATCH 05/10] kbuild: sort the list of symbols exported by the kernel (__ksymtab)

[Alan Jenkins]

Sam Ravnborg wrote:
> On Wed, Nov 04, 2009 at 10:00:50AM +0000, Alan Jenkins wrote:
>   
>> Rusty Russell wrote:
>>     
>>> On Tue, 3 Nov 2009 08:36:17 pm Alan Jenkins wrote:
>>>   
>>>       
>>>> +/*
>>>> + * We use CPP macros since they are more familiar than assembly macros.
>>>> + * Note that CPP macros eat newlines, so each statement must be terminated
>>>> + * by a semicolon.
>>>> + */
>>>> +
>>>> +#ifdef CONFIG_HAVE_SYMBOL_PREFIX
>>>> +#define __SYM(sym) _##sym
>>>> +#else
>>>> +#define __SYM(sym) sym
>>>> +#endif
>>>>     
>>>>         
>>> Ideally, you would used MODULE_SYMBOL_PREFIX here, but of course it's a
>>> string.  I don't think Kconfig can do arbitrary identifiers, so we can't
>>> make CONFIG_SYMBOL_PREFIX empty or _.
>>>
>>> Perhaps clarify it to a bool CONFIG_HAVE_MODULE_UNDERSCORE_PREFIX then,
>>> since that's what you're assuming here?
>>>
>>> Thanks,
>>> Rusty.
>>>       
>> I made the same assumption in patch 4.  The arch defines  
>> CONFIG_HAVE_SYMBOL_PREFIX, which then causes init/Kconfig to define  
>> CONFIG_SYMBOL_PREFIX="_".
>>
>> Mike suggested that I hack kbuild instead, to do something like
>>
>> unquote = ...
>> AFLAGS_.tmp_export-asm.o += -DSYMBOL_PREFIX=$(unquote CONFIG_SYMBOL_PREFIX)
>>
>> I'm experimenting with the idea, but I haven't managed to get it working  
>>     
>
> Something like this:
> unquote = $(patsubst "%",%,$($1))
>
> AFLAGS_.tmp_export-asm.o += -DSYMBOL_PREFIX=$(call unquote,CONFIG_SYMBOL_PREFIX)
>
> But the readability is low so we could be better off doing it direct:
>
> AFLAGS_.tmp_export-asm.o += -DSYMBOL_PREFIX=$(patsubst "%",",$(CONFIG_SYMBOL_PREFIX))
>
> 	Sam
>   

Thanks to both of you, I have something that works now. 

Unfortunately I had to export the variable AFLAGS_.tmp_export-asm.o, 
otherwise it had no effect.  I assume it's a limitation of the top-level 
Makefile. 

To make SYMBOL_PREFIX available for linker scripts, I also added it to 
"cpp_flags" in scripts/Makefile.lib.  (As far as I can see, cpp_flags is 
_only_ used for preprocessing linker scripts).

If this is all getting too brittle, I guess I could be less timid and 
add it to  the global KBUILD_CPPFLAGS instead :).

Here's the incremental diff:

diff --git a/Makefile b/Makefile
index d7e4ed9..dfd672b 100644
--- a/Makefile
+++ b/Makefile
@@ -912,6 +912,11 @@ vmlinux.o: $(modpost-init) $(vmlinux-main) FORCE
 # when loading modules.
 .tmp_exports-asm.S: vmlinux.o
 
+ifneq ($(CONFIG_SYMBOL_PREFIX),)
+export AFLAGS_.tmp_exports-asm.o += -DSYMBOL_PREFIX=$(patsubst "%",%,$(CONFIG_SYMBOL_PREFIX))
+endif
+
+
 # The actual objects are generated when descending, 
 # make sure no implicit rule kicks in
 $(sort $(vmlinux-init) $(vmlinux-main)) $(vmlinux-lds): $(vmlinux-dirs) ;
diff --git a/arch/blackfin/Kconfig b/arch/blackfin/Kconfig
index 6c99419..1983662 100644
--- a/arch/blackfin/Kconfig
+++ b/arch/blackfin/Kconfig
@@ -5,6 +5,10 @@
 
 mainmenu "Blackfin Kernel Configuration"
 
+config SYMBOL_PREFIX
+	string
+	default "_"
+
 config MMU
 	def_bool n
 
@@ -26,7 +30,6 @@ config BLACKFIN
 	select HAVE_KERNEL_BZIP2
 	select HAVE_KERNEL_LZMA
 	select HAVE_OPROFILE
-	select HAVE_SYMBOL_PREFIX
 	select ARCH_WANT_OPTIONAL_GPIOLIB
 
 config GENERIC_BUG
diff --git a/arch/blackfin/kernel/vmlinux.lds.S b/arch/blackfin/kernel/vmlinux.lds.S
index ffd90fb..1f585e8 100644
--- a/arch/blackfin/kernel/vmlinux.lds.S
+++ b/arch/blackfin/kernel/vmlinux.lds.S
@@ -27,8 +27,6 @@
  * 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
  */
 
-#define VMLINUX_SYMBOL(_sym_) _##_sym_
-
 #include <asm-generic/vmlinux.lds.h>
 #include <asm/mem_map.h>
 #include <asm/page.h>
diff --git a/arch/h8300/Kconfig b/arch/h8300/Kconfig
index cc03bbf..53cc669 100644
--- a/arch/h8300/Kconfig
+++ b/arch/h8300/Kconfig
@@ -9,7 +9,10 @@ config H8300
 	bool
 	default y
 	select HAVE_IDE
-	select HAVE_SYMBOL_PREFIX
+
+config SYMBOL_PREFIX
+	string
+	default "_"
 
 config MMU
 	bool
diff --git a/arch/h8300/kernel/vmlinux.lds.S b/arch/h8300/kernel/vmlinux.lds.S
index b9e2490..03d356d 100644
--- a/arch/h8300/kernel/vmlinux.lds.S
+++ b/arch/h8300/kernel/vmlinux.lds.S
@@ -1,4 +1,3 @@
-#define VMLINUX_SYMBOL(_sym_) _##_sym_
 #include <asm-generic/vmlinux.lds.h>
 #include <asm/page.h>
 
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 9feb474..4a0e9b2 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -52,10 +52,15 @@
 #define LOAD_OFFSET 0
 #endif
 
-#ifndef VMLINUX_SYMBOL
-#define VMLINUX_SYMBOL(_sym_) _sym_
+#ifndef SYMBOL_PREFIX
+#define VMLINUX_SYMBOL(sym) sym
+#else
+#define PASTE2(x,y) x##y
+#define PASTE(x,y) PASTE2(x,y)
+#define VMLINUX_SYMBOL(sym) PASTE(SYMBOL_PREFIX, sym)
 #endif
 
+
 /* Align . to a 8 byte boundary equals to maximum function alignment. */
 #define ALIGN_FUNCTION()  . = ALIGN(8)
 
diff --git a/include/linux/mod_export.h b/include/linux/mod_export.h
index 3bb14e9..f694ff5 100644
--- a/include/linux/mod_export.h
+++ b/include/linux/mod_export.h
@@ -18,7 +18,11 @@
 #endif
 
 /* Some toolchains use a `_' prefix for all user symbols. */
+#ifdef CONFIG_SYMBOL_PREFIX
 #define MODULE_SYMBOL_PREFIX CONFIG_SYMBOL_PREFIX
+#else
+#define MODULE_SYMBOL_PREFIX ""
+#endif
 
 
 #ifndef __GENKSYMS__
@@ -98,14 +102,14 @@ struct kernel_symbol {
  * by a semicolon.
  */
 
-#ifdef CONFIG_HAVE_SYMBOL_PREFIX
-#define __SYM(sym) _##sym
+#ifndef SYMBOL_PREFIX
+#define SYM(sym) sym
 #else
-#define __SYM(sym) sym
+#define PASTE2(x,y) x##y
+#define PASTE(x,y) PASTE2(x,y)
+#define SYM(sym) PASTE(SYMBOL_PREFIX, sym)
 #endif
 
-#define SYM(sym) __SYM(sym)
-
 
 #ifdef CONFIG_MODVERSIONS
 #define __CRC_SYMBOL(sym, crcsec)				\
diff --git a/init/Kconfig b/init/Kconfig
index 7f4ddf6..bb4051c 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1176,17 +1176,6 @@ config MODULE_SRCVERSION_ALL
 
 endif # MODULES
 
-config HAVE_SYMBOL_PREFIX
-	bool
-	help
-	  Some arch toolchains use a `_' prefix for all user symbols.
-	  This option will be taken into account when loading modules.
-
-config SYMBOL_PREFIX
-	string
-	default "_" if HAVE_SYMBOL_PREFIX
-	default ""
-
 config INIT_ALL_POSSIBLE
 	bool
 	help
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 7a77787..f9f1f6c 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -127,6 +127,11 @@ _c_flags += $(if $(patsubst n%,, \
 		$(CFLAGS_GCOV))
 endif
 
+ifdef CONFIG_SYMBOL_PREFIX
+_cpp_flags += -DSYMBOL_PREFIX=$(patsubst "%",%,$(CONFIG_SYMBOL_PREFIX))
+endif
+
+
 # If building the kernel in a separate objtree expand all occurrences
 # of -Idir to -I$(srctree)/dir except for absolute paths (starting with '/').
 
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 404b69a..b5a801d 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -19,7 +19,12 @@
 #include "../../include/linux/license.h"
 
 /* Some toolchains use a `_' prefix for all user symbols. */
+#ifdef CONFIG_SYMBOL_PREFIX
 #define MODULE_SYMBOL_PREFIX CONFIG_SYMBOL_PREFIX
+#else
+#define MODULE_SYMBOL_PREFIX ""
+#endif
+
 
 /* Are we using CONFIG_MODVERSIONS? */
 int modversions = 0;

Re: [PATCH 05/10] kbuild: sort the list of symbols exported by the kernel (__ksymtab)

[Mike Frysinger]

On Thu, Nov 5, 2009 at 09:24, Alan Jenkins wrote:
> Thanks to both of you, I have something that works now.
> Unfortunately I had to export the variable AFLAGS_.tmp_export-asm.o,
> otherwise it had no effect.  I assume it's a limitation of the top-level
> Makefile.
> To make SYMBOL_PREFIX available for linker scripts, I also added it to
> "cpp_flags" in scripts/Makefile.lib.  (As far as I can see, cpp_flags is
> _only_ used for preprocessing linker scripts).
>
> If this is all getting too brittle, I guess I could be less timid and add it
> to  the global KBUILD_CPPFLAGS instead :).

this stuff looks fine to me, thanks.  pushing the Blackfin stuff via
other trees as part of the set is OK in my book.
Acked-By: Mike Frysinger <vapier@gentoo.org>
-mike

Re: [PATCH 05/10] kbuild: sort the list of symbols exported by the kernel (__ksymtab)

[Mike Frysinger]

On Thu, Nov 5, 2009 at 09:24, Alan Jenkins wrote:
> Thanks to both of you, I have something that works now.
> Unfortunately I had to export the variable AFLAGS_.tmp_export-asm.o,
> otherwise it had no effect.  I assume it's a limitation of the top-level
> Makefile.
> To make SYMBOL_PREFIX available for linker scripts, I also added it to
> "cpp_flags" in scripts/Makefile.lib.  (As far as I can see, cpp_flags is
> _only_ used for preprocessing linker scripts).
>
> If this is all getting too brittle, I guess I could be less timid and add it
> to  the global KBUILD_CPPFLAGS instead :).

this stuff looks fine to me, thanks.  pushing the Blackfin stuff via
other trees as part of the set is OK in my book.
Acked-By: Mike Frysinger <vapier@gentoo.org>
-mike

Re: [PATCH 05/10] kbuild: sort the list of symbols exported by the kernel (__ksymtab)

[Rusty Russell]

On Fri, 6 Nov 2009 12:54:10 am Alan Jenkins wrote:
> Sam Ravnborg wrote:
> > On Wed, Nov 04, 2009 at 10:00:50AM +0000, Alan Jenkins wrote:
> >   
> >> Rusty Russell wrote:
> >>     
> >>> On Tue, 3 Nov 2009 08:36:17 pm Alan Jenkins wrote:
> >>>   
> >>>       
> >>>> +/*
> >>>> + * We use CPP macros since they are more familiar than assembly macros.
> >>>> + * Note that CPP macros eat newlines, so each statement must be terminated
> >>>> + * by a semicolon.
> >>>> + */
> >>>> +
> >>>> +#ifdef CONFIG_HAVE_SYMBOL_PREFIX
> >>>> +#define __SYM(sym) _##sym
> >>>> +#else
> >>>> +#define __SYM(sym) sym
> >>>> +#endif
> >>>>     
> >>>>         
> >>> Ideally, you would used MODULE_SYMBOL_PREFIX here, but of course it's a
> >>> string.  I don't think Kconfig can do arbitrary identifiers, so we can't
> >>> make CONFIG_SYMBOL_PREFIX empty or _.
> >>>
> >>> Perhaps clarify it to a bool CONFIG_HAVE_MODULE_UNDERSCORE_PREFIX then,
> >>> since that's what you're assuming here?
> >>>
> >>> Thanks,
> >>> Rusty.
> >>>       
> >> I made the same assumption in patch 4.  The arch defines  
> >> CONFIG_HAVE_SYMBOL_PREFIX, which then causes init/Kconfig to define  
> >> CONFIG_SYMBOL_PREFIX="_".
> >>
> >> Mike suggested that I hack kbuild instead, to do something like
> >>
> >> unquote = ...
> >> AFLAGS_.tmp_export-asm.o += -DSYMBOL_PREFIX=$(unquote CONFIG_SYMBOL_PREFIX)
> >>
> >> I'm experimenting with the idea, but I haven't managed to get it working  
> >>     
> >
> > Something like this:
> > unquote = $(patsubst "%",%,$($1))
> >
> > AFLAGS_.tmp_export-asm.o += -DSYMBOL_PREFIX=$(call unquote,CONFIG_SYMBOL_PREFIX)
> >
> > But the readability is low so we could be better off doing it direct:
> >
> > AFLAGS_.tmp_export-asm.o += -DSYMBOL_PREFIX=$(patsubst "%",",$(CONFIG_SYMBOL_PREFIX))
> >
> > 	Sam
> >   
> 
> Thanks to both of you, I have something that works now. 

I've taken this, but really, it's overkill.  Changing the config to a fixed
"underscore or nothing" was simpler and demonstrably sufficient for all cases
in the last 5 years.

Thanks,
Rusty.

Re: [PATCH 05/10] kbuild: sort the list of symbols exported by the kernel (__ksymtab)

[Tony Luck]

The sorted ksymtab breaks ia64 (and possibly ppc64 and
parisc too).

Alex Chiang did the bisect to find this change as
the cause of the breakage.  The problem is that ia64
expects that the first item in each ksymtab entry to be
a function pointer.  The code in modpost that creates
.tmp_exports-asm.S doesn't know about types of exported
objects, so it uses __EXPORT_SYMBOL from linux/mod_export.h
for everything. This results in

        PTR SYM(sym);
        PTR SYM(__kstrtab_##sym);

which the preprocessor expands to entries like:

        .long ____pagevec_lru_add
        .long __kstrtab____pagevec_lru_add

which puts the address of the first instruction of the
function into the table, rather than the address of a
function pointer (which on ia64 is a two element data
object containing the code address and the global data
pointer).

The syntax you need for this* is:

        .long @fptr(____pagevec_lru_add)
        .long __kstrtab____pagevec_lru_add

Note that you must only use the @fptr(name) syntax for
function exports. Exported data items just need an address.

-Tony

* On ia64 ... powerpc and parisc might need something else.

Re: [PATCH 05/10] kbuild: sort the list of symbols exported by the kernel (__ksymtab)

[Tony Luck]

The sorted ksymtab breaks ia64 (and possibly ppc64 and
parisc too).

Alex Chiang did the bisect to find this change as
the cause of the breakage.  The problem is that ia64
expects that the first item in each ksymtab entry to be
a function pointer.  The code in modpost that creates
.tmp_exports-asm.S doesn't know about types of exported
objects, so it uses __EXPORT_SYMBOL from linux/mod_export.h
for everything. This results in

        PTR SYM(sym);
        PTR SYM(__kstrtab_##sym);

which the preprocessor expands to entries like:

        .long ____pagevec_lru_add
        .long __kstrtab____pagevec_lru_add

which puts the address of the first instruction of the
function into the table, rather than the address of a
function pointer (which on ia64 is a two element data
object containing the code address and the global data
pointer).

The syntax you need for this* is:

        .long @fptr(____pagevec_lru_add)
        .long __kstrtab____pagevec_lru_add

Note that you must only use the @fptr(name) syntax for
function exports. Exported data items just need an address.

-Tony

* On ia64 ... powerpc and parisc might need something else.

Re: [PATCH 05/10] kbuild: sort the list of symbols exported by the kernel (__ksymtab)

[Alan Jenkins]

Tony Luck wrote:
> The sorted ksymtab breaks ia64 (and possibly ppc64 and
> parisc too).
>
> Alex Chiang did the bisect to find this change as
> the cause of the breakage.  The problem is that ia64
> expects that the first item in each ksymtab entry to be
> a function pointer.  The code in modpost that creates
> .tmp_exports-asm.S doesn't know about types of exported
> objects, so it uses __EXPORT_SYMBOL from linux/mod_export.h
> for everything. This results in
>
>         PTR SYM(sym);
>         PTR SYM(__kstrtab_##sym);
>
> which the preprocessor expands to entries like:
>
>         .long ____pagevec_lru_add
>         .long __kstrtab____pagevec_lru_add
>
> which puts the address of the first instruction of the
> function into the table, rather than the address of a
> function pointer (which on ia64 is a two element data
> object containing the code address and the global data
> pointer).
>
> The syntax you need for this* is:
>
>         .long @fptr(____pagevec_lru_add)
>         .long __kstrtab____pagevec_lru_add
>
> Note that you must only use the @fptr(name) syntax for
> function exports. Exported data items just need an address.
>
> -Tony
>
> * On ia64 ... powerpc and parisc might need something else.
>   

Thanks!  It doesn't sound too hard to retro-fit your suggestion.

Still, I can't help wondering if I've done this all wrong :-/.  Perhaps 
I should avoid the assembler.  Instead, I could write a tool to sort the 
ksymtab elf sections in-place (and mangle their relocations 
accordingly).  That should preserve any special handling for function 
symbols without arch-specific special cases.  It would also concentrate 
all the magic in one tool - rather than it being scattered between the 
modpost tool, mod_export.h, tmp_exports.S, and vmlinux.lds.h.

Alan

Re: [PATCH 05/10] kbuild: sort the list of symbols exported by the kernel (__ksymtab)

[Alex Chiang]

Hi Alan, Rusty,

* Alan Jenkins <alan-jenkins@tuffmail.co.uk>:
> Tony Luck wrote:
>> The sorted ksymtab breaks ia64 (and possibly ppc64 and
>> parisc too).

[snip]

>> The syntax you need for this* is:
>>
>>         .long @fptr(____pagevec_lru_add)
>>         .long __kstrtab____pagevec_lru_add
>>
>> Note that you must only use the @fptr(name) syntax for
>> function exports. Exported data items just need an address.
>>
>> -Tony
>>
>> * On ia64 ... powerpc and parisc might need something else.
>>   
> Thanks!  It doesn't sound too hard to retro-fit your suggestion.
>
> Still, I can't help wondering if I've done this all wrong :-/.  Perhaps  
> I should avoid the assembler.  Instead, I could write a tool to sort the  
> ksymtab elf sections in-place (and mangle their relocations  
> accordingly).  That should preserve any special handling for function  
> symbols without arch-specific special cases.  It would also concentrate  
> all the magic in one tool - rather than it being scattered between the  
> modpost tool, mod_export.h, tmp_exports.S, and vmlinux.lds.h.

In the meantime, while Alan is deciding the proper way to fix
this, would it be possible to drop the offending patch series
from linux-next?

It makes ia64 unbootable, and has ripple-through effects, since
mmotm is based on linux-next these days.

Thanks,
/ac

Re: [PATCH 05/10] kbuild: sort the list of symbols exported by the kernel (__ksymtab)

[Alan Jenkins]

Alex Chiang wrote:
> Hi Alan, Rusty,
>
> * Alan Jenkins <alan-jenkins@tuffmail.co.uk>:
>   
>> Tony Luck wrote:
>>     
>>> The sorted ksymtab breaks ia64 (and possibly ppc64 and
>>> parisc too).
>>>       
>
> [snip]
>
>   
>>> The syntax you need for this* is:
>>>
>>>         .long @fptr(____pagevec_lru_add)
>>>         .long __kstrtab____pagevec_lru_add
>>>
>>> Note that you must only use the @fptr(name) syntax for
>>> function exports. Exported data items just need an address.
>>>
>>> -Tony
>>>
>>> * On ia64 ... powerpc and parisc might need something else.
>>>   
>>>       
>> Thanks!  It doesn't sound too hard to retro-fit your suggestion.
>>
>> Still, I can't help wondering if I've done this all wrong :-/.  Perhaps  
>> I should avoid the assembler.  Instead, I could write a tool to sort the  
>> ksymtab elf sections in-place (and mangle their relocations  
>> accordingly).  That should preserve any special handling for function  
>> symbols without arch-specific special cases.  It would also concentrate  
>> all the magic in one tool - rather than it being scattered between the  
>> modpost tool, mod_export.h, tmp_exports.S, and vmlinux.lds.h.
>>     
>
> In the meantime, while Alan is deciding the proper way to fix
> this, would it be possible to drop the offending patch series
> from linux-next?
>
> It makes ia64 unbootable, and has ripple-through effects, since
> mmotm is based on linux-next these days.
>
> Thanks,
> /ac
>   

Sorry, I put this off until the last minute of today.  I settled on just 
adding @fptr as suggested.  I reviewed the other arches, and I think 
only IA64 needs anything special here.

Alan

Re: [PATCH 05/10] kbuild: sort the list of symbols exported by the kernel (__ksymtab)

[Rusty Russell]

On Tue, 24 Nov 2009 06:23:20 am Alex Chiang wrote:
> Hi Alan, Rusty,

Hi Alex,

> In the meantime, while Alan is deciding the proper way to fix
> this, would it be possible to drop the offending patch series
> from linux-next?

Done.  That takes the pressure off Alan, and makes sure he has time to get
it right.

Thanks,
Rusty.

Re: [PATCH 05/10] kbuild: sort the list of symbols exported by the kernel (__ksymtab)

[James Bottomley]

On Tue, 2009-11-24 at 11:27 +1030, Rusty Russell wrote:
> On Tue, 24 Nov 2009 06:23:20 am Alex Chiang wrote:
> > Hi Alan, Rusty,
> 
> Hi Alex,
> 
> > In the meantime, while Alan is deciding the proper way to fix
> > this, would it be possible to drop the offending patch series
> > from linux-next?
> 
> Done.  That takes the pressure off Alan, and makes sure he has time to get
> it right.

That probably suits us on parisc too.  I just checked out our build in
linux-next: we don't pass __modpost ... it looks like we have all the
module symbols undefined.  Will investigate more.

James

Re: [PATCH 05/10] kbuild: sort the list of symbols exported by the kernel (__ksymtab)

[Alan Jenkins]

James Bottomley wrote:
> On Tue, 2009-11-24 at 11:27 +1030, Rusty Russell wrote:
>   
>> On Tue, 24 Nov 2009 06:23:20 am Alex Chiang wrote:
>>     
>>> Hi Alan, Rusty,
>>>       
>> Hi Alex,
>>
>>     
>>> In the meantime, while Alan is deciding the proper way to fix
>>> this, would it be possible to drop the offending patch series
>>> from linux-next?
>>>       
>> Done.  That takes the pressure off Alan, and makes sure he has time to get
>> it right.
>>     
>
> That probably suits us on parisc too.  I just checked out our build in
> linux-next: we don't pass __modpost ... it looks like we have all the
> module symbols undefined.  Will investigate more.
>
> James
>   

I think parisc wants P'printk where ia64 uses @fptr(printk).

It may also need ".import printk,code" or similar.

Thanks
Alan

Re: [PATCH 05/10] kbuild: sort the list of symbols exported by the kernel (__ksymtab)

[James Bottomley]

On Tue, 2009-11-24 at 09:28 +0000, Alan Jenkins wrote:
> James Bottomley wrote:
> > On Tue, 2009-11-24 at 11:27 +1030, Rusty Russell wrote:
> >   
> >> On Tue, 24 Nov 2009 06:23:20 am Alex Chiang wrote:
> >>     
> >>> Hi Alan, Rusty,
> >>>       
> >> Hi Alex,
> >>
> >>     
> >>> In the meantime, while Alan is deciding the proper way to fix
> >>> this, would it be possible to drop the offending patch series
> >>> from linux-next?
> >>>       
> >> Done.  That takes the pressure off Alan, and makes sure he has time to get
> >> it right.
> >>     
> >
> > That probably suits us on parisc too.  I just checked out our build in
> > linux-next: we don't pass __modpost ... it looks like we have all the
> > module symbols undefined.  Will investigate more.
> >
> > James
> >   
> 
> I think parisc wants P'printk where ia64 uses @fptr(printk).
> 
> It may also need ".import printk,code" or similar.

I think if you have to make modpost architecture specific, there's
something a bit wrong in the patch series.

I can confirm that reverting this particular patch allows the parisc
build to work again.  It still won't boot because module symbols aren't
resolved.

James

Re: [PATCH 05/10] kbuild: sort the list of symbols exported by the kernel (__ksymtab)

[Alan Jenkins]

James Bottomley wrote:
> On Tue, 2009-11-24 at 09:28 +0000, Alan Jenkins wrote:
>   
>> James Bottomley wrote:
>>     
>>> On Tue, 2009-11-24 at 11:27 +1030, Rusty Russell wrote:
>>>   
>>>       
>>>> On Tue, 24 Nov 2009 06:23:20 am Alex Chiang wrote:
>>>>     
>>>>         
>>>>> Hi Alan, Rusty,
>>>>>       
>>>>>           
>>>> Hi Alex,
>>>>
>>>>     
>>>>         
>>>>> In the meantime, while Alan is deciding the proper way to fix
>>>>> this, would it be possible to drop the offending patch series
>>>>> from linux-next?
>>>>>       
>>>>>           
>>>> Done.  That takes the pressure off Alan, and makes sure he has time to get
>>>> it right.
>>>>     
>>>>         
>>> That probably suits us on parisc too.  I just checked out our build in
>>> linux-next: we don't pass __modpost ... it looks like we have all the
>>> module symbols undefined.  Will investigate more.
>>>
>>> James
>>>   
>>>       
>> I think parisc wants P'printk where ia64 uses @fptr(printk).
>>
>> It may also need ".import printk,code" or similar.
>>     
>
> I think if you have to make modpost architecture specific, there's
> something a bit wrong in the patch series.
>
> I can confirm that reverting this particular patch allows the parisc
> build to work again.  It still won't boot because module symbols aren't
> resolved.
>
> James
>   

Yes, the series as a whole relies on that patch.  Rusty pulled the 
series from linux-next (thanks rusty!).

I'm working on alternatives now.  I can't promise to avoid architecture 
specific code, but if it's necessary then I understand I have to arrange 
to test it myself :-).  Sorry for the inconvenience caused by 
"arch-independent assembly code" that wasn't.

Alan

Re: [PATCH 05/10] kbuild: sort the list of symbols exported by the kernel (__ksymtab)

[James Bottomley]

On Wed, 2009-11-25 at 09:15 +0000, Alan Jenkins wrote:
> James Bottomley wrote:
> > On Tue, 2009-11-24 at 09:28 +0000, Alan Jenkins wrote:
> >   
> >> James Bottomley wrote:
> >>     
> >>> On Tue, 2009-11-24 at 11:27 +1030, Rusty Russell wrote:
> >>>   
> >>>       
> >>>> On Tue, 24 Nov 2009 06:23:20 am Alex Chiang wrote:
> >>>>     
> >>>>         
> >>>>> Hi Alan, Rusty,
> >>>>>       
> >>>>>           
> >>>> Hi Alex,
> >>>>
> >>>>     
> >>>>         
> >>>>> In the meantime, while Alan is deciding the proper way to fix
> >>>>> this, would it be possible to drop the offending patch series
> >>>>> from linux-next?
> >>>>>       
> >>>>>           
> >>>> Done.  That takes the pressure off Alan, and makes sure he has time to get
> >>>> it right.
> >>>>     
> >>>>         
> >>> That probably suits us on parisc too.  I just checked out our build in
> >>> linux-next: we don't pass __modpost ... it looks like we have all the
> >>> module symbols undefined.  Will investigate more.
> >>>
> >>> James
> >>>   
> >>>       
> >> I think parisc wants P'printk where ia64 uses @fptr(printk).
> >>
> >> It may also need ".import printk,code" or similar.
> >>     
> >
> > I think if you have to make modpost architecture specific, there's
> > something a bit wrong in the patch series.
> >
> > I can confirm that reverting this particular patch allows the parisc
> > build to work again.  It still won't boot because module symbols aren't
> > resolved.
> >
> > James
> >   
> 
> Yes, the series as a whole relies on that patch.  Rusty pulled the 
> series from linux-next (thanks rusty!).

Not according to current linux-next:

jejb@ion> git log next-20091125|grep -3 'sort the list of symbols'
Author: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
Date:   Sat Nov 7 21:03:56 2009 +0000

    kbuild: sort the list of symbols exported by the kernel (__ksymtab)
    
    modpost of vmlinux.o now extracts the ksymtab sections and outputs
    sorted versions of them as .tmp_exports-asm.S.  These sorted sections

Could we please have this removed so we can resume our testing of next?

> I'm working on alternatives now.  I can't promise to avoid architecture 
> specific code, but if it's necessary then I understand I have to arrange 
> to test it myself :-).  Sorry for the inconvenience caused by 
> "arch-independent assembly code" that wasn't.

I'm not sure I understand what you're trying to do, but it seems to me
that the way to do what you want is to introduce an arch macro for
function pointer which we all define in our headers to be however our
assemblers represent it ... for non fptr arches this would be an
identity.

We don't need there to be no arch changes ... but we do need any arch
specificity confined to arch specific files.

James

Re: [PATCH 05/10] kbuild: sort the list of symbols exported by the kernel (__ksymtab)

[Alan Jenkins]

James Bottomley wrote:
> On Wed, 2009-11-25 at 09:15 +0000, Alan Jenkins wrote:
>   
>> James Bottomley wrote:
>>     
>>> On Tue, 2009-11-24 at 09:28 +0000, Alan Jenkins wrote:
>>>   
>>>       
>>>> James Bottomley wrote:
>>>>     
>>>>         
>>>>> On Tue, 2009-11-24 at 11:27 +1030, Rusty Russell wrote:
>>>>>   
>>>>>       
>>>>>           
>>>>>> On Tue, 24 Nov 2009 06:23:20 am Alex Chiang wrote:
>>>>>>     
>>>>>>         
>>>>>>             
>>>>>>> Hi Alan, Rusty,
>>>>>>>       
>>>>>>>           
>>>>>>>               
>>>>>> Hi Alex,
>>>>>>
>>>>>>     
>>>>>>         
>>>>>>             
>>>>>>> In the meantime, while Alan is deciding the proper way to fix
>>>>>>> this, would it be possible to drop the offending patch series
>>>>>>> from linux-next?
>>>>>>>       
>>>>>>>           
>>>>>>>               
>>>>>> Done.  That takes the pressure off Alan, and makes sure he has time to get
>>>>>> it right.
>>>>>>     
>>>>>>         
>>>>>>             
>>>>> That probably suits us on parisc too.  I just checked out our build in
>>>>> linux-next: we don't pass __modpost ... it looks like we have all the
>>>>> module symbols undefined.  Will investigate more.
>>>>>
>>>>> James
>>>>>   
>>>>>       
>>>>>           
>>>> I think parisc wants P'printk where ia64 uses @fptr(printk).
>>>>
>>>> It may also need ".import printk,code" or similar.
>>>>     
>>>>         
>>> I think if you have to make modpost architecture specific, there's
>>> something a bit wrong in the patch series.
>>>
>>> I can confirm that reverting this particular patch allows the parisc
>>> build to work again.  It still won't boot because module symbols aren't
>>> resolved.
>>>
>>> James
>>>   
>>>       
>> Yes, the series as a whole relies on that patch.  Rusty pulled the 
>> series from linux-next (thanks rusty!).
>>     
>
> Not according to current linux-next:
>
> jejb@ion> git log next-20091125|grep -3 'sort the list of symbols'
> Author: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
> Date:   Sat Nov 7 21:03:56 2009 +0000
>
>     kbuild: sort the list of symbols exported by the kernel (__ksymtab)
>     
>     modpost of vmlinux.o now extracts the ksymtab sections and outputs
>     sorted versions of them as .tmp_exports-asm.S.  These sorted sections
>
> Could we please have this removed so we can resume our testing of next?
>   

Note to Rusty: the patches which depend on this are

"module: speed up find_symbol() using binary search on the builtin 
symbol tables"

and

"modpost: fix modules on ia64 - use @fptr() on exported function symbols"

(the second one needs to be rewritten anyway, because -

> We don't need there to be no arch changes ... but we do need any arch
> specificity confined to arch specific files.
>
> James
>   

Re: [PATCH 05/10] kbuild: sort the list of symbols exported by the kernel (__ksymtab)

[Rusty Russell]

On Thu, 26 Nov 2009 01:38:57 am James Bottomley wrote:
> On Wed, 2009-11-25 at 09:15 +0000, Alan Jenkins wrote:
> > Yes, the series as a whole relies on that patch.  Rusty pulled the 
> > series from linux-next (thanks rusty!).
> 
> Not according to current linux-next:

Yeah, I pulled it for a day, then Alan produced the ia64 fix.

Pulling again until all acks are in...

Thanks,
Rusty.

Re: [PATCH 05/10] kbuild: sort the list of symbols exported by the kernel (__ksymtab)

[Andrew Morton]

On Tue,  3 Nov 2009 10:06:17 +0000
Alan Jenkins <alan-jenkins@tuffmail.co.uk> wrote:

> modpost of vmlinux.o now extracts the ksymtab sections and outputs
> sorted versions of them as .tmp_exports-asm.S.  These sorted sections
> are linked into vmlinux and the original unsorted sections are
> discarded.
> 
> This will allow modules to be loaded faster, resolving symbols using
> binary search, without any increase in the memory needed for the
> symbol tables.
> 
> This does not affect the building of modules, so hopefully it won't
> affect compile times too much.
> 
> Minimally tested on ARM under QEMU emulator.
> Build tested on blackfin; output of "size -A" unchanged.

I'm getting a segfault from write_exports().  

(gdb) bt
#0  0x0000003e5f075510 in strlen () from /lib64/libc.so.6
#1  0x0000003e5f045cb8 in vfprintf () from /lib64/libc.so.6
#2  0x0000003e5f06683a in vsnprintf () from /lib64/libc.so.6
#3  0x0000000000401897 in buf_printf (buf=0x7fff5514f5e0, 
    fmt=0x7fff5514f4e0 "0x%02x ") at scripts/mod/modpost.c:1692
#4  0x00000000004042c8 in main (argc=1024, argv=0x7fff5514f5e0)
    at scripts/mod/modpost.c:2063
(gdb) f 4
#4  0x00000000004042c8 in main (argc=1024, argv=0x7fff5514f5e0)
    at scripts/mod/modpost.c:2063
2063                    buf_printf(&buf, "__EXPORT_%s_SYMBOL(%s,"
(gdb) p sym
$1 = (struct symbol *) 0x65c9f0
(gdb) p *sym
$2 = {next = 0x64c3a0, module = 0x610010, crc = 4077789248, crc_valid = 1, 
  weak = 0, vmlinux = 0, kernel = 0, preloaded = 0, function = 1, 
  export = export_unknown, name = 0x65ca10 "simple_prepare_write"}
(gdb) p sym->export
$3 = export_unknown
(gdb) p/d sym->export
$4 = 5

but section_names[] (which could be static in write_exports() btw) has
only five entries.

Re: [PATCH 05/10] kbuild: sort the list of symbols exported by the kernel (__ksymtab)

[Alan Jenkins]

Andrew Morton wrote:
> On Tue,  3 Nov 2009 10:06:17 +0000
> Alan Jenkins <alan-jenkins@tuffmail.co.uk> wrote:
>
>   
>> modpost of vmlinux.o now extracts the ksymtab sections and outputs
>> sorted versions of them as .tmp_exports-asm.S.  These sorted sections
>> are linked into vmlinux and the original unsorted sections are
>> discarded.
>>
>> This will allow modules to be loaded faster, resolving symbols using
>> binary search, without any increase in the memory needed for the
>> symbol tables.
>>
>> This does not affect the building of modules, so hopefully it won't
>> affect compile times too much.
>>
>> Minimally tested on ARM under QEMU emulator.
>> Build tested on blackfin; output of "size -A" unchanged.
>>     
>
> I'm getting a segfault from write_exports().  
>
> (gdb) bt
> #0  0x0000003e5f075510 in strlen () from /lib64/libc.so.6
> #1  0x0000003e5f045cb8 in vfprintf () from /lib64/libc.so.6
> #2  0x0000003e5f06683a in vsnprintf () from /lib64/libc.so.6
> #3  0x0000000000401897 in buf_printf (buf=0x7fff5514f5e0, 
>     fmt=0x7fff5514f4e0 "0x%02x ") at scripts/mod/modpost.c:1692
> #4  0x00000000004042c8 in main (argc=1024, argv=0x7fff5514f5e0)
>     at scripts/mod/modpost.c:2063
> (gdb) f 4
> #4  0x00000000004042c8 in main (argc=1024, argv=0x7fff5514f5e0)
>     at scripts/mod/modpost.c:2063
> 2063                    buf_printf(&buf, "__EXPORT_%s_SYMBOL(%s,"
> (gdb) p sym
> $1 = (struct symbol *) 0x65c9f0
> (gdb) p *sym
> $2 = {next = 0x64c3a0, module = 0x610010, crc = 4077789248, crc_valid = 1, 
>   weak = 0, vmlinux = 0, kernel = 0, preloaded = 0, function = 1, 
>   export = export_unknown, name = 0x65ca10 "simple_prepare_write"}
> (gdb) p sym->export
> $3 = export_unknown
> (gdb) p/d sym->export
> $4 = 5
>
> but section_names[] (which could be static in write_exports() btw) has
> only five entries.
>   

Thanks.  I can't work out why this would happen.  Could you show the 
options modpost is being run with (make V=1 will do)?  Also confirm 
whether this is "MODPOST vmlinux.o" or "MODPOST 1001 modules".


 > main (argc=1024

It looks like this is the step "MODPOST 1001 modules".  But that 
shouldn't run write_exports().  We only run modpost with "-x" for the 
step "MODPOST vmlinux.o".  Also, the vmlinux-only modpost is run first, 
so I wonder why it didn't hit the same problem.

I don't know why sym->vmlinux==0 either; simple_prepare_write() should 
always be built into vmlinux.  Perhaps modpost is being run on libfs.o 
for some strange reason.

Alan

[PATCH 06/10] module: refactor symbol tables and try to reduce code size of each_symbol()

[Alan Jenkins]

find_symbol() will use bsearch() instead of each_symbol(). each_symbol()
is still desired by Ksplice (although it is not in-tree yet).  Let's try
to minimize the code which will be duplicated between these two
functions, by changing how the symbol tables are declared.

Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
---
 include/linux/module.h |  102 ++++++++++++-------
 kernel/module.c        |  251 ++++++++++++++++++++++++------------------------
 2 files changed, 191 insertions(+), 162 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index b9e860a..0bb0f74 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -168,6 +168,62 @@ void *__symbol_get(const char *symbol);
 void *__symbol_get_gpl(const char *symbol);
 #define symbol_get(x) ((typeof(&x))(__symbol_get(MODULE_SYMBOL_PREFIX #x)))
 
+#ifdef CONFIG_UNUSED_SYMBOLS
+enum export_type {
+	/* GPL-only exported symbols. */
+	__EXPORT_FLAG_GPL_ONLY =	0x1,
+
+	/* unused exported symbols. */
+	__EXPORT_FLAG_UNUSED =		0x2,
+
+	/* exports that will be GPL-only in the near future. */
+	__EXPORT_FLAG_GPL_FUTURE =	0x4,
+
+	EXPORT_TYPE_PLAIN =		0x0,
+	EXPORT_TYPE_GPL =		0x1,
+	EXPORT_TYPE_UNUSED =		0x2,
+	EXPORT_TYPE_UNUSED_GPL =	0x3,
+	EXPORT_TYPE_GPL_FUTURE = 	0x4,
+
+	EXPORT_TYPE_MAX
+};
+
+#else /* !CONFIG_UNUSED_EXPORTS */
+enum export_type {
+	__EXPORT_FLAG_UNUSED =		0x0,
+	__EXPORT_FLAG_GPL_ONLY =	0x1,
+	__EXPORT_FLAG_GPL_FUTURE =	0x2,
+
+	EXPORT_TYPE_PLAIN =		0x0,
+	EXPORT_TYPE_GPL =		0x1,
+	EXPORT_TYPE_GPL_FUTURE = 	0x2,
+	EXPORT_TYPE_MAX
+};
+#endif
+
+static inline bool export_is_gpl_only(enum export_type type)
+{
+	return (type & __EXPORT_FLAG_GPL_ONLY);
+}
+
+static inline bool export_is_unused(enum export_type type)
+{
+	return (type & __EXPORT_FLAG_UNUSED);
+}
+
+static inline bool export_is_gpl_future(enum export_type type)
+{
+	return (type & __EXPORT_FLAG_GPL_FUTURE);
+}
+
+struct ksymtab {
+	const struct kernel_symbol *syms;
+#ifdef CONFIG_MODVERSIONS
+	const unsigned long *crcs;
+#endif
+	unsigned int num_syms;
+};
+
 enum module_state
 {
 	MODULE_STATE_LIVE,
@@ -193,36 +249,12 @@ struct module
 	struct kobject *holders_dir;
 
 	/* Exported symbols */
-	const struct kernel_symbol *syms;
-	const unsigned long *crcs;
-	unsigned int num_syms;
+	struct ksymtab syms[EXPORT_TYPE_MAX];
 
 	/* Kernel parameters. */
 	struct kernel_param *kp;
 	unsigned int num_kp;
 
-	/* GPL-only exported symbols. */
-	unsigned int num_gpl_syms;
-	const struct kernel_symbol *gpl_syms;
-	const unsigned long *gpl_crcs;
-
-#ifdef CONFIG_UNUSED_SYMBOLS
-	/* unused exported symbols. */
-	const struct kernel_symbol *unused_syms;
-	const unsigned long *unused_crcs;
-	unsigned int num_unused_syms;
-
-	/* GPL-only, unused exported symbols. */
-	unsigned int num_unused_gpl_syms;
-	const struct kernel_symbol *unused_gpl_syms;
-	const unsigned long *unused_gpl_crcs;
-#endif
-
-	/* symbols that will be GPL-only in the near future. */
-	const struct kernel_symbol *gpl_future_syms;
-	const unsigned long *gpl_future_crcs;
-	unsigned int num_gpl_future_syms;
-
 	/* Exception table */
 	unsigned int num_exentries;
 	struct exception_table_entry *extable;
@@ -352,17 +384,6 @@ static inline int within_module_init(unsigned long addr, struct module *mod)
 /* Search for module by name: must hold module_mutex. */
 struct module *find_module(const char *name);
 
-struct symsearch {
-	const struct kernel_symbol *start, *stop;
-	const unsigned long *crcs;
-	enum {
-		NOT_GPL_ONLY,
-		GPL_ONLY,
-		WILL_BE_GPL_ONLY,
-	} licence;
-	bool unused;
-};
-
 /* Search for an exported symbol by name. */
 const struct kernel_symbol *find_symbol(const char *name,
 					struct module **owner,
@@ -370,9 +391,14 @@ const struct kernel_symbol *find_symbol(const char *name,
 					bool gplok,
 					bool warn);
 
+typedef bool each_symbol_fn_t (enum export_type type,
+			       const struct kernel_symbol *sym,
+			       const unsigned long *crc,
+			       struct module *owner,
+			       void *data);
+
 /* Walk the exported symbol table */
-bool each_symbol(bool (*fn)(const struct symsearch *arr, struct module *owner,
-			    unsigned int symnum, void *data), void *data);
+bool each_symbol(each_symbol_fn_t *fn, void *data);
 
 /* Returns 0 and fills in value, defined and namebuf, or -ERANGE if
    symnum out of range. */
diff --git a/kernel/module.c b/kernel/module.c
index fe748a8..38a2859 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -192,25 +192,64 @@ extern const unsigned long __start___kcrctab_unused[];
 extern const unsigned long __start___kcrctab_unused_gpl[];
 #endif
 
+static struct ksymtab ksymtab[EXPORT_TYPE_MAX];
+
+static int __init init_ksymtab(void)
+{
+	struct ksymtab tmp[EXPORT_TYPE_MAX] = {
+		[EXPORT_TYPE_PLAIN] = {
+			__start___ksymtab, __start___kcrctab,
+			__stop___ksymtab - __start___ksymtab,
+		},
+		[EXPORT_TYPE_GPL] = {
+			__start___ksymtab_gpl, __start___kcrctab_gpl,
+			__stop___ksymtab_gpl - __start___ksymtab_gpl,
+		},
+#ifdef CONFIG_UNUSED_EXPORTS
+		[EXPORT_TYPE_UNUSED] = {
+			__start___ksymtab_unused, __start___kcrctab_unused,
+			__stop___ksymtab_unused - __start___ksymtab_unused,
+		},
+		[EXPORT_TYPE_UNUSED_GPL] = {
+			__start___ksymtab_unused_gpl,
+			__start___kcrctab_unused_gpl,
+			__stop___ksymtab_unused_gpl -
+				__start___ksymtab_unused_gpl,
+		},
+#endif
+		[EXPORT_TYPE_GPL_FUTURE] = {
+			__start___ksymtab_gpl_future,
+			__start___kcrctab_gpl_future,
+			__stop___ksymtab_gpl_future -
+				__start___ksymtab_gpl_future,
+		},
+	};
+
+	memcpy(ksymtab, tmp, sizeof(ksymtab));
+
+	return 0;
+}
+pure_initcall(init_ksymtab);
+
 #ifndef CONFIG_MODVERSIONS
 #define symversion(base, idx) NULL
 #else
 #define symversion(base, idx) ((base != NULL) ? ((base) + (idx)) : NULL)
 #endif
 
-static bool each_symbol_in_section(const struct symsearch *arr,
-				   unsigned int arrsize,
+static bool each_symbol_in_section(const struct ksymtab syms[EXPORT_TYPE_MAX],
 				   struct module *owner,
-				   bool (*fn)(const struct symsearch *syms,
-					      struct module *owner,
-					      unsigned int symnum, void *data),
+				   each_symbol_fn_t *fn,
 				   void *data)
 {
-	unsigned int i, j;
+	enum export_type type;
+	unsigned int i;
 
-	for (j = 0; j < arrsize; j++) {
-		for (i = 0; i < arr[j].stop - arr[j].start; i++)
-			if (fn(&arr[j], owner, i, data))
+	for (type = 0; type < EXPORT_TYPE_MAX; type++) {
+		for (i = 0; i < syms[type].num_syms; i++)
+			if (fn(type, &syms[type].syms[i],
+			       symversion(syms[type].crcs, i),
+			       owner, data))
 				return true;
 	}
 
@@ -218,56 +257,15 @@ static bool each_symbol_in_section(const struct symsearch *arr,
 }
 
 /* Returns true as soon as fn returns true, otherwise false. */
-bool each_symbol(bool (*fn)(const struct symsearch *arr, struct module *owner,
-			    unsigned int symnum, void *data), void *data)
+bool each_symbol(each_symbol_fn_t *fn, void *data)
 {
 	struct module *mod;
-	const struct symsearch arr[] = {
-		{ __start___ksymtab, __stop___ksymtab, __start___kcrctab,
-		  NOT_GPL_ONLY, false },
-		{ __start___ksymtab_gpl, __stop___ksymtab_gpl,
-		  __start___kcrctab_gpl,
-		  GPL_ONLY, false },
-		{ __start___ksymtab_gpl_future, __stop___ksymtab_gpl_future,
-		  __start___kcrctab_gpl_future,
-		  WILL_BE_GPL_ONLY, false },
-#ifdef CONFIG_UNUSED_SYMBOLS
-		{ __start___ksymtab_unused, __stop___ksymtab_unused,
-		  __start___kcrctab_unused,
-		  NOT_GPL_ONLY, true },
-		{ __start___ksymtab_unused_gpl, __stop___ksymtab_unused_gpl,
-		  __start___kcrctab_unused_gpl,
-		  GPL_ONLY, true },
-#endif
-	};
 
-	if (each_symbol_in_section(arr, ARRAY_SIZE(arr), NULL, fn, data))
+	if (each_symbol_in_section(ksymtab, NULL, fn, data))
 		return true;
 
 	list_for_each_entry_rcu(mod, &modules, list) {
-		struct symsearch arr[] = {
-			{ mod->syms, mod->syms + mod->num_syms, mod->crcs,
-			  NOT_GPL_ONLY, false },
-			{ mod->gpl_syms, mod->gpl_syms + mod->num_gpl_syms,
-			  mod->gpl_crcs,
-			  GPL_ONLY, false },
-			{ mod->gpl_future_syms,
-			  mod->gpl_future_syms + mod->num_gpl_future_syms,
-			  mod->gpl_future_crcs,
-			  WILL_BE_GPL_ONLY, false },
-#ifdef CONFIG_UNUSED_SYMBOLS
-			{ mod->unused_syms,
-			  mod->unused_syms + mod->num_unused_syms,
-			  mod->unused_crcs,
-			  NOT_GPL_ONLY, true },
-			{ mod->unused_gpl_syms,
-			  mod->unused_gpl_syms + mod->num_unused_gpl_syms,
-			  mod->unused_gpl_crcs,
-			  GPL_ONLY, true },
-#endif
-		};
-
-		if (each_symbol_in_section(arr, ARRAY_SIZE(arr), mod, fn, data))
+		if (each_symbol_in_section(mod->syms, mod, fn, data))
 			return true;
 	}
 	return false;
@@ -281,24 +279,26 @@ struct find_symbol_arg {
 	bool warn;
 
 	/* Output */
-	struct module *owner;
-	const unsigned long *crc;
 	const struct kernel_symbol *sym;
+	const unsigned long *crc;
+	struct module *owner;
 };
 
-static bool find_symbol_in_section(const struct symsearch *syms,
+static bool find_symbol_in_section(enum export_type type,
+				   const struct kernel_symbol *sym,
+				   const unsigned long *crc,
 				   struct module *owner,
-				   unsigned int symnum, void *data)
+				   void *data)
 {
 	struct find_symbol_arg *fsa = data;
 
-	if (strcmp(syms->start[symnum].name, fsa->name) != 0)
+	if (strcmp(sym->name, fsa->name) != 0)
 		return false;
 
 	if (!fsa->gplok) {
-		if (syms->licence == GPL_ONLY)
+		if (export_is_gpl_only(type))
 			return false;
-		if (syms->licence == WILL_BE_GPL_ONLY && fsa->warn) {
+		if (export_is_gpl_future(type) && fsa->warn) {
 			printk(KERN_WARNING "Symbol %s is being used "
 			       "by a non-GPL module, which will not "
 			       "be allowed in the future\n", fsa->name);
@@ -309,7 +309,7 @@ static bool find_symbol_in_section(const struct symsearch *syms,
 	}
 
 #ifdef CONFIG_UNUSED_SYMBOLS
-	if (syms->unused && fsa->warn) {
+	if (export_is_unused(type) && fsa->warn) {
 		printk(KERN_WARNING "Symbol %s is marked as UNUSED, "
 		       "however this module is using it.\n", fsa->name);
 		printk(KERN_WARNING
@@ -322,9 +322,9 @@ static bool find_symbol_in_section(const struct symsearch *syms,
 	}
 #endif
 
+	fsa->sym = sym;
+	fsa->crc = crc;
 	fsa->owner = owner;
-	fsa->crc = symversion(syms->crcs, symnum);
-	fsa->sym = &syms->start[symnum];
 	return true;
 }
 
@@ -1564,23 +1564,13 @@ EXPORT_SYMBOL_GPL(__symbol_get);
 static int verify_export_symbols(struct module *mod)
 {
 	unsigned int i;
-	struct module *owner;
+	enum export_type type;
 	const struct kernel_symbol *s;
-	struct {
-		const struct kernel_symbol *sym;
-		unsigned int num;
-	} arr[] = {
-		{ mod->syms, mod->num_syms },
-		{ mod->gpl_syms, mod->num_gpl_syms },
-		{ mod->gpl_future_syms, mod->num_gpl_future_syms },
-#ifdef CONFIG_UNUSED_SYMBOLS
-		{ mod->unused_syms, mod->num_unused_syms },
-		{ mod->unused_gpl_syms, mod->num_unused_gpl_syms },
-#endif
-	};
+	struct module *owner;
 
-	for (i = 0; i < ARRAY_SIZE(arr); i++) {
-		for (s = arr[i].sym; s < arr[i].sym + arr[i].num; s++) {
+	for (type = 0; type < EXPORT_TYPE_MAX; type++) {
+		for (i = 0; i < mod->syms[type].num_syms; i++) {
+			s = &mod->syms[type].syms[i];
 			if (find_symbol(s->name, &owner, NULL, true, false)) {
 				printk(KERN_ERR
 				       "%s: exports duplicate symbol %s"
@@ -1812,24 +1802,30 @@ static void free_modinfo(struct module *mod)
 
 /* lookup symbol in given range of kernel_symbols */
 static const struct kernel_symbol *lookup_symbol(const char *name,
-	const struct kernel_symbol *start,
-	const struct kernel_symbol *stop)
+	const struct kernel_symbol *syms,
+	unsigned int count)
 {
-	const struct kernel_symbol *ks = start;
-	for (; ks < stop; ks++)
-		if (strcmp(ks->name, name) == 0)
-			return ks;
+	unsigned int i;
+
+	for (i = 0; i < count; i++)
+		if (strcmp(syms[i].name, name) == 0)
+			return &syms[i];
 	return NULL;
 }
 
 static int is_exported(const char *name, unsigned long value,
 		       const struct module *mod)
 {
+	const struct ksymtab *symtab;
 	const struct kernel_symbol *ks;
+
 	if (!mod)
-		ks = lookup_symbol(name, __start___ksymtab, __stop___ksymtab);
+		symtab = &ksymtab[EXPORT_TYPE_PLAIN];
 	else
-		ks = lookup_symbol(name, mod->syms, mod->syms + mod->num_syms);
+		symtab = &mod->syms[EXPORT_TYPE_PLAIN];
+
+	ks = lookup_symbol(name, symtab->syms, symtab->num_syms);
+
 	return ks != NULL && ks->value == value;
 }
 
@@ -2064,6 +2060,26 @@ static inline void kmemleak_load_module(struct module *mod, Elf_Ehdr *hdr,
 }
 #endif
 
+static const char *export_section_names[EXPORT_TYPE_MAX] = {
+	[EXPORT_TYPE_PLAIN] = "__ksymtab",
+	[EXPORT_TYPE_GPL] = "__ksymtab_gpl",
+#ifdef CONFIG_UNUSED_SYMBOLS
+	[EXPORT_TYPE_UNUSED] = "__ksymtab_unused",
+	[EXPORT_TYPE_UNUSED_GPL] = "__ksymtab_unused_gpl",
+#endif
+	[EXPORT_TYPE_GPL_FUTURE] = "__ksymtab_gpl_future",
+};
+
+static const char *crc_section_names[EXPORT_TYPE_MAX] = {
+	[EXPORT_TYPE_PLAIN] = "__kcrctab",
+	[EXPORT_TYPE_GPL] = "__kcrctab_gpl",
+#ifdef CONFIG_UNUSED_SYMBOLS
+	[EXPORT_TYPE_UNUSED] = "__kcrctab_unused",
+	[EXPORT_TYPE_UNUSED_GPL] = "__kcrctab_unused_gpl",
+#endif
+	[EXPORT_TYPE_GPL_FUTURE] = "__kcrctab_gpl_future",
+};
+
 /* Allocate and load the module: note that size of section 0 is always
    zero, and we rely on this for optional sections. */
 static noinline struct module *load_module(void __user *umod,
@@ -2078,6 +2094,7 @@ static noinline struct module *load_module(void __user *umod,
 	unsigned int symindex = 0;
 	unsigned int strindex = 0;
 	unsigned int modindex, versindex, infoindex, pcpuindex;
+	enum export_type export_type;
 	struct module *mod;
 	long err = 0;
 	void *percpu = NULL, *ptr = NULL; /* Stops spurious gcc warning */
@@ -2338,34 +2355,20 @@ static noinline struct module *load_module(void __user *umod,
 	 * find optional sections. */
 	mod->kp = section_objs(hdr, sechdrs, secstrings, "__param",
 			       sizeof(*mod->kp), &mod->num_kp);
-	mod->syms = section_objs(hdr, sechdrs, secstrings, "__ksymtab",
-				 sizeof(*mod->syms), &mod->num_syms);
-	mod->crcs = section_addr(hdr, sechdrs, secstrings, "__kcrctab");
-	mod->gpl_syms = section_objs(hdr, sechdrs, secstrings, "__ksymtab_gpl",
-				     sizeof(*mod->gpl_syms),
-				     &mod->num_gpl_syms);
-	mod->gpl_crcs = section_addr(hdr, sechdrs, secstrings, "__kcrctab_gpl");
-	mod->gpl_future_syms = section_objs(hdr, sechdrs, secstrings,
-					    "__ksymtab_gpl_future",
-					    sizeof(*mod->gpl_future_syms),
-					    &mod->num_gpl_future_syms);
-	mod->gpl_future_crcs = section_addr(hdr, sechdrs, secstrings,
-					    "__kcrctab_gpl_future");
 
-#ifdef CONFIG_UNUSED_SYMBOLS
-	mod->unused_syms = section_objs(hdr, sechdrs, secstrings,
-					"__ksymtab_unused",
-					sizeof(*mod->unused_syms),
-					&mod->num_unused_syms);
-	mod->unused_crcs = section_addr(hdr, sechdrs, secstrings,
-					"__kcrctab_unused");
-	mod->unused_gpl_syms = section_objs(hdr, sechdrs, secstrings,
-					    "__ksymtab_unused_gpl",
-					    sizeof(*mod->unused_gpl_syms),
-					    &mod->num_unused_gpl_syms);
-	mod->unused_gpl_crcs = section_addr(hdr, sechdrs, secstrings,
-					    "__kcrctab_unused_gpl");
+	for (export_type = 0; export_type < EXPORT_TYPE_MAX; export_type++) {
+		mod->syms[export_type].syms =
+				section_objs(hdr, sechdrs, secstrings,
+					     export_section_names[export_type],
+					     sizeof(struct kernel_symbol),
+					     &mod->syms[export_type].num_syms);
+#ifdef CONFIG_MODVERSIONS
+		mod->syms[export_type].crcs = 
+				section_addr(hdr, sechdrs, secstrings,
+					     crc_section_names[export_type]);
 #endif
+	}
+
 #ifdef CONFIG_CONSTRUCTORS
 	mod->ctors = section_objs(hdr, sechdrs, secstrings, ".ctors",
 				  sizeof(*mod->ctors), &mod->num_ctors);
@@ -2390,19 +2393,19 @@ static noinline struct module *load_module(void __user *umod,
 					     sizeof(*mod->ftrace_callsites),
 					     &mod->num_ftrace_callsites);
 #endif
+
 #ifdef CONFIG_MODVERSIONS
-	if ((mod->num_syms && !mod->crcs)
-	    || (mod->num_gpl_syms && !mod->gpl_crcs)
-	    || (mod->num_gpl_future_syms && !mod->gpl_future_crcs)
-#ifdef CONFIG_UNUSED_SYMBOLS
-	    || (mod->num_unused_syms && !mod->unused_crcs)
-	    || (mod->num_unused_gpl_syms && !mod->unused_gpl_crcs)
-#endif
-		) {
-		err = try_to_force_load(mod,
-					"no versions for exported symbols");
-		if (err)
-			goto cleanup;
+	for (export_type = 0; export_type < EXPORT_TYPE_MAX; export_type++) {
+		if (mod->syms[export_type].syms &&
+		    !mod->syms[export_type].crcs) {
+			err = try_to_force_load(mod,
+				"no versions for exported symbols");
+			if (err)
+				goto cleanup;
+
+			/* force load approved, don't check other sections */
+			break;
+		}
 	}
 #endif
 
-- 
1.6.3.2

Re: [PATCH 06/10] module: refactor symbol tables and try to reduce code size of each_symbol()

[Rusty Russell]

On Tue, 3 Nov 2009 08:36:18 pm Alan Jenkins wrote:
> find_symbol() will use bsearch() instead of each_symbol(). each_symbol()
> is still desired by Ksplice (although it is not in-tree yet).  Let's try
> to minimize the code which will be duplicated between these two
> functions, by changing how the symbol tables are declared.

Alan, this is a particularly neat cleanup.  Thanks!

> +typedef bool each_symbol_fn_t (enum export_type type,
> +			       const struct kernel_symbol *sym,
> +			       const unsigned long *crc,
> +			       struct module *owner,
> +			       void *data);
> +
>  /* Walk the exported symbol table */
> -bool each_symbol(bool (*fn)(const struct symsearch *arr, struct module *owner,
> -			    unsigned int symnum, void *data), void *data);
> +bool each_symbol(each_symbol_fn_t *fn, void *data);

I really hate throwaway typedefs like this.  But it's used in two other
places, so I'll suppress my distaste :)

> +static struct ksymtab ksymtab[EXPORT_TYPE_MAX];
> +
> +static int __init init_ksymtab(void)
> +{
> +	struct ksymtab tmp[EXPORT_TYPE_MAX] = {
> +		[EXPORT_TYPE_PLAIN] = {
> +			__start___ksymtab, __start___kcrctab,
> +			__stop___ksymtab - __start___ksymtab,
> +		},
> +		[EXPORT_TYPE_GPL] = {
> +			__start___ksymtab_gpl, __start___kcrctab_gpl,
> +			__stop___ksymtab_gpl - __start___ksymtab_gpl,
> +		},
> +#ifdef CONFIG_UNUSED_EXPORTS
> +		[EXPORT_TYPE_UNUSED] = {
> +			__start___ksymtab_unused, __start___kcrctab_unused,
> +			__stop___ksymtab_unused - __start___ksymtab_unused,
> +		},
> +		[EXPORT_TYPE_UNUSED_GPL] = {
> +			__start___ksymtab_unused_gpl,
> +			__start___kcrctab_unused_gpl,
> +			__stop___ksymtab_unused_gpl -
> +				__start___ksymtab_unused_gpl,
> +		},
> +#endif
> +		[EXPORT_TYPE_GPL_FUTURE] = {
> +			__start___ksymtab_gpl_future,
> +			__start___kcrctab_gpl_future,
> +			__stop___ksymtab_gpl_future -
> +				__start___ksymtab_gpl_future,
> +		},
> +	};
> +
> +	memcpy(ksymtab, tmp, sizeof(ksymtab));

This works, but I'd prefer you to open-code the assignments.  Simpler and
marginally more efficient.

> @@ -322,9 +322,9 @@ static bool find_symbol_in_section(const struct symsearch *syms,
>  	}
>  #endif
>  
> +	fsa->sym = sym;
> +	fsa->crc = crc;
>  	fsa->owner = owner;
> -	fsa->crc = symversion(syms->crcs, symnum);
> -	fsa->sym = &syms->start[symnum];
>  	return true;

Strange gratuitous reorder here?

> +static const char *export_section_names[EXPORT_TYPE_MAX] = {
> +	[EXPORT_TYPE_PLAIN] = "__ksymtab",
> +	[EXPORT_TYPE_GPL] = "__ksymtab_gpl",
> +#ifdef CONFIG_UNUSED_SYMBOLS
> +	[EXPORT_TYPE_UNUSED] = "__ksymtab_unused",
> +	[EXPORT_TYPE_UNUSED_GPL] = "__ksymtab_unused_gpl",
> +#endif
> +	[EXPORT_TYPE_GPL_FUTURE] = "__ksymtab_gpl_future",
> +};
> +
> +static const char *crc_section_names[EXPORT_TYPE_MAX] = {
> +	[EXPORT_TYPE_PLAIN] = "__kcrctab",
> +	[EXPORT_TYPE_GPL] = "__kcrctab_gpl",
> +#ifdef CONFIG_UNUSED_SYMBOLS
> +	[EXPORT_TYPE_UNUSED] = "__kcrctab_unused",
> +	[EXPORT_TYPE_UNUSED_GPL] = "__kcrctab_unused_gpl",
> +#endif
> +	[EXPORT_TYPE_GPL_FUTURE] = "__kcrctab_gpl_future",
> +};

You can use [] here for size instead of explicit EXPORT_TYPE_MAX.  We should
have named these sections better :(

> +	for (export_type = 0; export_type < EXPORT_TYPE_MAX; export_type++) {

Then use ARRAY_SIZE(export_section_names) here.

> +	for (export_type = 0; export_type < EXPORT_TYPE_MAX; export_type++) {
> +		if (mod->syms[export_type].syms &&

Similar ARRAY_SIZE(mod->syms).  It's less indirect, IMHO.

But all minor nitpicks; code is excellent!

Thanks,
Rusty.

Re: [PATCH 06/10] module: refactor symbol tables and try to reduce code size of each_symbol()

[Alan Jenkins]

Rusty Russell wrote:
>> +typedef bool each_symbol_fn_t (enum export_type type,
>> +			       const struct kernel_symbol *sym,
>> +			       const unsigned long *crc,
>> +			       struct module *owner,
>> +			       void *data);
>> +
>>  /* Walk the exported symbol table */
>> -bool each_symbol(bool (*fn)(const struct symsearch *arr, struct module *owner,
>> -			    unsigned int symnum, void *data), void *data);
>> +bool each_symbol(each_symbol_fn_t *fn, void *data);
>>     
>
> I really hate throwaway typedefs like this.  But it's used in two other
> places, so I'll suppress my distaste :)
>   

"const struct kernel_symbol *sym" was pushing lines over 80 columns in 
those other two places.


>> +static struct ksymtab ksymtab[EXPORT_TYPE_MAX];
>> +
>> +static int __init init_ksymtab(void)
>> +{
>> +	struct ksymtab tmp[EXPORT_TYPE_MAX] = {
>> +		[EXPORT_TYPE_PLAIN] = {
>> +			__start___ksymtab, __start___kcrctab,
>> +			__stop___ksymtab - __start___ksymtab,
>> +		},
>>     

>> +	};
>> +
>> +	memcpy(ksymtab, tmp, sizeof(ksymtab));
>>     
>
> This works, but I'd prefer you to open-code the assignments.  Simpler and
> marginally more efficient.
>   

Ok.

>> @@ -322,9 +322,9 @@ static bool find_symbol_in_section(const struct symsearch *syms,
>>  	}
>>  #endif
>>  
>> +	fsa->sym = sym;
>> +	fsa->crc = crc;
>>  	fsa->owner = owner;
>> -	fsa->crc = symversion(syms->crcs, symnum);
>> -	fsa->sym = &syms->start[symnum];
>>  	return true;
>>     
>
> Strange gratuitous reorder here?
>
>   

I can't see why I did that either.  I'll put it back in the original order.

>> +static const char *export_section_names[EXPORT_TYPE_MAX] = {
>> +	[EXPORT_TYPE_PLAIN] = "__ksymtab",
>> +	[EXPORT_TYPE_GPL] = "__ksymtab_gpl",
>> +#ifdef CONFIG_UNUSED_SYMBOLS
>> +	[EXPORT_TYPE_UNUSED] = "__ksymtab_unused",
>> +	[EXPORT_TYPE_UNUSED_GPL] = "__ksymtab_unused_gpl",
>> +#endif
>> +	[EXPORT_TYPE_GPL_FUTURE] = "__ksymtab_gpl_future",
>> +};
>> +
>> +static const char *crc_section_names[EXPORT_TYPE_MAX] = {
>> +	[EXPORT_TYPE_PLAIN] = "__kcrctab",
>> +	[EXPORT_TYPE_GPL] = "__kcrctab_gpl",
>> +#ifdef CONFIG_UNUSED_SYMBOLS
>> +	[EXPORT_TYPE_UNUSED] = "__kcrctab_unused",
>> +	[EXPORT_TYPE_UNUSED_GPL] = "__kcrctab_unused_gpl",
>> +#endif
>> +	[EXPORT_TYPE_GPL_FUTURE] = "__kcrctab_gpl_future",
>> +};
>>     
>
> You can use [] here for size instead of explicit EXPORT_TYPE_MAX.  We should
> have named these sections better :(
>
>   
>> +	for (export_type = 0; export_type < EXPORT_TYPE_MAX; export_type++) {
>>     
>
> Then use ARRAY_SIZE(export_section_names) here.
>
>   
>> +	for (export_type = 0; export_type < EXPORT_TYPE_MAX; export_type++) {
>> +		if (mod->syms[export_type].syms &&
>>     
>
> Similar ARRAY_SIZE(mod->syms).  It's less indirect, IMHO.
>   

The idea was to highlight EXPORT_TYPE when the loop variable is 
shortened to the less obvious "type".  But that only affects two sites 
(in patch 9), and in both cases it's fairly easy to see the definition

enum export_type type;

so on balance I have to agree.  I'll switch to using ARRAY_SIZE() 
throughout.

> But all minor nitpicks; code is excellent!
>
> Thanks,
> Rusty.
>   

Thanks for the close review
Alan

[PATCH 07/10] lib: Add generic binary search function to the kernel.

[Alan Jenkins]

From: Tim Abbott <tabbott@ksplice.com>

There a large number hand-coded binary searches in the kernel (run
"git grep search | grep binary" to find many of them).  Since in my
experience, hand-coding binary searches can be error-prone, it seems
worth cleaning this up by providing a generic binary search function.

This generic binary search implementation comes from Ksplice.  It has
the same basic API as the C library bsearch() function.  Ksplice uses
it in half a dozen places with 4 different comparison functions, and I
think our code is substantially cleaner because of this.

Signed-off-by: Tim Abbott <tabbott@ksplice.com>
Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
---
 include/linux/bsearch.h |    9 ++++++++
 lib/Makefile            |    2 +-
 lib/bsearch.c           |   53 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 63 insertions(+), 1 deletions(-)
 create mode 100644 include/linux/bsearch.h
 create mode 100644 lib/bsearch.c

diff --git a/include/linux/bsearch.h b/include/linux/bsearch.h
new file mode 100644
index 0000000..90b1aa8
--- /dev/null
+++ b/include/linux/bsearch.h
@@ -0,0 +1,9 @@
+#ifndef _LINUX_BSEARCH_H
+#define _LINUX_BSEARCH_H
+
+#include <linux/types.h>
+
+void *bsearch(const void *key, const void *base, size_t num, size_t size,
+	      int (*cmp)(const void *key, const void *elt));
+
+#endif /* _LINUX_BSEARCH_H */
diff --git a/lib/Makefile b/lib/Makefile
index 2e78277..fb60af1 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -21,7 +21,7 @@ lib-y	+= kobject.o kref.o klist.o
 
 obj-y += bcd.o div64.o sort.o parser.o halfmd4.o debug_locks.o random32.o \
 	 bust_spinlocks.o hexdump.o kasprintf.o bitmap.o scatterlist.o \
-	 string_helpers.o gcd.o
+	 string_helpers.o gcd.o bsearch.o
 
 ifeq ($(CONFIG_DEBUG_KOBJECT),y)
 CFLAGS_kobject.o += -DDEBUG
diff --git a/lib/bsearch.c b/lib/bsearch.c
new file mode 100644
index 0000000..4297c98
--- /dev/null
+++ b/lib/bsearch.c
@@ -0,0 +1,53 @@
+/*
+ * A generic implementation of binary search for the Linux kernel
+ *
+ * Copyright (C) 2008-2009 Ksplice, Inc.
+ * Author: Tim Abbott <tabbott@ksplice.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; version 2.
+ */
+
+#include <linux/module.h>
+#include <linux/bsearch.h>
+
+/*
+ * bsearch - binary search an array of elements
+ * @key: pointer to item being searched for
+ * @base: pointer to data to sort
+ * @num: number of elements
+ * @size: size of each element
+ * @cmp: pointer to comparison function
+ *
+ * This function does a binary search on the given array.  The
+ * contents of the array should already be in ascending sorted order
+ * under the provided comparison function.
+ *
+ * Note that the key need not have the same type as the elements in
+ * the array, e.g. key could be a string and the comparison function
+ * could compare the string with the struct's name field.  However, if
+ * the key and elements in the array are of the same type, you can use
+ * the same comparison function for both sort() and bsearch().
+ */
+void *bsearch(const void *key, const void *base, size_t num, size_t size,
+	      int (*cmp)(const void *key, const void *elt))
+{
+	int start = 0, end = num - 1, mid, result;
+	if (num == 0)
+		return NULL;
+
+	while (start <= end) {
+		mid = (start + end) / 2;
+		result = cmp(key, base + mid * size);
+		if (result < 0)
+			end = mid - 1;
+		else if (result > 0)
+			start = mid + 1;
+		else
+			return (void *)base + mid * size;
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL(bsearch);
-- 
1.6.3.2

[PATCH 08/10] lib: bsearch - remove redundant special case for arrays of size 0

[Alan Jenkins]

> On Thu, 24 Sep 2009, Rusty Russell wrote:
>
>> The if (num == 0) line is superfluous.

On 9/27/09, Tim Abbott <tabbott@ksplice.com> wrote:
>
> You are quite right.

Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
---
 lib/bsearch.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/lib/bsearch.c b/lib/bsearch.c
index 4297c98..2e70664 100644
--- a/lib/bsearch.c
+++ b/lib/bsearch.c
@@ -34,8 +34,6 @@ void *bsearch(const void *key, const void *base, size_t num, size_t size,
 	      int (*cmp)(const void *key, const void *elt))
 {
 	int start = 0, end = num - 1, mid, result;
-	if (num == 0)
-		return NULL;
 
 	while (start <= end) {
 		mid = (start + end) / 2;
-- 
1.6.3.2

[PATCH 09/10] module: speed up find_symbol() using binary search on the builtin symbol tables

[Alan Jenkins]

The builtin symbol tables are now sorted, so we can use a binary
search.

The symbol tables in modules are still unsorted and require linear
searching as before. But since the generic each_symbol() is removed,
the code size only goes up 8 bytes overall on i386.

On my EeePC 701, coldplug is mainly cpu bound and takes 1.5 seconds
during boot. perf showed this change eliminated 20% of cpu cycles during
coldplug, saving 0.3 seconds of real time.

These savings may not be representative since my config is not very well
tuned.  The numbers above represent the loading of 35 modules,
referencing a total of 441 symbols.  Nevertheless, it shows why I think
this is worth doing.

Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
---
 kernel/module.c |  144 ++++++++++++++++++++++++++++++++++--------------------
 1 files changed, 91 insertions(+), 53 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index 38a2859..122c10d 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -55,6 +55,7 @@
 #include <linux/async.h>
 #include <linux/percpu.h>
 #include <linux/kmemleak.h>
+#include <linux/bsearch.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/module.h>
@@ -272,36 +273,98 @@ bool each_symbol(each_symbol_fn_t *fn, void *data)
 }
 EXPORT_SYMBOL_GPL(each_symbol);
 
-struct find_symbol_arg {
-	/* Input */
-	const char *name;
-	bool gplok;
-	bool warn;
+static int symbol_compare(const void *key, const void *elt)
+{
+	const char *str = key;
+	const struct kernel_symbol *sym = elt;
+	return strcmp(str, sym->name);
+}
 
-	/* Output */
+/* binary search on sorted symbols */
+static const struct kernel_symbol *find_symbol_in_kernel(
+	const char *name,
+	enum export_type *t,
+	const unsigned long **crc)
+{
+	struct kernel_symbol *sym;
+	enum export_type type;
+	unsigned int i;
+
+	for (type = 0; type < EXPORT_TYPE_MAX; type++) {
+		sym = bsearch(name, ksymtab[type].syms, ksymtab[type].num_syms,
+			      sizeof(struct kernel_symbol), symbol_compare);
+		if (sym) {
+			i = sym - ksymtab[type].syms;
+			*crc = symversion(ksymtab[type].crcs, i);
+			*t = type;
+			return sym;
+		}
+	}
+
+	return NULL;
+}
+
+/* linear search on unsorted symbols */
+static const struct kernel_symbol *find_symbol_in_module(
+	struct module *mod,
+	const char *name,
+	enum export_type *t,
+	const unsigned long **crc)
+{
+	struct ksymtab *symtab = mod->syms;
 	const struct kernel_symbol *sym;
-	const unsigned long *crc;
-	struct module *owner;
-};
+	enum export_type type;
+	unsigned int i;
 
-static bool find_symbol_in_section(enum export_type type,
-				   const struct kernel_symbol *sym,
-				   const unsigned long *crc,
-				   struct module *owner,
-				   void *data)
+	for (type = 0; type < EXPORT_TYPE_MAX; type++) {
+		for (i = 0; i < symtab[type].num_syms; i++) {
+			sym = &symtab[type].syms[i];
+
+			if (strcmp(sym->name, name) == 0) {
+				*crc = symversion(symtab[type].crcs, i);
+				*t = type;
+				return sym;
+			}
+		}
+	}
+
+	return NULL;
+}
+
+/* Find a symbol and return it, along with, (optional) crc and
+ * (optional) module which owns it */
+const struct kernel_symbol *find_symbol(const char *name,
+					struct module **owner,
+					const unsigned long **crc,
+					bool gplok,
+					bool warn)
 {
-	struct find_symbol_arg *fsa = data;
+	struct module *mod = NULL;
+	const struct kernel_symbol *sym;
+	enum export_type type;
+	const unsigned long *crc_value;
 
-	if (strcmp(sym->name, fsa->name) != 0)
-		return false;
+	sym = find_symbol_in_kernel(name, &type, &crc_value);
+	if (sym)
+		goto found;
+
+	list_for_each_entry_rcu(mod, &modules, list) {
+		sym = find_symbol_in_module(mod, name, &type, &crc_value);
+		if (sym)
+			goto found;
+	}
+
+	DEBUGP("Failed to find symbol %s\n", name);
+	return NULL;
 
-	if (!fsa->gplok) {
+found:
+	if (!gplok) {
 		if (export_is_gpl_only(type))
-			return false;
-		if (export_is_gpl_future(type) && fsa->warn) {
+			return NULL;
+		if (export_is_gpl_future(type) && warn) {
 			printk(KERN_WARNING "Symbol %s is being used "
 			       "by a non-GPL module, which will not "
-			       "be allowed in the future\n", fsa->name);
+			       "be allowed in the future\n", name);
 			printk(KERN_WARNING "Please see the file "
 			       "Documentation/feature-removal-schedule.txt "
 			       "in the kernel source tree for more details.\n");
@@ -309,9 +372,9 @@ static bool find_symbol_in_section(enum export_type type,
 	}
 
 #ifdef CONFIG_UNUSED_SYMBOLS
-	if (export_is_unused(type) && fsa->warn) {
+	if (export_is_unused(type) && warn) {
 		printk(KERN_WARNING "Symbol %s is marked as UNUSED, "
-		       "however this module is using it.\n", fsa->name);
+		       "however this module is using it.\n", name);
 		printk(KERN_WARNING
 		       "This symbol will go away in the future.\n");
 		printk(KERN_WARNING
@@ -322,36 +385,11 @@ static bool find_symbol_in_section(enum export_type type,
 	}
 #endif
 
-	fsa->sym = sym;
-	fsa->crc = crc;
-	fsa->owner = owner;
-	return true;
-}
-
-/* Find a symbol and return it, along with, (optional) crc and
- * (optional) module which owns it */
-const struct kernel_symbol *find_symbol(const char *name,
-					struct module **owner,
-					const unsigned long **crc,
-					bool gplok,
-					bool warn)
-{
-	struct find_symbol_arg fsa;
-
-	fsa.name = name;
-	fsa.gplok = gplok;
-	fsa.warn = warn;
-
-	if (each_symbol(find_symbol_in_section, &fsa)) {
-		if (owner)
-			*owner = fsa.owner;
-		if (crc)
-			*crc = fsa.crc;
-		return fsa.sym;
-	}
-
-	DEBUGP("Failed to find symbol %s\n", name);
-	return NULL;
+	if (owner)
+		*owner = mod;
+	if (crc)
+		*crc = crc_value;
+	return sym;
 }
 EXPORT_SYMBOL_GPL(find_symbol);
 
-- 
1.6.3.2

Re: [PATCH 09/10] module: speed up find_symbol() using binary search on the builtin symbol tables

[Rusty Russell]

On Tue, 3 Nov 2009 08:36:21 pm Alan Jenkins wrote:
> +	for (type = 0; type < EXPORT_TYPE_MAX; type++) {
> +		sym = bsearch(name, ksymtab[type].syms, ksymtab[type].num_syms,
> +			      sizeof(struct kernel_symbol), symbol_compare);

One minor point: Prefer ARRAY_SIZE() here to EXPORT_TYPE_MAX.

It'd be cool if bsearch was typesafe, but that would freak out the Old School
kernel hackers :)

> +	for (type = 0; type < EXPORT_TYPE_MAX; type++) {
> +		for (i = 0; i < symtab[type].num_syms; i++) {
> +			sym = &symtab[type].syms[i];

Same.

Thanks!
Rusty.

[PATCH 10/10] module: fix is_exported() to return true for all types of exports

[Alan Jenkins]

/proc/kallsyms annotates module symbols as global (e.g. 'D' for a data
symbol) or local (e.g. 'd'), depending on whether is_exported() returns
true or false.

Historically, is_exported() only returns true if the symbol was exported
using EXPORT_SYMBOL(). EXPORT_SYMBOL_UNUSED(), for example, is not taken
into account. This looks like an oversight, so let's fix it.

Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
---
 kernel/module.c |   30 ++++++++----------------------
 1 files changed, 8 insertions(+), 22 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index 122c10d..617e1f2 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1838,33 +1838,19 @@ static void free_modinfo(struct module *mod)
 
 #ifdef CONFIG_KALLSYMS
 
-/* lookup symbol in given range of kernel_symbols */
-static const struct kernel_symbol *lookup_symbol(const char *name,
-	const struct kernel_symbol *syms,
-	unsigned int count)
-{
-	unsigned int i;
-
-	for (i = 0; i < count; i++)
-		if (strcmp(syms[i].name, name) == 0)
-			return &syms[i];
-	return NULL;
-}
-
 static int is_exported(const char *name, unsigned long value,
-		       const struct module *mod)
+		       struct module *mod)
 {
-	const struct ksymtab *symtab;
-	const struct kernel_symbol *ks;
+	const struct kernel_symbol *sym;
+	enum export_type type;
+	const unsigned long *crc;
 
-	if (!mod)
-		symtab = &ksymtab[EXPORT_TYPE_PLAIN];
+	if (mod)
+		sym = find_symbol_in_module(mod, name, &type, &crc);
 	else
-		symtab = &mod->syms[EXPORT_TYPE_PLAIN];
-
-	ks = lookup_symbol(name, symtab->syms, symtab->num_syms);
+		sym = find_symbol_in_kernel(name, &type, &crc);
 
-	return ks != NULL && ks->value == value;
+	return (sym && sym->value == value);
 }
 
 /* As per nm */
-- 
1.6.3.2

Re: [PATCH 10/10] module: fix is_exported() to return true for all types of exports

[Rusty Russell]

On Tue, 3 Nov 2009 08:36:22 pm Alan Jenkins wrote:
> /proc/kallsyms annotates module symbols as global (e.g. 'D' for a data
> symbol) or local (e.g. 'd'), depending on whether is_exported() returns
> true or false.
> 
> Historically, is_exported() only returns true if the symbol was exported
> using EXPORT_SYMBOL(). EXPORT_SYMBOL_UNUSED(), for example, is not taken
> into account. This looks like an oversight, so let's fix it.
> 
> Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>

Yep, I noticed the same thing when you patched it.

Thanks!
Rusty.
PS.  Very happy with this series, if no other issues I'll take it happily!

Re: Fast LKM symbol resolution

[Carmelo Amoroso]

Alan Jenkins wrote:
> Here's my latest code using binary search for symbol resolution.  This
> version has benefited from a little more testing (heh), in particular
> running on a simulated ARM system.
> 
>     web: <http://github.com/sourcejedi/linux-2.6/commits/module-V2-beta1>
> 
>     git clone git://github.com/sourcejedi/linux-2.6.git module-V2-beta1
> 
> As before, it saves 0.3s boot time for a modular kernel on my netbook
> (630 Mhz Celeron M).  On that system it represents something like 90%
> of the maximum possible speedup.  It is claimed that slower systems
> would benefit from a further speedup (e.g. using hash tables instead),
> but we don't have any numbers to illustrate this yet.
> 
> 

Hi Alan,

I'm going to complete the port of the hash table work against the mainline
(I had an implementation based on older 2.6.23.17 and 2.6.30).
I have figures on sh4 arch (2.6.23.17) but I would like to provide you with benchmarks
using the mainline on x86 too.
In my implementation there are not arch specific parts.

I intend to provide this solution as optional for now, so this will force some
#ifdef in the code that could make it less clean, but if the solution is valuable,
I could do a code tidy-up later easily.

The work to use GNU hash & bloom filtering is almost completed too, but it needs
some more tests, so the first patches I'll send are based on the SysV hash table.

Apologies for the delay in providing patches.

Carmelo