HWPOISON: Intro

Bail out early when hardware poisoned pages are found in page fault handling.

Since they are poisoned they should not be mapped freshly into processes, because that would cause another (potentially deadly) machine check

This is generally handled in the same way as OOM, just a different error code is returned to the architecture code.

Signed-off-by Andi Kleen <[email protected]>

---
 mm/memory.c |    3 +++
 1 file changed, 3 insertions(+)

Index: linux/mm/memory.c
===================================================================

Quoted 2 lines · Andi Kleen

Bail out early when hardware poisoned pages are found in page fault handling.

I suspect this patch is also not absolutely necessary: the poisoned page will normally have been isolated already.

Quoted 6 lines

mm/memory.c +0 -0

@@ -2797,6 +2797,9 @@
 	if (unlikely(ret & (VM_FAULT_ERROR | VM_FAULT_NOPAGE)))
 		return ret;
 

mm/memory.c line 2802

Quoted 3 lines

mm/memory.c +3 -0

+	if (unlikely(PageHWPoison(vmf.page)))
+		return VM_FAULT_HWPOISON;
+

Direct return with locked page could lockup someone later. Either drop this patch or fix it with this check?

Quoted 6 lines · Wu Fengguang

Quoted 2 lines · Andi Kleen

Bail out early when hardware poisoned pages are found in page fault handling.

I suspect this patch is also not absolutely necessary: the poisoned page will normally have been isolated already.

It's needed to prevent new pages comming in when there is a parallel fault while the memory failure handling is in process. Otherwise the pages could get remapped in that small window.

Quoted 6 lines

Quoted 6 lines

mm/memory.c +0 -0

@@ -2797,6 +2797,9 @@
 	if (unlikely(ret & (VM_FAULT_ERROR | VM_FAULT_NOPAGE)))
 		return ret;
 

Quoted 6 lines

mm/memory.c line 2802

Quoted 3 lines

mm/memory.c +3 -0

+	if (unlikely(PageHWPoison(vmf.page)))
+		return VM_FAULT_HWPOISON;
+

Direct return with locked page could lockup someone later. Either drop this patch or fix it with this check?

Fair point. Fixed.

Quoted 5 lines · Andi Kleen

Quoted 4 lines · Wu Fengguang

Quoted 2 lines · Andi Kleen

Bail out early when hardware poisoned pages are found in page fault handling.

Quoted 6 lines

Quoted 2 lines

I suspect this patch is also not absolutely necessary: the poisoned page will normally have been isolated already.

It's needed to prevent new pages comming in when there is a parallel fault while the memory failure handling is in process. Otherwise the pages could get remapped in that small window.

This patch makes no difference at least for file pages, including tmpfs.

Quoted 9 lines · Wu Fengguang

Quoted 8 lines · Andi Kleen

Quoted 6 lines · Wu Fengguang

Quoted 2 lines · Andi Kleen

Bail out early when hardware poisoned pages are found in page fault handling.

I suspect this patch is also not absolutely necessary: the poisoned page will normally have been isolated already.

Quoted 5 lines

Quoted 3 lines

It's needed to prevent new pages comming in when there is a parallel fault while the memory failure handling is in process. Otherwise the pages could get remapped in that small window.

This patch makes no difference at least for file pages, including tmpfs.

I was more thinking of anonymous pages with multiple mappers (e.g. COW after fork)

Quoted 14 lines · Andi Kleen

Quoted 13 lines · Wu Fengguang

Quoted 11 lines · Andi Kleen

Quoted 6 lines · Wu Fengguang

Quoted 2 lines · Andi Kleen

Bail out early when hardware poisoned pages are found in page fault handling.

I suspect this patch is also not absolutely necessary: the poisoned page will normally have been isolated already.

It's needed to prevent new pages comming in when there is a parallel fault while the memory failure handling is in process. Otherwise the pages could get remapped in that small window.

Quoted 4 lines

Quoted 1 line

This patch makes no difference at least for file pages, including tmpfs.

I was more thinking of anonymous pages with multiple mappers (e.g. COW after fork)

I guess they are handled by do_anonymous_page() or do_wp_page(), instead of do_linear_fault()/do_nonlinear_fault()?

Quoted 15 lines · Wu Fengguang

Quoted 14 lines · Andi Kleen

Quoted 13 lines · Wu Fengguang

Quoted 11 lines · Andi Kleen

Quoted 6 lines · Wu Fengguang

Quoted 2 lines · Andi Kleen

Bail out early when hardware poisoned pages are found in page fault handling.

I suspect this patch is also not absolutely necessary: the poisoned page will normally have been isolated already.

It's needed to prevent new pages comming in when there is a parallel fault while the memory failure handling is in process. Otherwise the pages could get remapped in that small window.

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

This patch makes no difference at least for file pages, including tmpfs.

I was more thinking of anonymous pages with multiple mappers (e.g. COW after fork)

I guess they are handled by do_anonymous_page() or do_wp_page(), instead of do_linear_fault()/do_nonlinear_fault()?

You're right. Sorry was a little confused in my earlier reply.

I think what I meant is: what happens during the window
when the page has just the poison bit set, but is not isolated/unmapped yet. During that window I want new mappers to not come in.
That is why that check is there.

Bail out early when hardware poisoned pages are found in page fault handling. Since they are poisoned they should not be mapped freshly into processes, because that would cause another (potentially deadly) machine check

This is generally handled in the same way as OOM, just a different error code is returned to the architecture code.

Signed-off-by Andi Kleen <[email protected]>

---
 mm/memory.c |    3 +++
 1 file changed, 3 insertions(+)

Index: linux/mm/memory.c
===================================================================

Quoted 22 lines · Andi Kleen

Bail out early when hardware poisoned pages are found in page fault handling. Since they are poisoned they should not be mapped freshly into processes, because that would cause another (potentially deadly) machine check

This is generally handled in the same way as OOM, just a different error code is returned to the architecture code.

Signed-off-by Andi Kleen <[email protected]>

---
 mm/memory.c |    3 +++
 1 file changed, 3 insertions(+)

Index: linux/mm/memory.c
===================================================================

mm/memory.c +0 -0

@@ -2797,6 +2797,9 @@
 	if (unlikely(ret & (VM_FAULT_ERROR | VM_FAULT_NOPAGE)))
 		return ret;
 

mm/memory.c line 2801

Quoted 2 lines

mm/memory.c +2 -0

+	if (unlikely(PageHWPoison(vmf.page)))
+		return VM_FAULT_HWPOISON;

Again, it would be nice if you just worry about this in your MCE handler and don't sprinkle things like this in fastpaths.

Quoted 23 lines · Nick Piggin

Quoted 22 lines · Andi Kleen

Bail out early when hardware poisoned pages are found in page fault handling. Since they are poisoned they should not be mapped freshly into processes, because that would cause another (potentially deadly) machine check

This is generally handled in the same way as OOM, just a different error code is returned to the architecture code.

Signed-off-by Andi Kleen <[email protected]>

---
 mm/memory.c |    3 +++
 1 file changed, 3 insertions(+)

Index: linux/mm/memory.c
===================================================================

mm/memory.c +0 -0

