[RFC PATCH 0/3] Mark literal strings in __init / __exit code

[RFC PATCH 0/3] Mark literal strings in __init / __exit code

[Mathias Krause]

This RFC 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.

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.

Patch 3 exemplarily changes all strings and format strings in
arch/x86/kernel/acpi/boot.c to use the new macros. It also addresses a
few styling issues, though. But this already leads to ~1.7 kB of r/o
data moved to the .init.rodata section, marking it for release after
init.


Open issues with this approach:

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/ The variables used in the macros of patch 1 will pollute the symtab
with unneeded entries. That'll be a problem in the KALLSYMS_ALL case
only, though. But the symtab will be huge then, anyway. However,
filtering those even in this case might be desirable.

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

   strncmp(str, "s4_nohwsig", 10)

becomes this:

   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.


So, is there interest in having such macros and markings? Patch 3 shows,
that there's actual value in it. A hacked up script, dully changing
pr_<level> to pi_<level> for __init functions under arch/x86/ already is
able to move ~8kB of r/o data into the .init section. 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. I just want to get some feedback
first.

Also documentation of the new macros is missing, maybe even a
checkpatch.pl change to propose using the new macros instead of pr_*()
or plain printk() in __init / __exit functions.

What do you think?

Regards,
Mathias

Mathias Krause (3):
  init.h: Add __init_str / __exit_str macros
  printk: Provide pi_<level> / pe_<level> macros for __init / __exit
    code
  x86, acpi: Mark __init strings as such

 arch/x86/kernel/acpi/boot.c |  162 ++++++++++++++++++++-----------------------
 include/linux/init.h        |   18 +++++
 include/linux/printk.h      |   52 ++++++++++++++
 3 files changed, 144 insertions(+), 88 deletions(-)

-- 
1.7.10.4

[RFC PATCH 1/3] init.h: Add __init_str / __exit_str macros

[Mathias Krause]

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

Signed-off-by: Mathias Krause <minipli@googlemail.com>
---
 include/linux/init.h |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/include/linux/init.h b/include/linux/init.h
index 2df8e8dd10..0a425b296e 100644
--- a/include/linux/init.h
+++ b/include/linux/init.h
@@ -35,6 +35,18 @@
  * 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 messages printed 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 void __init initme(void)
