Skip to contentSkip to conversation

module.c: add missing ifdefs for CONFIG_UNUSED_SYMBOLS

PATCH In review Denys Vlasenko 2 participants 3 messages Sep 13, 2007 Module support

lore
Share & tools
lore

Denys Vlasenko

author
#0 lore raw

PATCH 1/1 module.c: add missing ifdefs for CONFIG_UNUSED_SYMBOLS

Hi Andrew,

module.c and module.h conatains code for finding
exported symbols which are declared with EXPORT_UNUSED_SYMBOL, and this code is compiled in even if CONFIG_UNUSED_SYMBOLS is not set and thus there can be no EXPORT_UNUSED_SYMBOLs in modules anyway (because EXPORT_UNUSED_SYMBOL(x) are compiled out to nothing then).

This patch adds required #ifdefs.

This shrinks module.o and each *.ko file.

Patch also regroups some struct module members so that on 64 bit CPUs we are not wasting 32 bits on padding here:

        const struct kernel_symbol *unused_syms;
        unsigned int num_unused_syms;
        const unsigned long *unused_crcs;

It groups counters and pointers separately.

Patch makes small edit to help text of CONFIG_MODULE_UNLOAD - it explicitly says that without that option, kernel will be also faster, not only "smaller and simpler". When I realized how much churn is going on under the hood in order to make module unloading possible, I felt that users are not informed well enough about it in the help text.

And finally, structure members which hold length of module code (four such members there) and count of symbols are converted from longs to ints.

We cannot possibly have a module where 32 bits won't be enough to hold such counts.

For one, module loading checks module size for sanity before loading, so such insanely big module will fail that test first.

In short, patch makes trivial changes which are "obviously correct" (famous last words).

Patch is compile tested with various combinations of CONFIGs.

Please put it into -mm.

Signed-off-by Denys Vlasenko <[email protected]>

--
vda

include/linux/module.h +18 -16
@@ -264,28 +264,30 @@
 	struct kobject *holders_dir;
 
 	/* Exported symbols */
-	const struct kernel_symbol *syms;
 	unsigned int num_syms;
-	const unsigned long *crcs;
-
 	/* GPL-only exported symbols. */
-	const struct kernel_symbol *gpl_syms;
 	unsigned int num_gpl_syms;
-	const unsigned long *gpl_crcs;
-
+	/* symbols that will be GPL-only in the near future. */
+	unsigned int num_gpl_future_syms;
+#ifdef CONFIG_UNUSED_SYMBOLS
 	/* unused exported symbols. */
-	const struct kernel_symbol *unused_syms;
 	unsigned int num_unused_syms;
-	const unsigned long *unused_crcs;
 	/* GPL-only, unused exported symbols. */
-	const struct kernel_symbol *unused_gpl_syms;
 	unsigned int num_unused_gpl_syms;
-	const unsigned long *unused_gpl_crcs;
-
-	/* symbols that will be GPL-only in the near future. */
+#endif
+	/* and respective pointers... */
+	const struct kernel_symbol *syms;
+	const unsigned long *crcs;
+	const struct kernel_symbol *gpl_syms;
+	const unsigned long *gpl_crcs;
 	const struct kernel_symbol *gpl_future_syms;
-	unsigned int num_gpl_future_syms;
 	const unsigned long *gpl_future_crcs;
+#ifdef CONFIG_UNUSED_SYMBOLS
+	const struct kernel_symbol *unused_syms;
+	const unsigned long *unused_crcs;
+	const struct kernel_symbol *unused_gpl_syms;
+	const unsigned long *unused_gpl_crcs;
+#endif
 
 	/* Exception table */
 	unsigned int num_exentries;

@@ -301,10 +303,10 @@
 	void *module_core;
 
 	/* Here are the sizes of the init and core sections */
-	unsigned long init_size, core_size;
+	unsigned int init_size, core_size;
 
 	/* The size of the executable code in each section.  */
-	unsigned long init_text_size, core_text_size;
+	unsigned int init_text_size, core_text_size;
 
 	/* The handle returned from unwind_add_table. */
 	void *unwind_info;

