Re: [PATCH v3 0/3] A Solution to Re-enable hugetlb vmemmap optimize

[Yu Zhao]

On Wed, Jul 10, 2024 at 5:07 PM Yu Zhao <yuzhao@xxxxxxxxxx> wrote:
>
> On Wed, Jul 10, 2024 at 4:29 PM Catalin Marinas <catalin.marinas@xxxxxxx> wrote:
> >
> > On Wed, Jul 10, 2024 at 11:12:01AM -0600, Yu Zhao wrote:
> > > On Wed, Jul 10, 2024 at 10:51 AM Catalin Marinas
> > > <catalin.marinas@xxxxxxx> wrote:
> > > > On Fri, Jul 05, 2024 at 11:41:34AM -0600, Yu Zhao wrote:
> > > > > On Fri, Jul 5, 2024 at 9:49 AM Catalin Marinas <catalin.marinas@xxxxxxx> wrote:
> > > > > > If I did the maths right, for a 2MB hugetlb page, we have about 8
> > > > > > vmemmap pages (32K). Once we split a 2MB vmemap range,
> > > > >
> > > > > Correct.
> > > > >
> > > > > > whatever else
> > > > > > needs to be touched in this range won't require a stop_machine().
> > > > >
> > > > > There might be some misunderstandings here.
> > > > >
> > > > > To do HVO:
> > > > > 1. we split a PMD into 512 PTEs;
> > > > > 2. for every 8 PTEs:
> > > > >   2a. we allocate an order-0 page for PTE #0;
> > > > >   2b. we remap PTE #0 *RW* to this page;
> > > > >   2c. we remap PTEs #1-7 *RO* to this page;
> > > > >   2d. we free the original order-3 page.
> > > >
> > > > Thanks. I now remember why we reverted such support in 060a2c92d1b6
> > > > ("arm64: mm: hugetlb: Disable HUGETLB_PAGE_OPTIMIZE_VMEMMAP"). The main
> > > > problem is that point 2c also changes the output address of the PTE
> > > > (and the content of the page slightly). The architecture requires a
> > > > break-before-make in such scenario, though it would have been nice if it
> > > > was more specific on what could go wrong.
> > > >
> > > > We can do point 1 safely if we have FEAT_BBM level 2. For point 2, I
> > > > assume these 8 vmemmap pages may be accessed and that's why we can't do
> > > > a break-before-make safely.
> > >
> > > Correct
> > >
> > > > I was wondering whether we could make the
> > > > PTEs RO first and then change the output address but we have another
> > > > rule that the content of the page should be the same. I don't think
> > > > entries 1-7 are identical to entry 0 (though we could ask the architects
> > > > for clarification here). Also, can we guarantee that nothing writes to
> > > > entry 0 while we would do such remapping?
> > >
> > > Yes, it's already guaranteed.
> > >
> > > > We know entries 1-7 won't be
> > > > written as we mapped them as RO but entry 0 contains the head page.
> > > > Maybe it's ok to map it RO temporarily until the newly allocated hugetlb
> > > > page is returned.
> > >
> > > We can do that. I don't understand how this could elide BBM. After the
> > > above, we would still need to do:
> > > 3. remap entry 0 from RO to RW, mapping the `struct page` page that
> > > will be shared with entry 1-7.
> > > 4. remap entry 1-7 from their respective `struct page` pages to that
> > > of entry 0, while they remain RO.
> >
> > The Arm ARM states that we need a BBM if we change the output address
> > and: the old or new mappings are RW *or* the content of the page
> > changes. Ignoring the latter (page content), we can turn the PTEs RO
> > first without changing the pfn followed by changing the pfn while they
> > are RO. Once that's done, we make entry 0 RW and, of course, with
> > additional TLBIs between all these steps.
>
> Aha! This is easy to do -- I just made the RO guaranteed, as I
> mentioned earlier.
>
> Just to make sure I fully understand the workflow:
>
> 1. Split a RW PMD into 512 RO PTEs, pointing to the same 2MB `struct page` area.
> 2. TLBI once, after pmd_populate_kernel()
> 3. Remap PTE 1-7 to the 4KB `struct page` area of PTE 0, for every 8
> PTEs, while they remain RO.
> 4. TLBI once, after set_pte_at() on PTE 1-7.
> 5. Change PTE 0 from RO to RW, pointing to the same 4KB `struct page` area.
> 6. TLBI once, after set_pte_at() on PTE 0.
>
> No BBM required, regardless of FEAT_BBM level 2.

I just studied D8.16.1 from the reference manual, and it seems to me:
1. We still need either FEAT_BBM or BBM to split PMD.
2. We still need BBM when we change PTE 1-7, because even if they
remain RO, the content of the `struct page` page at the new location
does not match that at the old location.

> Is this correct?
>
> > Can we leave entry 0 RO? This
> > would save an additional TLBI.
>
> Unfortunately we can't. Otherwise we wouldn't be able to, e.g., grab a
> refcnt on any hugeTLB pages.
>
> > Now, I wonder if all this is worth it. What are the scenarios where the
> > 8 PTEs will be accessed? The vmemmap range corresponding to a 2MB
> > hugetlb page for example is pretty well defined - 8 x 4K pages, aligned.

One of the fundamental assumptions in core MM is that anyone can
read or try to grab (write) a refcnt from any `struct page`. Those
speculative PFN walkers include memory compaction, etc.


> > > > If we could get the above work, it would be a lot simpler than thinking
> > > > of stop_machine() or other locks to wait for such remapping.
> > >
> > > Steps 3/4 would not require BBM somehow?
> >
> > If we ignore the 'content' requirement, I think we could skip the BBM
> > but we need to make sure we don't change the permission and pfn at the
> > same time.
>
> Gotcha.
>
> > > > > To do de-HVO:
> > > > > 1. for every 8 PTEs:
> > > > >   1a. we allocate 7 order-0 pages.
> > > > >   1b. we remap PTEs #1-7 *RW* to those pages, respectively.
> > > >
> > > > Similar problem in 1.b, changing the output address. Here we could force
> > > > the content to be the same
> > >
> > > I don't follow the "the content to be the same" part. After HVO, we have:
> > >
> > > Entry 0 -> `struct page` page A, RW
> > > Entry 1 -> `struct page` page A, RO
> > > ...
> > > Entry 7 -> `struct page` page A, RO
> > >
> > > To de-HVO, we need to make them:
> > >
> > > Entry 0 -> `struct page` page A, RW
> > > Entry 1 -> `struct page` page B, RW
> > > ...
> > > Entry 7 -> `struct page` page H, RW
> > >
> > > I assume the same content means PTE_0 == PTE_1/.../7?
> >
> > That's the content of the page at the corresponding pfn before and after
> > the pte change. I'm pretty sure the Arm ARM states this in case the
> > hardware starts a load (e.g. unaligned) from one page and completes it
> > from another, the software should not see any difference. But for the
> > fields we care about in struct page, I assume they'd be the same (or
> > that we just don't care about inconsistencies during this transient
> > period).
>
> Thanks for the explanation. I'll cook up something if my understanding
> above is correct.