@@ -2797,6 +2797,9 @@
 	if (unlikely(ret & (VM_FAULT_ERROR | VM_FAULT_NOPAGE)))
 		return ret;
 

Quoted 5 lines

mm/memory.c line 2801

Quoted 2 lines

mm/memory.c +2 -0

+	if (unlikely(PageHWPoison(vmf.page)))
+		return VM_FAULT_HWPOISON;

Again, it would be nice if you just worry about this in your MCE handler and don't sprinkle things like this in fastpaths.

For this patch, I cannot imagine a clear usage case for it, and proposed to remove it until there comes a case.

Add VM_FAULT_HWPOISON handling to the x86 page fault handler. This is very similar to VM_FAULT_OOM, the only difference is that a different si_code is passed to user space and the new addr_lsb field is initialized.

v2: Make the printk more verbose/unique

Signed-off-by Andi Kleen <[email protected]>

---
 arch/x86/mm/fault.c |   19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

Index: linux/arch/x86/mm/fault.c
===================================================================

arch/x86/mm/fault.c +4 -1

@@ -166,6 +166,7 @@
 	info.si_errno	= 0;
 	info.si_code	= si_code;
 	info.si_addr	= (void __user *)address;
+	info.si_addr_lsb = si_code == BUS_MCEERR_AR ? PAGE_SHIFT : 0;
 
 	force_sig_info(si_signo, &info, tsk);
 }

@@ -797,10 +798,12 @@
 }
 
 static void
-do_sigbus(struct pt_regs *regs, unsigned long error_code, unsigned long address)
+do_sigbus(struct pt_regs *regs, unsigned long error_code, unsigned long address,
+	  unsigned int fault)
 {
 	struct task_struct *tsk = current;
 	struct mm_struct *mm = tsk->mm;
+	int code = BUS_ADRERR;
 
 	up_read(&mm->mmap_sem);
 

@@ -816,7 +819,15 @@
 	tsk->thread.error_code	= error_code;
 	tsk->thread.trap_no	= 14;

arch/x86/mm/fault.c +1 -0

+	force_sig_info_fault(SIGBUS, code, address, tsk);
 }
 
 static noinline void

Review

Quoted 9 lines · Andi Kleen

(quoted diff) +8 -1

-	force_sig_info_fault(SIGBUS, BUS_ADRERR, address, tsk);
+#ifdef CONFIG_MEMORY_FAILURE
+	if (fault & VM_FAULT_HWPOISON) {
+		printk(KERN_ERR
+	"MCE: Killing %s:%d due to hardware memory corruption fault at %lx\n",
+			tsk->comm, tsk->pid, address);
+		code = BUS_MCEERR_AR;
+	}
+#endif

If you make VM_FAULT_HWPOISON 0 when !CONFIG_MEMORY_FAILURE, then you can remove this ifdef, can't you?

try_to_unmap currently has multiple modi (migration, munlock, normal unmap) which are selected by magic flag variables. The logic is not very straight forward, because each of these flag change multiple behaviours (e.g. migration turns off aging, not only sets up migration ptes etc.) Also the different flags interact in magic ways.

A later patch in this series adds another mode to try_to_unmap, so this becomes quickly unmanageable.

Replace the different flags with a action code (migration, munlock, munmap) and some additional flags as modifiers (ignore mlock, ignore aging). This makes the logic more straight forward and allows easier extension to new behaviours. Change all the caller to declare what they want to do.

This patch is supposed to be a nop in behaviour. If anyone can prove it is not that would be a bug.

Cc [email protected]

Cc [email protected]

Signed-off-by Andi Kleen <[email protected]>

---
 include/linux/rmap.h |   14 +++++++++++++-
 mm/migrate.c         |    2 +-
 mm/rmap.c            |   40 ++++++++++++++++++++++------------------
 mm/vmscan.c          |    2 +-
 4 files changed, 37 insertions(+), 21 deletions(-)

Index: linux/include/linux/rmap.h
===================================================================

include/linux/rmap.h +1 -1

@@ -84,7 +84,19 @@
  * Called from mm/vmscan.c to handle paging out
  */
 int page_referenced(struct page *, int is_locked, struct mem_cgroup *cnt);
-int try_to_unmap(struct page *, int ignore_refs);
+

Quoted 45 lines · Andi Kleen

try_to_unmap currently has multiple modi (migration, munlock, normal unmap) which are selected by magic flag variables. The logic is not very straight forward, because each of these flag change multiple behaviours (e.g. migration turns off aging, not only sets up migration ptes etc.) Also the different flags interact in magic ways.

A later patch in this series adds another mode to try_to_unmap, so this becomes quickly unmanageable.

Replace the different flags with a action code (migration, munlock, munmap) and some additional flags as modifiers (ignore mlock, ignore aging). This makes the logic more straight forward and allows easier extension to new behaviours. Change all the caller to declare what they want to do.

This patch is supposed to be a nop in behaviour. If anyone can prove it is not that would be a bug.

Cc [email protected]

Cc [email protected]

Signed-off-by Andi Kleen <[email protected]>

---
 include/linux/rmap.h |   14 +++++++++++++-
 mm/migrate.c         |    2 +-
 mm/rmap.c            |   40 ++++++++++++++++++++++------------------
 mm/vmscan.c          |    2 +-
 4 files changed, 37 insertions(+), 21 deletions(-)

Index: linux/include/linux/rmap.h
===================================================================

include/linux/rmap.h +5 -1

@@ -84,7 +84,19 @@
  * Called from mm/vmscan.c to handle paging out
  */
 int page_referenced(struct page *, int is_locked, struct mem_cgroup *cnt);
-int try_to_unmap(struct page *, int ignore_refs);
+
+enum ttu_flags {
+	TTU_UNMAP = 0,			/* unmap mode */
+	TTU_MIGRATION = 1,		/* migration mode */
+	TTU_MUNLOCK = 2,		/* munlock mode */

include/linux/rmap.h line 97

Quoted 6 lines

include/linux/rmap.h +6 -0

+	TTU_ACTION_MASK = 0xff,
+
+	TTU_IGNORE_MLOCK = (1 << 8),	/* ignore mlock */
+	TTU_IGNORE_ACCESS = (1 << 9),	/* don't age */
+};
+#define TTU_ACTION(x) ((x) & TTU_ACTION_MASK)

I still think this is nasty and should work like Gfp flags.

Quoted 46 lines · Nick Piggin

Quoted 45 lines · Andi Kleen

try_to_unmap currently has multiple modi (migration, munlock, normal unmap) which are selected by magic flag variables. The logic is not very straight forward, because each of these flag change multiple behaviours (e.g. migration turns off aging, not only sets up migration ptes etc.) Also the different flags interact in magic ways.

A later patch in this series adds another mode to try_to_unmap, so this becomes quickly unmanageable.

Replace the different flags with a action code (migration, munlock, munmap) and some additional flags as modifiers (ignore mlock, ignore aging). This makes the logic more straight forward and allows easier extension to new behaviours. Change all the caller to declare what they want to do.

This patch is supposed to be a nop in behaviour. If anyone can prove it is not that would be a bug.

Cc [email protected]

Cc [email protected]

Signed-off-by Andi Kleen <[email protected]>

