[PATCH v3 0/8] A few cleanup and fixup patches for memory failure

[PATCH v3 0/8] A few cleanup and fixup patches for memory failure

[Miaohe Lin]

Hi,
This series contains a few patches to simplify the code logic, remove
unneeded variable and remove obsolete comment. Also we fix race changing
page more robustly in memory_failure. More details can be found in the
respective changelogs. Thanks!

---
v2->v3:
  limit the retry chance per Naoya.

v1->v2:
  Collect Acked-by tag and fix review comment.
  Thanks Naoya.
---

Miaohe Lin (8):
  mm/memory-failure.c: minor clean up for memory_failure_dev_pagemap
  mm/memory-failure.c: catch unexpected -EFAULT from vma_address()
  mm/memory-failure.c: rework the signaling logic in kill_proc
  mm/memory-failure.c: fix race with changing page more robustly
  mm/memory-failure.c: remove PageSlab check in hwpoison_filter_dev
  mm/memory-failure.c: rework the try_to_unmap logic in
    hwpoison_user_mappings()
  mm/memory-failure.c: remove obsolete comment in __soft_offline_page
  mm/memory-failure.c: remove unnecessary PageTransTail check

 mm/memory-failure.c | 87 +++++++++++++++++++++------------------------
 1 file changed, 40 insertions(+), 47 deletions(-)

-- 
2.23.0

[PATCH v3 1/8] mm/memory-failure.c: minor clean up for memory_failure_dev_pagemap

[Miaohe Lin]

