LKML Archive on lore.kernel.org
 help / color / Atom feed
* Re: Re:[RFC] a question about reuse hwpoison page in soft_offline_page()
       [not found] <518e6b02-47ef-4ba8-ab98-8d807e2de7d5.xishi.qiuxishi@alibaba-inc.com>
@ 2018-07-09 10:28 ` Naoya Horiguchi
       [not found]   ` <e223130b-6644-4210-9d20-a4224eaad912.xishi.qiuxishi@alibaba-inc.com>
  0 siblings, 1 reply; 4+ messages in thread
From: Naoya Horiguchi @ 2018-07-09 10:28 UTC (permalink / raw)
  To: 裘稀石(稀石)
  Cc: linux-mm, linux-kernel, 陈义全

On Mon, Jul 09, 2018 at 01:43:35PM +0800, 裘稀石(稀石) wrote:
> Hi Naoya,
> 
> Shall we fix this path too? It also will set hwpoison before 
> dissolve_free_huge_page().
> 
> soft_offline_huge_page
>     migrate_pages
>         unmap_and_move_huge_page
>             if (reason == MR_MEMORY_FAILURE && !test_set_page_hwpoison(hpage))
>     dissolve_free_huge_page

Thank you Xishi, I added it to the current (still draft) version below.

I start feeling that current code is broken about behavior of PageHWPoison
(at least) in soft offline. And this patch might not cover all of the issues.
My current questions/concerns are:

  - does the same issue happens on soft offlining of normal pages?
  - does hard offling of free (huge) page have the similar issue?

I'll try to clarify these next and will update the patch if necessary.
I'm happy if I get some comment around these.

Thanks,
Naoya Horiguchi
---
From 9ce4df899f4c859001571958be6a281cdaf5a58f Mon Sep 17 00:00:00 2001
From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Date: Mon, 9 Jul 2018 13:07:46 +0900
Subject: [PATCH] mm: fix race on soft-offlining free huge pages

There's a race condition between soft offline and hugetlb_fault which
causes unexpected process killing and/or hugetlb allocation failure.

The process killing is caused by the following flow:

  CPU 0               CPU 1              CPU 2

  soft offline
    get_any_page
    // find the hugetlb is free
                      mmap a hugetlb file
                      page fault
                        ...
                          hugetlb_fault
                            hugetlb_no_page
                              alloc_huge_page
                              // succeed
      soft_offline_free_page
      // set hwpoison flag
                                         mmap the hugetlb file
                                         page fault
                                           ...
                                             hugetlb_fault
                                               hugetlb_no_page
                                                 find_lock_page
                                                   return VM_FAULT_HWPOISON
                                           mm_fault_error
                                             do_sigbus
                                             // kill the process


The hugetlb allocation failure comes from the following flow:

  CPU 0                          CPU 1

                                 mmap a hugetlb file
                                 // reserve all free page but don't fault-in
  soft offline
    get_any_page
    // find the hugetlb is free
      soft_offline_free_page
      // set hwpoison flag
        dissolve_free_huge_page
        // fail because all free hugepages are reserved
                                 page fault
                                   ...
                                     hugetlb_fault
                                       hugetlb_no_page
                                         alloc_huge_page
                                           ...
                                             dequeue_huge_page_node_exact
                                             // ignore hwpoisoned hugepage
                                             // and finally fail due to no-mem

The root cause of this is that current soft-offline code is written
based on an assumption that PageHWPoison flag should beset at first to
avoid accessing the corrupted data.  This makes sense for memory_failure()
or hard offline, but does not for soft offline because soft offline is
not about corrected error and is safe from data lost.
This patch changes soft offline semantics where it sets PageHWPoison flag
only after containment of the error page completes succesfully.