+ * {
+ *    printk(__init_str(KERN_INFO "I am fully initialized, now\n"));
+ * }
  */
 
 /* These are for everybody (although not all archs will actually
@@ -45,6 +57,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

Re: [RFC PATCH 1/3] init.h: Add __init_str / __exit_str macros

[Joe Perches]

On Mon, 2014-06-23 at 00:46 +0200, Mathias Krause wrote:
> Add macros to be able to mark string literals used in __init / __exit
> functions.
[]
> diff --git a/include/linux/init.h b/include/linux/init.h
[]
> +#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; })
> +

You probably want to make these strings vanish
completely when !CONFIG_PRINTK.

As is, they will always exist in the image.

Re: [RFC PATCH 1/3] init.h: Add __init_str / __exit_str macros

[Mathias Krause]

On 24 June 2014 21:43, Joe Perches <joe@perches.com> wrote:
> On Mon, 2014-06-23 at 00:46 +0200, Mathias Krause wrote:
>> Add macros to be able to mark string literals used in __init / __exit
>> functions.
> []
>> diff --git a/include/linux/init.h b/include/linux/init.h
> []
>> +#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; })
>> +
>
> You probably want to make these strings vanish
> completely when !CONFIG_PRINTK.
>
> As is, they will always exist in the image.

They will not. They are vanished as printk() is an empty static inline
function for the !CONFIG_PRINTK case. gcc is clever enough to optimize
the variables away in this case.

Thanks,
Mathias

[RFC PATCH 2/3] printk: Provide pi_<level> / pe_<level> macros for __init / __exit code

[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.

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>
---
 include/linux/printk.h |   52 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index 319ff7e53e..b49dd6dbc1 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -226,6 +226,8 @@ 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 pi_*() and pe_*() variants are provided for usage in __init / __exit
+ * code.
  */
 #define pr_emerg(fmt, ...) \
 	printk(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
@@ -245,13 +247,55 @@ extern asmlinkage void dump_stack(void) __cold;
 #define pr_cont(fmt, ...) \
 	printk(KERN_CONT fmt, ##__VA_ARGS__)
 
+#define pi_emerg(fmt, ...) \
+	printk(__init_str(KERN_EMERG pr_fmt(fmt)), ##__VA_ARGS__)
+#define pi_alert(fmt, ...) \
+	printk(__init_str(KERN_ALERT pr_fmt(fmt)), ##__VA_ARGS__)
+#define pi_crit(fmt, ...) \
+	printk(__init_str(KERN_CRIT pr_fmt(fmt)), ##__VA_ARGS__)
+#define pi_err(fmt, ...) \
+	printk(__init_str(KERN_ERR pr_fmt(fmt)), ##__VA_ARGS__)
+#define pi_warning(fmt, ...) \
+	printk(__init_str(KERN_WARNING pr_fmt(fmt)), ##__VA_ARGS__)
+#define pi_warn pi_warning
+#define pi_notice(fmt, ...) \
+	printk(__init_str(KERN_NOTICE pr_fmt(fmt)), ##__VA_ARGS__)
+#define pi_info(fmt, ...) \
+	printk(__init_str(KERN_INFO pr_fmt(fmt)), ##__VA_ARGS__)
+#define pi_cont(fmt, ...) \
+	printk(__init_str(KERN_CONT fmt), ##__VA_ARGS__)
+
+#define pe_emerg(fmt, ...) \
+	printk(__exit_str(KERN_EMERG pr_fmt(fmt)), ##__VA_ARGS__)
+#define pe_alert(fmt, ...) \
+	printk(__exit_str(KERN_ALERT pr_fmt(fmt)), ##__VA_ARGS__)
+#define pe_crit(fmt, ...) \
+	printk(__exit_str(KERN_CRIT pr_fmt(fmt)), ##__VA_ARGS__)
+#define pe_err(fmt, ...) \
+	printk(__exit_str(KERN_ERR pr_fmt(fmt)), ##__VA_ARGS__)
+#define pe_warning(fmt, ...) \
+	printk(__exit_str(KERN_WARNING pr_fmt(fmt)), ##__VA_ARGS__)
+#define pe_warn pe_warning
+#define pe_notice(fmt, ...) \
+	printk(__exit_str(KERN_NOTICE pr_fmt(fmt)), ##__VA_ARGS__)
+#define pe_info(fmt, ...) \
+	printk(__exit_str(KERN_INFO pr_fmt(fmt)), ##__VA_ARGS__)
+#define pe_cont(fmt, ...) \
+	printk(__exit_str(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_str(KERN_DEBUG pr_fmt(fmt)), ##__VA_ARGS__)
+#define pe_devel(fmt, ...) \
+	printk(__exit_str(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>
@@ -261,12 +305,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_str(KERN_DEBUG pr_fmt(fmt)), ##__VA_ARGS__)
+#define pe_debug(fmt, ...) \
+	printk(__exit_str(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

[RFC PATCH 3/3] x86, acpi: Mark __init strings as such

[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.

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

Should the checkpatch warnings be handled, too, e.g. removing the line
breaks for strings?:

WARNING: quoted string split across lines
#421: FILE: arch/x86/kernel/acpi/boot.c:1263:
+               pi_notice("Warning: DMI blacklist says broken, but acpi "
+                         "forced\n");

I'd rather not remove the braces as it's IMHO more readable and more
consistent with the coding style with them, as this is just the start
of a huge if-else-if-.. block:

WARNING: braces {} are not necessary for single statement blocks
#453: FILE: arch/x86/kernel/acpi/boot.c:1520:
+       if (strcmp(arg, _("off")) == 0) {
                disable_acpi();
        }

 arch/x86/kernel/acpi/boot.c |  162 ++++++++++++++++++++-----------------------
 1 file changed, 74 insertions(+), 88 deletions(-)

diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 86281ffb96..dc7d8b1f54 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>
@@ -53,7 +55,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 */
@@ -167,15 +169,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,
@@ -196,7 +197,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;
 	}
 
@@ -236,11 +237,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;
@@ -319,7 +320,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;
 }
@@ -337,7 +338,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;
 }
@@ -420,14 +421,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");
 		}
 	}
 
@@ -503,7 +504,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);
 }
@@ -622,7 +623,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;
 	}
 
@@ -678,7 +679,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;
 	}
 
@@ -698,13 +699,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;
 	}
 
@@ -716,9 +716,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
@@ -729,21 +728,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
@@ -754,7 +751,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;
@@ -805,8 +802,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;
 }
@@ -833,8 +829,7 @@ static int __init early_acpi_parse_madt_lapic_addr_ovr(void)
 	    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;
 	}
 
@@ -860,8 +855,7 @@ static int __init acpi_parse_madt_lapic_entries(void)
 	    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;
 	}
 
@@ -877,11 +871,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;
 	}
@@ -892,7 +886,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;
 	}
@@ -950,7 +944,7 @@ 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
@@ -988,7 +982,7 @@ 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 */
 		}
 
@@ -1056,16 +1050,15 @@ int mp_register_gsi(struct device *dev, u32 gsi, int trigger, int polarity)
 
 	ioapic = mp_find_ioapic(gsi);
 	if (ioapic < 0) {
-		printk(KERN_WARNING "No IOAPIC for GSI %u\n", gsi);
+		pr_warn("No IOAPIC for GSI %u\n", gsi);
 		return gsi;
 	}
 
 	ioapic_pin = mp_find_ioapic_pin(ioapic, gsi);
 
 	if (ioapic_pin > MP_MAX_IOAPIC_PIN) {
-		printk(KERN_ERR "Invalid reference to IOAPIC pin "
-		       "%d-%d\n", mpc_ioapic_id(ioapic),
-		       ioapic_pin);
+		pr_err("Invalid reference to IOAPIC pin %d-%d\n",
+		       mpc_ioapic_id(ioapic), ioapic_pin);
 		return gsi;
 	}
 
@@ -1106,8 +1099,7 @@ 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;
 	}
 
@@ -1115,10 +1107,10 @@ static int __init acpi_parse_madt_ioapic_entries(void)
 	    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;
 	}
 
@@ -1126,8 +1118,7 @@ static int __init acpi_parse_madt_ioapic_entries(void)
 	    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;
 	}
@@ -1147,7 +1138,7 @@ static int __init acpi_parse_madt_ioapic_entries(void)
 	    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;
 	}
@@ -1180,8 +1171,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();
 		}
 	}
@@ -1216,8 +1206,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 {
@@ -1227,8 +1216,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;
 		}
 	}
@@ -1238,11 +1226,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;
 }
