Move code to isolate LRU pages into separate function

PATCH In review Martin Hicks 3 participants 6 messages Mar 14, 2005 – Mar 16, 2005

Subsystems (3)

Memory management MGLRU MM reclaim

lore

Share & tools

lore

Martin Hicks

author

2005-03-14 21:49 UTC #0 lore raw

PATCH 1/1 Move code to isolate LRU pages into separate function

Hi,

I noticed that the loop to pull pages out of the LRU lists for processing occurred twice. This just sticks that code into a separate function to improve readability.

The patch is against 2.6.11-mm2 but should apply to anything recent. Build and boot tested on sn2.

Thanks,
mh

Signed-off-by Martin Hicks <[email protected]>

 vmscan.c |  111 ++++++++++++++++++++++++++++++++-------------------------------
 1 files changed, 57 insertions(+), 54 deletions(-)

mm/vmscan.c +57 ~~-54~~

Index: linux-2.6.11/mm/vmscan.c
===================================================================
@@ -550,14 +550,57 @@
 }
 
 /*
- * zone->lru_lock is heavily contented.  We relieve it by quickly privatising
- * a batch of pages and working on them outside the lock.  Any pages which were
- * not freed will be added back to the LRU.
+ * zone->lru_lock is heavily contended.  Some of the functions that
+ * shrink the lists perform better by taking out a batch of pages
+ * and working on them outside the LRU lock.
  *
- * shrink_cache() adds the number of pages reclaimed to sc->nr_reclaimed
+ * For pagecache intensive workloads, this function is the hottest
+ * spot in the kernel (apart from copy_*_user functions).
+ *
+ * Appropriate locks must be held before calling this function.
+ *
+ * @nr_to_scan:	The number of pages to look through on the list.
+ * @src:	The LRU list to pull pages off.
+ * @dst:	The temp list to put pages on to.
+ * @scanned:	The number of pages that were scanned.
  *
- * For pagecache intensive workloads, the first loop here is the hottest spot
- * in the kernel (apart from the copy_*_user functions).
+ * returns how many pages were moved onto *@dst.
+ */
+static int isolate_lru_pages(int nr_to_scan, struct list_head *src,
+			     struct list_head *dst, int *scanned)
+{
+	int nr_taken = 0;
+	struct page *page;
+
+	BUG_ON(scanned == NULL);
+
+	*scanned = 0;
+	while (*scanned++ < nr_to_scan && !list_empty(src)) {
+		page = lru_to_page(src);
+		prefetchw_prev_lru_page(page, src, flags);
+
+		if (!TestClearPageLRU(page))
+			BUG();
+		list_del(&page->lru);
+		if (get_page_testone(page)) {
+			/*
+			 * It is being freed elsewhere
+			 */
+			__put_page(page);
+			SetPageLRU(page);
+			list_add(&page->lru, src);
+			continue;
+		} else {
+			list_add(&page->lru, dst);
+			nr_taken++;
+		}
+		*scanned++;
+	}
+	return nr_taken;
+}
+
+/*
+ * shrink_cache() adds the number of pages reclaimed to sc->nr_reclaimed
  */
 static void shrink_cache(struct zone *zone, struct scan_control *sc)
 {

@@ -571,32 +614,13 @@
 	spin_lock_irq(&zone->lru_lock);
 	while (max_scan > 0) {
 		struct page *page;
-		int nr_taken = 0;
-		int nr_scan = 0;
+		int nr_taken;
+		int nr_scan;
 		int nr_freed;
 
-		while (nr_scan++ < sc->swap_cluster_max &&
-				!list_empty(&zone->inactive_list)) {
-			page = lru_to_page(&zone->inactive_list);
-
-			prefetchw_prev_lru_page(page,
-						&zone->inactive_list, flags);
-
-			if (!TestClearPageLRU(page))
-				BUG();
-			list_del(&page->lru);
-			if (get_page_testone(page)) {
-				/*
-				 * It is being freed elsewhere
-				 */
-				__put_page(page);
-				SetPageLRU(page);
-				list_add(&page->lru, &zone->inactive_list);
-				continue;
-			}
-			list_add(&page->lru, &page_list);
-			nr_taken++;
-		}
+		nr_taken = isolate_lru_pages(sc->swap_cluster_max,
+					     &zone->inactive_list,
+					     &page_list, &nr_scan);
 		zone->nr_inactive -= nr_taken;
 		zone->pages_scanned += nr_scan;
 		spin_unlock_irq(&zone->lru_lock);

@@ -662,7 +686,7 @@
 {
 	int pgmoved;
 	int pgdeactivate = 0;
-	int pgscanned = 0;
+	int pgscanned;
 	int nr_pages = sc->nr_to_scan;
 	LIST_HEAD(l_hold);	/* The pages which were snipped off */
 	LIST_HEAD(l_inactive);	/* Pages to go onto the inactive_list */

@@ -675,30 +699,9 @@
 	long swap_tendency;
 
 	lru_add_drain();
-	pgmoved = 0;
 	spin_lock_irq(&zone->lru_lock);
-	while (pgscanned < nr_pages && !list_empty(&zone->active_list)) {
-		page = lru_to_page(&zone->active_list);
-		prefetchw_prev_lru_page(page, &zone->active_list, flags);
-		if (!TestClearPageLRU(page))
-			BUG();
-		list_del(&page->lru);
-		if (get_page_testone(page)) {
-			/*
-			 * It was already free!  release_pages() or put_page()
-			 * are about to remove it from the LRU and free it. So
-			 * put the refcount back and put the page back on the
-			 * LRU
-			 */
-			__put_page(page);
-			SetPageLRU(page);
-			list_add(&page->lru, &zone->active_list);
-		} else {
-			list_add(&page->lru, &l_hold);
-			pgmoved++;
-		}
-		pgscanned++;
-	}
+	pgmoved = isolate_lru_pages(nr_pages, &zone->active_list,
+				    &l_hold, &pgscanned);
 	zone->pages_scanned += pgscanned;
 	zone->nr_active -= pgmoved;
 	spin_unlock_irq(&zone->lru_lock);

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

Marcelo Tosatti

2005-03-15 15:37 UTC #1 lore raw

Hi Martin,

The -LHMS tree contains a similar "modularization" - which, however, does not add the being-remove-pages to any linked list and does not include the while loop.

steal_page_from_lru() (the -mhp version) is much more generic.

Check it out:
http://sr71.net/patches/2.6.11/

Quoted 167 lines · Martin Hicks

Hi,

I noticed that the loop to pull pages out of the LRU lists for processing occurred twice. This just sticks that code into a separate function to improve readability.

The patch is against 2.6.11-mm2 but should apply to anything recent. Build and boot tested on sn2.

Thanks,
mh

Signed-off-by Martin Hicks <[email protected]>

 vmscan.c |  111 ++++++++++++++++++++++++++++++++-------------------------------
 1 files changed, 57 insertions(+), 54 deletions(-)

Index: linux-2.6.11/mm/vmscan.c
===================================================================

mm/vmscan.c +57 ~~-55~~

@@ -550,14 +550,57 @@
 }
 
 /*
- * zone->lru_lock is heavily contented.  We relieve it by quickly privatising
- * a batch of pages and working on them outside the lock.  Any pages which were
- * not freed will be added back to the LRU.
+ * zone->lru_lock is heavily contended.  Some of the functions that
+ * shrink the lists perform better by taking out a batch of pages
+ * and working on them outside the LRU lock.
  *
- * shrink_cache() adds the number of pages reclaimed to sc->nr_reclaimed
+ * For pagecache intensive workloads, this function is the hottest
+ * spot in the kernel (apart from copy_*_user functions).
+ *
+ * Appropriate locks must be held before calling this function.
+ *
+ * @nr_to_scan:	The number of pages to look through on the list.
+ * @src:	The LRU list to pull pages off.
+ * @dst:	The temp list to put pages on to.
+ * @scanned:	The number of pages that were scanned.
  *
- * For pagecache intensive workloads, the first loop here is the hottest spot
- * in the kernel (apart from the copy_*_user functions).
+ * returns how many pages were moved onto *@dst.
+ */
+static int isolate_lru_pages(int nr_to_scan, struct list_head *src,
+			     struct list_head *dst, int *scanned)
+{
+	int nr_taken = 0;
+	struct page *page;
+
+	BUG_ON(scanned == NULL);
+
+	*scanned = 0;
+	while (*scanned++ < nr_to_scan && !list_empty(src)) {
+		page = lru_to_page(src);
+		prefetchw_prev_lru_page(page, src, flags);
+
+		if (!TestClearPageLRU(page))
+			BUG();
+		list_del(&page->lru);
+		if (get_page_testone(page)) {
+			/*
+			 * It is being freed elsewhere
+			 */
+			__put_page(page);
+			SetPageLRU(page);
+			list_add(&page->lru, src);
+			continue;
+		} else {
+			list_add(&page->lru, dst);
+			nr_taken++;
+		}
+		*scanned++;
+	}
+	return nr_taken;
+}
+
+/*
+ * shrink_cache() adds the number of pages reclaimed to sc->nr_reclaimed
  */
 static void shrink_cache(struct zone *zone, struct scan_control *sc)
 {

@@ -571,32 +614,13 @@
 	spin_lock_irq(&zone->lru_lock);
 	while (max_scan > 0) {
 		struct page *page;
-		int nr_taken = 0;
-		int nr_scan = 0;
+		int nr_taken;
+		int nr_scan;
 		int nr_freed;
 
-		while (nr_scan++ < sc->swap_cluster_max &&
-				!list_empty(&zone->inactive_list)) {
-			page = lru_to_page(&zone->inactive_list);
-
-			prefetchw_prev_lru_page(page,
-						&zone->inactive_list, flags);
-
-			if (!TestClearPageLRU(page))
-				BUG();
-			list_del(&page->lru);
-			if (get_page_testone(page)) {
-				/*
-				 * It is being freed elsewhere
-				 */
-				__put_page(page);
-				SetPageLRU(page);
-				list_add(&page->lru, &zone->inactive_list);
-				continue;
-			}
-			list_add(&page->lru, &page_list);
-			nr_taken++;
-		}
+		nr_taken = isolate_lru_pages(sc->swap_cluster_max,
+					     &zone->inactive_list,
+					     &page_list, &nr_scan);
 		zone->nr_inactive -= nr_taken;
 		zone->pages_scanned += nr_scan;
 		spin_unlock_irq(&zone->lru_lock);

@@ -662,7 +686,7 @@
 {
 	int pgmoved;
 	int pgdeactivate = 0;
-	int pgscanned = 0;
+	int pgscanned;
 	int nr_pages = sc->nr_to_scan;
 	LIST_HEAD(l_hold);	/* The pages which were snipped off */
 	LIST_HEAD(l_inactive);	/* Pages to go onto the inactive_list */

@@ -675,30 +699,9 @@
 	long swap_tendency;
 
 	lru_add_drain();
-	pgmoved = 0;
 	spin_lock_irq(&zone->lru_lock);
-	while (pgscanned < nr_pages && !list_empty(&zone->active_list)) {
-		page = lru_to_page(&zone->active_list);
-		prefetchw_prev_lru_page(page, &zone->active_list, flags);
-		if (!TestClearPageLRU(page))
-			BUG();
-		list_del(&page->lru);
-		if (get_page_testone(page)) {
-			/*
-			 * It was already free!  release_pages() or put_page()
-			 * are about to remove it from the LRU and free it. So
-			 * put the refcount back and put the page back on the
-			 * LRU
-			 */
-			__put_page(page);
-			SetPageLRU(page);
-			list_add(&page->lru, &zone->active_list);
-		} else {
-			list_add(&page->lru, &l_hold);
-			pgmoved++;
-		}
-		pgscanned++;
-	}
+	pgmoved = isolate_lru_pages(nr_pages, &zone->active_list,
+				    &l_hold, &pgscanned);
 	zone->pages_scanned += pgscanned;
 	zone->nr_active -= pgmoved;
 	spin_unlock_irq(&zone->lru_lock);
--

Quoted 4 lines

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>

Marcelo Tosatti

2005-03-15 16:29 UTC #2 lore raw

Quoted 5 lines · Marcelo Tosatti

Hi Martin,
The -LHMS tree contains a similar "modularization" - which, however, does not add the being-remove-pages to any linked list and does not include the while loop.

Addition: it removes only one page from the LRU, thus it can be used outside vmscan guts.

Quoted 5 lines

steal_page_from_lru() (the -mhp version) is much more generic.
Check it out:
http://sr71.net/patches/2.6.11/

Martin Hicks

author

2005-03-15 19:54 UTC #3 lore raw

In reply to Martin Hicks

Quoted 8 lines · Martin Hicks

Hi,
I noticed that the loop to pull pages out of the LRU lists for processing occurred twice. This just sticks that code into a separate function to improve readability.
The patch is against 2.6.11-mm2 but should apply to anything recent. Build and boot tested on sn2.

Whoops. I was double-incrementing scanned, so only half the pages that you asked for were being scanned.

This version fixes that and also allows passing in a NULL scanned argument if you don't care how many pages were scanned.

Signed-off-by Martin Hicks <[email protected]>

mm/vmscan.c +57 ~~-54~~

Index: linux-2.6.11/mm/vmscan.c
===================================================================
@@ -550,14 +550,57 @@
 }
 
 /*
- * zone->lru_lock is heavily contented.  We relieve it by quickly privatising
- * a batch of pages and working on them outside the lock.  Any pages which were
- * not freed will be added back to the LRU.
+ * zone->lru_lock is heavily contended.  Some of the functions that
+ * shrink the lists perform better by taking out a batch of pages
+ * and working on them outside the LRU lock.
  *
- * shrink_cache() adds the number of pages reclaimed to sc->nr_reclaimed
+ * For pagecache intensive workloads, this function is the hottest
+ * spot in the kernel (apart from copy_*_user functions).
+ *
+ * Appropriate locks must be held before calling this function.
+ *
+ * @nr_to_scan:	The number of pages to look through on the list.
+ * @src:	The LRU list to pull pages off.
+ * @dst:	The temp list to put pages on to.
+ * @scanned:	The number of pages that were scanned.
  *
- * For pagecache intensive workloads, the first loop here is the hottest spot
- * in the kernel (apart from the copy_*_user functions).
+ * returns how many pages were moved onto *@dst.
+ */
+static int isolate_lru_pages(int nr_to_scan, struct list_head *src,
+			     struct list_head *dst, int *scanned)
+{
+	int nr_taken = 0;
+	struct page *page;
+	int scan = 0;
+
+	while (scan++ < nr_to_scan && !list_empty(src)) {
+		page = lru_to_page(src);
+		prefetchw_prev_lru_page(page, src, flags);
+
+		if (!TestClearPageLRU(page))
+			BUG();
+		list_del(&page->lru);
+		if (get_page_testone(page)) {
+			/*
+			 * It is being freed elsewhere
+			 */
+			__put_page(page);
+			SetPageLRU(page);
+			list_add(&page->lru, src);
+			continue;
+		} else {
+			list_add(&page->lru, dst);
+			nr_taken++;
+		}
+	}
+
+	if (scanned)
+		*scanned = scan;
+	return nr_taken;
+}
+
+/*
+ * shrink_cache() adds the number of pages reclaimed to sc->nr_reclaimed
  */
 static void shrink_cache(struct zone *zone, struct scan_control *sc)
 {

@@ -571,32 +614,13 @@
 	spin_lock_irq(&zone->lru_lock);
 	while (max_scan > 0) {
 		struct page *page;
-		int nr_taken = 0;
-		int nr_scan = 0;
+		int nr_taken;
+		int nr_scan;
 		int nr_freed;
 
-		while (nr_scan++ < sc->swap_cluster_max &&
-				!list_empty(&zone->inactive_list)) {
-			page = lru_to_page(&zone->inactive_list);
-
-			prefetchw_prev_lru_page(page,
-						&zone->inactive_list, flags);
-
-			if (!TestClearPageLRU(page))
-				BUG();
-			list_del(&page->lru);
-			if (get_page_testone(page)) {
-				/*
-				 * It is being freed elsewhere
-				 */
-				__put_page(page);
-				SetPageLRU(page);
-				list_add(&page->lru, &zone->inactive_list);
-				continue;
-			}
-			list_add(&page->lru, &page_list);
-			nr_taken++;
-		}
+		nr_taken = isolate_lru_pages(sc->swap_cluster_max,
+					     &zone->inactive_list,
+					     &page_list, &nr_scan);
 		zone->nr_inactive -= nr_taken;
 		zone->pages_scanned += nr_scan;
 		spin_unlock_irq(&zone->lru_lock);

@@ -662,7 +686,7 @@
 {
 	int pgmoved;
 	int pgdeactivate = 0;
-	int pgscanned = 0;
+	int pgscanned;
 	int nr_pages = sc->nr_to_scan;
 	LIST_HEAD(l_hold);	/* The pages which were snipped off */
 	LIST_HEAD(l_inactive);	/* Pages to go onto the inactive_list */

@@ -675,30 +699,9 @@
 	long swap_tendency;
 
 	lru_add_drain();
-	pgmoved = 0;
 	spin_lock_irq(&zone->lru_lock);
-	while (pgscanned < nr_pages && !list_empty(&zone->active_list)) {
-		page = lru_to_page(&zone->active_list);
-		prefetchw_prev_lru_page(page, &zone->active_list, flags);
-		if (!TestClearPageLRU(page))
-			BUG();
-		list_del(&page->lru);
-		if (get_page_testone(page)) {
-			/*
-			 * It was already free!  release_pages() or put_page()
-			 * are about to remove it from the LRU and free it. So
-			 * put the refcount back and put the page back on the
-			 * LRU
-			 */
-			__put_page(page);
-			SetPageLRU(page);
-			list_add(&page->lru, &zone->active_list);
-		} else {
-			list_add(&page->lru, &l_hold);
-			pgmoved++;
-		}
-		pgscanned++;
-	}
+	pgmoved = isolate_lru_pages(nr_pages, &zone->active_list,
+				    &l_hold, &pgscanned);
 	zone->pages_scanned += pgscanned;
 	zone->nr_active -= pgmoved;
 	spin_unlock_irq(&zone->lru_lock);

Andrew Morton

2005-03-16 06:37 UTC #4 lore raw

Quoted 8 lines · Martin Hicks

Quoted 6 lines · Martin Hicks
Hi,
I noticed that the loop to pull pages out of the LRU lists for processing occurred twice. This just sticks that code into a separate function to improve readability.

Quoted 9 lines

Quoted 2 lines
The patch is against 2.6.11-mm2 but should apply to anything recent. Build and boot tested on sn2.
Whoops. I was double-incrementing scanned, so only half the pages that you asked for were being scanned.
This version fixes that and also allows passing in a NULL scanned argument if you don't care how many pages were scanned.

But neither caller passes in a NULL argument.
--
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>

Martin Hicks

author

2005-03-16 15:10 UTC #5 lore raw

Quoted 5 lines · Andrew Morton

Quoted 3 lines
This version fixes that and also allows passing in a NULL scanned argument if you don't care how many pages were scanned.
But neither caller passes in a NULL argument.

I was playing around with another function, and I really didn't care how many pages were scanned. I just thought that it seemed like a silly requirement to pass a variable in for scanned if you didn't care.

Signature · 6 lines

Martin Hicks   ||   Silicon Graphics Inc.   ||   [email protected]
--
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>