Patchwork vsprintf.c: Reduce sizeof struct printf_spec from 24 to 8 bytes

login
register
mail settings
Submitter Joe Perches
Date March 7, 2010, 1:10 a.m.
Message ID <1267924214.1937.12.camel@Joe-Laptop.home>
Download mbox | patch
Permalink /patch/47063/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Joe Perches - March 7, 2010, 1:10 a.m.
On Sat, 2010-03-06 at 15:58 -0800, Linus Torvalds wrote:
> 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.

Signed-off-by: Joe Perches <joe@perches.com>
---
 lib/vsprintf.c |   22 ++++++++++++----------
 1 files changed, 12 insertions(+), 10 deletions(-)



--
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
Linus Torvalds - March 7, 2010, 2:03 a.m.
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
Linus Torvalds - March 7, 2010, 2:24 a.m.
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

Patch

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);