Skip to contentSkip to conversation

mm: Avoid putting a bad page back on the LRU v7

PATCH In review Russ Anderson 2 participants 4 messages Jul 18, 2008 – Jul 24, 2008

lore
Share & tools
lore

Russ Anderson

author
#0 lore raw

PATCH 1/2 mm: Avoid putting a bad page back on the LRU v7

[PATCH 1/2] mm: Avoid putting a bad page back on the LRU v7

Prevent a page with a physical memory error from being placed back on the LRU. A new page flag (PG_memerror) is added if CONFIG_PAGEFLAGS_EXTENDED is defined.

Signed-off-by Russ Anderson <[email protected]>
Reviewed-by Christoph Lameter <[email protected]>
---
 include/linux/page-flags.h |   18 ++++++++++++++++--
 mm/migrate.c               |   31 ++++++++++++++++++++++++++++++-
 mm/page_alloc.c            |   13 ++++++++-----
 3 files changed, 54 insertions(+), 8 deletions(-)

Index: linux/mm/page_alloc.c
=================================--- linux.orig/mm/page_alloc.c 2008-07-18 15:15:48.000000000 -0500
+++ linux/mm/page_alloc.c 2008-07-18 15:16:09.000000000 -0500
@@ -602,10 +602,10 @@ static int prep_new_page(struct page *pa
bad_page(page);

/*
- * For now, we report if PG_reserved was found set, but do not
- * clear it, and do not allocate the page: as a safety net.
+ * For now, we report if PG_reserved or PG_memerror was found set, but
+ * do not clear it, and do not allocate the page: as a safety net.
*/
- if (PageReserved(page))
+ if (PageReserved(page) || PageMemError(page))
return 1;

page->flags &= ~(1 << PG_uptodate | 1 << PG_error | 1 << PG_reclaim |
@@ -2475,8 +2475,11 @@ static void setup_zone_migrate_reserve(s
continue;
page = pfn_to_page(pfn);

- /* Blocks with reserved pages will never free, skip them. */
- if (PageReserved(page))
+ /*
+ * Blocks with reserved pages or memory errors will never
+ * free, skip them.
+ */
+ if (PageReserved(page) || PageMemError(page))
continue;

block_migratetype = get_pageblock_migratetype(page);
Index: linux/mm/migrate.c
=================================--- linux.orig/mm/migrate.c 2008-07-18 15:15:48.000000000 -0500
+++ linux/mm/migrate.c 2008-07-18 15:16:09.000000000 -0500
@@ -720,7 +720,25 @@ unlock:
* restored.
*/
list_del(&page->lru);
- move_to_lru(page);
+ if (PageMemError(page))
+ if (rc = 0)
+ /*
+ * A page with a memory error that has
+ * been migrated will not be moved to
+ * the LRU.
+ */
+ goto move_newpage;
+ else
+ /*
+ * The page failed to migrate and will not
+ * be added to the bad page list. Clearing
+ * the error bit will allow another attempt
+ * to migrate if it gets another correctable
+ * error.
+ */
+ ClearPageMemError(page);
+
+ move_to_lru(page);
}

move_newpage:
@@ -790,6 +808,17 @@ int migrate_pages(struct list_head *from
}
}
}
+
+ if (rc != 0)
+ list_for_each_entry_safe(page, page2, from, lru)
+ if (PageMemError(page))
+ /*
+ * The page failed to migrate. Clearing
+ * the error bit will allow another attempt
+ * to migrate if it gets another correctable
+ * error.
+ */
+ ClearPageMemError(page);
rc = 0;
out:
if (!swapwrite)
Index: linux/include/linux/page-flags.h
=================================--- linux.orig/include/linux/page-flags.h 2008-07-18 15:15:48.000000000 -0500
+++ linux/include/linux/page-flags.h 2008-07-18 15:16:09.000000000 -0500
@@ -84,6 +84,7 @@ enum pageflags {
PG_private, /* If pagecache, has fs-private data */
PG_writeback, /* Page is under writeback */
#ifdef CONFIG_PAGEFLAGS_EXTENDED
+ PG_memerror, /* Page has a physical memory error */
PG_head, /* A head page */
PG_tail, /* A tail page */
#else
@@ -132,15 +133,21 @@ static inline int TestSetPage##uname(str
static inline int TestClearPage##uname(struct page *page) \
{ return test_and_clear_bit(PG_##lname, &page->flags); }

+#define PAGEFLAGMASK(uname, lname) \
+static inline int PAGEMASK_##uname(void) \
+ { return (1 << PG_##lname); }

#define PAGEFLAG(uname, lname) TESTPAGEFLAG(uname, lname) \
- SETPAGEFLAG(uname, lname) CLEARPAGEFLAG(uname, lname)
+ SETPAGEFLAG(uname, lname) CLEARPAGEFLAG(uname, lname) \
+ PAGEFLAGMASK(uname, lname)

#define __PAGEFLAG(uname, lname) TESTPAGEFLAG(uname, lname) \
__SETPAGEFLAG(uname, lname) __CLEARPAGEFLAG(uname, lname)

#define PAGEFLAG_FALSE(uname) \
static inline int Page##uname(struct page *page) \
+ { return 0; } \
+static inline int PAGEMASK_##uname(void) \
{ return 0; }

#define TESTSCFLAG(uname, lname) \
@@ -308,9 +315,16 @@ static inline void __ClearPageTail(struc

#endif /* !PAGEFLAGS_EXTENDED */

+#ifdef CONFIG_PAGEFLAGS_EXTENDED
+PAGEFLAG(MemError, memerror)
+#else
+PAGEFLAG_FALSE(MemError)
+#endif
+
#define PAGE_FLAGS (1 << PG_lru | 1 << PG_private | 1 << PG_locked | \

 			 1 << PG_buddy | 1 << PG_writeback | \
-			 1 << PG_slab  | 1 << PG_swapcache | 1 << PG_active)
+			 1 << PG_slab  | 1 << PG_swapcache | 1 << PG_active) |\

+ PAGEMASK_MemError()

/*
* Flags checked in bad_page(). Pages on the free list should not have

Signature · 2 lines
Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc          [email protected]
#1 lore raw
Quoted 36 lines · Russ Anderson

[PATCH 1/2] mm: Avoid putting a bad page back on the LRU v7

Prevent a page with a physical memory error from being placed back on the LRU. A new page flag (PG_memerror) is added if CONFIG_PAGEFLAGS_EXTENDED is defined.

Signed-off-by Russ Anderson <[email protected]>
Reviewed-by Christoph Lameter <[email protected]>
---
 include/linux/page-flags.h |   18 ++++++++++++++++--
 mm/migrate.c               |   31 ++++++++++++++++++++++++++++++-
 mm/page_alloc.c            |   13 ++++++++-----
 3 files changed, 54 insertions(+), 8 deletions(-)

Index: linux/mm/page_alloc.c
=================================> --- linux.orig/mm/page_alloc.c 2008-07-18 15:15:48.000000000 -0500
+++ linux/mm/page_alloc.c 2008-07-18 15:16:09.000000000 -0500

(quoted diff) +3 -3
@@ -602,10 +602,10 @@ static int prep_new_page(struct page *pa
 		bad_page(page);
 
 	/*
-	 * For now, we report if PG_reserved was found set, but do not
-	 * clear it, and do not allocate the page: as a safety net.
+	 * For now, we report if PG_reserved or PG_memerror was found set, but
+	 * do not clear it, and do not allocate the page: as a safety net.
 	 */
-	if (PageReserved(page))
+	if (PageReserved(page) || PageMemError(page))
 		return 1;
 
 	page->flags &= ~(1 << PG_uptodate | 1 << PG_error | 1 << PG_reclaim |

@@ -2475,8 +2475,11 @@ static void setup_zone_migrate_reserve(s
 			continue;
 		page = pfn_to_page(pfn);

Review static void setup_zone_migrate_reserve(s · line 2485

Quoted 10 lines
(quoted diff) +5 -2
-		/* Blocks with reserved pages will never free, skip them. */
-		if (PageReserved(page))
+		/*
+		 * Blocks with reserved pages or memory errors will never
+		 * free, skip them.
+		 */
+		if (PageReserved(page) || PageMemError(page))
 			continue;
 
 		block_migratetype = get_pageblock_migratetype(page);

I don't like adding more branches like this into fastpaths like this. It would make a lot more sense to me if you just had some private module that does the job of isolating the page from the lru and/or elevating their refcount so that they do not get put back on freelists.

Migration may need something to perhaps allow migrations of pages not on LRU lists but have PageMemError set which is OK, but I really don't like adding code and branches to page_alloc.c if possible....

Thanks,
Nick

author
#2 lore raw
Quoted 46 lines · Nick Piggin
Quoted 45 lines · Russ Anderson

[PATCH 1/2] mm: Avoid putting a bad page back on the LRU v7

Prevent a page with a physical memory error from being placed back on the LRU. A new page flag (PG_memerror) is added if CONFIG_PAGEFLAGS_EXTENDED is defined.

Signed-off-by Russ Anderson <[email protected]>
Reviewed-by Christoph Lameter <[email protected]>
---
 include/linux/page-flags.h |   18 ++++++++++++++++--
 mm/migrate.c               |   31 ++++++++++++++++++++++++++++++-
 mm/page_alloc.c            |   13 ++++++++-----
 3 files changed, 54 insertions(+), 8 deletions(-)

Index: linux/mm/page_alloc.c
=================================> > --- linux.orig/mm/page_alloc.c 2008-07-18 15:15:48.000000000 -0500
+++ linux/mm/page_alloc.c 2008-07-18 15:16:09.000000000 -0500

(quoted diff) +8 -5
@@ -602,10 +602,10 @@ static int prep_new_page(struct page *pa
 		bad_page(page);
 
 	/*
-	 * For now, we report if PG_reserved was found set, but do not
-	 * clear it, and do not allocate the page: as a safety net.
+	 * For now, we report if PG_reserved or PG_memerror was found set, but
+	 * do not clear it, and do not allocate the page: as a safety net.
 	 */
-	if (PageReserved(page))
+	if (PageReserved(page) || PageMemError(page))
 		return 1;
 
 	page->flags &= ~(1 << PG_uptodate | 1 << PG_error | 1 << PG_reclaim |

@@ -2475,8 +2475,11 @@ static void setup_zone_migrate_reserve(s
 			continue;
 		page = pfn_to_page(pfn);
 
-		/* Blocks with reserved pages will never free, skip them. */
-		if (PageReserved(page))
+		/*
+		 * Blocks with reserved pages or memory errors will never
+		 * free, skip them.
+		 */
+		if (PageReserved(page) || PageMemError(page))
 			continue;
Quoted 6 lines

Review static void setup_zone_migrate_reserve(s · line 2485

Quoted 1 line
(quoted diff) +0 -0
 		block_migratetype = get_pageblock_migratetype(page);

I don't like adding more branches like this into fastpaths like this. It would make a lot more sense to me if you just had some private module that does the job of isolating the page from the lru and/or elevating their refcount so that they do not get put back on freelists.

That is how it works. If PageMemError is set the migration code leaves the page with an elevated refcount. The PageMemError() check was to avoid reallocating the page was an additional safty net. I'll pull the checks.

Quoted 6 lines

Migration may need something to perhaps allow migrations of pages not on LRU lists but have PageMemError set which is OK, but I really don't like adding code and branches to page_alloc.c if possible....

Thanks,
Nick

Signature · 2 lines
Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc          [email protected]
#3 lore raw
Quoted 54 lines · Russ Anderson
Quoted 46 lines · Nick Piggin
Quoted 45 lines · Russ Anderson

[PATCH 1/2] mm: Avoid putting a bad page back on the LRU v7

Prevent a page with a physical memory error from being placed back on the LRU. A new page flag (PG_memerror) is added if CONFIG_PAGEFLAGS_EXTENDED is defined.

Signed-off-by Russ Anderson <[email protected]>
Reviewed-by Christoph Lameter <[email protected]>
---
 include/linux/page-flags.h |   18 ++++++++++++++++--
 mm/migrate.c               |   31 ++++++++++++++++++++++++++++++-
 mm/page_alloc.c            |   13 ++++++++-----
 3 files changed, 54 insertions(+), 8 deletions(-)

Index: linux/mm/page_alloc.c
=================================> > > --- linux.orig/mm/page_alloc.c 2008-07-18 15:15:48.000000000 -0500
+++ linux/mm/page_alloc.c 2008-07-18 15:16:09.000000000 -0500

(quoted diff) +1 -2
@@ -602,10 +602,10 @@ static int prep_new_page(struct page *pa
 		bad_page(page);
 
 	/*
-	 * For now, we report if PG_reserved was found set, but do not
-	 * clear it, and do not allocate the page: as a safety net.
+	 * For now, we report if PG_reserved or PG_memerror was found set,

but + * do not clear it, and do not allocate the page: as a safety
net. */

(quoted diff) +6 -3
-	if (PageReserved(page))
+	if (PageReserved(page) || PageMemError(page))
 		return 1;
 
 	page->flags &= ~(1 << PG_uptodate | 1 << PG_error | 1 << PG_reclaim |

@@ -2475,8 +2475,11 @@ static void setup_zone_migrate_reserve(s
 			continue;
 		page = pfn_to_page(pfn);
 
-		/* Blocks with reserved pages will never free, skip them. */
-		if (PageReserved(page))
+		/*
+		 * Blocks with reserved pages or memory errors will never
+		 * free, skip them.
+		 */
+		if (PageReserved(page) || PageMemError(page))
 			continue;
Quoted 6 lines

Review static void setup_zone_migrate_reserve(s · line 2485

Quoted 1 line
(quoted diff) +0 -0
 		block_migratetype = get_pageblock_migratetype(page);

I don't like adding more branches like this into fastpaths like this. It would make a lot more sense to me if you just had some private module that does the job of isolating the page from the lru and/or elevating their refcount so that they do not get put back on freelists.

Quoted 4 lines

That is how it works. If PageMemError is set the migration code leaves the page with an elevated refcount. The PageMemError() check was to avoid reallocating the page was an additional safty net. I'll pull the checks.

Ah, if it's that easy, great, then I have no problems with the patch. Note that I have no problems with putting a VM_BUG_ON(PageMemError()) or something like that instead -- perhaps not quite the safty net you would like. But helpful for developers and testers, and it acts as a helpful little comment.