reduce stack usage of node_read_meminfo()

PATCH In review KOSAKI Motohiro 2 participants 5 messages Jul 8, 2010 – Jul 9, 2010

Subsystems (2)

Memory management Driver core

lore

Share & tools

lore

KOSAKI Motohiro

author

2010-07-08 09:20 UTC #0 lore raw

PATCH 1/1 reduce stack usage of node_read_meminfo()

Now, cmpilation node_read_meminfo() output following warning. Because it has very large sprintf() argument.

drivers/base/node.c: In function 'node_read_meminfo':
drivers/base/node.c:139: warning: the frame size of 848 bytes is larger than 512 bytes

This patch fixes it by splitting sprintf() in three parts. It doesn't have functional change.

Signed-off-by KOSAKI Motohiro <[email protected]>

---
 drivers/base/node.c |   46 +++++++++++++++++++++++-----------------------
 1 files changed, 23 insertions(+), 23 deletions(-)

drivers/base/node.c +23 ~~-23~~

index 2bdd8a9..a0fb2ef 100644
@@ -66,8 +66,7 @@ static ssize_t node_read_meminfo(struct sys_device * dev,
 	struct sysinfo i;
 
 	si_meminfo_node(&i, nid);
-
-	n = sprintf(buf, "\n"
+	n = sprintf(buf,
 		       "Node %d MemTotal:       %8lu kB\n"
 		       "Node %d MemFree:        %8lu kB\n"
 		       "Node %d MemUsed:        %8lu kB\n"

@@ -78,13 +77,33 @@ static ssize_t node_read_meminfo(struct sys_device * dev,
 		       "Node %d Active(file):   %8lu kB\n"
 		       "Node %d Inactive(file): %8lu kB\n"
 		       "Node %d Unevictable:    %8lu kB\n"
-		       "Node %d Mlocked:        %8lu kB\n"
+		       "Node %d Mlocked:        %8lu kB\n",
+		       nid, K(i.totalram),
+		       nid, K(i.freeram),
+		       nid, K(i.totalram - i.freeram),
+		       nid, K(node_page_state(nid, NR_ACTIVE_ANON) +
+				node_page_state(nid, NR_ACTIVE_FILE)),
+		       nid, K(node_page_state(nid, NR_INACTIVE_ANON) +
+				node_page_state(nid, NR_INACTIVE_FILE)),
+		       nid, K(node_page_state(nid, NR_ACTIVE_ANON)),
+		       nid, K(node_page_state(nid, NR_INACTIVE_ANON)),
+		       nid, K(node_page_state(nid, NR_ACTIVE_FILE)),
+		       nid, K(node_page_state(nid, NR_INACTIVE_FILE)),
+		       nid, K(node_page_state(nid, NR_UNEVICTABLE)),
+		       nid, K(node_page_state(nid, NR_MLOCK)));
+
 #ifdef CONFIG_HIGHMEM
+	n += sprintf(buf,
 		       "Node %d HighTotal:      %8lu kB\n"
 		       "Node %d HighFree:       %8lu kB\n"
 		       "Node %d LowTotal:       %8lu kB\n"
-		       "Node %d LowFree:        %8lu kB\n"
+		       "Node %d LowFree:        %8lu kB\n",
+		       nid, K(i.totalhigh),
+		       nid, K(i.freehigh),
+		       nid, K(i.totalram - i.totalhigh),
+		       nid, K(i.freeram - i.freehigh));
 #endif
+	n += sprintf(buf,
 		       "Node %d Dirty:          %8lu kB\n"
 		       "Node %d Writeback:      %8lu kB\n"
 		       "Node %d FilePages:      %8lu kB\n"

@@ -99,25 +118,6 @@ static ssize_t node_read_meminfo(struct sys_device * dev,
 		       "Node %d Slab:           %8lu kB\n"
 		       "Node %d SReclaimable:   %8lu kB\n"
 		       "Node %d SUnreclaim:     %8lu kB\n",
-		       nid, K(i.totalram),
-		       nid, K(i.freeram),
-		       nid, K(i.totalram - i.freeram),
-		       nid, K(node_page_state(nid, NR_ACTIVE_ANON) +
-				node_page_state(nid, NR_ACTIVE_FILE)),
-		       nid, K(node_page_state(nid, NR_INACTIVE_ANON) +
-				node_page_state(nid, NR_INACTIVE_FILE)),
-		       nid, K(node_page_state(nid, NR_ACTIVE_ANON)),
-		       nid, K(node_page_state(nid, NR_INACTIVE_ANON)),
-		       nid, K(node_page_state(nid, NR_ACTIVE_FILE)),
-		       nid, K(node_page_state(nid, NR_INACTIVE_FILE)),
-		       nid, K(node_page_state(nid, NR_UNEVICTABLE)),
-		       nid, K(node_page_state(nid, NR_MLOCK)),
-#ifdef CONFIG_HIGHMEM
-		       nid, K(i.totalhigh),
-		       nid, K(i.freehigh),
-		       nid, K(i.totalram - i.totalhigh),
-		       nid, K(i.freeram - i.freehigh),
-#endif
 		       nid, K(node_page_state(nid, NR_FILE_DIRTY)),
 		       nid, K(node_page_state(nid, NR_WRITEBACK)),
 		       nid, K(node_page_state(nid, NR_FILE_PAGES)),

Signature · 1 line

1.6.5.2

KOSAKI Motohiro

author

2010-07-08 10:42 UTC #1 lore raw

Grr, I did sent old ver. right patch is here ;-)
sorry.

================================================
Subject: [PATCH] reduce stack usage of node_read_meminfo()

Now, cmpilation node_read_meminfo() output following warning. Because it has very large sprintf() argument.

drivers/base/node.c: In function 'node_read_meminfo':
drivers/base/node.c:139: warning: the frame size of 848 bytes is larger than 512 bytes

This patch fixes it by splitting sprintf() in three parts. This also reduce CONFIG_HIGHMEM mess a bit.

Signed-off-by KOSAKI Motohiro <[email protected]>

---
 drivers/base/node.c |   46 +++++++++++++++++++++++-----------------------
 1 files changed, 23 insertions(+), 23 deletions(-)

drivers/base/node.c +23 ~~-23~~

index 2bdd8a9..2872e86 100644
@@ -66,8 +66,7 @@ static ssize_t node_read_meminfo(struct sys_device * dev,
 	struct sysinfo i;
 
 	si_meminfo_node(&i, nid);
-
-	n = sprintf(buf, "\n"
+	n = sprintf(buf,
 		       "Node %d MemTotal:       %8lu kB\n"
 		       "Node %d MemFree:        %8lu kB\n"
 		       "Node %d MemUsed:        %8lu kB\n"

@@ -78,13 +77,33 @@ static ssize_t node_read_meminfo(struct sys_device * dev,
 		       "Node %d Active(file):   %8lu kB\n"
 		       "Node %d Inactive(file): %8lu kB\n"
 		       "Node %d Unevictable:    %8lu kB\n"
-		       "Node %d Mlocked:        %8lu kB\n"
+		       "Node %d Mlocked:        %8lu kB\n",
+		       nid, K(i.totalram),
+		       nid, K(i.freeram),
+		       nid, K(i.totalram - i.freeram),
+		       nid, K(node_page_state(nid, NR_ACTIVE_ANON) +
+				node_page_state(nid, NR_ACTIVE_FILE)),
+		       nid, K(node_page_state(nid, NR_INACTIVE_ANON) +
+				node_page_state(nid, NR_INACTIVE_FILE)),
+		       nid, K(node_page_state(nid, NR_ACTIVE_ANON)),
+		       nid, K(node_page_state(nid, NR_INACTIVE_ANON)),
+		       nid, K(node_page_state(nid, NR_ACTIVE_FILE)),
+		       nid, K(node_page_state(nid, NR_INACTIVE_FILE)),
+		       nid, K(node_page_state(nid, NR_UNEVICTABLE)),
+		       nid, K(node_page_state(nid, NR_MLOCK)));
+
 #ifdef CONFIG_HIGHMEM
+	n += sprintf(buf + n,
 		       "Node %d HighTotal:      %8lu kB\n"
 		       "Node %d HighFree:       %8lu kB\n"
 		       "Node %d LowTotal:       %8lu kB\n"
-		       "Node %d LowFree:        %8lu kB\n"
+		       "Node %d LowFree:        %8lu kB\n",
+		       nid, K(i.totalhigh),
+		       nid, K(i.freehigh),
+		       nid, K(i.totalram - i.totalhigh),
+		       nid, K(i.freeram - i.freehigh));
 #endif
+	n += sprintf(buf + n,
 		       "Node %d Dirty:          %8lu kB\n"
 		       "Node %d Writeback:      %8lu kB\n"
 		       "Node %d FilePages:      %8lu kB\n"

@@ -99,25 +118,6 @@ static ssize_t node_read_meminfo(struct sys_device * dev,
 		       "Node %d Slab:           %8lu kB\n"
 		       "Node %d SReclaimable:   %8lu kB\n"
 		       "Node %d SUnreclaim:     %8lu kB\n",
-		       nid, K(i.totalram),
-		       nid, K(i.freeram),
-		       nid, K(i.totalram - i.freeram),
-		       nid, K(node_page_state(nid, NR_ACTIVE_ANON) +
-				node_page_state(nid, NR_ACTIVE_FILE)),
-		       nid, K(node_page_state(nid, NR_INACTIVE_ANON) +
-				node_page_state(nid, NR_INACTIVE_FILE)),
-		       nid, K(node_page_state(nid, NR_ACTIVE_ANON)),
-		       nid, K(node_page_state(nid, NR_INACTIVE_ANON)),
-		       nid, K(node_page_state(nid, NR_ACTIVE_FILE)),
-		       nid, K(node_page_state(nid, NR_INACTIVE_FILE)),
-		       nid, K(node_page_state(nid, NR_UNEVICTABLE)),
-		       nid, K(node_page_state(nid, NR_MLOCK)),
-#ifdef CONFIG_HIGHMEM
-		       nid, K(i.totalhigh),
-		       nid, K(i.freehigh),
-		       nid, K(i.totalram - i.totalhigh),
-		       nid, K(i.freeram - i.freehigh),
-#endif
 		       nid, K(node_page_state(nid, NR_FILE_DIRTY)),
 		       nid, K(node_page_state(nid, NR_WRITEBACK)),
 		       nid, K(node_page_state(nid, NR_FILE_PAGES)),

Signature · 1 line

1.6.5.2

Andrew Morton

maintainer

2010-07-08 20:59 UTC #2 lore raw

Quoted 2 lines · On Thu, 8 Jul 2010 19:41:57 +0900 (JST) KOSAKI Motohiro…

Grr, I did sent old ver. right patch is here ;-)
sorry.

oop, there it is. Did you check the output carefully?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Andrew Morton

maintainer

2010-07-08 20:58 UTC #3 lore raw

In reply to KOSAKI Motohiro

Quoted 7 lines · On Thu, 8 Jul 2010 18:20:14 +0900 (JST) KOSAKI Motohiro…

Now, cmpilation node_read_meminfo() output following warning. Because it has very large sprintf() argument.
drivers/base/node.c: In function 'node_read_meminfo':
drivers/base/node.c:139: warning: the frame size of 848 bytes is larger than 512 bytes

hm, I'm surprised it's that much.

Quoted 13 lines

drivers/base/node.c +1 -2

@@ -66,8 +66,7 @@ static ssize_t node_read_meminfo(struct sys_device * dev,
 	struct sysinfo i;
 
 	si_meminfo_node(&i, nid);
-
-	n = sprintf(buf, "\n"
+	n = sprintf(buf,
 		       "Node %d MemTotal:       %8lu kB\n"
 		       "Node %d MemFree:        %8lu kB\n"
 		       "Node %d MemUsed:        %8lu kB\n"

drivers/base/node.c static ssize_t node_read_meminfo(struct sys_device * dev, · line 85

Quoted 10 lines

drivers/base/node.c +6 -1

@@ -78,13 +77,33 @@ static ssize_t node_read_meminfo(struct sys_device * dev,
 		       "Node %d Active(file):   %8lu kB\n"
 		       "Node %d Inactive(file): %8lu kB\n"
 		       "Node %d Unevictable:    %8lu kB\n"
-		       "Node %d Mlocked:        %8lu kB\n"
+		       "Node %d Mlocked:        %8lu kB\n",
+		       nid, K(i.totalram),
+		       nid, K(i.freeram),
+		       nid, K(i.totalram - i.freeram),
+		       nid, K(node_page_state(nid, NR_ACTIVE_ANON) +
+				node_page_state(nid, NR_ACTIVE_FILE)),

Why the heck did we decide to print the same node-id 10000 times?

drivers/base/node.c static ssize_t node_read_meminfo(struct sys_device * dev, · line 86

Quoted 1 line

drivers/base/node.c +1 -0
+	n += sprintf(buf,

You just got caught sending untested patches.

drivers/base/node.c renamed from drivers/base/node.c~drivers-base-nodec-reduce-stack-usage-of-node_read_meminfo-fix +2 -2

@@ -93,7 +93,7 @@ static ssize_t node_read_meminfo(struct 
 		       nid, K(node_page_state(nid, NR_MLOCK)));
 
 #ifdef CONFIG_HIGHMEM
-	n += sprintf(buf,
+	n += sprintf(buf + n,
 		       "Node %d HighTotal:      %8lu kB\n"
 		       "Node %d HighFree:       %8lu kB\n"
 		       "Node %d LowTotal:       %8lu kB\n"

@@ -103,7 +103,7 @@ static ssize_t node_read_meminfo(struct 
 		       nid, K(i.totalram - i.totalhigh),
 		       nid, K(i.freeram - i.freehigh));
 #endif
-	n += sprintf(buf,
+	n += sprintf(buf + n,
 		       "Node %d Dirty:          %8lu kB\n"
 		       "Node %d Writeback:      %8lu kB\n"
 		       "Node %d FilePages:      %8lu kB\n"

Please, run the code and check that we didn't muck up the output.

KOSAKI Motohiro

author

2010-07-09 00:14 UTC #4 lore raw

Quoted 4 lines

Quoted 1 line · On Thu, 8 Jul 2010 18:20:14 +0900 (JST) KOSAKI Motohiro…

Quoted 8 lines

Quoted 6 lines
Now, cmpilation node_read_meminfo() output following warning. Because it has very large sprintf() argument.
drivers/base/node.c: In function 'node_read_meminfo':
drivers/base/node.c:139: warning: the frame size of 848 bytes is larger than 512 bytes
hm, I'm surprised it's that much.

me too.

Quoted 18 lines

drivers/base/node.c +1 -3

@@ -66,8 +66,7 @@ static ssize_t node_read_meminfo(struct sys_device * dev,
 	struct sysinfo i;
 
 	si_meminfo_node(&i, nid);
-
-	n = sprintf(buf, "\n"
+	n = sprintf(buf,
 		       "Node %d MemTotal:       %8lu kB\n"
 		       "Node %d MemFree:        %8lu kB\n"
 		       "Node %d MemUsed:        %8lu kB\n"

@@ -78,13 +77,33 @@ static ssize_t node_read_meminfo(struct sys_device * dev,
 		       "Node %d Active(file):   %8lu kB\n"
 		       "Node %d Inactive(file): %8lu kB\n"
 		       "Node %d Unevictable:    %8lu kB\n"
-		       "Node %d Mlocked:        %8lu kB\n"

Quoted 8 lines

drivers/base/node.c static ssize_t node_read_meminfo(struct sys_device * dev, · line 85
Quoted 6 lines
drivers/base/node.c +6 -0
+		       "Node %d Mlocked:        %8lu kB\n",
+		       nid, K(i.totalram),
+		       nid, K(i.freeram),
+		       nid, K(i.totalram - i.freeram),
+		       nid, K(node_page_state(nid, NR_ACTIVE_ANON) +
+				node_page_state(nid, NR_ACTIVE_FILE)),
Why the heck did we decide to print the same node-id 10000 times?

dunno. but I don't want to make behavior change for only stack reducing.

Quoted 28 lines

drivers/base/node.c static ssize_t node_read_meminfo(struct sys_device * dev, · line 86

Quoted 2 lines

drivers/base/node.c +1 -0
+	n += sprintf(buf,

You just got caught sending untested patches.

drivers/base/node.c renamed from drivers/base/node.c~drivers-base-nodec-reduce-stack-usage-of-node_read_meminfo-fix +2 -2

@@ -93,7 +93,7 @@ static ssize_t node_read_meminfo(struct 
 		       nid, K(node_page_state(nid, NR_MLOCK)));
 
 #ifdef CONFIG_HIGHMEM
-	n += sprintf(buf,
+	n += sprintf(buf + n,
 		       "Node %d HighTotal:      %8lu kB\n"
 		       "Node %d HighFree:       %8lu kB\n"
 		       "Node %d LowTotal:       %8lu kB\n"

@@ -103,7 +103,7 @@ static ssize_t node_read_meminfo(struct 
 		       nid, K(i.totalram - i.totalhigh),
 		       nid, K(i.freeram - i.freehigh));
 #endif
-	n += sprintf(buf,
+	n += sprintf(buf + n,
 		       "Node %d Dirty:          %8lu kB\n"
 		       "Node %d Writeback:      %8lu kB\n"
 		       "Node %d FilePages:      %8lu kB\n"

Quoted 1 line

Please, run the code and check that we didn't muck up the output.

100% my fault. I ran it, but I forgot to merge two patches ;)