On Tue, Apr 02, 2024 at 12:05:49PM -0700, Nathan Chancellor wrote:
> Hi Peter (and LoongArch folks),
>
> On Wed, Mar 27, 2024 at 11:23:24AM -0400, pet...@redhat.com wrote:
> > From: Peter Xu <pet...@redhat.com>
> >
> > The comment in the code explains the reasons. We took a different approach
> > comparing to pmd_pfn() by providing a fallback function.
> >
> > Another option is to provide some lower level config options (compare to
> > HUGETLB_PAGE or THP) to identify which layer an arch can support for such
> > huge mappings. However that can be an overkill.
> >
> > Cc: Mike Rapoport (IBM) <r...@kernel.org>
> > Cc: Matthew Wilcox <wi...@infradead.org>
> > Reviewed-by: Jason Gunthorpe <j...@nvidia.com>
> > Signed-off-by: Peter Xu <pet...@redhat.com>
> > ---
> > arch/riscv/include/asm/pgtable.h | 1 +
> > arch/s390/include/asm/pgtable.h | 1 +
> > arch/sparc/include/asm/pgtable_64.h | 1 +
> > arch/x86/include/asm/pgtable.h | 1 +
> > include/linux/pgtable.h | 10 ++++++++++
> > 5 files changed, 14 insertions(+)
> >
> > diff --git a/arch/riscv/include/asm/pgtable.h
> > b/arch/riscv/include/asm/pgtable.h
> > index 20242402fc11..0ca28cc8e3fa 100644
> > --- a/arch/riscv/include/asm/pgtable.h
> > +++ b/arch/riscv/include/asm/pgtable.h
> > @@ -646,6 +646,7 @@ static inline unsigned long pmd_pfn(pmd_t pmd)
> >
> > #define __pud_to_phys(pud) (__page_val_to_pfn(pud_val(pud)) << PAGE_SHIFT)
> >
> > +#define pud_pfn pud_pfn
> > static inline unsigned long pud_pfn(pud_t pud)
> > {
> > return ((__pud_to_phys(pud) & PUD_MASK) >> PAGE_SHIFT);
> > diff --git a/arch/s390/include/asm/pgtable.h
> > b/arch/s390/include/asm/pgtable.h
> > index 1a71cb19c089..6cbbe473f680 100644
> > --- a/arch/s390/include/asm/pgtable.h
> > +++ b/arch/s390/include/asm/pgtable.h
> > @@ -1414,6 +1414,7 @@ static inline unsigned long pud_deref(pud_t pud)
> > return (unsigned long)__va(pud_val(pud) & origin_mask);
> > }
> >
> > +#define pud_pfn pud_pfn
> > static inline unsigned long pud_pfn(pud_t pud)
> > {
> > return __pa(pud_deref(pud)) >> PAGE_SHIFT;
> > diff --git a/arch/sparc/include/asm/pgtable_64.h
> > b/arch/sparc/include/asm/pgtable_64.h
> > index 4d1bafaba942..26efc9bb644a 100644
> > --- a/arch/sparc/include/asm/pgtable_64.h
> > +++ b/arch/sparc/include/asm/pgtable_64.h
> > @@ -875,6 +875,7 @@ static inline bool pud_leaf(pud_t pud)
> > return pte_val(pte) & _PAGE_PMD_HUGE;
> > }
> >
> > +#define pud_pfn pud_pfn
> > static inline unsigned long pud_pfn(pud_t pud)
> > {
> > pte_t pte = __pte(pud_val(pud));
> > diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
> > index cefc7a84f7a4..273f7557218c 100644
> > --- a/arch/x86/include/asm/pgtable.h
> > +++ b/arch/x86/include/asm/pgtable.h
> > @@ -234,6 +234,7 @@ static inline unsigned long pmd_pfn(pmd_t pmd)
> > return (pfn & pmd_pfn_mask(pmd)) >> PAGE_SHIFT;
> > }
> >
> > +#define pud_pfn pud_pfn
> > static inline unsigned long pud_pfn(pud_t pud)
> > {
> > phys_addr_t pfn = pud_val(pud);
> > diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
> > index 600e17d03659..75fe309a4e10 100644
> > --- a/include/linux/pgtable.h
> > +++ b/include/linux/pgtable.h
> > @@ -1817,6 +1817,16 @@ typedef unsigned int pgtbl_mod_mask;
> > #define pte_leaf_size(x) PAGE_SIZE
> > #endif
> >
> > +/*
> > + * We always define pmd_pfn for all archs as it's used in lots of generic
> > + * code. Now it happens too for pud_pfn (and can happen for larger
> > + * mappings too in the future; we're not there yet). Instead of defining
> > + * it for all archs (like pmd_pfn), provide a fallback.
> > + */
> > +#ifndef pud_pfn
> > +#define pud_pfn(x) ({ BUILD_BUG(); 0; })
> > +#endif
> > +
> > /*
> > * Some architectures have MMUs that are configurable or selectable at boot
> > * time. These lead to variable PTRS_PER_x. For statically allocated
> > arrays it
> > --
> > 2.44.0
> >
>
> This BUILD_BUG() triggers for LoongArch with their defconfig, so it
> seems like they need to provide an implementation of pud_pfn()?
>
> In function 'follow_huge_pud',
> inlined from 'follow_pud_mask' at mm/gup.c:1075:10,
> inlined from 'follow_p4d_mask' at mm/gup.c:1105:9,
> inlined from 'follow_page_mask' at mm/gup.c:1151:10:
> include/linux/compiler_types.h:460:45: error: call to
> '__compiletime_assert_382' declared with attribute error: BUILD_BUG failed
> 460 | _compiletime_assert(condition, msg, __compiletime_assert_,
> __COUNTER__)
> | ^
> include/linux/compiler_types.h:441:25: note: in definition of macro
> '__compiletime_assert'
> 441 | prefix ## suffix();
> \
> | ^~~~~~
> include/linux/compiler_types.h:460:9: note: in expansion of macro
> '_compiletime_assert'
> 460 | _compiletime_assert(condition, msg, __compiletime_assert_,
> __COUNTER__)
> | ^~~~~~~~~~~~~~~~~~~
> include/linux/build_bug.h:39:37: note: in expansion of macro
> 'compiletime_assert'
> 39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
> | ^~~~~~~~~~~~~~~~~~
> include/linux/build_bug.h:59:21: note: in expansion of macro
> 'BUILD_BUG_ON_MSG'
> 59 | #define BUILD_BUG() BUILD_BUG_ON_MSG(1, "BUILD_BUG failed")
> | ^~~~~~~~~~~~~~~~
> include/linux/pgtable.h:1887:23: note: in expansion of macro 'BUILD_BUG'
> 1887 | #define pud_pfn(x) ({ BUILD_BUG(); 0; })
> | ^~~~~~~~~
> mm/gup.c:679:29: note: in expansion of macro 'pud_pfn'
> 679 | unsigned long pfn = pud_pfn(pud);
> | ^~~~~~~
I actually tested this without hitting the issue (even though I didn't
mention it in the cover letter..). I re-kicked the build test, it turns
out my "make alldefconfig" on loongarch will generate a config with both
HUGETLB=n && THP=n, while arch/loongarch/configs/loongson3_defconfig has
THP=y (which I assume was the one above build used). I didn't further
check how "make alldefconfig" generated the config; a bit surprising that
it didn't fetch from there.
(and it also surprises me that this BUILD_BUG can trigger.. I used to try
triggering it elsewhere but failed..)
For loongarch the best thing is not compile in follow_huge_pud(), as it
doesn't support pud dax, neither does it support pud hugetlb. However
again that may require some more CONFIG_* options to declare the level one
arch supports on HUGETLB_PAGE. Here maybe the simplest (and it should also
cover all the rest archs on similar issues if ever possible to happen) is
we remove the BUILD_BUG() and explain why. It should be safe for loongarch
too here in this case to not defined it until properly supported.
Thanks,
===8<===
>From 585f34aa3d5b12cd2186367b0882d4293f792062 Mon Sep 17 00:00:00 2001
From: Peter Xu <pet...@redhat.com>
Date: Tue, 2 Apr 2024 18:31:07 -0400
Subject: [PATCH] fixup! mm/arch: provide pud_pfn() fallback
Signed-off-by: Peter Xu <pet...@redhat.com>
---
include/linux/pgtable.h | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
index fa8f92f6e2d7..0f4b2faa1d71 100644
--- a/include/linux/pgtable.h
+++ b/include/linux/pgtable.h
@@ -1882,9 +1882,13 @@ typedef unsigned int pgtbl_mod_mask;
* code. Now it happens too for pud_pfn (and can happen for larger
* mappings too in the future; we're not there yet). Instead of defining
* it for all archs (like pmd_pfn), provide a fallback.
+ *
+ * Note that returning 0 here means any arch that didn't define this can
+ * get severely wrong when it hits a real pud leaf. It's arch's
+ * responsibility to properly define it when a huge pud is possible.
*/
#ifndef pud_pfn
-#define pud_pfn(x) ({ BUILD_BUG(); 0; })
+#define pud_pfn(x) 0
#endif
/*
--
2.44.0
--
Peter Xu
