| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CADrL8HVkrevuvUXLDJ9hvk4e+wnyQu3+z49rZjxg+=OB=KB_+Q@mail.gmail.com>
Date: Tue, 21 Feb 2023 08:36:08 -0800
From: James Houghton <jthoughton@...gle.com>
To: Mina Almasry <almasrymina@...gle.com>
Cc: Mike Kravetz <mike.kravetz@...cle.com>,
Muchun Song <songmuchun@...edance.com>,
Peter Xu <peterx@...hat.com>,
Andrew Morton <akpm@...ux-foundation.org>,
David Hildenbrand <david@...hat.com>,
David Rientjes <rientjes@...gle.com>,
Axel Rasmussen <axelrasmussen@...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>,
Frank van der Linden <fvdl@...gle.com>,
Jiaqi Yan <jiaqiyan@...gle.com>, linux-mm@...ck.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 11/46] hugetlb: add hugetlb_pte to track HugeTLB page
table entries
On Fri, Feb 17, 2023 at 9:24 PM Mina Almasry <almasrymina@...gle.com> wrote:
>
> On Fri, Feb 17, 2023 at 4:28 PM James Houghton <jthoughton@...gle.com> wrote:
> >
> > After high-granularity mapping, page table entries for HugeTLB pages can
> > be of any size/type. (For example, we can have a 1G page mapped with a
> > mix of PMDs and PTEs.) This struct is to help keep track of a HugeTLB
> > PTE after we have done a page table walk.
> >
> > Without this, we'd have to pass around the "size" of the PTE everywhere.
> > We effectively did this before; it could be fetched from the hstate,
> > which we pass around pretty much everywhere.
> >
> > hugetlb_pte_present_leaf is included here as a helper function that will
> > be used frequently later on.
> >
> > Signed-off-by: James Houghton <jthoughton@...gle.com>
> >
>
> Only nits.
>
> Reviewed-by: Mina Almasry <almasrymina@...gle.com>
Thanks Mina!
>
> > diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> > index a1ceb9417f01..eeacadf3272b 100644
> > --- a/include/linux/hugetlb.h
> > +++ b/include/linux/hugetlb.h
> > @@ -26,6 +26,25 @@ typedef struct { unsigned long pd; } hugepd_t;
> > #define __hugepd(x) ((hugepd_t) { (x) })
> > #endif
> >
> > +enum hugetlb_level {
> > + HUGETLB_LEVEL_PTE = 1,
> > + /*
> > + * We always include PMD, PUD, and P4D in this enum definition so that,
> > + * when logged as an integer, we can easily tell which level it is.
> > + */
> > + HUGETLB_LEVEL_PMD,
> > + HUGETLB_LEVEL_PUD,
> > + HUGETLB_LEVEL_P4D,
> > + HUGETLB_LEVEL_PGD,
> > +};
> > +
> > +struct hugetlb_pte {
> > + pte_t *ptep;
> > + unsigned int shift;
> > + enum hugetlb_level level;
> > + spinlock_t *ptl;
> > +};
> > +
> > #ifdef CONFIG_HUGETLB_PAGE
> >
> > #include <linux/mempolicy.h>
> > @@ -39,6 +58,20 @@ typedef struct { unsigned long pd; } hugepd_t;
> > */
> > #define __NR_USED_SUBPAGE 3
> >
> > +static inline
> > +unsigned long hugetlb_pte_size(const struct hugetlb_pte *hpte)
> > +{
> > + return 1UL << hpte->shift;
> > +}
> > +
> > +static inline
> > +unsigned long hugetlb_pte_mask(const struct hugetlb_pte *hpte)
> > +{
> > + return ~(hugetlb_pte_size(hpte) - 1);
> > +}
> > +
> > +bool hugetlb_pte_present_leaf(const struct hugetlb_pte *hpte, pte_t pte);
> > +
> > struct hugepage_subpool {
> > spinlock_t lock;
> > long count;
> > @@ -1234,6 +1267,45 @@ static inline spinlock_t *huge_pte_lock(struct hstate *h,
> > return ptl;
> > }
> >
> > +static inline
> > +spinlock_t *hugetlb_pte_lockptr(struct hugetlb_pte *hpte)
> > +{
> > + return hpte->ptl;
> > +}
>
> I find this helper unnecessary. I would remove it.
Ok. Will do.
>
> > +
> > +static inline
> > +spinlock_t *hugetlb_pte_lock(struct hugetlb_pte *hpte)
> > +{
> > + spinlock_t *ptl = hugetlb_pte_lockptr(hpte);
> > +
> > + spin_lock(ptl);
>
> Here 'spin_lock(hpte->ptl)' would be more immediately understandable
> IMO, for example.
>
> > + return ptl;
> > +}
> > +
> > +static inline
> > +void __hugetlb_pte_init(struct hugetlb_pte *hpte, pte_t *ptep,
> > + unsigned int shift, enum hugetlb_level level,
> > + spinlock_t *ptl)
> > +{
> > + /*
> > + * If 'shift' indicates that this PTE is contiguous, then @ptep must
> > + * be the first pte of the contiguous bunch.
> > + */
>
> I would move the comment to above the function as a pseudo doc. It
> seems to instruct the user of the function of how to use it.
Right. Will do.
>
> > + hpte->ptl = ptl;
> > + hpte->ptep = ptep;
> > + hpte->shift = shift;
> > + hpte->level = level;
> > +}
> > +
> > +static inline
> > +void hugetlb_pte_init(struct mm_struct *mm, struct hugetlb_pte *hpte,
> > + pte_t *ptep, unsigned int shift,
> > + enum hugetlb_level level)
> > +{
> > + __hugetlb_pte_init(hpte, ptep, shift, level,
> > + huge_pte_lockptr(shift, mm, ptep));
> > +}
> > +
> > #if defined(CONFIG_HUGETLB_PAGE) && defined(CONFIG_CMA)
> > extern void __init hugetlb_cma_reserve(int order);
> > #else
> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > index 5ca9eae0ac42..6c74adff43b6 100644
> > --- a/mm/hugetlb.c
> > +++ b/mm/hugetlb.c
> > @@ -1269,6 +1269,35 @@ static bool vma_has_reserves(struct vm_area_struct *vma, long chg)
> > return false;
> > }
> >
> > +bool hugetlb_pte_present_leaf(const struct hugetlb_pte *hpte, pte_t pte)
> > +{
> > + pgd_t pgd;
> > + p4d_t p4d;
> > + pud_t pud;
> > + pmd_t pmd;
> > +
> > + switch (hpte->level) {
> > + case HUGETLB_LEVEL_PGD:
> > + pgd = __pgd(pte_val(pte));
> > + return pgd_present(pgd) && pgd_leaf(pgd);
> > + case HUGETLB_LEVEL_P4D:
> > + p4d = __p4d(pte_val(pte));
> > + return p4d_present(p4d) && p4d_leaf(p4d);
> > + case HUGETLB_LEVEL_PUD:
> > + pud = __pud(pte_val(pte));
> > + return pud_present(pud) && pud_leaf(pud);
> > + case HUGETLB_LEVEL_PMD:
> > + pmd = __pmd(pte_val(pte));
> > + return pmd_present(pmd) && pmd_leaf(pmd);
> > + case HUGETLB_LEVEL_PTE:
> > + return pte_present(pte);
> > + default:
> > + WARN_ON_ONCE(1);
> > + return false;
> > + }
> > +}
> > +
> > +
> > static void enqueue_hugetlb_folio(struct hstate *h, struct folio *folio)
> > {
> > int nid = folio_nid(folio);
> > --
> > 2.39.2.637.g21b0678d19-goog
> >
Powered by blists - more mailing lists