diff mbox series

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

Message ID 20210304123013.23560-5-linmiaohe@huawei.com (mailing list archive)
State New
Headers show
Series Cleanup and fixup for khugepaged | expand

Commit Message

Miaohe Lin March 4, 2021, 12:30 p.m. UTC
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(-)

Comments

Kirill A. Shutemov March 5, 2021, 5:38 p.m. UTC | #1
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?
Miaohe Lin March 6, 2021, 3:18 a.m. UTC | #2
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.
diff mbox series

Patch

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 */
 }