Message-ID: <1f00fdc3-a3a3-464b-8565-4c1b23d34f8d@linux.alibaba.com>
Date: Thu, 22 May 2025 17:39:26 +0800
From: Baolin Wang <baolin.wang@...ux.alibaba.com>
To: Nico Pache <npache@...hat.com>, David Rientjes <rientjes@...gle.com>,
 zokeefe@...gle.com
Cc: linux-mm@...ck.org, linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org,
 david@...hat.com, ziy@...dia.com, lorenzo.stoakes@...cle.com,
 Liam.Howlett@...cle.com, ryan.roberts@....com, dev.jain@....com,
 corbet@....net, rostedt@...dmis.org, mhiramat@...nel.org,
 mathieu.desnoyers@...icios.com, akpm@...ux-foundation.org,
 baohua@...nel.org, willy@...radead.org, peterx@...hat.com,
 wangkefeng.wang@...wei.com, usamaarif642@...il.com, sunnanyong@...wei.com,
 vishal.moola@...il.com, thomas.hellstrom@...ux.intel.com,
 yang@...amperecomputing.com, kirill.shutemov@...ux.intel.com,
 aarcange@...hat.com, raquini@...hat.com, anshuman.khandual@....com,
 catalin.marinas@....com, tiwai@...e.de, will@...nel.org,
 dave.hansen@...ux.intel.com, jack@...e.cz, cl@...two.org,
 jglisse@...gle.com, surenb@...gle.com, hannes@...xchg.org, mhocko@...e.com,
 rdunlap@...radead.org
Subject: Re: [PATCH v7 06/12] khugepaged: introduce khugepaged_scan_bitmap for
 mTHP support



On 2025/5/21 18:23, Nico Pache wrote:
> On Tue, May 20, 2025 at 4:09 AM Baolin Wang
> <baolin.wang@...ux.alibaba.com> wrote:
>>
>> Sorry for late reply.
>>
>> On 2025/5/17 14:47, Nico Pache wrote:
>>> On Thu, May 15, 2025 at 9:20 PM Baolin Wang
>>> <baolin.wang@...ux.alibaba.com> wrote:
>>>>
>>>>
>>>>
>>>> On 2025/5/15 11:22, Nico Pache wrote:
>>>>> khugepaged scans anons PMD ranges for potential collapse to a hugepage.
>>>>> To add mTHP support we use this scan to instead record chunks of utilized
>>>>> sections of the PMD.
>>>>>
>>>>> khugepaged_scan_bitmap uses a stack struct to recursively scan a bitmap
>>>>> that represents chunks of utilized regions. We can then determine what
>>>>> mTHP size fits best and in the following patch, we set this bitmap while
>>>>> scanning the anon PMD. A minimum collapse order of 2 is used as this is
>>>>> the lowest order supported by anon memory.
>>>>>
>>>>> max_ptes_none is used as a scale to determine how "full" an order must
>>>>> be before being considered for collapse.
>>>>>
>>>>> When attempting to collapse an order that has its order set to "always"
>>>>> lets always collapse to that order in a greedy manner without
>>>>> considering the number of bits set.
>>>>>
>>>>> Signed-off-by: Nico Pache <npache@...hat.com>
>>>>
>>>> Sigh. You still haven't addressed or explained the issues I previously
>>>> raised [1], so I don't know how to review this patch again...
>>> Can you still reproduce this issue?
>>
>> Yes, I can still reproduce this issue with today's (5/20) mm-new branch.
>>
>> I've disabled PMD-sized THP in my system:
>> [root]# cat /sys/kernel/mm/transparent_hugepage/enabled
>> always madvise [never]
>> [root]# cat /sys/kernel/mm/transparent_hugepage/hugepages-2048kB/enabled
>> always inherit madvise [never]
>>
>> And I tried calling madvise() with MADV_COLLAPSE for anonymous memory,
>> and I can still see it collapsing to a PMD-sized THP.
> Hi Baolin ! Thank you for your reply and willingness to test again :)
> 
> I didn't realize we were talking about madvise collapse-- this makes
> sense now. I also figured out why I could "reproduce" it before. My
> script was always enabling the THP settings in two places, and I only
> commented out one to test this. But this time I was doing more manual
> testing.
> 
> The original design of madvise_collapse ignores the sysfs and
> collapses even if you have an order disabled. I believe this behavior
> is wrong, but by design. I spent some time playing around with madvise
> collapses with and w/o my changes. This is not a new thing, I
> reproduced the issue in 6.11 (Fedora 41), and I think its been
> possible since the inception of madvise collapse 3 years ago. I
> noticed a similar behavior on one of my RFC since it was "breaking"
> selftests, and the fix was to reincorporate this broken sysfs
> behavior.

OK. Thanks for the explanation.

> 7d8faaf15545 ("mm/madvise: introduce MADV_COLLAPSE sync hugepage collapse")
> "This call is independent of the system-wide THP sysfs settings, but
> will fail for memory marked VM_NOHUGEPAGE."
> 
> The second condition holds true (and fails for VM_NOHUGEPAGE), but I
> dont know if we actually want madvise_collapse to be independent of
> the system-wide.

This design principle surprised me a bit, and I failed to find the 
reason in the commit log. I agree that "never should mean never," and we 
should respect the THP/mTHP sysfs setting. Additionally, for the 
'shmem_enabled' sysfs interface controlled for shmem/tmpfs, THP collapse 
can still be prohibited through the 'deny' configuration. The rules here 
are somewhat confusing.

> So I'll ask the authors
> +David Rientjes +zokeefe@...gle.com
> Was this brought up as a concern when this feature was first
> introduced, was there any pushback, what was the outcome of the
> discussion if so?
> I can easily fix this and it would further simplify the code (by
> removing the is_khugepaged and friends). As David H. has brought up in
> other discussions around similar topics, never should mean never, is
> this the only exception we should allow?

I don't think we need this exception, unless there is some solid reason.

Powered by blists - more mailing lists