@@ -341,7 +343,7 @@
 #ifdef CONFIG_KALLSYMS
 	/* We keep the symbol and string tables for kallsyms. */
 	Elf_Sym *symtab;
-	unsigned long num_symtab;
+	unsigned int num_symtab;
 	char *strtab;
 
 	/* Section attributes */
init/Kconfig +2 -2
@@ -611,8 +611,8 @@
 	help
 	  Without this option you will not be able to unload any
 	  modules (note that some modules may not be unloadable
-	  anyway), which makes your kernel slightly smaller and
-	  simpler.  If unsure, say Y.
+	  anyway), which makes your kernel smaller, faster
+	  and simpler.  If unsure, say Y.
 
 config MODULE_FORCE_UNLOAD
 	bool "Forced module unloading"
kernel/module.c +30 -12

154 lines not on this page · raw message

#1 lore raw
Quoted 33 lines · On Thu, 13 Sep 2007 23:30:33 +0100 Denys Vlasenko <[email protected]>…

module.c and module.h conatains code for finding
exported symbols which are declared with EXPORT_UNUSED_SYMBOL, and this code is compiled in even if CONFIG_UNUSED_SYMBOLS is not set and thus there can be no EXPORT_UNUSED_SYMBOLs in modules anyway (because EXPORT_UNUSED_SYMBOL(x) are compiled out to nothing then).

This patch adds required #ifdefs.

This shrinks module.o and each *.ko file.

Patch also regroups some struct module members so that on 64 bit CPUs we are not wasting 32 bits on padding here:

        const struct kernel_symbol *unused_syms;
        unsigned int num_unused_syms;
        const unsigned long *unused_crcs;

It groups counters and pointers separately.

Patch makes small edit to help text of CONFIG_MODULE_UNLOAD - it explicitly says that without that option, kernel will be also faster, not only "smaller and simpler". When I realized how much churn is going on under the hood in order to make module unloading possible, I felt that users are not informed well enough about it in the help text.

And finally, structure members which hold length of module code (four such members there) and count of symbols are converted from longs to ints.

We cannot possibly have a module where 32 bits won't be enough to hold such counts.

Quoted 6 lines

For one, module loading checks module size for sanity before loading, so such insanely big module will fail that test first.

In short, patch makes trivial changes which are "obviously correct" (famous last words).

The intent seems reasonable. Would have preferred separate patches for the separate things though..

This:

akpm:/usr/src/25> grep '^+#' patches/modulec-add-missing-ifdefs-for-config_unused_symbols.patch +#ifdef CONFIG_UNUSED_SYMBOLS
+#endif
+#ifdef CONFIG_UNUSED_SYMBOLS
+#endif
+#ifdef CONFIG_UNUSED_SYMBOLS
+#endif
+#ifdef CONFIG_UNUSED_SYMBOLS
+#endif
+#ifdef CONFIG_UNUSED_SYMBOLS
+#endif
+#ifdef CONFIG_UNUSED_SYMBOLS
+#endif
+#ifdef CONFIG_UNUSED_SYMBOLS
+#endif
+#ifdef CONFIG_UNUSED_SYMBOLS
+#endif
+#ifdef CONFIG_UNUSED_SYMBOLS
+#endif
+#ifdef CONFIG_UNUSED_SYMBOLS
+#endif
+#ifdef CONFIG_UNUSED_SYMBOLS
+#endif

is a bit of a maintenance problem though. Can you think of a way in whcih we can cut down on that?

author
#2 lore raw
Quoted 31 lines · Andrew Morton
Quoted 2 lines

In short, patch makes trivial changes which are "obviously correct" (famous last words).

The intent seems reasonable. Would have preferred separate patches for the separate things though..

This:

akpm:/usr/src/25> grep '^+#' patches/modulec-add-missing-ifdefs-for-config_unused_symbols.patch +#ifdef CONFIG_UNUSED_SYMBOLS