---
 include/linux/rmap.h |   14 +++++++++++++-
 mm/migrate.c         |    2 +-
 mm/rmap.c            |   40 ++++++++++++++++++++++------------------
 mm/vmscan.c          |    2 +-
 4 files changed, 37 insertions(+), 21 deletions(-)

Index: linux/include/linux/rmap.h
===================================================================

include/linux/rmap.h +5 -1

@@ -84,7 +84,19 @@
  * Called from mm/vmscan.c to handle paging out
  */
 int page_referenced(struct page *, int is_locked, struct mem_cgroup *cnt);
-int try_to_unmap(struct page *, int ignore_refs);
+
+enum ttu_flags {
+	TTU_UNMAP = 0,			/* unmap mode */
+	TTU_MIGRATION = 1,		/* migration mode */
+	TTU_MUNLOCK = 2,		/* munlock mode */

Quoted 8 lines

include/linux/rmap.h line 97

Quoted 6 lines

include/linux/rmap.h +6 -0

+	TTU_ACTION_MASK = 0xff,
+
+	TTU_IGNORE_MLOCK = (1 << 8),	/* ignore mlock */
+	TTU_IGNORE_ACCESS = (1 << 9),	/* don't age */
+};
+#define TTU_ACTION(x) ((x) & TTU_ACTION_MASK)

I still think this is nasty and should work like Gfp flags.

I don't see big problems here.

We have page_zone() and gfp_zone(), so why not TTU_ACTION()? We could allocate one bit for each action code, but in principle they are exclusive.

When a page has the poison bit set replace the PTE with a poison entry. This causes the right error handling to be done later when a process runs into it.

Also add a new flag to not do that (needed for the memory-failure handler later)

Signed-off-by Andi Kleen <[email protected]>

---
 include/linux/rmap.h |    1 +
 mm/rmap.c            |    9 ++++++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

Index: linux/mm/rmap.c
===================================================================

mm/rmap.c +8 -1

@@ -943,7 +943,14 @@
 	/* Update high watermark before we lower rss */
 	update_hiwater_rss(mm);
 