@@ -1250,8 +1238,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;
@@ -1260,8 +1247,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;
@@ -1270,11 +1256,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;
 }
@@ -1285,8 +1271,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;
@@ -1469,9 +1455,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;
 		}
@@ -1531,32 +1517,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. */
@@ -1569,7 +1555,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;
 }
@@ -1580,9 +1566,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
@@ -1609,16 +1595,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
-- 
1.7.10.4

Re: [RFC PATCH 0/3] Mark literal strings in __init / __exit code

[Joe Perches]

On Mon, 2014-06-23 at 00:46 +0200, Mathias Krause wrote:
> This RFC 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.
> 
> 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.
> 
> Patch 3 exemplarily changes all strings and format strings in
> arch/x86/kernel/acpi/boot.c to use the new macros. It also addresses a
> few styling issues, though. But this already leads to ~1.7 kB of r/o
> data moved to the .init.rodata section, marking it for release after
> init.
> 
> 
> Open issues with this approach:
> 
> 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/ The variables used in the macros of patch 1 will pollute the symtab
> with unneeded entries. That'll be a problem in the KALLSYMS_ALL case
> only, though. But the symtab will be huge then, anyway. However,
> filtering those even in this case might be desirable.
> 
> 3/ It only seamlessly integrates for the pr_<level>() kind of use cases.
> For other literal strings it gets slightly less readable, e.g. this:
> 
>    strncmp(str, "s4_nohwsig", 10)
> 
> becomes this:
> 
>    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.
> 
> 
> So, is there interest in having such macros and markings? Patch 3 shows,
> that there's actual value in it. A hacked up script, dully changing
> pr_<level> to pi_<level> for __init functions under arch/x86/ already is
> able to move ~8kB of r/o data into the .init section. 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. I just want to get some feedback
> first.
> 
> Also documentation of the new macros is missing, maybe even a
> checkpatch.pl change to propose using the new macros instead of pr_*()
> or plain printk() in __init / __exit functions.
> 
> What do you think?

I once proposed a similar thing.

https://lkml.org/lkml/2009/7/21/421

Matt Mackall replied

https://lkml.org/lkml/2009/7/21/463

Re: [RFC PATCH 0/3] Mark literal strings in __init / __exit code

[Mathias Krause]

On 23 June 2014 00:56, Joe Perches <joe@perches.com> wrote:
> On Mon, 2014-06-23 at 00:46 +0200, Mathias Krause wrote:
>> [...] 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.
>>
>> Patch 3 exemplarily changes all strings and format strings in
>> arch/x86/kernel/acpi/boot.c to use the new macros. It also addresses a
>> few styling issues, though. But this already leads to ~1.7 kB of r/o
>> data moved to the .init.rodata section, marking it for release after
>> init.
>>
>> [...]
>
> I once proposed a similar thing.
>
> https://lkml.org/lkml/2009/7/21/421
>
> Matt Mackall replied
>
> https://lkml.org/lkml/2009/7/21/463
>

Thanks for the pointers. Have you looked at patch 2 and 3? I don't
think it makes the printk() case ugly. In fact, using pi_<level>()
should be no less readable then pr_<level>, no?

Thanks,
Mathias

Re: [RFC PATCH 0/3] Mark literal strings in __init / __exit code

[Joe Perches]

On Mon, 2014-06-23 at 08:23 +0200, Mathias Krause wrote:
> On 23 June 2014 00:56, Joe Perches <joe@perches.com> wrote:
> > On Mon, 2014-06-23 at 00:46 +0200, Mathias Krause wrote:
> >> [...] 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.
> >>
> >> Patch 3 exemplarily changes all strings and format strings in
> >> arch/x86/kernel/acpi/boot.c to use the new macros. It also addresses a
> >> few styling issues, though. But this already leads to ~1.7 kB of r/o
> >> data moved to the .init.rodata section, marking it for release after
> >> init.
> >>
> >> [...]
> >
> > I once proposed a similar thing.
> >
> > https://lkml.org/lkml/2009/7/21/421
> >
> > Matt Mackall replied
> >
> > https://lkml.org/lkml/2009/7/21/463
> >
> 
> Thanks for the pointers. Have you looked at patch 2 and 3? I don't
> think it makes the printk() case ugly. In fact, using pi_<level>()
> should be no less readable then pr_<level>, no?

I don't think it's particularly less readable, but I
do think using the plug-in mechanism might be a better
option as it would need no manual markings at all.

cheers, Joe

Re: [RFC PATCH 0/3] Mark literal strings in __init / __exit code

[Rasmus Villemoes]

Joe Perches <joe@perches.com> writes:

> On Mon, 2014-06-23 at 08:23 +0200, Mathias Krause wrote:
>> On 23 June 2014 00:56, Joe Perches <joe@perches.com> wrote:
>> > On Mon, 2014-06-23 at 00:46 +0200, Mathias Krause wrote:
>> >> [...] 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.
>> >>
>> >> Patch 3 exemplarily changes all strings and format strings in
>> >> arch/x86/kernel/acpi/boot.c to use the new macros. It also addresses a
>> >> few styling issues, though. But this already leads to ~1.7 kB of r/o
>> >> data moved to the .init.rodata section, marking it for release after
>> >> init.
>> >>
>> >> [...]
>> >
>> > I once proposed a similar thing.
>> >
>> > https://lkml.org/lkml/2009/7/21/421
>> >
>> > Matt Mackall replied
>> >
>> > https://lkml.org/lkml/2009/7/21/463
>> >
>> 
>> Thanks for the pointers. Have you looked at patch 2 and 3? I don't
>> think it makes the printk() case ugly. In fact, using pi_<level>()
>> should be no less readable then pr_<level>, no?
>
> I don't think it's particularly less readable, but I
> do think using the plug-in mechanism might be a better
> option as it would need no manual markings at all.

