[PATCH 0/5] Cleanup and fixup for khugepaged

[PATCH 0/5] Cleanup and fixup for khugepaged

[Miaohe Lin]

Hi all,
This series contains cleanups to remove unneeded return value, use
helper function and so on. And there is one fix to correct the wrong
result value for trace_mm_collapse_huge_page_isolate().

More details can be found in the respective changelogs. Thanks!

Miaohe Lin (5):
  khugepaged: remove unneeded return value of
    khugepaged_collapse_pte_mapped_thps()
  khugepaged: reuse the smp_wmb() inside __SetPageUptodate()
  khugepaged: use helper khugepaged_test_exit() in __khugepaged_enter()
  khugepaged: remove unnecessary mem_cgroup_uncharge() in
    collapse_[file|huge_page]
  khugepaged: fix wrong result value for
    trace_mm_collapse_huge_page_isolate()

 mm/khugepaged.c | 47 ++++++++++++++++++++---------------------------
 1 file changed, 20 insertions(+), 27 deletions(-)

-- 
2.19.1

[PATCH 1/5] khugepaged: remove unneeded return value of khugepaged_collapse_pte_mapped_thps()

[Miaohe Lin]

The return value of khugepaged_collapse_pte_mapped_thps() is never checked
since it's introduced. We should remove such unneeded return value.

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

diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index a7d6cb912b05..d43812c5ce16 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -1533,16 +1533,16 @@ void collapse_pte_mapped_thp(struct mm_struct *mm, unsigned long addr)
 	goto drop_hpage;
 }
 
-static int khugepaged_collapse_pte_mapped_thps(struct mm_slot *mm_slot)
+static void khugepaged_collapse_pte_mapped_thps(struct mm_slot *mm_slot)
 {
 	struct mm_struct *mm = mm_slot->mm;
 	int i;
 
 	if (likely(mm_slot->nr_pte_mapped_thp == 0))
-		return 0;
+		return;
 
 	if (!mmap_write_trylock(mm))
-		return -EBUSY;
+		return;
 
 	if (unlikely(khugepaged_test_exit(mm)))
 		goto out;
@@ -1553,7 +1553,6 @@ static int khugepaged_collapse_pte_mapped_thps(struct mm_slot *mm_slot)
 out:
 	mm_slot->nr_pte_mapped_thp = 0;
 	mmap_write_unlock(mm);
-	return 0;
 }
 
 static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff)
@@ -2057,9 +2056,8 @@ static void khugepaged_scan_file(struct mm_struct *mm,
 	BUILD_BUG();
 }
 
-static int khugepaged_collapse_pte_mapped_thps(struct mm_slot *mm_slot)
+static void khugepaged_collapse_pte_mapped_thps(struct mm_slot *mm_slot)
 {
-	return 0;
 }
 #endif
 
-- 
2.19.1

[PATCH 2/5] khugepaged: reuse the smp_wmb() inside __SetPageUptodate()

[Miaohe Lin]

smp_wmb() is needed to avoid the copy_huge_page writes to become visible
after the set_pmd_at() write here. But we can reuse the smp_wmb() inside
__SetPageUptodate() to remove this redundant one.

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

diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index d43812c5ce16..287e7ecf978c 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -1183,19 +1183,18 @@ static void collapse_huge_page(struct mm_struct *mm,
 	__collapse_huge_page_copy(pte, new_page, vma, address, pte_ptl,
 			&compound_pagelist);
 	pte_unmap(pte);
+	/*
+	 * spin_lock() below is not the equivalent of smp_wmb(), but
+	 * the smp_wmb() inside __SetPageUptodate() can be reused to
+	 * avoid the copy_huge_page writes to become visible after
+	 * the set_pmd_at() write.
+	 */
 	__SetPageUptodate(new_page);
 	pgtable = pmd_pgtable(_pmd);
 
 	_pmd = mk_huge_pmd(new_page, vma->vm_page_prot);
 	_pmd = maybe_pmd_mkwrite(pmd_mkdirty(_pmd), vma);
 
-	/*
-	 * spin_lock() below is not the equivalent of smp_wmb(), so
-	 * this is needed to avoid the copy_huge_page writes to become
-	 * visible after the set_pmd_at() write.
-	 */
-	smp_wmb();
-
 	spin_lock(pmd_ptl);
 	BUG_ON(!pmd_none(*pmd));
 	page_add_new_anon_rmap(new_page, vma, address, true);
-- 
2.19.1

[PATCH 3/5] khugepaged: use helper khugepaged_test_exit() in __khugepaged_enter()

[Miaohe Lin]

Commit 4d45e75a9955 ("mm: remove the now-unnecessary mmget_still_valid()
hack") have made khugepaged_test_exit() suitable for check mm->mm_users
against 0. Use this helper here.

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

diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index 287e7ecf978c..e886a8618c33 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -481,7 +481,7 @@ int __khugepaged_enter(struct mm_struct *mm)
 		return -ENOMEM;
 
 	/* __khugepaged_exit() must not run from under us */
-	VM_BUG_ON_MM(atomic_read(&mm->mm_users) == 0, mm);
+	VM_BUG_ON_MM(khugepaged_test_exit(mm), mm);
 	if (unlikely(test_and_set_bit(MMF_VM_HUGEPAGE, &mm->flags))) {
 		free_mm_slot(mm_slot);
 		return 0;
-- 
2.19.1

[PATCH 4/5] khugepaged: remove unnecessary mem_cgroup_uncharge() in collapse_[file|huge_page]

[Miaohe Lin]

Since commit 7ae88534cdd9 ("mm: move mem_cgroup_uncharge out of
__page_cache_release()"), the mem_cgroup will be uncharged when hpage is
freed. Uncharge mem_cgroup here is harmless but it looks confusing and
buggy: if mem_cgroup charge failed, we will call mem_cgroup_uncharge()
uncorrectly in error path because hpage is not IS_ERR_OR_NULL().

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

diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index e886a8618c33..68579cdbdc9b 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -1211,8 +1211,6 @@ static void collapse_huge_page(struct mm_struct *mm,
 out_up_write:
 	mmap_write_unlock(mm);
 out_nolock:
-	if (!IS_ERR_OR_NULL(*hpage))
-		mem_cgroup_uncharge(*hpage);
 	trace_mm_collapse_huge_page(mm, isolated, result);
 	return;
 out:
@@ -1968,8 +1966,6 @@ static void collapse_file(struct mm_struct *mm,
 	unlock_page(new_page);
 out:
 	VM_BUG_ON(!list_empty(&pagelist));
-	if (!IS_ERR_OR_NULL(*hpage))
-		mem_cgroup_uncharge(*hpage);
 	/* TODO: tracepoints */
 }
 
-- 
2.19.1

[PATCH 5/5] khugepaged: fix wrong result value for trace_mm_collapse_huge_page_isolate()

[Miaohe Lin]

In writable and !referenced case, the result value should be
SCAN_LACK_REFERENCED_PAGE for trace_mm_collapse_huge_page_isolate()
instead of default 0 (SCAN_FAIL) here.

Fixes: 7d2eba0557c1 ("mm: add tracepoint for scanning pages")
Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/khugepaged.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index 68579cdbdc9b..50100f1c7e53 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -716,17 +716,17 @@ static int __collapse_huge_page_isolate(struct vm_area_struct *vma,
 		if (pte_write(pteval))
 			writable = true;
 	}
-	if (likely(writable)) {
-		if (likely(referenced)) {
-			result = SCAN_SUCCEED;
-			trace_mm_collapse_huge_page_isolate(page, none_or_zero,
-							    referenced, writable, result);
-			return 1;
-		}
-	} else {
+
+	if (unlikely(!writable)) {
 		result = SCAN_PAGE_RO;
+	} else if (unlikely(!referenced)) {
+		result = SCAN_LACK_REFERENCED_PAGE;
+	} else {
+		result = SCAN_SUCCEED;
+		trace_mm_collapse_huge_page_isolate(page, none_or_zero,
+						    referenced, writable, result);
+		return 1;
 	}
-
 out:
 	release_pte_pages(pte, _pte, compound_pagelist);
 	trace_mm_collapse_huge_page_isolate(page, none_or_zero,
-- 
2.19.1

Re: [PATCH 4/5] khugepaged: remove unnecessary mem_cgroup_uncharge() in collapse_[file|huge_page]

[Kirill A. Shutemov]

On Thu, Mar 04, 2021 at 07:30:12AM -0500, Miaohe Lin wrote:
> Since commit 7ae88534cdd9 ("mm: move mem_cgroup_uncharge out of
> __page_cache_release()"), the mem_cgroup will be uncharged when hpage is
> freed. Uncharge mem_cgroup here is harmless but it looks confusing and
> buggy: if mem_cgroup charge failed, we will call mem_cgroup_uncharge()
> uncorrectly in error path because hpage is not IS_ERR_OR_NULL().
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Hm. I'm not sure about this patch.

For !NUMA the page will get allocated and freed very early: in
khugepaged_do_scan() and with the change mem_cgroup_charge() may get
called twice for two different mm_structs.

Is it safe?

-- 
 Kirill A. Shutemov

Re: [PATCH 0/5] Cleanup and fixup for khugepaged

[Kirill A. Shutemov]

On Thu, Mar 04, 2021 at 07:30:08AM -0500, Miaohe Lin wrote:
> Hi all,
> This series contains cleanups to remove unneeded return value, use
> helper function and so on. And there is one fix to correct the wrong
> result value for trace_mm_collapse_huge_page_isolate().
> 
> More details can be found in the respective changelogs. Thanks!
> 
> Miaohe Lin (5):
>   khugepaged: remove unneeded return value of
>     khugepaged_collapse_pte_mapped_thps()
>   khugepaged: reuse the smp_wmb() inside __SetPageUptodate()
>   khugepaged: use helper khugepaged_test_exit() in __khugepaged_enter()
>   khugepaged: remove unnecessary mem_cgroup_uncharge() in
>     collapse_[file|huge_page]
>   khugepaged: fix wrong result value for
>     trace_mm_collapse_huge_page_isolate()
> 
>  mm/khugepaged.c | 47 ++++++++++++++++++++---------------------------
>  1 file changed, 20 insertions(+), 27 deletions(-)

Apart from patch 4/5, looks fine. For the rest, you can use:

Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

-- 
 Kirill A. Shutemov

Re: [PATCH 0/5] Cleanup and fixup for khugepaged

[Miaohe Lin]

On 2021/3/6 1:40, Kirill A. Shutemov wrote:
> On Thu, Mar 04, 2021 at 07:30:08AM -0500, Miaohe Lin wrote:
>> Hi all,
>> This series contains cleanups to remove unneeded return value, use
>> helper function and so on. And there is one fix to correct the wrong
>> result value for trace_mm_collapse_huge_page_isolate().
>>
>> More details can be found in the respective changelogs. Thanks!
>>
>> Miaohe Lin (5):
>>   khugepaged: remove unneeded return value of
>>     khugepaged_collapse_pte_mapped_thps()
>>   khugepaged: reuse the smp_wmb() inside __SetPageUptodate()
>>   khugepaged: use helper khugepaged_test_exit() in __khugepaged_enter()
>>   khugepaged: remove unnecessary mem_cgroup_uncharge() in
>>     collapse_[file|huge_page]
>>   khugepaged: fix wrong result value for
>>     trace_mm_collapse_huge_page_isolate()
>>
>>  mm/khugepaged.c | 47 ++++++++++++++++++++---------------------------
>>  1 file changed, 20 insertions(+), 27 deletions(-)
> 
> Apart from patch 4/5, looks fine. For the rest, you can use:
> 
> Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

Many thanks for review and ack! :)

> 

Re: [PATCH 4/5] khugepaged: remove unnecessary mem_cgroup_uncharge() in collapse_[file|huge_page]

[Miaohe Lin]

On 2021/3/6 1:38, Kirill A. Shutemov wrote:
> On Thu, Mar 04, 2021 at 07:30:12AM -0500, Miaohe Lin wrote:
>> Since commit 7ae88534cdd9 ("mm: move mem_cgroup_uncharge out of
>> __page_cache_release()"), the mem_cgroup will be uncharged when hpage is
>> freed. Uncharge mem_cgroup here is harmless but it looks confusing and
>> buggy: if mem_cgroup charge failed, we will call mem_cgroup_uncharge()
>> uncorrectly in error path because hpage is not IS_ERR_OR_NULL().
>>
>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> 
> Hm. I'm not sure about this patch.
> 
> For !NUMA the page will get allocated and freed very early: in
> khugepaged_do_scan() and with the change mem_cgroup_charge() may get
> called twice for two different mm_structs.

Many thanks for point it out.

> > Is it safe?

I'am sorry I missed the !NUMA case! :(

In !NUMA case, hpage may not be freed in the khugepaged_do_scan() while loop. Thus mem_cgroup_charge()
may get called twice for two different mm_structs. In fact, mem_cgroup_uncharge() may also get called
twice __but__ it's safe to do this.

The imbalance of mem_cgroup_charge() and mem_cgroup_uncharge() looks buggy and weird __but__ it's safe
to call mem_cgroup_uncharge() many times with or without a successful mem_cgroup_charge() call.
So I would drop this patch.

> 

Thanks again.