[PATCH 0/8] mm/memory-failure.c: A few cleanup patches for memory failure

[PATCH 0/8] mm/memory-failure.c: A few cleanup 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. More details can be found
in the respective changelogs. Thanks!

Miaohe Lin (8):
  mm/memory-failure.c: minor clean up for memory_failure_dev_pagemap
  mm/memory-failure.c: avoid walking page table when vma_address()
    return -EFAULT
  mm/memory-failure.c: rework the signaling logic in kill_proc
  mm/memory-failure.c: remove unneeded orig_head
  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 | 74 ++++++++++++++++++---------------------------
 1 file changed, 29 insertions(+), 45 deletions(-)

-- 
2.23.0

[PATCH 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>
---
 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 2/8] mm/memory-failure.c: avoid walking page table when vma_address() return -EFAULT

[Miaohe Lin]

It's unnecessary to walk the page table when vma_address() return -EFAULT.
Return early if so to save some cpu cycles.

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

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

[PATCH 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>
---
 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 f86819145ea8..2dd7f35ee65a 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 4/8] mm/memory-failure.c: remove unneeded orig_head

[Miaohe Lin]

orig_head is used to check whether the page have changed compound pages
during the locking. But it's always equal to hpage. So we can use hpage
directly and remove this redundant one.

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

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 2dd7f35ee65a..4370c2f407c5 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1691,7 +1691,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;
@@ -1737,7 +1736,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();
 
 	/*
@@ -1821,7 +1820,7 @@ int memory_failure(unsigned long pfn, int flags)
 	 * The page could have changed compound pages during the locking.
 	 * If this happens just bail out.
 	 */
