From: Peter Xu <peterx@redhat.com>
[based on akpm/mm-unstable latest commit 9af2e4c429b5]
v1:
- Rebase, remove RFC tag
- Fixed powerpc patch build issue, enhancing commit message [Michael]
- Optimize patch 1 & 3 on "none || !present" check [Jason]
In previous work [1], we removed the pXd_large() API, which is arch
specific. This patchset further removes the hugetlb pXd_huge() API.
Hugetlb was never special on creating huge mappings when compared with
other huge mappings. Having a standalone API just to detect such pgtable
entries is more or less redundant, especially after the pXd_leaf() API set
is introduced with/without CONFIG_HUGETLB_PAGE.
When looking at this problem, a few issues are also exposed that we don't
have a clear definition of the *_huge() variance API. This patchset
started by cleaning these issues first, then replace all *_huge() users to
use *_leaf(), then drop all *_huge() code.
On x86/sparc, swap entries will be reported "true" in pXd_huge(), while for
all the rest archs they're reported "false" instead. This part is done in
patch 1-5, in which I suspect patch 1 can be seen as a bug fix, but I'll
leave that to hmm experts to decide.
Besides, there are three archs (arm, arm64, powerpc) that have slightly
different definitions between the *_huge() v.s. *_leaf() variances. I
tackled them separately so that it'll be easier for arch experts to chim in
when necessary. This part is done in patch 6-9.
The final patches 10-13 do the rest on the final removal, since *_leaf()
will be the ultimate API in the future, and we seem to have quite some
confusions on how *_huge() APIs can be defined, provide a rich comment for
*_leaf() API set to define them properly to avoid future misuse, and
hopefully that'll also help new archs to start support huge mappings and
avoid traps (like either swap entries, or PROT_NONE entry checks).
The whole series is only lightly tested on x86, while as usual I don't have
the capability to test all archs that it touches.
[1] https://lore.kernel.org/r/20240305043750.93762-1-peterx@redhat.com
Peter Xu (13):
mm/hmm: Process pud swap entry without pud_huge()
mm/gup: Cache p4d in follow_p4d_mask()
mm/gup: Check p4d presence before going on
mm/x86: Change pXd_huge() behavior to exclude swap entries
mm/sparc: Change pXd_huge() behavior to exclude swap entries
mm/arm: Use macros to define pmd/pud helpers
mm/arm: Redefine pmd_huge() with pmd_leaf()
mm/arm64: Merge pXd_huge() and pXd_leaf() definitions
mm/powerpc: Redefine pXd_huge() with pXd_leaf()
mm/gup: Merge pXd huge mapping checks
mm/treewide: Replace pXd_huge() with pXd_leaf()
mm/treewide: Remove pXd_huge()
mm: Document pXd_leaf() API
arch/arm/include/asm/pgtable-2level.h | 4 +--
arch/arm/include/asm/pgtable-3level-hwdef.h | 1 +
arch/arm/include/asm/pgtable-3level.h | 6 ++--
arch/arm/mm/Makefile | 1 -
arch/arm/mm/hugetlbpage.c | 34 -------------------
arch/arm64/include/asm/pgtable.h | 6 +++-
arch/arm64/mm/hugetlbpage.c | 18 ++--------
arch/loongarch/mm/hugetlbpage.c | 12 +------
arch/mips/include/asm/pgtable-32.h | 2 +-
arch/mips/include/asm/pgtable-64.h | 2 +-
arch/mips/mm/hugetlbpage.c | 10 ------
arch/mips/mm/tlb-r4k.c | 2 +-
arch/parisc/mm/hugetlbpage.c | 11 ------
.../include/asm/book3s/64/pgtable-4k.h | 20 -----------
.../include/asm/book3s/64/pgtable-64k.h | 25 --------------
arch/powerpc/include/asm/book3s/64/pgtable.h | 27 +++++++--------
arch/powerpc/include/asm/nohash/pgtable.h | 10 ------
arch/powerpc/mm/pgtable_64.c | 6 ++--
arch/riscv/mm/hugetlbpage.c | 10 ------
arch/s390/mm/hugetlbpage.c | 10 ------
arch/sh/mm/hugetlbpage.c | 10 ------
arch/sparc/mm/hugetlbpage.c | 12 -------
arch/x86/mm/hugetlbpage.c | 26 --------------
arch/x86/mm/pgtable.c | 4 +--
include/linux/hugetlb.h | 24 -------------
include/linux/pgtable.h | 24 ++++++++++---
mm/gup.c | 24 ++++++-------
mm/hmm.c | 9 ++---
mm/memory.c | 2 +-
29 files changed, 68 insertions(+), 284 deletions(-)
delete mode 100644 arch/arm/mm/hugetlbpage.c
--
2.44.0
From: Peter Xu <peterx@redhat.com>
Swap pud entries do not always return true for pud_huge() for all archs.
x86 and sparc (so far) allow it, but all the rest do not accept a swap
entry to be reported as pud_huge(). So it's not safe to check swap entries
within pud_huge(). Check swap entries before pud_huge(), so it should be
always safe.
This is the only place in the kernel that (IMHO, wrongly) relies on
pud_huge() to return true on pud swap entries. The plan is to cleanup
pXd_huge() to only report non-swap mappings for all archs.
Cc: Alistair Popple <apopple@nvidia.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
mm/hmm.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
@@ -424,7 +424,7 @@ static int hmm_vma_walk_pud(pud_t *pudp, unsigned long start, unsigned long end,walk->action=ACTION_CONTINUE;pud=READ_ONCE(*pudp);-if(pud_none(pud)){+if(!pud_present(pud)){spin_unlock(ptl);returnhmm_vma_walk_hole(start,end,-1,walk);}
@@ -435,11 +435,6 @@ static int hmm_vma_walk_pud(pud_t *pudp, unsigned long start, unsigned long end,unsignedlong*hmm_pfns;unsignedlongcpu_flags;-if(!pud_present(pud)){-spin_unlock(ptl);-returnhmm_vma_walk_hole(start,end,-1,walk);-}-i=(addr-range->start)>>PAGE_SHIFT;npages=(end-addr)>>PAGE_SHIFT;hmm_pfns=&range->hmm_pfns[i];
From: Peter Xu <peterx@redhat.com>
Add a variable to cache p4d in follow_p4d_mask(). It's a good practise to
make sure all the following checks will have a consistent view of the entry.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
mm/gup.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
From: Peter Xu <peterx@redhat.com>
Currently there should have no p4d swap entries so it may not matter much,
however this may help us to rule out swap entries in pXd_huge() API, which
will include p4d_huge(). The p4d_present() checks make it 100% clear that
we won't rely on p4d_huge() for swap entries.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
mm/gup.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Peter Xu <peterx@redhat.com>
This patch partly reverts below commits:
3a194f3f8ad0 ("mm/hugetlb: make pud_huge() and follow_huge_pud() aware of non-present pud entry")
cbef8478bee5 ("mm/hugetlb: pmd_huge() returns true for non-present hugepage")
Right now, pXd_huge() definition across kernel is unclear. We have two
groups that think differently on swap entries:
- x86/sparc: Allow pXd_huge() to accept swap entries
- all the rest: Doesn't allow pXd_huge() to accept swap entries
This is so confusing. Since the sparc helpers seem to be added in 2016,
which is after x86's (2015), so sparc could have followed a trend. x86
proposed such swap handling in 2015 to resolve hugetlb swap entries hit in
GUP, but now GUP guards swap entries with !pXd_present() in all layers so
we should be safe.
We should define this API properly, one way or another, rather than keep
them defined differently across archs.
Gut feeling tells me that pXd_huge() shouldn't include swap entries, and it
turns out that I am not the only one thinking so, the question was raised
when the current pmd_huge() for x86 was proposed by Ville Syrjälä:
https://lore.kernel.org/all/Y2WQ7I4LXh8iUIRd@intel.com/
I might also be missing something obvious, but why is it even necessary
to treat PRESENT==0+PSE==0 as a huge entry?
It is also questioned when Jason Gunthorpe reviewed the other patchset on
swap entry handlings:
https://lore.kernel.org/all/20240221125753.GQ13330@nvidia.com/
Revert its meaning back to original. It shouldn't have any functional
change as we should be ready with guards on !pXd_present() explicitly
everywhere.
Note that I also dropped the "#if CONFIG_PGTABLE_LEVELS > 2", it was there
probably because it was breaking things when 3a194f3f8ad0 was proposed,
according to the report here:
https://lore.kernel.org/all/Y2LYXItKQyaJTv8j@intel.com/
Now we shouldn't need that.
Instead of reverting to _PAGE_PSE raw check, leverage pXd_leaf().
Cc: Naoya Horiguchi <redacted>
Cc: Thomas Gleixner <redacted>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: x86@kernel.org
Signed-off-by: Peter Xu <peterx@redhat.com>
---
arch/x86/mm/hugetlbpage.c | 18 ++++--------------
1 file changed, 4 insertions(+), 14 deletions(-)
From: Peter Xu <peterx@redhat.com>
Please refer to the previous patch on the reasoning for x86. Now sparc is
the only architecture that will allow swap entries to be reported as
pXd_huge(). After this patch, all architectures should forbid swap entries
in pXd_huge().
Cc: David S. Miller <davem@davemloft.net>
Cc: Andreas Larsson <andreas@gaisler.com>
Cc: sparclinux@vger.kernel.org
Signed-off-by: Peter Xu <peterx@redhat.com>
---
arch/sparc/mm/hugetlbpage.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
From: Peter Xu <peterx@redhat.com>
Unlike most archs, aarch64 defines pXd_huge() and pXd_leaf() slightly
differently. Redefine the pXd_huge() with pXd_leaf().
There used to be two traps for old aarch64 definitions over these APIs that
I found when reading the code around, they're:
(1) 4797ec2dc83a ("arm64: fix pud_huge() for 2-level pagetables")
(2) 23bc8f69f0ec ("arm64: mm: fix p?d_leaf()")
Define pXd_huge() with the current pXd_leaf() will make sure (2) isn't a
problem (on PROT_NONE checks). To make sure it also works for (1), we move
over the __PAGETABLE_PMD_FOLDED check to pud_leaf(), allowing it to
constantly returning "false" for 2-level pgtables, which looks even safer
to cover both now.
Cc: Muchun Song <muchun.song@linux.dev>
Cc: Mark Salter <redacted>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org
Signed-off-by: Peter Xu <peterx@redhat.com>
---
arch/arm64/include/asm/pgtable.h | 4 ++++
arch/arm64/mm/hugetlbpage.c | 8 ++------
2 files changed, 6 insertions(+), 6 deletions(-)
From: Peter Xu <peterx@redhat.com>
PowerPC book3s 4K mostly has the same definition on both, except pXd_huge()
constantly returns 0 for hash MMUs. As Michael Ellerman pointed out [1],
it is safe to check _PAGE_PTE on hash MMUs, as the bit will never be set so
it will keep returning false.
As a reference, __p[mu]d_mkhuge() will trigger a BUG_ON trying to create
such huge mappings for 4K hash MMUs. Meanwhile, the major powerpc hugetlb
pgtable walker __find_linux_pte() already used pXd_leaf() to check hugetlb
mappings.
The goal should be that we will have one API pXd_leaf() to detect all kinds
of huge mappings. AFAICT we need to use the pXd_leaf() impl (rather than
pXd_huge() ones) to make sure ie. THPs on hash MMU will also return true.
This helps to simplify a follow up patch to drop pXd_huge() treewide.
NOTE: *_leaf() definition need to be moved before the inclusion of
asm/book3s/64/pgtable-4k.h, which defines pXd_huge() with it.
[1] https://lore.kernel.org/r/87v85zo6w7.fsf@mail.lhotse
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Nicholas Piggin <npiggin@gmail.com>
Cc: Christophe Leroy <redacted>
Cc: "Aneesh Kumar K.V" <aneesh.kumar@kernel.org>
Cc: "Naveen N. Rao" <redacted>
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Peter Xu <peterx@redhat.com>
---
.../include/asm/book3s/64/pgtable-4k.h | 14 ++--------
arch/powerpc/include/asm/book3s/64/pgtable.h | 27 +++++++++----------
2 files changed, 14 insertions(+), 27 deletions(-)
From: Peter Xu <peterx@redhat.com>
Huge mapping checks in GUP are slightly redundant and can be simplified.
pXd_huge() now is the same as pXd_leaf(). pmd_trans_huge() and
pXd_devmap() should both imply pXd_leaf(). Time to merge them into one.
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
mm/gup.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
From: Peter Xu <peterx@redhat.com>
Most of the archs already define these two APIs the same way. ARM is more
complicated in two aspects:
- For pXd_huge() it's always checking against !PXD_TABLE_BIT, while for
pXd_leaf() it's always checking against PXD_TYPE_SECT.
- SECT/TABLE bits are defined differently on 2-level v.s. 3-level ARM
pgtables, which makes the whole thing even harder to follow.
Luckily, the second complexity should be hidden by the pmd_leaf()
implementation against 2-level v.s. 3-level headers. Invoke pmd_leaf()
directly for pmd_huge(), to remove the first part of complexity. This
prepares to drop pXd_huge() API globally.
When at it, drop the obsolete comments - it's outdated.
Cc: Russell King <linux@armlinux.org.uk>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Krzysztof Kozlowski <redacted>
Cc: Bjorn Andersson <andersson@kernel.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Konrad Dybcio <redacted>
Cc: Fabio Estevam <redacted>
Cc: linux-arm-kernel@lists.infradead.org
Signed-off-by: Peter Xu <peterx@redhat.com>
---
arch/arm/mm/hugetlbpage.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
@@ -64,7 +64,7 @@ uint64_t pmd_to_entrylo(unsigned long pmd_val){uint64_tval;/* PMD as PTE. Must be huge page */-if(!pmd_huge(__pmd(pmd_val)))+if(!pmd_leaf(__pmd(pmd_val)))panic("%s",__func__);val=pmd_val^_PAGE_HUGE;
@@ -326,7 +326,7 @@ void __update_tlb(struct vm_area_struct * vma, unsigned long address, pte_t pte)idx=read_c0_index();#ifdef CONFIG_MIPS_HUGE_TLB_SUPPORT/* this could be a huge page */-if(pmd_huge(*pmdp)){+if(pmd_leaf(*pmdp)){unsignedlonglo;write_c0_pagemask(PM_HUGE_MASK);ptep=(pte_t*)pmdp;
@@ -731,7 +731,7 @@ int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot)return0;/* Bail out if we are we on a populated non-leaf entry: */-if(pud_present(*pud)&&!pud_huge(*pud))+if(pud_present(*pud)&&!pud_leaf(*pud))return0;set_pte((pte_t*)pud,pfn_pte(
@@ -760,7 +760,7 @@ int pmd_set_huge(pmd_t *pmd, phys_addr_t addr, pgprot_t prot)}/* Bail out if we are we on a populated non-leaf entry: */-if(pmd_present(*pmd)&&!pmd_huge(*pmd))+if(pmd_present(*pmd)&&!pmd_leaf(*pmd))return0;set_pte((pte_t*)pmd,pfn_pte(
@@ -429,7 +429,7 @@ static int hmm_vma_walk_pud(pud_t *pudp, unsigned long start, unsigned long end,returnhmm_vma_walk_hole(start,end,-1,walk);}-if(pud_huge(pud)&&pud_devmap(pud)){+if(pud_leaf(pud)&&pud_devmap(pud)){unsignedlongi,npages,pfn;unsignedintrequired_fault;unsignedlong*hmm_pfns;
From: Peter Xu <peterx@redhat.com>
There's one small section already, but since we're going to remove
pXd_huge(), that comment may start to obsolete.
Rewrite that section with more information, hopefully with that the API is
crystal clear on what it implies.
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
include/linux/pgtable.h | 24 +++++++++++++++++++-----
1 file changed, 19 insertions(+), 5 deletions(-)
@@ -1770,11 +1770,25 @@ typedef unsigned int pgtbl_mod_mask;#endif/*-*p?d_leaf()-trueifthisentryisafinalmappingtoaphysicaladdress.-*Thisdiffersfromp?d_huge()bythefactthattheyarealwaysavailable(if-*thearchitecturesupportslargepagesattheappropriatelevel)even-*ifCONFIG_HUGETLB_PAGEisnotdefined.-*Onlymeaningfulwhencalledonavalidentry.+*pXd_leaf()istheAPItocheckwhetherapgtableentryisahugepage+*mapping.Itshouldworkgloballyacrossallarchs,withoutany+*dependencyonCONFIG_*options.Forarchitecturesthatdonotsupport+*hugemappingsonspecificlevels,belowfallbackswillbeused.+*+*Aleafpgtableentryshouldalwaysimplythefollowing:+*+*-Itisa"present"entry.IOW,beforeusingthisAPI,pleasecheckit+*withpXd_present()first.NOTE:itmaynotalwaysmeanthe"present+*bit" is set. For example, PROT_NONE entries are always "present".+*+*-Itshould_never_beaswapentryofanytype.Above"present"check+*shouldhaveguardedthis,butlet'sbecrystalclearonthis.+*+*-ItshouldcontainahugePFN,whichpointstoahugepagelargerthan+*PAGE_SIZEoftheplatform.ThePFNformatisn'timportanthere.+*+*-Itshouldcoverallkindsofhugemappings(e.g.,pXd_trans_huge(),+*pXd_devmap(),orhugetlbmappings).*/#ifndef pgd_leaf#define pgd_leaf(x) false
Le 13/03/2024 à 22:47, peterx@redhat.com a écrit :
From: Peter Xu <peterx@redhat.com>
PowerPC book3s 4K mostly has the same definition on both, except pXd_huge()
constantly returns 0 for hash MMUs. As Michael Ellerman pointed out [1],
it is safe to check _PAGE_PTE on hash MMUs, as the bit will never be set so
it will keep returning false.
As a reference, __p[mu]d_mkhuge() will trigger a BUG_ON trying to create
such huge mappings for 4K hash MMUs. Meanwhile, the major powerpc hugetlb
pgtable walker __find_linux_pte() already used pXd_leaf() to check hugetlb
mappings.
The goal should be that we will have one API pXd_leaf() to detect all kinds
of huge mappings. AFAICT we need to use the pXd_leaf() impl (rather than
pXd_huge() ones) to make sure ie. THPs on hash MMU will also return true.
All kinds of huge mappings ?
pXd_leaf() will detect only leaf mappings (like pXd_huge() ). There are
also huge mappings through hugepd. On powerpc 8xx we have 8M huge pages
and 512k huge pages. A PGD entry covers 4M so pgd_leaf() won't report
those huge pages.
quoted hunk
This helps to simplify a follow up patch to drop pXd_huge() treewide.
NOTE: *_leaf() definition need to be moved before the inclusion of
asm/book3s/64/pgtable-4k.h, which defines pXd_huge() with it.
[1] https://lore.kernel.org/r/87v85zo6w7.fsf@mail.lhotse
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Nicholas Piggin <npiggin@gmail.com>
Cc: Christophe Leroy <redacted>
Cc: "Aneesh Kumar K.V" <aneesh.kumar@kernel.org>
Cc: "Naveen N. Rao" <redacted>
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Peter Xu <peterx@redhat.com>
---
.../include/asm/book3s/64/pgtable-4k.h | 14 ++--------
arch/powerpc/include/asm/book3s/64/pgtable.h | 27 +++++++++----------
2 files changed, 14 insertions(+), 27 deletions(-)
@@ -429,7 +429,7 @@ static int hmm_vma_walk_pud(pud_t *pudp, unsigned long start, unsigned long end,returnhmm_vma_walk_hole(start,end,-1,walk);}-if(pud_huge(pud)&&pud_devmap(pud)){+if(pud_leaf(pud)&&pud_devmap(pud)){
Didn't previous patch say devmap implies leaf ? Or is it only for GUP ?
unsigned long i, npages, pfn;
unsigned int required_fault;
unsigned long *hmm_pfns;
From: Peter Xu <peterx@redhat.com> Date: 2024-03-14 12:54:04
On Thu, Mar 14, 2024 at 08:45:34AM +0000, Christophe Leroy wrote:
Le 13/03/2024 à 22:47, peterx@redhat.com a écrit :
quoted
From: Peter Xu <peterx@redhat.com>
PowerPC book3s 4K mostly has the same definition on both, except pXd_huge()
constantly returns 0 for hash MMUs. As Michael Ellerman pointed out [1],
it is safe to check _PAGE_PTE on hash MMUs, as the bit will never be set so
it will keep returning false.
As a reference, __p[mu]d_mkhuge() will trigger a BUG_ON trying to create
such huge mappings for 4K hash MMUs. Meanwhile, the major powerpc hugetlb
pgtable walker __find_linux_pte() already used pXd_leaf() to check hugetlb
mappings.
The goal should be that we will have one API pXd_leaf() to detect all kinds
of huge mappings. AFAICT we need to use the pXd_leaf() impl (rather than
pXd_huge() ones) to make sure ie. THPs on hash MMU will also return true.
All kinds of huge mappings ?
pXd_leaf() will detect only leaf mappings (like pXd_huge() ). There are
also huge mappings through hugepd. On powerpc 8xx we have 8M huge pages
and 512k huge pages. A PGD entry covers 4M so pgd_leaf() won't report
those huge pages.
Ah yes, I should always mention this is in the context of leaf huge pages
only. Are the examples you provided all fall into hugepd category? If so
I can reword the commit message, as:
As a reference, __p[mu]d_mkhuge() will trigger a BUG_ON trying to
create such huge mappings for 4K hash MMUs. Meanwhile, the major
powerpc hugetlb pgtable walker __find_linux_pte() already used
pXd_leaf() to check leaf hugetlb mappings.
The goal should be that we will have one API pXd_leaf() to detect
all kinds of huge mappings except hugepd. AFAICT we need to use
the pXd_leaf() impl (rather than pXd_huge() ones) to make sure
ie. THPs on hash MMU will also return true.
Does this look good to you?
Thanks,
--
Peter Xu
Previous patch said pmd_trans_huge() implies pmd_leaf().
Ah here I remember I kept this arm definition there because I think we
should add a patch to drop pmd_thp_or_huge() completely. If you won't mind
I can add one more patch instead of doing it here. Then I keep this patch
purely as a replacement patch without further changes on arch-cleanups.
@@ -429,7 +429,7 @@ static int hmm_vma_walk_pud(pud_t *pudp, unsigned long start, unsigned long end,returnhmm_vma_walk_hole(start,end,-1,walk);}-if(pud_huge(pud)&&pud_devmap(pud)){+if(pud_leaf(pud)&&pud_devmap(pud)){
Didn't previous patch say devmap implies leaf ? Or is it only for GUP ?
This is an extra safety check that I didn't remove. Devmap used separate
bits even though I'm not clear on why. It should still imply a leaf though.
Thanks,
quoted
unsigned long i, npages, pfn;
unsigned int required_fault;
unsigned long *hmm_pfns;
On Thu, Mar 14, 2024 at 08:45:34AM +0000, Christophe Leroy wrote:
quoted
Le 13/03/2024 à 22:47, peterx@redhat.com a écrit :
quoted
From: Peter Xu <peterx@redhat.com>
PowerPC book3s 4K mostly has the same definition on both, except pXd_huge()
constantly returns 0 for hash MMUs. As Michael Ellerman pointed out [1],
it is safe to check _PAGE_PTE on hash MMUs, as the bit will never be set so
it will keep returning false.
As a reference, __p[mu]d_mkhuge() will trigger a BUG_ON trying to create
such huge mappings for 4K hash MMUs. Meanwhile, the major powerpc hugetlb
pgtable walker __find_linux_pte() already used pXd_leaf() to check hugetlb
mappings.
The goal should be that we will have one API pXd_leaf() to detect all kinds
of huge mappings. AFAICT we need to use the pXd_leaf() impl (rather than
pXd_huge() ones) to make sure ie. THPs on hash MMU will also return true.
All kinds of huge mappings ?
pXd_leaf() will detect only leaf mappings (like pXd_huge() ). There are
also huge mappings through hugepd. On powerpc 8xx we have 8M huge pages
and 512k huge pages. A PGD entry covers 4M so pgd_leaf() won't report
those huge pages.
Ah yes, I should always mention this is in the context of leaf huge pages
only. Are the examples you provided all fall into hugepd category? If so
I can reword the commit message, as:
On powerpc 8xx, only the 8M huge pages fall into the hugepd case.
The 512k hugepages are at PTE level, they are handled more or less like
CONT_PTE on ARM. see function set_huge_pte_at() for more context.
You can also look at pte_leaf_size() and pgd_leaf_size().
By the way pgd_leaf_size() looks odd because it is called only when
pgd_leaf_size() returns true, which never happens for 8M pages.
As a reference, __p[mu]d_mkhuge() will trigger a BUG_ON trying to
create such huge mappings for 4K hash MMUs. Meanwhile, the major
powerpc hugetlb pgtable walker __find_linux_pte() already used
pXd_leaf() to check leaf hugetlb mappings.
The goal should be that we will have one API pXd_leaf() to detect
all kinds of huge mappings except hugepd. AFAICT we need to use
the pXd_leaf() impl (rather than pXd_huge() ones) to make sure
ie. THPs on hash MMU will also return true.
Does this look good to you?
Thanks,
@@ -129,7 +129,7 @@ static inline int pmd_none(pmd_t pmd)staticinlineintpmd_bad(pmd_tpmd){#ifdef CONFIG_MIPS_HUGE_TLB_SUPPORT-/* pmd_huge(pmd) but inline */+/* pmd_leaf(pmd) but inline */
Shouldn't this comment have been changed in patch 11 ?
IMHO it's fine to be here, as this is the patch to finally drop _huge().
Patch 11 only converts the callers to use _leaf()s. So this comment is
still valid until this patch, because this patch removes that definition.
quoted
if (unlikely(pmd_val(pmd) & _PAGE_HUGE))
Unlike pmd_huge() which is an outline function, pmd_leaf() is a macro so
it could be used here instead of open coping.
I worry it will break things as pmd_leaf() can sometimes be defined after
arch *pgtable.h headers. So I avoided touching it except what I think I'm
confident. I had a feeling it's inlined just because of a similar reason
for the old _huge().
Should we keep this comment somewhere for documentation ?
The 2nd/3rd paragraphs are definitely obsolete, so should be dropped.
OTOH, I'm not sure how much that will help if e.g. I move that over to
pmd_leaf(): a check over cpu_to_be64(_PAGE_PTE) is an implementation as
simple as it could be to explain itself with even no comment to me..
I also don't fully digest why that 1st paragraph discusses PGD entries: for
example, there's no pgd_huge() defined. It may not mean that the comment
is wrong, perhaps it means that I may lack some knowledge around this area
on Power..
Would you suggest how I should move paragraph 1 (and help to explain what
it is describing)? Or maybe we can provide a separate patch for Power's
huge page sizes but posted separately (and very possibly I'm not the best
candidate then..).
quoted
-static inline int pmd_huge(pmd_t pmd)
-{
- /*
- * leaf pte for huge page
- */
- return !!(pmd_raw(pmd) & cpu_to_be64(_PAGE_PTE));
-}
-
-static inline int pud_huge(pud_t pud)
-{
- /*
- * leaf pte for huge page
- */
- return !!(pud_raw(pud) & cpu_to_be64(_PAGE_PTE));
-}
/*
* With 64k page size, we have hugepage ptes in the pgd and pmd entries. We don't
From: Jason Gunthorpe <jgg@nvidia.com> Date: 2024-03-18 16:15:23
On Thu, Mar 14, 2024 at 01:11:59PM +0000, Christophe Leroy wrote:
Le 14/03/2024 à 13:53, Peter Xu a écrit :
quoted
On Thu, Mar 14, 2024 at 08:45:34AM +0000, Christophe Leroy wrote:
quoted
Le 13/03/2024 à 22:47, peterx@redhat.com a écrit :
quoted
From: Peter Xu <peterx@redhat.com>
PowerPC book3s 4K mostly has the same definition on both, except pXd_huge()
constantly returns 0 for hash MMUs. As Michael Ellerman pointed out [1],
it is safe to check _PAGE_PTE on hash MMUs, as the bit will never be set so
it will keep returning false.
As a reference, __p[mu]d_mkhuge() will trigger a BUG_ON trying to create
such huge mappings for 4K hash MMUs. Meanwhile, the major powerpc hugetlb
pgtable walker __find_linux_pte() already used pXd_leaf() to check hugetlb
mappings.
The goal should be that we will have one API pXd_leaf() to detect all kinds
of huge mappings. AFAICT we need to use the pXd_leaf() impl (rather than
pXd_huge() ones) to make sure ie. THPs on hash MMU will also return true.
All kinds of huge mappings ?
pXd_leaf() will detect only leaf mappings (like pXd_huge() ). There are
also huge mappings through hugepd. On powerpc 8xx we have 8M huge pages
and 512k huge pages. A PGD entry covers 4M so pgd_leaf() won't report
those huge pages.
Ah yes, I should always mention this is in the context of leaf huge pages
only. Are the examples you provided all fall into hugepd category? If so
I can reword the commit message, as:
On powerpc 8xx, only the 8M huge pages fall into the hugepd case.
The 512k hugepages are at PTE level, they are handled more or less like
CONT_PTE on ARM. see function set_huge_pte_at() for more context.
You can also look at pte_leaf_size() and pgd_leaf_size().
IMHO leaf should return false if the thing is pointing to a next level
page table, even if that next level is fully populated with contiguous
pages.
This seems more aligned with the contig page direction that hugepd
should be moved over to..
By the way pgd_leaf_size() looks odd because it is called only when
pgd_leaf_size() returns true, which never happens for 8M pages.
Like this, you should reach the actual final leaf that the HW will
load and leaf_size() should say it is greater size than the current
table level. Other levels should return 0.
If necessary the core MM code should deal with this by iterating over
adjacent tables.
Jason
From: Jason Gunthorpe <jgg@nvidia.com> Date: 2024-03-18 16:16:28
On Thu, Mar 14, 2024 at 08:59:22AM -0400, Peter Xu wrote:
quoted
quoted
--- a/mm/hmm.c+++ b/mm/hmm.c
@@ -429,7 +429,7 @@ static int hmm_vma_walk_pud(pud_t *pudp, unsigned long start, unsigned long end,returnhmm_vma_walk_hole(start,end,-1,walk);}-if(pud_huge(pud)&&pud_devmap(pud)){+if(pud_leaf(pud)&&pud_devmap(pud)){
Didn't previous patch say devmap implies leaf ? Or is it only for GUP ?
This is an extra safety check that I didn't remove. Devmap used separate
bits even though I'm not clear on why. It should still imply a leaf though.
Yes, something is very wrong if devmap is true on non-leaf..
Jason
On Thu, Mar 14, 2024 at 01:11:59PM +0000, Christophe Leroy wrote:
quoted
Le 14/03/2024 à 13:53, Peter Xu a écrit :
quoted
On Thu, Mar 14, 2024 at 08:45:34AM +0000, Christophe Leroy wrote:
quoted
Le 13/03/2024 à 22:47, peterx@redhat.com a écrit :
quoted
From: Peter Xu <peterx@redhat.com>
PowerPC book3s 4K mostly has the same definition on both, except pXd_huge()
constantly returns 0 for hash MMUs. As Michael Ellerman pointed out [1],
it is safe to check _PAGE_PTE on hash MMUs, as the bit will never be set so
it will keep returning false.
As a reference, __p[mu]d_mkhuge() will trigger a BUG_ON trying to create
such huge mappings for 4K hash MMUs. Meanwhile, the major powerpc hugetlb
pgtable walker __find_linux_pte() already used pXd_leaf() to check hugetlb
mappings.
The goal should be that we will have one API pXd_leaf() to detect all kinds
of huge mappings. AFAICT we need to use the pXd_leaf() impl (rather than
pXd_huge() ones) to make sure ie. THPs on hash MMU will also return true.
All kinds of huge mappings ?
pXd_leaf() will detect only leaf mappings (like pXd_huge() ). There are
also huge mappings through hugepd. On powerpc 8xx we have 8M huge pages
and 512k huge pages. A PGD entry covers 4M so pgd_leaf() won't report
those huge pages.
Ah yes, I should always mention this is in the context of leaf huge pages
only. Are the examples you provided all fall into hugepd category? If so
I can reword the commit message, as:
On powerpc 8xx, only the 8M huge pages fall into the hugepd case.
The 512k hugepages are at PTE level, they are handled more or less like
CONT_PTE on ARM. see function set_huge_pte_at() for more context.
You can also look at pte_leaf_size() and pgd_leaf_size().
IMHO leaf should return false if the thing is pointing to a next level
page table, even if that next level is fully populated with contiguous
pages.
This seems more aligned with the contig page direction that hugepd
should be moved over to..
Should hugepd be moved to the contig page direction, really ?
Would it be acceptable that a 8M hugepage requires 2048 contig entries
in 2 page tables, when the hugepd allows a single entry ? Would it be
acceptable performancewise ?
quoted
By the way pgd_leaf_size() looks odd because it is called only when
pgd_leaf_size() returns true, which never happens for 8M pages.
Like this, you should reach the actual final leaf that the HW will
load and leaf_size() should say it is greater size than the current
table level. Other levels should return 0.
If necessary the core MM code should deal with this by iterating over
adjacent tables.
Jason
From: Jason Gunthorpe <jgg@nvidia.com> Date: 2024-03-19 23:27:00
On Tue, Mar 19, 2024 at 11:07:08PM +0000, Christophe Leroy wrote:
Le 18/03/2024 à 17:15, Jason Gunthorpe a écrit :
quoted
On Thu, Mar 14, 2024 at 01:11:59PM +0000, Christophe Leroy wrote:
quoted
Le 14/03/2024 à 13:53, Peter Xu a écrit :
quoted
On Thu, Mar 14, 2024 at 08:45:34AM +0000, Christophe Leroy wrote:
quoted
Le 13/03/2024 à 22:47, peterx@redhat.com a écrit :
quoted
From: Peter Xu <peterx@redhat.com>
PowerPC book3s 4K mostly has the same definition on both, except pXd_huge()
constantly returns 0 for hash MMUs. As Michael Ellerman pointed out [1],
it is safe to check _PAGE_PTE on hash MMUs, as the bit will never be set so
it will keep returning false.
As a reference, __p[mu]d_mkhuge() will trigger a BUG_ON trying to create
such huge mappings for 4K hash MMUs. Meanwhile, the major powerpc hugetlb
pgtable walker __find_linux_pte() already used pXd_leaf() to check hugetlb
mappings.
The goal should be that we will have one API pXd_leaf() to detect all kinds
of huge mappings. AFAICT we need to use the pXd_leaf() impl (rather than
pXd_huge() ones) to make sure ie. THPs on hash MMU will also return true.
All kinds of huge mappings ?
pXd_leaf() will detect only leaf mappings (like pXd_huge() ). There are
also huge mappings through hugepd. On powerpc 8xx we have 8M huge pages
and 512k huge pages. A PGD entry covers 4M so pgd_leaf() won't report
those huge pages.
Ah yes, I should always mention this is in the context of leaf huge pages
only. Are the examples you provided all fall into hugepd category? If so
I can reword the commit message, as:
On powerpc 8xx, only the 8M huge pages fall into the hugepd case.
The 512k hugepages are at PTE level, they are handled more or less like
CONT_PTE on ARM. see function set_huge_pte_at() for more context.
You can also look at pte_leaf_size() and pgd_leaf_size().
IMHO leaf should return false if the thing is pointing to a next level
page table, even if that next level is fully populated with contiguous
pages.
This seems more aligned with the contig page direction that hugepd
should be moved over to..
Should hugepd be moved to the contig page direction, really ?
Sure? Is there any downside for the reading side to do so?
Would it be acceptable that a 8M hugepage requires 2048 contig entries
in 2 page tables, when the hugepd allows a single entry ?
? I thought we agreed the only difference would be that something new
is needed to merge the two identical sibling page tables into one, ie
you pay 2x the page table memory if that isn't fixed. That is write
side only change and I imagine it could be done with a single PPC
special API.
Honestly not totally sure that is a big deal, it is already really
memory inefficient compared to every other arch's huge page by needing
the child page table in the first place.
Would it be acceptable performancewise ?
Isn't this particular PPC sub platform ancient? Are there current real
users that are going to have hugetlbfs special code and care about
this performance detail on a 6.20 era kernel?
In today's world wouldn't it be performance better if these platforms
could support THP by aligning to the contig API instead of being
special?
Am I wrong to question why we are polluting the core code for this
special optimization?
Jason
On Tue, Mar 19, 2024 at 11:07:08PM +0000, Christophe Leroy wrote:
quoted
Le 18/03/2024 à 17:15, Jason Gunthorpe a écrit :
quoted
On Thu, Mar 14, 2024 at 01:11:59PM +0000, Christophe Leroy wrote:
quoted
Le 14/03/2024 à 13:53, Peter Xu a écrit :
quoted
On Thu, Mar 14, 2024 at 08:45:34AM +0000, Christophe Leroy wrote:
quoted
Le 13/03/2024 à 22:47, peterx@redhat.com a écrit :
quoted
From: Peter Xu <peterx@redhat.com>
PowerPC book3s 4K mostly has the same definition on both, except pXd_huge()
constantly returns 0 for hash MMUs. As Michael Ellerman pointed out [1],
it is safe to check _PAGE_PTE on hash MMUs, as the bit will never be set so
it will keep returning false.
As a reference, __p[mu]d_mkhuge() will trigger a BUG_ON trying to create
such huge mappings for 4K hash MMUs. Meanwhile, the major powerpc hugetlb
pgtable walker __find_linux_pte() already used pXd_leaf() to check hugetlb
mappings.
The goal should be that we will have one API pXd_leaf() to detect all kinds
of huge mappings. AFAICT we need to use the pXd_leaf() impl (rather than
pXd_huge() ones) to make sure ie. THPs on hash MMU will also return true.
All kinds of huge mappings ?
pXd_leaf() will detect only leaf mappings (like pXd_huge() ). There are
also huge mappings through hugepd. On powerpc 8xx we have 8M huge pages
and 512k huge pages. A PGD entry covers 4M so pgd_leaf() won't report
those huge pages.
Ah yes, I should always mention this is in the context of leaf huge pages
only. Are the examples you provided all fall into hugepd category? If so
I can reword the commit message, as:
On powerpc 8xx, only the 8M huge pages fall into the hugepd case.
The 512k hugepages are at PTE level, they are handled more or less like
CONT_PTE on ARM. see function set_huge_pte_at() for more context.
You can also look at pte_leaf_size() and pgd_leaf_size().
IMHO leaf should return false if the thing is pointing to a next level
page table, even if that next level is fully populated with contiguous
pages.
This seems more aligned with the contig page direction that hugepd
should be moved over to..
Should hugepd be moved to the contig page direction, really ?
Sure? Is there any downside for the reading side to do so?
Probably not.
quoted
Would it be acceptable that a 8M hugepage requires 2048 contig entries
in 2 page tables, when the hugepd allows a single entry ?
? I thought we agreed the only difference would be that something new
is needed to merge the two identical sibling page tables into one, ie
you pay 2x the page table memory if that isn't fixed. That is write
side only change and I imagine it could be done with a single PPC
special API.
Honestly not totally sure that is a big deal, it is already really
memory inefficient compared to every other arch's huge page by needing
the child page table in the first place.
quoted
Would it be acceptable performancewise ?
Isn't this particular PPC sub platform ancient? Are there current real
users that are going to have hugetlbfs special code and care about
this performance detail on a 6.20 era kernel?
Ancient yes but still widely in use and with the emergence of voice over
IP in Air Trafic Control, performance becomes more and more challenge
with those old boards that have another 10 years in front of them.
In today's world wouldn't it be performance better if these platforms
could support THP by aligning to the contig API instead of being
special?
Indeed, if we can promote THP that'd be even better.
Am I wrong to question why we are polluting the core code for this
special optimization?
At the first place that was to get a close fit between hardware
pagetable topology and linux pagetable topology. But obviously we
already stepped back for 512k pages, so let's go one more step aside and
do similar with 8M pages.
I'll give it a try and see how it goes.
Christophe
From: Peter Xu <peterx@redhat.com> Date: 2024-03-20 16:09:12
On Wed, Mar 20, 2024 at 06:16:43AM +0000, Christophe Leroy wrote:
At the first place that was to get a close fit between hardware
pagetable topology and linux pagetable topology. But obviously we
already stepped back for 512k pages, so let's go one more step aside and
do similar with 8M pages.
I'll give it a try and see how it goes.
So you're talking about 8M only for 8xx, am I right?
There seem to be other PowerPC systems use hugepd. Is it possible that we
convert all hugepd into cont_pte form?
Thanks,
--
Peter Xu
On Wed, Mar 20, 2024 at 06:16:43AM +0000, Christophe Leroy wrote:
quoted
At the first place that was to get a close fit between hardware
pagetable topology and linux pagetable topology. But obviously we
already stepped back for 512k pages, so let's go one more step aside and
do similar with 8M pages.
I'll give it a try and see how it goes.
So you're talking about 8M only for 8xx, am I right?
Yes I am.
There seem to be other PowerPC systems use hugepd. Is it possible that we
convert all hugepd into cont_pte form?
Indeed.
Seems like we have hugepd for book3s/64 and for nohash.
For book3s I don't know, may Aneesh can answer.
For nohash I think it should be possible because TLB misses are handled
by software. Even the e6500 which has a hardware tablewalk falls back on
software walk when it is a hugepage IIUC.
Christophe
From: Peter Xu <peterx@redhat.com> Date: 2024-03-20 20:24:33
On Wed, Mar 20, 2024 at 05:40:39PM +0000, Christophe Leroy wrote:
Le 20/03/2024 à 17:09, Peter Xu a écrit :
quoted
On Wed, Mar 20, 2024 at 06:16:43AM +0000, Christophe Leroy wrote:
quoted
At the first place that was to get a close fit between hardware
pagetable topology and linux pagetable topology. But obviously we
already stepped back for 512k pages, so let's go one more step aside and
do similar with 8M pages.
I'll give it a try and see how it goes.
So you're talking about 8M only for 8xx, am I right?
Yes I am.
quoted
There seem to be other PowerPC systems use hugepd. Is it possible that we
convert all hugepd into cont_pte form?
Indeed.
Seems like we have hugepd for book3s/64 and for nohash.
For book3s I don't know, may Aneesh can answer.
For nohash I think it should be possible because TLB misses are handled
by software. Even the e6500 which has a hardware tablewalk falls back on
software walk when it is a hugepage IIUC.
It'll be great if I can get some answer here, and then I know the path for
hugepd in general. I don't want to add any new code into core mm to
something destined to fade away soon.
One option for me is I can check a macro of hugepd existance, so all new
code will only work when hugepd is not supported on such arch. However
that'll start to make some PowerPC systems special (which I still tried
hard to avoid, if that wasn't proved in the past..), meanwhile we'll also
need to keep some generic-mm paths (that I can already remove along with
the new code) only for these hugepd systems. But it's still okay to me,
it'll be just a matter of when to drop those codes, sooner or later.
Thanks,
--
Peter Xu