| Message ID | 20210304123013.23560-5-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Cleanup and fixup for khugepaged | expand |
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?
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 --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 */ }
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(-)