Avoid unmapping THP when it is failed to be split.

PATCH In review Jin Dongming 2 participants 3 messages Jan 25, 2011 – Jan 27, 2011

Subsystems (2)

Memory management HWPoison memory failure handling

lore

Share & tools

lore

Jin Dongming

author

2011-01-25 05:42 UTC #0 lore raw

PATCH 1/3 Avoid unmapping THP when it is failed to be split.

If the THP is failed to be split,
1. The processes using the poisoned page could not be collected. (Because page_mapped_in_vma() in collect_procs_anon() always returns NULL.)
2. The poisoned page could not be unmapped.
(If CONFIG_DEBUG_VM is "y", VM_BUG_ON(PageTransHuge(page))
in try_to_unmap() will be called, and system panic will be caused.)
3. The processes using the poisoned page could not be killed, too. (Because no process is collected in 1.)

So if splitting THP is failed, it is better to stop unmapping rather than causing panic. System might servive if the page is freed later.

Signed-off-by Jin Dongming <[email protected]>

Reviewed-by Hidetoshi Seto <[email protected]>

---
 mm/memory-failure.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

mm/memory-failure.c +11 -2

index 548fbd7..55f7d07 100644
@@ -386,8 +386,6 @@ static void collect_procs_anon(struct page *page, struct list_head *to_kill,
 	struct task_struct *tsk;
 	struct anon_vma *av;
 
-	if (!PageHuge(page) && unlikely(split_huge_page(page)))
-		return;
 	read_lock(&tasklist_lock);
 	av = page_lock_anon_vma(page);
 	if (av == NULL)	/* Not actually mapped anymore */

@@ -896,6 +894,17 @@ static int hwpoison_user_mappings(struct page *p, unsigned long pfn,
 		}
 	}
 
+	if (!PageHuge(hpage) && PageAnon(hpage) &&
+	    unlikely(split_huge_page(hpage))) {
+		/*
+		 * FIXME: if splitting THP is failed, it is better to stop
+		 * the following operation rather than causing panic
+		 * by unmapping. System might survive if the page is freed later.
+		 */
+		printk(KERN_INFO "MCE %#lx: failed to split THP\n", pfn);
+		return SWAP_FAIL;
+	}
+
 	/*
 	 * First collect all the processes that have the page
 	 * mapped in dirty form.  This has to be done before try_to_unmap,

Signature · 1 line

1.7.2.2

Andrea Arcangeli

2011-01-25 22:02 UTC #1 lore raw

Quoted 4 lines · Jin Dongming

If the THP is failed to be split,
1. The processes using the poisoned page could not be collected. (Because page_mapped_in_vma() in collect_procs_anon() always returns NULL.)

page_mapped_in_vma is only called after split_huge_page without requiring this change.

Quoted 4 lines

2. The poisoned page could not be unmapped.
(If CONFIG_DEBUG_VM is "y", VM_BUG_ON(PageTransHuge(page))
in try_to_unmap() will be called, and system panic will be caused.)

This is more reasonable argument for the change because at times collect_procs may fail kmalloc or for other reasons kill may be set to 1 without split_huge_page having run.

Quoted 2 lines

3. The processes using the poisoned page could not be killed, too. (Because no process is collected in 1.)

I don't see the point of the change for 1.

Quoted 3 lines

So if splitting THP is failed, it is better to stop unmapping rather than causing panic. System might servive if the page is freed later.

Splitting THP can't fail unless the page is freed under split_huge_page (like if the process quits while you're calling split_huge_page and in turn the anon_vma was already unusable).

Patch looks good but just for point 2.

Jin Dongming

author

2011-01-27 01:09 UTC #2 lore raw

Hi, Andrea

Quoted 14 lines · Andrea Arcangeli

Quoted 4 lines · Jin Dongming
If the THP is failed to be split,
1. The processes using the poisoned page could not be collected. (Because page_mapped_in_vma() in collect_procs_anon() always returns NULL.)
page_mapped_in_vma is only called after split_huge_page without requiring this change.
Quoted 4 lines
2. The poisoned page could not be unmapped.
(If CONFIG_DEBUG_VM is "y", VM_BUG_ON(PageTransHuge(page))
in try_to_unmap() will be called, and system panic will be caused.)

Quoted 9 lines

This is more reasonable argument for the change because at times collect_procs may fail kmalloc or for other reasons kill may be set to 1 without split_huge_page having run.
Quoted 2 lines
3. The processes using the poisoned page could not be killed, too. (Because no process is collected in 1.)
I don't see the point of the change for 1.

I am sorry for my poor description.

Quoted 8 lines

Quoted 3 lines
So if splitting THP is failed, it is better to stop unmapping rather than causing panic. System might servive if the page is freed later.
Splitting THP can't fail unless the page is freed under split_huge_page (like if the process quits while you're calling split_huge_page and in turn the anon_vma was already unusable).

Basically I agree with you.

But this patch could do the following things.
1st, as your comment for point 2, this patch could avoid
unexpected failure which is not the real failure of split_huge_page().
2nd, from the result of failure of split_huge_page(),
the operations after it is not meaningful any more or unexpected.
3rd, this patch can reduce the account of patches after it.

So I will modify the description like following.
1. Describe the result of failure of split_huge_page() clearly.
2. Add the reason why move split_huge_page() here.

How do you think about it?

Quoted 1 line

Patch looks good but just for point 2.

Thanks.

Best Regards,
Jin Dongming

Quoted 7 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/