(quoted diff) +21 -0
+#endif
+#ifdef CONFIG_UNUSED_SYMBOLS
+#endif
+#ifdef CONFIG_UNUSED_SYMBOLS
+#endif
+#ifdef CONFIG_UNUSED_SYMBOLS
+#endif
+#ifdef CONFIG_UNUSED_SYMBOLS
+#endif
+#ifdef CONFIG_UNUSED_SYMBOLS
+#endif
+#ifdef CONFIG_UNUSED_SYMBOLS
+#endif
+#ifdef CONFIG_UNUSED_SYMBOLS
+#endif
+#ifdef CONFIG_UNUSED_SYMBOLS
+#endif
+#ifdef CONFIG_UNUSED_SYMBOLS
+#endif
+#ifdef CONFIG_UNUSED_SYMBOLS
+#endif
Quoted 2 lines

is a bit of a maintenance problem though. Can you think of a way in whcih we can cut down on that?

Attached patch reduces number of inserted #ifdefs from 11 to 8. --
vda

include/linux/module.h +18 -17
diff -urpN linux-2.6.23-rc6/include/linux/module.h linux-2.6.23-rc6.module/include/linux/module.h
@@ -263,28 +263,29 @@ struct module
 	const char *srcversion;
 	struct kobject *holders_dir;
 
-	/* Exported symbols */
-	const struct kernel_symbol *syms;
+	/* Counts: exported symbols */
 	unsigned int num_syms;
-	const unsigned long *crcs;
-
-	/* GPL-only exported symbols. */
-	const struct kernel_symbol *gpl_syms;
+	/* GPL-only exported symbols */
 	unsigned int num_gpl_syms;
-	const unsigned long *gpl_crcs;
+	/* symbols that will be GPL-only in the near future */
+	unsigned int num_gpl_future_syms;
+#ifdef CONFIG_UNUSED_SYMBOLS
+	/* unused exported symbols */
+	unsigned int num_unused_syms;
+	/* GPL-only, unused exported symbols */
+	unsigned int num_unused_gpl_syms;
 
-	/* unused exported symbols. */
+	/* And respective pointers */
 	const struct kernel_symbol *unused_syms;
-	unsigned int num_unused_syms;
 	const unsigned long *unused_crcs;
-	/* GPL-only, unused exported symbols. */
 	const struct kernel_symbol *unused_gpl_syms;
-	unsigned int num_unused_gpl_syms;
 	const unsigned long *unused_gpl_crcs;
-
-	/* symbols that will be GPL-only in the near future. */
+#endif
+	const struct kernel_symbol *syms;
+	const unsigned long *crcs;
+	const struct kernel_symbol *gpl_syms;
+	const unsigned long *gpl_crcs;
 	const struct kernel_symbol *gpl_future_syms;
-	unsigned int num_gpl_future_syms;
 	const unsigned long *gpl_future_crcs;
 
 	/* Exception table */

@@ -301,10 +302,10 @@ struct module
 	void *module_core;
 
 	/* Here are the sizes of the init and core sections */
-	unsigned long init_size, core_size;
+	unsigned int init_size, core_size;
 
 	/* The size of the executable code in each section.  */
-	unsigned long init_text_size, core_text_size;
+	unsigned int init_text_size, core_text_size;
 
 	/* The handle returned from unwind_add_table. */
 	void *unwind_info;

@@ -341,7 +342,7 @@ struct module
 #ifdef CONFIG_KALLSYMS
 	/* We keep the symbol and string tables for kallsyms. */
 	Elf_Sym *symtab;
-	unsigned long num_symtab;
+	unsigned int num_symtab;
 	char *strtab;
 
 	/* Section attributes */
init/Kconfig +2 -2
diff -urpN linux-2.6.23-rc6/init/Kconfig linux-2.6.23-rc6.module/init/Kconfig
@@ -611,8 +611,8 @@ config MODULE_UNLOAD
 	help
 	  Without this option you will not be able to unload any
 	  modules (note that some modules may not be unloadable
-	  anyway), which makes your kernel slightly smaller and
-	  simpler.  If unsure, say Y.
+	  anyway), which makes your kernel smaller, faster
+	  and simpler.  If unsure, say Y.
 
 config MODULE_FORCE_UNLOAD
 	bool "Forced module unloading"
