Re: [RFC 00/11] khugepaged: mTHP support

Dev Jain
2025-01-27T09:32:04Z

On 21/01/25 3:49 pm, David Hildenbrand wrote:
>> Hmm that's an interesting idea; If I've understood, we would
>> effectively test
>> the PMD for collapse as if we were collapsing to PMD-size, but then do
>> the
>> actual collapse to the "highest allowed order" (dictated by what's
>> enabled +
>> MADV_HUGEPAGE config).
>>
>> I'm not so sure this is a good way to go; there would be no way to
>> support VMAs
>> (or parts of VMAs) that don't span a full PMD.
>
>
> In Nicos approach to locking, we temporarily have to remove the PTE
> table either way. While holding the mmap lock in write mode, the VMAs
> cannot go away, so we could scan the whole PTE table to figure it out.
>
> To just figure out "none" vs. "non-none" vs. "swap PTE", we'd probably
> don't need the other VMA information. Figuring out "shared" is trickier,
> because we have to obtain the folio and would have to walk the other VMAs.
>
> It's a good question if we would have to VMA-write-lock the other
> affected VMAs as well in order to temporarily remove the PTE table that
> crosses multiple VMAs, or if we'd need something different (collapse PMD
> marker) so the page table walkers could handle that case properly --
> keep retrying or fallback to the mmap lock.

I missed this reply, could have saved me some trouble :) When collapsing
for VMAs < PMD, we *will* have to write lock the VMAs, write lock the
anon_vma's, and write lock vma->vm_file->f_mapping for file VMAs,
otherwise someone may fault on another VMA mapping the same PTE table. I
was trying to implement this, but cannot find a clean way: we will have
to implement it like mm_take_all_locks(), with a similar bit like
AS_MM_ALL_LOCKS, because, suppose we need to lock all anon_vma's, then
two VMAs may have the same anon_vma, and we cannot get away with the
following check:

lock only if !rwsem_is_locked(&vma->anon_vma->root->rwsem)

since I need to skip the lock only when it is khugepaged which has taken
the lock.

I guess the way to go about this then is the PMD-marker thingy, which I
am not very familiar with.

>
>> And I can imagine we might see
>> memory bloat; imagine you have 2M=madvise, 64K=always,
>> max_ptes_none=511, and
>> let's say we have a 2M (aligned portion of a) VMA that does NOT have
>> MADV_HUGEPAGE set and has a single page populated. It passes the PMD-
>> size test,
>> but we opt to collapse to 64K (since 2M=madvise). So now we end up
>> with 32x 64K
>> folios, 31 of which are all zeros. We have spent the same amount of
>> memory as if
>> 2M=always. Perhaps that's a detail that could be solved by ignoring
>> fully none
>> 64K blocks when collapsing to 64K...
>
> Yes, that's what I had in mind. No need to collapse where there is
> nothing at all ...
>
>>
>> Personally, I think your "enforce simplicifation of the tunables for mTHP
>> collapse" idea is the best we have so far.
>
> Right.
>
>>
>> But I'll just push against your pushback of the per-VMA cursor idea
>> briefly. It
>> strikes me that this could be useful for khugepaged regardless of mTHP
>> support.
>
> Not a clear pushback, as you say to me this is a different optimization
> and I am missing how it could really solve the problem at hand here.
>
> Note that we're already fighting with not growing VMAs (see the VMA
> locking changes under review), but maybe we could still squeeze it in
> there without requiring a bigger slab.
>
>> Today, it starts scanning a VMA, collapses the first PMD it finds that
>> meets the
>> requirements, then switches to scanning another VMA. When it
>> eventually gets
>> back to scanning the first VMA, it starts from the beginning again.
>> Wouldn't a
>> cursor help reduce the amount of scanning it has to do?
>
> Yes, that whole scanning approach sound weird. I would have assumed that
> it might nowdays be smarter to just scan the MM sequentially, and not
> jump between VMAs.
>
> Assume you only have a handfull of large VMAs (like in a VMM), you'd end
> up scanning the same handful of VMAs over and over again.
>
> I think a lot of the khugepaged codebase is just full with historical
> baggage that must be cleaned up and re-validated if it still required ...
>