Skip to contentSkip to conversation

mm/vmscan.c: wrap five parameters into arg_container in shrink_page_list()

PATCH In review Chen Yucong 3 participants 4 messages Jun 12, 2014

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 mm/vmscan.c: wrap five parameters into arg_container in shrink_page_list()

shrink_page_list() has too many arguments that have already reached ten. Some of those arguments and temporary variables introduces extra 80 bytes on the stack.

This patch wraps five parameters into arg_container and removes some temporary variables, thus making shrink_page_list() to consume fewer stack space.

Before mm/vmscan.c is modified:
text data bss dec hex filename
6876698 957224 966656 8800578 864942 vmlinux-3.15

After mm/vmscan.c is changed:
text data bss dec hex filename
6876506 957224 966656 8800386 864882 vmlinux-3.15

---
 mm/vmscan.c |   64 +++++++++++++++++++++++++++--------------------------------
 1 file changed, 29 insertions(+), 35 deletions(-)
mm/vmscan.c +29 -35
index a8ffe4e..538cdcf 100644
@@ -790,6 +790,14 @@ static void page_check_dirty_writeback(struct page *page,
 		mapping->a_ops->is_dirty_writeback(page, dirty, writeback);
 }
 
+struct arg_container {
+	unsigned long nr_dirty;
+	unsigned long nr_unqueued_dirty;
+	unsigned long nr_congested;
+	unsigned long nr_writeback;
+	unsigned long nr_immediate;
+};
+
 /*
  * shrink_page_list() returns the number of reclaimed pages
  */
@@ -797,22 +805,13 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 				      struct zone *zone,
 				      struct scan_control *sc,
 				      enum ttu_flags ttu_flags,
-				      unsigned long *ret_nr_dirty,
-				      unsigned long *ret_nr_unqueued_dirty,
-				      unsigned long *ret_nr_congested,
-				      unsigned long *ret_nr_writeback,
-				      unsigned long *ret_nr_immediate,
+				      struct arg_container *ac,
 				      bool force_reclaim)
 {
 	LIST_HEAD(ret_pages);
 	LIST_HEAD(free_pages);
 	int pgactivate = 0;
-	unsigned long nr_unqueued_dirty = 0;
-	unsigned long nr_dirty = 0;
-	unsigned long nr_congested = 0;
 	unsigned long nr_reclaimed = 0;
-	unsigned long nr_writeback = 0;
-	unsigned long nr_immediate = 0;
 
 	cond_resched();
 
@@ -858,10 +857,10 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 		 */
 		page_check_dirty_writeback(page, &dirty, &writeback);
 		if (dirty || writeback)
-			nr_dirty++;
+			ac->nr_dirty++;
 
 		if (dirty && !writeback)
-			nr_unqueued_dirty++;
+			ac->nr_unqueued_dirty++;
 
 		/*
 		 * Treat this page as congested if the underlying BDI is or if
@@ -872,7 +871,7 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 		mapping = page_mapping(page);
 		if ((mapping && bdi_write_congested(mapping->backing_dev_info)) ||
 		    (writeback && PageReclaim(page)))
-			nr_congested++;
+			ac->nr_congested++;
 
 		/*
 		 * If a page at the tail of the LRU is under writeback, there
@@ -916,7 +915,7 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 			if (current_is_kswapd() &&
 			    PageReclaim(page) &&
 			    zone_is_reclaim_writeback(zone)) {
-				nr_immediate++;
+				ac->nr_immediate++;
 				goto keep_locked;
 
 			/* Case 2 above */
@@ -934,7 +933,7 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 				 * and it's also appropriate in global reclaim.
 				 */
 				SetPageReclaim(page);
-				nr_writeback++;
+				ac->nr_writeback++;
 
 				goto keep_locked;
 
@@ -1132,11 +1131,6 @@ keep:
 	list_splice(&ret_pages, page_list);
 	count_vm_events(PGACTIVATE, pgactivate);
 	mem_cgroup_uncharge_end();
-	*ret_nr_dirty += nr_dirty;
-	*ret_nr_congested += nr_congested;
-	*ret_nr_unqueued_dirty += nr_unqueued_dirty;
-	*ret_nr_writeback += nr_writeback;
-	*ret_nr_immediate += nr_immediate;
 	return nr_reclaimed;
 }
 
@@ -1148,7 +1142,8 @@ unsigned long reclaim_clean_pages_from_list(struct zone *zone,
 		.priority = DEF_PRIORITY,
 		.may_unmap = 1,
 	};
