Message-ID: <e2e31d1d-5214-47f2-8400-6a30caf5d0fa@redhat.com>
Date: Thu, 21 Aug 2025 19:26:57 +0200
From: David Hildenbrand <david@...hat.com>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>,
 Nico Pache <npache@...hat.com>
Cc: Dev Jain <dev.jain@....com>, linux-mm@...ck.org,
 linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-trace-kernel@...r.kernel.org, ziy@...dia.com,
 baolin.wang@...ux.alibaba.com, Liam.Howlett@...cle.com,
 ryan.roberts@....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, zokeefe@...gle.com,
 hannes@...xchg.org, rientjes@...gle.com, mhocko@...e.com,
 rdunlap@...radead.org, hughd@...gle.com
Subject: Re: [PATCH v10 00/13] khugepaged: mTHP support

On 21.08.25 18:54, Lorenzo Stoakes wrote:
> On Thu, Aug 21, 2025 at 10:46:18AM -0600, Nico Pache wrote:
>>>>>> Thanks and I"ll have a look, but this series is unmergeable with a broken
>>>>>> default in
>>>>>> /sys/kernel/mm/transparent_hugepage/khugepaged/mthp_max_ptes_none_ratio
>>>>>> sorry.
>>>>>>
>>>>>> We need to have a new tunable as far as I can tell. I also find the use of
>>>>>> this PMD-specific value as an arbitrary way of expressing a ratio pretty
>>>>>> gross.
>>>>> The first thing that comes to mind is that we can pin max_ptes_none to
>>>>> 255 if it exceeds 255. It's worth noting that the issue occurs only
>>>>> for adjacently enabled mTHP sizes.
>>>
>>> No! Presumably the default of 511 (for PMDs with 512 entries) is set for a
>>> reason, arbitrarily changing this to suit a specific case seems crazy no?
>> We wouldn't be changing it for PMD collapse, just for the new
>> behavior. At 511, no mTHP collapses would ever occur anyways, unless
>> you have 2MB disabled and other mTHP sizes enabled. Technically at 511
>> only the highest enabled order always gets collapsed.
>>
>> Ive also argued in the past that 511 is a terrible default for
>> anything other than thp.enabled=always, but that's a whole other can
>> of worms we dont need to discuss now.
>>
>> with this cap of 255, the PMD scan/collapse would work as intended,
>> then in mTHP collapses we would never introduce this undesired
>> behavior. We've discussed before that this would be a hard problem to
>> solve without introducing some expensive way of tracking what has
>> already been through a collapse, and that doesnt even consider what
>> happens if things change or are unmapped, and rescanning that section
>> would be helpful. So having a strictly enforced limit of 255 actually
>> seems like a good idea to me, as it completely avoids the undesired
>> behavior and does not require the admins to be aware of such an issue.
>>
>> Another thought similar to what (IIRC) Dev has mentioned before, if we
>> have max_ptes_none > 255 then we only consider collapses to the
>> largest enabled order, that way no creep to the largest enabled order
>> would occur in the first place, and we would get there straight away.
>>
>> To me one of these two solutions seem sane in the context of what we
>> are dealing with.
>>>
>>>>>
>>>>> ie)
>>>>> if order!=HPAGE_PMD_ORDER && khugepaged_max_ptes_none > 255
>>>>>        temp_max_ptes_none = 255;
>>>> Oh and my second point, introducing a new tunable to control mTHP
>>>> collapse may become exceedingly complex from a tuning and code
>>>> management standpoint.
>>>
>>> Umm right now you hve a ratio expressed in PTES per mTHP * ((PTEs per PMD) /
>>> PMD) 'except please don't set to the usual default when using mTHP' and it's
>>> currently default-broken.
>>>
>>> I'm really not sure how that is simpler than a seprate tunable that can be
>>> expressed as a ratio (e.g. percentage) that actually makes some kind of sense?
>> I agree that the current tunable wasn't designed for this, but we
>> tried to come up with something that leverages the tunable we have to
>> avoid new tunables and added complexity.
>>>
>>> And we can make anything workable from a code management point of view by
>>> refactoring/developing appropriately.
>> What happens if max_ptes_none = 0 and the ratio is 50% - 1 pte
>> (ideally the max number)? seems like we would be saying we want no new
>> none pages, but also to allow new none pages. To me that seems equally
>> broken and more confusing than just taking a scale of the current
>> number (now with a cap).
>>
>>
> 
> The one thing we absolutely cannot have is a default that causes this
> 'creeping' behaviour. This feels like shipping something that is broken and
> alluding to it in the documentation.
> 
> I spoke to David off-list and he gave some insight into this and perhaps
> some reasonable means of avoiding an additional tunable.
> 
> I don't want to rehash what he said as I think it's more productive for him
> to reply when he has time but broadly I think how we handle this needs
> careful consideration.
> 
> To me it's clear that some sense of ratio is just immediately very very
> confusing, but then again this interface is already confusing, as with much
> of THP.
> 
> Anyway I'll let David respond here so we don't loop around before he has a
> chance to add his input.

I've been summoned.

As raised in the past, I would initially only support specific values here like

0 				  : Never collapse with any pte_none/zeropage
511 (HPAGE_PMD_NR - 1) / default  : Always collapse, ignoring pte_none/zeropage

Once could also easily support the value 255 (HPAGE_PMD_NR / 2- 1), but not sure
if we have to add that for now.

Because, as raised in the past, I'm afraid nobody on this earth has a clue how
to set this parameter to values different to 0 (don't waste memory with khugepaged)
and 511 (page fault behavior).


If any other value is set, essentially
	pr_warn("Unsupported 'max_ptes_none' value for mTHP collapse");

for now and just disable it.

-- 
Cheers

David / dhildenb

Powered by blists - more mailing lists