-	if (PageAnon(page)) {
+	if (PageHWPoison(page) && !(flags & TTU_IGNORE_HWPOISON)) {
+		if (PageAnon(page))
+			dec_mm_counter(mm, anon_rss);
+		else if (!is_migration_entry(pte_to_swp_entry(*pte)))
+			dec_mm_counter(mm, file_rss);
+		set_pte_at(mm, address, pte,
+				swp_entry_to_pte(make_hwpoison_entry(page)));
+	} else if (PageAnon(page)) {
 		swp_entry_t entry = { .val = page_private(page) };
 
 		if (PageSwapCache(page)) {

Index: linux/include/linux/rmap.h
===================================================================

include/linux/rmap.h line 96

Quoted 9 lines

Index: linux/include/linux/rmap.h
===================================================================

include/linux/rmap.h +1 -0

@@ -93,6 +93,7 @@
 
 	TTU_IGNORE_MLOCK = (1 << 8),	/* ignore mlock */
 	TTU_IGNORE_ACCESS = (1 << 9),	/* don't age */
+	TTU_IGNORE_HWPOISON = (1 << 10),/* corrupted page is recoverable */

Or more precisely comment it as "corrupted data is recoverable"?

Bail out early in set_page_dirty for poisoned pages. We don't want any of the dirty accounting done or file system write back started, because the page will be just thrown away.

---
 mm/page-writeback.c |    4 ++++
 1 file changed, 4 insertions(+)

Index: linux/mm/page-writeback.c
===================================================================

Quoted 4 lines · Andi Kleen

Bail out early in set_page_dirty for poisoned pages. We don't want any of the dirty accounting done or file system write back started, because the page will be just thrown away.

I'm afraid this patch is not necessary and could be harmful.

It is not necessary because a poisoned page will normally already be isolated from page cache, or likely cannot be isolated because it has dirty buffers.

It is harmful because it put the page into dirty state without queuing it for IO by moving it to s_io. When more normal pages are dirtied later, __set_page_dirty_nobuffers() won't move the inode into s_io, hence delaying the writeback of good pages for arbitrary long time.

Bail out early in set_page_dirty for poisoned pages. We don't want any of the dirty accounting done or file system write back started, because the page will be just thrown away.

Signed-off-by Andi Kleen <[email protected]>

---
 mm/page-writeback.c |    4 ++++
 1 file changed, 4 insertions(+)

Index: linux/mm/page-writeback.c
===================================================================

Quoted 4 lines · Andi Kleen

Bail out early in set_page_dirty for poisoned pages. We don't want any of the dirty accounting done or file system write back started, because the page will be just thrown away.

I don't agree with adding overhead to fastpaths like this. Your MCE handler should have already taken care of this so I can't see what it can gain.

Quoted 15 lines

Signed-off-by Andi Kleen <[email protected]>

---
 mm/page-writeback.c |    4 ++++
 1 file changed, 4 insertions(+)

Index: linux/mm/page-writeback.c
===================================================================

mm/page-writeback.c +0 -0

@@ -1304,6 +1304,10 @@
 {
 	struct address_space *mapping = page_mapping(page);
 

mm/page-writeback.c line 1313

Quoted 7 lines

mm/page-writeback.c +4 -0

+	if (unlikely(PageHWPoison(page))) {
+		SetPageDirty(page);
+		return 0;
+	}
 	if (likely(mapping)) {
 		int (*spd)(struct page *) = mapping->a_ops->set_page_dirty;
 #ifdef CONFIG_BLOCK

From: Wu Fengguang <[email protected]>

If memory corruption hits the free buddy pages, we can safely ignore them. No one will access them until page allocation time, then prep_new_page() will automatically check and isolate PG_hwpoison page for us (for 0-order allocation).

This patch expands prep_new_page() to check every component page in a high order page allocation, in order to completely stop PG_hwpoison pages from being recirculated.

Note that the common case -- only allocating a single page, doesn't do any more work than before. Allocating > order 0 does a bit more work, but that's relatively uncommon.

This simple implementation may drop some innocent neighbor pages, hopefully it is not a big problem because the event should be rare enough.

This patch adds some runtime costs to high order page users.

[AK: Improved description]

v2: Andi Kleen:
Port to -mm code
Move check into separate function.
Don't dump stack in bad_pages for hwpoisoned pages.

Signed-off-by Wu Fengguang <[email protected]>

Signed-off-by Andi Kleen <[email protected]>

---
 mm/page_alloc.c |   20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

Index: linux/mm/page_alloc.c
===================================================================

mm/page_alloc.c +12 -1

@@ -237,6 +237,12 @@
 	static unsigned long nr_shown;
 	static unsigned long nr_unshown;
 
+	/* Don't complain about poisoned pages */
+	if (PageHWPoison(page)) {
+		__ClearPageBuddy(page);
+		return;
+	}
+
 	/*
 	 * Allow a burst of 60 reports, then keep quiet for that minute;
 	 * or allow a steady drip of one report per second.

@@ -650,7 +656,7 @@
 /*
  * This page is about to be returned from the page allocator
  */
-static int prep_new_page(struct page *page, int order, gfp_t gfp_flags)
+static inline int check_new_page(struct page *page)
 {
 	if (unlikely(page_mapcount(page) |
 		(page->mapping != NULL)  |

@@ -659,6 +665,18 @@
 		bad_page(page);
 		return 1;
 	}
+	return 0;
+}
+
+static int prep_new_page(struct page *page, int order, gfp_t gfp_flags)
+{

Quoted 7 lines · Andi Kleen

From: Wu Fengguang <[email protected]>

If memory corruption hits the free buddy pages, we can safely ignore them. No one will access them until page allocation time, then prep_new_page() will automatically check and isolate PG_hwpoison page for us (for 0-order allocation).

It would be kinda nice if this could be done in the handler directly (ie. take the page directly out of the allocator or pcp list). Completely avoiding fastpaths would be a wonderful goal.

mm/page_alloc.c line 245

Quoted 7 lines

mm/page_alloc.c +6 -0

+	/* Don't complain about poisoned pages */
+	if (PageHWPoison(page)) {
+		__ClearPageBuddy(page);
+		return;
+	}
+

Quoted 29 lines

mm/page_alloc.c +13 -1

 	/*
 	 * Allow a burst of 60 reports, then keep quiet for that minute;
 	 * or allow a steady drip of one report per second.

@@ -650,7 +656,7 @@
 /*
  * This page is about to be returned from the page allocator
  */
-static int prep_new_page(struct page *page, int order, gfp_t gfp_flags)
+static inline int check_new_page(struct page *page)
 {
 	if (unlikely(page_mapcount(page) |
 		(page->mapping != NULL)  |

@@ -659,6 +665,18 @@
 		bad_page(page);
 		return 1;
 	}
+	return 0;
+}
+
+static int prep_new_page(struct page *page, int order, gfp_t gfp_flags)
+{
+	int i;
+
+	for (i = 0; i < (1 << order); i++) {
+		struct page *p = page + i;
+		if (unlikely(check_new_page(p)))
+			return 1;
+	}
 

mm/page_alloc.c line 682

Quoted 2 lines

mm/page_alloc.c +0 -0

 	set_page_private(page, 0);
 	set_page_refcounted(page);

Quoted 9 lines · Nick Piggin

Quoted 7 lines · Andi Kleen

From: Wu Fengguang <[email protected]>

If memory corruption hits the free buddy pages, we can safely ignore them. No one will access them until page allocation time, then prep_new_page() will automatically check and isolate PG_hwpoison page for us (for 0-order allocation).

Quoted 4 lines

It would be kinda nice if this could be done in the handler directly (ie. take the page directly out of the allocator or pcp list). Completely avoiding fastpaths would be a wonderful goal.

In fact Andi have code to do that. We prefer this one because
- it's simple
- it's good sanity check for possible software BUGs
- it mainly adds overhead to high order pages, which is acceptable

Thanks,
Fengguang

Quoted 7 lines

mm/page_alloc.c line 245

Quoted 7 lines

mm/page_alloc.c +6 -0

+	/* Don't complain about poisoned pages */
+	if (PageHWPoison(page)) {
+		__ClearPageBuddy(page);
+		return;
+	}
+

I do think the above chunk is not absolutely necessary, though.

Thanks,
Fengguang

Quoted 29 lines

Quoted 29 lines

mm/page_alloc.c +13 -1

 	/*
 	 * Allow a burst of 60 reports, then keep quiet for that minute;
 	 * or allow a steady drip of one report per second.

@@ -650,7 +656,7 @@
 /*
  * This page is about to be returned from the page allocator
  */
-static int prep_new_page(struct page *page, int order, gfp_t gfp_flags)
+static inline int check_new_page(struct page *page)
 {
 	if (unlikely(page_mapcount(page) |
 		(page->mapping != NULL)  |

@@ -659,6 +665,18 @@
 		bad_page(page);
 		return 1;
 	}
+	return 0;
+}
+
+static int prep_new_page(struct page *page, int order, gfp_t gfp_flags)
+{
+	int i;
+
+	for (i = 0; i < (1 << order); i++) {
+		struct page *p = page + i;
+		if (unlikely(check_new_page(p)))
+			return 1;
+	}
 

Quoted 2 lines

mm/page_alloc.c line 682

Quoted 2 lines

mm/page_alloc.c +0 -0

 	set_page_private(page, 0);
 	set_page_refcounted(page);

Quoted 15 lines · Wu Fengguang

Quoted 9 lines · Nick Piggin

Quoted 7 lines · Andi Kleen

From: Wu Fengguang <[email protected]>

If memory corruption hits the free buddy pages, we can safely ignore them. No one will access them until page allocation time, then prep_new_page() will automatically check and isolate PG_hwpoison page for us (for 0-order allocation).

Quoted 4 lines

It would be kinda nice if this could be done in the handler directly (ie. take the page directly out of the allocator or pcp list). Completely avoiding fastpaths would be a wonderful goal.

Quoted 4 lines

In fact Andi have code to do that. We prefer this one because
- it's simple
- it's good sanity check for possible software BUGs
- it mainly adds overhead to high order pages, which is acceptable

Yeah it's not bad. But we don't have much other non-debug options that check for random memory corruption like this. Given that the struct page is a very tiny proportion of memory, then I'm not totally convinced that all this checking in the page allocator is worthwhile for everyone. It's a much bigger cost if checks and branches have to be there just for hwpoison.

And I don't think removing a free page from the page allocator is too much more complex than removing a live page from the pagecache ;)

Quoted 48 lines

Thanks,
Fengguang

Quoted 7 lines

mm/page_alloc.c line 245

Quoted 7 lines

mm/page_alloc.c +6 -0

+	/* Don't complain about poisoned pages */
+	if (PageHWPoison(page)) {
+		__ClearPageBuddy(page);
+		return;
+	}
+

I do think the above chunk is not absolutely necessary, though.

Thanks,
Fengguang

Quoted 29 lines

Quoted 29 lines

mm/page_alloc.c +13 -1

 	/*
 	 * Allow a burst of 60 reports, then keep quiet for that minute;
 	 * or allow a steady drip of one report per second.

@@ -650,7 +656,7 @@
 /*
  * This page is about to be returned from the page allocator
  */
-static int prep_new_page(struct page *page, int order, gfp_t gfp_flags)
+static inline int check_new_page(struct page *page)
 {
 	if (unlikely(page_mapcount(page) |
 		(page->mapping != NULL)  |

@@ -659,6 +665,18 @@
 		bad_page(page);
 		return 1;
 	}
+	return 0;
+}
+
+static int prep_new_page(struct page *page, int order, gfp_t gfp_flags)
+{
+	int i;
+
+	for (i = 0; i < (1 << order); i++) {
+		struct page *p = page + i;
+		if (unlikely(check_new_page(p)))
+			return 1;
+	}
 

Quoted 2 lines

Quoted 2 lines

mm/page_alloc.c line 682

Quoted 2 lines

mm/page_alloc.c +0 -0

 	set_page_private(page, 0);
 	set_page_refcounted(page);

Quoted 2 lines · Nick Piggin

And I don't think removing a free page from the page allocator is too much more complex than removing a live page from the pagecache ;)

There are usable functions for doing pagecache isolations, but no one to isolate one specific page from the buddy system.

Plus, if we did present such a function, you'll then ask for it being included in page_alloc.c, injecting a big chunk of dead code into the really hot code blocks and possibly polluting the L2 cache. Will it be

better than just inserting several lines? Hardly. Smaller text itself yields faster speed.

Quoted 6 lines · Wu Fengguang

Quoted 2 lines · Nick Piggin

And I don't think removing a free page from the page allocator is too much more complex than removing a live page from the pagecache ;)

There are usable functions for doing pagecache isolations, but no one to isolate one specific page from the buddy system.

But it shouldn't be too hard. Anyway you wanted to reinvent your own functions for pagecache isolations ;)

Quoted 3 lines

Plus, if we did present such a function, you'll then ask for it being included in page_alloc.c, injecting a big chunk of dead code into the really hot code blocks and possibly polluting the L2 cache. Will it be

But you would say no because you like it better in your memory isolation file ;)

Quoted 2 lines

better than just inserting several lines? Hardly. Smaller text itself yields faster speed.

Oh speed I'm definitely thinking about, don't worry about that.

Moving hot and cold functions together could become an issue indeed. Mostly it probably matters a little less than code within a single function due to their size. But I think gcc already has options to annotate this kind of thing which we could be using.

So it's not such a good argument against moving things out of hotpaths, or guiding in which files to place functions.

Anyway, in this case it is not a "nack" from me. Just that I would like to see the non-fastpath code too or at least if it can be thought about.

Quoted 5 lines

Moving hot and cold functions together could become an issue indeed. Mostly it probably matters a little less than code within a single function due to their size. But I think gcc already has options to annotate this kind of thing which we could be using.

Can we tell gcc "I bet this _function_ is rarely used"?

Quoted 2 lines

So it's not such a good argument against moving things out of hotpaths, or guiding in which files to place functions.

Yes.

Quoted 3 lines

Anyway, in this case it is not a "nack" from me. Just that I would like to see the non-fastpath code too or at least if it can be thought about.

I think Andi would be pleased to present you with his buddy page isolation code :)

From: Nick Piggin <[email protected]>

Extract out truncate_inode_page() out of the truncate path so that it can be used by memory-failure.c

[AK: description, headers, fix typos]

Signed-off-by Andi Kleen <[email protected]>

---
 include/linux/mm.h |    2 ++
 mm/truncate.c      |   24 ++++++++++++------------
 2 files changed, 14 insertions(+), 12 deletions(-)

Index: linux/mm/truncate.c
===================================================================

mm/truncate.c +0 -0

@@ -135,6 +135,16 @@
 	return ret;
 }
 

Quoted 4 lines

(quoted diff) +4 -0

+void truncate_inode_page(struct address_space *mapping, struct page *page)
+{
+	if (page_mapped(page)) {
+		unmap_mapping_range(mapping,

Review

Quoted 6 lines

(quoted diff) +6 -0

+		  (loff_t)page->index<<PAGE_CACHE_SHIFT,
+		  PAGE_CACHE_SIZE, 0);
+	}
+	truncate_complete_page(mapping, page);
+}
+

Small style cleanup:

(quoted diff) +9 -0

+/*
+ * Clean (or cleaned) page cache page.
+ */
+static int me_pagecache_clean(struct page *p, unsigned long pfn)
+{
+       struct address_space *mapping;
+
+       if (!isolate_lru_page(p))
+               page_cache_release(p);

(quoted diff) +21 -0

+               printk(KERN_ERR
+                       "MCE %#lx: page looks like a unsupported file system metadata page\n",
+                       pfn);
+               return FAILED;
+       }
+       if (mapping) {
+               truncate_inode_page(mapping, p);
+               if (page_has_private(p) && !try_to_release_page(p, GFP_NOIO)) {
+                       pr_debug(KERN_ERR "MCE %#lx: failed to release buffers\n",
+                               pfn);
+                       return FAILED;
+               }
+       }
+       return RECOVERED;
+}
+
+/*
+ * Dirty cache page page
+ * Issues: when the error hit a hole page the error is not properly
+ * propagated.
+ */

(quoted diff) +6 -0

+                *
+                * This has the disadvantage that it gets cleared on
+                * the first operation that returns an error, while
+                * the PageError bit is more sticky and only cleared
+                * when the page is reread or dropped.  If an
+                * application assumes it will always get error on

(quoted diff) +6 -0

+                * the first EIO, but we're not worse than other parts
+                * of the kernel.
+                */
+               mapping_set_error(mapping, EIO);
+       }
+

+ * try_to_unmap(TTU_IGNORE_HWPOISON) to convert the normal PTEs to swap PTEs,
+ * and then

+ *      - clear dirty bit to prevent IO
+ *      - remove from LRU
+ *      - but keep in the swap cache, so that when we return to it on
+ *        a later page fault, we know the application is accessing
+ *        corrupted data and shall be killed (we installed simple
+ *        interception code in do_swap_page to catch it).

+ *
+ * Clean swap cache pages can be directly isolated. A later page fault will
+ * bring in the known good data from disk.
+ */

(quoted diff) +97 -0

+
+       if (!isolate_lru_page(p))
+               page_cache_release(p);
+
+       return DELAYED;
+}
+
+static int me_swapcache_clean(struct page *p, unsigned long pfn)
+{
+       if (!isolate_lru_page(p))
+               page_cache_release(p);
+
+       delete_from_swap_cache(p);
+
+       return RECOVERED;
+}
+
+/*
+ * Huge pages. Needs work.
+ * Issues:
+ * No rmap support so we cannot find the original mapper. In theory could walk
+ * all MMs and look for the mappings, but that would be non atomic and racy.
+ * Need rmap for hugepages for this. Alternatively we could employ a heuristic,
+ * like just walking the current process and hoping it has it mapped (that
+ * should be usually true for the common "shared database cache" case)
+ * Should handle free huge pages and dequeue them too, but this needs to
+ * handle huge page accounting correctly.
+ */
+static int me_huge_page(struct page *p, unsigned long pfn)
+{
+       return FAILED;
+}
+
+/*
+ * Various page states we can handle.
+ *
+ * A page state is defined by its current page->flags bits.
+ * The table matches them in order and calls the right handler.
+ *
+ * This is quite tricky because we can access page at any time
+ * in its live cycle, so all accesses have to be extremly careful.
+ *
+ * This is not complete. More states could be added.
+ * For any missing state don't attempt recovery.
+ */
+
+#define dirty          (1UL << PG_dirty)
+#define sc             (1UL << PG_swapcache)
+#define unevict                (1UL << PG_unevictable)
+#define mlock          (1UL << PG_mlocked)
+#define writeback      (1UL << PG_writeback)
+#define lru            (1UL << PG_lru)
+#define swapbacked     (1UL << PG_swapbacked)
+#define head           (1UL << PG_head)
+#define tail           (1UL << PG_tail)
+#define compound       (1UL << PG_compound)
+#define slab           (1UL << PG_slab)
+#define buddy          (1UL << PG_buddy)
+#define reserved       (1UL << PG_reserved)
+
+static struct page_state {
+       unsigned long mask;
+       unsigned long res;
+       char *msg;
+       int (*action)(struct page *p, unsigned long pfn);
+} error_states[] = {
+       { reserved,     reserved,       "reserved kernel",      me_ignore },
+       { buddy,        buddy,          "free kernel",  me_free },
+
+       /*
+        * Could in theory check if slab page is free or if we can drop
+        * currently unused objects without touching them. But just
+        * treat it as standard kernel for now.
+        */
+       { slab,         slab,           "kernel slab",  me_kernel },
+
+#ifdef CONFIG_PAGEFLAGS_EXTENDED
+       { head,         head,           "huge",         me_huge_page },
+       { tail,         tail,           "huge",         me_huge_page },
+#else
+       { compound,     compound,       "huge",         me_huge_page },
+#endif
+
+       { sc|dirty,     sc|dirty,       "swapcache",    me_swapcache_dirty },
+       { sc|dirty,     sc,             "swapcache",    me_swapcache_clean },
+
+#ifdef CONFIG_UNEVICTABLE_LRU
+       { unevict|dirty, unevict|dirty, "unevictable LRU", me_pagecache_dirty},
+       { unevict,      unevict,        "unevictable LRU", me_pagecache_clean},
+#endif
+
+#ifdef CONFIG_HAVE_MLOCKED_PAGE_BIT
+       { mlock|dirty,  mlock|dirty,    "mlocked LRU",  me_pagecache_dirty },
+       { mlock,        mlock,          "mlocked LRU",  me_pagecache_clean },
+#endif
+
+       { lru|dirty,    lru|dirty,      "LRU",          me_pagecache_dirty },

(quoted diff) +44 -0

+       if (page_count(p) != 1)
+               printk(KERN_ERR
+                      "MCE %#lx: %s page still referenced by %d users\n",
+                      pfn, ps->msg, page_count(p) - 1);
+
+       /* Could do more checks here if page looks ok */
+       atomic_long_add(1, &mce_bad_pages);
+
+       /*
+        * Could adjust zone counters here to correct for the missing page.
+        */
+}
+
+#define N_UNMAP_TRIES 5
+
+/*
+ * Do all that is necessary to remove user space mappings. Unmap
+ * the pages and send SIGBUS to the processes if the data was dirty.
+ */
+static void hwpoison_user_mappings(struct page *p, unsigned long pfn,
+                                 int trapno)
+{
+       enum ttu_flags ttu = TTU_UNMAP | TTU_IGNORE_MLOCK | TTU_IGNORE_ACCESS;
+       int kill = sysctl_memory_failure_early_kill;
+       struct address_space *mapping;
+       LIST_HEAD(tokill);
+       int ret;
+       int i;
+
+       if (PageReserved(p) || PageCompound(p) || PageSlab(p))
+               return;
+
+       if (!PageLRU(p))
+               lru_add_drain();
+
+       /*
+        * This check implies we don't kill processes if their pages
+        * are in the swap cache early. Those are always late kills.
+        */
+       if (!page_mapped(p))
+               return;
+
+       if (PageSwapCache(p)) {
+               printk(KERN_ERR

(quoted diff) +18 -0

+               }
+       }
+
+       /*
+        * First collect all the processes that have the page
+        * mapped.  This has to be done before try_to_unmap,
+        * because ttu takes the rmap data structures down.
+        *
+        * This also has the side effect to propagate the dirty
+        * bit from PTEs into the struct page. This is needed
+        * to actually decide if something needs to be killed
+        * or errored, or if it's ok to just drop the page.
+        *
+        * Error handling: We ignore errors here because
+        * there's nothing that can be done.
+        */
+       if (kill)
+               collect_procs(p, &tokill);

(quoted diff) +33 -0

+       }
+
+       /*
+        * Now that the dirty bit has been propagated to the
+        * struct page and all unmaps done we can decide if
+        * killing is needed or not.  Only kill when the page
+        * was dirty, otherwise the tokill list is merely
+        * freed.  When there was a problem unmapping earlier
+        * use a more force-full uncatchable kill to prevent
+        * any accesses to the poisoned memory.
+        */
+       kill_procs_ao(&tokill, !!PageDirty(p), trapno,
+                     ret != SWAP_SUCCESS, pfn);
+}
+
+/**
+ * memory_failure - Handle memory failure of a page.
+ * @pfn: Page Number of the corrupted page
+ * @trapno: Trap number reported in the signal to user space.
+ *
+ * This function is called by the low level machine check code
+ * of an architecture when it detects hardware memory corruption
+ * of a page. It tries its best to recover, which includes
+ * dropping pages, killing processes etc.
+ *
+ * The function is primarily of use for corruptions that
+ * happen outside the current execution context (e.g. when
+ * detected by a background scrubber)
+ *
+ * Must run in process context (e.g. a work queue) with interrupts
+ * enabled and no spinlocks hold.
+ */
+void memory_failure(unsigned long pfn, int trapno)

(quoted diff) +30 -0

+       p = pfn_to_page(pfn);
+       if (TestSetPageHWPoison(p)) {
+               action_result(pfn, "already hardware poisoned", IGNORED);
+               return;
+       }
+
+       /*
+        * We need/can do nothing about count=0 pages.
+        * 1) it's a free page, and therefore in safe hand:
+        *    prep_new_page() will be the gate keeper.
+        * 2) it's part of a non-compound high order page.
+        *    Implies some kernel user: cannot stop them from
+        *    R/W the page; let's pray that the page has been
+        *    used and will be freed some time later.
+        * In fact it's dangerous to directly bump up page count from 0,
+        * that may make page_freeze_refs()/page_unfreeze_refs() mismatch.
+        */
+       if (!get_page_unless_zero(compound_head(p))) {
+               action_result(pfn, "free or high order kernel", IGNORED);
+               return;
+       }
+
+       /*
+        * Lock the page and wait for writeback to finish.
+        * It's very difficult to mess with pages currently under IO
+        * and in many cases impossible, so we just avoid it here.
+        */
+       lock_page_nosync(p);
+       wait_on_page_writeback(p);
+

(quoted diff) +12 -0

+               goto out;
+       }
+
+       for (ps = error_states;; ps++) {
+               if ((p->flags & ps->mask) == ps->res) {
+                       page_action(ps, p, pfn);
+                       break;
+               }
+       }
+out:
+       unlock_page(p);
+}

Index: linux/include/linux/mm.h
===================================================================

include/linux/mm.h 2009-06-03 20:13:43.000000000 +0200 renamed from include/linux/mm.h 2009-06-03 20:13:43.000000000 +0200 +5 -0

@@ -1326,5 +1326,10 @@
 extern int account_locked_memory(struct mm_struct *mm, struct rlimit *rlim,
                                 size_t size);
 extern void refund_locked_memory(struct mm_struct *mm, size_t size);
+
+extern void memory_failure(unsigned long pfn, int trapno);
+extern int sysctl_memory_failure_early_kill;
+extern atomic_long_t mce_bad_pages;
+
 #endif /* __KERNEL__ */
 #endif /* _LINUX_MM_H */

Index: linux/kernel/sysctl.c
===================================================================

kernel/sysctl.c 2009-06-03 20:13:43.000000000 +0200 renamed from kernel/sysctl.c 2009-06-03 19:37:38.000000000 +0200 +8 -0

+               .mode           = 0644,
+               .proc_handler   = &proc_dointvec_minmax,
+               .strategy       = &sysctl_intvec,
+               .extra1         = &zero,
+               .extra2         = &one,
+       },
+#endif
+
 /*
  * NOTE: do not add new entries to this table unless you have read
  * Documentation/sysctl/ctl_unnumbered.txt

Index: linux/fs/proc/meminfo.c
===================================================================

fs/proc/meminfo.c 2009-06-03 20:13:43.000000000 +0200 renamed from fs/proc/meminfo.c 2009-06-03 19:37:38.000000000 +0200 +1 -0

+#endif
                );
 
        hugetlb_report_meminfo(m);

Index: linux/mm/Kconfig
===================================================================

mm/Kconfig 2009-06-03 20:39:48.000000000 +0200 renamed from mm/Kconfig 2009-06-03 19:37:38.000000000 +0200 +0 -0

 config NOMMU_INITIAL_TRIM_EXCESS
        int "Turn on mmap() excess space trimming before booting"
        depends on !MMU

Index: linux/Documentation/sysctl/vm.txt
===================================================================

Documentation/sysctl/vm.txt 2009-06-03 20:13:43.000000000 +0200 renamed from Documentation/sysctl/vm.txt 2009-06-03 19:37:38.000000000 +0200 +3 -1

@@ -32,6 +32,7 @@
 - legacy_va_layout
 - lowmem_reserve_ratio
 - max_map_count
+- memory_failure_early_kill
 - min_free_kbytes
 - min_slab_ratio
 - min_unmapped_ratio

@@ -53,7 +54,6 @@
 - vfs_cache_pressure
 - zone_reclaim_mode
 
-
 ==============================================================
 
 block_dump

@@ -275,6 +275,25 @@
 
 The default value is 65536.
 
+=============================================================
+

Documentation/sysctl/vm.txt 2009-06-03 20:13:43.000000000 +0200 renamed from Documentation/sysctl/vm.txt 2009-06-03 19:37:38.000000000 +0200 +6 -0

+The kill is done using a catchable SIGBUS, so processes can handle this
+if they want to.
+
+This is only active on architectures/platforms with advanced machine
+check handling and depends on the hardware capabilities.
+
 ==============================================================
 
 min_free_kbytes:

Index: linux/mm/filemap.c
===================================================================

mm/filemap.c 2009-06-03 20:13:43.000000000 +0200 renamed from mm/filemap.c 2009-06-03 19:37:38.000000000 +0200 +4 -0

@@ -105,6 +105,10 @@
  *
  *  ->task->proc_lock
  *    ->dcache_lock            (proc_pid_lookup)
+ *
+ *  (code doesn't rely on that order, so you could switch it around)
+ *  ->tasklist_lock             (memory_failure, collect_procs_ao)
+ *    ->i_mmap_lock
  */
 
 /*

Index: linux/mm/rmap.c
===================================================================

On Thu, Jun 04, 2009 at 02:46:47AM +0800, Andi Kleen wrote:

[snip]

This patch is full of this style error (the old version didn't have this problem though):

ERROR: code indent should use tabs where possible

Review

Quoted 11 lines

(quoted diff) +11 -0

+               page_cache_release(p);
+
+       /*
+        * Now truncate the page in the page cache. This is really
+        * more like a "temporary hole punch"
+        * Don't do this for block devices when someone else
+        * has a reference, because it could be file system metadata
+        * and that's not safe to truncate.
+        */
+       mapping = page_mapping(p);
+       if (mapping && S_ISBLK(mapping->host->i_mode) && page_count(p) > 1) {

Shall use (page_count > 2) to count for the page cache reference.

Or can we base the test on busy buffers instead of page count? Nick?

Review

Quoted 6 lines

(quoted diff) +6 -0

+       SetPageError(p);
+       /* TBD: print more information about the file. */
+       if (mapping) {
+               /*
+                * IO error will be reported by write(), fsync(), etc.
+                * who check the mapping.

btw, here are some side notes on EIO.

close() *may* also report it. NFS will sync file on close.

Review

Quoted 3 lines

(quoted diff) +3 -0

+                * at the wrong time.
+                *
+                * So right now we assume that the application DTRT on

DTRT = do the return value test?

Review

Quoted 3 lines

(quoted diff) +3 -0

+};
+
+static void action_result(unsigned long pfn, char *msg, int ret)

rename 'ret' to 'action'?

Review

Quoted 4 lines

(quoted diff) +4 -0

+        * need this to decide if we should kill or just drop the page.
+        */
+       mapping = page_mapping(p);
+       if (!PageDirty(p) && !PageAnon(p) && !PageSwapBacked(p) &&

!PageAnon(p) could be removed: the below non-zero mapping check will do the work implicitly.

Review

Quoted 5 lines

(quoted diff) +5 -0

+                       kill = 0;
+                       printk(KERN_INFO
+       "MCE %#lx: corrupted page was clean: dropped without side effects\n",
+                               pfn);
+                       ttu |= TTU_IGNORE_HWPOISON;

Why not put the two assignment lines together? :)

Review

Quoted 7 lines

(quoted diff) +7 -0

+        * Try a few times (RED-PEN better strategy?)
+        */
+       for (i = 0; i < N_UNMAP_TRIES; i++) {
+               ret = try_to_unmap(p, ttu);
+               if (ret == SWAP_SUCCESS)
+                       break;
+               pr_debug("MCE %#lx: try_to_unmap retry needed %d\n", pfn,  ret);

Can we make it a printk? This is a serious accident.

Review

Quoted 8 lines

(quoted diff) +8 -0

+        */
+       hwpoison_user_mappings(p, pfn, trapno);
+
+       /*
+        * Torn down by someone else?
+        */
+       if (PageLRU(p) && !PageSwapCache(p) && p->mapping == NULL) {
+               action_result(pfn, "already unmapped LRU", IGNORED);

"NULL mapping LRU" or "already truncated page"?
At least page_mapped != page_mapping.

Quoted 4 lines

(quoted diff) +0 -0

@@ -1311,6 +1311,20 @@
                .mode           = 0644,
                .proc_handler   = &scan_unevictable_handler,
        },

Review line 1319

Quoted 6 lines

(quoted diff) +6 -0

+#ifdef CONFIG_MEMORY_FAILURE
+       {
+               .ctl_name       = CTL_UNNUMBERED,
+               .procname       = "memory_failure_early_kill",
+               .data           = &sysctl_memory_failure_early_kill,
+               .maxlen         = sizeof(vm_highmem_is_dirtyable),

s/vm_highmem_is_dirtyable/sysctl_memory_failure_early_kill/

Quoted 8 lines

(quoted diff) +0 -0

  * Documentation/sysctl/ctl_unnumbered.txt

Index: linux/fs/proc/meminfo.c
===================================================================

fs/proc/meminfo.c 2009-06-03 20:13:43.000000000 +0200 renamed from fs/proc/meminfo.c 2009-06-03 19:37:38.000000000 +0200 +0 -0

@@ -95,7 +95,11 @@
                "Committed_AS:   %8lu kB\n"
                "VmallocTotal:   %8lu kB\n"

fs/proc/meminfo.c 2009-06-03 20:13:43.000000000 +0200 line 100

Quoted 5 lines

fs/proc/meminfo.c 2009-06-03 20:13:43.000000000 +0200 renamed from fs/proc/meminfo.c 2009-06-03 19:37:38.000000000 +0200 +3 -1

                "VmallocUsed:    %8lu kB\n"
-               "VmallocChunk:   %8lu kB\n",
+               "VmallocChunk:   %8lu kB\n"
+#ifdef CONFIG_MEMORY_FAILURE
+               "BadPages:       %8lu kB\n"

"HWPoison:" or something like that?
People is more likely to misinterpret "BadPages".

fs/proc/meminfo.c 2009-06-03 20:13:43.000000000 +0200 line 104

Quoted 4 lines

fs/proc/meminfo.c 2009-06-03 20:13:43.000000000 +0200 renamed from fs/proc/meminfo.c 2009-06-03 19:37:38.000000000 +0200 +2 -0

                vmi.used >> 10,
                vmi.largest_chunk >> 10
+#ifdef CONFIG_MEMORY_FAILURE
+               ,atomic_long_read(&mce_bad_pages) << (PAGE_SHIFT - 10)

ERROR: space required after that ','

fs/proc/meminfo.c 2009-06-03 20:13:43.000000000 +0200

Quoted 6 lines

fs/proc/meminfo.c 2009-06-03 20:13:43.000000000 +0200 renamed from fs/proc/meminfo.c 2009-06-03 19:37:38.000000000 +0200 +0 -0

          Enable the KSM kernel module to allow page sharing of equal pages

among different tasks.

fs/proc/meminfo.c 2009-06-03 20:13:43.000000000 +0200 renamed from fs/proc/meminfo.c 2009-06-03 19:37:38.000000000 +0200 +3 -0

+config MEMORY_FAILURE
+       bool
+

Do we have code to automatically enable/disable CONFIG_MEMORY_FAILURE based on hardware capability?

Quoted 6 lines

fs/proc/meminfo.c 2009-06-03 20:13:43.000000000 +0200 renamed from fs/proc/meminfo.c 2009-06-03 19:37:38.000000000 +0200 +6 -0

+
+Control how to kill processes when uncorrected memory error (typically
+a 2bit error in a memory module) is detected in the background by hardware.
+
+1: Kill all processes that have the corrupted page mapped as soon as the
+corruption is detected.

fs/proc/meminfo.c 2009-06-03 20:13:43.000000000 +0200

Quoted 3 lines

fs/proc/meminfo.c 2009-06-03 20:13:43.000000000 +0200 renamed from fs/proc/meminfo.c 2009-06-03 19:37:38.000000000 +0200 +3 -0

+
+0: Only unmap the page from all processes and only kill a process
+who tries to access it.

Note that
- no process will be killed if the page data is clean and can be safely reloaded from disk
- pages in swap cache is always late killed.

Quoted 6 lines · Wu Fengguang

On Thu, Jun 04, 2009 at 02:46:47AM +0800, Andi Kleen wrote:

[snip]

This patch is full of this style error (the old version didn't have this problem though):

I don't see that here. At least nothing new compared to old.

Quoted 2 lines

ERROR: code indent should use tabs where possible

It's checkpath clean for me, except for a few > 80 lines on printks, one list_for_each_entry_safe (which I think checkpatch is wrong on) and the meminfo comma error which I also think checkpath.pl is wrong on too.

Quoted 12 lines

Quoted 11 lines

(quoted diff) +11 -0

+               page_cache_release(p);
+
+       /*
+        * Now truncate the page in the page cache. This is really
+        * more like a "temporary hole punch"
+        * Don't do this for block devices when someone else
+        * has a reference, because it could be file system metadata
+        * and that's not safe to truncate.
+        */
+       mapping = page_mapping(p);
+       if (mapping && S_ISBLK(mapping->host->i_mode) && page_count(p) > 1) {

Quoted 1 line

Shall use (page_count > 2) to count for the page cache reference.

I think the page cache reference got dropped in

	  if (!isolate_lru_page(p))
                page_cache_release(p);

So it should be only one if there are no other users

Quoted 1 line

Or can we base the test on busy buffers instead of page count? Nick?

At least the S_ISBLK test is the best one I came up with. I'm not saying it's the absolutely best.

Quoted 7 lines

Quoted 6 lines

(quoted diff) +6 -0

+       SetPageError(p);
+       /* TBD: print more information about the file. */
+       if (mapping) {
+               /*
+                * IO error will be reported by write(), fsync(), etc.
+                * who check the mapping.

Quoted 3 lines

btw, here are some side notes on EIO.

close() *may* also report it. NFS will sync file on close.

I think the comment is already too verbose, sure there are other details too that it doesn't describe. It's not trying to be a full reference on linux error reporting. So I prefer to not add more cases.

Quoted 5 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+                * at the wrong time.
+                *
+                * So right now we assume that the application DTRT on

DTRT = do the return value test?

Do The Right Thing

Quoted 5 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+};
+
+static void action_result(unsigned long pfn, char *msg, int ret)

rename 'ret' to 'action'?

But's not an action (as in a page state handler), it's a return value? (RECOVERED, FAILED etc.) I can name it result.

Quoted 7 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+        * need this to decide if we should kill or just drop the page.
+        */
+       mapping = page_mapping(p);
+       if (!PageDirty(p) && !PageAnon(p) && !PageSwapBacked(p) &&

!PageAnon(p) could be removed: the below non-zero mapping check will do the work implicitly.

You mean !page_mapped? Ok.

Quoted 7 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+                       kill = 0;
+                       printk(KERN_INFO
+       "MCE %#lx: corrupted page was clean: dropped without side effects\n",
+                               pfn);
+                       ttu |= TTU_IGNORE_HWPOISON;

Why not put the two assignment lines together? :)

Ok. But that was your patch @)

Quoted 8 lines

Quoted 7 lines

(quoted diff) +7 -0

+        * Try a few times (RED-PEN better strategy?)
+        */
+       for (i = 0; i < N_UNMAP_TRIES; i++) {
+               ret = try_to_unmap(p, ttu);
+               if (ret == SWAP_SUCCESS)
+                       break;
+               pr_debug("MCE %#lx: try_to_unmap retry needed %d\n", pfn,  ret);

Quoted 1 line

Can we make it a printk? This is a serious accident.

I think it can actually happen due to races, e.g. when a remap is currently in process.

Quoted 9 lines

Quoted 8 lines

(quoted diff) +8 -0

+        */
+       hwpoison_user_mappings(p, pfn, trapno);
+
+       /*
+        * Torn down by someone else?
+        */
+       if (PageLRU(p) && !PageSwapCache(p) && p->mapping == NULL) {
+               action_result(pfn, "already unmapped LRU", IGNORED);

Quoted 2 lines

"NULL mapping LRU" or "already truncated page"?
At least page_mapped != page_mapping.

It's "already truncated" now.