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 22EC4C433EF for ; Tue, 28 Jun 2022 20:25:39 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 594838E0002; Tue, 28 Jun 2022 16:25:39 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 5443E8E0001; Tue, 28 Jun 2022 16:25:39 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 40C808E0002; Tue, 28 Jun 2022 16:25:39 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 30EEF8E0001 for ; Tue, 28 Jun 2022 16:25:39 -0400 (EDT) Received: from smtpin24.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 019BE2270D for ; Tue, 28 Jun 2022 20:25:38 +0000 (UTC) X-FDA: 79628775198.24.5B6A630 Received: from mail-vs1-f49.google.com (mail-vs1-f49.google.com [209.85.217.49]) by imf08.hostedemail.com (Postfix) with ESMTP id 9B2FE16003B for ; Tue, 28 Jun 2022 20:25:38 +0000 (UTC) Received: by mail-vs1-f49.google.com with SMTP id q28so912512vsp.7 for ; Tue, 28 Jun 2022 13:25:38 -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=Enhe/nonN+cjOXq0VkfmEAmv3jt9mvt8AKrDL/yyTa4=; b=Rx5Sf1FiofQmqFlsHUKetNpDiqLe6X3tIU4aiSmJvBRAYqoE9sL+qBVERKwWnwMfbx I3T5xfw85xlU+iQZvHg7QGgJTVPand15b7S3gi8JQu+bZLutZokdnkvKhRd1XxhASkYL 4k4P4r012MlC/z4Mp/mwZFm4pIqsvPQF/oNQHsI5xhcJhw6KKsOLEwgzXhXrsMW7Ip5e 15oSy2weWA5aVhX0PBO0xv1S9Rw+NF7vPODlhu2oWBrTK457zvxzlehOW8tr50P0g5Rl zE3cZ8Y/4hwS7mUAyetOFxPq0PwO1r5FETxIAUhQY5LtVkYx0xQMaRv/z5ov1cwqbv17 ep2g== 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=Enhe/nonN+cjOXq0VkfmEAmv3jt9mvt8AKrDL/yyTa4=; b=2aK0kmUxxOLuH1q35CzqiztKgj3Q4vEA9OZsxYRe+nhIwJVax0HLXsQygT2MNGgtFU NqAyyKzbD9agXBAn8S+z61bVoDlrSWq/1Ht3ZtSPySijWcXQvjFtc6m9pwrLmzSUXMuW wtwoXcT0whg5ICupCZDtFKK+w9pUGkqMMT/T4LIUDedoNQ9HlEiogoAWhIiTRPDOIAMk rniRUNjEhAfUbgpjKuweeUotBIx3slIkuZbREVZskcFzNhMsa06FqRnfPxQqppMuqeyo HEuAP4qqGYCsCpuBsRJ5CWrsY9+RfTkl/NkWygFGniloAg5k7onLTrvu/ed+vjs69VE4 j7QA== X-Gm-Message-State: AJIora9AyBI0l7C6WQ+MT/dyl6l2rqVKUVG09mIHPUlfp4f7ftwsf1cL 9in+LDNg+LUxHF5nVYlIM3ER+x+F8QCWXo74T8hLFw== X-Google-Smtp-Source: AGRyM1t0zvxZV+6K5EnJlEG+JRDrvBQyifa/s+ZMlTRWe7hYhBOlX03K5Qu9qT+JlLR85kbBFRgfWQiHtCZKYj80XRI= X-Received: by 2002:a05:6102:f81:b0:354:3967:118a with SMTP id e1-20020a0561020f8100b003543967118amr2538942vsv.67.1656447937672; Tue, 28 Jun 2022 13:25:37 -0700 (PDT) MIME-Version: 1.0 References: <20220624173656.2033256-1-jthoughton@google.com> <20220624173656.2033256-8-jthoughton@google.com> In-Reply-To: <20220624173656.2033256-8-jthoughton@google.com> From: Mina Almasry Date: Tue, 28 Jun 2022 13:25:26 -0700 Message-ID: Subject: Re: [RFC PATCH 07/26] hugetlb: add hugetlb_pte to track HugeTLB page table entries To: James Houghton 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=1656447938; a=rsa-sha256; cv=none; b=Iq3cP80uUlAe+7M1HZZ/a+0+0Ec0o66foANnatBapwUFW9AD+Mnb4FKBqqvrCEUN20OU6x sdVTk+gY4QwnPQRfPqE8+BVJBmg5nkKxBWzlOcqEVmzUtFGqCynoijmD+rwsvmUycB9bY9 ZO4Zdz3D+pgHWS1AN2ycF0G0Le4cpAg= ARC-Authentication-Results: i=1; imf08.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=Rx5Sf1Fi; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf08.hostedemail.com: domain of almasrymina@google.com designates 209.85.217.49 as permitted sender) smtp.mailfrom=almasrymina@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1656447938; 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=Enhe/nonN+cjOXq0VkfmEAmv3jt9mvt8AKrDL/yyTa4=; b=RammLeJ73xiWzBihB5g/d6tQcDzHs8+XjBR8szssJLO26JD2pUWsiriDHY7e4HexvzEvK6 r+fLX7eN9oAjRohG1ROvJznxN3U2Ibt6+qHS5/kxuXjrqeaCG2xD5/YFs7FrAlotwq0nXn 4W1VMYAUcnugW4/nzrX58kEu9Ove2Dw= X-Stat-Signature: 6un1i5qjc1d9wdp8cgx1ppjzshx5to1i X-Rspamd-Queue-Id: 9B2FE16003B Authentication-Results: imf08.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=Rx5Sf1Fi; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf08.hostedemail.com: domain of almasrymina@google.com designates 209.85.217.49 as permitted sender) smtp.mailfrom=almasrymina@google.com X-Rspamd-Server: rspam10 X-Rspam-User: X-HE-Tag: 1656447938-667799 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 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; ? > +} > + > +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. > +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. > +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. > +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? > +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? > + 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 ==? 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? > + 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 >