All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCHv3 0/9] Mark literal strings in __init / __exit code
@ 2014-08-21 12:23 Mathias Krause
  2014-08-21 12:23 ` [PATCHv3 1/9] init.h: Add __init_str / __exit_str macros Mathias Krause
                   ` (10 more replies)
  0 siblings, 11 replies; 18+ messages in thread
From: Mathias Krause @ 2014-08-21 12:23 UTC (permalink / raw)
  To: Andrew Morton, Thomas Gleixner, Ingo Molnar, H. Peter Anvin
  Cc: Joe Perches, Rasmus Villemoes, linux-kernel, Mathias Krause

This is v3 of the patch series initially posted here:

  https://lkml.org/lkml/2014/6/22/149

This series tries to address the problem of dangling strings of __init
functions after initialization, as well as __exit strings for code not
even included in the final kernel image. The code might get freed, but
the format strings are not, as they're in the wrong section.

One solution to the problem might be to declare variables in the code
and mark those variables as __initconst. That, though, makes the code
ugly, as can be seen, e.g., in drivers/hwmon/w83627ehf.c -- a pile of
'static const char[] __initconst' lines just for the pr_info() call.

To be able to mark strings easily patch 1 adds macros to init.h to do so
without the need to explicitly define variables in the code. (Internally
it'll declare ones nonetheless, as this seem to be the only way to
attach an __attribute__() to a verbatim string.) That's already enough to
solve the problem -- mark the corresponding stings by using these
macros. But patch 2 adds some syntactical sugar for the most popular use
case, by providing pr_<level> alike macros, namely pi_<level> for __init
code and pe_<level> for __exit code. This hides the use of the marker
macros behind the commonly known printing functions -- with just a
single character changed. For code that cannot be changed to use the
pi_<level>() / pe_<level>() helpers printk_init() and printk_exit()
macros are provided, too.

The (hidden) variables used in the macros of patch 1 will pollute the
kernel's symbol table with unneeded entries. It'll be a problem in the
KALLSYMS_ALL case only, however, patch 3 takes care of filtering the
pseudo symbols. They have no value for us beside being the only way to
attach an __attribute__ to a string literal.

If users of the new macros get it wrong, e.g. use printk_init() /
pi_<level>() in non-init code or vice versa printk_exit() / pe_<level>()
in non-exit code, modpost will detect the error as a section mismatch
and report it accordingly. Still, the message printed by modpost with
CONFIG_DEBUG_SECTION_MISMATCH=y is rather confusing as the __initconst /
__exitdata annotation is hidden behind the macros. That's what patch 4
takes care of -- detecting such cases and providing better modpost
messages, guiding the user on how to fix the error.

The remaining patches (5 to 9) exemplarily change strings and format
strings in a selection of files under arch/x86/ to use the new macros.
They also address a few styling issues, e.g., patches 4 and 5 are
cleanup patches I stumbled across while changing the corresponding code
to make use of the new pi_*() helpers. The changes to arch/x86/ already
lead to moving ~3kb of memory from .rodata to .init.rodata. This should
free up a page after init on almost any x86 system.

To show that there's actual more value to it: A hacked up script, dully
changing pr_<level> to pi_<level> for __init functions under arch/x86/
is able to move ~8kB of r/o data into the .init section (partly already
covered by the patches of this series). The script, though, is dump. It
does not handle any of the printk() calls, nor does it handle panic()
calls or other strings used only in initialization code. So there's more
to squeeze out.


Open issues:

1/ When CONFIG_DYNAMIC_DEBUG is enabled, pi_debug() and pe_debug()
fall-back to pr_debug() as there is currently no way of removing the
dynamic entries from the dynamic debug code after init.

2/ It only seamlessly integrates for the pr_<level>() / printk() kind of
use cases. For other literal strings it gets slightly less readable,
e.g. this:

   strncmp(str, "s4_nohwsig", 10)

becomes that:

   strncmp(str, __init_str("s4_nohwsig"), 10)

That might be okay, though, as it marks the string clearly as an init
string, so might actually increase the understanding of the life time of
the string literal.

3/ Documentation: Should the new helpers be documented any further than
already done with the patches themselves, e.g. through a new entry to
Documentation/DocBook/kernel-hacking.tmpl? I'm uncertain as there is no
entry for the pr_<level>() helpers but for the __init / __exit markers.


If this series looks good to you as is, I think patches 1 to 4 should be
merged through Andrew, patches 5 to 9 through the x86 maintainers tree.
Would that work?


Regards,
Mathias

v3: 
- mention modpost error catching on wrong usage in patch 2 (Andrew)
- handle __init / __exit strings in modpost for better diagnostics
- reordered patches: 1-4: arch agnostic, 5-9: x86 specific
- rebased to v3.17-rc1

v2:
- introduce printk_init / printk_exit macros (suggested by Joe)
- filter __init / __exit string pseudo symols in kallsyms
- more x86 related patches making use of the new helpers


Mathias Krause (9):
  init.h: Add __init_str / __exit_str macros
  printk: Provide pi_<level> / pe_<level> macros for __init / __exit
    code
  kallsyms: exclude pseudo symbols for __init / __exit strings
  modpost: provide better diagnostics for __init / __exit strings
  x86, acpi: Mark __init strings as such
  x86, mm: Make x86_init.memory_setup() return a const char *
  x86, mm: early_panic() - pass on the message as string
  x86, mm: e820 - mark __init strings as such
  x86: setup - mark __init strings as such

 arch/x86/include/asm/e820.h     |    4 +-
 arch/x86/include/asm/x86_init.h |    2 +-
 arch/x86/kernel/acpi/boot.c     |  155 ++++++++++++++++++---------------------
 arch/x86/kernel/acpi/sleep.c    |   17 ++---
 arch/x86/kernel/e820.c          |   95 ++++++++++++------------
 arch/x86/kernel/setup.c         |   70 +++++++++---------
 arch/x86/lguest/boot.c          |    2 +-
 arch/x86/xen/setup.c            |    4 +-
 arch/x86/xen/xen-ops.h          |    4 +-
 include/linux/init.h            |   23 ++++++
 include/linux/printk.h          |   59 +++++++++++++++
 scripts/kallsyms.c              |   13 ++++
 scripts/mod/modpost.c           |   94 +++++++++++++++++++-----
 13 files changed, 339 insertions(+), 203 deletions(-)

-- 
1.7.10.4


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCHv3 1/9] init.h: Add __init_str / __exit_str macros
  2014-08-21 12:23 [PATCHv3 0/9] Mark literal strings in __init / __exit code Mathias Krause
@ 2014-08-21 12:23 ` Mathias Krause
  2014-08-21 12:23 ` [PATCHv3 2/9] printk: Provide pi_<level> / pe_<level> macros for __init / __exit code Mathias Krause
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 18+ messages in thread
From: Mathias Krause @ 2014-08-21 12:23 UTC (permalink / raw)
  To: Andrew Morton, Thomas Gleixner, Ingo Molnar, H. Peter Anvin
  Cc: Joe Perches, Rasmus Villemoes, linux-kernel, Mathias Krause

Add macros to be able to mark string literals used in __init / __exit
functions.

Signed-off-by: Mathias Krause <minipli@googlemail.com>

---
v2: - use a better code example
---
 include/linux/init.h |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/include/linux/init.h b/include/linux/init.h
index 2df8e8dd10..2c1cf10bb7 100644
--- a/include/linux/init.h
+++ b/include/linux/init.h
@@ -35,6 +35,20 @@
  * Don't forget to initialize data not at file scope, i.e. within a function,
  * as gcc otherwise puts the data into the bss section and not into the init
  * section.
+ *
+ * For strings used in __init / __exit functions the __init_str() /
+ * __exit_str() macros will take care of marking the strings accordingly so
+ * they can be freed, too. Otherwise the strings would resist in memory, even
+ * though they are no longer referenced.
+ *
+ * Use them like this:
+ *
+ * static int __init my_setup(char *arg)
+ * {
+ *    if (!strcmp(arg, __init_str("disable")))
+ *       enabled = false;
+ * }
+ * __setup("mydev=", my_setup);
  */
 
 /* These are for everybody (although not all archs will actually
@@ -45,6 +59,12 @@
 #define __exitdata	__section(.exit.data)
 #define __exit_call	__used __section(.exitcall.exit)
 
+/* Those can be used to mark strings used in __init / __exit functions. */
+#define __init_str(str)	__mark_str(str, __UNIQUE_ID(_init_str_), __initconst)
+#define __exit_str(str)	__mark_str(str, __UNIQUE_ID(_exit_str_), __exitdata)
+#define __mark_str(str, var, __section) \
+	({ static const char var[] __section __aligned(1) = str; var; })
+
 /*
  * Some architecture have tool chains which do not handle rodata attributes
  * correctly. For those disable special sections for const, so that other
-- 
1.7.10.4


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCHv3 2/9] printk: Provide pi_<level> / pe_<level> macros for __init / __exit code
  2014-08-21 12:23 [PATCHv3 0/9] Mark literal strings in __init / __exit code Mathias Krause
  2014-08-21 12:23 ` [PATCHv3 1/9] init.h: Add __init_str / __exit_str macros Mathias Krause
@ 2014-08-21 12:23 ` Mathias Krause
  2014-08-21 12:23 ` [PATCHv3 3/9] kallsyms: exclude pseudo symbols for __init / __exit strings Mathias Krause
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 18+ messages in thread
From: Mathias Krause @ 2014-08-21 12:23 UTC (permalink / raw)
  To: Andrew Morton, Thomas Gleixner, Ingo Molnar, H. Peter Anvin
  Cc: Joe Perches, Rasmus Villemoes, linux-kernel, Mathias Krause

The memory used for functions marked with __init will be released after
initialization, albeit static data referenced by such code will not, if
not explicitly marked this way, too. This is especially true for format
strings used in messages printed by such code. Those are not marked and
therefore will survive the __init cleanup -- dangling in memory without
ever being referenced again.

Ideally we would like the compiler to automatically recognise such
constructs but it does not and it's not as simple as it might sound, as
strings used in initialization code might latter still be used, e.g. as
a slab cache name. Therefore we need to explicitly mark the strings we
want to release together with the function itself.

For a seamless integration of the necessary __init_str() / __exit_str()
macros to mark the format strings, this patch provides new variants of
the well known pr_<level>() macros as pi_<level>() for __init code and
pe_<level>() for __exit code. Changing existing code should thereby be
as simple as changing a single letter.

For code that cannot be changed to use the pi_<level>() / pe_<level>()
helpers printk_init() and printk_exit() macros are provided, too.

If users get it wrong, e.g. use printk_init() / pi_<level>() in non-init
code or printk_exit() / pe_<level>() in non-exit code, modpost will
detect the error as a section mismatch and report it accordingly.

One remark, though: We cannot provide appropriate p[ie]_debug() macros
for the CONFIG_DYNAMIC_DEBUG case as there is (currently) no way to
remove entries from dyndbg after initialization. But supporting that
scenario would require more work (and code), therefore not necessarily
justifying the memory savings.

Signed-off-by: Mathias Krause <minipli@googlemail.com>

---
v3: change description to mention modpost error catching
v2: introduce printk_init / printk_exit macros (suggested by Joe)
---
 include/linux/init.h   |    7 ++++--
 include/linux/printk.h |   59 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+), 2 deletions(-)

diff --git a/include/linux/init.h b/include/linux/init.h
index 2c1cf10bb7..dc5691b2f3 100644
--- a/include/linux/init.h
+++ b/include/linux/init.h
@@ -39,14 +39,17 @@
  * For strings used in __init / __exit functions the __init_str() /
  * __exit_str() macros will take care of marking the strings accordingly so
  * they can be freed, too. Otherwise the strings would resist in memory, even
- * though they are no longer referenced.
+ * though they are no longer referenced. They're also used to implement the
+ * printk_init() / printk_exit() macros.
  *
  * Use them like this:
  *
  * static int __init my_setup(char *arg)
  * {
- *    if (!strcmp(arg, __init_str("disable")))
+ *    if (!strcmp(arg, __init_str("disable"))) {
+ *       printk_init("Disabled by commandline\n");
  *       enabled = false;
+ *   }
  * }
  * __setup("mydev=", my_setup);
  */
diff --git a/include/linux/printk.h b/include/linux/printk.h
index d78125f73a..3be8e228db 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -229,6 +229,10 @@ extern asmlinkage void dump_stack(void) __cold;
  * All of these will print unconditionally, although note that pr_debug()
  * and other debug macros are compiled out unless either DEBUG is defined
  * or CONFIG_DYNAMIC_DEBUG is set.
