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

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

[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.)
   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 <jin.dongming@np.css.fujitsu.com>
Reviewed-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>
---
 mm/memory-failure.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 548fbd7..55f7d07 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -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,
-- 
1.7.2.2

Re: [PATCH 1/3] Avoid unmapping THP when it is failed to be split.

[Andrea Arcangeli]

On Tue, Jan 25, 2011 at 02:45:27PM +0900, Jin Dongming wrote:
> 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.

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

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

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

Re: [PATCH 1/3] Avoid unmapping THP when it is failed to be split.

[Jin Dongming]

Hi, Andrea

(2011/01/26 7:01), Andrea Arcangeli wrote:
> On Tue, Jan 25, 2011 at 02:45:27PM +0900, Jin Dongming wrote:
>> 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.
> 
>>    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.
> 
>>    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.

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

> Patch looks good but just for point 2.

Thanks.

Best Regards,
Jin Dongming

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