-	unsigned long ret, dummy1, dummy2, dummy3, dummy4, dummy5;
+	unsigned long ret;
+	struct arg_container dummy;
 	struct page *page, *next;
 	LIST_HEAD(clean_pages);
 
@@ -1161,8 +1156,7 @@ unsigned long reclaim_clean_pages_from_list(struct zone *zone,
 	}
 
 	ret = shrink_page_list(&clean_pages, zone, &sc,
-			TTU_UNMAP|TTU_IGNORE_ACCESS,
-			&dummy1, &dummy2, &dummy3, &dummy4, &dummy5, true);
+			TTU_UNMAP|TTU_IGNORE_ACCESS, &dummy, true);
 	list_splice(&clean_pages, page_list);
 	mod_zone_page_state(zone, NR_ISOLATED_FILE, -ret);
 	return ret;
@@ -1469,11 +1463,13 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,
 	unsigned long nr_scanned;
 	unsigned long nr_reclaimed = 0;
 	unsigned long nr_taken;
-	unsigned long nr_dirty = 0;
-	unsigned long nr_congested = 0;
-	unsigned long nr_unqueued_dirty = 0;
-	unsigned long nr_writeback = 0;
-	unsigned long nr_immediate = 0;
+	struct arg_container ac = {
+		.nr_dirty = 0,
+		.nr_congested = 0,
+		.nr_unqueued_dirty = 0,
+		.nr_writeback = 0,
+		.nr_immediate = 0,
+	};
 	isolate_mode_t isolate_mode = 0;
 	int file = is_file_lru(lru);
 	struct zone *zone = lruvec_zone(lruvec);
@@ -1515,9 +1511,7 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,
 		return 0;
 
 	nr_reclaimed = shrink_page_list(&page_list, zone, sc, TTU_UNMAP,
-				&nr_dirty, &nr_unqueued_dirty, &nr_congested,
-				&nr_writeback, &nr_immediate,
-				false);
+					&ac, false);
 
 	spin_lock_irq(&zone->lru_lock);
 
@@ -1554,7 +1548,7 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,
 	 * of pages under pages flagged for immediate reclaim and stall if any
 	 * are encountered in the nr_immediate check below.
 	 */
-	if (nr_writeback && nr_writeback == nr_taken)
+	if (ac.nr_writeback && ac.nr_writeback == nr_taken)
 		zone_set_flag(zone, ZONE_WRITEBACK);
 
 	/*
@@ -1566,7 +1560,7 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,
 		 * Tag a zone as congested if all the dirty pages scanned were
 		 * backed by a congested BDI and wait_iff_congested will stall.
 		 */
-		if (nr_dirty && nr_dirty == nr_congested)
+		if (ac.nr_dirty && ac.nr_dirty == ac.nr_congested)
 			zone_set_flag(zone, ZONE_CONGESTED);
 
 		/*
@@ -1576,7 +1570,7 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,
 		 * pages from reclaim context. It will forcibly stall in the
 		 * next check.
 		 */
-		if (nr_unqueued_dirty == nr_taken)
+		if (ac.nr_unqueued_dirty == nr_taken)
 			zone_set_flag(zone, ZONE_TAIL_LRU_DIRTY);
 
 		/*
@@ -1585,7 +1579,7 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,
 		 * implies that pages are cycling through the LRU faster than
 		 * they are written so also forcibly stall.
 		 */
-		if ((nr_unqueued_dirty == nr_taken || nr_immediate) &&
+		if ((ac.nr_unqueued_dirty == nr_taken || ac.nr_immediate) &&
 		    current_may_throttle())
 			congestion_wait(BLK_RW_ASYNC, HZ/10);
 	}
Signature · 7 lines
1.7.10.4

--
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>
#1 lore raw
Quoted 6 lines · Chen Yucong

shrink_page_list() has too many arguments that have already reached ten. Some of those arguments and temporary variables introduces extra 80 bytes on the stack.

This patch wraps five parameters into arg_container and removes some temporary variables, thus making shrink_page_list() to consume fewer stack space.

Won't the container with those arguments now live on the stack, using up the same space that the variables used to take?

Quoted 195 lines

Before mm/vmscan.c is modified:
text data bss dec hex filename
6876698 957224 966656 8800578 864942 vmlinux-3.15