The flags always has MF_ACTION_REQUIRED and MF_MUST_KILL set. So we do
not need to check these flags again.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
Acked-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
---
 mm/memory-failure.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 55edb0cc3848..b3ff7e99a421 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1644,7 +1644,7 @@ static int memory_failure_dev_pagemap(unsigned long pfn, int flags,
 	 * SIGBUS (i.e. MF_MUST_KILL)
 	 */
 	flags |= MF_ACTION_REQUIRED | MF_MUST_KILL;
-	collect_procs(page, &tokill, flags & MF_ACTION_REQUIRED);
+	collect_procs(page, &tokill, true);
 
 	list_for_each_entry(tk, &tokill, nd)
 		if (tk->size_shift)
@@ -1659,7 +1659,7 @@ static int memory_failure_dev_pagemap(unsigned long pfn, int flags,
 		start = (page->index << PAGE_SHIFT) & ~(size - 1);
 		unmap_mapping_range(page->mapping, start, size, 0);
 	}
-	kill_procs(&tokill, flags & MF_MUST_KILL, false, pfn, flags);
+	kill_procs(&tokill, true, false, pfn, flags);
 	rc = 0;
 unlock:
 	dax_unlock_page(page, cookie);
-- 
2.23.0

[PATCH v3 2/8] mm/memory-failure.c: catch unexpected -EFAULT from vma_address()

[Miaohe Lin]

It's unexpected to walk the page table when vma_address() return -EFAULT.
But dev_pagemap_mapping_shift() is called only when vma associated to the
error page is found already in collect_procs_{file,anon}, so vma_address()
should not return -EFAULT except with some bug, as Naoya pointed out. We
can use VM_BUG_ON_VMA() to catch this bug here.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
Acked-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
---
 mm/memory-failure.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index b3ff7e99a421..eaa241058401 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -315,6 +315,7 @@ static unsigned long dev_pagemap_mapping_shift(struct page *page,
 	pmd_t *pmd;
 	pte_t *pte;
 
+	VM_BUG_ON_VMA(address == -EFAULT, vma);
 	pgd = pgd_offset(vma->vm_mm, address);
 	if (!pgd_present(*pgd))
 		return 0;
-- 
2.23.0

[PATCH v3 3/8] mm/memory-failure.c: rework the signaling logic in kill_proc

[Miaohe Lin]

BUS_MCEERR_AR code is only sent when MF_ACTION_REQUIRED is set and the
target is current. Rework the code to make this clear.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
Acked-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
---
 mm/memory-failure.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index eaa241058401..7e205d91b2d7 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -258,16 +258,13 @@ static int kill_proc(struct to_kill *tk, unsigned long pfn, int flags)
 	pr_err("Memory failure: %#lx: Sending SIGBUS to %s:%d due to hardware memory corruption\n",
 			pfn, t->comm, t->pid);
 
-	if (flags & MF_ACTION_REQUIRED) {
-		if (t == current)
-			ret = force_sig_mceerr(BUS_MCEERR_AR,
-					 (void __user *)tk->addr, addr_lsb);
-		else
-			/* Signal other processes sharing the page if they have PF_MCE_EARLY set. */
-			ret = send_sig_mceerr(BUS_MCEERR_AO, (void __user *)tk->addr,
-				addr_lsb, t);
-	} else {
+	if ((flags & MF_ACTION_REQUIRED) && (t == current))
+		ret = force_sig_mceerr(BUS_MCEERR_AR,
+				 (void __user *)tk->addr, addr_lsb);
+	else
 		/*
+		 * Signal other processes sharing the page if they have
+		 * PF_MCE_EARLY set.
 		 * Don't use force here, it's convenient if the signal
 		 * can be temporarily blocked.
 		 * This could cause a loop when the user sets SIGBUS
@@ -275,7 +272,6 @@ static int kill_proc(struct to_kill *tk, unsigned long pfn, int flags)
 		 */
 		ret = send_sig_mceerr(BUS_MCEERR_AO, (void __user *)tk->addr,
 				      addr_lsb, t);  /* synchronous? */
-	}
 	if (ret < 0)
 		pr_info("Memory failure: Error sending signal to %s:%d: %d\n",
 			t->comm, t->pid, ret);
-- 
2.23.0

[PATCH v3 4/8] mm/memory-failure.c: fix race with changing page more robustly

[Miaohe Lin]

We're only intended to deal with the non-Compound page after we split thp
in memory_failure. However, the page could have changed compound pages due
to race window. If this happens, we could retry once to hopefully handle
the page next round. Also remove unneeded orig_head. It's always equal to
the hpage. So we can use hpage directly and remove this redundant one.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/memory-failure.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 7e205d91b2d7..2ca50cfa601f 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1690,7 +1690,6 @@ int memory_failure(unsigned long pfn, int flags)
 {
 	struct page *p;
 	struct page *hpage;
-	struct page *orig_head;
 	struct dev_pagemap *pgmap;
 	int res = 0;
 	unsigned long page_flags;
@@ -1736,7 +1735,7 @@ int memory_failure(unsigned long pfn, int flags)
 		goto unlock_mutex;
 	}
 
-	orig_head = hpage = compound_head(p);
+	hpage = compound_head(p);
 	num_poisoned_pages_inc();
 
 	/*
@@ -1817,10 +1816,21 @@ int memory_failure(unsigned long pfn, int flags)
 	lock_page(p);
 
 	/*
-	 * The page could have changed compound pages during the locking.
-	 * If this happens just bail out.
+	 * We're only intended to deal with the non-Compound page here.
+	 * However, the page could have changed compound pages due to
+	 * race window. If this happens, we could try again to hopefully
+	 * handle the page next round.
 	 */
-	if (PageCompound(p) && compound_head(p) != orig_head) {
+	if (PageCompound(p)) {
+		if (retry) {
+			if (TestClearPageHWPoison(p))
+				num_poisoned_pages_dec();
+			unlock_page(p);
+			put_page(p);
+			flags &= ~MF_COUNT_INCREASED;
+			retry = false;
+			goto try_again;
+		}
 		action_result(pfn, MF_MSG_DIFFERENT_COMPOUND, MF_IGNORED);
 		res = -EBUSY;
 		goto unlock_page;
-- 
2.23.0

[PATCH v3 5/8] mm/memory-failure.c: remove PageSlab check in hwpoison_filter_dev

[Miaohe Lin]

Since commit 03e5ac2fc3bf ("mm: fix crash when using XFS on loopback"),
page_mapping() can handle the Slab pages. So remove this unnecessary
PageSlab check and obsolete comment.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
Acked-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
---
 mm/memory-failure.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 2ca50cfa601f..a6f6fdf708ae 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -130,12 +130,6 @@ static int hwpoison_filter_dev(struct page *p)
 	    hwpoison_filter_dev_minor == ~0U)
 		return 0;
 
-	/*
-	 * page_mapping() does not accept slab pages.
-	 */
-	if (PageSlab(p))
-		return -EINVAL;
-
 	mapping = page_mapping(p);
 	if (mapping == NULL || mapping->host == NULL)
 		return -EINVAL;
-- 
2.23.0

[PATCH v3 6/8] mm/memory-failure.c: rework the try_to_unmap logic in hwpoison_user_mappings()

[Miaohe Lin]

Only for hugetlb pages in shared mappings, try_to_unmap should take
semaphore in write mode here. Rework the code to make it clear.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
Acked-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
---
 mm/memory-failure.c | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index a6f6fdf708ae..365384c41da1 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1404,26 +1404,22 @@ static bool hwpoison_user_mappings(struct page *p, unsigned long pfn,
 	if (kill)
 		collect_procs(hpage, &tokill, flags & MF_ACTION_REQUIRED);
 
-	if (!PageHuge(hpage)) {
-		try_to_unmap(hpage, ttu);
+	if (PageHuge(hpage) && !PageAnon(hpage)) {
+		/*
+		 * For hugetlb pages in shared mappings, try_to_unmap
+		 * could potentially call huge_pmd_unshare.  Because of
+		 * this, take semaphore in write mode here and set
+		 * TTU_RMAP_LOCKED to indicate we have taken the lock
+		 * at this higher level.
+		 */
+		mapping = hugetlb_page_mapping_lock_write(hpage);
+		if (mapping) {
+			try_to_unmap(hpage, ttu|TTU_RMAP_LOCKED);
+			i_mmap_unlock_write(mapping);
+		} else
+			pr_info("Memory failure: %#lx: could not lock mapping for mapped huge page\n", pfn);
 	} else {
-		if (!PageAnon(hpage)) {
-			/*
-			 * For hugetlb pages in shared mappings, try_to_unmap
-			 * could potentially call huge_pmd_unshare.  Because of
-			 * this, take semaphore in write mode here and set
-			 * TTU_RMAP_LOCKED to indicate we have taken the lock
-			 * at this higher level.
-			 */
-			mapping = hugetlb_page_mapping_lock_write(hpage);
-			if (mapping) {
-				try_to_unmap(hpage, ttu|TTU_RMAP_LOCKED);
-				i_mmap_unlock_write(mapping);
-			} else
-				pr_info("Memory failure: %#lx: could not lock mapping for mapped huge page\n", pfn);
-		} else {
-			try_to_unmap(hpage, ttu);
-		}
+		try_to_unmap(hpage, ttu);
 	}
 
 	unmap_success = !page_mapped(hpage);
-- 
2.23.0

[PATCH v3 7/8] mm/memory-failure.c: remove obsolete comment in __soft_offline_page

[Miaohe Lin]

Since commit add05cecef80 ("mm: soft-offline: don't free target page in
successful page migration"), set_migratetype_isolate logic is removed.
Remove this obsolete comment.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/memory-failure.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 365384c41da1..6bd555f12972 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -2171,10 +2171,6 @@ static int __soft_offline_page(struct page *page)
 		ret = invalidate_inode_page(page);
 	unlock_page(page);
 
-	/*
-	 * RED-PEN would be better to keep it isolated here, but we
-	 * would need to fix isolation locking first.
-	 */
 	if (ret) {
 		pr_info("soft_offline: %#lx: invalidated\n", pfn);
 		page_handle_poison(page, false, true);
-- 
2.23.0

[PATCH v3 8/8] mm/memory-failure.c: remove unnecessary PageTransTail check

[Miaohe Lin]

When we reach here, we're guaranteed to have non-compound page as thp is
already splited. Remove this unnecessary PageTransTail check.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
Acked-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
---
 mm/memory-failure.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 6bd555f12972..accd1c5d7fa0 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1848,7 +1848,7 @@ int memory_failure(unsigned long pfn, int flags)
 	 * page_lock. We need wait writeback completion for this page or it
 	 * may trigger vfs BUG while evict inode.
 	 */
-	if (!PageTransTail(p) && !PageLRU(p) && !PageWriteback(p))
+	if (!PageLRU(p) && !PageWriteback(p))
 		goto identify_page_state;
 
 	/*
-- 
2.23.0

Re: [PATCH v3 4/8] mm/memory-failure.c: fix race with changing page more robustly

[HORIGUCHI NAOYA(堀口 直也)]

On Fri, Feb 18, 2022 at 05:01:14PM +0800, Miaohe Lin wrote:
> We're only intended to deal with the non-Compound page after we split thp
> in memory_failure. However, the page could have changed compound pages due
> to race window. If this happens, we could retry once to hopefully handle
> the page next round. Also remove unneeded orig_head. It's always equal to
> the hpage. So we can use hpage directly and remove this redundant one.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Acked-by: Naoya Horiguchi <naoya.horiguchi@nec.com>

> ---
>  mm/memory-failure.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 7e205d91b2d7..2ca50cfa601f 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1690,7 +1690,6 @@ int memory_failure(unsigned long pfn, int flags)
>  {
>  	struct page *p;
>  	struct page *hpage;
> -	struct page *orig_head;
>  	struct dev_pagemap *pgmap;
>  	int res = 0;
>  	unsigned long page_flags;
> @@ -1736,7 +1735,7 @@ int memory_failure(unsigned long pfn, int flags)
>  		goto unlock_mutex;
>  	}
>  
> -	orig_head = hpage = compound_head(p);
> +	hpage = compound_head(p);
>  	num_poisoned_pages_inc();
>  
>  	/*
> @@ -1817,10 +1816,21 @@ int memory_failure(unsigned long pfn, int flags)
>  	lock_page(p);
>  
>  	/*
> -	 * The page could have changed compound pages during the locking.
> -	 * If this happens just bail out.
> +	 * We're only intended to deal with the non-Compound page here.
> +	 * However, the page could have changed compound pages due to
> +	 * race window. If this happens, we could try again to hopefully
> +	 * handle the page next round.
>  	 */
> -	if (PageCompound(p) && compound_head(p) != orig_head) {
> +	if (PageCompound(p)) {
> +		if (retry) {
> +			if (TestClearPageHWPoison(p))
> +				num_poisoned_pages_dec();
> +			unlock_page(p);
> +			put_page(p);
> +			flags &= ~MF_COUNT_INCREASED;
> +			retry = false;
> +			goto try_again;
> +		}
>  		action_result(pfn, MF_MSG_DIFFERENT_COMPOUND, MF_IGNORED);
>  		res = -EBUSY;
>  		goto unlock_page;
> -- 
> 2.23.0