gcc already seems to contain infrastructure for this kind of thing, so
maybe it doesn't even require a plugin, but simply a little coordination
with the gcc folks. This snippet from gcc internals seems relevant:

-- Target Hook: section * TARGET_ASM_FUNCTION_RODATA_SECTION (tree
          DECL)
     Return the readonly data section associated with 'DECL_SECTION_NAME
     (DECL)'.  The default version of this function selects
     '.gnu.linkonce.r.name' if the function's section is
     '.gnu.linkonce.t.name', '.rodata.name' if function is in
     '.text.name', and the normal readonly-data section otherwise.

Rasmus

Re: [RFC PATCH 0/3] Mark literal strings in __init / __exit code

[Mathias Krause]

On 24 June 2014 16:31, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> Joe Perches <joe@perches.com> writes:
>
>> On Mon, 2014-06-23 at 08:23 +0200, Mathias Krause wrote:
>>> On 23 June 2014 00:56, Joe Perches <joe@perches.com> wrote:
>>> > On Mon, 2014-06-23 at 00:46 +0200, Mathias Krause wrote:
>>> >> [...] 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.
>>> >>
>>> >> Patch 3 exemplarily changes all strings and format strings in
>>> >> arch/x86/kernel/acpi/boot.c to use the new macros. It also addresses a
>>> >> few styling issues, though. But this already leads to ~1.7 kB of r/o
>>> >> data moved to the .init.rodata section, marking it for release after
>>> >> init.
>>> >>
>>> >> [...]
>>> >
>>> > I once proposed a similar thing.
>>> >
>>> > https://lkml.org/lkml/2009/7/21/421
>>> >
>>> > Matt Mackall replied
>>> >
>>> > https://lkml.org/lkml/2009/7/21/463
>>> >
>>>
>>> Thanks for the pointers. Have you looked at patch 2 and 3? I don't
>>> think it makes the printk() case ugly. In fact, using pi_<level>()
>>> should be no less readable then pr_<level>, no?
>>
>> I don't think it's particularly less readable, but I
>> do think using the plug-in mechanism might be a better
>> option as it would need no manual markings at all.
>
> gcc already seems to contain infrastructure for this kind of thing, so
> maybe it doesn't even require a plugin, but simply a little coordination
> with the gcc folks. This snippet from gcc internals seems relevant:
>
> -- Target Hook: section * TARGET_ASM_FUNCTION_RODATA_SECTION (tree
>           DECL)
>      Return the readonly data section associated with 'DECL_SECTION_NAME
>      (DECL)'.  The default version of this function selects
>      '.gnu.linkonce.r.name' if the function's section is
>      '.gnu.linkonce.t.name', '.rodata.name' if function is in
>      '.text.name', and the normal readonly-data section otherwise.
>

I don't think it's that easy. You cannot simply put all strings into
the .init.rodata section when code currently gets emitted to
.init.text. The reason is because strings used in __init code might be
referenced later on, too. For example, the name passed to
class_create() won't be copied. If that one would go into the
.init.rodata section automatically, we would have dangling pointers
after the .init.* memory got freed. Therefore a compiler driven
approach would need to be implemented as a compiler extension, a gcc
plugin to handle such cases -- know when a string can safely be put
into the .init.rodata section and when not. But that decision is not
as easy as Joe might think it would be. How would the plugin know
which strings to put into the .init.rodata section? Would it only
handle the ones passed to printk()? How to ensure those are actually
safe strings? The pr_<level> macros can ensure this by using
preprocessor string concatenation internally. Therefore those are
unique and can safely be put into the .init.rodata section. But the
compiler won't see the pr_<level> calls any more, only the printk().
But that one might be a direct call, called with a string used later
on, too. How would a plugin be able to detect this?
What about strings used in strcmp() and the like? What about strings
stored in temporary variables before getting passed to printk()? Those
would require the plugin to track variable assignment, too. That,
again, complicates the plugin.
For a fully automatic handling of such strings a global analysis is
needed, e.g. LTO is *required* to know if the strings passed to the
functions will be stored in data structures not backed in .init.*
sections. That's far from being easy to implement and even farther
away from being widely used. Beside that, for being able to use gcc
plugins, the plugin headers need to be installed -- not only the
compiler. Again, a perquisite, not available by default.

Therefore I still strongly believe it's better to do this manually.
It's way simpler and much more explicit and therefore less surprising.
Even if this means we need to change quite a lot of code to get the
best out of it. For a start it really depends on a few subsystem
maintainers to accept patches to get an initial value out of it. Heck,
even for the non-embedded case, it has value -- at least for me. I
like to get rid of the unused strings. It's just a few pages,
admitted. But hey, we can get them almost for free. So, why not just
do it?

Regards,
Mathias

Re: [RFC PATCH 0/3] Mark literal strings in __init / __exit code

[Joe Perches]