After mm/vmscan.c is changed:
text data bss dec hex filename
6876506 957224 966656 8800386 864882 vmlinux-3.15

---
 mm/vmscan.c |   64 +++++++++++++++++++++++++++--------------------------------
 1 file changed, 29 insertions(+), 35 deletions(-)
mm/vmscan.c +29 -35
index a8ffe4e..538cdcf 100644
@@ -790,6 +790,14 @@ static void page_check_dirty_writeback(struct page *page,
 		mapping->a_ops->is_dirty_writeback(page, dirty, writeback);
 }
 
+struct arg_container {
+	unsigned long nr_dirty;
+	unsigned long nr_unqueued_dirty;
+	unsigned long nr_congested;
+	unsigned long nr_writeback;
+	unsigned long nr_immediate;
+};
+
 /*
  * shrink_page_list() returns the number of reclaimed pages
  */
@@ -797,22 +805,13 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 				      struct zone *zone,
 				      struct scan_control *sc,
 				      enum ttu_flags ttu_flags,
-				      unsigned long *ret_nr_dirty,
-				      unsigned long *ret_nr_unqueued_dirty,
-				      unsigned long *ret_nr_congested,
-				      unsigned long *ret_nr_writeback,
-				      unsigned long *ret_nr_immediate,
+				      struct arg_container *ac,
 				      bool force_reclaim)
 {
 	LIST_HEAD(ret_pages);
 	LIST_HEAD(free_pages);
 	int pgactivate = 0;
-	unsigned long nr_unqueued_dirty = 0;
-	unsigned long nr_dirty = 0;
-	unsigned long nr_congested = 0;
 	unsigned long nr_reclaimed = 0;
-	unsigned long nr_writeback = 0;
-	unsigned long nr_immediate = 0;
 
 	cond_resched();
 
@@ -858,10 +857,10 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 		 */
 		page_check_dirty_writeback(page, &dirty, &writeback);
 		if (dirty || writeback)
-			nr_dirty++;
+			ac->nr_dirty++;
 
 		if (dirty && !writeback)
-			nr_unqueued_dirty++;
+			ac->nr_unqueued_dirty++;
 
 		/*
 		 * Treat this page as congested if the underlying BDI is or if
@@ -872,7 +871,7 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 		mapping = page_mapping(page);
 		if ((mapping && bdi_write_congested(mapping->backing_dev_info)) ||
 		    (writeback && PageReclaim(page)))
-			nr_congested++;
+			ac->nr_congested++;
 
 		/*
 		 * If a page at the tail of the LRU is under writeback, there
@@ -916,7 +915,7 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 			if (current_is_kswapd() &&
 			    PageReclaim(page) &&
 			    zone_is_reclaim_writeback(zone)) {
-				nr_immediate++;
+				ac->nr_immediate++;
 				goto keep_locked;
 
 			/* Case 2 above */
@@ -934,7 +933,7 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 				 * and it's also appropriate in global reclaim.
 				 */
 				SetPageReclaim(page);
-				nr_writeback++;
+				ac->nr_writeback++;
 
 				goto keep_locked;
 
@@ -1132,11 +1131,6 @@ keep:
 	list_splice(&ret_pages, page_list);
 	count_vm_events(PGACTIVATE, pgactivate);
 	mem_cgroup_uncharge_end();
-	*ret_nr_dirty += nr_dirty;
-	*ret_nr_congested += nr_congested;
-	*ret_nr_unqueued_dirty += nr_unqueued_dirty;
-	*ret_nr_writeback += nr_writeback;
-	*ret_nr_immediate += nr_immediate;
 	return nr_reclaimed;
 }
 
@@ -1148,7 +1142,8 @@ unsigned long reclaim_clean_pages_from_list(struct zone *zone,
 		.priority = DEF_PRIORITY,
 		.may_unmap = 1,
 	};
-	unsigned long ret, dummy1, dummy2, dummy3, dummy4, dummy5;
+	unsigned long ret;
+	struct arg_container dummy;
 	struct page *page, *next;
 	LIST_HEAD(clean_pages);
 
@@ -1161,8 +1156,7 @@ unsigned long reclaim_clean_pages_from_list(struct zone *zone,
 	}
 
 	ret = shrink_page_list(&clean_pages, zone, &sc,
-			TTU_UNMAP|TTU_IGNORE_ACCESS,
-			&dummy1, &dummy2, &dummy3, &dummy4, &dummy5, true);
+			TTU_UNMAP|TTU_IGNORE_ACCESS, &dummy, true);
 	list_splice(&clean_pages, page_list);
 	mod_zone_page_state(zone, NR_ISOLATED_FILE, -ret);
 	return ret;