+ *
+ * The printk_init() and pi_<level>() macros shall be used in __init code in
+ * favour of printk() / pr_<level>(). The printk_exit() and pe_<level>()
+ * variants shall be used in __exit code.
  */
 #define pr_emerg(fmt, ...) \
 	printk(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
@@ -248,13 +252,60 @@ extern asmlinkage void dump_stack(void) __cold;
 #define pr_cont(fmt, ...) \
 	printk(KERN_CONT fmt, ##__VA_ARGS__)
 
+#define printk_init(fmt, ...) \
+	printk(__init_str(fmt), ##__VA_ARGS__)
+#define printk_exit(fmt, ...) \
+	printk(__exit_str(fmt), ##__VA_ARGS__)
+
+#define pi_emerg(fmt, ...) \
+	printk_init(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
+#define pi_alert(fmt, ...) \
+	printk_init(KERN_ALERT pr_fmt(fmt), ##__VA_ARGS__)
+#define pi_crit(fmt, ...) \
+	printk_init(KERN_CRIT pr_fmt(fmt), ##__VA_ARGS__)
+#define pi_err(fmt, ...) \
+	printk_init(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
+#define pi_warning(fmt, ...) \
+	printk_init(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
+#define pi_warn pi_warning
+#define pi_notice(fmt, ...) \
+	printk_init(KERN_NOTICE pr_fmt(fmt), ##__VA_ARGS__)
+#define pi_info(fmt, ...) \
+	printk_init(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
+#define pi_cont(fmt, ...) \
+	printk_init(KERN_CONT fmt, ##__VA_ARGS__)
+
+#define pe_emerg(fmt, ...) \
+	printk_exit(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
+#define pe_alert(fmt, ...) \
+	printk_exit(KERN_ALERT pr_fmt(fmt), ##__VA_ARGS__)
+#define pe_crit(fmt, ...) \
+	printk_exit(KERN_CRIT pr_fmt(fmt), ##__VA_ARGS__)
+#define pe_err(fmt, ...) \
+	printk_exit(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
+#define pe_warning(fmt, ...) \
+	printk_exit(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
+#define pe_warn pe_warning
+#define pe_notice(fmt, ...) \
+	printk_exit(KERN_NOTICE pr_fmt(fmt), ##__VA_ARGS__)
+#define pe_info(fmt, ...) \
+	printk_exit(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
+#define pe_cont(fmt, ...) \
+	printk_exit(KERN_CONT fmt, ##__VA_ARGS__)
+
 /* pr_devel() should produce zero code unless DEBUG is defined */
 #ifdef DEBUG
 #define pr_devel(fmt, ...) \
 	printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
+#define pi_devel(fmt, ...) \
+	printk_init(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
+#define pe_devel(fmt, ...) \
+	printk_exit(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
 #else
 #define pr_devel(fmt, ...) \
 	no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
+#define pi_devel pr_devel
+#define pe_devel pr_devel
 #endif
 
 #include <linux/dynamic_debug.h>
@@ -264,12 +315,20 @@ extern asmlinkage void dump_stack(void) __cold;
 /* dynamic_pr_debug() uses pr_fmt() internally so we don't need it here */
 #define pr_debug(fmt, ...) \
 	dynamic_pr_debug(fmt, ##__VA_ARGS__)
+#define pi_debug pr_debug
+#define pe_debug pr_debug
 #elif defined(DEBUG)
 #define pr_debug(fmt, ...) \
 	printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
+#define pi_debug(fmt, ...) \
+	printk_init(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
+#define pe_debug(fmt, ...) \
+	printk_exit(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
 #else
 #define pr_debug(fmt, ...) \
 	no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
+#define pi_debug pr_debug
+#define pe_debug pr_debug
 #endif
 
 /*
-- 
1.7.10.4


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCHv3 3/9] kallsyms: exclude pseudo symbols for __init / __exit strings
  2014-08-21 12:23 [PATCHv3 0/9] Mark literal strings in __init / __exit code Mathias Krause
  2014-08-21 12:23 ` [PATCHv3 1/9] init.h: Add __init_str / __exit_str macros Mathias Krause
  2014-08-21 12:23 ` [PATCHv3 2/9] printk: Provide pi_<level> / pe_<level> macros for __init / __exit code Mathias Krause
@ 2014-08-21 12:23 ` Mathias Krause
  2014-08-21 12:23 ` [PATCHv3 4/9] modpost: provide better diagnostics " Mathias Krause
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 18+ messages in thread
From: Mathias Krause @ 2014-08-21 12:23 UTC (permalink / raw)
  To: Andrew Morton, Thomas Gleixner, Ingo Molnar, H. Peter Anvin
  Cc: Joe Perches, Rasmus Villemoes, linux-kernel, Mathias Krause

The __init_str() / __exit_str() annotations create pseudo symbols.
Ignore them, they have no value for us.

Signed-off-by: Mathias Krause <minipli@googlemail.com>
---
 scripts/kallsyms.c |   13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
index dc7aa45e80..8fd0f2965d 100644
--- a/scripts/kallsyms.c
+++ b/scripts/kallsyms.c
@@ -88,6 +88,16 @@ static inline int is_arm_mapping_symbol(const char *str)
 	       && (str[2] == '\0' || str[2] == '.');
 }
 
+/*
+ * This ignores the pseudo symbols for __init_str() / __exit_str() annotated
+ * strings.
+ */
+static inline int is_init_exit_str_symbol(const char *str)
+{
+	return strncmp(str, "__UNIQUE_ID__init_str_", 22) == 0 ||
+	       strncmp(str, "__UNIQUE_ID__exit_str_", 22) == 0;
+}
+
 static int check_symbol_range(const char *sym, unsigned long long addr,
 			      struct addr_range *ranges, int entries)
 {
@@ -158,6 +168,9 @@ static int read_symbol(FILE *in, struct sym_entry *s)
 	/* exclude debugging symbols */
 	else if (stype == 'N')
 		return -1;
+	/* exclude pseudo symbols for __init / __exit strings */
+	else if (stype == 't' && is_init_exit_str_symbol(sym))
+		return -1;
 
 	/* include the type field in the symbol name, so that it gets
 	 * compressed together */
-- 
1.7.10.4


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCHv3 4/9] modpost: provide better diagnostics for __init / __exit strings
  2014-08-21 12:23 [PATCHv3 0/9] Mark literal strings in __init / __exit code Mathias Krause
                   ` (2 preceding siblings ...)
  2014-08-21 12:23 ` [PATCHv3 3/9] kallsyms: exclude pseudo symbols for __init / __exit strings Mathias Krause
@ 2014-08-21 12:23 ` Mathias Krause
  2014-08-21 12:23 ` [PATCHv3 5/9] x86, acpi: Mark __init strings as such Mathias Krause
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 18+ messages in thread
From: Mathias Krause @ 2014-08-21 12:23 UTC (permalink / raw)
  To: Andrew Morton, Thomas Gleixner, Ingo Molnar, H. Peter Anvin
  Cc: Joe Perches, Rasmus Villemoes, linux-kernel, Mathias Krause

Modpost will detect wrong usage of the pe_*() / pi_*() macros or the
__init_str() / __exit_str() annotations in general. But it'll print
cryptic diagnoses in this case that don't really help fixing the issue
in the code. Detect __init / __exit string section mismatches by there
unique symbol name pattern and provide better help texts in this case.

Signed-off-by: Mathias Krause <minipli@googlemail.com>
---
 This patch generates quite a few checkpatch warnings related to strings
 split across lines. But I choose to do so anyway to not make the code
 differ from other fprintf()s in this file.

 scripts/mod/modpost.c |   94 +++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 75 insertions(+), 19 deletions(-)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 091d90573b..7b462d5d01 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1150,6 +1150,16 @@ static inline int is_arm_mapping_symbol(const char *str)
 	       && (str[2] == '\0' || str[2] == '.');
 }
 
+static inline int is_init_str(const char *str)
+{
+	return !strncmp(str, "__UNIQUE_ID__init_str_", 22);
+}
+
+static inline int is_exit_str(const char *str)
+{
+	return !strncmp(str, "__UNIQUE_ID__exit_str_", 22);
+}
+
 /*
  * If there's no name there, ignore it; likewise, ignore it if it's
  * one of the magic symbols emitted used by current ARM tools.
@@ -1305,12 +1315,24 @@ static void report_sec_mismatch(const char *modname,
 		prl_to = sec2annotation(tosec);
 		fprintf(stderr,
 		"The function %s%s() references\n"
-		"the %s %s%s%s.\n"
-		"This is often because %s lacks a %s\n"
-		"annotation or the annotation of %s is wrong.\n",
+		"the %s %s%s%s.\n",
 		prl_from, fromsym,
-		to, prl_to, tosym, to_p,
-		fromsym, prl_to, tosym);
+		to, prl_to, tosym, to_p);
+		if (!to_is_func && is_init_str(tosym)) {
+			fprintf(stderr,
+			"This is probably due to a wrong use of the "
+			"pi_<level>() macro\n"
+			"or a wrongly __init_str() annotated string literal.\n"
+			"The fix is to either change the pi_<level>() to its "
+			"pr_<level>()\n"
+			"counterpart or to remove the __init_str() annotation "
+			"of the string literal\n");
+		} else {
+			fprintf(stderr,
+			"This is often because %s lacks a %s\n"
+			"annotation or the annotation of %s is wrong.\n",
+			fromsym, prl_to, tosym);
+		}
 		free(prl_from);
 		free(prl_to);
 		break;
@@ -1330,10 +1352,24 @@ static void report_sec_mismatch(const char *modname,
 	case TEXT_TO_ANY_EXIT:
 		prl_to = sec2annotation(tosec);
 		fprintf(stderr,
-		"The function %s() references a %s in an exit section.\n"
-		"Often the %s %s%s has valid usage outside the exit section\n"
-		"and the fix is to remove the %sannotation of %s.\n",
-		fromsym, to, to, tosym, to_p, prl_to, tosym);
+		"The function %s() references a %s in an exit section.\n",
+		fromsym, to);
+		if (!to_is_func && is_exit_str(tosym)) {
+			fprintf(stderr,
+			"This is probably due to a wrong use of the "
+			"pe_<level>() macro\n"
+			"or a wrongly __exit_str() annotated string literal.\n"
+			"The fix is to either change the pe_<level>() to its "
+			"pr_<level>()\n"
+			"counterpart or to remove the __exit_str() annotation "
+			"of the string literal\n");
+		} else {
+			fprintf(stderr,
+			"Often the %s %s%s has valid usage outside the exit "
+			"section\n"
+			"and the fix is to remove the %sannotation of %s.\n",
+			to, tosym, to_p, prl_to, tosym);
+		}
 		free(prl_to);
 		break;
 	case DATA_TO_ANY_EXIT: {
@@ -1372,12 +1408,22 @@ static void report_sec_mismatch(const char *modname,
 		"a %s %s%s%s.\n"
 		"This is often seen when error handling "
 		"in the init function\n"
-		"uses functionality in the exit path.\n"
-		"The fix is often to remove the %sannotation of\n"
-		"%s%s so it may be used outside an exit section.\n",
+		"uses functionality of the exit path.\n",
 		from, prl_from, fromsym, from_p,
-		to, prl_to, tosym, to_p,
-		prl_to, tosym, to_p);
+		to, prl_to, tosym, to_p);
+		if (!to_is_func && is_exit_str(tosym)) {
+			fprintf(stderr,
+			"The fix is often to either change the pe_<level>() to "
+			"its pr_<level>()\n"
+			"counterpart or to remove the __exit_str() annotation "
+			"of the string literal\n"
+			"so it may be used outside an exit section.\n");
+		} else {
+			fprintf(stderr,
+			"The fix is often to remove the %sannotation of\n"
+			"%s%s so it may be used outside an exit section.\n",
+			prl_to, tosym, to_p);
+		}
 		free(prl_from);
 		free(prl_to);
 		break;
@@ -1389,12 +1435,22 @@ static void report_sec_mismatch(const char *modname,
 		"a %s %s%s%s.\n"
 		"This is often seen when error handling "
 		"in the exit function\n"
-		"uses functionality in the init path.\n"
-		"The fix is often to remove the %sannotation of\n"
-		"%s%s so it may be used outside an init section.\n",
+		"uses functionality of the init path.\n",
 		from, prl_from, fromsym, from_p,
-		to, prl_to, tosym, to_p,
-		prl_to, tosym, to_p);
+		to, prl_to, tosym, to_p);
+		if (!to_is_func && is_init_str(tosym)) {
+			fprintf(stderr,
+			"The fix is often to either change the pi_<level>() to "
+			"its pr_<level>()\n"
+			"counterpart or to remove the __init_str() annotation "
+			"of the string literal\n"
+			"so it may be used outside an init section.\n");
+		} else {
+			fprintf(stderr,
+			"The fix is often to remove the %sannotation of\n"
+			"%s%s so it may be used outside an init section.\n",
+			prl_to, tosym, to_p);
+		}
 		free(prl_from);
 		free(prl_to);
 		break;
-- 
1.7.10.4


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCHv3 5/9] x86, acpi: Mark __init strings as such
  2014-08-21 12:23 [PATCHv3 0/9] Mark literal strings in __init / __exit code Mathias Krause
                   ` (3 preceding siblings ...)
  2014-08-21 12:23 ` [PATCHv3 4/9] modpost: provide better diagnostics " Mathias Krause
@ 2014-08-21 12:23 ` Mathias Krause
  2014-08-21 12:23 ` [PATCHv3 6/9] x86, mm: Make x86_init.memory_setup() return a const char * Mathias Krause
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 18+ messages in thread
From: Mathias Krause @ 2014-08-21 12:23 UTC (permalink / raw)
  To: Andrew Morton, Thomas Gleixner, Ingo Molnar, H. Peter Anvin
  Cc: Joe Perches, Rasmus Villemoes, linux-kernel, Mathias Krause

Make use of the pi_<level>() helpers to mark the strings printed during
initialization for automatic release. Do so for the strings used in
command line parsing as well, by using the __init_str() macro.

Also convert the remaining printk() calls to their pr_<level> /
pi_<level> counterparts.

This moves ~1.7 kB from the .rodata section to .init.rodata, marking it
for release after initialization.

Signed-off-by: Mathias Krause <minipli@googlemail.com>

---
v2: handle all of arch/x86/kernel/acpi/
---
 arch/x86/kernel/acpi/boot.c  |  155 +++++++++++++++++++-----------------------
 arch/x86/kernel/acpi/sleep.c |   17 +++--
 2 files changed, 79 insertions(+), 93 deletions(-)

diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index b436fc735a..1be7b86424 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -23,6 +23,8 @@
  * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  */
 
+#define pr_fmt(fmt) "ACPI: " fmt
+
 #include <linux/init.h>
 #include <linux/acpi.h>
 #include <linux/acpi_pmtmr.h>
@@ -55,7 +57,7 @@ EXPORT_SYMBOL(acpi_disabled);
 # include <asm/proto.h>
 #endif				/* X86 */
 
-#define PREFIX			"ACPI: "
+#define _(x)	__init_str(x)
 
 int acpi_noirq;				/* skip ACPI IRQ initialization */
 int acpi_pci_disabled;		/* skip ACPI PCI scan and IRQ initialization */
@@ -128,15 +130,14 @@ static int __init acpi_parse_madt(struct acpi_table_header *table)
 
 	madt = (struct acpi_table_madt *)table;
 	if (!madt) {
-		printk(KERN_WARNING PREFIX "Unable to map MADT\n");
+		pi_warn("Unable to map MADT\n");
 		return -ENODEV;
 	}
 
 	if (madt->address) {
 		acpi_lapic_addr = (u64) madt->address;
 
-		printk(KERN_DEBUG PREFIX "Local APIC address 0x%08x\n",
-		       madt->address);
+		pi_debug("Local APIC address 0x%08x\n", madt->address);
 	}
 
 	default_acpi_madt_oem_check(madt->header.oem_id,
@@ -157,7 +158,7 @@ static int acpi_register_lapic(int id, u8 enabled)
 	unsigned int ver = 0;
 
 	if (id >= MAX_LOCAL_APIC) {
-		printk(KERN_INFO PREFIX "skipped apicid that is too big\n");
+		pr_info("skipped apicid that is too big\n");
 		return -EINVAL;
 	}
 
@@ -197,11 +198,11 @@ acpi_parse_x2apic(struct acpi_subtable_header *header, const unsigned long end)
 	 * when we use CPU hotplug.
 	 */
 	if (!apic->apic_id_valid(apic_id) && enabled)
-		printk(KERN_WARNING PREFIX "x2apic entry ignored\n");
+		pi_warn("x2apic entry ignored\n");
 	else
 		acpi_register_lapic(apic_id, enabled);
 #else
-	printk(KERN_WARNING PREFIX "x2apic entry ignored\n");
+	pi_warn("x2apic entry ignored\n");
 #endif
 
 	return 0;
@@ -280,7 +281,7 @@ acpi_parse_x2apic_nmi(struct acpi_subtable_header *header,
 	acpi_table_print_madt_entry(header);
 
 	if (x2apic_nmi->lint != 1)
-		printk(KERN_WARNING PREFIX "NMI not connected to LINT 1!\n");
+		pi_warn("NMI not connected to LINT 1!\n");
 
 	return 0;
 }
@@ -298,7 +299,7 @@ acpi_parse_lapic_nmi(struct acpi_subtable_header * header, const unsigned long e
 	acpi_table_print_madt_entry(header);
 
 	if (lapic_nmi->lint != 1)
-		printk(KERN_WARNING PREFIX "NMI not connected to LINT 1!\n");
+		pi_warn("NMI not connected to LINT 1!\n");
 
 	return 0;
 }
@@ -514,14 +515,14 @@ acpi_parse_int_src_ovr(struct acpi_subtable_header * header,
 
 	if (intsrc->source_irq == 0) {
 		if (acpi_skip_timer_override) {
-			printk(PREFIX "BIOS IRQ0 override ignored.\n");
+			pi_info("BIOS IRQ0 override ignored.\n");
 			return 0;
 		}
 
 		if ((intsrc->global_irq == 2) && acpi_fix_pin2_polarity
 			&& (intsrc->inti_flags & ACPI_MADT_POLARITY_MASK)) {
 			intsrc->inti_flags &= ~ACPI_MADT_POLARITY_MASK;
-			printk(PREFIX "BIOS IRQ0 pin2 override: forcing polarity to high active.\n");
+			pi_info("BIOS IRQ0 pin2 override: forcing polarity to high active.\n");
 		}
 	}
 
@@ -597,7 +598,7 @@ void __init acpi_pic_sci_set_trigger(unsigned int irq, u16 trigger)
 	if (old == new)
 		return;
 
-	printk(PREFIX "setting ELCR to %04x (from %04x)\n", new, old);
+	pi_info("setting ELCR to %04x (from %04x)\n", new, old);
 	outb(new, 0x4d0);
 	outb(new >> 8, 0x4d1);
 }
@@ -719,7 +720,7 @@ static int _acpi_map_lsapic(acpi_handle handle, int physid, int *pcpu)
 
 	cpu = acpi_register_lapic(physid, ACPI_MADT_ENABLED);
 	if (cpu < 0) {
-		pr_info(PREFIX "Unable to map lapic to logical cpu number\n");
+		pr_info("Unable to map lapic to logical cpu number\n");
 		return cpu;
 	}
 
@@ -775,7 +776,7 @@ static int __init acpi_parse_sbf(struct acpi_table_header *table)
 
 	sb = (struct acpi_table_boot *)table;
 	if (!sb) {
-		printk(KERN_WARNING PREFIX "Unable to map SBF\n");
+		pi_warn("Unable to map SBF\n");
 		return -ENODEV;
 	}
 
@@ -795,13 +796,12 @@ static int __init acpi_parse_hpet(struct acpi_table_header *table)
 
 	hpet_tbl = (struct acpi_table_hpet *)table;
 	if (!hpet_tbl) {
-		printk(KERN_WARNING PREFIX "Unable to map HPET\n");
+		pi_warn("Unable to map HPET\n");
 		return -ENODEV;
 	}
 
 	if (hpet_tbl->address.space_id != ACPI_SPACE_MEM) {
-		printk(KERN_WARNING PREFIX "HPET timers must be located in "
-		       "memory.\n");
+		pi_warn("HPET timers must be located in memory.\n");
 		return -1;
 	}
 
@@ -813,9 +813,8 @@ static int __init acpi_parse_hpet(struct acpi_table_header *table)
 	 * want to allocate a resource there.
 	 */
 	if (!hpet_address) {
-		printk(KERN_WARNING PREFIX
-		       "HPET id: %#x base: %#lx is invalid\n",
-		       hpet_tbl->id, hpet_address);
+		pi_warn("HPET id: %#x base: %#lx is invalid\n", hpet_tbl->id,
+			hpet_address);
 		return 0;
 	}
 #ifdef CONFIG_X86_64
@@ -826,21 +825,19 @@ static int __init acpi_parse_hpet(struct acpi_table_header *table)
 	 */
 	if (hpet_address == 0xfed0000000000000UL) {
 		if (!hpet_force_user) {
-			printk(KERN_WARNING PREFIX "HPET id: %#x "
-			       "base: 0xfed0000000000000 is bogus\n "
-			       "try hpet=force on the kernel command line to "
-			       "fix it up to 0xfed00000.\n", hpet_tbl->id);
+			pi_warn("HPET id: %#x base: %#lx is bogus\n",
+				hpet_tbl->id, 0xfed0000000000000UL);
+			pi_cont("try hpet=force on the kernel command line "
+				"to fix it up to 0xfed00000.\n"),
 			hpet_address = 0;
 			return 0;
 		}
-		printk(KERN_WARNING PREFIX
-		       "HPET id: %#x base: 0xfed0000000000000 fixed up "
-		       "to 0xfed00000.\n", hpet_tbl->id);
+		pi_warn("HPET id: %#x base: %#lx fixed up to 0xfed00000.\n",
+			hpet_tbl->id, 0xfed0000000000000UL);
 		hpet_address >>= 32;
 	}
 #endif
-	printk(KERN_INFO PREFIX "HPET id: %#x base: %#lx\n",
-	       hpet_tbl->id, hpet_address);
+	pi_info("HPET id: %#x base: %#lx\n", hpet_tbl->id, hpet_address);
 
 	/*
 	 * Allocate and initialize the HPET firmware resource for adding into
@@ -851,7 +848,7 @@ static int __init acpi_parse_hpet(struct acpi_table_header *table)
 
 	hpet_res->name = (void *)&hpet_res[1];
 	hpet_res->flags = IORESOURCE_MEM;
-	snprintf((char *)hpet_res->name, HPET_RESOURCE_NAME_SIZE, "HPET %u",
+	snprintf((char *)hpet_res->name, HPET_RESOURCE_NAME_SIZE, _("HPET %u"),
 		 hpet_tbl->sequence);
 
 	hpet_res->start = hpet_address;
@@ -902,8 +899,7 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table)
 		pmtmr_ioport = acpi_gbl_FADT.pm_timer_block;
 	}
 	if (pmtmr_ioport)
-		printk(KERN_INFO PREFIX "PM-Timer IO Port: %#x\n",
-		       pmtmr_ioport);
+		pi_info("PM-Timer IO Port: %#x\n", pmtmr_ioport);
 #endif
 	return 0;
 }
@@ -929,8 +925,7 @@ static int __init early_acpi_parse_madt_lapic_addr_ovr(void)
 	count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC_OVERRIDE,
 				      acpi_parse_lapic_addr_ovr, 0);
 	if (count < 0) {
-		printk(KERN_ERR PREFIX
-		       "Error parsing LAPIC address override entry\n");
+		pi_err("Error parsing LAPIC address override entry\n");
 		return count;
 	}
 
@@ -955,8 +950,7 @@ static int __init acpi_parse_madt_lapic_entries(void)
 	count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC_OVERRIDE,
 				      acpi_parse_lapic_addr_ovr, 0);
 	if (count < 0) {
-		printk(KERN_ERR PREFIX
-		       "Error parsing LAPIC address override entry\n");
+		pi_err("Error parsing LAPIC address override entry\n");
 		return count;
 	}
 
@@ -972,11 +966,11 @@ static int __init acpi_parse_madt_lapic_entries(void)
 					acpi_parse_lapic, MAX_LOCAL_APIC);
 	}
 	if (!count && !x2count) {
-		printk(KERN_ERR PREFIX "No LAPIC entries present\n");
+		pi_err("No LAPIC entries present\n");
 		/* TBD: Cleanup to allow fallback to MPS */
 		return -ENODEV;
 	} else if (count < 0 || x2count < 0) {
-		printk(KERN_ERR PREFIX "Error parsing LAPIC entry\n");
+		pi_err("Error parsing LAPIC entry\n");
 		/* TBD: Cleanup to allow fallback to MPS */
 		return count;
 	}
@@ -986,7 +980,7 @@ static int __init acpi_parse_madt_lapic_entries(void)
 	count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC_NMI,
 				      acpi_parse_lapic_nmi, 0);
 	if (count < 0 || x2count < 0) {
-		printk(KERN_ERR PREFIX "Error parsing LAPIC NMI entry\n");
+		pi_err("Error parsing LAPIC NMI entry\n");
 		/* TBD: Cleanup to allow fallback to MPS */
 		return count;
 	}
@@ -1007,7 +1001,7 @@ static void __init mp_config_acpi_legacy_irqs(void)
 	mp_bus_id_to_type[MP_ISA_BUS] = MP_BUS_ISA;
 #endif
 	set_bit(MP_ISA_BUS, mp_bus_not_pci);
-	pr_debug("Bus #%d is ISA\n", MP_ISA_BUS);
+	pi_debug("Bus #%d is ISA\n", MP_ISA_BUS);
 
 	/*
 	 * Use the default configuration for the IRQs 0-15.  Unless
@@ -1045,7 +1039,7 @@ static void __init mp_config_acpi_legacy_irqs(void)
 		}
 
 		if (idx != mp_irq_entries) {
-			printk(KERN_DEBUG "ACPI: IRQ%d used by override.\n", i);
+			pi_debug("IRQ%d used by override.\n", i);
 			continue;	/* IRQ already used */
 		}
 
@@ -1085,26 +1079,24 @@ static int __init acpi_parse_madt_ioapic_entries(void)
 	 * if "noapic" boot option, don't look for IO-APICs
 	 */
 	if (skip_ioapic_setup) {
-		printk(KERN_INFO PREFIX "Skipping IOAPIC probe "
-		       "due to 'noapic' option.\n");
+		pi_info("Skipping IOAPIC probe due to 'noapic' option.\n");
 		return -ENODEV;
 	}
 
 	count = acpi_table_parse_madt(ACPI_MADT_TYPE_IO_APIC, acpi_parse_ioapic,
 				      MAX_IO_APICS);
 	if (!count) {
-		printk(KERN_ERR PREFIX "No IOAPIC entries present\n");
+		pi_err("No IOAPIC entries present\n");
 		return -ENODEV;
 	} else if (count < 0) {
-		printk(KERN_ERR PREFIX "Error parsing IOAPIC entry\n");
+		pi_err("Error parsing IOAPIC entry\n");
 		return count;
 	}
 
 	count = acpi_table_parse_madt(ACPI_MADT_TYPE_INTERRUPT_OVERRIDE,
 				      acpi_parse_int_src_ovr, nr_irqs);
 	if (count < 0) {
-		printk(KERN_ERR PREFIX
-		       "Error parsing interrupt source overrides entry\n");
+		pi_err("Error parsing interrupt source overrides entry\n");
 		/* TBD: Cleanup to allow fallback to MPS */
 		return count;
 	}
@@ -1123,7 +1115,7 @@ static int __init acpi_parse_madt_ioapic_entries(void)
 	count = acpi_table_parse_madt(ACPI_MADT_TYPE_NMI_SOURCE,
 				      acpi_parse_nmi_src, nr_irqs);
 	if (count < 0) {
-		printk(KERN_ERR PREFIX "Error parsing NMI SRC entry\n");
+		pi_err("Error parsing NMI SRC entry\n");
 		/* TBD: Cleanup to allow fallback to MPS */
 		return count;
 	}
@@ -1156,8 +1148,7 @@ static void __init early_acpi_process_madt(void)
 			/*
 			 * Dell Precision Workstation 410, 610 come here.
 			 */
-			printk(KERN_ERR PREFIX
-			       "Invalid BIOS MADT, disabling ACPI\n");
+			pi_err("Invalid BIOS MADT, disabling ACPI\n");
 			disable_acpi();
 		}
 	}
@@ -1192,8 +1183,7 @@ static void __init acpi_process_madt(void)
 			/*
 			 * Dell Precision Workstation 410, 610 come here.
 			 */
-			printk(KERN_ERR PREFIX
-			       "Invalid BIOS MADT, disabling ACPI\n");
+			pi_err("Invalid BIOS MADT, disabling ACPI\n");
 			disable_acpi();
 		}
 	} else {
@@ -1203,8 +1193,7 @@ static void __init acpi_process_madt(void)
  		 * Boot with "acpi=off" to use MPS on such a system.
  		 */
 		if (smp_found_config) {
-			printk(KERN_WARNING PREFIX
-				"No APIC-table, disabling MPS\n");
+			pi_warn("No APIC-table, disabling MPS\n");
 			smp_found_config = 0;
 		}
 	}
@@ -1214,11 +1203,11 @@ static void __init acpi_process_madt(void)
 	 * processors, where MPS only supports physical.
 	 */
 	if (acpi_lapic && acpi_ioapic)
-		printk(KERN_INFO "Using ACPI (MADT) for SMP configuration "
-		       "information\n");
+		pi_info("Using ACPI (MADT) for SMP configuration "
+			"information\n");
 	else if (acpi_lapic)
-		printk(KERN_INFO "Using ACPI for processor (LAPIC) "
-		       "configuration information\n");
+		pi_info("Using ACPI for processor (LAPIC) configuration "
+			"information\n");
 #endif
 	return;
 }
@@ -1226,8 +1215,7 @@ static void __init acpi_process_madt(void)
 static int __init disable_acpi_irq(const struct dmi_system_id *d)
 {
 	if (!acpi_force) {
-		printk(KERN_NOTICE "%s detected: force use of acpi=noirq\n",
-		       d->ident);
+		pi_info("%s detected: force use of acpi=noirq\n", d->ident);
 		acpi_noirq_set();
 	}
 	return 0;
@@ -1236,8 +1224,7 @@ static int __init disable_acpi_irq(const struct dmi_system_id *d)
 static int __init disable_acpi_pci(const struct dmi_system_id *d)
 {
 	if (!acpi_force) {
-		printk(KERN_NOTICE "%s detected: force use of pci=noacpi\n",
-		       d->ident);
+		pi_notice("%s detected: force use of pci=noacpi\n", d->ident);
 		acpi_disable_pci();
 	}
 	return 0;
@@ -1246,11 +1233,11 @@ static int __init disable_acpi_pci(const struct dmi_system_id *d)
 static int __init dmi_disable_acpi(const struct dmi_system_id *d)
 {
 	if (!acpi_force) {
-		printk(KERN_NOTICE "%s detected: acpi off\n", d->ident);
+		pi_notice("%s detected: acpi off\n", d->ident);
 		disable_acpi();
 	} else {
-		printk(KERN_NOTICE
-		       "Warning: DMI blacklist says broken, but acpi forced\n");
+		pi_notice("Warning: DMI blacklist says broken, but acpi "
+			  "forced\n");
 	}
 	return 0;
 }
@@ -1261,8 +1248,8 @@ static int __init dmi_disable_acpi(const struct dmi_system_id *d)
 static int __init dmi_ignore_irq0_timer_override(const struct dmi_system_id *d)
 {
 	if (!acpi_skip_timer_override) {
-		pr_notice("%s detected: Ignoring BIOS IRQ0 override\n",
-			d->ident);
+		pi_notice("%s detected: Ignoring BIOS IRQ0 override\n",
+			  d->ident);
 		acpi_skip_timer_override = 1;
 	}
 	return 0;
@@ -1445,9 +1432,9 @@ void __init acpi_boot_table_init(void)
 	 */
 	if (acpi_blacklisted()) {
 		if (acpi_force) {
-			printk(KERN_WARNING PREFIX "acpi=force override\n");
+			pi_warn("acpi=force override\n");
 		} else {
-			printk(KERN_WARNING PREFIX "Disabling ACPI support\n");
+			pi_warn("Disabling ACPI support\n");
 			disable_acpi();
 			return;
 		}
@@ -1507,32 +1494,32 @@ static int __init parse_acpi(char *arg)
 		return -EINVAL;
 
 	/* "acpi=off" disables both ACPI table parsing and interpreter */
-	if (strcmp(arg, "off") == 0) {
+	if (strcmp(arg, _("off")) == 0) {
 		disable_acpi();
 	}
 	/* acpi=force to over-ride black-list */
-	else if (strcmp(arg, "force") == 0) {
+	else if (strcmp(arg, _("force")) == 0) {
 		acpi_force = 1;
 		acpi_disabled = 0;
 	}
 	/* acpi=strict disables out-of-spec workarounds */
-	else if (strcmp(arg, "strict") == 0) {
+	else if (strcmp(arg, _("strict")) == 0) {
 		acpi_strict = 1;
 	}
 	/* acpi=rsdt use RSDT instead of XSDT */
-	else if (strcmp(arg, "rsdt") == 0) {
+	else if (strcmp(arg, _("rsdt")) == 0) {
 		acpi_gbl_do_not_use_xsdt = TRUE;
 	}
 	/* "acpi=noirq" disables ACPI interrupt routing */
-	else if (strcmp(arg, "noirq") == 0) {
+	else if (strcmp(arg, _("noirq")) == 0) {
 		acpi_noirq_set();
 	}
 	/* "acpi=copy_dsdt" copys DSDT */
-	else if (strcmp(arg, "copy_dsdt") == 0) {
+	else if (strcmp(arg, _("copy_dsdt")) == 0) {
 		acpi_gbl_copy_dsdt_locally = 1;
 	}
 	/* "acpi=nocmcff" disables FF mode for corrected errors */
-	else if (strcmp(arg, "nocmcff") == 0) {
+	else if (strcmp(arg, _("nocmcff")) == 0) {
 		acpi_disable_cmcff = 1;
 	} else {
 		/* Core will printk when we return error. */
@@ -1545,7 +1532,7 @@ early_param("acpi", parse_acpi);
 /* FIXME: Using pci= for an ACPI parameter is a travesty. */
 static int __init parse_pci(char *arg)
 {
-	if (arg && strcmp(arg, "noacpi") == 0)
+	if (arg && strcmp(arg, _("noacpi")) == 0)
 		acpi_disable_pci();
 	return 0;
 }
@@ -1556,9 +1543,9 @@ int __init acpi_mps_check(void)
 #if defined(CONFIG_X86_LOCAL_APIC) && !defined(CONFIG_X86_MPPARSE)
 /* mptable code is not built-in*/
 	if (acpi_disabled || acpi_noirq) {
-		printk(KERN_WARNING "MPS support code is not built-in.\n"
-		       "Using acpi=off or acpi=noirq or pci=noacpi "
-		       "may have problem\n");
+		pi_warn("MPS support code is not built-in.\n");
+		pi_cont("Using acpi=off or acpi=noirq or pci=noacpi "
+			"may have problems.\n");
 		return 1;
 	}
 #endif
@@ -1585,16 +1572,16 @@ static int __init setup_acpi_sci(char *s)
 {
 	if (!s)
 		return -EINVAL;
-	if (!strcmp(s, "edge"))
+	if (!strcmp(s, _("edge")))
 		acpi_sci_flags =  ACPI_MADT_TRIGGER_EDGE |
 			(acpi_sci_flags & ~ACPI_MADT_TRIGGER_MASK);
-	else if (!strcmp(s, "level"))
+	else if (!strcmp(s, _("level")))
 		acpi_sci_flags = ACPI_MADT_TRIGGER_LEVEL |
 			(acpi_sci_flags & ~ACPI_MADT_TRIGGER_MASK);
-	else if (!strcmp(s, "high"))
+	else if (!strcmp(s, _("high")))
 		acpi_sci_flags = ACPI_MADT_POLARITY_ACTIVE_HIGH |
 			(acpi_sci_flags & ~ACPI_MADT_POLARITY_MASK);
-	else if (!strcmp(s, "low"))
+	else if (!strcmp(s, _("low")))
 		acpi_sci_flags = ACPI_MADT_POLARITY_ACTIVE_LOW |
 			(acpi_sci_flags & ~ACPI_MADT_POLARITY_MASK);
 	else
diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
index 3136820783..dd75669104 100644
--- a/arch/x86/kernel/acpi/sleep.c
+++ b/arch/x86/kernel/acpi/sleep.c
@@ -114,27 +114,26 @@ int x86_acpi_suspend_lowlevel(void)
 static int __init acpi_sleep_setup(char *str)
 {
 	while ((str != NULL) && (*str != '\0')) {
-		if (strncmp(str, "s3_bios", 7) == 0)
+		if (strncmp(str, __init_str("s3_bios"), 7) == 0)
 			acpi_realmode_flags |= 1;
-		if (strncmp(str, "s3_mode", 7) == 0)
+		if (strncmp(str, __init_str("s3_mode"), 7) == 0)
 			acpi_realmode_flags |= 2;
-		if (strncmp(str, "s3_beep", 7) == 0)
+		if (strncmp(str, __init_str("s3_beep"), 7) == 0)
 			acpi_realmode_flags |= 4;
 #ifdef CONFIG_HIBERNATION
-		if (strncmp(str, "s4_nohwsig", 10) == 0)
+		if (strncmp(str, __init_str("s4_nohwsig"), 10) == 0)
 			acpi_no_s4_hw_signature();
 #endif
-		if (strncmp(str, "nonvs", 5) == 0)
+		if (strncmp(str, __init_str("nonvs"), 5) == 0)
 			acpi_nvs_nosave();
-		if (strncmp(str, "nonvs_s3", 8) == 0)
+		if (strncmp(str, __init_str("nonvs_s3"), 8) == 0)
 			acpi_nvs_nosave_s3();
-		if (strncmp(str, "old_ordering", 12) == 0)
+		if (strncmp(str, __init_str("old_ordering"), 12) == 0)
 			acpi_old_suspend_ordering();
 		str = strchr(str, ',');
 		if (str != NULL)
-			str += strspn(str, ", \t");
+			str += strspn(str, __init_str(", \t"));
 	}
 	return 1;
 }
-
 __setup("acpi_sleep=", acpi_sleep_setup);
-- 
1.7.10.4


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCHv3 6/9] x86, mm: Make x86_init.memory_setup() return a const char *
  2014-08-21 12:23 [PATCHv3 0/9] Mark literal strings in __init / __exit code Mathias Krause
                   ` (4 preceding siblings ...)
  2014-08-21 12:23 ` [PATCHv3 5/9] x86, acpi: Mark __init strings as such Mathias Krause
@ 2014-08-21 12:23 ` Mathias Krause
  2014-08-21 12:23 ` [PATCHv3 7/9] x86, mm: early_panic() - pass on the message as string Mathias Krause
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 18+ messages in thread
From: Mathias Krause @ 2014-08-21 12:23 UTC (permalink / raw)
  To: Andrew Morton, Thomas Gleixner, Ingo Molnar, H. Peter Anvin
  Cc: Joe Perches, Rasmus Villemoes, linux-kernel, Mathias Krause

All implementations of x86_init.memory_setup() actually return a const
char *.  Adjust the prototypes to reflect reality.

Also add the missing __init annotation to xen_auto_xlated_memory_setup().

Signed-off-by: Mathias Krause <minipli@googlemail.com>
---
 arch/x86/include/asm/e820.h     |    4 ++--
 arch/x86/include/asm/x86_init.h |    2 +-
 arch/x86/kernel/e820.c          |    8 ++++----
 arch/x86/lguest/boot.c          |    2 +-
 arch/x86/xen/setup.c            |    4 ++--
 arch/x86/xen/xen-ops.h          |    4 ++--
 6 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/arch/x86/include/asm/e820.h b/arch/x86/include/asm/e820.h
index 779c2efe2e..7d17fb2b16 100644
--- a/arch/x86/include/asm/e820.h
+++ b/arch/x86/include/asm/e820.h
@@ -17,7 +17,7 @@ extern unsigned long pci_mem_start;
 extern int e820_any_mapped(u64 start, u64 end, unsigned type);
 extern int e820_all_mapped(u64 start, u64 end, unsigned type);
 extern void e820_add_region(u64 start, u64 size, int type);
-extern void e820_print_map(char *who);
+extern void e820_print_map(const char *who);
 extern int
 sanitize_e820_map(struct e820entry *biosmap, int max_nr_map, u32 *pnr_map);
 extern u64 e820_update_range(u64 start, u64 size, unsigned old_type,
@@ -59,7 +59,7 @@ extern void finish_e820_parsing(void);
 extern void e820_reserve_resources(void);
 extern void e820_reserve_resources_late(void);
 extern void setup_memory_map(void);
-extern char *default_machine_specific_memory_setup(void);
+extern const char *default_machine_specific_memory_setup(void);
 
 /*
  * Returns true iff the specified range [s,e) is completely contained inside
diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index e45e4da96b..64e80c9815 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -42,7 +42,7 @@ struct x86_init_mpparse {
 struct x86_init_resources {
 	void (*probe_roms)(void);
 	void (*reserve_resources)(void);
-	char *(*memory_setup)(void);
+	const char *(*memory_setup)(void);
 };
 
 /**
diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 988c00a1f6..14323a7c66 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -155,7 +155,7 @@ static void __init e820_print_type(u32 type)
 	}
 }
 
-void __init e820_print_map(char *who)
+void __init e820_print_map(const char *who)
 {
 	int i;
 
@@ -1021,9 +1021,9 @@ void __init e820_reserve_resources_late(void)
 	}
 }
 
-char *__init default_machine_specific_memory_setup(void)
+const char *__init default_machine_specific_memory_setup(void)
 {
-	char *who = "BIOS-e820";
+	const char *who = "BIOS-e820";
 	u32 new_nr;
 	/*
 	 * Try to copy the BIOS-supplied E820-map.
@@ -1061,7 +1061,7 @@ char *__init default_machine_specific_memory_setup(void)
 
 void __init setup_memory_map(void)
 {
-	char *who;
+	const char *who;
 
 	who = x86_init.resources.memory_setup();
 	memcpy(&e820_saved, &e820, sizeof(struct e820map));
diff --git a/arch/x86/lguest/boot.c b/arch/x86/lguest/boot.c
index aae94132bc..28457edc2f 100644
--- a/arch/x86/lguest/boot.c
+++ b/arch/x86/lguest/boot.c
@@ -1167,7 +1167,7 @@ static struct notifier_block paniced = {
 };
 
 /* Setting up memory is fairly easy. */
-static __init char *lguest_memory_setup(void)
+static __init const char *lguest_memory_setup(void)
 {
 	/*
 	 * The Linux bootloader header contains an "e820" memory map: the
diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
index 2e555163c2..fabe61f6f3 100644
--- a/arch/x86/xen/setup.c
+++ b/arch/x86/xen/setup.c
@@ -336,7 +336,7 @@ void xen_ignore_unusable(struct e820entry *list, size_t map_size)
 /**
  * machine_specific_memory_setup - Hook for machine specific memory setup.
  **/
-char * __init xen_memory_setup(void)
+const char * __init xen_memory_setup(void)
 {
 	static struct e820entry map[E820MAX] __initdata;
 
@@ -502,7 +502,7 @@ char * __init xen_memory_setup(void)
 /*
  * Machine specific memory setup for auto-translated guests.
  */
-char * __init xen_auto_xlated_memory_setup(void)
+const char * __init xen_auto_xlated_memory_setup(void)
 {
 	static struct e820entry map[E820MAX] __initdata;
 
diff --git a/arch/x86/xen/xen-ops.h b/arch/x86/xen/xen-ops.h
index 28c7e0be56..da12e3413c 100644
--- a/arch/x86/xen/xen-ops.h
+++ b/arch/x86/xen/xen-ops.h
@@ -35,8 +35,8 @@ void xen_mm_pin_all(void);
 void xen_mm_unpin_all(void);
 void xen_set_pat(u64);
 
-char * __init xen_memory_setup(void);
-char * xen_auto_xlated_memory_setup(void);
+const char * __init xen_memory_setup(void);
+const char * __init xen_auto_xlated_memory_setup(void);
 void __init xen_arch_setup(void);
 void xen_enable_sysenter(void);
 void xen_enable_syscall(void);
-- 
1.7.10.4


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCHv3 7/9] x86, mm: early_panic() - pass on the message as string
  2014-08-21 12:23 [PATCHv3 0/9] Mark literal strings in __init / __exit code Mathias Krause
                   ` (5 preceding siblings ...)
  2014-08-21 12:23 ` [PATCHv3 6/9] x86, mm: Make x86_init.memory_setup() return a const char * Mathias Krause
@ 2014-08-21 12:23 ` Mathias Krause
  2014-08-21 12:23 ` [PATCHv3 8/9] x86, mm: e820 - mark __init strings as such Mathias Krause
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 18+ messages in thread
From: Mathias Krause @ 2014-08-21 12:23 UTC (permalink / raw)
  To: Andrew Morton, Thomas Gleixner, Ingo Molnar, H. Peter Anvin
  Cc: Joe Perches, Rasmus Villemoes, linux-kernel, Mathias Krause

early_printk() and panic() expect a format string as first argument.
Change early_panic() to pass on the message as string instead of a
format string.

Signed-off-by: Mathias Krause <minipli@googlemail.com>
---
 arch/x86/kernel/e820.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 14323a7c66..bfe9238e12 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -801,10 +801,10 @@ unsigned long __init e820_end_of_low_ram_pfn(void)
 	return e820_end_pfn(1UL<<(32 - PAGE_SHIFT), E820_RAM);
 }
 
-static void early_panic(char *msg)
+static void early_panic(const char *msg)
 {
-	early_printk(msg);
-	panic(msg);
+	early_printk("%s", msg);
+	panic("%s", msg);
 }
 
 static int userdef __initdata;
-- 
1.7.10.4


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCHv3 8/9] x86, mm: e820 - mark __init strings as such
  2014-08-21 12:23 [PATCHv3 0/9] Mark literal strings in __init / __exit code Mathias Krause
                   ` (6 preceding siblings ...)
  2014-08-21 12:23 ` [PATCHv3 7/9] x86, mm: early_panic() - pass on the message as string Mathias Krause
@ 2014-08-21 12:23 ` Mathias Krause
  2014-08-21 12:23 ` [PATCHv3 9/9] x86: setup " Mathias Krause
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 18+ messages in thread
From: Mathias Krause @ 2014-08-21 12:23 UTC (permalink / raw)
  To: Andrew Morton, Thomas Gleixner, Ingo Molnar, H. Peter Anvin
  Cc: Joe Perches, Rasmus Villemoes, linux-kernel, Mathias Krause

Make use of the pi_<level>() helpers to mark the strings printed during
initialization for automatic release. Do so for the strings used in
command line parsing as well, by using the __init_str() macro.

The debug messages have been converted to printk_init(KERN_DEBUG ...)
instead of pi_debug(...), though. They might have some value beyond
debugging. Moreover, this ensures we won't loose any message by this
conversion.

This moves ~0.8 kB from the .rodata section to .init.rodata, marking it
for release after initialization.

Signed-off-by: Mathias Krause <minipli@googlemail.com>
---
 arch/x86/kernel/e820.c |   87 ++++++++++++++++++++++++------------------------
 1 file changed, 44 insertions(+), 43 deletions(-)

diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index bfe9238e12..e347a272a9 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -25,6 +25,8 @@
 #include <asm/proto.h>
 #include <asm/setup.h>
 
+#define _(x)	__init_str(x)
+
 /*
  * The e820 map is the map that gets modified e.g. with command line parameters
  * and that is also registered with modifications in the kernel resource tree
@@ -113,7 +115,7 @@ static void __init __e820_add_region(struct e820map *e820x, u64 start, u64 size,
 	int x = e820x->nr_map;
 
 	if (x >= ARRAY_SIZE(e820x->map)) {
-		printk(KERN_ERR "e820: too many entries; ignoring [mem %#010llx-%#010llx]\n",
+		pi_err("e820: too many entries; ignoring [mem %#010llx-%#010llx]\n",
 		       (unsigned long long) start,
 		       (unsigned long long) (start + size - 1));
 		return;
@@ -135,22 +137,22 @@ static void __init e820_print_type(u32 type)
 	switch (type) {
 	case E820_RAM:
 	case E820_RESERVED_KERN:
-		printk(KERN_CONT "usable");
+		pi_cont("usable");
 		break;
 	case E820_RESERVED:
-		printk(KERN_CONT "reserved");
+		pi_cont("reserved");
 		break;
 	case E820_ACPI:
-		printk(KERN_CONT "ACPI data");
+		pi_cont("ACPI data");
 		break;
 	case E820_NVS:
-		printk(KERN_CONT "ACPI NVS");
+		pi_cont("ACPI NVS");
 		break;
 	case E820_UNUSABLE:
-		printk(KERN_CONT "unusable");
+		pi_cont("unusable");
 		break;
 	default:
-		printk(KERN_CONT "type %u", type);
+		pi_cont("type %u", type);
 		break;
 	}
 }
@@ -160,12 +162,12 @@ void __init e820_print_map(const char *who)
 	int i;
 
 	for (i = 0; i < e820.nr_map; i++) {
-		printk(KERN_INFO "%s: [mem %#018Lx-%#018Lx] ", who,
-		       (unsigned long long) e820.map[i].addr,
-		       (unsigned long long)
-		       (e820.map[i].addr + e820.map[i].size - 1));
+		pi_info("%s: [mem %#018Lx-%#018Lx] ", who,
+			(unsigned long long) e820.map[i].addr,
+			(unsigned long long)
+			(e820.map[i].addr + e820.map[i].size - 1));
 		e820_print_type(e820.map[i].type);
-		printk(KERN_CONT "\n");
+		pi_cont("\n");
 	}
 }
 
@@ -430,12 +432,12 @@ static u64 __init __e820_update_range(struct e820map *e820x, u64 start,
 		size = ULLONG_MAX - start;
 
 	end = start + size;
-	printk(KERN_DEBUG "e820: update [mem %#010Lx-%#010Lx] ",
-	       (unsigned long long) start, (unsigned long long) (end - 1));
+	printk_init(KERN_DEBUG "e820: update [mem %#010Lx-%#010Lx] ",
+		    (unsigned long long) start, (unsigned long long) (end - 1));
 	e820_print_type(old_type);
-	printk(KERN_CONT " ==> ");
+	pi_cont(" ==> ");
 	e820_print_type(new_type);
-	printk(KERN_CONT "\n");
+	pi_cont("\n");
 
 	for (i = 0; i < e820x->nr_map; i++) {
 		struct e820entry *ei = &e820x->map[i];
@@ -510,11 +512,11 @@ u64 __init e820_remove_range(u64 start, u64 size, unsigned old_type,
 		size = ULLONG_MAX - start;
 
 	end = start + size;
-	printk(KERN_DEBUG "e820: remove [mem %#010Lx-%#010Lx] ",
-	       (unsigned long long) start, (unsigned long long) (end - 1));
+	printk_init(KERN_DEBUG "e820: remove [mem %#010Lx-%#010Lx] ",
+		    (unsigned long long) start, (unsigned long long) (end - 1));
 	if (checktype)
 		e820_print_type(old_type);
-	printk(KERN_CONT "\n");
+	pi_cont("\n");
 
 	for (i = 0; i < e820.nr_map; i++) {
 		struct e820entry *ei = &e820.map[i];
@@ -567,8 +569,8 @@ void __init update_e820(void)
 	if (sanitize_e820_map(e820.map, ARRAY_SIZE(e820.map), &nr_map))
 		return;
 	e820.nr_map = nr_map;
-	printk(KERN_INFO "e820: modified physical RAM map:\n");
-	e820_print_map("modified");
+	pi_info("e820: modified physical RAM map:\n");
+	e820_print_map(_("modified"));
 }
 static void __init update_e820_saved(void)
 {
@@ -636,7 +638,7 @@ __init void e820_setup_gap(void)
 #ifdef CONFIG_X86_64
 	if (!found) {
 		gapstart = (max_pfn << PAGE_SHIFT) + 1024*1024;
-		printk(KERN_ERR
+		pi_err(
 	"e820: cannot find a gap in the 32bit address range\n"
 	"e820: PCI devices with unassigned 32bit BARs may break!\n");
 	}
@@ -647,9 +649,8 @@ __init void e820_setup_gap(void)
 	 */
 	pci_mem_start = gapstart;
 
-	printk(KERN_INFO
-	       "e820: [mem %#010lx-%#010lx] available for PCI devices\n",
-	       gapstart, gapstart + gapsize - 1);
+	pi_info("e820: [mem %#010lx-%#010lx] available for PCI devices\n",
+		gapstart, gapstart + gapsize - 1);
 }
 
 /**
@@ -670,8 +671,8 @@ void __init parse_e820_ext(u64 phys_addr, u32 data_len)
 	__append_e820_map(extmap, entries);
 	sanitize_e820_map(e820.map, ARRAY_SIZE(e820.map), &e820.nr_map);
 	early_iounmap(sdata, data_len);
-	printk(KERN_INFO "e820: extended physical RAM map:\n");
-	e820_print_map("extended");
+	pi_info("e820: extended physical RAM map:\n");
+	e820_print_map(_("extended"));
 }
 
 #if defined(CONFIG_X86_64) || \
@@ -737,7 +738,7 @@ u64 __init early_reserve_e820(u64 size, u64 align)
 	addr = __memblock_alloc_base(size, align, MEMBLOCK_ALLOC_ACCESSIBLE);
 	if (addr) {
 		e820_update_range_saved(addr, size, E820_RAM, E820_RESERVED);
-		printk(KERN_INFO "e820: update e820_saved for early_reserve_e820\n");
+		pi_info("e820: update e820_saved for early_reserve_e820\n");
 		update_e820_saved();
 	}
 
@@ -787,8 +788,8 @@ static unsigned long __init e820_end_pfn(unsigned long limit_pfn, unsigned type)
 	if (last_pfn > max_arch_pfn)
 		last_pfn = max_arch_pfn;
 
-	printk(KERN_INFO "e820: last_pfn = %#lx max_arch_pfn = %#lx\n",
-			 last_pfn, max_arch_pfn);
+	pi_info("e820: last_pfn = %#lx max_arch_pfn = %#lx\n", last_pfn,
+		max_arch_pfn);
 	return last_pfn;
 }
 unsigned long __init e820_end_of_ram_pfn(void)
@@ -803,8 +804,8 @@ unsigned long __init e820_end_of_low_ram_pfn(void)
 
 static void early_panic(const char *msg)
 {
-	early_printk("%s", msg);
-	panic("%s", msg);
+	early_printk(_("%s"), msg);
+	panic(_("%s"), msg);
 }
 
 static int userdef __initdata;
@@ -817,12 +818,12 @@ static int __init parse_memopt(char *p)
 	if (!p)
 		return -EINVAL;
 
-	if (!strcmp(p, "nopentium")) {
+	if (!strcmp(p, _("nopentium"))) {
 #ifdef CONFIG_X86_32
 		setup_clear_cpu_cap(X86_FEATURE_PSE);
 		return 0;
 #else
-		printk(KERN_WARNING "mem=nopentium ignored! (only supported on x86_32)\n");
+		pi_warn("mem=nopentium ignored! (only supported on x86_32)\n");
 		return -EINVAL;
 #endif
 	}
@@ -846,7 +847,7 @@ static int __init parse_memmap_one(char *p)
 	if (!p)
 		return -EINVAL;
 
-	if (!strncmp(p, "exactmap", 8)) {
+	if (!strncmp(p, _("exactmap"), 8)) {
 #ifdef CONFIG_CRASH_DUMP
 		/*
 		 * If we are doing a crash dump, we still need to know
@@ -902,11 +903,11 @@ void __init finish_e820_parsing(void)
 		u32 nr = e820.nr_map;
 
 		if (sanitize_e820_map(e820.map, ARRAY_SIZE(e820.map), &nr) < 0)
-			early_panic("Invalid user supplied memory map");
+			early_panic(_("Invalid user supplied memory map"));
 		e820.nr_map = nr;
 
-		printk(KERN_INFO "e820: user-defined physical RAM map:\n");
-		e820_print_map("user");
+		pi_info("e820: user-defined physical RAM map:\n");
+		e820_print_map(_("user"));
 	}
 }
 
@@ -1013,7 +1014,7 @@ void __init e820_reserve_resources_late(void)
 			end = MAX_RESOURCE_SIZE;
 		if (start >= end)
 			continue;
-		printk(KERN_DEBUG
+		printk_init(KERN_DEBUG
 		       "e820: reserve RAM buffer [mem %#010llx-%#010llx]\n",
 		       start, end);
 		reserve_region_with_split(&iomem_resource, start, end,
@@ -1023,7 +1024,7 @@ void __init e820_reserve_resources_late(void)
 
 const char *__init default_machine_specific_memory_setup(void)
 {
-	const char *who = "BIOS-e820";
+	const char *who = _("BIOS-e820");
 	u32 new_nr;
 	/*
 	 * Try to copy the BIOS-supplied E820-map.
@@ -1044,10 +1045,10 @@ const char *__init default_machine_specific_memory_setup(void)
 		if (boot_params.alt_mem_k
 		    < boot_params.screen_info.ext_mem_k) {
 			mem_size = boot_params.screen_info.ext_mem_k;
-			who = "BIOS-88";
+			who = _("BIOS-88");
 		} else {
 			mem_size = boot_params.alt_mem_k;
-			who = "BIOS-e801";
+			who = _("BIOS-e801");
 		}
 
 		e820.nr_map = 0;
@@ -1065,7 +1066,7 @@ void __init setup_memory_map(void)
 
 	who = x86_init.resources.memory_setup();
 	memcpy(&e820_saved, &e820, sizeof(struct e820map));
-	printk(KERN_INFO "e820: BIOS-provided physical RAM map:\n");
+	pi_info("e820: BIOS-provided physical RAM map:\n");
 	e820_print_map(who);
 }
 
-- 
1.7.10.4


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCHv3 9/9] x86: setup - mark __init strings as such
  2014-08-21 12:23 [PATCHv3 0/9] Mark literal strings in __init / __exit code Mathias Krause
                   ` (7 preceding siblings ...)
  2014-08-21 12:23 ` [PATCHv3 8/9] x86, mm: e820 - mark __init strings as such Mathias Krause
@ 2014-08-21 12:23 ` Mathias Krause
  2014-08-21 12:57 ` [PATCHv3 0/9] Mark literal strings in __init / __exit code Ingo Molnar
  2014-08-21 16:25 ` Sam Ravnborg
  10 siblings, 0 replies; 18+ messages in thread
From: Mathias Krause @ 2014-08-21 12:23 UTC (permalink / raw)
  To: Andrew Morton, Thomas Gleixner, Ingo Molnar, H. Peter Anvin
  Cc: Joe Perches, Rasmus Villemoes, linux-kernel, Mathias Krause

Make use of the pi_<level>() helpers to mark the strings printed during
initialization for automatic release. Do so for the strings used in boot
parameters parsing as well, by using the __init_str() macro.

The debug messages have been converted to printk_init(KERN_DEBUG ...)
instead of pi_debug(...), though. They might have some value beyond
debugging. Moreover, this ensures we won't loose any message by this
conversion.

This moves ~0.5 kB from the .rodata section to .init.rodata, marking it
for release after initialization.

Signed-off-by: Mathias Krause <minipli@googlemail.com>
---
 arch/x86/kernel/setup.c |   70 +++++++++++++++++++++++------------------------
 1 file changed, 34 insertions(+), 36 deletions(-)

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 41ead8d3bc..82f211a310 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -111,6 +111,8 @@
 #include <asm/alternative.h>
 #include <asm/prom.h>
 
+#define _(x)	__init_str(x)
+
 /*
  * max_low_pfn_mapped: highest direct mapped pfn under 4GB
  * max_pfn_mapped:     highest direct mapped pfn over 4GB
@@ -331,7 +333,7 @@ static void __init relocate_initrd(void)
 						   area_size, PAGE_SIZE);
 
 	if (!relocated_ramdisk)
-		panic("Cannot find place for new RAMDISK of size %lld\n",
+		panic(_("Cannot find place for new RAMDISK of size %lld\n"),
 		      ramdisk_size);
 
 	/* Note: this includes all the mem currently occupied by
@@ -339,8 +341,8 @@ static void __init relocate_initrd(void)
 	memblock_reserve(relocated_ramdisk, area_size);
 	initrd_start = relocated_ramdisk + PAGE_OFFSET;
 	initrd_end   = initrd_start + ramdisk_size;
-	printk(KERN_INFO "Allocated new RAMDISK: [mem %#010llx-%#010llx]\n",
-	       relocated_ramdisk, relocated_ramdisk + ramdisk_size - 1);
+	pi_info("Allocated new RAMDISK: [mem %#010llx-%#010llx]\n",
+		relocated_ramdisk, relocated_ramdisk + ramdisk_size - 1);
 
 	q = (char *)initrd_start;
 
@@ -361,8 +363,7 @@ static void __init relocate_initrd(void)
 
 	ramdisk_image = get_ramdisk_image();
 	ramdisk_size  = get_ramdisk_size();
-	printk(KERN_INFO "Move RAMDISK from [mem %#010llx-%#010llx] to"
-		" [mem %#010llx-%#010llx]\n",
+	pi_info("Move RAMDISK from [mem %#010llx-%#010llx] to [mem %#010llx-%#010llx]\n",
 		ramdisk_image, ramdisk_image + ramdisk_size - 1,
 		relocated_ramdisk, relocated_ramdisk + ramdisk_size - 1);
 }
@@ -396,12 +397,12 @@ static void __init reserve_initrd(void)
 
 	mapped_size = memblock_mem_size(max_pfn_mapped);
 	if (ramdisk_size >= (mapped_size>>1))
-		panic("initrd too large to handle, "
-		       "disabling initrd (%lld needed, %lld available)\n",
-		       ramdisk_size, mapped_size>>1);
+		panic(_("initrd too large to handle, "
+			"disabling initrd (%lld needed, %lld available)\n"),
+		      ramdisk_size, mapped_size>>1);
 
-	printk(KERN_INFO "RAMDISK: [mem %#010llx-%#010llx]\n", ramdisk_image,
-			ramdisk_end - 1);
+	pi_info("RAMDISK: [mem %#010llx-%#010llx]\n", ramdisk_image,
+		ramdisk_end - 1);
 
 	if (pfn_range_is_mapped(PFN_DOWN(ramdisk_image),
 				PFN_DOWN(ramdisk_end))) {
@@ -478,8 +479,8 @@ static void __init e820_reserve_setup_data(void)
 
 	sanitize_e820_map(e820.map, ARRAY_SIZE(e820.map), &e820.nr_map);
 	memcpy(&e820_saved, &e820, sizeof(struct e820map));
-	printk(KERN_INFO "extended physical RAM map:\n");
-	e820_print_map("reserve setup_data");
+	pi_info("extended physical RAM map:\n");
+	e820_print_map(_("reserve setup_data"));
 }
 
 static void __init memblock_x86_reserve_range_setup_data(void)
@@ -550,16 +551,16 @@ static void __init reserve_crashkernel_low(void)
 
 	if (!low_base) {
 		if (!auto_set)
-			pr_info("crashkernel low reservation failed - No suitable area found.\n");
+			pi_info("crashkernel low reservation failed - No suitable area found.\n");
 
 		return;
 	}
 
 	memblock_reserve(low_base, low_size);
-	pr_info("Reserving %ldMB of low memory at %ldMB for crashkernel (System low RAM: %ldMB)\n",
-			(unsigned long)(low_size >> 20),
-			(unsigned long)(low_base >> 20),
-			(unsigned long)(total_low_mem >> 20));
+	pi_info("Reserving %ldMB of low memory at %ldMB for crashkernel (System low RAM: %ldMB)\n",
+		(unsigned long)(low_size >> 20),
+		(unsigned long)(low_base >> 20),
+		(unsigned long)(total_low_mem >> 20));
 	crashk_low_res.start = low_base;
 	crashk_low_res.end   = low_base + low_size - 1;
 	insert_resource(&iomem_resource, &crashk_low_res);
@@ -599,7 +600,7 @@ static void __init reserve_crashkernel(void)
 					crash_size, alignment);
 
 		if (!crash_base) {
-			pr_info("crashkernel reservation failed - No suitable area found.\n");
+			pi_info("crashkernel reservation failed - No suitable area found.\n");
 			return;
 		}
 
@@ -609,17 +610,16 @@ static void __init reserve_crashkernel(void)
 		start = memblock_find_in_range(crash_base,
 				 crash_base + crash_size, crash_size, 1<<20);
 		if (start != crash_base) {
-			pr_info("crashkernel reservation failed - memory is in use.\n");
+			pi_info("crashkernel reservation failed - memory is in use.\n");
 			return;
 		}
 	}
 	memblock_reserve(crash_base, crash_size);
 
-	printk(KERN_INFO "Reserving %ldMB of memory at %ldMB "
-			"for crashkernel (System RAM: %ldMB)\n",
-			(unsigned long)(crash_size >> 20),
-			(unsigned long)(crash_base >> 20),
-			(unsigned long)(total_mem >> 20));
+	pi_info("Reserving %ldMB of memory at %ldMB for crashkernel (System RAM: %ldMB)\n",
+		(unsigned long)(crash_size >> 20),
+		(unsigned long)(crash_base >> 20),
+		(unsigned long)(total_mem >> 20));
 
 	crashk_res.start = crash_base;
 	crashk_res.end   = crash_base + crash_size - 1;
@@ -727,7 +727,7 @@ static void __init trim_snb_memory(void)
 	if (!snb_gfx_workaround_needed())
 		return;
 
-	printk(KERN_DEBUG "reserving inaccessible SNB gfx pages\n");
+	printk_init(KERN_DEBUG "reserving inaccessible SNB gfx pages\n");
 
 	/*
 	 * Reserve all memory below the 1 MB mark that has not
@@ -737,8 +737,7 @@ static void __init trim_snb_memory(void)
 	
 	for (i = 0; i < ARRAY_SIZE(bad_pages); i++) {
 		if (memblock_reserve(bad_pages[i], PAGE_SIZE))
-			printk(KERN_WARNING "failed to reserve 0x%08lx\n",
-			       bad_pages[i]);
+			pi_warn("failed to reserve 0x%08lx\n", bad_pages[i]);
 	}
 }
 
@@ -793,7 +792,7 @@ static void __init e820_add_kernel_range(void)
 	if (e820_all_mapped(start, start + size, E820_RAM))
 		return;
 
-	pr_warn(".text .data .bss are not marked as E820_RAM!\n");
+	pi_warn(".text .data .bss are not marked as E820_RAM!\n");
 	e820_remove_range(start, size, E820_RAM, 0);
 	e820_add_region(start, size, E820_RAM);
 }
@@ -819,7 +818,6 @@ static int __init parse_reservelow(char *p)
 
 	return 0;
 }
-
 early_param("reservelow", parse_reservelow);
 
 static void __init trim_low_memory_range(void)
@@ -881,7 +879,7 @@ void __init setup_arch(char **cmdline_p)
 	load_cr3(swapper_pg_dir);
 	__flush_tlb_all();
 #else
-	printk(KERN_INFO "Command line: %s\n", boot_command_line);
+	pi_info("Command line: %s\n", boot_command_line);
 #endif
 
 	/*
@@ -924,10 +922,10 @@ void __init setup_arch(char **cmdline_p)
 #endif
 #ifdef CONFIG_EFI
 	if (!strncmp((char *)&boot_params.efi_info.efi_loader_signature,
-		     EFI32_LOADER_SIGNATURE, 4)) {
+		     _(EFI32_LOADER_SIGNATURE), 4)) {
 		set_bit(EFI_BOOT, &efi.flags);
 	} else if (!strncmp((char *)&boot_params.efi_info.efi_loader_signature,
-		     EFI64_LOADER_SIGNATURE, 4)) {
+			    _(EFI64_LOADER_SIGNATURE), 4)) {
 		set_bit(EFI_BOOT, &efi.flags);
 		set_bit(EFI_64BIT, &efi.flags);
 	}
@@ -1033,8 +1031,8 @@ void __init setup_arch(char **cmdline_p)
 		e820_update_range(0x70000000ULL, 0x40000ULL, E820_RAM,
 				  E820_RESERVED);
 		sanitize_e820_map(e820.map, ARRAY_SIZE(e820.map), &e820.nr_map);
-		printk(KERN_INFO "fixed physical RAM map:\n");
-		e820_print_map("bad_ppro");
+		pi_info("fixed physical RAM map:\n");
+		e820_print_map(_("bad_ppro"));
 	}
 #else
 	early_gart_iommu_check();
@@ -1103,8 +1101,8 @@ void __init setup_arch(char **cmdline_p)
 #endif
 
 #ifdef CONFIG_X86_32
-	printk(KERN_DEBUG "initial memory mapped: [mem 0x00000000-%#010lx]\n",
-			(max_pfn_mapped<<PAGE_SHIFT) - 1);
+	printk_init(KERN_DEBUG "initial memory mapped: [mem 0x00000000-%#010lx]\n",
+		    (max_pfn_mapped<<PAGE_SHIFT) - 1);
 #endif
 
 	reserve_real_mode();
-- 
1.7.10.4


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCHv3 0/9] Mark literal strings in __init / __exit code
  2014-08-21 12:23 [PATCHv3 0/9] Mark literal strings in __init / __exit code Mathias Krause
                   ` (8 preceding siblings ...)
  2014-08-21 12:23 ` [PATCHv3 9/9] x86: setup " Mathias Krause
@ 2014-08-21 12:57 ` Ingo Molnar
  2014-08-21 14:29   ` Mathias Krause
  2014-08-21 16:25 ` Sam Ravnborg
  10 siblings, 1 reply; 18+ messages in thread
From: Ingo Molnar @ 2014-08-21 12:57 UTC (permalink / raw)
  To: Mathias Krause
  Cc: Andrew Morton, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	Joe Perches, Rasmus Villemoes, linux-kernel, Linus Torvalds


* Mathias Krause <minipli@googlemail.com> wrote:

> This is v3 of the patch series initially posted here:
> 
>   https://lkml.org/lkml/2014/6/22/149
> 
> This series tries to address the problem of dangling strings of __init
> functions after initialization, as well as __exit strings for code not
> even included in the final kernel image. The code might get freed, but
> the format strings are not, as they're in the wrong section.
> 
> One solution to the problem might be to declare variables in the code
> and mark those variables as __initconst. That, though, makes the code
> ugly, as can be seen, e.g., in drivers/hwmon/w83627ehf.c -- a pile of
> 'static const char[] __initconst' lines just for the pr_info() call.
> 
> To be able to mark strings easily patch 1 adds macros to init.h to do so
> without the need to explicitly define variables in the code. (Internally
> it'll declare ones nonetheless, as this seem to be the only way to
> attach an __attribute__() to a verbatim string.) That's already enough to
> solve the problem -- mark the corresponding stings by using these
> macros. But patch 2 adds some syntactical sugar for the most popular use
> case, by providing pr_<level> alike macros, namely pi_<level> for __init
> code and pe_<level> for __exit code. This hides the use of the marker
> macros behind the commonly known printing functions -- with just a
> single character changed. For code that cannot be changed to use the
> pi_<level>() / pe_<level>() helpers printk_init() and printk_exit()
> macros are provided, too.
> 
> The (hidden) variables used in the macros of patch 1 will pollute the
> kernel's symbol table with unneeded entries. It'll be a problem in the
> KALLSYMS_ALL case only, however, patch 3 takes care of filtering the
> pseudo symbols. They have no value for us beside being the only way to
> attach an __attribute__ to a string literal.
> 
> If users of the new macros get it wrong, e.g. use printk_init() /
> pi_<level>() in non-init code or vice versa printk_exit() / pe_<level>()
> in non-exit code, modpost will detect the error as a section mismatch
> and report it accordingly. Still, the message printed by modpost with
> CONFIG_DEBUG_SECTION_MISMATCH=y is rather confusing as the __initconst /
> __exitdata annotation is hidden behind the macros. That's what patch 4
> takes care of -- detecting such cases and providing better modpost
> messages, guiding the user on how to fix the error.
> 
> The remaining patches (5 to 9) exemplarily change strings and format
> strings in a selection of files under arch/x86/ to use the new macros.
> They also address a few styling issues, e.g., patches 4 and 5 are
> cleanup patches I stumbled across while changing the corresponding code
> to make use of the new pi_*() helpers. The changes to arch/x86/ already
> lead to moving ~3kb of memory from .rodata to .init.rodata. This should
> free up a page after init on almost any x86 system.
> 
> To show that there's actual more value to it: A hacked up script, dully
> changing pr_<level> to pi_<level> for __init functions under arch/x86/
> is able to move ~8kB of r/o data into the .init section (partly already
> covered by the patches of this series). The script, though, is dump. It
> does not handle any of the printk() calls, nor does it handle panic()
> calls or other strings used only in initialization code. So there's more
> to squeeze out.

It feels like a burdensome hack that kernel developers are 
forced to use different printing facilities, depending on the 
life cycle of a method. We want to simplify init annotations, 
we don't want to complicate them!

Why isn't this problem solved transparently at build time, via 
tooling, instead of burdening developers? If a particular 
string is only used by an init (or exit) function, it can be 
moved to an init section without forcing the developer to 
declare it as such.

And if tooling isn't ready for this, then wouldn't the right 
solution be to improve tooling - instead of complicating the 
kernel?

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCHv3 0/9] Mark literal strings in __init / __exit code
  2014-08-21 12:57 ` [PATCHv3 0/9] Mark literal strings in __init / __exit code Ingo Molnar
@ 2014-08-21 14:29   ` Mathias Krause
  2014-08-22  8:24     ` Ingo Molnar
  0 siblings, 1 reply; 18+ messages in thread
From: Mathias Krause @ 2014-08-21 14:29 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Andrew Morton, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	Joe Perches, Rasmus Villemoes, linux-kernel, Linus Torvalds

On 21 August 2014 14:57, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Mathias Krause <minipli@googlemail.com> wrote:
>
>> This is v3 of the patch series initially posted here:
>>
>>   https://lkml.org/lkml/2014/6/22/149
>>
>> This series tries to address the problem of dangling strings of __init
>> functions after initialization, as well as __exit strings for code not
>> even included in the final kernel image. The code might get freed, but
>> the format strings are not, as they're in the wrong section.
>>
>> One solution to the problem might be to declare variables in the code
>> and mark those variables as __initconst. That, though, makes the code
>> ugly, as can be seen, e.g., in drivers/hwmon/w83627ehf.c -- a pile of
>> 'static const char[] __initconst' lines just for the pr_info() call.
>>
>> To be able to mark strings easily patch 1 adds macros to init.h to do so
>> without the need to explicitly define variables in the code. (Internally
>> it'll declare ones nonetheless, as this seem to be the only way to
>> attach an __attribute__() to a verbatim string.) That's already enough to
>> solve the problem -- mark the corresponding stings by using these
>> macros. But patch 2 adds some syntactical sugar for the most popular use
>> case, by providing pr_<level> alike macros, namely pi_<level> for __init
>> code and pe_<level> for __exit code. This hides the use of the marker
>> macros behind the commonly known printing functions -- with just a
>> single character changed. For code that cannot be changed to use the
>> pi_<level>() / pe_<level>() helpers printk_init() and printk_exit()
>> macros are provided, too.
>>
>> The (hidden) variables used in the macros of patch 1 will pollute the
>> kernel's symbol table with unneeded entries. It'll be a problem in the
>> KALLSYMS_ALL case only, however, patch 3 takes care of filtering the
>> pseudo symbols. They have no value for us beside being the only way to
>> attach an __attribute__ to a string literal.
>>
>> If users of the new macros get it wrong, e.g. use printk_init() /
>> pi_<level>() in non-init code or vice versa printk_exit() / pe_<level>()
>> in non-exit code, modpost will detect the error as a section mismatch
>> and report it accordingly. Still, the message printed by modpost with
>> CONFIG_DEBUG_SECTION_MISMATCH=y is rather confusing as the __initconst /
>> __exitdata annotation is hidden behind the macros. That's what patch 4
>> takes care of -- detecting such cases and providing better modpost
>> messages, guiding the user on how to fix the error.
>>
>> The remaining patches (5 to 9) exemplarily change strings and format
>> strings in a selection of files under arch/x86/ to use the new macros.
>> They also address a few styling issues, e.g., patches 4 and 5 are
>> cleanup patches I stumbled across while changing the corresponding code
>> to make use of the new pi_*() helpers. The changes to arch/x86/ already
>> lead to moving ~3kb of memory from .rodata to .init.rodata. This should
>> free up a page after init on almost any x86 system.
>>
>> To show that there's actual more value to it: A hacked up script, dully
>> changing pr_<level> to pi_<level> for __init functions under arch/x86/
>> is able to move ~8kB of r/o data into the .init section (partly already
>> covered by the patches of this series). The script, though, is dump. It
>> does not handle any of the printk() calls, nor does it handle panic()
>> calls or other strings used only in initialization code. So there's more
>> to squeeze out.
>
> It feels like a burdensome hack that kernel developers are
> forced to use different printing facilities, depending on the
> life cycle of a method. We want to simplify init annotations,
> we don't want to complicate them!

Does this:

    pi_info("mkiss: AX.25 Multikiss, Hans Albas PE1AYX\n");

really look more complicated compared to this?:

    static const char banner[] __initconst = KERN_INFO \
        "mkiss: AX.25 Multikiss, Hans Albas PE1AYX\n";
    printk(banner);

The latter can be found in drivers/net/hamradio/mkiss.c. Aged code,
admitted. But we have a few more of those and the pi_info() looks way
more appealing to me by doing the very same thing ;)

>
> Why isn't this problem solved transparently at build time, via
> tooling, instead of burdening developers? If a particular
> string is only used by an init (or exit) function, it can be
> moved to an init section without forcing the developer to
> declare it as such.

That's the point. It's not easy to decide "[i]f a particular string is
only used by an init (or exit) function" at the toolchain level. I
agree that it would be best if gcc / ld could detect the life span of
objects automatically and put them into the appropriate sections. But
this is harder to do then it might seems to on the first sight. Not
all string literals used in __init / __exit code can be put into the
corresponding .init / .exit section because some of those strings will
be used later on -- the name passed to class_create(), for example. To
make the toolchain detect which cases are safe to move and which are
not would require it to have a complete view of the resulting kernel
image -- be an LTO stage, so to speak. Otherwise it would either be
impossible -- how would gcc / ld know what a function does with the
string it gets passed to? -- or only work in very special cases, e.g.
where the string is only used within the scope of the compilation unit
-- for strcpy() and such, functions gcc is aware of what they're
doing.

>
> And if tooling isn't ready for this, then wouldn't the right
> solution be to improve tooling - instead of complicating the
> kernel?

Plugin based approaches for this have been mentioned before but those
suffer from the same "global view" problem. So, IMHO it's not that
easy to solve it at the toolchain level. Not to speak, that the kernel
has no infrastructure support for gcc plugins so far. A source level
change seems like a good trade-off. It not only is able to change
existing printk(KERN_* ..) constructs to there pi_*() / pe_*()
counterparts but also marks the memory for release, thereby allows
freeing memory otherwise just lingering around.


Thanks,
Mathias

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCHv3 0/9] Mark literal strings in __init / __exit code
  2014-08-21 12:23 [PATCHv3 0/9] Mark literal strings in __init / __exit code Mathias Krause
                   ` (9 preceding siblings ...)
  2014-08-21 12:57 ` [PATCHv3 0/9] Mark literal strings in __init / __exit code Ingo Molnar
@ 2014-08-21 16:25 ` Sam Ravnborg
  2014-08-24 16:04   ` Mathias Krause
  10 siblings, 1 reply; 18+ messages in thread
From: Sam Ravnborg @ 2014-08-21 16:25 UTC (permalink / raw)
  To: Mathias Krause
  Cc: Andrew Morton, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	Joe Perches, Rasmus Villemoes, linux-kernel

On Thu, Aug 21, 2014 at 02:23:03PM +0200, Mathias Krause wrote:
> This is v3 of the patch series initially posted here:
> 
>   https://lkml.org/lkml/2014/6/22/149
> 
> This series tries to address the problem of dangling strings of __init
> functions after initialization, as well as __exit strings for code not
> even included in the final kernel image. The code might get freed, but
> the format strings are not, as they're in the wrong section.

What potential are we looking at here?
Anything less than a page in size would likely not matter as this
is the granularity we look into.
And the code needs to be built-in - which most drivers are not.

So is it really worth it?

The places that already moves the string to __init/__exit should maybe
be droopped to avoid the complication.

	Sam

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCHv3 0/9] Mark literal strings in __init / __exit code
  2014-08-21 14:29   ` Mathias Krause
@ 2014-08-22  8:24     ` Ingo Molnar
  2014-08-30 15:28       ` Mathias Krause
  0 siblings, 1 reply; 18+ messages in thread
From: Ingo Molnar @ 2014-08-22  8:24 UTC (permalink / raw)
  To: Mathias Krause
  Cc: Andrew Morton, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	Joe Perches, Rasmus Villemoes, linux-kernel, Linus Torvalds


* Mathias Krause <minipli@googlemail.com> wrote:

> > It feels like a burdensome hack that kernel developers are 
> > forced to use different printing facilities, depending on 
> > the life cycle of a method. We want to simplify init 
> > annotations, we don't want to complicate them!
> 
> Does this:
> 
>     pi_info("mkiss: AX.25 Multikiss, Hans Albas PE1AYX\n");
> 
> really look more complicated compared to this?:
> 
>     static const char banner[] __initconst = KERN_INFO \
>         "mkiss: AX.25 Multikiss, Hans Albas PE1AYX\n";
>     printk(banner);
> 
> The latter can be found in drivers/net/hamradio/mkiss.c. Aged 
> code, admitted. But we have a few more of those and the 
> pi_info() looks way more appealing to me by doing the very 
> same thing ;)

My point is that both are complicated, compared to using the 
regular printk() variants.

In general printk()s should not be aware of what function 
context they are in.

> > And if tooling isn't ready for this, then wouldn't the 
> > right solution be to improve tooling - instead of 
> > complicating the kernel?
> 
> Plugin based approaches for this have been mentioned before 
> but those suffer from the same "global view" problem. So, 
> IMHO it's not that easy to solve it at the toolchain level. 

It might be solved via a regular tooling feature, or via 
tooling extension based on plugins.

Regardless of how it's implemented on the tooling side, my 
point remains: this kind of optimization is done on the tooling 
side in a natural fashion, while it's an ongoing maintenance 
concern on the kernel side.

So it should be done on the tooling side, especially as the 
benefits appear to be marginal.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCHv3 0/9] Mark literal strings in __init / __exit code
  2014-08-21 16:25 ` Sam Ravnborg
@ 2014-08-24 16:04   ` Mathias Krause
  2014-08-24 16:28     ` Joe Perches
  0 siblings, 1 reply; 18+ messages in thread
From: Mathias Krause @ 2014-08-24 16:04 UTC (permalink / raw)
  To: Sam Ravnborg
  Cc: Andrew Morton, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	Joe Perches, Rasmus Villemoes, linux-kernel

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

On 21 August 2014 18:25, Sam Ravnborg <sam@ravnborg.org> wrote:
> On Thu, Aug 21, 2014 at 02:23:03PM +0200, Mathias Krause wrote:
>> This is v3 of the patch series initially posted here:
>>
>>   https://lkml.org/lkml/2014/6/22/149
>>
>> This series tries to address the problem of dangling strings of __init
>> functions after initialization, as well as __exit strings for code not
>> even included in the final kernel image. The code might get freed, but
>> the format strings are not, as they're in the wrong section.
>
> What potential are we looking at here?

I tried to address that a few lines later in that email, also in the
individual patches. Let me quote the relevant lines here again:

>> The changes to arch/x86/ already
>> lead to moving ~3kb of memory from .rodata to .init.rodata.

>> To show that there's actual more value to it: A hacked up script, dully
>> changing pr_<level> to pi_<level> for __init functions under arch/x86/
>> is able to move ~8kB of r/o data into the .init section (partly already
>> covered by the patches of this series). The script, though, is dump. It
>> does not handle any of the printk() calls, nor does it handle panic()
>> calls or other strings used only in initialization code. So there's more
>> to squeeze out.

> Anything less than a page in size would likely not matter as this
> is the granularity we look into.

This is not true. The goal is not to increase the size of an .init
section but to decrease the size of the .text and .rodata sections.
For these even minor changes can lead to an additional free page if
that decrease makes the size of the .text / .rodata section cross a
page boundary. Thereby even small moves to the .init section like
format strings used only during initialization might make this happen.

> And the code needs to be built-in - which most drivers are not.

This, again, is not true. In fact, quite the opposite is true as
modules can benefit from it much better as the small reduction of
.text / .rodata sizes are not summed up but accounted per module. If
such a small decrease makes the .text / .rodata section cross a page
boundary, a whole page can be saved.

> So is it really worth it?

I took the hacky script mentioned in the first email (now also
attached, init_str.awk), let it ran over the whole kernel, fixed the
fall-out and did an allmodconfig build. Beside the script being far
from perfect (it catches a few false positives and, more important,
only covers a fraction of the potential strings -- only the pr_*()
macros) it already leads to 24 modules requiring a page less during
run-time because of the afore mentioned page crossing. It includes
popular modules like hid.ko and ipv6.ko, so almost anybody would
benefit from it. But as the page crossing effect depends on the
particular kernel configuration and toolchain setup, fell free to do
your own tests. The script doing the measurement is attached, too
(mod_info.sh)

> The places that already moves the string to __init/__exit should maybe
> be droopped to avoid the complication.

...or just be changed to hide the "magic".


Thanks,
Mathias

[-- Attachment #2: init_str.awk --]
[-- Type: application/x-awk, Size: 1125 bytes --]

[-- Attachment #3: mod_info.sh --]
[-- Type: application/x-sh, Size: 1799 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCHv3 0/9] Mark literal strings in __init / __exit code
  2014-08-24 16:04   ` Mathias Krause
@ 2014-08-24 16:28     ` Joe Perches
  0 siblings, 0 replies; 18+ messages in thread
From: Joe Perches @ 2014-08-24 16:28 UTC (permalink / raw)
  To: Mathias Krause
  Cc: Sam Ravnborg, Andrew Morton, Thomas Gleixner, Ingo Molnar,
	H. Peter Anvin, Rasmus Villemoes, linux-kernel

On Sun, 2014-08-24 at 18:04 +0200, Mathias Krause wrote:
> On 21 August 2014 18:25, Sam Ravnborg <sam@ravnborg.org> wrote:
> > So is it really worth it?
> 
> I took the hacky script mentioned in the first email (now also
> attached, init_str.awk), let it ran over the whole kernel, fixed the
> fall-out and did an allmodconfig build. Beside the script being far
> from perfect (it catches a few false positives and, more important,
> only covers a fraction of the potential strings -- only the pr_*()
> macros) it already leads to 24 modules requiring a page less during
> run-time because of the afore mentioned page crossing. It includes
> popular modules like hid.ko and ipv6.ko, so almost anybody would
> benefit from it. But as the page crossing effect depends on the
> particular kernel configuration and toolchain setup, fell free to do
> your own tests. The script doing the measurement is attached, too
> (mod_info.sh)

I think it would be worth it.

I think the added complexity converting:
	printk/pr_<level>
to
	pi_<level>/pe_<level>
for init/exit sections is a lightweight, one-time cost and
continuing upstream maintainer involvement is next to nil.

This mechanism supports all existing compiler toolchains.

Updating toolchains and or using a gcc compiler plugin
means that extra work would also need to be done for llvm
and any other alternate compiler.

And, as Mathias mentioned, the string deduplication logic
may be somewhat complicated for each compiler.

The worst that happens if a printk is added that is not
written as pi_<level> is some unnecessary .text is consumed.

No doubt someone will every so often run Mathias' scripts
against the kernel tree and submit the odd patch to fix it
up.



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCHv3 0/9] Mark literal strings in __init / __exit code
  2014-08-22  8:24     ` Ingo Molnar
@ 2014-08-30 15:28       ` Mathias Krause
  2014-09-16  8:57         ` Ingo Molnar
  0 siblings, 1 reply; 18+ messages in thread
From: Mathias Krause @ 2014-08-30 15:28 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Andrew Morton, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	Joe Perches, Rasmus Villemoes, linux-kernel, Linus Torvalds

On 22 August 2014 10:24, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Mathias Krause <minipli@googlemail.com> wrote:
>
>> > It feels like a burdensome hack that kernel developers are
>> > forced to use different printing facilities, depending on
>> > the life cycle of a method. We want to simplify init
>> > annotations, we don't want to complicate them!
>>
>> Does this:
>>
>>     pi_info("mkiss: AX.25 Multikiss, Hans Albas PE1AYX\n");
>>
>> really look more complicated compared to this?:
>>
>>     static const char banner[] __initconst = KERN_INFO \
>>         "mkiss: AX.25 Multikiss, Hans Albas PE1AYX\n";
>>     printk(banner);
>>
>> The latter can be found in drivers/net/hamradio/mkiss.c. Aged
>> code, admitted. But we have a few more of those and the
>> pi_info() looks way more appealing to me by doing the very
>> same thing ;)
>
> My point is that both are complicated, compared to using the
> regular printk() variants.
>
> In general printk()s should not be aware of what function
> context they are in.

I agree on the latter. But, unfortunately, the tooling side is not
able to allow this kind of optimization without being told so.

>
>> > And if tooling isn't ready for this, then wouldn't the
>> > right solution be to improve tooling - instead of
>> > complicating the kernel?
>>
>> Plugin based approaches for this have been mentioned before
>> but those suffer from the same "global view" problem. So,
>> IMHO it's not that easy to solve it at the toolchain level.
>
> It might be solved via a regular tooling feature, or via
> tooling extension based on plugins.

Joe has some valid points here. Solving it on the tooling side is 1/
not that easy to do (needs a compiler plugin or direct changes to the
compiler) and 2/ would require it to be solved for all tool-chains, so
all of them could benefit from it. Even more, solving it on the
tool-chain side, i.e. the compiler, would limit it to a fairly recent
tool-chain -- one that still needs to be written. OTOH, solving it on
the source code level makes it compatible with existing tool-chains.
It also allows a fine grained control of what code will be
instrumented and which will not -- much like we do with the __init /
__exit annotations today. It's explicit, not implicit. It would be
hard to express exceptions to the 'put format string into the
.init.rodata section for __init code' rule if it would be solved on
the tooling side. For source level changes it would be easy -- just do
not change that specific printk() call.

>
> Regardless of how it's implemented on the tooling side, my
> point remains: this kind of optimization is done on the tooling
> side in a natural fashion, while it's an ongoing maintenance
> concern on the kernel side.

The costs of making the required changes to the code, i.e. changing
printk() / pr_*() to pi_*() / pe_*(), are a necessary pain but are a
one-time cost, as Joe already said. Beside that, they're an
optimization, after all. Init / Exit code not changed to make use of
the pi_* / pe_* helpers just does not get the run-time memory savings.
So the existing code base can be changed on demand -- just as it is
done with the printk() to pr_*() conversion. Just in this case the
conversion has some additional gain beside cleaning up the code. It
allows saving run-time memory. So it's more than a cosmetically change
to the code.

>
> So it should be done on the tooling side, especially as the
> benefits appear to be marginal.

But still, they are measurable. As I showed on the other thread, even
popular modules like hid.ko or ipv6.ko can gain from dumb, scripted
changes. So, to repeat Joe's words: It's worth it, IMHO.


Regards,
Mathias

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCHv3 0/9] Mark literal strings in __init / __exit code
  2014-08-30 15:28       ` Mathias Krause
@ 2014-09-16  8:57         ` Ingo Molnar
  0 siblings, 0 replies; 18+ messages in thread
From: Ingo Molnar @ 2014-09-16  8:57 UTC (permalink / raw)
  To: Mathias Krause
  Cc: Andrew Morton, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	Joe Perches, Rasmus Villemoes, linux-kernel, Linus Torvalds


* Mathias Krause <minipli@googlemail.com> wrote:

> > Regardless of how it's implemented on the tooling side, my 
> > point remains: this kind of optimization is done on the 
> > tooling side in a natural fashion, while it's an ongoing 
> > maintenance concern on the kernel side.
> 
> The costs of making the required changes to the code, i.e. 
> changing printk() / pr_*() to pi_*() / pe_*(), are a necessary 
> pain but are a one-time cost, as Joe already said. [...]

That argument is bogus - the costs form increased complexity are 
ongoing for all new code affected by such constructs, and they 
are an ongoing cost for all changes to the code as well.

> > So it should be done on the tooling side, especially as the 
> > benefits appear to be marginal.
> 
> But still, they are measurable. [...]

So is the cost of complexity measurable: we already got rid of 
__init annotation variants, and we want to keep it simple and 
maintainable, not litter the code with new variants again, only 
to be warned about in build time checks that few developers run.

And when it comes to weighing increased complexity against some 
marginal benefit, usually the simpler approach is preferred, 
especially since it could all be solved via tooling. Sure, you 
have to implement the tooling support for that, and have to wait 
for that to trickle through to actual build environments - but in 
turn that would benefit a lot more projects than the kernel 
alone. If you are impatient you could do tooling in the kernel as 
well, in tools/ for example.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2014-09-16  8:57 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-08-21 12:23 [PATCHv3 0/9] Mark literal strings in __init / __exit code Mathias Krause
2014-08-21 12:23 ` [PATCHv3 1/9] init.h: Add __init_str / __exit_str macros Mathias Krause
2014-08-21 12:23 ` [PATCHv3 2/9] printk: Provide pi_<level> / pe_<level> macros for __init / __exit code Mathias Krause
2014-08-21 12:23 ` [PATCHv3 3/9] kallsyms: exclude pseudo symbols for __init / __exit strings Mathias Krause
2014-08-21 12:23 ` [PATCHv3 4/9] modpost: provide better diagnostics " Mathias Krause
2014-08-21 12:23 ` [PATCHv3 5/9] x86, acpi: Mark __init strings as such Mathias Krause
2014-08-21 12:23 ` [PATCHv3 6/9] x86, mm: Make x86_init.memory_setup() return a const char * Mathias Krause
2014-08-21 12:23 ` [PATCHv3 7/9] x86, mm: early_panic() - pass on the message as string Mathias Krause
2014-08-21 12:23 ` [PATCHv3 8/9] x86, mm: e820 - mark __init strings as such Mathias Krause
2014-08-21 12:23 ` [PATCHv3 9/9] x86: setup " Mathias Krause
2014-08-21 12:57 ` [PATCHv3 0/9] Mark literal strings in __init / __exit code Ingo Molnar
2014-08-21 14:29   ` Mathias Krause
2014-08-22  8:24     ` Ingo Molnar
2014-08-30 15:28       ` Mathias Krause
2014-09-16  8:57         ` Ingo Molnar
2014-08-21 16:25 ` Sam Ravnborg
2014-08-24 16:04   ` Mathias Krause
2014-08-24 16:28     ` Joe Perches

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.