kernel/module.c +26 -12
diff -urpN linux-2.6.23-rc6/kernel/module.c linux-2.6.23-rc6.module/kernel/module.c
@@ -158,6 +158,7 @@ static const struct kernel_symbol *looku
 	return NULL;
 }
 
+#ifdef CONFIG_UNUSED_SYMBOLS
 static void printk_unused_warning(const char *name)
 {
 	printk(KERN_WARNING "Symbol %s is marked as UNUSED, "

@@ -168,6 +169,7 @@ static void printk_unused_warning(const 
 		"mailinglist together with submitting your code for "
 		"inclusion.\n");
 }
+#endif
 
 /* Find a symbol, return value, crc and module which owns it */
 static unsigned long __find_symbol(const char *name,

@@ -211,6 +213,7 @@ static unsigned long __find_symbol(const
 		return ks->value;
 	}
 
+#ifdef CONFIG_UNUSED_SYMBOLS
 	ks = lookup_symbol(name, __start___ksymtab_unused,
 				 __stop___ksymtab_unused);
 	if (ks) {

@@ -229,6 +232,7 @@ static unsigned long __find_symbol(const
 				  (ks - __start___ksymtab_unused_gpl));
 		return ks->value;
 	}
+#endif
 
 	/* Now try modules. */ 
 	list_for_each_entry(mod, &modules, list) {

@@ -248,13 +252,13 @@ static unsigned long __find_symbol(const
 				return ks->value;
 			}
 		}
+#ifdef CONFIG_UNUSED_SYMBOLS
 		ks = lookup_symbol(name, mod->unused_syms, mod->unused_syms + mod->num_unused_syms);
 		if (ks) {
 			printk_unused_warning(name);
 			*crc = symversion(mod->unused_crcs, (ks - mod->unused_syms));
 			return ks->value;
 		}
-
 		if (gplok) {
 			ks = lookup_symbol(name, mod->unused_gpl_syms,
 					   mod->unused_gpl_syms + mod->num_unused_gpl_syms);

@@ -265,6 +269,7 @@ static unsigned long __find_symbol(const
 				return ks->value;
 			}
 		}
+#endif
 		ks = lookup_symbol(name, mod->gpl_future_syms,
 				   (mod->gpl_future_syms +
 				    mod->num_gpl_future_syms));

@@ -1332,7 +1337,7 @@ static int simplify_symbols(Elf_Shdr *se
 }
 
 /* Update size with this section: return offset. */
-static long get_offset(unsigned long *size, Elf_Shdr *sechdr)
+static long get_offset(unsigned int *size, Elf_Shdr *sechdr)
 {
 	long ret;
 

@@ -1570,10 +1575,12 @@ static struct module *load_module(void _
 	unsigned int gplfutureindex;
 	unsigned int gplfuturecrcindex;
 	unsigned int unwindex = 0;
+#ifdef CONFIG_UNUSED_SYMBOLS
 	unsigned int unusedindex;
 	unsigned int unusedcrcindex;
 	unsigned int unusedgplindex;
 	unsigned int unusedgplcrcindex;
+#endif
 	struct module *mod;
 	long err = 0;
 	void *percpu = NULL, *ptr = NULL; /* Stops spurious gcc warning */

@@ -1654,13 +1661,15 @@ static struct module *load_module(void _
 	exportindex = find_sec(hdr, sechdrs, secstrings, "__ksymtab");
 	gplindex = find_sec(hdr, sechdrs, secstrings, "__ksymtab_gpl");
 	gplfutureindex = find_sec(hdr, sechdrs, secstrings, "__ksymtab_gpl_future");
-	unusedindex = find_sec(hdr, sechdrs, secstrings, "__ksymtab_unused");
-	unusedgplindex = find_sec(hdr, sechdrs, secstrings, "__ksymtab_unused_gpl");
 	crcindex = find_sec(hdr, sechdrs, secstrings, "__kcrctab");
 	gplcrcindex = find_sec(hdr, sechdrs, secstrings, "__kcrctab_gpl");
 	gplfuturecrcindex = find_sec(hdr, sechdrs, secstrings, "__kcrctab_gpl_future");
+#ifdef CONFIG_UNUSED_SYMBOLS
+	unusedindex = find_sec(hdr, sechdrs, secstrings, "__ksymtab_unused");
+	unusedgplindex = find_sec(hdr, sechdrs, secstrings, "__ksymtab_unused_gpl");
 	unusedcrcindex = find_sec(hdr, sechdrs, secstrings, "__kcrctab_unused");
 	unusedgplcrcindex = find_sec(hdr, sechdrs, secstrings, "__kcrctab_unused_gpl");
+#endif
 	setupindex = find_sec(hdr, sechdrs, secstrings, "__param");
 	exindex = find_sec(hdr, sechdrs, secstrings, "__ex_table");
 	obsparmindex = find_sec(hdr, sechdrs, secstrings, "__obsparm");

@@ -1813,27 +1822,32 @@ static struct module *load_module(void _
 		mod->gpl_crcs = (void *)sechdrs[gplcrcindex].sh_addr;
 	mod->num_gpl_future_syms = sechdrs[gplfutureindex].sh_size /
 					sizeof(*mod->gpl_future_syms);
-	mod->num_unused_syms = sechdrs[unusedindex].sh_size /
-					sizeof(*mod->unused_syms);
-	mod->num_unused_gpl_syms = sechdrs[unusedgplindex].sh_size /
-					sizeof(*mod->unused_gpl_syms);
 	mod->gpl_future_syms = (void *)sechdrs[gplfutureindex].sh_addr;
 	if (gplfuturecrcindex)
 		mod->gpl_future_crcs = (void *)sechdrs[gplfuturecrcindex].sh_addr;
 
+#ifdef CONFIG_UNUSED_SYMBOLS
+	mod->num_unused_syms = sechdrs[unusedindex].sh_size /
+					sizeof(*mod->unused_syms);
+	mod->num_unused_gpl_syms = sechdrs[unusedgplindex].sh_size /
+					sizeof(*mod->unused_gpl_syms);
 	mod->unused_syms = (void *)sechdrs[unusedindex].sh_addr;
 	if (unusedcrcindex)
 		mod->unused_crcs = (void *)sechdrs[unusedcrcindex].sh_addr;
 	mod->unused_gpl_syms = (void *)sechdrs[unusedgplindex].sh_addr;
 	if (unusedgplcrcindex)
 		mod->unused_crcs = (void *)sechdrs[unusedgplcrcindex].sh_addr;
+#endif
 
 #ifdef CONFIG_MODVERSIONS
 	if ((mod->num_syms && !crcindex) || 
 	    (mod->num_gpl_syms && !gplcrcindex) ||
-	    (mod->num_gpl_future_syms && !gplfuturecrcindex) ||
-	    (mod->num_unused_syms && !unusedcrcindex) ||
-	    (mod->num_unused_gpl_syms && !unusedgplcrcindex)) {
+	    (mod->num_gpl_future_syms && !gplfuturecrcindex)
+#ifdef CONFIG_UNUSED_SYMBOLS
+	    || (mod->num_unused_syms && !unusedcrcindex)
+	    || (mod->num_unused_gpl_syms && !unusedgplcrcindex)
+#endif
+	) {
 		printk(KERN_WARNING "%s: No versions for exported symbols."
 		       " Tainting kernel.\n", mod->name);
 		add_taint_module(mod, TAINT_FORCED_MODULE);

@@ -2269,7 +2283,7 @@ static int m_show(struct seq_file *m, vo
 	struct module *mod = list_entry(p, struct module, list);
 	char buf[8];
 
-	seq_printf(m, "%s %lu",
+	seq_printf(m, "%s %u",
 		   mod->name, mod->init_size + mod->core_size);
 	print_unload_info(m, mod);