On Thu, Aug 21, 2025 at 09:27:19AM -0600, Nico Pache wrote: > On Thu, Aug 21, 2025 at 9:25 AM Nico Pache <npache@xxxxxxxxxx> wrote: > > > > On Thu, Aug 21, 2025 at 9:20 AM Lorenzo Stoakes > > <lorenzo.stoakes@xxxxxxxxxx> wrote: > > > > > > On Thu, Aug 21, 2025 at 08:43:18PM +0530, Dev Jain wrote: > > > > > > > > On 21/08/25 8:31 pm, Lorenzo Stoakes wrote: > > > > > OK so I noticed in patch 13/13 (!) where you change the documentation that you > > > > > essentially state that the whole method used to determine the ratio of PTEs to > > > > > collapse to mTHP is broken: > > > > > > > > > > khugepaged uses max_ptes_none scaled to the order of the enabled > > > > > mTHP size to determine collapses. When using mTHPs it's recommended > > > > > to set max_ptes_none low-- ideally less than HPAGE_PMD_NR / 2 (255 > > > > > on 4k page size). This will prevent undesired "creep" behavior that > > > > > leads to continuously collapsing to the largest mTHP size; when we > > > > > collapse, we are bringing in new non-zero pages that will, on a > > > > > subsequent scan, cause the max_ptes_none check of the +1 order to > > > > > always be satisfied. By limiting this to less than half the current > > > > > order, we make sure we don't cause this feedback > > > > > loop. max_ptes_shared and max_ptes_swap have no effect when > > > > > collapsing to a mTHP, and mTHP collapse will fail on shared or > > > > > swapped out pages. > > > > > > > > > > This seems to me to suggest that using > > > > > /sys/kernel/mm/transparent_hugepage/khugepaged/max_ptes_none as some means > > > > > of establishing a 'ratio' to do this calculation is fundamentally flawed. > > > > > > > > > > So surely we ought to introduce a new sysfs tunable for this? Perhaps > > > > > > > > > > /sys/kernel/mm/transparent_hugepage/khugepaged/mthp_max_ptes_none_ratio > > > > > > > > > > Or something like this? > > > > > > > > > > It's already questionable that we are taking a value that is expressed > > > > > essentially in terms of PTE entries per PMD and then use it implicitly to > > > > > determine the ratio for mTHP, but to then say 'oh but the default value is > > > > > known-broken' is just a blocker for the series in my opinion. > > > > > > > > > > This really has to be done a different way I think. > > > > > > > > > > Cheers, Lorenzo > > > > > > > > FWIW this was my version of the documentation patch: > > > > https://lore.kernel.org/all/20250211111326.14295-18-dev.jain@xxxxxxx/ > > > > > > > > The discussion about the creep problem started here: > > > > https://lore.kernel.org/all/7098654a-776d-413b-8aca-28f811620df7@xxxxxxx/ > > > > > > > > and the discussion continuing here: > > > > https://lore.kernel.org/all/37375ace-5601-4d6c-9dac-d1c8268698e9@xxxxxxxxxx/ > > > > > > > > ending with a summary I gave here: > > > > https://lore.kernel.org/all/8114d47b-b383-4d6e-ab65-a0e88b99c873@xxxxxxx/ > > > > > > > > This should help you with the context. > > > > > > > > > > > > > > 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? > > > > 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? And we can make anything workable from a code management point of view by refactoring/developing appropriately. And given you're now proposing changing the default for even THP pages with a cap or perhaps having mTHP being used silently change the cap - that is clearly _far_ worse from a tuning standpoint. With a new tunable you can just set a sensible default and people don't even necessarily have to think about it.
