Message-ID: <CADrL8HVU5qvh5Y_aT-ksvNQhzCRMmh7oNi-Og885DrFJesOYPA@mail.gmail.com>
Date:   Tue, 13 Dec 2022 19:04:58 -0500
From:   James Houghton <jthoughton@...gle.com>
To:     Mike Kravetz <mike.kravetz@...cle.com>
Cc:     Muchun Song <songmuchun@...edance.com>,
        Peter Xu <peterx@...hat.com>,
        David Hildenbrand <david@...hat.com>,
        David Rientjes <rientjes@...gle.com>,
        Axel Rasmussen <axelrasmussen@...gle.com>,
        Mina Almasry <almasrymina@...gle.com>,
        "Zach O'Keefe" <zokeefe@...gle.com>,
        Manish Mishra <manish.mishra@...anix.com>,
        Naoya Horiguchi <naoya.horiguchi@....com>,
        "Dr . David Alan Gilbert" <dgilbert@...hat.com>,
        "Matthew Wilcox (Oracle)" <willy@...radead.org>,
        Vlastimil Babka <vbabka@...e.cz>,
        Baolin Wang <baolin.wang@...ux.alibaba.com>,
        Miaohe Lin <linmiaohe@...wei.com>,
        Yang Shi <shy828301@...il.com>,
        Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH v2 11/47] hugetlb: add hugetlb_pmd_alloc and hugetlb_pte_alloc

