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

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

[Russ Anderson]

Prevent a page with a physical memory error from being placed back
on the LRU.  A new page flag bit is added.  PG_memerror is given
bit 32 and is only defined on 64 bit architectures.  This avoids
burning a page flag bit on 32 bit architectures.  "BadPages:" is
added to the output of /proc/meminfo to display the amount of memory 
marked bad (on 64 bit architectures).

Signed-off-by: Russ Anderson <rja@sgi.com>

---
 fs/proc/proc_misc.c        |    6 ++++++
 include/linux/kernel.h     |    1 +
 include/linux/page-flags.h |   23 +++++++++++++++++++++++
 include/linux/swap.h       |    1 +
 mm/migrate.c               |    3 +++
 mm/page_alloc.c            |   45 +++++++++++----------------------------------
 mm/swap.c                  |    4 ++++
 7 files changed, 49 insertions(+), 34 deletions(-)

Index: test/mm/swap.c
=================================--- test.orig/mm/swap.c	2008-04-25 15:22:11.204934760 -0500
+++ test/mm/swap.c	2008-04-25 15:42:02.806263586 -0500
@@ -208,6 +208,8 @@ void lru_cache_add(struct page *page)
 	struct pagevec *pvec = &get_cpu_var(lru_add_pvecs);
 
 	page_cache_get(page);
+	if (unlikely(PageMemError(page)))
+		return;		/* Don't add bad pages to the page list */
 	if (!pagevec_add(pvec, page))
 		__pagevec_lru_add(pvec);
 	put_cpu_var(lru_add_pvecs);
@@ -218,6 +220,8 @@ void lru_cache_add_active(struct page *p
 	struct pagevec *pvec = &get_cpu_var(lru_add_active_pvecs);
 
 	page_cache_get(page);
+	if (unlikely(PageMemError(page)))
+		return;		/* Don't add bad pages to the page list */
 	if (!pagevec_add(pvec, page))
 		__pagevec_lru_add_active(pvec);
 	put_cpu_var(lru_add_active_pvecs);
Index: test/mm/page_alloc.c
=================================--- test.orig/mm/page_alloc.c	2008-04-25 15:22:11.196933787 -0500
+++ test/mm/page_alloc.c	2008-04-25 15:42:02.818265056 -0500
@@ -70,6 +70,7 @@ unsigned long totalram_pages __read_most
 unsigned long totalreserve_pages __read_mostly;
 long nr_swap_pages;
 int percpu_pagelist_fraction;
+unsigned int totalbad_pages;
 
 #ifdef CONFIG_HUGETLB_PAGE_SIZE_VARIABLE
 int pageblock_order __read_mostly;
@@ -236,16 +237,7 @@ static void bad_page(struct page *page)
 	printk(KERN_EMERG "Trying to fix it up, but a reboot is needed\n"
 		KERN_EMERG "Backtrace:\n");
 	dump_stack();
-	page->flags &= ~(1 << PG_lru	|
-			1 << PG_private |
-			1 << PG_locked	|
-			1 << PG_active	|
-			1 << PG_dirty	|
-			1 << PG_reclaim |
-			1 << PG_slab    |
-			1 << PG_swapcache |
-			1 << PG_writeback |
-			1 << PG_buddy );
+	page->flags &= ~(PAGE_FLAGS_RECLAIM);
 	set_page_count(page, 0);
 	reset_page_mapcount(page);
 	page->mapping = NULL;
@@ -462,16 +454,7 @@ static inline int free_pages_check(struc
 		(page->mapping != NULL)  |
 		(page_get_page_cgroup(page) != NULL) |
 		(page_count(page) != 0)  |
-		(page->flags & (
-			1 << PG_lru	|
-			1 << PG_private |
-			1 << PG_locked	|
-			1 << PG_active	|
-			1 << PG_slab	|
-			1 << PG_swapcache |
-			1 << PG_writeback |
-			1 << PG_reserved |
-			1 << PG_buddy ))))
+		(page->flags & (PAGE_FLAGS_RESERVE))))
 		bad_page(page);
 	if (PageDirty(page))
 		__ClearPageDirty(page);