@@ -1469,11 +1463,13 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,
 	unsigned long nr_scanned;
 	unsigned long nr_reclaimed = 0;
 	unsigned long nr_taken;
-	unsigned long nr_dirty = 0;
-	unsigned long nr_congested = 0;
-	unsigned long nr_unqueued_dirty = 0;
-	unsigned long nr_writeback = 0;
-	unsigned long nr_immediate = 0;
+	struct arg_container ac = {
+		.nr_dirty = 0,
+		.nr_congested = 0,
+		.nr_unqueued_dirty = 0,
+		.nr_writeback = 0,
+		.nr_immediate = 0,
+	};
 	isolate_mode_t isolate_mode = 0;
 	int file = is_file_lru(lru);
 	struct zone *zone = lruvec_zone(lruvec);
@@ -1515,9 +1511,7 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,
 		return 0;
 
 	nr_reclaimed = shrink_page_list(&page_list, zone, sc, TTU_UNMAP,
-				&nr_dirty, &nr_unqueued_dirty, &nr_congested,
-				&nr_writeback, &nr_immediate,
-				false);
+					&ac, false);
 
 	spin_lock_irq(&zone->lru_lock);
 
@@ -1554,7 +1548,7 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,
 	 * of pages under pages flagged for immediate reclaim and stall if any
 	 * are encountered in the nr_immediate check below.
 	 */
-	if (nr_writeback && nr_writeback == nr_taken)
+	if (ac.nr_writeback && ac.nr_writeback == nr_taken)
 		zone_set_flag(zone, ZONE_WRITEBACK);
 
 	/*
@@ -1566,7 +1560,7 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,
 		 * Tag a zone as congested if all the dirty pages scanned were
 		 * backed by a congested BDI and wait_iff_congested will stall.
 		 */
-		if (nr_dirty && nr_dirty == nr_congested)
+		if (ac.nr_dirty && ac.nr_dirty == ac.nr_congested)
 			zone_set_flag(zone, ZONE_CONGESTED);
 
 		/*
@@ -1576,7 +1570,7 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,
 		 * pages from reclaim context. It will forcibly stall in the
 		 * next check.
 		 */
-		if (nr_unqueued_dirty == nr_taken)
+		if (ac.nr_unqueued_dirty == nr_taken)
 			zone_set_flag(zone, ZONE_TAIL_LRU_DIRTY);
 
 		/*
@@ -1585,7 +1579,7 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,
 		 * implies that pages are cycling through the LRU faster than
 		 * they are written so also forcibly stall.
 		 */
-		if ((nr_unqueued_dirty == nr_taken || nr_immediate) &&
+		if ((ac.nr_unqueued_dirty == nr_taken || ac.nr_immediate) &&
 		    current_may_throttle())
 			congestion_wait(BLK_RW_ASYNC, HZ/10);
 	}
Signature · 7 lines
All rights reversed

--
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>
#2 lore raw
Quoted 6 lines · Rik van Riel
Quoted 1 line

shrink_page_list() has too many arguments that have already reached

ten.

Quoted 1 line

Some of those arguments and temporary variables introduces extra 80

bytes

Quoted 2 lines

on the stack.

Quoted 8 lines
Quoted 1 line

This patch wraps five parameters into arg_container and removes some

temporary

Quoted 1 line

variables, thus making shrink_page_list() to consume fewer stack

space.

Won't the container with those arguments now live on the stack, using up the same space that the variables used to take?

Of course, the container with those arguments live on the stack.

One of the key reason for introducing this patch is to avoid passing five pointer arguments to shrink_page_list().

The arg_container also uses up the same space that the variables used to take.
If the those arguments is wrapped to arg_container, we just need to pass one pointer to shrink_page_list instead of five.

thx!
cyc

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

maintainer
#3 lore raw
In reply to Chen Yucong
Quoted 7 lines · On Thu, 12 Jun 2014 17:36:35 +0800 Chen Yucong <[email protected]> wrote:

shrink_page_list() has too many arguments that have already reached ten. Some of those arguments and temporary variables introduces extra 80 bytes on the stack.

This patch wraps five parameters into arg_container and removes some temporary variables, thus making shrink_page_list() to consume fewer stack space.

