[PATCH 0/4] Cleanups and fixup for memcontrol

[PATCH 0/4] Cleanups and fixup for memcontrol

[Miaohe Lin]

Hi all,
This series contains cleanups to remove unneeded variable, fix some
obsolete comments and so on. Also we fix potential pte_unmap_unlock
on wrong pte. More details can be found in the respective changelogs.
Thanks!

Miaohe Lin (4):
  mm/hwpoison: remove unneeded variable unmap_success
  mm/hwpoison: fix potential pte_unmap_unlock pte error
  mm/hwpoison: change argument struct page **hpagep to *hpage
  mm/hwpoison: fix some obsolete comments

 mm/memory-failure.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

-- 
2.23.0

[PATCH 1/4] mm/hwpoison: remove unneeded variable unmap_success

[Miaohe Lin]

unmap_success is used to indicate whether page is successfully unmapped
but it's irrelated with ZONE_DEVICE page and unmap_success is always
true here. Remove this unneeded one.

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

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 03f83e7d075b..052ec9ee7cf6 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1512,7 +1512,6 @@ static int memory_failure_dev_pagemap(unsigned long pfn, int flags,
 		struct dev_pagemap *pgmap)
 {
 	struct page *page = pfn_to_page(pfn);
-	const bool unmap_success = true;
 	unsigned long size = 0;
 	struct to_kill *tk;
 	LIST_HEAD(tokill);
@@ -1584,7 +1583,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, !unmap_success, pfn, flags);
+	kill_procs(&tokill, flags & MF_MUST_KILL, false, pfn, flags);
 	rc = 0;
 unlock:
 	dax_unlock_page(page, cookie);
-- 
2.23.0

Re: [PATCH 1/4] mm/hwpoison: remove unneeded variable unmap_success

[HORIGUCHI NAOYA(堀口 直也)]

On Sat, Aug 14, 2021 at 06:51:28PM +0800, Miaohe Lin wrote:
> unmap_success is used to indicate whether page is successfully unmapped
> but it's irrelated with ZONE_DEVICE page and unmap_success is always
> true here. Remove this unneeded one.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Hi Miaohe,

Thank you for finding the issues.

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

[PATCH 2/4] mm/hwpoison: fix potential pte_unmap_unlock pte error

[Miaohe Lin]

If the first pte is equal to poisoned_pfn, i.e. check_hwpoisoned_entry()
return 1, the wrong ptep - 1 would be passed to pte_unmap_unlock().