On Tue, 2014-06-24 at 21:13 +0200, Mathias Krause wrote:
> On 24 June 2014 16:31, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
[]
> > gcc already seems to contain infrastructure for this kind of thing, so
> > maybe it doesn't even require a plugin, but simply a little coordination
> > with the gcc folks. This snippet from gcc internals seems relevant:
> >
> > -- Target Hook: section * TARGET_ASM_FUNCTION_RODATA_SECTION (tree
> >           DECL)
> >      Return the readonly data section associated with 'DECL_SECTION_NAME
> >      (DECL)'.  The default version of this function selects
> >      '.gnu.linkonce.r.name' if the function's section is
> >      '.gnu.linkonce.t.name', '.rodata.name' if function is in
> >      '.text.name', and the normal readonly-data section otherwise.
> >
> 
> I don't think it's that easy. You cannot simply put all strings into
> the .init.rodata section when code currently gets emitted to
> .init.text. The reason is because strings used in __init code might be
> referenced later on, too. For example, the name passed to
> class_create() won't be copied. If that one would go into the
> .init.rodata section automatically, we would have dangling pointers
> after the .init.* memory got freed. Therefore a compiler driven
> approach would need to be implemented as a compiler extension, a gcc
> plugin to handle such cases -- know when a string can safely be put
> into the .init.rodata section and when not. But that decision is not
> as easy as Joe might think it would be. How would the plugin know
> which strings to put into the .init.rodata section? Would it only
> handle the ones passed to printk()?

Yes.

> I still strongly believe it's better to do this manually.

Maybe.

It'd work with any version of the compiler that way too.

It's a pretty simple transform.

I believe this will show most all of the __init
uses of printks:

$ grep-2.5.4 -rP --include=*.[ch] -n '\b__init\b[^\n][^\}]+\n}' * | \
  grep -P '^[\w\/\.]+:\d+:|\bprintk\b|\bpr_[a-z]+' | \
  grep -P -B1 '\bprintk\b|\bpr_[a-z]+'

This shows a little more than a 1000 __init printks
treewide that could be converted.

