From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id D6598C433EF for ; Wed, 29 Jun 2022 16:42:29 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 1FA058E0017; Wed, 29 Jun 2022 12:42:29 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 1A9B68E0015; Wed, 29 Jun 2022 12:42:29 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 048A88E0017; Wed, 29 Jun 2022 12:42:28 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id E776A8E0015 for ; Wed, 29 Jun 2022 12:42:28 -0400 (EDT) Received: from smtpin03.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id A282860679 for ; Wed, 29 Jun 2022 16:42:28 +0000 (UTC) X-FDA: 79631841576.03.A0F3A15 Received: from mail-pf1-f178.google.com (mail-pf1-f178.google.com [209.85.210.178]) by imf03.hostedemail.com (Postfix) with ESMTP id 42F7F2001C for ; Wed, 29 Jun 2022 16:42:28 +0000 (UTC) Received: by mail-pf1-f178.google.com with SMTP id n12so15705448pfq.0 for ; Wed, 29 Jun 2022 09:42:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=jg5dL8VeaP4lhGUhYxLWAQpleaZnARr0D9dVS5+rf+s=; b=GKCxVKO3nuKTXidvS7dXfkcjgoTupxMi6XYUY4WZYbaevrhKhF1SJ8GASlgumxh++X bEstNBBXmPFP67+qQVX6pIjVY7pBL/XH7QCxFupbaf+4mck6HCV0GhoS0h10ev7id99n wuV0VoXIvFn3jKXg5B/S/OBynbSH+i0mVBmmGQAVEzjSPoWEOoI+0lXUvN7QVleegDpq vXMhlXgNJAeIa0JFYjJBADA1RVuyAJQG7H8bm9LEmFA6h2JhSi7xxzLkYlHejZfiaa6y tAiT6sQkuQOeTphAHlTBCF2sKRFcJSpCrFGPAbxTy4MEGIoRvISShfUwHdeqk0UFmFpS XLPA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=jg5dL8VeaP4lhGUhYxLWAQpleaZnARr0D9dVS5+rf+s=; b=nl5Im3QKWCvj3IMCaeU14POadIns4HLl+2ziTwy84pYp3AGj6qNhjezwFUDn9AW5jM Mgcm627Q968rhBQvPOFpEiQNRaXhbmgbZhY1g6a2t6W12x5hb9GX9GhPv9W3ak0bEKz5 EDAGOI0HVZ6yfNWMizqsfHhMIPkrBVFb1czjy8sLGJOaLFhRYf2Wgf3xfmzCpCSsT0+C NgFmx87yDiXNd1w1Ymu9KG00hn/EJ4HOMOd98keFkhYrFh+1zdXZaoT1wD0AmQR+rSd/ kaxG5bUVU9QKTQAgRJucFqtEftXt+TU0i1ozga7rQ27kWyIzRlyYLFeisW8S3JgDfYvB 8CZw== X-Gm-Message-State: AJIora/YAiNsHyTJMV1WvaruGpjhRHz4iqlPaOV2HQbAgFZtXpmXwSXI 5c0zWiQX/DxDPGOcXT5foajit23xtuUH54GJxL/ykw== X-Google-Smtp-Source: AGRyM1vqX+WkywPIv6RcJnB0cgz/e6Td+5b3FeWqwVZI9EO6LCmmalhWYin9vLHIKrz2UyFBc5Meo/mtdZwb7nbNNIE= X-Received: by 2002:a63:5449:0:b0:40d:c8d4:ed6a with SMTP id e9-20020a635449000000b0040dc8d4ed6amr3662961pgm.9.1656520947107; Wed, 29 Jun 2022 09:42:27 -0700 (PDT) MIME-Version: 1.0 References: <20220624173656.2033256-1-jthoughton@google.com> <20220624173656.2033256-8-jthoughton@google.com> In-Reply-To: From: James Houghton Date: Wed, 29 Jun 2022 09:42:16 -0700 Message-ID: Subject: Re: [RFC PATCH 07/26] hugetlb: add hugetlb_pte to track HugeTLB page table entries To: Mina Almasry Cc: Mike Kravetz , Muchun Song , Peter Xu , David Hildenbrand , David Rientjes , Axel Rasmussen , Jue Wang , Manish Mishra , "Dr . David Alan Gilbert" , linux-mm@kvack.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1656520948; a=rsa-sha256; cv=none; b=iWmMh14m5xmegy7vO/dLXDOF2DbZiQXkvvLj4vMXYeoJ2KvAsyQ1x0RjZ/7cQHWY9ziK6T eDy7kWWD+nkYfiy3tUQAe2mcS2BR8eGUOVHq0JdLOeGV8WMCVE2hTQGe4zo3TFVhThyp3S Bg9Ha+jknZct0SMo7mCdMU0b/u9jAiw= ARC-Authentication-Results: i=1; imf03.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=GKCxVKO3; spf=pass (imf03.hostedemail.com: domain of jthoughton@google.com designates 209.85.210.178 as permitted sender) smtp.mailfrom=jthoughton@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1656520948; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=jg5dL8VeaP4lhGUhYxLWAQpleaZnARr0D9dVS5+rf+s=; b=p+wtUrCTbW4AEPgTkA0/F3diRKZ9PYLekV0PMwBj2bP6e9/oGSuWEScGN+HT3rMRrSL+w2 9R5mxicybRzp61HlA6gvo9BulHJUQIRa6Dyx9YlV7PujN2hASyfZhnMQnvlrewb8II0RPT p6usEiqoQKgAYOycUd3YZkpPAcD4cKY= X-Rspamd-Server: rspam09 X-Rspamd-Queue-Id: 42F7F2001C Authentication-Results: imf03.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=GKCxVKO3; spf=pass (imf03.hostedemail.com: domain of jthoughton@google.com designates 209.85.210.178 as permitted sender) smtp.mailfrom=jthoughton@google.com; dmarc=pass (policy=reject) header.from=google.com X-Rspam-User: X-Stat-Signature: aymcu8mr5cq8aqb6m4wtf5uou8b7d5kr X-HE-Tag: 1656520948-104738 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Tue, Jun 28, 2022 at 1:25 PM Mina Almasry wrote: > > On Fri, Jun 24, 2022 at 10:37 AM James Houghton 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. > > > > This commit includes definitions for some basic helper functions that > > are used later. These helper functions wrap existing PTE > > inspection/modification functions, where the correct version is picked > > depending on if the HugeTLB PTE is actually "huge" or not. (Previously, > > all HugeTLB PTEs were "huge"). > > > > For example, hugetlb_ptep_get wraps huge_ptep_get and ptep_get, where > > ptep_get is used when the HugeTLB PTE is PAGE_SIZE, and huge_ptep_get is > > used in all other cases. > > > > Signed-off-by: James Houghton > > --- > > include/linux/hugetlb.h | 84 +++++++++++++++++++++++++++++++++++++++++ > > mm/hugetlb.c | 57 ++++++++++++++++++++++++++++ > > 2 files changed, 141 insertions(+) > > > > diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h > > index 5fe1db46d8c9..1d4ec9dfdebf 100644 > > --- a/include/linux/hugetlb.h > > +++ b/include/linux/hugetlb.h > > @@ -46,6 +46,68 @@ enum { > > __NR_USED_SUBPAGE, > > }; > > > > +struct hugetlb_pte { > > + pte_t *ptep; > > + unsigned int shift; > > +}; > > + > > +static inline > > +void hugetlb_pte_init(struct hugetlb_pte *hpte) > > +{ > > + hpte->ptep = NULL; > > shift = 0; ? I don't think this is necessary (but, admittedly, it is quite harmless to add). ptep = NULL means that the hugetlb_pte isn't valid, and shift could be anything. Originally I had a separate `bool valid`, but ptep=NULL was exactly the same as valid=false. > > > +} > > + > > +static inline > > +void hugetlb_pte_populate(struct hugetlb_pte *hpte, pte_t *ptep, > > + unsigned int shift) > > +{ > > + BUG_ON(!ptep); > > + hpte->ptep = ptep; > > + hpte->shift = shift; > > +} > > + > > +static inline > > +unsigned long hugetlb_pte_size(const struct hugetlb_pte *hpte) > > +{ > > + BUG_ON(!hpte->ptep); > > + return 1UL << hpte->shift; > > +} > > + > > This helper is quite redundant in my opinion. Putting 1UL << hugetlb_pte_shit(hpte) everywhere is kind of annoying. :) > > > +static inline > > +unsigned long hugetlb_pte_mask(const struct hugetlb_pte *hpte) > > +{ > > + BUG_ON(!hpte->ptep); > > + return ~(hugetlb_pte_size(hpte) - 1); > > +} > > + > > +static inline > > +unsigned int hugetlb_pte_shift(const struct hugetlb_pte *hpte) > > +{ > > + BUG_ON(!hpte->ptep); > > + return hpte->shift; > > +} > > + > > This one jumps as quite redundant too. To make sure we aren't using an invalid hugetlb_pte, I want to remove all places where I directly access hpte->shift -- really they should all go through hugetlb_pte_shift. > > > +static inline > > +bool hugetlb_pte_huge(const struct hugetlb_pte *hpte) > > +{ > > + return !IS_ENABLED(CONFIG_HUGETLB_HIGH_GRANULARITY_MAPPING) || > > + hugetlb_pte_shift(hpte) > PAGE_SHIFT; > > +} > > + > > I'm guessing the !IS_ENABLED() check is because only the HGM code > would store a non-huge pte in a hugetlb_pte struct. I think it's a bit > fragile because anyone can add code in the future that uses > hugetlb_pte in unexpected ways, but I will concede that it is correct > as written. I added this so that, if HGM isn't enabled, the compiler would have an easier time optimizing things. I don't really have strong feelings about keeping/removing it. > > > +static inline > > +void hugetlb_pte_copy(struct hugetlb_pte *dest, const struct hugetlb_pte *src) > > +{ > > + dest->ptep = src->ptep; > > + dest->shift = src->shift; > > +} > > + > > +bool hugetlb_pte_present_leaf(const struct hugetlb_pte *hpte); > > +bool hugetlb_pte_none(const struct hugetlb_pte *hpte); > > +bool hugetlb_pte_none_mostly(const struct hugetlb_pte *hpte); > > +pte_t hugetlb_ptep_get(const struct hugetlb_pte *hpte); > > +void hugetlb_pte_clear(struct mm_struct *mm, const struct hugetlb_pte *hpte, > > + unsigned long address); > > + > > struct hugepage_subpool { > > spinlock_t lock; > > long count; > > @@ -1130,6 +1192,28 @@ static inline spinlock_t *huge_pte_lock_shift(unsigned int shift, > > return ptl; > > } > > > > +static inline > > Maybe for organization, move all the static functions you're adding > above the hugetlb_pte_* declarations you're adding? Will do. > > > +spinlock_t *hugetlb_pte_lockptr(struct mm_struct *mm, struct hugetlb_pte *hpte) > > +{ > > + > > + BUG_ON(!hpte->ptep); > > + // Only use huge_pte_lockptr if we are at leaf-level. Otherwise use > > + // the regular page table lock. > > Does checkpatch.pl not complain about // style comments? I think those > are not allowed, no? It didn't :( I thought I went through and removed them all -- I guess I missed some. > > > + if (hugetlb_pte_none(hpte) || hugetlb_pte_present_leaf(hpte)) > > + return huge_pte_lockptr(hugetlb_pte_shift(hpte), > > + mm, hpte->ptep); > > + return &mm->page_table_lock; > > +} > > + > > +static inline > > +spinlock_t *hugetlb_pte_lock(struct mm_struct *mm, struct hugetlb_pte *hpte) > > +{ > > + spinlock_t *ptl = hugetlb_pte_lockptr(mm, hpte); > > + > > + spin_lock(ptl); > > + return ptl; > > +} > > + > > #if defined(CONFIG_HUGETLB_PAGE) && defined(CONFIG_CMA) > > extern void __init hugetlb_cma_reserve(int order); > > extern void __init hugetlb_cma_check(void); > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > > index d6d0d4c03def..1a1434e29740 100644 > > --- a/mm/hugetlb.c > > +++ b/mm/hugetlb.c > > @@ -1120,6 +1120,63 @@ static bool vma_has_reserves(struct vm_area_struct *vma, long chg) > > return false; > > } > > > > +bool hugetlb_pte_present_leaf(const struct hugetlb_pte *hpte) > > +{ > > + pgd_t pgd; > > + p4d_t p4d; > > + pud_t pud; > > + pmd_t pmd; > > + > > + BUG_ON(!hpte->ptep); > > + if (hugetlb_pte_size(hpte) >= PGDIR_SIZE) { > > + pgd = *(pgd_t *)hpte->ptep; > > + return pgd_present(pgd) && pgd_leaf(pgd); > > + } else if (hugetlb_pte_size(hpte) >= P4D_SIZE) { > > + p4d = *(p4d_t *)hpte->ptep; > > + return p4d_present(p4d) && p4d_leaf(p4d); > > + } else if (hugetlb_pte_size(hpte) >= PUD_SIZE) { > > + pud = *(pud_t *)hpte->ptep; > > + return pud_present(pud) && pud_leaf(pud); > > + } else if (hugetlb_pte_size(hpte) >= PMD_SIZE) { > > + pmd = *(pmd_t *)hpte->ptep; > > + return pmd_present(pmd) && pmd_leaf(pmd); > > + } else if (hugetlb_pte_size(hpte) >= PAGE_SIZE) > > + return pte_present(*hpte->ptep); > > The use of >= is a bit curious to me. Shouldn't these be ==? These (except PGDIR_SIZE) should be >=. This is because some architectures support multiple huge PTE sizes at the same page table level. For example, on arm64, you can have 2M PMDs, and you can also have 32M PMDs[1]. [1]: https://www.kernel.org/doc/html/latest/arm64/hugetlbpage.html > > Also probably doesn't matter but I was thinking to use *_SHIFTs > instead of *_SIZE so you don't have to calculate the size 5 times in > this routine, or calculate hugetlb_pte_size() once for some less code > duplication and re-use? I'll change this to use the shift, and I'll move the computation so it's only done once (it is probably helpful for readability too). (I imagine the compiler only actually computes the size once here.) > > > + BUG(); > > +} > > + > > +bool hugetlb_pte_none(const struct hugetlb_pte *hpte) > > +{ > > + if (hugetlb_pte_huge(hpte)) > > + return huge_pte_none(huge_ptep_get(hpte->ptep)); > > + return pte_none(ptep_get(hpte->ptep)); > > +} > > + > > +bool hugetlb_pte_none_mostly(const struct hugetlb_pte *hpte) > > +{ > > + if (hugetlb_pte_huge(hpte)) > > + return huge_pte_none_mostly(huge_ptep_get(hpte->ptep)); > > + return pte_none_mostly(ptep_get(hpte->ptep)); > > +} > > + > > +pte_t hugetlb_ptep_get(const struct hugetlb_pte *hpte) > > +{ > > + if (hugetlb_pte_huge(hpte)) > > + return huge_ptep_get(hpte->ptep); > > + return ptep_get(hpte->ptep); > > +} > > + > > +void hugetlb_pte_clear(struct mm_struct *mm, const struct hugetlb_pte *hpte, > > + unsigned long address) > > +{ > > + BUG_ON(!hpte->ptep); > > + unsigned long sz = hugetlb_pte_size(hpte); > > + > > + if (sz > PAGE_SIZE) > > + return huge_pte_clear(mm, address, hpte->ptep, sz); > > + return pte_clear(mm, address, hpte->ptep); > > +} > > + > > static void enqueue_huge_page(struct hstate *h, struct page *page) > > { > > int nid = page_to_nid(page); > > -- > > 2.37.0.rc0.161.g10f37bed90-goog > >