Fixes: ad9c59c24095 ("mm,hwpoison: send SIGBUS with error virutal address")
Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/memory-failure.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 052ec9ee7cf6..54f61133bf60 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -632,7 +632,7 @@ static int hwpoison_pte_range(pmd_t *pmdp, unsigned long addr,
 {
 	struct hwp_walk *hwp = (struct hwp_walk *)walk->private;
 	int ret = 0;
-	pte_t *ptep;
+	pte_t *ptep, *mapped_pte;
 	spinlock_t *ptl;
 
 	ptl = pmd_trans_huge_lock(pmdp, walk->vma);
@@ -645,14 +645,15 @@ static int hwpoison_pte_range(pmd_t *pmdp, unsigned long addr,
 	if (pmd_trans_unstable(pmdp))
 		goto out;
 
-	ptep = pte_offset_map_lock(walk->vma->vm_mm, pmdp, addr, &ptl);
+	mapped_pte = ptep = pte_offset_map_lock(walk->vma->vm_mm, pmdp,
+						addr, &ptl);
 	for (; addr != end; ptep++, addr += PAGE_SIZE) {
 		ret = check_hwpoisoned_entry(*ptep, addr, PAGE_SHIFT,
 					     hwp->pfn, &hwp->tk);
 		if (ret == 1)
 			break;
 	}
-	pte_unmap_unlock(ptep - 1, ptl);
+	pte_unmap_unlock(mapped_pte, ptl);
 out:
 	cond_resched();
 	return ret;
-- 
2.23.0

Re: [PATCH 2/4] mm/hwpoison: fix potential pte_unmap_unlock pte error

[HORIGUCHI NAOYA(堀口 直也)]

On Sat, Aug 14, 2021 at 06:51:29PM +0800, Miaohe Lin wrote:
> If the first pte is equal to poisoned_pfn, i.e. check_hwpoisoned_entry()
> return 1, the wrong ptep - 1 would be passed to pte_unmap_unlock().
> 
> Fixes: ad9c59c24095 ("mm,hwpoison: send SIGBUS with error virutal address")
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

I agree with the change itself, so

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

One question is that according to "grep -r pte_unmap_unlock ." command over
whole kernel source code, pte_unmap_unlock() is called with "ptep - 1" in some places.
I think that none of them seems to have "break in for loop" in locked period,
so the same problem does not occur there.  But I'm still not sure why some place
call with "ptep - 1" and the others call with pte returned by pte_offset_map_lock().

Re: [PATCH 2/4] mm/hwpoison: fix potential pte_unmap_unlock pte error

[Miaohe Lin]

On 2021/8/17 15:29, HORIGUCHI NAOYA(堀口 直也) wrote:
> On Sat, Aug 14, 2021 at 06:51:29PM +0800, Miaohe Lin wrote:
>> If the first pte is equal to poisoned_pfn, i.e. check_hwpoisoned_entry()
>> return 1, the wrong ptep - 1 would be passed to pte_unmap_unlock().
>>
>> Fixes: ad9c59c24095 ("mm,hwpoison: send SIGBUS with error virutal address")
>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> 
> I agree with the change itself, so
> 
> Acked-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
> 

Many thanks for your review and Acked-by tag!

> One question is that according to "grep -r pte_unmap_unlock ." command over
> whole kernel source code, pte_unmap_unlock() is called with "ptep - 1" in some places.
> I think that none of them seems to have "break in for loop" in locked period,
> so the same problem does not occur there.  But I'm still not sure why some place
> call with "ptep - 1" and the others call with pte returned by pte_offset_map_lock().

IMO pte_unmap_unlock() works as long as the passed in pte belongs to the same page returned
from pte_offset_map_lock(). I have fixed some similar place where pte_unmap_unlock() is called
with wrong "ptep - 1" when I was learning the related mm code.

>

Re: [PATCH 2/4] mm/hwpoison: fix potential pte_unmap_unlock pte error

[HORIGUCHI NAOYA(堀口 直也)]

On Tue, Aug 17, 2021 at 04:24:43PM +0800, Miaohe Lin wrote:
> On 2021/8/17 15:29, HORIGUCHI NAOYA(堀口 直也) wrote:
...
> > One question is that according to "grep -r pte_unmap_unlock ." command over
> > whole kernel source code, pte_unmap_unlock() is called with "ptep - 1" in some places.
> > I think that none of them seems to have "break in for loop" in locked period,
> > so the same problem does not occur there.  But I'm still not sure why some place
> > call with "ptep - 1" and the others call with pte returned by pte_offset_map_lock().
> 
> IMO pte_unmap_unlock() works as long as the passed in pte belongs to the same page returned
> from pte_offset_map_lock(). I have fixed some similar place where pte_unmap_unlock() is called
> with wrong "ptep - 1" when I was learning the related mm code.

Great, thanks for clarification.

- Naoya

[PATCH 3/4] mm/hwpoison: change argument struct page **hpagep to *hpage

[Miaohe Lin]

It's unnecessary to pass in a struct page **hpagep because it's never
modified. Changing to use *hpage to simplify the code.

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

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 54f61133bf60..a4e585f812c1 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1265,14 +1265,13 @@ static int get_hwpoison_page(struct page *p, unsigned long flags)
  * the pages and send SIGBUS to the processes if the data was dirty.
  */
 static bool hwpoison_user_mappings(struct page *p, unsigned long pfn,
-				  int flags, struct page **hpagep)
+				  int flags, struct page *hpage)
 {
 	enum ttu_flags ttu = TTU_IGNORE_MLOCK | TTU_SYNC;
 	struct address_space *mapping;
 	LIST_HEAD(tokill);
 	bool unmap_success;
 	int kill = 1, forcekill;
-	struct page *hpage = *hpagep;
 	bool mlocked = PageMlocked(hpage);
 
 	/*
@@ -1497,7 +1496,7 @@ static int memory_failure_hugetlb(unsigned long pfn, int flags)
 		goto out;
 	}
 
-	if (!hwpoison_user_mappings(p, pfn, flags, &head)) {
+	if (!hwpoison_user_mappings(p, pfn, flags, head)) {
 		action_result(pfn, MF_MSG_UNMAP_FAILED, MF_IGNORED);
 		res = -EBUSY;
 		goto out;
@@ -1777,7 +1776,7 @@ int memory_failure(unsigned long pfn, int flags)
 	 * Now take care of user space mappings.
 	 * Abort on fail: __delete_from_page_cache() assumes unmapped page.
 	 */
-	if (!hwpoison_user_mappings(p, pfn, flags, &p)) {
+	if (!hwpoison_user_mappings(p, pfn, flags, p)) {
 		action_result(pfn, MF_MSG_UNMAP_FAILED, MF_IGNORED);
 		res = -EBUSY;
 		goto unlock_page;
-- 
2.23.0

Re: [PATCH 3/4] mm/hwpoison: change argument struct page **hpagep to *hpage

[HORIGUCHI NAOYA(堀口 直也)]

On Sat, Aug 14, 2021 at 06:51:30PM +0800, Miaohe Lin wrote:
> It's unnecessary to pass in a struct page **hpagep because it's never
> modified. Changing to use *hpage to simplify the code.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

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

[PATCH 4/4] mm/hwpoison: fix some obsolete comments

[Miaohe Lin]

Since commit cb731d6c62bb ("vmscan: per memory cgroup slab shrinkers"),
shrink_node_slabs is renamed to drop_slab_node. And doit argument is
changed to forcekill since commit 6751ed65dc66 ("x86/mce: Fix
siginfo_t->si_addr value for non-recoverable memory faults").

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

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index a4e585f812c1..a6f2384d9933 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -296,7 +296,7 @@ void shake_page(struct page *p, int access)
 	}
 
 	/*
-	 * Only call shrink_node_slabs here (which would also shrink
+	 * Only call drop_slab_node here (which would also shrink
 	 * other caches) if access is not potentially fatal.
 	 */
 	if (access)
@@ -391,8 +391,8 @@ static void add_to_kill(struct task_struct *tsk, struct page *p,
 /*
  * Kill the processes that have been collected earlier.
  *
- * Only do anything when DOIT is set, otherwise just free the list
- * (this is used for clean pages which do not need killing)
+ * Only do anything when FORCEKILL is set, otherwise just free the
+ * list (this is used for clean pages which do not need killing)
  * Also when FAIL is set do a force kill because something went
  * wrong earlier.
  */
-- 
2.23.0

Re: [PATCH 4/4] mm/hwpoison: fix some obsolete comments

[HORIGUCHI NAOYA(堀口 直也)]

On Sat, Aug 14, 2021 at 06:51:31PM +0800, Miaohe Lin wrote:
> Since commit cb731d6c62bb ("vmscan: per memory cgroup slab shrinkers"),
> shrink_node_slabs is renamed to drop_slab_node. And doit argument is
> changed to forcekill since commit 6751ed65dc66 ("x86/mce: Fix
> siginfo_t->si_addr value for non-recoverable memory faults").
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

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

Re: [PATCH 0/4] Cleanups and fixup for memcontrol

[Miaohe Lin]

On 2021/8/14 18:51, Miaohe Lin wrote:
> Hi all,
> This series contains cleanups to remove unneeded variable, fix some
> obsolete comments and so on. Also we fix potential pte_unmap_unlock
> on wrong pte. More details can be found in the respective changelogs.
> Thanks!
> 
> Miaohe Lin (4):
>   mm/hwpoison: remove unneeded variable unmap_success
>   mm/hwpoison: fix potential pte_unmap_unlock pte error
>   mm/hwpoison: change argument struct page **hpagep to *hpage
>   mm/hwpoison: fix some obsolete comments
> 
>  mm/memory-failure.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 

My bad! The title should be "[PATCH 0/4] Cleanups and fixup for hwpoison". Sorry for inconvenience!