For example:
arch/ia64/include/asm/cyclone.h:6:extern void __init cyclone_setup(void);
        printk(KERN_ERR "Cyclone Counter: System not configured"
--
arch/ia64/kernel/acpi.c:66:static unsigned long __init acpi_find_rsdp(void)
                printk(KERN_WARNING PREFIX
--
arch/ia64/kernel/acpi.c:366:static int __init acpi_parse_madt(struct acpi_table
        printk(KERN_INFO PREFIX "Local APIC address %p\n", ipi_base_addr);

etc...

There are maybe 200 or so __exit ones.

Re: [RFC PATCH 0/3] Mark literal strings in __init / __exit code

[Mathias Krause]

On 24 June 2014 21:37, Joe Perches <joe@perches.com> wrote:
> On Tue, 2014-06-24 at 21:13 +0200, Mathias Krause wrote:
>> I don't think it's that easy. You cannot simply put all strings into
>> the .init.rodata section when code currently gets emitted to
>> .init.text. The reason is because strings used in __init code might be
>> referenced later on, too. For example, the name passed to
>> class_create() won't be copied. If that one would go into the
>> .init.rodata section automatically, we would have dangling pointers
>> after the .init.* memory got freed. Therefore a compiler driven
>> approach would need to be implemented as a compiler extension, a gcc
>> plugin to handle such cases -- know when a string can safely be put
>> into the .init.rodata section and when not. But that decision is not
>> as easy as Joe might think it would be. How would the plugin know
>> which strings to put into the .init.rodata section? Would it only
>> handle the ones passed to printk()?
>
> Yes.

Well, I would like to handle the easy ones, too. E.g. strings used in
parameter parsing, i.e. strcmp()s.

>> I still strongly believe it's better to do this manually.
>
> Maybe.
>
> It'd work with any version of the compiler that way too.

That's a much stronger argument, IMHO. It'll work with gcc < 4.5 and
clang, even. And, it does not require us to maintain compatibility to
the repeating gcc plugin API breakage.

> It's a pretty simple transform.

Indeed, it is.

> I believe this will show most all of the __init
> uses of printks:
>
> $ grep-2.5.4 -rP --include=*.[ch] -n '\b__init\b[^\n][^\}]+\n}' * | \
>   grep -P '^[\w\/\.]+:\d+:|\bprintk\b|\bpr_[a-z]+' | \
>   grep -P -B1 '\bprintk\b|\bpr_[a-z]+'
>
> This shows a little more than a 1000 __init printks
> treewide that could be converted.

A simple awk script found additional 5399 pr_<level> calls in __init
code and 188 calls in __exit code. So it's worth it, IMHO.

>
> For example:
> arch/ia64/include/asm/cyclone.h:6:extern void __init cyclone_setup(void);
>         printk(KERN_ERR "Cyclone Counter: System not configured"
> --
> arch/ia64/kernel/acpi.c:66:static unsigned long __init acpi_find_rsdp(void)
>                 printk(KERN_WARNING PREFIX
> --
> arch/ia64/kernel/acpi.c:366:static int __init acpi_parse_madt(struct acpi_table
>         printk(KERN_INFO PREFIX "Local APIC address %p\n", ipi_base_addr);
>

Those might benefit twice from the change by being converted to
pi_<level> calls along the way. So it's a win-win on all sides, no?

> etc...
>
> There are maybe 200 or so __exit ones.
>

Yeah, the __exit ones are used less. Nonetheless, they should be
converted, too. For completeness, at least.

Thanks,
Mathias

Re: [RFC PATCH 0/3] Mark literal strings in __init / __exit code

[Joe Perches]

On Tue, 2014-06-24 at 22:10 +0200, Mathias Krause wrote:
> I would like to handle the easy ones, too. E.g. strings used in
> parameter parsing, i.e. strcmp()s.

Sure, but that change is separable from printk conversions.

Any idea how much would be changed treewide and whether
or not those strings are not already in rodata?

Looking at it, I see generic strings like "on", "off",
"device", "high", "low".  All these are likely to be
duplications of strings in rodata.

Re: [RFC PATCH 0/3] Mark literal strings in __init / __exit code

[Mathias Krause]

On 24 June 2014 22:30, Joe Perches <joe@perches.com> wrote:
> On Tue, 2014-06-24 at 22:10 +0200, Mathias Krause wrote:
>> I would like to handle the easy ones, too. E.g. strings used in
>> parameter parsing, i.e. strcmp()s.
>
> Sure, but that change is separable from printk conversions.

Yes. Those need to be marked manually, based on auditing the code for
reuses of those strings in non-init code.

>
> Any idea how much would be changed treewide and whether
> or not those strings are not already in rodata?

No, sorry. I haven't looked for those specifically yet. There is even
more to look for: calls to panic() in __init code, or the name of
kmem_cache_create() -- it get's copied. So there is more to optimize
if one is patient enough ;)
And all of those strings should be in the .rodata section, now. But
why you're asking?

>
> Looking at it, I see generic strings like "on", "off",
> "device", "high", "low".  All these are likely to be
> duplications of strings in rodata.

Yes. My vanilla vmlinux build has quite a lot of copies of "off" in
it. But I doubt any linker would merge those. Does LTO do so?

Mathias

Re: [RFC PATCH 0/3] Mark literal strings in __init / __exit code

[Joe Perches]

On Tue, 2014-06-24 at 22:41 +0200, Mathias Krause wrote:
> there is more to optimize
> if one is patient enough ;)

Ever thus.

> And all of those strings should be in the .rodata section, now. But
> why you're asking?

Because now they will be duplicated in .rodata
and the __init section no?

> Yes. My vanilla vmlinux build has quite a lot of copies of "off" in
> it. But I doubt any linker would merge those. Does LTO do so?

I would expect that duplicated strings in
separate sections would not be merged.

Re: [RFC PATCH 0/3] Mark literal strings in __init / __exit code

[Mathias Krause]

On 24 June 2014 22:57, Joe Perches <joe@perches.com> wrote:
> On Tue, 2014-06-24 at 22:41 +0200, Mathias Krause wrote:
>> there is more to optimize
>> if one is patient enough ;)
>
> Ever thus.
>
>> And all of those strings should be in the .rodata section, now. But
>> why you're asking?
>
> Because now they will be duplicated in .rodata
> and the __init section no?

No. A string marked with __init_str() will only life in the
.init.rodata section. No duplication.

>> Yes. My vanilla vmlinux build has quite a lot of copies of "off" in
>> it. But I doubt any linker would merge those. Does LTO do so?
>
> I would expect that duplicated strings in
> separate sections would not be merged.

I do hope so, too! :D Because if strings in .rodata would be merged
with ones in .init.rodata the former would be dangling when the latter
are freed after initialization.
Having duplicated strings in .rodata and .init.rodata is also no
problem as the latter will be freed.

Regards,
Mathias

Re: [RFC PATCH 0/3] Mark literal strings in __init / __exit code

[Joe Perches]

On Tue, 2014-06-24 at 23:06 +0200, Mathias Krause wrote:
> On 24 June 2014 22:57, Joe Perches <joe@perches.com> wrote:
> > On Tue, 2014-06-24 at 22:41 +0200, Mathias Krause wrote:
> >> And all of those strings should be in the .rodata section, now. But
> >> why you're asking?
> > Because now they will be duplicated in .rodata
> > and the __init section no?
> No. A string marked with __init_str() will only life in the
> .init.rodata section. No duplication.

Unless the same string is used in another bit
of code.  Then there's duplication.

> Having duplicated strings in .rodata and .init.rodata is also no
> problem as the latter will be freed.

They increase image size.

Re: [RFC PATCH 0/3] Mark literal strings in __init / __exit code

[Mathias Krause]

On 24 June 2014 23:45, Joe Perches <joe@perches.com> wrote:
> On Tue, 2014-06-24 at 23:06 +0200, Mathias Krause wrote:
>> On 24 June 2014 22:57, Joe Perches <joe@perches.com> wrote:
>> > On Tue, 2014-06-24 at 22:41 +0200, Mathias Krause wrote:
>> >> And all of those strings should be in the .rodata section, now. But
>> >> why you're asking?
>> > Because now they will be duplicated in .rodata
>> > and the __init section no?
>> No. A string marked with __init_str() will only life in the
>> .init.rodata section. No duplication.
>
> Unless the same string is used in another bit
> of code.  Then there's duplication.

Rather, unless the same string gets used in an __init and a non-__init
section of the same compilation unit. Otherwise it would be only in
one section. But having the same string used in two sections of the
same compilation unit should be a rather rare case. Merging strings
across multiple compilation units does not happen, anyway -- not now,
not with the new macros.

>
>> Having duplicated strings in .rodata and .init.rodata is also no
>> problem as the latter will be freed.
>
> They increase image size.

But as this is a rare case, it shouldn't matter, really. The
compression should compensate that, compressing multiple occurrences
of the same string efficiently.

In the more likely case, strings used only in __init code, we would
have no image size increase but an increase of free memory after
initialization.


Thanks,
Mathias

Re: [RFC PATCH 0/3] Mark literal strings in __init / __exit code

[Rasmus Villemoes]

Mathias Krause <minipli@googlemail.com> writes:

> On 24 June 2014 16:31, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
>> gcc already seems to contain infrastructure for this kind of thing, so
>> maybe it doesn't even require a plugin, but simply a little coordination
>> with the gcc folks. This snippet from gcc internals seems relevant:
>>
>> -- Target Hook: section * TARGET_ASM_FUNCTION_RODATA_SECTION (tree
>>           DECL)
>>      Return the readonly data section associated with 'DECL_SECTION_NAME
>>      (DECL)'.  The default version of this function selects
>>      '.gnu.linkonce.r.name' if the function's section is
>>      '.gnu.linkonce.t.name', '.rodata.name' if function is in
>>      '.text.name', and the normal readonly-data section otherwise.
>>
>
> I don't think it's that easy. You cannot simply put all strings into
> the .init.rodata section when code currently gets emitted to
> .init.text. The reason is because strings used in __init code might be
> referenced later on, too. For example, the name passed to
> class_create() won't be copied.

Right, didn't think about that, so yes, the source would need
to be annotated some way or other, or gcc would need to learn the
semantics of certain kernel functions.

Speaking of dangling pointers: A similar disaster would happen if some
code containing pi_* calls gets copy-pasted to some non-__init
function. Could checkpatch learn to warn about calling these functions
from the wrong context?


Mathias Krause <minipli@googlemail.com> writes:

> Merging strings across multiple compilation units does not happen,
> anyway -- not now, not with the new macros.

Certainly string merging seems to happen, at least at -O1 and higher:

$ grep . *.c
a.c:const char *a(void) { return "654321"; }
b.c:const char *b(void) { return "4321"; }
c.c:const char *c(void) { return "654321"; }
main.c:#include <stdio.h>
main.c:const char *a(void);
main.c:const char *b(void);
main.c:const char *c(void);
main.c:int main(void)
main.c:{
main.c: printf("%p\n", a());
main.c: printf("%p\n", b());
main.c: printf("%p\n", c());
main.c: return 0;
main.c:}
$ gcc -O1 -c a.c && gcc -O1 -c b.c && gcc -O1 -c c.c
$ gcc -O1 main.c a.o b.o c.o
$ ./a.out 
0x400630
0x400632
0x400630

So not only are identical strings merged; suffixes are also optimized.

Rasmus

Re: [RFC PATCH 0/3] Mark literal strings in __init / __exit code

[Joe Perches]

On Wed, 2014-06-25 at 09:35 +0200, Rasmus Villemoes wrote:

> yes, the source would need
> to be annotated some way or other, or gcc would need to learn the
> semantics of certain kernel functions.
> 
> Speaking of dangling pointers: A similar disaster would happen if some
> code containing pi_* calls gets copy-pasted to some non-__init
> function.

This is my biggest issue with adding these new,
somewhat obscure macros.

> Could checkpatch learn to warn about calling these functions
> from the wrong context?

It's not possible.  checkpatch works on patch chunks.
Any patch chunk may not contain the function attributes.

> Mathias Krause <minipli@googlemail.com> writes:
> 
> > Merging strings across multiple compilation units does not happen,
> > anyway -- not now, not with the new macros.
> 
> Certainly string merging seems to happen, at least at -O1 and higher:
> 
> $ grep . *.c
> a.c:const char *a(void) { return "654321"; }
> b.c:const char *b(void) { return "4321"; }
> c.c:const char *c(void) { return "654321"; }
> main.c:#include <stdio.h>
> main.c:const char *a(void);
> main.c:const char *b(void);
> main.c:const char *c(void);
> main.c:int main(void)
> main.c:{
> main.c: printf("%p\n", a());
> main.c: printf("%p\n", b());
> main.c: printf("%p\n", c());
> main.c: return 0;
> main.c:}
> $ gcc -O1 -c a.c && gcc -O1 -c b.c && gcc -O1 -c c.c
> $ gcc -O1 main.c a.o b.o c.o
> $ ./a.out 
> 0x400630
> 0x400632
> 0x400630
> 
> So not only are identical strings merged; suffixes are also optimized.

Yup.  Nice example.

Re: [RFC PATCH 0/3] Mark literal strings in __init / __exit code

[Mathias Krause]

On 25 June 2014 09:48, Joe Perches <joe@perches.com> wrote:
> On Wed, 2014-06-25 at 09:35 +0200, Rasmus Villemoes wrote:
>> Speaking of dangling pointers: A similar disaster would happen if some
>> code containing pi_* calls gets copy-pasted to some non-__init
>> function.
>
> This is my biggest issue with adding these new,
> somewhat obscure macros.

modpost will handle these cases.

>> Could checkpatch learn to warn about calling these functions
>> from the wrong context?
>
> It's not possible.  checkpatch works on patch chunks.
> Any patch chunk may not contain the function attributes.

checkpatch.pl -f might detect them, though :/

Thanks,
Mathias

Re: [RFC PATCH 0/3] Mark literal strings in __init / __exit code

[Joe Perches]

On Wed, 2014-06-25 at 10:34 +0200, Mathias Krause wrote:
> On 25 June 2014 09:48, Joe Perches <joe@perches.com> wrote:
> > On Wed, 2014-06-25 at 09:35 +0200, Rasmus Villemoes wrote:
> >> Speaking of dangling pointers: A similar disaster would happen if some
> >> code containing pi_* calls gets copy-pasted to some non-__init
> >> function.
> >
> > This is my biggest issue with adding these new,
> > somewhat obscure macros.
> 
> modpost will handle these cases.

If so, then I don't see a problem.

> >> Could checkpatch learn to warn about calling these functions
> >> from the wrong context?
> >
> > It's not possible.  checkpatch works on patch chunks.
> > Any patch chunk may not contain the function attributes.
> 
> checkpatch.pl -f might detect them, though :/

Something like your awk script would be easier.

Re: [RFC PATCH 0/3] Mark literal strings in __init / __exit code

[Mathias Krause]

On 25 June 2014 09:35, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> Mathias Krause <minipli@googlemail.com> writes:
>
>> On 24 June 2014 16:31, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
>>> gcc already seems to contain infrastructure for this kind of thing, so
>>> maybe it doesn't even require a plugin, but simply a little coordination
>>> with the gcc folks. This snippet from gcc internals seems relevant:
>>>
>>> -- Target Hook: section * TARGET_ASM_FUNCTION_RODATA_SECTION (tree
>>>           DECL)
>>>      Return the readonly data section associated with 'DECL_SECTION_NAME
>>>      (DECL)'.  The default version of this function selects
>>>      '.gnu.linkonce.r.name' if the function's section is
>>>      '.gnu.linkonce.t.name', '.rodata.name' if function is in
>>>      '.text.name', and the normal readonly-data section otherwise.
>>>
>>
>> I don't think it's that easy. You cannot simply put all strings into
>> the .init.rodata section when code currently gets emitted to
>> .init.text. The reason is because strings used in __init code might be
>> referenced later on, too. For example, the name passed to
>> class_create() won't be copied.
>
> Right, didn't think about that, so yes, the source would need
> to be annotated some way or other, or gcc would need to learn the
> semantics of certain kernel functions.

I would rather like to avoid the latter.

>
> Speaking of dangling pointers: A similar disaster would happen if some
> code containing pi_* calls gets copy-pasted to some non-__init
> function. Could checkpatch learn to warn about calling these functions
> from the wrong context?

But the same is already true for code. If an __init function gets
referenced from a non-__init function, the same dangling references
would occur. That's what scripts/mod/modpost is for -- detecting such
cases and warn about them. So this is already handled by the kernel
build system. A checkpatch warning might be a nice addition, anyway.

>
> Mathias Krause <minipli@googlemail.com> writes:
>
>> Merging strings across multiple compilation units does not happen,
>> anyway -- not now, not with the new macros.
>
> Certainly string merging seems to happen, at least at -O1 and higher:
>
> $ grep . *.c
> a.c:const char *a(void) { return "654321"; }
> b.c:const char *b(void) { return "4321"; }
> c.c:const char *c(void) { return "654321"; }
> main.c:#include <stdio.h>
> main.c:const char *a(void);
> main.c:const char *b(void);
> main.c:const char *c(void);
> main.c:int main(void)
> main.c:{
> main.c: printf("%p\n", a());
> main.c: printf("%p\n", b());
> main.c: printf("%p\n", c());
> main.c: return 0;
> main.c:}
> $ gcc -O1 -c a.c && gcc -O1 -c b.c && gcc -O1 -c c.c
> $ gcc -O1 main.c a.o b.o c.o
> $ ./a.out
> 0x400630
> 0x400632
> 0x400630
>
> So not only are identical strings merged; suffixes are also optimized.

Oh, indeed! But they're luckily not merged across sections. So no
problems with dangling pointers here, too.

Thanks,
Mathias

Re: [RFC PATCH 0/3] Mark literal strings in __init / __exit code

[Joe Perches]

On Mon, 2014-06-23 at 00:46 +0200, Mathias Krause wrote:
> This RFC 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.
> 
> 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.

Another solution might be, as David Daney suggested, using
gcc 4.5+ plug-ins to extract these format strings and
const char * arrays into specific sections automatically.

https://lkml.org/lkml/2009/7/21/483

Seems feasible, but there might be a negative of string
duplication in multiple sections that would otherwise
be consolidated into a single object.

Re: [RFC PATCH 0/3] Mark literal strings in __init / __exit code

[Mathias Krause]

On 23 June 2014 03:30, Joe Perches <joe@perches.com> wrote:
> On Mon, 2014-06-23 at 00:46 +0200, Mathias Krause wrote:
>> This RFC 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.
>>
>> 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.
>
> Another solution might be, as David Daney suggested, using
> gcc 4.5+ plug-ins to extract these format strings and
> const char * arrays into specific sections automatically.
>
> https://lkml.org/lkml/2009/7/21/483
>
> Seems feasible, but there might be a negative of string
> duplication in multiple sections that would otherwise
> be consolidated into a single object.
>

There is currently no infrastructure for gcc plugins in the kernel
tree. And using plugins might make the kernel even more depended on a
particular gcc version, as the plugin API changes with every version.
In fact, there is none, beside "use every exported function you can
get your hand on". And that API breaks with each and every new version
of gcc. This would put quite a bigger maintenance burden on such an
approach than providing appropriate wrappers, fixing the obvious
candidates and adding a checkpatch warning.

Thanks,
Mathias