| Message ID | 1267924214.1937.12.camel@Joe-Laptop.home |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Sat, 6 Mar 2010, Joe Perches wrote: > > Reducing the size of struct printf_spec is 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 6 Mar 2010, Linus Torvalds wrote: > 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/lib/vsprintf.c b/lib/vsprintf.c index af4aaa6..fdee7f7 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -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);