[As gcc seems unable to help us out selecting the appropriate data segment
for the code, how about we did something like this?]
When using printk from __init functions it would be desirable to place
the printk format strings in __initdata. Add a printk_init() variant
which does this.
This printk_init() is necessarily a #define so that we can declare the
format string in static scope and mark it __initdata. We then call a
newly introduced __printk_init() variant which is identicle to printk() but
marked __init itself. By ensuring that an __init variant of printk is used
we get proper section violation warnings when this is used incorrectly:
WARNING: vmlinux.o(.text+0x3): Section mismatch in reference from the
function something() to the variable .init.data:__printk_init_fmt.31426
The function something() references
the variable __initdata __printk_init_fmt.31426.
This is often because something lacks a __initdata
annotation or the annotation of __printk_init_fmt.31426 is wrong.
Note I have followed printk's pattern for __cold annotations.
Signed-off-by: Andy Whitcroft <[email protected]>
---
include/linux/kernel.h | 10 ++++++++++
kernel/printk.c | 12 ++++++++++++
2 files changed, 22 insertions(+), 0 deletions(-)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 792bf0a..7754196 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -180,6 +180,13 @@ struct pid;
extern struct pid *session_of_pgrp(struct pid *pgrp);
#ifdef CONFIG_PRINTK
+#define printk_init(fmt, args...) \
+do { \
+ static char __printk_init_fmt[] __initdata = fmt; \
+ __printk_init(__printk_init_fmt, ##args); \
+} while (0)
+asmlinkage int __printk_init(const char * fmt, ...)
+ __attribute__ ((format (printf, 1, 2))) __cold;
asmlinkage int vprintk(const char *fmt, va_list args)
__attribute__ ((format (printf, 1, 0)));
asmlinkage int printk(const char * fmt, ...)
@@ -196,6 +203,9 @@ extern int __printk_ratelimit(int ratelimit_jiffies, int ratelimit_burst);
extern bool printk_timed_ratelimit(unsigned long *caller_jiffies,
unsigned int interval_msec);
#else
+asmlinkage int printk_init(const char * fmt, ...)
+ __attribute__ ((format (printf, 1, 2))) __cold;
+static inline int __cold printk_init(const char *s, ...) { return 0; }
static inline int vprintk(const char *s, va_list args)
__attribute__ ((format (printf, 1, 0)));
static inline int vprintk(const char *s, va_list args) { return 0; }
diff --git a/kernel/printk.c b/kernel/printk.c
index 8fb01c3..992a5c0 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -616,6 +616,18 @@ asmlinkage int printk(const char *fmt, ...)
return r;
}
+asmlinkage __init int __printk_init(const char *fmt, ...)
+{
+ va_list args;
+ int r;
+
+ va_start(args, fmt);
+ r = vprintk(fmt, args);
+ va_end(args);
+
+ return r;
+}
+
/* cpu currently holding logbuf_lock */
static volatile unsigned int printk_cpu = UINT_MAX;
Hi,
Andy Whitcroft <[email protected]> writes:
> [As gcc seems unable to help us out selecting the appropriate data segment
> for the code, how about we did something like this?]
>
> When using printk from __init functions it would be desirable to place
> the printk format strings in __initdata. Add a printk_init() variant
> which does this.
http://lkml.org/lkml/2008/2/4/185
I don't know if `desirable' is the technical argument Sam wants to hear :)
Hannes
> +asmlinkage __init int __printk_init(const char *fmt, ...)
> +{
> + va_list args;
> + int r;
> +
> + va_start(args, fmt);
> + r = vprintk(fmt, args);
> + va_end(args);
> +
> + return r;
> +}
> +
Wouldn't you have to export that?
johannes
On Tue, 3 Jun 2008 10:27:32 +0100 Andy Whitcroft <[email protected]> wrote:
>
> [As gcc seems unable to help us out selecting the appropriate data segment
> for the code, how about we did something like this?]
>
> When using printk from __init functions it would be desirable to place
> the printk format strings in __initdata. Add a printk_init() variant
> which does this.
>
> This printk_init() is necessarily a #define so that we can declare the
> format string in static scope and mark it __initdata. We then call a
> newly introduced __printk_init() variant which is identicle to printk() but
> marked __init itself. By ensuring that an __init variant of printk is used
> we get proper section violation warnings when this is used incorrectly:
>
> WARNING: vmlinux.o(.text+0x3): Section mismatch in reference from the
> function something() to the variable .init.data:__printk_init_fmt.31426
> The function something() references
> the variable __initdata __printk_init_fmt.31426.
> This is often because something lacks a __initdata
> annotation or the annotation of __printk_init_fmt.31426 is wrong.
>
> Note I have followed printk's pattern for __cold annotations.
>
Ho hum. This give everyone another way in which to bury everyone else
with patches.
Wouldn't it be great if checkpatch were to detect
fail-to-use-printk_init() in an __init function?
oh, speaking of checkpatch: please use it :)
> ---
> include/linux/kernel.h | 10 ++++++++++
> kernel/printk.c | 12 ++++++++++++
> 2 files changed, 22 insertions(+), 0 deletions(-)
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 792bf0a..7754196 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -180,6 +180,13 @@ struct pid;
> extern struct pid *session_of_pgrp(struct pid *pgrp);
>
> #ifdef CONFIG_PRINTK
> +#define printk_init(fmt, args...) \
> +do { \
> + static char __printk_init_fmt[] __initdata = fmt; \
> + __printk_init(__printk_init_fmt, ##args); \
> +} while (0)
> +asmlinkage int __printk_init(const char * fmt, ...)
> + __attribute__ ((format (printf, 1, 2))) __cold;
> asmlinkage int vprintk(const char *fmt, va_list args)
> __attribute__ ((format (printf, 1, 0)));
> asmlinkage int printk(const char * fmt, ...)
> @@ -196,6 +203,9 @@ extern int __printk_ratelimit(int ratelimit_jiffies, int ratelimit_burst);
> extern bool printk_timed_ratelimit(unsigned long *caller_jiffies,
> unsigned int interval_msec);
> #else
> +asmlinkage int printk_init(const char * fmt, ...)
> + __attribute__ ((format (printf, 1, 2))) __cold;
> +static inline int __cold printk_init(const char *s, ...) { return 0; }
> static inline int vprintk(const char *s, va_list args)
> __attribute__ ((format (printf, 1, 0)));
> static inline int vprintk(const char *s, va_list args) { return 0; }
> diff --git a/kernel/printk.c b/kernel/printk.c
> index 8fb01c3..992a5c0 100644
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -616,6 +616,18 @@ asmlinkage int printk(const char *fmt, ...)
> return r;
> }
>
> +asmlinkage __init int __printk_init(const char *fmt, ...)
> +{
> + va_list args;
> + int r;
> +
> + va_start(args, fmt);
> + r = vprintk(fmt, args);
> + va_end(args);
> +
> + return r;
> +}
We're going to want to be able to call printk_init() from modules.
Please fix and test that, if we decide to proceed.
Oh, and we're going to need printk_meminit() and printk_cpuinit() and
whatever.
Which probably means that __printk_init() can't be __init, unless all
the CONFIG_ settings which control __cpuinit, __meminit etc are blowing
in the right direction.
It would be good if we could get some idea of the savings here, because
boy this is going to be a pain.
It is important about embedded system, too.
So I add CC, linux-embedded.
On Wed, Jun 4, 2008 at 5:16 PM, Andrew Morton <[email protected]> wrote:
> On Tue, 3 Jun 2008 10:27:32 +0100 Andy Whitcroft <[email protected]> wrote:
>
>>
>> [As gcc seems unable to help us out selecting the appropriate data segment
>> for the code, how about we did something like this?]
>>
>> When using printk from __init functions it would be desirable to place
>> the printk format strings in __initdata. Add a printk_init() variant
>> which does this.
>>
>> This printk_init() is necessarily a #define so that we can declare the
>> format string in static scope and mark it __initdata. We then call a
>> newly introduced __printk_init() variant which is identicle to printk() but
>> marked __init itself. By ensuring that an __init variant of printk is used
>> we get proper section violation warnings when this is used incorrectly:
>>
>> WARNING: vmlinux.o(.text+0x3): Section mismatch in reference from the
>> function something() to the variable .init.data:__printk_init_fmt.31426
>> The function something() references
>> the variable __initdata __printk_init_fmt.31426.
>> This is often because something lacks a __initdata
>> annotation or the annotation of __printk_init_fmt.31426 is wrong.
>>
>> Note I have followed printk's pattern for __cold annotations.
>>
>
> Ho hum. This give everyone another way in which to bury everyone else
> with patches.
>
> Wouldn't it be great if checkpatch were to detect
> fail-to-use-printk_init() in an __init function?
>
> oh, speaking of checkpatch: please use it :)
>
>> ---
>> include/linux/kernel.h | 10 ++++++++++
>> kernel/printk.c | 12 ++++++++++++
>> 2 files changed, 22 insertions(+), 0 deletions(-)
>> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
>> index 792bf0a..7754196 100644
>> --- a/include/linux/kernel.h
>> +++ b/include/linux/kernel.h
>> @@ -180,6 +180,13 @@ struct pid;
>> extern struct pid *session_of_pgrp(struct pid *pgrp);
>>
>> #ifdef CONFIG_PRINTK
>> +#define printk_init(fmt, args...) \
>> +do { \
>> + static char __printk_init_fmt[] __initdata = fmt; \
>> + __printk_init(__printk_init_fmt, ##args); \
>> +} while (0)
>> +asmlinkage int __printk_init(const char * fmt, ...)
>> + __attribute__ ((format (printf, 1, 2))) __cold;
>> asmlinkage int vprintk(const char *fmt, va_list args)
>> __attribute__ ((format (printf, 1, 0)));
>> asmlinkage int printk(const char * fmt, ...)
>> @@ -196,6 +203,9 @@ extern int __printk_ratelimit(int ratelimit_jiffies, int ratelimit_burst);
>> extern bool printk_timed_ratelimit(unsigned long *caller_jiffies,
>> unsigned int interval_msec);
>> #else
>> +asmlinkage int printk_init(const char * fmt, ...)
>> + __attribute__ ((format (printf, 1, 2))) __cold;
>> +static inline int __cold printk_init(const char *s, ...) { return 0; }
>> static inline int vprintk(const char *s, va_list args)
>> __attribute__ ((format (printf, 1, 0)));
>> static inline int vprintk(const char *s, va_list args) { return 0; }
>> diff --git a/kernel/printk.c b/kernel/printk.c
>> index 8fb01c3..992a5c0 100644
>> --- a/kernel/printk.c
>> +++ b/kernel/printk.c
>> @@ -616,6 +616,18 @@ asmlinkage int printk(const char *fmt, ...)
>> return r;
>> }
>>
>> +asmlinkage __init int __printk_init(const char *fmt, ...)
>> +{
>> + va_list args;
>> + int r;
>> +
>> + va_start(args, fmt);
>> + r = vprintk(fmt, args);
>> + va_end(args);
>> +
>> + return r;
>> +}
>
> We're going to want to be able to call printk_init() from modules.
> Please fix and test that, if we decide to proceed.
>
> Oh, and we're going to need printk_meminit() and printk_cpuinit() and
> whatever.
>
> Which probably means that __printk_init() can't be __init, unless all
> the CONFIG_ settings which control __cpuinit, __meminit etc are blowing
> in the right direction.
>
> It would be good if we could get some idea of the savings here, because
> boy this is going to be a pain.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
--
Kinds regards,
MinChan Kim
On Tue, 2008-06-03 at 10:27 +0100, Andy Whitcroft wrote:
> +#define printk_init(fmt, args...) \
> +do { \
> + static char __printk_init_fmt[] __initdata = fmt; \
> + __printk_init(__printk_init_fmt, ##args); \
> +} while (0)
Hm, do these strings still get merged? Perhaps we want them in
a .initdata.str section on their own?
Bonus points for letting the linker 'merge' such strings into the
real .rodata.str section if they exist there too.
--
dwmw2
On Wed, 2008-06-04 at 01:16 -0700, Andrew Morton wrote:
> We're going to want to be able to call printk_init() from modules.
> Please fix and test that, if we decide to proceed.
Can we fix that by making it an alias for printk in the module case?
The only reason we need it to be __init is so that we get the section
warnings when you use it from non-init code, right? Won't we get the
warning when non-init code refers to the string in initdata anyway?
--
dwmw2
On Wed, 2008-06-04 at 09:59 +0100, David Woodhouse wrote:
> On Wed, 2008-06-04 at 01:16 -0700, Andrew Morton wrote:
> > We're going to want to be able to call printk_init() from modules.
> > Please fix and test that, if we decide to proceed.
>
> Can we fix that by making it an alias for printk in the module case?
>
> The only reason we need it to be __init is so that we get the section
> warnings when you use it from non-init code, right? Won't we get the
> warning when non-init code refers to the string in initdata anyway?
In fact, wasn't the warning Andy showed such a warning?
> WARNING: vmlinux.o(.text+0x3): Section mismatch in reference from the
> function something() to the variable .init.data:__printk_init_fmt.31426
> The function something() references
> the variable __initdata __printk_init_fmt.31426.
> This is often because something lacks a __initdata
> annotation or the annotation of __printk_init_fmt.31426 is wrong.
johannes
On Wed, 2008-06-04 at 11:10 +0200, Johannes Berg wrote:
> On Wed, 2008-06-04 at 09:59 +0100, David Woodhouse wrote:
> > On Wed, 2008-06-04 at 01:16 -0700, Andrew Morton wrote:
> > > We're going to want to be able to call printk_init() from modules.
> > > Please fix and test that, if we decide to proceed.
> >
> > Can we fix that by making it an alias for printk in the module case?
> >
> > The only reason we need it to be __init is so that we get the section
> > warnings when you use it from non-init code, right? Won't we get the
> > warning when non-init code refers to the string in initdata anyway?
>
> In fact, wasn't the warning Andy showed such a warning?
Hm, yes it was. Why do we need __printk_init() to be anything other than
an alias for printk, then?
--
dwmw2
On Wed, 2008-06-04 at 10:17 +0100, David Woodhouse wrote:
> On Wed, 2008-06-04 at 11:10 +0200, Johannes Berg wrote:
> > On Wed, 2008-06-04 at 09:59 +0100, David Woodhouse wrote:
> > > On Wed, 2008-06-04 at 01:16 -0700, Andrew Morton wrote:
> > > > We're going to want to be able to call printk_init() from modules.
> > > > Please fix and test that, if we decide to proceed.
> > >
> > > Can we fix that by making it an alias for printk in the module case?
> > >
> > > The only reason we need it to be __init is so that we get the section
> > > warnings when you use it from non-init code, right? Won't we get the
> > > warning when non-init code refers to the string in initdata anyway?
> >
> > In fact, wasn't the warning Andy showed such a warning?
>
> Hm, yes it was. Why do we need __printk_init() to be anything other than
> an alias for printk, then?
It looks to me like we wouldn't need it at all, just the printk_init()
macro that puts the format string into the right section.
johannes
> > When using printk from __init functions it would be desirable to place
> > the printk format strings in __initdata. Add a printk_init() variant
> > which does this.
Russell played with this years ago I seem to remember and broke the ARM
gcc in the process. If we have any compilers generating near pointer
references for string constants then suddenely hiding them in another
section is going to be interesting.
> Wouldn't it be great if checkpatch were to detect
> fail-to-use-printk_init() in an __init function?
Can we have a 'correctpatch' to go with it someday perhaps ;)