Make functions of dev_<level> macros, recursive vsnprintf
Subsystems (6)
cover Make functions of dev_<level> macros, recursive vsnprintf
dev_<level> macros use a lot of repetitive string space.
Eliminate the string prefixes and function arguments from all the macro uses and consolidate them in functions.
This patchset saves about 60K.
This implementation also adds the ability to use a struct va_format to emit a format string along with va_list arguments.
This %pV implementation should not be used without a wrapper that does printf argument verification like the dev_<level> functions.
Inspired a bit by Nick Andrew's patches and Linus' comments in December 2008
http://lkml.org/lkml/2008/12/6/15
http://lkml.org/lkml/2008/12/6/101
Joe Perches (2):
vsprintf: Recursive vsnprintf: Add "%pV", struct va_format device.h drivers/base/core.c Convert dev_<level> macros to functions
drivers/base/core.c | 124 ++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/device.h | 102 +++++++++++++++++++++++++++++---------- include/linux/kernel.h | 5 ++ lib/vsprintf.c | 9 ++++ 4 files changed, 214 insertions(+), 26 deletions(-)
PATCH 1/2 vsprintf: Recursive vsnprintf: Add "%pV", struct va_format
Add the ability to print a format and va_list from a structure pointer
Allows __dev_printk to be implemented as a single printk while minimizing string space duplication.
%pV should not be used without some mechanism to verify the format and argument use ala __attribute__(format (printf(...))).
--- include/linux/kernel.h | 5 +++++ lib/vsprintf.c | 9 +++++++++ 2 files changed, 14 insertions(+), 0 deletions(-)
include/linux/kernel.h +5 -0
@@ -169,6 +169,11 @@ static inline void might_fault(void) }#endif +struct va_format { +const char *fmt; +va_list *va; +}; +extern struct atomic_notifier_head panic_notifier_list;extern long (*panic_blink)(long time); NORET_TYPE void panic(const char * fmt, ...)
lib/vsprintf.c +9 -0
@@ -950,6 +950,11 @@ static char *uuid_string(char *buf, char *end, const u8 *addr,* [0][1][2][3]-[4][5]-[6][7]-[8][9]-[10][11][12][13][14][15] * little endian output byte order is: * [3][2][1][0]-[5][4]-[7][6]-[8][9]-[10][11][12][13][14][15] +* - 'V' For a struct va_format which contains a format string * and va_list *, +* call vsnprintf(->format, *->va_list). +* Implements a "recursive vsnprintf". +* Do not use this feature without some mechanism to verify the +* correctness of the format string and va_list arguments. * * Note: The difference between 'S' and 'F' is that on ia64 and ppc64 * function pointers are really function descriptors, which contain a
@@ -994,6 +999,10 @@ static char *pointer(const char *fmt, char *buf, char *end, void *ptr,break ;case 'U' :return uuid_string (buf, end, ptr, spec, fmt); +case 'V' : +return buf +vsnprintf (buf, end - buf, + ((struct va_format *)ptr)->fmt, + *(((struct va_format *)ptr)->va)); } spec.flags |=SMALL ;if (spec.field_width ==-1 ) {
Signature · 1 line
1.7.0.14.g7e948
PATCH 1/2 vsprintf: Recursive vsnprintf: Add "%pV", struct va_format
In reply to Joe PerchesAdd the ability to print a format and va_list from a structure pointer
Allows __dev_printk to be implemented as a single printk while minimizing string space duplication.
%pV should not be used without some mechanism to verify the format and argument use ala __attribute__(format (printf(...))).
--- include/linux/kernel.h | 5 +++++ lib/vsprintf.c | 9 +++++++++ 2 files changed, 14 insertions(+), 0 deletions(-)
include/linux/kernel.h +5 -0
@@ -169,6 +169,11 @@ static inline void might_fault(void) }#endif +struct va_format { +const char *fmt; +va_list *va; +}; +extern struct atomic_notifier_head panic_notifier_list;extern long (*panic_blink)(long time); NORET_TYPE void panic(const char * fmt, ...)
lib/vsprintf.c +9 -0
@@ -950,6 +950,11 @@ static char *uuid_string(char *buf, char *end, const u8 *addr,* [0][1][2][3]-[4][5]-[6][7]-[8][9]-[10][11][12][13][14][15] * little endian output byte order is: * [3][2][1][0]-[5][4]-[7][6]-[8][9]-[10][11][12][13][14][15] +* - 'V' For a struct va_format which contains a format string * and va_list *, +* call vsnprintf(->format, *->va_list). +* Implements a "recursive vsnprintf". +* Do not use this feature without some mechanism to verify the +* correctness of the format string and va_list arguments. * * Note: The difference between 'S' and 'F' is that on ia64 and ppc64 * function pointers are really function descriptors, which contain a
@@ -994,6 +999,10 @@ static char *pointer(const char *fmt, char *buf, char *end, void *ptr,break ;case 'U' :return uuid_string (buf, end, ptr, spec, fmt); +case 'V' : +return buf +vsnprintf (buf, end - buf, + ((struct va_format *)ptr)->fmt, + *(((struct va_format *)ptr)->va)); } spec.flags |=SMALL ;if (spec.field_width ==-1 ) {
Signature · 1 line
1.7.0.14.g7e948
PATCH 2/2 device.h drivers/base/core.c Convert dev_<level> macros to functions
In reply to Joe PerchesSave ~60k in a defconfig
Use %pV and struct va_format
Format arguments are verified before printk
--- drivers/base/core.c | 124 ++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/device.h | 102 +++++++++++++++++++++++++++++---------- 2 files changed, 200 insertions(+), 26 deletions(-)
drivers/base/core.c +124 -0
@@ -1745,3 +1745,127 @@ void device_shutdown(void) }async_synchronize_full (); } + +/* +* Device logging functions +*/ + +#ifdef CONFIG_PRINTK + +static int __dev_printk (const char *level,const struct device *dev, +const char *fmt,va_list args) +{ +struct va_format vaf; + + vaf.fmt = fmt; + vaf.va = &args; +return printk ("%s%s %s: %pV" , + level,dev_driver_string (dev),dev_name (dev), &vaf); +} + +int dev_printk (const char *level,const struct device *dev, +const char *fmt, ...) +{ +int r; +va_list args; + +va_start (args, fmt); + r =__dev_printk (level, dev, fmt, args); +va_end (args); + +return r; +} +EXPORT_SYMBOL (dev_printk); + +int dev_emerg (const struct device *dev,const char *fmt, ...) +{ +int r; +va_list args; + +va_start (args, fmt); + r =__dev_printk (KERN_EMERG , dev, fmt, args); +va_end (args); + +return r; +} +EXPORT_SYMBOL (dev_emerg); + +int dev_alert (const struct device *dev,const char *fmt, ...) +{ +int r; +va_list args; + +va_start (args, fmt); + r =__dev_printk (KERN_ALERT , dev, fmt, args); +va_end (args); + +return r; +} +EXPORT_SYMBOL (dev_alert); + +int dev_crit (const struct device *dev,const char *fmt, ...) +{ +int r; +va_list args; + +va_start (args, fmt); + r =__dev_printk (KERN_CRIT , dev, fmt, args); +va_end (args); + +return r; +} +EXPORT_SYMBOL (dev_crit); + +int dev_err (const struct device *dev,const char *fmt, ...) +{ +int r; +va_list args; + +va_start (args, fmt); + r =__dev_printk (KERN_ERR , dev, fmt, args); +va_end (args); + +return r; +} +EXPORT_SYMBOL (dev_err); + +int dev_warn (const struct device *dev,const char *fmt, ...) +{ +int r; +va_list args; + +va_start (args, fmt); + r =__dev_printk (KERN_WARNING , dev, fmt, args); +va_end (args); + +return r; +} +EXPORT_SYMBOL (dev_warn); + +int dev_notice (const struct device *dev,const char *fmt, ...) +{ +int r; +va_list args; + +va_start (args, fmt); + r =__dev_printk (KERN_NOTICE , dev, fmt, args); +va_end (args); + +return r; +} +EXPORT_SYMBOL (dev_notice); + +int dev_info (const struct device *dev,const char *fmt, ...) +{ +int r; +va_list args; + +va_start (args, fmt); + r =__dev_printk (KERN_INFO , dev, fmt, args); +va_end (args); + +return r; +} +EXPORT_SYMBOL (dev_info); + +#endif
include/linux/device.h +76 -26
119 lines not on this page · raw message
Signature · 1 line
1.7.0.14.g7e948
Andrew Morton
maintainerRe: [PATCH 2/2] device.h drivers/base/core.c Convert dev_<level> macros to functions
Quoted 4 lines · On Wed, 3 Mar 2010 22:27:23 -0800 Joe Perches <[email protected]> wrote:
Save ~60k in a defconfig
Use %pV and struct
va_format
Format arguments are verified before printk
Well that doesn't work very well.
drivers/net/pcmcia/pcnet_cs.c:117: error: 'dev_info' redeclared as different kind of symbol include/linux/device.h:645: error: previous declaration of 'dev_info' was here
there are lots of other dev_info's which will fail plus perhaps dev_err's, etc.
Andrew Morton
maintainerRe: [PATCH 2/2] device.h drivers/base/core.c Convert dev_<level> macros to functions
Quoted 7 lines · On Thu, 4 Mar 2010 16:56:09 -0800 Andrew Morton <[email protected]>…
Quoted 4 lines · On Wed, 3 Mar 2010 22:27:23 -0800 Joe Perches <[email protected]> wrote:
Save ~60k in a defconfig
Use %pV and struct
va_format
Format arguments are verified before printk
Quoted 8 lines
Well that doesn't work very well.
drivers/net/pcmcia/pcnet_cs.c:117: error: 'dev_info' redeclared as different kind of symbolinclude/linux/device.h:645: error: previous declaration of 'dev_info' was herethere are lots of other dev_info's which will fail plus perhaps dev_err's, etc.
btw, this may be kludgeable aroundable by doing
int _dev_info(const struct device *dev, const char *fmt, ...);
#define dev_info(...) _dev_info(...)
which will use the preprocessor's separation of foo' from foo()' to fix things up.
But it would be better to rename all those dev_info's to device_info or whatever, IMO.
Re: [PATCH 2/2] device.h drivers/base/core.c Convert dev_<level> macros to functions
Quoted 17 lines · Andrew Morton
Quoted 4 lines · On Thu, 4 Mar 2010 16:56:09 -0800 Andrew Morton <[email protected]>…
Quoted 3 lines · On Wed, 3 Mar 2010 22:27:23 -0800 Joe Perches <[email protected]> wrote:
Save ~60k in a defconfig
Use %pV and structva_format
Format arguments are verified before printkQuoted 8 lines
Well that doesn't work very well.
drivers/net/pcmcia/pcnet_cs.c:117: error: 'dev_info' redeclared as different kind of symbolinclude/linux/device.h:645: error: previous declaration of 'dev_info' was herethere are lots of other dev_info's which will fail plus perhaps dev_err's, etc.
btw, this may be kludgeable aroundable by doing
int
_dev_info(const struct device *dev, const char *fmt, ...);
Quoted 7 lines
#define
dev_info(...)_dev_info(...)which will use the preprocessor's separation of
foo' fromfoo()' to fix things up.But it would be better to rename all those dev_info's to
device_infoor whatever, IMO.
Perhaps using the function name _dev_info() and adding the macro above is an acceptable workaround until the dev_info struct names or local variable names could be renamed if necessary.
dev_err and dev_warn exist in source comments only and don't seem to be a problem.
David Miller? Do you have an opinion on the renaming of dev_info local variables and structs?
PATCH 2/2 device.h drivers/base/core.c Convert dev_<level> macros to functions
In reply to Joe PerchesSave ~60k in a defconfig
Use %pV and struct va_format
Format arguments are verified before printk
--- drivers/base/core.c | 124 ++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/device.h | 102 +++++++++++++++++++++++++++++---------- 2 files changed, 200 insertions(+), 26 deletions(-)
drivers/base/core.c +124 -0
@@ -1745,3 +1745,127 @@ void device_shutdown(void) }async_synchronize_full (); } + +/* +* Device logging functions +*/ + +#ifdef CONFIG_PRINTK + +static int __dev_printk (const char *level,const struct device *dev, +const char *fmt,va_list args) +{ +struct va_format vaf; + + vaf.fmt = fmt; + vaf.va = &args; +return printk ("%s%s %s: %pV" , + level,dev_driver_string (dev),dev_name (dev), &vaf); +} + +int dev_printk (const char *level,const struct device *dev, +const char *fmt, ...) +{ +int r; +va_list args; + +va_start (args, fmt); + r =__dev_printk (level, dev, fmt, args); +va_end (args); + +return r; +} +EXPORT_SYMBOL (dev_printk); + +int dev_emerg (const struct device *dev,const char *fmt, ...) +{ +int r; +va_list args; + +va_start (args, fmt); + r =__dev_printk (KERN_EMERG , dev, fmt, args); +va_end (args); + +return r; +} +EXPORT_SYMBOL (dev_emerg); + +int dev_alert (const struct device *dev,const char *fmt, ...) +{ +int r; +va_list args; + +va_start (args, fmt); + r =__dev_printk (KERN_ALERT , dev, fmt, args); +va_end (args); + +return r; +} +EXPORT_SYMBOL (dev_alert); + +int dev_crit (const struct device *dev,const char *fmt, ...) +{ +int r; +va_list args; + +va_start (args, fmt); + r =__dev_printk (KERN_CRIT , dev, fmt, args); +va_end (args); + +return r; +} +EXPORT_SYMBOL (dev_crit); + +int dev_err (const struct device *dev,const char *fmt, ...) +{ +int r; +va_list args; + +va_start (args, fmt); + r =__dev_printk (KERN_ERR , dev, fmt, args); +va_end (args); + +return r; +} +EXPORT_SYMBOL (dev_err); + +int dev_warn (const struct device *dev,const char *fmt, ...) +{ +int r; +va_list args; + +va_start (args, fmt); + r =__dev_printk (KERN_WARNING , dev, fmt, args); +va_end (args); + +return r; +} +EXPORT_SYMBOL (dev_warn); + +int dev_notice (const struct device *dev,const char *fmt, ...) +{ +int r; +va_list args; + +va_start (args, fmt); + r =__dev_printk (KERN_NOTICE , dev, fmt, args); +va_end (args); + +return r; +} +EXPORT_SYMBOL (dev_notice); + +int dev_info (const struct device *dev,const char *fmt, ...) +{ +int r; +va_list args; + +va_start (args, fmt); + r =__dev_printk (KERN_INFO , dev, fmt, args); +va_end (args); + +return r; +} +EXPORT_SYMBOL (dev_info); + +#endif
include/linux/device.h +76 -26
119 lines not on this page · raw message
Signature · 1 line
1.7.0.14.g7e948
Andrew Morton
maintainerQuoted 1 line · On Wed, 3 Mar 2010 22:27:21 -0800 Joe Perches <[email protected]> wrote:
(Typo'ed Linus' email address, added Greg KH and netdev)
What is the networking significance here?
Quoted 10 lines
dev_<level> macros use a lot of repetitive string space.
Eliminate the string prefixes and function arguments from all the macro uses and consolidate them in functions.
This patchset saves about 60K.
This implementation also adds the ability to use a struct
va_formatto emit a format string along withva_listarguments.
Quoted 7 lines
This %pV implementation should not be used without a wrapper that does printf argument verification like the dev_<level> functions.
Inspired a bit by Nick Andrew's patches and Linus' comments in December 2008
http://lkml.org/lkml/2008/12/6/15
http://lkml.org/lkml/2008/12/6/101
Looks like a reasonable approach, although I didn't check how much additional stack the recursion will take. Bear in mind that printk() can be called from super-deep contexts.
What would I need to do to make it recur more than once? Include a %pV in a string, like dev_printk("%s", %%pV")?
Quoted 3 lines · Andrew Morton
What would I need to do to make it recur more than once? Include a %pV in a string, like
dev_printk("%s", %%pV")?
I would argue that if somebody does that, they're just broken.
The single level of recursion is really nice - it solves a real annoyance. But if somebody starts trying to do multiple levels, that's just crazy.
Of course, I guess we don't relly have any way of sanely _counting_ the recursion level, so we'd have to just trust people to not do crazy things.
And that's a big thing to take on trust ;)
Linus
Quoted 6 lines · Linus Torvalds
Quoted 2 lines · Andrew Morton
What would I need to do to make it recur more than once? Include a %pV in a string, like
dev_printk("%s", %%pV")?I would argue that if somebody does that, they're just broken.
Quoted 7 lines
The single level of recursion is really nice - it solves a real annoyance. But if somebody starts trying to do multiple levels, that's just crazy.
Of course, I guess we don't relly have any way of sanely _counting_ the recursion level, so we'd have to just trust people to not do crazy things.
And that's a big thing to take on trust ;)
Is that an ack, a nack or a get the hell out?
Quoted 2 lines · Joe Perches
Is that an ack, a nack or a get the hell out?
It's mostly an Ack. I think the concept is great. I'd love to have some way to limit recursion, and I'd also love to see some actual numbers of how deep the vsnprintf stack frame is, but I don't see how to do the first, and I'm hoping the second isn't too horrible.
Linus
Quoted 4 lines · Linus Torvalds
Quoted 1 line · Joe Perches
Is that an ack, a nack or a get the hell out?
It's mostly an Ack. I think the concept is great. I'd love to have some way to limit recursion,
Maybe limit the %pV recursion depth to 1 with something like:
(in vsprintf.c: pointer() )
case 'V': {
char *rtn;
static int rcount++;
if (rcount > 1) { /* Don't recurse more than once */
rcount--;
break; /* Use print the pointer */
}
rtn = buf + vsnprintf(buf, end - buf,
((struct va_format *)ptr)->fmt,
*(((struct va_format *)ptr)->va));
rcount--;
return rtn;
}Quoted 3 lines
and I'd also love to see some actual numbers of
how deep the vsnprintf stack frame is, but I don't see how to do the first, and I'm hoping the second isn't too horrible.
I believe it's the arguments, a long long, a couple of pointers, and a struct printf_spec. Not too bad.
Quoted 3 lines · Joe Perches
Maybe limit the %pV recursion depth to 1 with something like:
(invsprintf.c:pointer())
Nope. Think about concurrent users.
The thing is, you have to hide it in local storage. We could make it thread-local (not cpu-local), but even that interacts badly with interrupts.
The only really workable approach would be to have a stack slot that is created by the externally visible routines (and initialized to zero), and those then passe the address of that as an argument to the lower levels, and then the recursion happens entirely within those lower level functions that update the value.
So it's doable, it's just not pretty.
Quoted 5 lines
Quoted 2 lines
and I'd also love to see some actual numbers of > > how deep the vsnprintf stack frame is, but I don't see how to do the first, and I'm hoping the second isn't too horrible.
I believe it's the arguments, a long long, a couple of pointers, and a struct
printf_spec. Not too bad.
I'm not convinced. We pass that 'printf_spec' around a lot, including nesting. Not as a pointer, either.
(Bjorn Helgaas has a patch that gets rid of _some_ of the stack usage, but not nearly all).
Linus
Quoted 3 lines · Linus Torvalds
I'm not convinced. We pass that '
printf_spec' around a lot, including nesting. Not as a pointer, either.
Btw, I wonder if those fields could become bitfields, or 'unsigned char' etc. That printf_spec really is unnecessarily big. It should _easily_ fit in a single register on a 64-bit platform, possibly even a 32-bit one (we could limit field width and precision to 5 bits, for example)
Linus
Quoted 10 lines · Linus Torvalds
Quoted 3 lines · Linus Torvalds
I'm not convinced. We pass that '
printf_spec' around a lot, including nesting. Not as a pointer, either.Btw, I wonder if those fields could become bitfields, or 'unsigned char' etc. That
printf_specreally is unnecessarily big. It should _easily_ fit in a single register on a 64-bit platform, possibly even a 32-bit one (we could limit field width and precision to 5 bits, for example)
I believe 32 bits isn't possible.
type:6
flags:8
width:8
base:6
precision:6
qualifier:8
So maybe something like this:
lib/vsprintf.c | 22 ++++++++++++---------- 1 files changed, 12 insertions(+), 10 deletions(-)
---
lib/vsprintf.c +12 -10
@@ -408,12 +408,12 @@ enum format_type { };struct printf_spec { -enum format_type type; -int flags;/* flags to number() */ -int field_width;/* width of output field */ -int base; -int precision;/* # of digits/chars */ -int qualifier; +u16 type; +s16 field_width;/* width of output field */ +u8 flags;/* flags to number() */ +u8 base; +s8 precision;/* # of digits/chars */ +u8 qualifier; }; static char *number(char *buf, char *end, unsigned long long num,
@@ -1333,7 +1333,7 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)break ;case FORMAT_TYPE_NRCHARS : { -int qualifier = spec.qualifier; +u8 qualifier = spec.qualifier;if (qualifier =='l' ) { long *ip = va_arg(args, long *);
@@ -1619,7 +1619,7 @@ do { \case FORMAT_TYPE_NRCHARS : {/* skip %n 's argument */ -int qualifier = spec.qualifier; +u8 qualifier = spec.qualifier;void *skip_arg;if (qualifier =='l' ) skip_arg = va_arg(args, long *);
@@ -1885,7 +1885,9 @@ int vsscanf(const char *buf, const char *fmt, va_list args)char *next;char digit;int num =0 ; -int qualifier, base, field_width; +u8 qualifier; +u8 base; +s16 field_width;bool is_sign;while (*fmt && *str) {
@@ -1963,7 +1965,7 @@ int vsscanf(const char *buf, const char *fmt, va_list args) { char *s = (char *)va_arg(args, char *);if (field_width ==-1 ) - field_width =INT_MAX ; + field_width =SHORT_MAX ;/* first, skip leading white space in buffer */ str =skip_spaces (str);
Quoted 9 lines · Joe Perches
I believe 32 bits isn't possible.
type:6
flags:8
width:8
base:6
precision:6
qualifier:8
type: 5
flags: 7
width: 5 (make it 6 bits and signed)
base: 5
precision: 5 (make it 6 bits, and signed)
qualifier: 3
Now, admittedly, right now 'qualifier' really has to be 8, but that's just because we use an ASCII character for it. But I don't think there are more than 8 actual legal qualifiers. And 'base' could be just 2 bits, since there are only a couple of legal bases, but I left room for them all anyway.
And the other really should fit in those numbers unchanged, I think.
Linus
Quoted 3 lines · Linus Torvalds
width: 5 (make it 6 bits and signed)
precision: 5 (make it 6 bits, and signed)
Oh, actually, no, we do that whole left-justification with the flags, we don't need that signed stuff. We do need one special value for the "no width, no precision", though.
Linus
Quoted 6 lines · Linus Torvalds
Quoted 2 lines · Linus Torvalds
width: 5 (make it 6 bits and signed)
precision: 5 (make it 6 bits, and signed)Oh, actually, no, we do that whole left-justification with the flags, we don't need that signed stuff. We do need one special value for the "no width, no precision", though.
What I posted is pretty simple.
I think the added complexity isn't worth it.
If you or anyone else does, all yours.
Quoted 10 lines · Linus Torvalds
Quoted 3 lines · Linus Torvalds
width: 5 (make it 6 bits and signed)
precision: 5 (make it 6 bits, and signed)Oh, actually, no, we do that whole left-justification with the flags, we don't need that signed stuff. We do need one special value for the "no width, no precision", though.
Btw, yeah, we do have a couple of "%.100s" etc, but we could fix those if we really wanted to do this.
And no, I guess we don't really need to make it 32-bit. Fitting in 64 bits would already be a big improvement over what we have now.
Linus
PATCH 1/1 vsprintf.c: Reduce sizeof struct printf_spec from 24 to 8 bytes
Quoted 2 lines · Linus Torvalds
And no, I guess we don't really need to make it 32-bit. Fitting in 64 bits would already be a big improvement over what we have now.
Reducing the size of struct printf_spec is a good thing because multiple instances are commonly passed on stack.
It's possible for type to be u8 and field_width to be s8, but this is likely small enough for now.
--- lib/vsprintf.c | 22 ++++++++++++---------- 1 files changed, 12 insertions(+), 10 deletions(-)
lib/vsprintf.c +12 -10
@@ -408,12 +408,12 @@ enum format_type { };struct printf_spec { -enum format_type type; -int flags;/* flags to number() */ -int field_width;/* width of output field */ -int base; -int precision;/* # of digits/chars */ -int qualifier; +u16 type; +s16 field_width;/* width of output field */ +u8 flags;/* flags to number() */ +u8 base; +s8 precision;/* # of digits/chars */ +u8 qualifier; }; static char *number(char *buf, char *end, unsigned long long num,
@@ -1333,7 +1333,7 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)break ;case FORMAT_TYPE_NRCHARS : { -int qualifier = spec.qualifier; +u8 qualifier = spec.qualifier;if (qualifier =='l' ) { long *ip = va_arg(args, long *);
@@ -1619,7 +1619,7 @@ do { \case FORMAT_TYPE_NRCHARS : {/* skip %n 's argument */ -int qualifier = spec.qualifier; +u8 qualifier = spec.qualifier;void *skip_arg;if (qualifier =='l' ) skip_arg = va_arg(args, long *);
@@ -1885,7 +1885,9 @@ int vsscanf(const char *buf, const char *fmt, va_list args)char *next;char digit;int num =0 ; -int qualifier, base, field_width; +u8 qualifier; +u8 base; +s16 field_width;bool is_sign;while (*fmt && *str) {
@@ -1963,7 +1965,7 @@ int vsscanf(const char *buf, const char *fmt, va_list args) { char *s = (char *)va_arg(args, char *);if (field_width ==-1 ) - field_width =INT_MAX ; + field_width =SHORT_MAX ;/* first, skip leading white space in buffer */ str =skip_spaces (str);
Re: [PATCH] vsprintf.c: Reduce sizeof struct printf_spec from 24 to 8 bytes
Quoted 3 lines · Joe Perches
Reducing the size of struct
printf_specis a good thing because multiple instances are commonly passed on stack.
Sadly, this is not enough.
the 'pointer()' function has a 200+ byte stack footprint on x86-64. And vsnprintf itself is about 100+ bytes. So that stack depth is way bigger than I would have expected.
I'm not sure _why_ that stack footprint for 'pointer()' is so big, but I bet it's due to some simple inlining, and gcc (once more) sucking at it and not being able to combine stack frames. It's a damn shame.
I suspect it's mac_address_string() having 20-odd bytes of temporary data, ip6_compressed_string() having some more, ip6_addr_string() having 50 bytes, uuid_string() adding thirty-odd bytes etc. Inline them all, suck up merging stack slots, and 200 bytes is easy.
So no, I don't think we can do the recursion as things stand. I've applied your cleanup patch, along with the two from Bjorn Helgaas (which he did for his pnp set), but they don't help this fundamental problem.
A few noinlines might be appropriate. As would a good gcc cluestick about inlining and stack usage. The latter is unlikely to materialize, I guess.
Linus
Re: [PATCH] vsprintf.c: Reduce sizeof struct printf_spec from 24 to 8 bytes
Quoted 7 lines · Linus Torvalds
the '
pointer()' function has a 200+ byte stack footprint on x86-64. And vsnprintf itself is about 100+ bytes. So that stack depth is way bigger than I would have expected.I'm not sure _why_ that stack footprint for '
pointer()' is so big, but I bet it's due to some simple inlining, and gcc (once more) sucking at it and not being able to combine stack frames. It's a damn shame.
Yeah, a few noinline's gets 'pointer()' to just save registers on the stack, no need for any extra buffers (which then is ok for your recursion case - the other subfunctions it can call have their own buffers, of course, but they won't be in the recursive call-path except at the leaf.
vsnprintf() itself seems less obviously fixable. I'm not sure wht gcc decides it needs 88 bytes of temp-space there.
Linus
PATCH 1/1 vsprintf.c: Use noinline_for_stack
In reply to Linus TorvaldsQuoted 1 line · Linus Torvalds
A few noinlines might be appropriate.
Mark static functions with noinline_for_stack
--- lib/vsprintf.c | 67 +++++++++++++++++++++++++++++++++++--------------------- 1 files changed, 42 insertions(+), 25 deletions(-)
lib/vsprintf.c +42 -25
@@ -266,7 +266,8 @@ int strict_strtoll(const char *cp, unsigned int base, long long *res) }EXPORT_SYMBOL (strict_strtoll); -static int skip_atoi (const char **s) +static noinline_for_stack +int skip_atoi(const char **s) {int i =0 ;
@@ -286,7 +287,8 @@ static int skip_atoi(const char **s)/* Formats correctly any integer in [0,99999]. * Outputs from one to five digits depending on input. * On i386 gcc 4.1.2 -O2: ~250 bytes of code. */ -static char *put_dec_trunc (char *buf,unsigned q) +static noinline_for_stack +char *put_dec_trunc(char *buf, unsigned q) {unsigned d3, d2, d1, d0; d1 = (q>>4 ) &0xf ;
@@ -323,7 +325,8 @@ static char *put_dec_trunc(char *buf, unsigned q)return buf; }/* Same with if's removed. Always emits five digits */ -static char *put_dec_full (char *buf,unsigned q) +static noinline_for_stack +char *put_dec_full(char *buf, unsigned q) {/* BTW, if q is in [0,9999], 8-bit ints will be enough, */ /* but anyway, gcc produces better code with full-sized ints */
@@ -365,7 +368,8 @@ static char *put_dec_full(char *buf, unsigned q)return buf; }/* No inlining helps gcc to use registers better */ -static noinline char *put_dec(char *buf, unsigned long long num) +static noinline_for_stack +char *put_dec(char *buf, unsigned long long num) {while (1 ) {unsigned rem;
@@ -416,8 +420,9 @@ struct printf_spec {u8 qualifier; }; -static char *number (char *buf,char *end,unsigned long long num, -struct printf_spec spec) +static noinline_for_stack +char *number(char *buf, char *end, unsigned long long num, +struct printf_spec spec) {/* we are called with base 8, 10 or 16, only, thus don't need "G..." */ static const char digits[16 ] ="0123456789ABCDEF" ;/* "GHIJKLMNOPQRSTUVWXYZ"; */
@@ -536,7 +541,8 @@ static char *number(char *buf, char *end, unsigned long long num,return buf; } -static char *string (char *buf,char *end,const char *s,struct printf_spec spec) +static noinline_for_stack +char *string(char *buf, char *end, const char *s, struct printf_spec spec) {int len, i;
@@ -566,8 +572,9 @@ static char *string(char *buf, char *end, const char *s, struct printf_spec specreturn buf; } -static char *symbol_string (char *buf,char *end,void *ptr, -struct printf_spec spec,char ext) +static noinline_for_stack +char *symbol_string(char *buf, char *end, void *ptr, +struct printf_spec spec,char ext) {unsigned long value = (unsigned long ) ptr;#ifdef CONFIG_KALLSYMS
@@ -587,8 +594,9 @@ static char *symbol_string(char *buf, char *end, void *ptr,#endif } -static char *resource_string (char *buf,char *end,struct resource *res, -struct printf_spec spec,const char *fmt) +static noinline_for_stack +char *resource_string(char *buf, char *end, struct resource *res, +struct printf_spec spec,const char *fmt) {#ifndef IO_RSRC_PRINTK_SIZE #define IO_RSRC_PRINTK_SIZE 6
@@ -678,8 +686,9 @@ static char *resource_string(char *buf, char *end, struct resource *res,return string (buf, end, sym, spec); } -static char *mac_address_string (char *buf,char *end,u8 *addr, -struct printf_spec spec,const char *fmt) +static noinline_for_stack +char *mac_address_string(char *buf, char *end, u8 *addr, +struct printf_spec spec,const char *fmt) {char mac_addr[sizeof ("xx:xx:xx:xx:xx:xx" )];char *p = mac_addr;
@@ -702,7 +711,8 @@ static char *mac_address_string(char *buf, char *end, u8 *addr,return string (buf, end, mac_addr, spec); } -static char *ip4_string (char *p,const u8 *addr,const char *fmt) +static noinline_for_stack +char *ip4_string(char *p, const u8 *addr, const char *fmt) {int i;bool leading_zeros = (fmt[0 ] =='i' );
@@ -751,7 +761,8 @@ static char *ip4_string(char *p, const u8 *addr, const char *fmt)return p; } -static char *ip6_compressed_string (char *p,const char *addr) +static noinline_for_stack +char *ip6_compressed_string(char *p, const char *addr) {int i, j, range;unsigned char zerolength[8 ];
@@ -831,7 +842,8 @@ static char *ip6_compressed_string(char *p, const char *addr)return p; } -static char *ip6_string (char *p,const char *addr,const char *fmt) +static noinline_for_stack +char *ip6_string(char *p, const char *addr, const char *fmt) {int i;
@@ -846,8 +858,9 @@ static char *ip6_string(char *p, const char *addr, const char *fmt)return p; } -static char *ip6_addr_string (char *buf,char *end,const u8 *addr, -struct printf_spec spec,const char *fmt) +static noinline_for_stack +char *ip6_addr_string(char *buf, char *end, const u8 *addr, +struct printf_spec spec,const char *fmt) {char ip6_addr[sizeof ("xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:255.255.255.255" )];
@@ -859,8 +872,9 @@ static char *ip6_addr_string(char *buf, char *end, const u8 *addr,return string (buf, end, ip6_addr, spec); } -static char *ip4_addr_string (char *buf,char *end,const u8 *addr, -struct printf_spec spec,const char *fmt) +static noinline_for_stack +char *ip4_addr_string(char *buf, char *end, const u8 *addr, +struct printf_spec spec,const char *fmt) {char ip4_addr[sizeof ("255.255.255.255" )];
@@ -869,8 +883,9 @@ static char *ip4_addr_string(char *buf, char *end, const u8 *addr,return string (buf, end, ip4_addr, spec); } -static char *uuid_string (char *buf,char *end,const u8 *addr, -struct printf_spec spec,const char *fmt) +static noinline_for_stack +char *uuid_string(char *buf, char *end, const u8 *addr, +struct printf_spec spec,const char *fmt) {char uuid[sizeof ("xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx" )];char *p = uuid;
@@ -958,8 +973,9 @@ static char *uuid_string(char *buf, char *end, const u8 *addr,* function pointers are really function descriptors, which contain a * pointer to the real address. */ -static char *pointer (const char *fmt,char *buf,char *end,void *ptr, -struct printf_spec spec) +static noinline_for_stack +char *pointer(const char *fmt, char *buf, char *end, void *ptr, +struct printf_spec spec) {if (!ptr)return string (buf, end,"(null)" , spec);
@@ -1028,7 +1044,8 @@ static char *pointer(const char *fmt, char *buf, char *end, void *ptr,* @precision: precision of a number * @qualifier: qualifier of a number (long, size_t, ...) */ -static int format_decode (const char *fmt,struct printf_spec *spec) +static noinline_for_stack +int format_decode(const char *fmt, struct printf_spec *spec) {const char *start = fmt;
Re: [PATCH] vsprintf.c: Use noinline_for_stack
Quoted 3 lines · Joe Perches
Quoted 1 line · Linus Torvalds
A few noinlines might be appropriate.
Mark static functions with
noinline_for_stack
It's fine by me that the vsnprintf recursion and (pr|dev|netdev)_<level> text reduction patches didn't make .34.
I'd like to know what you think necessary to get them into .35.
Perhaps it'd be useful if they could go into -next as-is for a bit of wider testing.
http://patchwork.kernel.org/patch/83940/
http://patchwork.kernel.org/patch/83724/
http://patchwork.kernel.org/patch/83726/
http://patchwork.kernel.org/patch/83725/
Andrew Morton
maintainerRe: [PATCH] vsprintf.c: Use noinline_for_stack
In reply to Joe PerchesQuoted 5 lines · On Sat, 06 Mar 2010 18:33:35 -0800 Joe Perches <[email protected]> wrote:
Quoted 1 line · Linus Torvalds
A few noinlines might be appropriate.
Mark static functions with
noinline_for_stack
-ENOTESTINGRESULTS.
Before:
akpm:/usr/src/25> objdump -d lib/vsprintf.o | perl scripts/checkstack.pl
0x00000e82 pointer [vsprintf.o]: 344 0x0000198c pointer [vsprintf.o]: 344 0x000025d6 scnprintf [vsprintf.o]: 216 0x00002648 scnprintf [vsprintf.o]: 216 0x00002565 snprintf [vsprintf.o]: 208 0x0000267c sprintf [vsprintf.o]: 208 0x000030a3 bprintf [vsprintf.o]: 208 0x00003b1e sscanf [vsprintf.o]: 208 0x00000608 number [vsprintf.o]: 136 0x00000937 number [vsprintf.o]: 136
After:
akpm:/usr/src/25> objdump -d lib/vsprintf.o | perl scripts/checkstack.pl
0x00000a7c symbol_string [vsprintf.o]: 248 0x00000ae8 symbol_string [vsprintf.o]: 248 0x00002310 scnprintf [vsprintf.o]: 216 0x00002382 scnprintf [vsprintf.o]: 216 0x0000229f snprintf [vsprintf.o]: 208 0x000023b6 sprintf [vsprintf.o]: 208 0x00002ddd bprintf [vsprintf.o]: 208 0x00003858 sscanf [vsprintf.o]: 208 0x00000625 number [vsprintf.o]: 136 0x00000954 number [vsprintf.o]: 136
nice.
Re: [PATCH] vsprintf.c: Use noinline_for_stack
Quoted 31 lines · Andrew Morton
-ENOTESTINGRESULTS.Before:
akpm:/usr/src/25> objdump -d
lib/vsprintf.o| perlscripts/checkstack.pl0x00000e82 pointer [vsprintf.o]: 344 0x0000198c pointer [vsprintf.o]: 344 0x000025d6 scnprintf [vsprintf.o]: 216 0x00002648 scnprintf [vsprintf.o]: 216 0x00002565 snprintf [vsprintf.o]: 208 0x0000267c sprintf [vsprintf.o]: 208 0x000030a3 bprintf [vsprintf.o]: 208 0x00003b1e sscanf [vsprintf.o]: 208 0x00000608 number [vsprintf.o]: 136 0x00000937 number [vsprintf.o]: 136After:
akpm:/usr/src/25> objdump -d
lib/vsprintf.o| perlscripts/checkstack.pl0x00000a7c symbol_string [vsprintf.o]: 248 0x00000ae8 symbol_string [vsprintf.o]: 248 0x00002310 scnprintf [vsprintf.o]: 216 0x00002382 scnprintf [vsprintf.o]: 216 0x0000229f snprintf [vsprintf.o]: 208 0x000023b6 sprintf [vsprintf.o]: 208 0x00002ddd bprintf [vsprintf.o]: 208 0x00003858 sscanf [vsprintf.o]: 208 0x00000625 number [vsprintf.o]: 136 0x00000954 number [vsprintf.o]: 136
Quoted 1 line
nice.
Note that the fact that the numbers are smaller is to some degree less important than _where_ the numbers are.
In the "before" side, it's the "pointer()" function that has a big stack depth. And the recursion that is going to happen is very much about vsnprintf -> pointer -> vsnprintf, so that is bad.
Now it's the new non-inlined leaf functions that still have a big stack footprint, and that's much better, because they wouldn't be part of any recursive behavior.
Not that I think it's wonderful even now. Especially that whole 'symbol_string()' thing is not only a big stack user, it ends up calling down a fair number of other functions. Non-recursively, but still.
That, in turn, is due to this:
- include/linux/kallsyms.h:
#define KSYM_NAME_LEN 128
#define KSYM_SYMBOL_LEN (sizeof("%s+%#lx/%#lx [%s]") + (KSYM_NAME_LEN - 1) + \ - symbol_string():
char sym[KSYM_SYMBOL_LEN];
ie we "need" about 150 bytes for just that silly symbol expansion (rounded up etc). Which is ridiculous, since we could/should limit it to something sane. But the kallsyms_lookup()/sprint_symbol() functions don't take a length parameter, so we have to do the worst-case thing (which itself has tons of unnecessary padding).
Gaah. We do _not_ want a kmalloc() or something like that in this path, since its' very much used for oopses (which in turn may be due to various slab bugs etc).
Linus
Re: [PATCH] vsprintf.c: Use noinline_for_stack
Quoted 5 lines · Linus Torvalds
Quoted 1 line · Andrew Morton
nice.
But the kallsyms_lookup()/sprint_symbol() functions don't take a length parameter, so we have to do the worst-case thing (which itself has tons of unnecessary padding).
I sent a patch once about that using a struct
because I didn't like the unbounded sprint
http://lkml.org/lkml/2009/4/15/16
Quoted 3 lines
Gaah. We do _not_ want a
kmalloc()or something like that in this path, since its' very much used for oopses (which in turn may be due to various slab bugs etc).
Perhaps a new snprint_symbol function with the
other kallsyms... functions changed as necessary.
thoughts?
PATCH 1/1 vsprintf.c: remove stack variable ksym from
Quoted 8 lines · Joe Perches
Quoted 5 lines · Linus Torvalds
Quoted 1 line · Andrew Morton
nice.
But the kallsyms_lookup()/sprint_symbol() functions don't take a length parameter, so we have to do the worst-case thing (which itself has tons of unnecessary padding).
Perhaps a new
snprint_symbolfunction with the
other kallsyms... functions changed as necessary.
Perhaps something like this:
Functions added:
kallsyms_lookup_n length limited kallsyms_lookupsnprint_symbol length limited sprint_symbolmodule_address_lookup_n length limited module_address_lookup
Changes to vsprintf.c, function symbol_string:
remove stack variable ksym, just use output buffer and length limited functions.
Compiled, untested
--- include/linux/kallsyms.h | 7 ++++ include/linux/module.h | 15 ++++++++ kernel/kallsyms.c | 91 ++++++++++++++++++++++++++++++++++++++++------ kernel/module.c | 33 +++++++++++++++++ lib/vsprintf.c | 14 ++++--- 5 files changed, 143 insertions(+), 17 deletions(-)
include/linux/kallsyms.h +7 -0
@@ -34,8 +34,15 @@ const char *kallsyms_lookup(unsigned long addr,unsigned long *offset, char **modname, char *namebuf); +const char *kallsyms_lookup_n (unsigned long addr, +unsigned long *symbolsize, +unsigned long *offset, +char **modname,char *namebuf,size_t size); +/* Look up a kernel symbol and return it in a text buffer. */ extern int sprint_symbol (char *buffer,unsigned long address); +/* Look up a kernel symbol and return it in a text buffer. */ +extern int snprint_symbol (char *buffer,size_t size,unsigned long address);/* Look up a kernel symbol and print it to the kernel messages. */ extern void __print_symbol (const char *fmt,unsigned long address);
include/linux/module.h +15 -0
@@ -520,6 +520,11 @@ const char *module_address_lookup(unsigned long addr,unsigned long *offset, char **modname, char *namebuf); +const char *module_address_lookup_n (unsigned long addr, +unsigned long *symbolsize, +unsigned long *offset, +char **modname, +char *namebuf,size_t size);int lookup_module_symbol_name (unsigned long addr,char *symname);int lookup_module_symbol_attrs (unsigned long addr,unsigned long *size,unsigned long *offset,char *modname,char *name);
@@ -598,6 +603,16 @@ static inline const char *module_address_lookup(unsigned long addr,return NULL ; } +/* For kallsyms to ask for address resolution. NULL means not found. */ +static inline const char *module_address_lookup_n (unsigned long addr, +unsigned long *symbolsize, +unsigned long *offset, +char **modname, +char *namebuf,size_t size) +{ +return NULL ; +} +static inline int lookup_module_symbol_name (unsigned long addr,char *symname) {return -ERANGE ;
kernel/kallsyms.c +80 -11
159 lines not on this page · raw message
kernel/module.c +33 -0
@@ -2607,6 +2607,39 @@ const char *module_address_lookup(unsigned long addr,return ret; } +/* For kallsyms to ask for address resolution. NULL means not found. Careful +* not to lock to avoid deadlock on oopses, simply disable preemption. */ +const char *module_address_lookup_n (unsigned long addr, +unsigned long *ksize, +unsigned long *offset, +char **modname, +char *namebuf,size_t size) +{ +struct module *mod; +const char *ret =NULL ; + +preempt_disable (); +list_for_each_entry_rcu (mod, &modules, list) { +if (within_module_init (addr, mod) || +within_module_core (addr, mod)) { +if (modname) + *modname = mod->name; + ret =get_ksymbol (mod, addr, ksize, offset); +break ; + } + } +/* Make a copy in here where it's safe */ +if (ret) { +if (size) { +strncpy (namebuf, ret, size -1 ); + namebuf[size -1 ] =0 ; + } + ret = namebuf; + } +preempt_enable (); +return ret; +} +int lookup_module_symbol_name (unsigned long addr,char *symname) {struct module *mod;
lib/vsprintf.c +8 -6
@@ -571,13 +571,15 @@ static char *symbol_string(char *buf, char *end, void *ptr, {unsigned long value = (unsigned long ) ptr;#ifdef CONFIG_KALLSYMS -char sym[KSYM_SYMBOL_LEN ]; -if (ext !='f' && ext !='s' ) -sprint_symbol (sym, value); -else -kallsyms_lookup (value,NULL ,NULL ,NULL , sym); +/* Cap ending position to spec.field_width if specified */ +if (spec.field_width >0 && (end - buf) > spec.field_width) + end = buf + spec.field_width; -return string (buf, end, sym, spec); +if (ext !='f' && ext !='s' ) +return buf +snprint_symbol (buf, end - buf, value); + +kallsyms_lookup_n (value,NULL ,NULL ,NULL , buf, end - buf); +return buf +strlen (buf);#else spec.field_width =2 *sizeof (void *); spec.flags |=SPECIAL |SMALL |ZEROPAD ;
Re: [PATCH] vsprintf.c: remove stack variable ksym from
Quoted 11 lines · Joe Perches
Quoted 8 lines · Joe Perches
Quoted 5 lines · Linus Torvalds
Quoted 1 line · Andrew Morton
nice.
But the kallsyms_lookup()/sprint_symbol() functions don't take a length parameter, so we have to do the worst-case thing (which itself has tons of unnecessary padding).
Perhaps a new
snprint_symbolfunction with the
other kallsyms... functions changed as necessary.Perhaps something like this:
Just one minor nit:
[...]
kernel/kallsyms.c static unsigned int kallsyms_expand_symbol(unsigned int off, char *result) · line 126
Quoted 4 lines
kernel/kallsyms.c +2
-1- *result ='\0' ; +if (size) + *result ='\0' ;
This test seems to be here to handle the "size == 0" case, but
kernel/kallsyms.c const char *kallsyms_lookup(unsigned long addr, · line 325
Quoted 9 lines
[...]
kernel/kallsyms.c +8
-0+const char *kallsyms_lookup_n (unsigned long addr, +unsigned long *symbolsize, +unsigned long *offset, +char **modname,char *namebuf,size_t size) +{ +if (size) + namebuf[size -1 ] =0 ; + namebuf[0 ] =0 ;
here we seem to write the namebuf[0] even if "size == 0". So maybe both assignments in this function should be inside the "if (size)" test.
Other than that, the patch looks good:
Signature · 4 lines
Paulo Marques - www.grupopie.com "As far as we know, our computer has never had an undetected error." Weisert