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@xxxxxxxxxx>
> 
> 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.