@@ -612,24 +595,14 @@ static int prep_new_page(struct page *pa
 		(page->mapping != NULL)  |
 		(page_get_page_cgroup(page) != NULL) |
 		(page_count(page) != 0)  |
-		(page->flags & (
-			1 << PG_lru	|
-			1 << PG_private	|
-			1 << PG_locked	|
-			1 << PG_active	|
-			1 << PG_dirty	|
-			1 << PG_slab    |
-			1 << PG_swapcache |
-			1 << PG_writeback |
-			1 << PG_reserved |
-			1 << PG_buddy ))))
+		(page->flags & (PAGE_FLAGS_DIRTY))))
 		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_readahead |
@@ -1762,6 +1735,7 @@ void si_meminfo(struct sysinfo *val)
 	val->totalhigh = totalhigh_pages;
 	val->freehigh = nr_free_highpages();
 	val->mem_unit = PAGE_SIZE;
+	val->badpages = totalbad_pages;
 }
 
 EXPORT_SYMBOL(si_meminfo);
@@ -2478,6 +2452,9 @@ static void setup_zone_migrate_reserve(s
 
 		/* Blocks with reserved pages will never free, skip them. */
 		if (PageReserved(page))
+			continue;
+		/* Blocks with error pages will never free, skip them. */
+		if (PageMemError(page))
 			continue;
 
 		block_migratetype = get_pageblock_migratetype(page);
Index: test/include/linux/kernel.h
=================================--- test.orig/include/linux/kernel.h	2008-04-25 15:22:07.640501421 -0500
+++ test/include/linux/kernel.h	2008-04-25 15:42:02.826266036 -0500
@@ -412,6 +412,7 @@ struct sysinfo {
 	unsigned long totalhigh;	/* Total high memory size */
 	unsigned long freehigh;		/* Available high memory size */
 	unsigned int mem_unit;		/* Memory unit size in bytes */
+	unsigned int badpages;		/* Number of bad (PG_memerror) pages */
 	char _f[20-2*sizeof(long)-sizeof(int)];	/* Padding: libc5 uses this.. */
 };
 
Index: test/include/linux/swap.h
=================================--- test.orig/include/linux/swap.h	2008-04-25 15:22:08.040550056 -0500
+++ test/include/linux/swap.h	2008-04-25 15:42:02.854269468 -0500
@@ -165,6 +165,7 @@ extern unsigned long totalreserve_pages;
 extern long nr_swap_pages;
 extern unsigned int nr_free_buffer_pages(void);
 extern unsigned int nr_free_pagecache_pages(void);
+extern unsigned int totalbad_pages;
 
 /* Definition of global_page_state not available yet */
 #define nr_free_pages() global_page_state(NR_FREE_PAGES)
Index: test/mm/migrate.c
=================================--- test.orig/mm/migrate.c	2008-04-25 15:22:11.192933300 -0500
+++ test/mm/migrate.c	2008-04-28 13:27:48.686601486 -0500
@@ -368,6 +368,7 @@ static void migrate_page_copy(struct pag
 {
 	copy_highpage(newpage, page);
 
+	/* Do not migrate PG_memerror to the new page */
 	if (PageError(page))
 		SetPageError(newpage);
 	if (PageReferenced(page))
@@ -714,6 +715,8 @@ unlock:
  		 */
  		list_del(&page->lru);
  		move_to_lru(page);
+		if (PageMemError(page))
+			totalbad_pages++;
 	}
 
 move_newpage:
Index: test/fs/proc/proc_misc.c
=================================--- test.orig/fs/proc/proc_misc.c	2008-04-25 15:22:04.684142013 -0500
+++ test/fs/proc/proc_misc.c	2008-04-25 15:42:02.890273879 -0500
@@ -179,6 +179,9 @@ static int meminfo_read_proc(char *page,
 		"PageTables:   %8lu kB\n"
 		"NFS_Unstable: %8lu kB\n"
 		"Bounce:       %8lu kB\n"
+#if (BITS_PER_LONG > 32)
+		"BadPages:     %8u kB\n"
+#endif
 		"CommitLimit:  %8lu kB\n"
 		"Committed_AS: %8lu kB\n"
 		"VmallocTotal: %8lu kB\n"
@@ -210,6 +213,9 @@ static int meminfo_read_proc(char *page,
 		K(global_page_state(NR_PAGETABLE)),
 		K(global_page_state(NR_UNSTABLE_NFS)),
 		K(global_page_state(NR_BOUNCE)),
+#if (BITS_PER_LONG > 32)
+		K(i.badpages),
+#endif
 		K(allowed),
 		K(committed),
 		(unsigned long)VMALLOC_TOTAL >> 10,
Index: test/include/linux/page-flags.h
=================================--- test.orig/include/linux/page-flags.h	2008-04-25 15:22:07.868529143 -0500
+++ test/include/linux/page-flags.h	2008-04-25 15:42:02.914276820 -0500
@@ -48,6 +48,9 @@
  * PG_buddy is set to indicate that the page is free and in the buddy system
  * (see mm/page_alloc.c).
  *
+ * PG_memerror indicates a physical memory error on the page.  The page
+ * should not be reused (ie put on the LRU).
+ *
  */
 
 /*
@@ -106,6 +109,7 @@
  *         63                            32                              0
  */
 #define PG_uncached		31	/* Page has been mapped as uncached */
+#define PG_memerror		32	/* Page has a physical memory error */
 #endif
 
 /*
@@ -307,5 +311,24 @@ static inline void set_page_writeback(st
 {
 	test_set_page_writeback(page);
 }
+
+#if (BITS_PER_LONG > 32)
+#define PageMemError(page)	test_bit(PG_memerror, &(page)->flags)
+#define SetPageMemError(page)	set_bit(PG_memerror, &(page)->flags)
+#define ClearPageMemError(page)	clear_bit(PG_memerror, &(page)->flags)
+#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 | \
+			 1UL << PG_memerror)
+#else
+#define PageMemError(page)	0
+#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)
+#endif
+#define PAGE_FLAGS_RECLAIM	(PAGE_FLAGS | 1 << PG_reclaim | 1 << PG_dirty)
+#define PAGE_FLAGS_RESERVE	(PAGE_FLAGS | 1 << PG_reserved)
+#define PAGE_FLAGS_DIRTY	(PAGE_FLAGS | 1 << PG_reserved | 1 << PG_dirty)
+
 
 #endif	/* PAGE_FLAGS_H */
-- 
Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc          rja@sgi.com

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

[Christoph Lameter]

On Mon, 28 Apr 2008, Russ Anderson wrote:

> +#if (BITS_PER_LONG > 32)
> +#define PageMemError(page)	test_bit(PG_memerror, &(page)->flags)
> +#define SetPageMemError(page)	set_bit(PG_memerror, &(page)->flags)
> +#define ClearPageMemError(page)	clear_bit(PG_memerror, &(page)->flags)
> +#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 | \
> +			 1UL << PG_memerror)
> +#else
> +#define PageMemError(page)	0
> +#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)
> +#endif
> +#define PAGE_FLAGS_RECLAIM	(PAGE_FLAGS | 1 << PG_reclaim | 1 << PG_dirty)
> +#define PAGE_FLAGS_RESERVE	(PAGE_FLAGS | 1 << PG_reserved)
> +#define PAGE_FLAGS_DIRTY	(PAGE_FLAGS | 1 << PG_reserved | 1 << PG_dirty)
> +

All the PAGE_FLAGS_xx things could be in a separate cleanup patch. Also 
note that the way page flags are defined may change soon.

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

[Arjan van de Ven]

On Tue, 29 Apr 2008 14:33:29 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Mon, 28 Apr 2008 14:23:58 -0500
> Russ Anderson <rja@sgi.com> wrote:
> 
> > --- test.orig/include/linux/kernel.h	2008-04-25
> > 15:22:07.640501421 -0500 +++ test/include/linux/kernel.h
> > 2008-04-25 15:42:02.826266036 -0500 @@ -412,6 +412,7 @@ struct
> > sysinfo { unsigned long totalhigh;	/* Total high memory size
> > */ unsigned long freehigh;		/* Available high memory
> > size */ unsigned int mem_unit;		/* Memory unit size
> > in bytes */
> > +	unsigned int badpages;		/* Number of bad
> > (PG_memerror) pages */ char
> > _f[20-2*sizeof(long)-sizeof(int)];	/* Padding: libc5 uses
> > this.. */ };
> 
> eek.  sys_sysinfo() copies this structure out to userspace verbatim. 
> Increasing its size by four bytes will cause memory scribbles in
> userspace applications.

not if you also reduce the padding ??

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

[Andrew Morton]

On Mon, 28 Apr 2008 14:23:58 -0500
Russ Anderson <rja@sgi.com> wrote:

> --- test.orig/include/linux/kernel.h	2008-04-25 15:22:07.640501421 -0500
> +++ test/include/linux/kernel.h	2008-04-25 15:42:02.826266036 -0500
> @@ -412,6 +412,7 @@ struct sysinfo {
>  	unsigned long totalhigh;	/* Total high memory size */
>  	unsigned long freehigh;		/* Available high memory size */
>  	unsigned int mem_unit;		/* Memory unit size in bytes */
> +	unsigned int badpages;		/* Number of bad (PG_memerror) pages */
>  	char _f[20-2*sizeof(long)-sizeof(int)];	/* Padding: libc5 uses this.. */
>  };

eek.  sys_sysinfo() copies this structure out to userspace verbatim. 
Increasing its size by four bytes will cause memory scribbles in userspace
applications.

That's fixable by suitably adjusting sysinfo._f, but given the lack of
version information in the sysinfo struct, I'm not sure how useful this
change really is.

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

[Russ Anderson]

On Tue, Apr 29, 2008 at 02:33:29PM -0700, Andrew Morton wrote:
> On Mon, 28 Apr 2008 14:23:58 -0500
> Russ Anderson <rja@sgi.com> wrote:
> 
> > --- test.orig/include/linux/kernel.h	2008-04-25 15:22:07.640501421 -0500
> > +++ test/include/linux/kernel.h	2008-04-25 15:42:02.826266036 -0500
> > @@ -412,6 +412,7 @@ struct sysinfo {
> >  	unsigned long totalhigh;	/* Total high memory size */
> >  	unsigned long freehigh;		/* Available high memory size */
> >  	unsigned int mem_unit;		/* Memory unit size in bytes */
> > +	unsigned int badpages;		/* Number of bad (PG_memerror) pages */
> >  	char _f[20-2*sizeof(long)-sizeof(int)];	/* Padding: libc5 uses this.. */
> >  };
> 
> eek.  sys_sysinfo() copies this structure out to userspace verbatim. 
> Increasing its size by four bytes will cause memory scribbles in userspace
> applications.
> 
> That's fixable by suitably adjusting sysinfo._f, but given the lack of
> version information in the sysinfo struct, I'm not sure how useful this
> change really is.

OK.  The only reason badpages was added was to print the number
of page pages in /proc/meminfo.  I'll go with plan B, a badmem driver 
for displaying bad page info. 

-- 
Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc          rja@sgi.com

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

[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 <rja@sgi.com>
Reviewed-by: Christoph Lameter <clameter@sgi.com>

---
 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
-- 
Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc          rja@sgi.com

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

[Nick Piggin]

On Saturday 19 July 2008 06:36, Russ Anderson wrote:
> [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 <rja@sgi.com>
> Reviewed-by: Christoph Lameter <clameter@sgi.com>
>
> ---
>  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);

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

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

[Russ Anderson]

On Tue, Jul 22, 2008 at 12:42:10PM +1000, Nick Piggin wrote:
> On Saturday 19 July 2008 06:36, Russ Anderson wrote:
> > [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 <rja@sgi.com>
> > Reviewed-by: Christoph Lameter <clameter@sgi.com>
> >
> > ---
> >  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);
> 
> 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.

> 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

-- 
Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc          rja@sgi.com

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

[Nick Piggin]

On Thursday 24 July 2008 08:13, Russ Anderson wrote:
> On Tue, Jul 22, 2008 at 12:42:10PM +1000, Nick Piggin wrote:
> > On Saturday 19 July 2008 06:36, Russ Anderson wrote:
> > > [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 <rja@sgi.com>
> > > Reviewed-by: Christoph Lameter <clameter@sgi.com>
> > >
> > > ---
> > >  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);
> >
> > 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.

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.