* [RFC PATCH 0/3] Mark literal strings in __init / __exit code @ 2014-06-22 22:46 Mathias Krause 2014-06-22 22:46 ` [RFC PATCH 1/3] init.h: Add __init_str / __exit_str macros Mathias Krause ` (4 more replies) 0 siblings, 5 replies; 26+ messages in thread From: Mathias Krause @ 2014-06-22 22:46 UTC (permalink / raw) To: linux-kernel Cc: Andrew Morton, Greg Kroah-Hartman, Steven Rostedt, Joe Perches, Rafael J. Wysocki, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Jason Baron, 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [RFC PATCH 1/3] init.h: Add __init_str / __exit_str macros 2014-06-22 22:46 [RFC PATCH 0/3] Mark literal strings in __init / __exit code Mathias Krause @ 2014-06-22 22:46 ` Mathias Krause 2014-06-24 19:43 ` Joe Perches 2014-06-22 22:46 ` [RFC PATCH 2/3] printk: Provide pi_<level> / pe_<level> macros for __init / __exit code Mathias Krause ` (3 subsequent siblings) 4 siblings, 1 reply; 26+ messages in thread From: Mathias Krause @ 2014-06-22 22:46 UTC (permalink / raw) To: linux-kernel Cc: Andrew Morton, Greg Kroah-Hartman, Steven Rostedt, Joe Perches, Rafael J. Wysocki, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Jason Baron, 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [RFC PATCH 1/3] init.h: Add __init_str / __exit_str macros 2014-06-22 22:46 ` [RFC PATCH 1/3] init.h: Add __init_str / __exit_str macros Mathias Krause @ 2014-06-24 19:43 ` Joe Perches 2014-06-24 20:13 ` Mathias Krause 0 siblings, 1 reply; 26+ messages in thread From: Joe Perches @ 2014-06-24 19:43 UTC (permalink / raw) To: Mathias Krause Cc: linux-kernel, Andrew Morton, Greg Kroah-Hartman, Steven Rostedt, Rafael J. Wysocki, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Jason Baron 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC PATCH 1/3] init.h: Add __init_str / __exit_str macros 2014-06-24 19:43 ` Joe Perches @ 2014-06-24 20:13 ` Mathias Krause 0 siblings, 0 replies; 26+ messages in thread From: Mathias Krause @ 2014-06-24 20:13 UTC (permalink / raw) To: Joe Perches Cc: linux-kernel, Andrew Morton, Greg Kroah-Hartman, Steven Rostedt, Rafael J. Wysocki, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Jason Baron 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [RFC PATCH 2/3] printk: Provide pi_<level> / pe_<level> macros for __init / __exit code 2014-06-22 22:46 [RFC PATCH 0/3] Mark literal strings in __init / __exit code Mathias Krause 2014-06-22 22:46 ` [RFC PATCH 1/3] init.h: Add __init_str / __exit_str macros Mathias Krause @ 2014-06-22 22:46 ` Mathias Krause 2014-06-22 22:46 ` [RFC PATCH 3/3] x86, acpi: Mark __init strings as such Mathias Krause ` (2 subsequent siblings) 4 siblings, 0 replies; 26+ messages in thread From: Mathias Krause @ 2014-06-22 22:46 UTC (permalink / raw) To: linux-kernel Cc: Andrew Morton, Greg Kroah-Hartman, Steven Rostedt, Joe Perches, Rafael J. Wysocki, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Jason Baron, 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [RFC PATCH 3/3] x86, acpi: Mark __init strings as such 2014-06-22 22:46 [RFC PATCH 0/3] Mark literal strings in __init / __exit code Mathias Krause 2014-06-22 22:46 ` [RFC PATCH 1/3] init.h: Add __init_str / __exit_str macros Mathias Krause 2014-06-22 22:46 ` [RFC PATCH 2/3] printk: Provide pi_<level> / pe_<level> macros for __init / __exit code Mathias Krause @ 2014-06-22 22:46 ` Mathias Krause 2014-06-22 22:56 ` [RFC PATCH 0/3] Mark literal strings in __init / __exit code Joe Perches 2014-06-23 1:30 ` Joe Perches 4 siblings, 0 replies; 26+ messages in thread From: Mathias Krause @ 2014-06-22 22:46 UTC (permalink / raw) To: linux-kernel Cc: Andrew Morton, Greg Kroah-Hartman, Steven Rostedt, Joe Perches, Rafael J. Wysocki, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Jason Baron, 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [RFC PATCH 0/3] Mark literal strings in __init / __exit code 2014-06-22 22:46 [RFC PATCH 0/3] Mark literal strings in __init / __exit code Mathias Krause ` (2 preceding siblings ...) 2014-06-22 22:46 ` [RFC PATCH 3/3] x86, acpi: Mark __init strings as such Mathias Krause @ 2014-06-22 22:56 ` Joe Perches 2014-06-23 6:23 ` Mathias Krause 2014-06-23 1:30 ` Joe Perches 4 siblings, 1 reply; 26+ messages in thread From: Joe Perches @ 2014-06-22 22:56 UTC (permalink / raw) To: Mathias Krause Cc: linux-kernel, Andrew Morton, Greg Kroah-Hartman, Steven Rostedt, Rafael J. Wysocki, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Jason Baron 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC PATCH 0/3] Mark literal strings in __init / __exit code 2014-06-22 22:56 ` [RFC PATCH 0/3] Mark literal strings in __init / __exit code Joe Perches @ 2014-06-23 6:23 ` Mathias Krause 2014-06-23 6:33 ` Joe Perches 0 siblings, 1 reply; 26+ messages in thread From: Mathias Krause @ 2014-06-23 6:23 UTC (permalink / raw) To: Joe Perches Cc: linux-kernel, Andrew Morton, Greg Kroah-Hartman, Steven Rostedt, Rafael J. Wysocki, Thomas Gleixner, Ingo Molnar, H. Peter Anvin 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC PATCH 0/3] Mark literal strings in __init / __exit code 2014-06-23 6:23 ` Mathias Krause @ 2014-06-23 6:33 ` Joe Perches 2014-06-24 14:31 ` Rasmus Villemoes 0 siblings, 1 reply; 26+ messages in thread From: Joe Perches @ 2014-06-23 6:33 UTC (permalink / raw) To: Mathias Krause Cc: linux-kernel, Andrew Morton, Greg Kroah-Hartman, Steven Rostedt, Rafael J. Wysocki, Thomas Gleixner, Ingo Molnar, H. Peter Anvin 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC PATCH 0/3] Mark literal strings in __init / __exit code 2014-06-23 6:33 ` Joe Perches @ 2014-06-24 14:31 ` Rasmus Villemoes 2014-06-24 19:13 ` Mathias Krause 0 siblings, 1 reply; 26+ messages in thread From: Rasmus Villemoes @ 2014-06-24 14:31 UTC (permalink / raw) To: Joe Perches Cc: Mathias Krause, linux-kernel, Andrew Morton, Greg Kroah-Hartman, Steven Rostedt, Rafael J. Wysocki, Thomas Gleixner, Ingo Molnar, H. Peter Anvin 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC PATCH 0/3] Mark literal strings in __init / __exit code 2014-06-24 14:31 ` Rasmus Villemoes @ 2014-06-24 19:13 ` Mathias Krause 2014-06-24 19:37 ` Joe Perches 0 siblings, 1 reply; 26+ messages in thread From: Mathias Krause @ 2014-06-24 19:13 UTC (permalink / raw) To: Rasmus Villemoes, Joe Perches Cc: linux-kernel, Andrew Morton, Greg Kroah-Hartman, Steven Rostedt, Rafael J. Wysocki, Thomas Gleixner, Ingo Molnar, H. Peter Anvin 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC PATCH 0/3] Mark literal strings in __init / __exit code 2014-06-24 19:13 ` Mathias Krause @ 2014-06-24 19:37 ` Joe Perches 2014-06-24 20:10 ` Mathias Krause 0 siblings, 1 reply; 26+ messages in thread From: Joe Perches @ 2014-06-24 19:37 UTC (permalink / raw) To: Mathias Krause Cc: Rasmus Villemoes, linux-kernel, Andrew Morton, Greg Kroah-Hartman, Steven Rostedt, Rafael J. Wysocki, Thomas Gleixner, Ingo Molnar, H. Peter Anvin 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC PATCH 0/3] Mark literal strings in __init / __exit code 2014-06-24 19:37 ` Joe Perches @ 2014-06-24 20:10 ` Mathias Krause 2014-06-24 20:30 ` Joe Perches 0 siblings, 1 reply; 26+ messages in thread From: Mathias Krause @ 2014-06-24 20:10 UTC (permalink / raw) To: Joe Perches Cc: Rasmus Villemoes, linux-kernel, Andrew Morton, Greg Kroah-Hartman, Steven Rostedt, Rafael J. Wysocki, Thomas Gleixner, Ingo Molnar, H. Peter Anvin 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC PATCH 0/3] Mark literal strings in __init / __exit code 2014-06-24 20:10 ` Mathias Krause @ 2014-06-24 20:30 ` Joe Perches 2014-06-24 20:41 ` Mathias Krause 0 siblings, 1 reply; 26+ messages in thread From: Joe Perches @ 2014-06-24 20:30 UTC (permalink / raw) To: Mathias Krause Cc: Rasmus Villemoes, linux-kernel, Andrew Morton, Greg Kroah-Hartman, Steven Rostedt, Rafael J. Wysocki, Thomas Gleixner, Ingo Molnar, H. Peter Anvin 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC PATCH 0/3] Mark literal strings in __init / __exit code 2014-06-24 20:30 ` Joe Perches @ 2014-06-24 20:41 ` Mathias Krause 2014-06-24 20:57 ` Joe Perches 0 siblings, 1 reply; 26+ messages in thread From: Mathias Krause @ 2014-06-24 20:41 UTC (permalink / raw) To: Joe Perches Cc: Rasmus Villemoes, linux-kernel, Andrew Morton, Greg Kroah-Hartman, Steven Rostedt, Rafael J. Wysocki, Thomas Gleixner, Ingo Molnar, H. Peter Anvin 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC PATCH 0/3] Mark literal strings in __init / __exit code 2014-06-24 20:41 ` Mathias Krause @ 2014-06-24 20:57 ` Joe Perches 2014-06-24 21:06 ` Mathias Krause 0 siblings, 1 reply; 26+ messages in thread From: Joe Perches @ 2014-06-24 20:57 UTC (permalink / raw) To: Mathias Krause Cc: Rasmus Villemoes, linux-kernel, Andrew Morton, Greg Kroah-Hartman, Steven Rostedt, Rafael J. Wysocki, Thomas Gleixner, Ingo Molnar, H. Peter Anvin 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC PATCH 0/3] Mark literal strings in __init / __exit code 2014-06-24 20:57 ` Joe Perches @ 2014-06-24 21:06 ` Mathias Krause 2014-06-24 21:45 ` Joe Perches 0 siblings, 1 reply; 26+ messages in thread From: Mathias Krause @ 2014-06-24 21:06 UTC (permalink / raw) To: Joe Perches Cc: Rasmus Villemoes, linux-kernel, Andrew Morton, Greg Kroah-Hartman, Steven Rostedt, Rafael J. Wysocki, Thomas Gleixner, Ingo Molnar, H. Peter Anvin 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC PATCH 0/3] Mark literal strings in __init / __exit code 2014-06-24 21:06 ` Mathias Krause @ 2014-06-24 21:45 ` Joe Perches 2014-06-25 5:55 ` Mathias Krause 0 siblings, 1 reply; 26+ messages in thread From: Joe Perches @ 2014-06-24 21:45 UTC (permalink / raw) To: Mathias Krause Cc: Rasmus Villemoes, linux-kernel, Andrew Morton, Greg Kroah-Hartman, Steven Rostedt, Rafael J. Wysocki, Thomas Gleixner, Ingo Molnar, H. Peter Anvin 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC PATCH 0/3] Mark literal strings in __init / __exit code 2014-06-24 21:45 ` Joe Perches @ 2014-06-25 5:55 ` Mathias Krause 2014-06-25 7:35 ` Rasmus Villemoes 0 siblings, 1 reply; 26+ messages in thread From: Mathias Krause @ 2014-06-25 5:55 UTC (permalink / raw) To: Joe Perches Cc: Rasmus Villemoes, linux-kernel, Andrew Morton, Greg Kroah-Hartman, Steven Rostedt, Rafael J. Wysocki, Thomas Gleixner, Ingo Molnar, H. Peter Anvin 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC PATCH 0/3] Mark literal strings in __init / __exit code 2014-06-25 5:55 ` Mathias Krause @ 2014-06-25 7:35 ` Rasmus Villemoes 2014-06-25 7:48 ` Joe Perches 2014-06-25 8:17 ` Mathias Krause 0 siblings, 2 replies; 26+ messages in thread From: Rasmus Villemoes @ 2014-06-25 7:35 UTC (permalink / raw) To: Mathias Krause Cc: Joe Perches, linux-kernel, Andrew Morton, Greg Kroah-Hartman, Steven Rostedt, Rafael J. Wysocki, Thomas Gleixner, Ingo Molnar, H. Peter Anvin 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC PATCH 0/3] Mark literal strings in __init / __exit code 2014-06-25 7:35 ` Rasmus Villemoes @ 2014-06-25 7:48 ` Joe Perches 2014-06-25 8:34 ` Mathias Krause 2014-06-25 8:17 ` Mathias Krause 1 sibling, 1 reply; 26+ messages in thread From: Joe Perches @ 2014-06-25 7:48 UTC (permalink / raw) To: Rasmus Villemoes Cc: Mathias Krause, linux-kernel, Andrew Morton, Greg Kroah-Hartman, Steven Rostedt, Rafael J. Wysocki, Thomas Gleixner, Ingo Molnar, H. Peter Anvin 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC PATCH 0/3] Mark literal strings in __init / __exit code 2014-06-25 7:48 ` Joe Perches @ 2014-06-25 8:34 ` Mathias Krause 2014-06-25 11:22 ` Joe Perches 0 siblings, 1 reply; 26+ messages in thread From: Mathias Krause @ 2014-06-25 8:34 UTC (permalink / raw) To: Joe Perches Cc: Rasmus Villemoes, linux-kernel, Andrew Morton, Greg Kroah-Hartman, Steven Rostedt, Rafael J. Wysocki, Thomas Gleixner, Ingo Molnar, H. Peter Anvin 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC PATCH 0/3] Mark literal strings in __init / __exit code 2014-06-25 8:34 ` Mathias Krause @ 2014-06-25 11:22 ` Joe Perches 0 siblings, 0 replies; 26+ messages in thread From: Joe Perches @ 2014-06-25 11:22 UTC (permalink / raw) To: Mathias Krause Cc: Rasmus Villemoes, linux-kernel, Andrew Morton, Greg Kroah-Hartman, Steven Rostedt, Rafael J. Wysocki, Thomas Gleixner, Ingo Molnar, H. Peter Anvin 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC PATCH 0/3] Mark literal strings in __init / __exit code 2014-06-25 7:35 ` Rasmus Villemoes 2014-06-25 7:48 ` Joe Perches @ 2014-06-25 8:17 ` Mathias Krause 1 sibling, 0 replies; 26+ messages in thread From: Mathias Krause @ 2014-06-25 8:17 UTC (permalink / raw) To: Rasmus Villemoes Cc: Joe Perches, linux-kernel, Andrew Morton, Greg Kroah-Hartman, Steven Rostedt, Rafael J. Wysocki, Thomas Gleixner, Ingo Molnar, H. Peter Anvin 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC PATCH 0/3] Mark literal strings in __init / __exit code 2014-06-22 22:46 [RFC PATCH 0/3] Mark literal strings in __init / __exit code Mathias Krause ` (3 preceding siblings ...) 2014-06-22 22:56 ` [RFC PATCH 0/3] Mark literal strings in __init / __exit code Joe Perches @ 2014-06-23 1:30 ` Joe Perches 2014-06-23 6:29 ` Mathias Krause 4 siblings, 1 reply; 26+ messages in thread From: Joe Perches @ 2014-06-23 1:30 UTC (permalink / raw) To: Mathias Krause, David Daney Cc: linux-kernel, Andrew Morton, Greg Kroah-Hartman, Steven Rostedt, Rafael J. Wysocki, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Jason Baron 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC PATCH 0/3] Mark literal strings in __init / __exit code 2014-06-23 1:30 ` Joe Perches @ 2014-06-23 6:29 ` Mathias Krause 0 siblings, 0 replies; 26+ messages in thread From: Mathias Krause @ 2014-06-23 6:29 UTC (permalink / raw) To: Joe Perches Cc: David Daney, linux-kernel, Andrew Morton, Greg Kroah-Hartman, Steven Rostedt, Rafael J. Wysocki, Thomas Gleixner, Ingo Molnar, H. Peter Anvin 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2014-06-25 11:22 UTC | newest] Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-06-22 22:46 [RFC PATCH 0/3] Mark literal strings in __init / __exit code Mathias Krause 2014-06-22 22:46 ` [RFC PATCH 1/3] init.h: Add __init_str / __exit_str macros Mathias Krause 2014-06-24 19:43 ` Joe Perches 2014-06-24 20:13 ` Mathias Krause 2014-06-22 22:46 ` [RFC PATCH 2/3] printk: Provide pi_<level> / pe_<level> macros for __init / __exit code Mathias Krause 2014-06-22 22:46 ` [RFC PATCH 3/3] x86, acpi: Mark __init strings as such Mathias Krause 2014-06-22 22:56 ` [RFC PATCH 0/3] Mark literal strings in __init / __exit code Joe Perches 2014-06-23 6:23 ` Mathias Krause 2014-06-23 6:33 ` Joe Perches 2014-06-24 14:31 ` Rasmus Villemoes 2014-06-24 19:13 ` Mathias Krause 2014-06-24 19:37 ` Joe Perches 2014-06-24 20:10 ` Mathias Krause 2014-06-24 20:30 ` Joe Perches 2014-06-24 20:41 ` Mathias Krause 2014-06-24 20:57 ` Joe Perches 2014-06-24 21:06 ` Mathias Krause 2014-06-24 21:45 ` Joe Perches 2014-06-25 5:55 ` Mathias Krause 2014-06-25 7:35 ` Rasmus Villemoes 2014-06-25 7:48 ` Joe Perches 2014-06-25 8:34 ` Mathias Krause 2014-06-25 11:22 ` Joe Perches 2014-06-25 8:17 ` Mathias Krause 2014-06-23 1:30 ` Joe Perches 2014-06-23 6:29 ` Mathias Krause
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).