-	if (PageCompound(p) && compound_head(p) != orig_head) {
+	if (PageCompound(p) && compound_head(p) != hpage) {
 		action_result(pfn, MF_MSG_DIFFERENT_COMPOUND, MF_IGNORED);
 		res = -EBUSY;
 		goto unlock_page;
-- 
2.23.0

[PATCH 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>
---
 mm/memory-failure.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 4370c2f407c5..71bd78279669 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 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>
---
 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 71bd78279669..78aba0b3e983 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1405,26 +1405,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 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 78aba0b3e983..3d0803a13703 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -2161,10 +2161,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 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>
---
 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 3d0803a13703..3e404b06efdc 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1838,7 +1838,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 1/8] mm/memory-failure.c: minor clean up for memory_failure_dev_pagemap

[Naoya Horiguchi]

On Thu, Feb 10, 2022 at 10:17:26PM +0800, Miaohe Lin wrote:
> 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>

Re: [PATCH 2/8] mm/memory-failure.c: avoid walking page table when vma_address() return -EFAULT

[Naoya Horiguchi]

On Thu, Feb 10, 2022 at 10:17:27PM +0800, Miaohe Lin wrote:
> It's unnecessary to walk the page table when vma_address() return -EFAULT.
> Return early if so to save some cpu cycles.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

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

Does this patch fix the real problem rather than just saving cpu cycles?
Without this patch, "address == -EFAULT" seems to make pgd_offset() return
invalid pointer and result in some serious result like general protection fault.
If that's the case, this patch might be worth sending to stable.

Thanks,
Naoya Horiguchi

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

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

[Naoya Horiguchi]

On Thu, Feb 10, 2022 at 10:17:28PM +0800, Miaohe Lin wrote:
> 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>

Re: [PATCH 4/8] mm/memory-failure.c: remove unneeded orig_head

[Naoya Horiguchi]

On Thu, Feb 10, 2022 at 10:17:29PM +0800, Miaohe Lin wrote:
> orig_head is used to check whether the page have changed compound pages
> during the locking. But it's always equal to hpage. So we can use hpage
> directly and remove this redundant one.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  mm/memory-failure.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 2dd7f35ee65a..4370c2f407c5 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1691,7 +1691,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;
> @@ -1737,7 +1736,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();
> 
>  	/*
> @@ -1821,7 +1820,7 @@ int memory_failure(unsigned long pfn, int flags)
>  	 * The page could have changed compound pages during the locking.
>  	 * If this happens just bail out.
>  	 */
> -	if (PageCompound(p) && compound_head(p) != orig_head) {
> +	if (PageCompound(p) && compound_head(p) != hpage) {

I think that this if-check was intended to detect the case that page p
belongs to a thp when memory_failure() is called and belongs to a compound
page in different size (like slab or some driver page) after the thp is
split.  But your suggestion makes me aware that the page p could be embedded
on a thp again after thp split.  I think this might be rare, but if it
happens the current if-check (or suggested one) cannot detect it.
So I feel that simply dropping compound_head() check might be better?

-	if (PageCompound(p) && compound_head(p) != orig_head) {
+	if (PageCompound(p)) {

This should ensure the assumption (mentioned in 8/8 patch) more that
the error page never belongs to compound page after taking page lock.

Thanks,
Naoya Horiguchi

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

[Naoya Horiguchi]

On Thu, Feb 10, 2022 at 10:17:30PM +0800, Miaohe Lin wrote:
> 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>

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

[Naoya Horiguchi]

On Thu, Feb 10, 2022 at 10:17:31PM +0800, Miaohe Lin wrote:
> 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>

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

[Naoya Horiguchi]

On Thu, Feb 10, 2022 at 10:17:33PM +0800, Miaohe Lin wrote:
> 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>

Re: [PATCH 2/8] mm/memory-failure.c: avoid walking page table when vma_address() return -EFAULT

[Miaohe Lin]

On 2022/2/14 22:48, Naoya Horiguchi wrote:
> On Thu, Feb 10, 2022 at 10:17:27PM +0800, Miaohe Lin wrote:
>> It's unnecessary to walk the page table when vma_address() return -EFAULT.
>> Return early if so to save some cpu cycles.
>>
>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> 
> Acked-by: Naoya Horiguchi <naoya.horiguchi@nec.com>

Many thanks for your review and Acked-by tag!

> 
> Does this patch fix the real problem rather than just saving cpu cycles?
> Without this patch, "address == -EFAULT" seems to make pgd_offset() return
> invalid pointer and result in some serious result like general protection fault.

I think you're right. We might dereference the invalid pointer in the following pagetable
walk and results in general protection fault.

> If that's the case, this patch might be worth sending to stable.

But I'am not sure vma_address will return -EFAULT for dax pages in the real workload? If so, I will
send a v2 with Fixes tag.

Thanks again.

> 
> Thanks,
> Naoya Horiguchi
> 
>> ---
>>  mm/memory-failure.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>> index b3ff7e99a421..f86819145ea8 100644
>> --- a/mm/memory-failure.c
>> +++ b/mm/memory-failure.c
>> @@ -315,6 +315,8 @@ static unsigned long dev_pagemap_mapping_shift(struct page *page,
>>  	pmd_t *pmd;
>>  	pte_t *pte;
>>
>> +	if (address == -EFAULT)
>> +		return 0;
>>  	pgd = pgd_offset(vma->vm_mm, address);
>>  	if (!pgd_present(*pgd))
>>  		return 0;
>> ---
>> 2.23.0
>>
> .
> 

Re: [PATCH 4/8] mm/memory-failure.c: remove unneeded orig_head

[Miaohe Lin]

On 2022/2/14 22:50, Naoya Horiguchi wrote:
> On Thu, Feb 10, 2022 at 10:17:29PM +0800, Miaohe Lin wrote:
>> orig_head is used to check whether the page have changed compound pages
>> during the locking. But it's always equal to hpage. So we can use hpage
>> directly and remove this redundant one.
>>
>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>> ---
>>  mm/memory-failure.c | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>> index 2dd7f35ee65a..4370c2f407c5 100644
>> --- a/mm/memory-failure.c
>> +++ b/mm/memory-failure.c
>> @@ -1691,7 +1691,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;
>> @@ -1737,7 +1736,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();
>>
>>  	/*
>> @@ -1821,7 +1820,7 @@ int memory_failure(unsigned long pfn, int flags)
>>  	 * The page could have changed compound pages during the locking.
>>  	 * If this happens just bail out.
>>  	 */
>> -	if (PageCompound(p) && compound_head(p) != orig_head) {
>> +	if (PageCompound(p) && compound_head(p) != hpage) {
> 
> I think that this if-check was intended to detect the case that page p
> belongs to a thp when memory_failure() is called and belongs to a compound
> page in different size (like slab or some driver page) after the thp is
> split.  But your suggestion makes me aware that the page p could be embedded
> on a thp again after thp split.  I think this might be rare, but if it

IIUC, this page can't be embedded on a thp again after thp split because memory_failure hold
an __extra__ page refcnt. I think there exist below race windows:

memory_failure
  orig_head = hpage = compound_head(p); -- page is Non-compound yet
  < -- Page becomes compound page, like thp, slab, some driver page and even hugetlb page -- >
  get_hwpoison_page
  failed to split thp page, as hpage is Non-compound ...
  lock_page

> happens the current if-check (or suggested one) cannot detect it.
> So I feel that simply dropping compound_head() check might be better?
> 
> -	if (PageCompound(p) && compound_head(p) != orig_head) {
> +	if (PageCompound(p)) {

However this change could also catch the above race correctly. In fact, we can't handle
compound page here. But is it enough to just return -EBUSY here as it's really rare or
we should do more things (like split thp, retry if in PageHuge case)?

> 
> This should ensure the assumption (mentioned in 8/8 patch) more that
> the error page never belongs to compound page after taking page lock.

Agree, this really helps.

>> Thanks,
> Naoya Horiguchi
> .
> 

Many thanks.

Re: [PATCH 2/8] mm/memory-failure.c: avoid walking page table when vma_address() return -EFAULT

[HORIGUCHI NAOYA(堀口 直也)]

On Tue, Feb 15, 2022 at 10:40:02AM +0800, Miaohe Lin wrote:
> On 2022/2/14 22:48, Naoya Horiguchi wrote:
> > On Thu, Feb 10, 2022 at 10:17:27PM +0800, Miaohe Lin wrote:
> >> It's unnecessary to walk the page table when vma_address() return -EFAULT.
> >> Return early if so to save some cpu cycles.
> >>
> >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> > 
> > Acked-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
> 
> Many thanks for your review and Acked-by tag!

You're welcome :)

> 
> > 
> > Does this patch fix the real problem rather than just saving cpu cycles?
> > Without this patch, "address == -EFAULT" seems to make pgd_offset() return
> > invalid pointer and result in some serious result like general protection fault.
> 
> I think you're right. We might dereference the invalid pointer in the following pagetable
> walk and results in general protection fault.
> 
> > If that's the case, this patch might be worth sending to stable.
> 
> But I'am not sure vma_address will return -EFAULT for dax pages in the real workload?
> If so, I will send a v2 with Fixes tag.

Hm, actually I'm not sure either.  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.
So VM_BUG_ON() might be more suitable?

Thanks,
Naoya Horiguchi

Re: [PATCH 4/8] mm/memory-failure.c: remove unneeded orig_head

[HORIGUCHI NAOYA(堀口 直也)]

On Tue, Feb 15, 2022 at 11:14:07AM +0800, Miaohe Lin wrote:
> On 2022/2/14 22:50, Naoya Horiguchi wrote:
> > On Thu, Feb 10, 2022 at 10:17:29PM +0800, Miaohe Lin wrote:
> >> orig_head is used to check whether the page have changed compound pages
> >> during the locking. But it's always equal to hpage. So we can use hpage
> >> directly and remove this redundant one.
> >>
> >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> >> ---
> >>  mm/memory-failure.c | 5 ++---
> >>  1 file changed, 2 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> >> index 2dd7f35ee65a..4370c2f407c5 100644
> >> --- a/mm/memory-failure.c
> >> +++ b/mm/memory-failure.c
> >> @@ -1691,7 +1691,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;
> >> @@ -1737,7 +1736,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();
> >>
> >>  	/*
> >> @@ -1821,7 +1820,7 @@ int memory_failure(unsigned long pfn, int flags)
> >>  	 * The page could have changed compound pages during the locking.
> >>  	 * If this happens just bail out.
> >>  	 */
> >> -	if (PageCompound(p) && compound_head(p) != orig_head) {
> >> +	if (PageCompound(p) && compound_head(p) != hpage) {
> > 
> > I think that this if-check was intended to detect the case that page p
> > belongs to a thp when memory_failure() is called and belongs to a compound
> > page in different size (like slab or some driver page) after the thp is
> > split.  But your suggestion makes me aware that the page p could be embedded
> > on a thp again after thp split.  I think this might be rare, but if it
> 
> IIUC, this page can't be embedded on a thp again after thp split because memory_failure hold
> an __extra__ page refcnt. I think there exist below race windows:
> 
> memory_failure
>   orig_head = hpage = compound_head(p); -- page is Non-compound yet
>   < -- Page becomes compound page, like thp, slab, some driver page and even hugetlb page -- >
>   get_hwpoison_page
>   failed to split thp page, as hpage is Non-compound ...
>   lock_page
> 
> > happens the current if-check (or suggested one) cannot detect it.
> > So I feel that simply dropping compound_head() check might be better?
> > 
> > -	if (PageCompound(p) && compound_head(p) != orig_head) {
> > +	if (PageCompound(p)) {
> 
> However this change could also catch the above race correctly. In fact, we can't handle
> compound page here. But is it enough to just return -EBUSY here as it's really rare or
> we should do more things (like split thp, retry if in PageHuge case)?

Hmm, both could make sense and hard to judge to me, so it's upto you.
We already have goto label "try_again" so retrying might not be so surprising.

Thanks,
Naoya Horiguchi

Re: [PATCH 4/8] mm/memory-failure.c: remove unneeded orig_head

[Miaohe Lin]

On 2022/2/15 16:48, HORIGUCHI NAOYA(堀口 直也) wrote:
> On Tue, Feb 15, 2022 at 11:14:07AM +0800, Miaohe Lin wrote:
>> On 2022/2/14 22:50, Naoya Horiguchi wrote:
>>> On Thu, Feb 10, 2022 at 10:17:29PM +0800, Miaohe Lin wrote:
>>>> orig_head is used to check whether the page have changed compound pages
>>>> during the locking. But it's always equal to hpage. So we can use hpage
>>>> directly and remove this redundant one.
>>>>
>>>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>>>> ---
>>>>  mm/memory-failure.c | 5 ++---
>>>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>>>> index 2dd7f35ee65a..4370c2f407c5 100644
>>>> --- a/mm/memory-failure.c
>>>> +++ b/mm/memory-failure.c
>>>> @@ -1691,7 +1691,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;
>>>> @@ -1737,7 +1736,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();
>>>>
>>>>  	/*
>>>> @@ -1821,7 +1820,7 @@ int memory_failure(unsigned long pfn, int flags)
>>>>  	 * The page could have changed compound pages during the locking.
>>>>  	 * If this happens just bail out.
>>>>  	 */
>>>> -	if (PageCompound(p) && compound_head(p) != orig_head) {
>>>> +	if (PageCompound(p) && compound_head(p) != hpage) {
>>>
>>> I think that this if-check was intended to detect the case that page p
>>> belongs to a thp when memory_failure() is called and belongs to a compound
>>> page in different size (like slab or some driver page) after the thp is
>>> split.  But your suggestion makes me aware that the page p could be embedded
>>> on a thp again after thp split.  I think this might be rare, but if it
>>
>> IIUC, this page can't be embedded on a thp again after thp split because memory_failure hold
>> an __extra__ page refcnt. I think there exist below race windows:
>>
>> memory_failure
>>   orig_head = hpage = compound_head(p); -- page is Non-compound yet
>>   < -- Page becomes compound page, like thp, slab, some driver page and even hugetlb page -- >
>>   get_hwpoison_page
>>   failed to split thp page, as hpage is Non-compound ...
>>   lock_page
>>
>>> happens the current if-check (or suggested one) cannot detect it.
>>> So I feel that simply dropping compound_head() check might be better?
>>>
>>> -	if (PageCompound(p) && compound_head(p) != orig_head) {
>>> +	if (PageCompound(p)) {
>>
>> However this change could also catch the above race correctly. In fact, we can't handle
>> compound page here. But is it enough to just return -EBUSY here as it's really rare or
>> we should do more things (like split thp, retry if in PageHuge case)?
> 
> Hmm, both could make sense and hard to judge to me, so it's upto you.
> We already have goto label "try_again" so retrying might not be so surprising.
> 

try_again sounds a good idea. Will send a V2. Many thanks.

> Thanks,
> Naoya Horiguchi
> 

Re: [PATCH 2/8] mm/memory-failure.c: avoid walking page table when vma_address() return -EFAULT

[Miaohe Lin]

On 2022/2/15 16:37, HORIGUCHI NAOYA(堀口 直也) wrote:
> On Tue, Feb 15, 2022 at 10:40:02AM +0800, Miaohe Lin wrote:
>> On 2022/2/14 22:48, Naoya Horiguchi wrote:
>>> On Thu, Feb 10, 2022 at 10:17:27PM +0800, Miaohe Lin wrote:
>>>> It's unnecessary to walk the page table when vma_address() return -EFAULT.
>>>> Return early if so to save some cpu cycles.
>>>>
>>>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>>>
>>> Acked-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
>>
>> Many thanks for your review and Acked-by tag!
> 
> You're welcome :)
> 
>>
>>>
>>> Does this patch fix the real problem rather than just saving cpu cycles?
>>> Without this patch, "address == -EFAULT" seems to make pgd_offset() return
>>> invalid pointer and result in some serious result like general protection fault.
>>
>> I think you're right. We might dereference the invalid pointer in the following pagetable
>> walk and results in general protection fault.
>>
>>> If that's the case, this patch might be worth sending to stable.
>>
>> But I'am not sure vma_address will return -EFAULT for dax pages in the real workload?
>> If so, I will send a v2 with Fixes tag.
> 
> Hm, actually I'm not sure either.  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.
> So VM_BUG_ON() might be more suitable?

Agree. anon_vma_interval_tree_foreach/vma_interval_tree_foreach in collect_procs_{file,anon} should
have guaranteed the validity of the vma_address(). And rmap_walk_anon and rmap_walk_file do the
VM_BUG_ON_VMA(address == -EFAULT, vma). So VM_BUG_ON() might be really more suitable. Will do this
in v2.

Many thanks.

> 
> Thanks,
> Naoya Horiguchi
>