Reported-by: Xishi Qiu <xishi.qiuxishi@alibaba-inc.com>
Suggested-by: Xishi Qiu <xishi.qiuxishi@alibaba-inc.com>
Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
---
 mm/hugetlb.c        | 11 +++++------
 mm/memory-failure.c | 13 +++++++------
 mm/migrate.c        |  2 --
 3 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index d34225c1cb5b..3c9ce4c05f1b 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -1479,22 +1479,20 @@ static int free_pool_huge_page(struct hstate *h, nodemask_t *nodes_allowed,
 /*
  * Dissolve a given free hugepage into free buddy pages. This function does
  * nothing for in-use (including surplus) hugepages. Returns -EBUSY if the
- * number of free hugepages would be reduced below the number of reserved
- * hugepages.
+ * dissolution fails because a give page is not a free hugepage, or because
+ * free hugepages are fully reserved.
  */
 int dissolve_free_huge_page(struct page *page)
 {
-	int rc = 0;
+	int rc = -EBUSY;
 
 	spin_lock(&hugetlb_lock);
 	if (PageHuge(page) && !page_count(page)) {
 		struct page *head = compound_head(page);
 		struct hstate *h = page_hstate(head);
 		int nid = page_to_nid(head);
-		if (h->free_huge_pages - h->resv_huge_pages == 0) {
-			rc = -EBUSY;
+		if (h->free_huge_pages - h->resv_huge_pages == 0)
 			goto out;
-		}
 		/*
 		 * Move PageHWPoison flag from head page to the raw error page,
 		 * which makes any subpages rather than the error page reusable.
@@ -1508,6 +1506,7 @@ int dissolve_free_huge_page(struct page *page)
 		h->free_huge_pages_node[nid]--;
 		h->max_huge_pages--;
 		update_and_free_page(h, head);
+		rc = 0;
 	}
 out:
 	spin_unlock(&hugetlb_lock);
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 9d142b9b86dc..7a519d947408 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1598,8 +1598,9 @@ static int soft_offline_huge_page(struct page *page, int flags)
 		if (ret > 0)
 			ret = -EIO;
 	} else {
-		if (PageHuge(page))
-			dissolve_free_huge_page(page);
+		ret = dissolve_free_huge_page(page);
+		if (!ret)
+			num_poisoned_pages_inc();
 	}
 	return ret;
 }
@@ -1715,13 +1716,13 @@ static int soft_offline_in_use_page(struct page *page, int flags)
 
 static void soft_offline_free_page(struct page *page)
 {
+	int rc = 0;
 	struct page *head = compound_head(page);
 
-	if (!TestSetPageHWPoison(head)) {
+	if (PageHuge(head))
+		rc = dissolve_free_huge_page(page);
+	if (!rc && !TestSetPageHWPoison(page))
 		num_poisoned_pages_inc();
-		if (PageHuge(head))
-			dissolve_free_huge_page(page);
-	}
 }
 
 /**
diff --git a/mm/migrate.c b/mm/migrate.c
index 198af4289f9b..3ae213b799a1 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1318,8 +1318,6 @@ static int unmap_and_move_huge_page(new_page_t get_new_page,
 out:
 	if (rc != -EAGAIN)
 		putback_active_hugepage(hpage);
-	if (reason == MR_MEMORY_FAILURE && !test_set_page_hwpoison(hpage))
-		num_poisoned_pages_inc();
 
 	/*
 	 * If migration was not successful and there's a freeing callback, use
-- 
2.7.4

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: 回复:Re:[RFC] a question about reuse hwpoison page in soft_offline_page()
       [not found]   ` <e223130b-6644-4210-9d20-a4224eaad912.xishi.qiuxishi@alibaba-inc.com>
@ 2018-07-10  8:15     ` Naoya Horiguchi
  0 siblings, 0 replies; 4+ messages in thread
From: Naoya Horiguchi @ 2018-07-10  8:15 UTC (permalink / raw)
  To: 裘稀石(稀石)
  Cc: linux-mm, linux-kernel, 陈义全

Hi Xishi,

On Mon, Jul 09, 2018 at 09:13:07PM +0800, 裘稀石(稀石) wrote:
> Hi Naoya,
> 
>  - does the same issue happens on soft offlining of normal pages?
> 
> I thinks yes. We can do anything during get_any_page and set hwpoison flag.
> soft_offline_page
>     get_any_page
>     soft_offline_free_page
>         SetPageHWPoison
> 
> I search the key word of PageHWPoison, and these two paths maybe have issue.
> do_swap_page
>     if (PageHWPoison(page)) - ret = VM_FAULT_HWPOISON;
> __do_fault
>     if (unlikely(PageHWPoison(vmf->page))) - return VM_FAULT_HWPOISON;
> 
> It may cause mce kill later.

I agree.

I checked git history and found some related to this race.
IIRC, the race windows seems to open at commit c6c919eb90e0 ("mm: use put_page()
to free page instead of putback_lru_page()") which makes migration source pages
freed by put_page().
Before that commit, we kept migration source pages pinned to avoid reusing,
introduced by commit add05cecef80 ("mm: soft-offline: don't free target page
in successful page migration".)
And before this commit, migration source pages were freed via putback_lru_page()
with changing migration type to MIGRATE_ISOLATE.

The oldest behavior was buggy because of freeing with putback_lru_page(),
but "changing migration type" part was still correct if there's a time window
between freeing a page and setting the PageHWPoison flag.
At c6c919eb90e0 we returned back to the behavior of releasing source pages
(then we have such time window again,) so we should have restored to change
the migration type.
So I feel that this solves the above issues in the normal page's case.
Does it make sense?

This part of code has changed repeatedly and it's painful to track the whole
story in git-log, so I'll clarify the latest (hopefully saner) behavior
in inline comment.

> 
> As you said allocation failure, I think we will get oom first.

We get OOM if alloc_huge_page() returns ENOMEM, and that happens when
 - vma_needs_reservation() returns negative value,
 - hugetlb_cgroup_charge_cgroup() fails to charge.
It seems to me that hwposion doesn't affect both of these.

I presumed that dequeue_huge_page_vma() failed with NULL return due to
dequeue_huge_page_node_exact()'s failure.
And the result should be ENOSPC or allocation success by overcommitting.
So the allocation failure might cause sigbus?

> 
>   - does hard offling of free (huge) page have the similar issue?
> We can kill process in anytime, right? 

Yes, so as-is might be fine.

Thanks,
Naoya Horiguchi

> 
> Thanks,
> Xishi Qiu
> On Mon, Jul 09, 2018 at 01:43:35PM +0800, 裘稀石(稀石) wrote:
> > Hi Naoya,
> > 
> > Shall we fix this path too? It also will set hwpoison before 
> > dissolve_free_huge_page().
> > 
> > soft_offline_huge_page
> >     migrate_pages
> >         unmap_and_move_huge_page
> >             if (reason == MR_MEMORY_FAILURE && !test_set_page_hwpoison
> (hpage))
> >     dissolve_free_huge_page
> 
> Thank you Xishi, I added it to the current (still draft) version below.
> 
> I start feeling that current code is broken about behavior of PageHWPoison
> (at least) in soft offline. And this patch might not cover all of the issues.
> My current questions/concerns are:
> 
>   - does the same issue happens on soft offlining of normal pages?
>   - does hard offling of free (huge) page have the similar issue?
> 
> I'll try to clarify these next and will update the patch if necessary.
> I'm happy if I get some comment around these.
> 
> Thanks,
> Naoya Horiguchi
> ---
> From 9ce4df899f4c859001571958be6a281cdaf5a58f Mon Sep 17 00:00:00 2001
> From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Date: Mon, 9 Jul 2018 13:07:46 +0900
> Subject: [PATCH] mm: fix race on soft-offlining free huge pages
> 
> There's a race condition between soft offline and hugetlb_fault which
> causes unexpected process killing and/or hugetlb allocation failure.
> 
> The process killing is caused by the following flow:
> 
>   CPU 0               CPU 1              CPU 2
> 
>   soft offline
>     get_any_page
>     // find the hugetlb is free
>                       mmap a hugetlb file
>                       page fault
>                         ...
>                           hugetlb_fault
>                             hugetlb_no_page
>                               alloc_huge_page
>                               // succeed
>       soft_offline_free_page
>       // set hwpoison flag
>                                          mmap the hugetlb file
>                                          page fault
>                                            ...
>                                              hugetlb_fault
>                                                hugetlb_no_page
>                                                  find_lock_page
>                                                    return VM_FAULT_HWPOISON
>                                            mm_fault_error
>                                              do_sigbus
>                                              // kill the process
> 
> 
> The hugetlb allocation failure comes from the following flow:
> 
>   CPU 0                          CPU 1
> 
>                                  mmap a hugetlb file
>                                  // reserve all free page but don't fault-in
>   soft offline
>     get_any_page
>     // find the hugetlb is free
>       soft_offline_free_page
>       // set hwpoison flag
>         dissolve_free_huge_page
>         // fail because all free hugepages are reserved
>                                  page fault
>                                    ...
>                                      hugetlb_fault
>                                        hugetlb_no_page
>                                          alloc_huge_page
>                                            ...
>                                              dequeue_huge_page_node_exact
>                                              // ignore hwpoisoned hugepage
>                                              // and finally fail due to no-mem
> 
> The root cause of this is that current soft-offline code is written
> based on an assumption that PageHWPoison flag should beset at first to
> avoid accessing the corrupted data.  This makes sense for memory_failure()
> or hard offline, but does not for soft offline because soft offline is
> not about corrected error and is safe from data lost.
> This patch changes soft offline semantics where it sets PageHWPoison flag
> only after containment of the error page completes succesfully.
> 
> Reported-by: Xishi Qiu <xishi.qiuxishi@alibaba-inc.com>
> Suggested-by: Xishi Qiu <xishi.qiuxishi@alibaba-inc.com>
> Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> ---
>  mm/hugetlb.c        | 11 +++++------
>  mm/memory-failure.c | 13 +++++++------
>  mm/migrate.c        |  2 --
>  3 files changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index d34225c1cb5b..3c9ce4c05f1b 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -1479,22 +1479,20 @@ static int free_pool_huge_page
> (struct hstate *h, nodemask_t *nodes_allowed,
>  /*
>   * Dissolve a given free hugepage into free buddy pages. This function does
>   * nothing for in-use (including surplus) hugepages. Returns -EBUSY if the
> - * number of free hugepages would be reduced below the number of reserved
> - * hugepages.
> + * dissolution fails because a give page is not a free hugepage, or because
> + * free hugepages are fully reserved.
>   */
>  int dissolve_free_huge_page(struct page *page)
>  {
> - int rc = 0;
> + int rc = -EBUSY;
>  
>   spin_lock(&hugetlb_lock);
>   if (PageHuge(page) && !page_count(page)) {
>    struct page *head = compound_head(page);
>    struct hstate *h = page_hstate(head);
>    int nid = page_to_nid(head);
> -  if (h->free_huge_pages - h->resv_huge_pages == 0) {
> -   rc = -EBUSY;
> +  if (h->free_huge_pages - h->resv_huge_pages == 0)
>     goto out;
> -  }
>    /*
>     * Move PageHWPoison flag from head page to the raw error page,
>     * which makes any subpages rather than the error page reusable.
> @@ -1508,6 +1506,7 @@ int dissolve_free_huge_page(struct page *page)
>    h->free_huge_pages_node[nid]--;
>    h->max_huge_pages--;
>    update_and_free_page(h, head);
> +  rc = 0;
>   }
>  out:
>   spin_unlock(&hugetlb_lock);
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 9d142b9b86dc..7a519d947408 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1598,8 +1598,9 @@ static int soft_offline_huge_page
> (struct page *page, int flags)
>    if (ret > 0)
>     ret = -EIO;
>   } else {
> -  if (PageHuge(page))
> -   dissolve_free_huge_page(page);
> +  ret = dissolve_free_huge_page(page);
> +  if (!ret)
> +   num_poisoned_pages_inc();
>   }
>   return ret;
>  }
> @@ -1715,13 +1716,13 @@ static int soft_offline_in_use_page
> (struct page *page, int flags)
>  
>  static void soft_offline_free_page(struct page *page)
>  {
> + int rc = 0;
>   struct page *head = compound_head(page);
>  
> - if (!TestSetPageHWPoison(head)) {
> + if (PageHuge(head))
> +  rc = dissolve_free_huge_page(page);
> + if (!rc && !TestSetPageHWPoison(page))
>    num_poisoned_pages_inc();
> -  if (PageHuge(head))
> -   dissolve_free_huge_page(page);
> - }
>  }
>  
>  /**
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 198af4289f9b..3ae213b799a1 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1318,8 +1318,6 @@ static int unmap_and_move_huge_page
> (new_page_t get_new_page,
>  out:
>   if (rc != -EAGAIN)
>    putback_active_hugepage(hpage);
> - if (reason == MR_MEMORY_FAILURE && !test_set_page_hwpoison(hpage))
> -  num_poisoned_pages_inc();
>  
>   /*
>    * If migration was not successful and there's a freeing callback, use
> -- 
> 2.7.4
> 

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: Re:[RFC] a question about reuse hwpoison page in soft_offline_page()
       [not found] <95e5634d-f78f-45bb-9847-1eb5bbdac3cf.xishi.qiuxishi@alibaba-inc.com>
@ 2018-07-09  4:16 ` Naoya Horiguchi
  0 siblings, 0 replies; 4+ messages in thread
From: Naoya Horiguchi @ 2018-07-09  4:16 UTC (permalink / raw)
  To: 裘稀石(稀石)
  Cc: linux-mm, linux-kernel, 陈义全

On Mon, Jul 09, 2018 at 10:31:25AM +0800, 裘稀石(稀石) wrote:
> Hi Naoya,
> 
> I think the double check can not fix the problem as I said in another email.
> If someone mmap before soft offline, so the page_count(head) is still zero
> in soft offline, then hwpoison flag set and it can not be alloced again in
> dequeue_huge_page_node_exact() during page fault, so page fault return
> no-mem, and someone will be killed (not mce kill).
> 
> How about just set hwpoison flag after soft_offline_free_page - 
> dissolve_free_huge_page
> successfully? It will fix the both two problems (mce kill and no-mem kill).

Thank you for elaborating, you're right.
So do you like a fix like this?

---
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index d34225c1cb5b..3c9ce4c05f1b 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -1479,22 +1479,20 @@ static int free_pool_huge_page(struct hstate *h, nodemask_t *nodes_allowed,
 /*
  * Dissolve a given free hugepage into free buddy pages. This function does
  * nothing for in-use (including surplus) hugepages. Returns -EBUSY if the
- * number of free hugepages would be reduced below the number of reserved
- * hugepages.
+ * dissolution fails because a give page is not a free hugepage, or because
+ * free hugepages are fully reserved.
  */
 int dissolve_free_huge_page(struct page *page)
 {
-	int rc = 0;
+	int rc = -EBUSY;
 
 	spin_lock(&hugetlb_lock);
 	if (PageHuge(page) && !page_count(page)) {
 		struct page *head = compound_head(page);
 		struct hstate *h = page_hstate(head);
 		int nid = page_to_nid(head);
-		if (h->free_huge_pages - h->resv_huge_pages == 0) {
-			rc = -EBUSY;
+		if (h->free_huge_pages - h->resv_huge_pages == 0)
 			goto out;
-		}
 		/*
 		 * Move PageHWPoison flag from head page to the raw error page,
 		 * which makes any subpages rather than the error page reusable.
@@ -1508,6 +1506,7 @@ int dissolve_free_huge_page(struct page *page)
 		h->free_huge_pages_node[nid]--;
 		h->max_huge_pages--;
 		update_and_free_page(h, head);
+		rc = 0;
 	}
 out:
 	spin_unlock(&hugetlb_lock);
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 9d142b9b86dc..e4c7e3ec7b10 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1715,13 +1715,13 @@ static int soft_offline_in_use_page(struct page *page, int flags)
 
 static void soft_offline_free_page(struct page *page)
 {
+	int rc = 0;
 	struct page *head = compound_head(page);
 
-	if (!TestSetPageHWPoison(head)) {
+	if (PageHuge(head))
+		rc = dissolve_free_huge_page(page);
+	if (!rc && !TestSetPageHWPoison(head))
 		num_poisoned_pages_inc();
-		if (PageHuge(head))
-			dissolve_free_huge_page(page);
-	}
 }
 
 /**

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: Re:[RFC] a question about reuse hwpoison page in soft_offline_page()
       [not found] <ac6e703d-b0df-40d2-8918-63a63f3c5d68.xishi.qiuxishi@alibaba-inc.com>
@ 2018-07-09  0:38 ` Naoya Horiguchi
  0 siblings, 0 replies; 4+ messages in thread
From: Naoya Horiguchi @ 2018-07-09  0:38 UTC (permalink / raw)
  To: 裘稀石(稀石)
  Cc: linux-mm, linux-kernel, 陈义全

On Fri, Jul 06, 2018 at 05:59:15PM +0800, 裘稀石(稀石) wrote:
> 
> Hi Naoya,
> 
> How about this case, we only trigger soft offline page, but someone killed
> later.
> As the race I said before, then someone may use the hwpoisoned hugetlb page
> again.
> Please see the following.
> 
> soft offline: 
>     get_any_page - find the hugetlb is free
> process A:
>     do_page_fault - handle_mm_fault - hugetlb_fault - hugetlb_no_page
> - alloc_huge_page
> soft offline:
>     soft_offline_free_page - set hwpoison flag
> process B:
>     mmap the hugetlb file from A, hugetlb_fault - hugetlb_no_page
> - find_lock_page
>     it find hwpoison flag is already set, so ret = VM_FAULT_HWPOISON
>     then mm_fault_error - do_sigbus - mce kill
> process B was killed by soft offline, right?

Right, this and your another email shows how things go bad.
Soft offline handler is simply racy now, so we had better cancel it if
the target page was allocated during soft offline handling by double
checking as I mentioned.

Thanks,
Naoya Horiguchi


> 
> Thanks,
> Xishi Qiu 
> 
>     ------------------------------------------------------------------
>     发件人:Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
>     发送时间:2018年7月6日(星期五) 16:19
>     收件人:裘稀石(稀石) <xishi.qiuxishi@alibaba-inc.com>
>     抄 送:linux-mm <linux-mm@kvack.org>; linux-kernel
>     <linux-kernel@vger.kernel.org>; 陈义全 <zy.zhengyi@alibaba-inc.com>
>     主 题:Re: [RFC] a question about reuse hwpoison page in soft_offline_page
>     ()
> 
>     On Fri, Jul 06, 2018 at 11:37:41AM +0800, 裘稀石(稀石) wrote:
>     > This patch add05cec
>     > (mm: soft-offline: don't free target page in successful page migration)
>      removes
>     > set_migratetype_isolate() and unset_migratetype_isolate()
>      in soft_offline_page
>     > ().
>     > 
>     > And this patch 243abd5b
>     > (mm: hugetlb: prevent reuse of hwpoisoned free hugepages) changes
>     > if (!is_migrate_isolate_page(page)) to if (!PageHWPoison
>     (page)), so it could
>     > prevent someone
>     > reuse the free hugetlb again after set the hwpoison flag
>     > in soft_offline_free_page()
>     > 
>     > My question is that if someone reuse the free hugetlb again before 
>     > soft_offline_free_page() and
>     > after get_any_page
>     (), then it uses the hopoison page, and this may trigger mce
>     > kill later, right?
> 
>     Hi Xishi,
> 
>     Thank you for pointing out the issue. That's nice catch.
> 
>     I think that the race condition itself could happen, but it doesn't lead
>     to MCE kill because PageHWPoison is not visible to HW which triggers MCE.
>     PageHWPoison flag is just a flag in struct page to report the memory error
>     from kernel to userspace. So even if a CPU is accessing to the page whose
>     struct page has PageHWPoison set, that doesn't cause a MCE unless the page
>     is physically broken.
>     The type of memory error that soft offline tries to handle is corrected
>     one which is not a failure yet although it's starting to wear.
>     So such PageHWPoison page can be reused, but that's not critical because
>     the page is freed at some point afterword and error containment completes.
> 
>     However, I noticed that there's a small pain in free hugetlb case.
>     We call dissolve_free_huge_page() in soft_offline_free_page() which moves
>     the PageHWPoison flag from the head page to the raw error page.
>     If the reported race happens, dissolve_free_huge_page() just return without
>     doing any dissolve work because "if (PageHuge(page) && !page_count(page))"
>     block is skipped.
>     The hugepage is allocated and used as usual, but the contaiment doesn't
>     complete as expected in the normal page, because free_huge_pages() doesn't
>     call dissolve_free_huge_page() for hwpoison hugepage. This is not critical
>     because such error hugepage just reside in free hugepage list. But this
>     might looks like a kind of memory leak. And even worse when hugepage pool
>     is shrinked and the hwpoison hugepage is freed, the PageHWPoison flag is
>     still on the head page which is unlikely to be an actual error page.
> 
>     So I think we need improvement here, how about the fix like below?
> 
>       (not tested yet, sorry)
> 
>       diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>       --- a/mm/memory-failure.c
>       +++ b/mm/memory-failure.c
>       @@ -1883,6 +1883,11 @@ static void soft_offline_free_page
>     (struct page *page)
>               struct page *head = compound_head(page);
>       
>               if (!TestSetPageHWPoison(head)) {
>       +               if (page_count(head)) {
>       +                       ClearPageHWPoison(head);
>       +                       return;
>       +               }
>       +
>                       num_poisoned_pages_inc();
>                       if (PageHuge(head))
>                               dissolve_free_huge_page(page);
> 
>     Thanks,
>     Naoya Horiguchi
> 

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <518e6b02-47ef-4ba8-ab98-8d807e2de7d5.xishi.qiuxishi@alibaba-inc.com>
2018-07-09 10:28 ` Re:[RFC] a question about reuse hwpoison page in soft_offline_page() Naoya Horiguchi
     [not found]   ` <e223130b-6644-4210-9d20-a4224eaad912.xishi.qiuxishi@alibaba-inc.com>
2018-07-10  8:15     ` 回复:Re:[RFC] " Naoya Horiguchi
     [not found] <95e5634d-f78f-45bb-9847-1eb5bbdac3cf.xishi.qiuxishi@alibaba-inc.com>
2018-07-09  4:16 ` Re:[RFC] " Naoya Horiguchi
     [not found] <ac6e703d-b0df-40d2-8918-63a63f3c5d68.xishi.qiuxishi@alibaba-inc.com>
2018-07-09  0:38 ` Naoya Horiguchi

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git
	git clone --mirror https://lore.kernel.org/lkml/10 lkml/git/10.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git