On Tue, Dec 13, 2022 at 3:18 PM James Houghton <jthoughton@...gle.com> wrote:
>
> On Tue, Dec 13, 2022 at 2:32 PM Mike Kravetz <mike.kravetz@...cle.com> wrote:
> >
> > On 10/21/22 16:36, James Houghton wrote:
> > > These functions are used to allocate new PTEs below the hstate PTE. This
> > > will be used by hugetlb_walk_step, which implements stepping forwards in
> > > a HugeTLB high-granularity page table walk.
> > >
> > > The reasons that we don't use the standard pmd_alloc/pte_alloc*
> > > functions are:
> > >  1) This prevents us from accidentally overwriting swap entries or
> > >     attempting to use swap entries as present non-leaf PTEs (see
> > >     pmd_alloc(); we assume that !pte_none means pte_present and
> > >     non-leaf).
> > >  2) Locking hugetlb PTEs can different than regular PTEs. (Although, as
> > >     implemented right now, locking is the same.)
> > >  3) We can maintain compatibility with CONFIG_HIGHPTE. That is, HugeTLB
> > >     HGM won't use HIGHPTE, but the kernel can still be built with it,
> > >     and other mm code will use it.
> > >
> > > When GENERAL_HUGETLB supports P4D-based hugepages, we will need to
> > > implement hugetlb_pud_alloc to implement hugetlb_walk_step.
> > >
> > > Signed-off-by: James Houghton <jthoughton@...gle.com>
> > > ---
> > >  include/linux/hugetlb.h |  5 +++
> > >  mm/hugetlb.c            | 94 +++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 99 insertions(+)
> > >
> > > diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> > > index d30322108b34..003255b0e40f 100644
> > > --- a/include/linux/hugetlb.h
> > > +++ b/include/linux/hugetlb.h
> > > @@ -119,6 +119,11 @@ void hugetlb_pte_copy(struct hugetlb_pte *dest, const struct hugetlb_pte *src)
> > >
> > >  bool hugetlb_pte_present_leaf(const struct hugetlb_pte *hpte, pte_t pte);
> > >
> > > +pmd_t *hugetlb_pmd_alloc(struct mm_struct *mm, struct hugetlb_pte *hpte,
> > > +             unsigned long addr);
> > > +pte_t *hugetlb_pte_alloc(struct mm_struct *mm, struct hugetlb_pte *hpte,
> > > +             unsigned long addr);
> > > +
> > >  struct hugepage_subpool {
> > >       spinlock_t lock;
> > >       long count;
> > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > > index a0e46d35dabc..e3733388adee 100644
> > > --- a/mm/hugetlb.c
> > > +++ b/mm/hugetlb.c
> > > @@ -341,6 +341,100 @@ static bool has_same_uncharge_info(struct file_region *rg,
> > >  #endif
> > >  }
> > >
> > > +pmd_t *hugetlb_pmd_alloc(struct mm_struct *mm, struct hugetlb_pte *hpte,
> > > +             unsigned long addr)
> >
> > A little confused as there are no users yet ... Is hpte the 'hstate PTE'
> > that we are trying to allocate ptes under?  For example, in the case of
> > a hugetlb_pmd_alloc caller hpte would be a PUD or CONT_PMD size pte?
>
> The hpte is the level above the level we're trying to allocate (not
> necessarily the 'hstate PTE'). I'll make that clear in the comments
> for both functions.
>
> So consider allocating 4K PTEs for a 1G HugeTLB page:
> - With the hstate 'PTE' (PUD), we make a hugetlb_pte with that PUD
> (let's call it 'hpte')
> - We call hugetlb_pmd_alloc(hpte) which will leave 'hpte' the same,
> but the pud_t that hpte->ptep points to is no longer a leaf.
> - We call hugetlb_walk_step(hpte) to step down a level to get a PMD,
> changing hpte. The hpte->ptep is now pointing to a blank pmd_t.
> - We call hugetlb_pte_alloc(hpte) to allocate a bunch of PTEs and
> populate the pmd_t.
> - We call hugetlb_walk_step(hpte) to step down again.

Erm actually this isn't entirely accurate. The general flow is about
right, but hugetlb_pmd_alloc/hugetlb_pte_alloc are actually part of
hugetlb_walk_step. (See hugetlb_hgm_walk for the ground truth :P)

- James

>
> This is basically what hugetlb_hgm_walk does (in the next patch). We
> only change 'hpte' when we do a step, and that is when we populate
> 'shift'. The 'sz' parameter for hugetlb_walk_step is what
> architectures can use to populate hpte->shift appropriately (ignored
> for x86).
>
> For arm64, we can use 'sz' to populate hpte->shift with what the
> caller wants when we are free to choose (like if all the PTEs are
> none, we can do CONT_PTE_SHIFT). See [1]'s implementation of
> hugetlb_walk_step for what I *think* is correct for arm64.
>
> [1] https://github.com/48ca/linux/commit/bf3b8742e95c58c2431c80c5bed5cb5cb95885af
>
> >
> > > +{
> > > +     spinlock_t *ptl = hugetlb_pte_lockptr(mm, hpte);
> > > +     pmd_t *new;
> > > +     pud_t *pudp;
> > > +     pud_t pud;
> > > +
> > > +     if (hpte->level != HUGETLB_LEVEL_PUD)
> > > +             return ERR_PTR(-EINVAL);
> >
> > Ah yes, it is PUD level.  However, I guess CONT_PMD would also be valid
> > on arm64?
>
> The level is always PGD, P4D, PUD, PMD, or PTE. CONT_PTE is on
> HUGETLB_LEVEL_PTE, CONT_PMD is on HUGETLB_LEVEL_PMD.
>
> These functions are supposed to be used for all architectures (in
> their implementations of 'hugetlb_walk_step'; that's why they're not
> static, actually. I'll make that clear in the commit description).
>
> >
> > > +
> > > +     pudp = (pud_t *)hpte->ptep;
> > > +retry:
> > > +     pud = *pudp;
> >
> > We might want to consider a READ_ONCE here.  I am not an expert on such
> > things, but recall a similar as pointed out in the now obsolete commit
> > 27ceae9833843.
>
> Agreed. Will try to change all PTE reading to use READ_ONCE, though
> they can be easy to miss... :(
>
> Thanks very much for the reviews so far, Mike!
>
> - James
>
> >
> > --
> > Mike Kravetz
> >

Powered by blists - more mailing lists