Quoted 7 lines

Before mm/vmscan.c is modified:
text data bss dec hex filename
6876698 957224 966656 8800578 864942 vmlinux-3.15

After mm/vmscan.c is changed:
text data bss dec hex filename
6876506 957224 966656 8800386 864882 vmlinux-3.15

Code size reduction is a good sign.

Quoted 1 line
 1 file changed, 29 insertions(+), 35 deletions(-)

We can look at the frame pointer alterations. Requires CONFIG_FRAME_POINTER. There's also scripts/checkstack.pl.

Without:

shrink_page_list:

	pushq	%rbp	#
	movq	%rsp, %rbp	#,
	pushq	%r15	#
	pushq	%r14	#
	pushq	%r13	#
	pushq	%r12	#
	pushq	%rbx	#
	subq	$184, %rsp	#,

With:

shrink_page_list:

	pushq	%rbp	#
	movq	%rsp, %rbp	#,
	pushq	%r15	#
	pushq	%r14	#
	pushq	%r13	#
	pushq	%r12	#
	pushq	%rbx	#
	subq	$136, %rsp	#,

So we've saved approx 184-136=48 bytes of stack in shrink_page_list(). shrink_inactive_list() stack space is unchanged.

Please do this sort of analysis yourself and include it in the changelogs.

mm/vmscan.c static void page_check_dirty_writeback(struct page *page, · line 799

Quoted 13 lines
mm/vmscan.c +7 -0
@@ -790,6 +790,14 @@ static void page_check_dirty_writeback(struct page *page,
 		mapping->a_ops->is_dirty_writeback(page, dirty, writeback);
 }
 
+struct arg_container {
+	unsigned long nr_dirty;
+	unsigned long nr_unqueued_dirty;
+	unsigned long nr_congested;
+	unsigned long nr_writeback;
+	unsigned long nr_immediate;
+};

This name is dreadful. Let's give it a nice, meaningful name and document it appropriately. So it all looks like a part of the vmscan code and not some hack which was bolted onto the side to save a bit of stack.

Something like

/*
* Callers pass a prezeroed shrink_result into the shrink functions to gather
* statistics about how many pages of particular states were processed */

struct shrink_result {
	...

mm/vmscan.c unsigned long reclaim_clean_pages_from_list(struct zone *zone, · line 1146

Quoted 13 lines
mm/vmscan.c +2 -1
 /*
  * shrink_page_list() returns the number of reclaimed pages
  */
 
...
@@ -1148,7 +1142,8 @@ unsigned long reclaim_clean_pages_from_list(struct zone *zone,
 		.priority = DEF_PRIORITY,
 		.may_unmap = 1,
 	};
-	unsigned long ret, dummy1, dummy2, dummy3, dummy4, dummy5;
+	unsigned long ret;
+	struct arg_container dummy;

If we're not going to use this then we can make it static and save more stack. That will have some runtime cost as different CPUs fight over ownership of cachelines but I doubt if it will be significant.

If we leave it on the stack then this code will send kmemcheck berzerk with all the used-uninitialized errors. Presumably that it already the case. Perhaps `dummy' should be initialized if kmemcheck is in operation, dunno.

Quoted 12 lines
mm/vmscan.c +0 -2
 	struct page *page, *next;
 	LIST_HEAD(clean_pages);
 
 
...
@@ -1469,11 +1463,13 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,
 	unsigned long nr_scanned;
 	unsigned long nr_reclaimed = 0;
 	unsigned long nr_taken;
-	unsigned long nr_dirty = 0;
-	unsigned long nr_congested = 0;

mm/vmscan.c shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec, · line 1472

Quoted 10 lines
mm/vmscan.c +7 -3
-	unsigned long nr_unqueued_dirty = 0;
-	unsigned long nr_writeback = 0;
-	unsigned long nr_immediate = 0;
+	struct arg_container ac = {
+		.nr_dirty = 0,
+		.nr_congested = 0,
+		.nr_unqueued_dirty = 0,
+		.nr_writeback = 0,
+		.nr_immediate = 0,
+	};

This:

	struct arg_container ac = { };

mm/vmscan.c shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,

Quoted 6 lines
mm/vmscan.c +0 -0
 	isolate_mode_t isolate_mode = 0;
 	int file = is_file_lru(lru);
 	struct zone *zone = lruvec_zone(lruvec);
 
...

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