Re: [PATCH v4 0/2] fix MADV_COLLAPSE issue if THP settings are disabled

[Baolin Wang <baolin.wang@linux.alibaba.com>]

On 2025/6/25 16:37, Lorenzo Stoakes wrote:
> On Wed, Jun 25, 2025 at 10:24:53AM +0200, David Hildenbrand wrote:
>> On 25.06.25 10:12, Lorenzo Stoakes wrote:
>>> On Wed, Jun 25, 2025 at 08:55:28AM +0100, Lorenzo Stoakes wrote:
>>>> I suppose the least awful way of addressing Baolin's concerns re: mTHP
>>>> while simultaneosly keeping existing semantics is:
>>>>
>>>> 1. Introduce deny to mean what never should have meant.
>>>
>>> To fix Baolin's issue btw we'd have to add 'deny' to both 'global' settings
>>> _and_ each page size setting.
>>>
>>> Because otherwise we'd end up in a weird case where say:
>>>
>>> global 'deny'
>>>
>>>    2 MiB 'never'
>>> 64 KiB 'inherit'
>>>
>>> And err... get 2 MiB THP pages from MADV_COLLAPSE :)
>>>
>>> Or:
>>>
>>> global 'deny'
>>>
>>>    2 MiB 'never'
>>> 64 KiB 'always'
>>>
>>> Or:
>>>
>>> global 'never'
>>>
>>>    2 MiB 'never'
>>> 64 KiB 'always'
>>>
>>> Or:
>>>
>>> global 'never'
>>>
>>>    2 MiB 'madvise'
>>> 64 KiB 'always'
>>>
>>> All doing the same. Not very clear is it?
>>>
>>> We have sowed the seeds of something terrible here, truly.
>>
>> Fully agreed. "Deny" is nasty. Maybe if we really need a way to disable
>> "madv_collapse", it should be done differently, not using this toggle here.
> 
> Yeah maybe the best way is to just have another tunable for this?
> 
> /sys/kernel/mm/transparent_hugepage/disable_collapse perhaps?
> 
> What do you think Hugh, Baolin?

I think it's not necessary to find a way to disable madvise_collapse. 
Essentially, it's a conflict between the semantics of madvise_collapse 
and the '/sys/kernel/mm/transparent_hugepage/enabled' interface. We 
should reach a consensus on the semantics first:

Semantic 1: madv_collapse() should ignore any THP system settings, 
meaning we need to update the 'never' semantics in 
'/sys/kernel/mm/transparent_hugepage/enabled', which would only disable 
page fault and khugepaged, not including madvise_collapse. If we agree 
on this, then the 'never' for per-sized mTHP would have the same 
semantics, i.e., when I set 64K mTHP to 'always' and 2M mTHP to 'never', 
madvise_collapse would still allow the collapse of 2M THP. We should 
document this clearly in case users still want 64K mTHP from 
madvise_collapse.


Semantic 2: madv_collapse() needs to respect THP system settings, which 
is what my patch does. Never means never, and we would need to update 
the documentation of madv_collapse() to make it clearer.

>> Regarding MADV_COLLAPSE, I strongly assume that we should not change it to
>> collapse smaller mTHPs as part of the khugepaged mTHP work. For now, it will
>> simply always collapse to PMD THPs.
> 
> Yeah thinking about it maybe this is the best way. And we can then update
> the man page to make this ABUNDANTLY clear (am happy to do this).
> 
> This keeps things simple.

Yes, agree.

> (One side note on PMD-sized MADV_COLLAPSE - this is basically completely
> useless for 64 KB page size arm64 systems where PMD's are 512 MB :)
> 
> Thoughts Baolin?

We should not collapse 512MB THP on 64K pagesize kernel. So seems 
madv_collapse() can not work on 64K pagesize kernel.

>> Once we want to support other sizes, likely MADV_COLLAPSE users want to have
>> better control over which size to use, at which point it all gets nasty.
> 
> madvise2() this time with extra parameters? ;)
> 
> I sort of wish we had added a flags parameter there.
> 
> But lacking a time machine... :)
> 
>>
>> --
>> Cheers,
>>
>> David / dhildenb
>>
> 
> To summarise:
> 
> Drop series:
> 
> * Might degrade performance for very specific users using
>    never/MADV_COLLAPSE (quite possibly via process_madvise() + a remote
>    process).
> 
> * Matches 'de jure' interpretation of documentation.
> 
> Keep series:
> 
> * Provides no means whatsoever to have a 'manual only' collapse mode,
>    though does provide for manual khugepaged THP.
> 
> * MADV_COLLAPSE automatically gets mTHP support based on obeying 'never'.
> 
> * Matches likely 'de facto' understanding system admins have about THP
>    usage.
> 
> Action items:
> 
> * Either way, I (Lorenzo) will improve documentation.

Great. Thanks.

> * If we drop the series, provide another means to disable
>    MADV_COLLAPSE. But not using existing sysfs toggles, something new. We
>    will document MADV_COLLAPSE as PMD only.
> 
> * If we drop the series, also consider how we might provide mTHP-compatible
>    MADV_COLLAPSE.

Yes. Agree. Will be another mess I guess :)

> 
> * Totally and completely refactor the hell out of the THP implementation
>    from top-to-bottom (over time this is becoming more and more of a me
>    thing... as I'm getting ever more frustrated with the implementation ;)

Yes.