Re: [PATCH 21/46] hugetlb: use struct hugetlb_pte for walk_hugetlb_range

[James Houghton]

On Thu, Jan 19, 2023 at 3:07 PM Peter Xu <peterx@xxxxxxxxxx> wrote:
>
> On Thu, Jan 19, 2023 at 02:35:12PM -0800, James Houghton wrote:
> > On Thu, Jan 19, 2023 at 2:23 PM Peter Xu <peterx@xxxxxxxxxx> wrote:
> > >
> > > On Thu, Jan 19, 2023 at 02:00:32PM -0800, Mike Kravetz wrote:
> > > > I do not know much about the (primary) live migration use case.  My
> > > > guess is that page table lock contention may be an issue?  In this use
> > > > case, HGM is only enabled for the duration the live migration operation,
> > > > then a MADV_COLLAPSE is performed.  If contention is likely to be an
> > > > issue during this time, then yes we would need to pass around with
> > > > something like hugetlb_pte.
> > >
> > > I'm not aware of any such contention issue.  IMHO the migration problem is
> > > majorly about being too slow transferring a page being so large.  Shrinking
> > > the page size should resolve the major problem already here IIUC.
> >
> > This will be problematic if you scale up VMs to be quite large.
>
> Do you mean that for the postcopy use case one can leverage e.g. 2M
> mappings (over 1G) to avoid lock contentions when VM is large I agree it
> should be more efficient than having 512 4K page installed, but I think
> it'll make the page fault resolution slower too if some thead is only
> looking for a 4k portion of it.

No, that's not what I meant. Sorry. If you can use the PTL that is
normally used for 4K PTEs, then you're right, there is no contention
problem. However, this PTL is determined by the value of the PMD, so
you need a pointer to the PMD to determine what the PTL should be (or
a pointer to the PTL itself).

In hugetlb, we only ever pass around the PTE pointer, and we rely on
huge_pte_lockptr() to find the PTL for us (and it does so
appropriately for everything except 4K PTEs). We would need to add the
complexity of passing around a PMD or PTL everywhere, and that's what
hugetlb_pte does for us. So that complexity is basically unavoidable,
unless you're ok with 4K PTEs with taking mm->page_table_lock (I'm
not).

>
> > Google upstreamed the "TDP MMU" for KVM/x86 that removed the need to take
> > the MMU lock for writing in the EPT violation path. We found that this
> > change is required for VMs >200 or so vCPUs to consistently avoid CPU
> > soft lockups in the guest.
>
> After the kvm mmu rwlock convertion, it'll allow concurrent page faults
> even if only 4K pages are used, so it seems not directly relevant to what
> we're discussing here, no?

Right. I was just bringing it up to say that if 4K PTLs were
mm->page_table_lock, we would have a problem.

>
> >
> > Requiring each UFFDIO_CONTINUE (in the post-copy path) to serialize on
> > the same PTL would be problematic in the same way.
>
> Pte-level pgtable lock only covers 2M range, so I think it depends on which
> is the address that the vcpu is faulted on?  IIUC the major case should be
> that the faulted threads are not falling upon the same 2M range.

Right. I think my comment should make more sense with the above clarification.

>
> >
> > >
> > > AFAIU 4K-only solution should only reduce any lock contention because locks
> > > will always be pte-level if VM_HUGETLB_HGM set.  When walking and creating
> > > the intermediate pgtable entries we can use atomic ops just like generic
> > > mm, so no lock needed at all.  With uncertainty on the size of mappings,
> > > we'll need to take any of the multiple layers of locks.
> > >
> >
> > Other than taking the HugeTLB VMA lock for reading, walking/allocating
> > page tables won't need any additional locking.
>
> Actually when revisiting the locks I'm getting a bit confused on whether
> the vma lock is needed if pmd sharing is anyway forbidden for HGM.  I
> raised a question in the other patch of MADV_COLLAPSE, maybe they're
> related questions so we can keep it there.

We can discuss there. :) I take both the VMA lock and mapping lock so
that it can stay in sync with huge_pmd_unshare(), and so HGM walks
have the same synchronization as regular hugetlb PT walks.

>
> >
> > We take the PTL to allocate the next level down, but so does generic
> > mm (look at __pud_alloc, __pmd_alloc for example). Maybe I am
> > misunderstanding.
>
> Sorry you're right, please ignore that.  I don't know why I had that
> impression that spinlocks are not needed in that process.
>
> Actually I am also curious why atomics won't work (by holding mmap read
> lock, then do cmpxchg(old_entry=0, new_entry) upon the pgtable entries).  I
> think it's possible I just missed something else.

I think there are cases where we need to make sure the value of a PTE
isn't going to change from under us while we're doing some kind of
other operation, and so a compare-and-swap won't really be what we
need.