Re: [PATCH 3/3] mm/hugetlb: Introduce HAVE_ARCH_CLEAR_HUGEPAGE_FLAGS
On Tue, 5 May 2020 08:21:34 +0530 Anshuman Khandual wrote: > >>> static inline void arch_clear_hugepage_flags(struct page *page) > >>> { > >>> > >>> } > >>> #define arch_clear_hugepage_flags arch_clear_hugepage_flags > >>> > >>> It's a small difference - mainly to avoid adding two variables to the > >>> overall namespace where one would do. > >> > >> Understood, will change and resend. > > > > That's OK - I've queued up that fix. > > > > Hello Andrew, > > I might not have searched all the relevant trees or might have just searched > earlier than required. But I dont see these patches (or your proposed fixes) > either in mmotm (2020-04-29-23-04) or in next-20200504. Wondering if you are > waiting on a V2 for this series accommodating the changes you had proposed. hm. I think I must have got confused and thought you were referring to a different patch. Yes please, let's have v2.
Re: [PATCH 3/3] mm/hugetlb: Introduce HAVE_ARCH_CLEAR_HUGEPAGE_FLAGS
On 04/26/2020 08:31 AM, Andrew Morton wrote: > On Sun, 26 Apr 2020 08:13:17 +0530 Anshuman Khandual > wrote: > >> >> >> On 04/26/2020 06:25 AM, Andrew Morton wrote: >>> On Tue, 14 Apr 2020 17:14:30 +0530 Anshuman Khandual >>> wrote: >>> There are multiple similar definitions for arch_clear_hugepage_flags() on various platforms. This introduces HAVE_ARCH_CLEAR_HUGEPAGE_FLAGS for those platforms that need to define their own arch_clear_hugepage_flags() while also providing a generic fallback definition for others to use. This help reduce code duplication. ... --- a/include/linux/hugetlb.h +++ b/include/linux/hugetlb.h @@ -544,6 +544,10 @@ static inline int is_hugepage_only_range(struct mm_struct *mm, } #endif +#ifndef HAVE_ARCH_CLEAR_HUGEPAGE_FLAGS +static inline void arch_clear_hugepage_flags(struct page *page) { } +#endif + #ifndef arch_make_huge_pte static inline pte_t arch_make_huge_pte(pte_t entry, struct vm_area_struct *vma, struct page *page, int writable) >>> >>> This is the rather old-school way of doing it. The Linus-suggested way is >>> >>> #ifndef arch_clear_hugepage_flags >>> static inline void arch_clear_hugepage_flags(struct page *page) >>> { >>> } >>> #define arch_clear_hugepage_flags arch_clear_hugepage_flags >> >> Do we need that above line here ? Is not that implicit. > > It depends if other header files want to test whether > arch_clear_hugepage_flags is already defined. If the header heorarchy > is well-defined and working properly, they shouldn't need to, because > we're reliably indluding the relevant arch header before (or early > within) include/linux/hugetlb.h. > > It would be nice if > > #define arch_clear_hugepage_flags arch_clear_hugepage_flags > #define arch_clear_hugepage_flags arch_clear_hugepage_flags > > were to generate an compiler error but it doesn't. If it did we could > detect these incorrect inclusion orders. > >>> #endif >>> >>> And the various arch headers do >>> >>> static inline void arch_clear_hugepage_flags(struct page *page) >>> { >>> >>> } >>> #define arch_clear_hugepage_flags arch_clear_hugepage_flags >>> >>> It's a small difference - mainly to avoid adding two variables to the >>> overall namespace where one would do. >> >> Understood, will change and resend. > > That's OK - I've queued up that fix. > Hello Andrew, I might not have searched all the relevant trees or might have just searched earlier than required. But I dont see these patches (or your proposed fixes) either in mmotm (2020-04-29-23-04) or in next-20200504. Wondering if you are waiting on a V2 for this series accommodating the changes you had proposed. - Anshuman
Re: [PATCH 3/3] mm/hugetlb: Introduce HAVE_ARCH_CLEAR_HUGEPAGE_FLAGS
On Sun, 26 Apr 2020 08:13:17 +0530 Anshuman Khandual wrote: > > > On 04/26/2020 06:25 AM, Andrew Morton wrote: > > On Tue, 14 Apr 2020 17:14:30 +0530 Anshuman Khandual > > wrote: > > > >> There are multiple similar definitions for arch_clear_hugepage_flags() on > >> various platforms. This introduces HAVE_ARCH_CLEAR_HUGEPAGE_FLAGS for those > >> platforms that need to define their own arch_clear_hugepage_flags() while > >> also providing a generic fallback definition for others to use. This help > >> reduce code duplication. > >> > >> ... > >> > >> --- a/include/linux/hugetlb.h > >> +++ b/include/linux/hugetlb.h > >> @@ -544,6 +544,10 @@ static inline int is_hugepage_only_range(struct > >> mm_struct *mm, > >> } > >> #endif > >> > >> +#ifndef HAVE_ARCH_CLEAR_HUGEPAGE_FLAGS > >> +static inline void arch_clear_hugepage_flags(struct page *page) { } > >> +#endif > >> + > >> #ifndef arch_make_huge_pte > >> static inline pte_t arch_make_huge_pte(pte_t entry, struct vm_area_struct > >> *vma, > >> struct page *page, int writable) > > > > This is the rather old-school way of doing it. The Linus-suggested way is > > > > #ifndef arch_clear_hugepage_flags > > static inline void arch_clear_hugepage_flags(struct page *page) > > { > > } > > #define arch_clear_hugepage_flags arch_clear_hugepage_flags > > Do we need that above line here ? Is not that implicit. It depends if other header files want to test whether arch_clear_hugepage_flags is already defined. If the header heorarchy is well-defined and working properly, they shouldn't need to, because we're reliably indluding the relevant arch header before (or early within) include/linux/hugetlb.h. It would be nice if #define arch_clear_hugepage_flags arch_clear_hugepage_flags #define arch_clear_hugepage_flags arch_clear_hugepage_flags were to generate an compiler error but it doesn't. If it did we could detect these incorrect inclusion orders. > > #endif > > > > And the various arch headers do > > > > static inline void arch_clear_hugepage_flags(struct page *page) > > { > > > > } > > #define arch_clear_hugepage_flags arch_clear_hugepage_flags > > > > It's a small difference - mainly to avoid adding two variables to the > > overall namespace where one would do. > > Understood, will change and resend. That's OK - I've queued up that fix.
Re: [PATCH 3/3] mm/hugetlb: Introduce HAVE_ARCH_CLEAR_HUGEPAGE_FLAGS
On 04/26/2020 06:25 AM, Andrew Morton wrote: > On Tue, 14 Apr 2020 17:14:30 +0530 Anshuman Khandual > wrote: > >> There are multiple similar definitions for arch_clear_hugepage_flags() on >> various platforms. This introduces HAVE_ARCH_CLEAR_HUGEPAGE_FLAGS for those >> platforms that need to define their own arch_clear_hugepage_flags() while >> also providing a generic fallback definition for others to use. This help >> reduce code duplication. >> >> ... >> >> --- a/include/linux/hugetlb.h >> +++ b/include/linux/hugetlb.h >> @@ -544,6 +544,10 @@ static inline int is_hugepage_only_range(struct >> mm_struct *mm, >> } >> #endif >> >> +#ifndef HAVE_ARCH_CLEAR_HUGEPAGE_FLAGS >> +static inline void arch_clear_hugepage_flags(struct page *page) { } >> +#endif >> + >> #ifndef arch_make_huge_pte >> static inline pte_t arch_make_huge_pte(pte_t entry, struct vm_area_struct >> *vma, >> struct page *page, int writable) > > This is the rather old-school way of doing it. The Linus-suggested way is > > #ifndef arch_clear_hugepage_flags > static inline void arch_clear_hugepage_flags(struct page *page) > { > } > #define arch_clear_hugepage_flags arch_clear_hugepage_flags Do we need that above line here ? Is not that implicit. > #endif > > And the various arch headers do > > static inline void arch_clear_hugepage_flags(struct page *page) > { > > } > #define arch_clear_hugepage_flags arch_clear_hugepage_flags > > It's a small difference - mainly to avoid adding two variables to the > overall namespace where one would do. Understood, will change and resend. > >
Re: [PATCH 3/3] mm/hugetlb: Introduce HAVE_ARCH_CLEAR_HUGEPAGE_FLAGS
On Tue, 14 Apr 2020 17:14:30 +0530 Anshuman Khandual wrote: > There are multiple similar definitions for arch_clear_hugepage_flags() on > various platforms. This introduces HAVE_ARCH_CLEAR_HUGEPAGE_FLAGS for those > platforms that need to define their own arch_clear_hugepage_flags() while > also providing a generic fallback definition for others to use. This help > reduce code duplication. > > ... > > --- a/include/linux/hugetlb.h > +++ b/include/linux/hugetlb.h > @@ -544,6 +544,10 @@ static inline int is_hugepage_only_range(struct > mm_struct *mm, > } > #endif > > +#ifndef HAVE_ARCH_CLEAR_HUGEPAGE_FLAGS > +static inline void arch_clear_hugepage_flags(struct page *page) { } > +#endif > + > #ifndef arch_make_huge_pte > static inline pte_t arch_make_huge_pte(pte_t entry, struct vm_area_struct > *vma, > struct page *page, int writable) This is the rather old-school way of doing it. The Linus-suggested way is #ifndef arch_clear_hugepage_flags static inline void arch_clear_hugepage_flags(struct page *page) { } #define arch_clear_hugepage_flags arch_clear_hugepage_flags #endif And the various arch headers do static inline void arch_clear_hugepage_flags(struct page *page) { } #define arch_clear_hugepage_flags arch_clear_hugepage_flags It's a small difference - mainly to avoid adding two variables to the overall namespace where one would do.
[PATCH 3/3] mm/hugetlb: Introduce HAVE_ARCH_CLEAR_HUGEPAGE_FLAGS
There are multiple similar definitions for arch_clear_hugepage_flags() on various platforms. This introduces HAVE_ARCH_CLEAR_HUGEPAGE_FLAGS for those platforms that need to define their own arch_clear_hugepage_flags() while also providing a generic fallback definition for others to use. This help reduce code duplication. Cc: Russell King Cc: Catalin Marinas Cc: Will Deacon Cc: Tony Luck Cc: Fenghua Yu Cc: Thomas Bogendoerfer Cc: "James E.J. Bottomley" Cc: Helge Deller Cc: Benjamin Herrenschmidt Cc: Paul Mackerras Cc: Michael Ellerman Cc: Paul Walmsley Cc: Palmer Dabbelt Cc: Heiko Carstens Cc: Vasily Gorbik Cc: Christian Borntraeger Cc: Yoshinori Sato Cc: Rich Felker Cc: "David S. Miller" Cc: Thomas Gleixner Cc: Ingo Molnar Cc: Borislav Petkov Cc: "H. Peter Anvin" Cc: Mike Kravetz Cc: x...@kernel.org Cc: linux-arm-ker...@lists.infradead.org Cc: linux-i...@vger.kernel.org Cc: linux-m...@vger.kernel.org Cc: linux-par...@vger.kernel.org Cc: linuxppc-dev@lists.ozlabs.org Cc: linux-ri...@lists.infradead.org Cc: linux-s...@vger.kernel.org Cc: linux...@vger.kernel.org Cc: sparcli...@vger.kernel.org Cc: linux...@kvack.org Cc: linux-a...@vger.kernel.org Cc: linux-ker...@vger.kernel.org Signed-off-by: Anshuman Khandual --- arch/arm/include/asm/hugetlb.h | 1 + arch/arm64/include/asm/hugetlb.h | 1 + arch/ia64/include/asm/hugetlb.h| 4 arch/mips/include/asm/hugetlb.h| 4 arch/parisc/include/asm/hugetlb.h | 4 arch/powerpc/include/asm/hugetlb.h | 4 arch/riscv/include/asm/hugetlb.h | 4 arch/s390/include/asm/hugetlb.h| 1 + arch/sh/include/asm/hugetlb.h | 1 + arch/sparc/include/asm/hugetlb.h | 4 arch/x86/include/asm/hugetlb.h | 4 include/linux/hugetlb.h| 4 12 files changed, 8 insertions(+), 28 deletions(-) diff --git a/arch/arm/include/asm/hugetlb.h b/arch/arm/include/asm/hugetlb.h index 9ecd516d1ff7..7107c1e6f020 100644 --- a/arch/arm/include/asm/hugetlb.h +++ b/arch/arm/include/asm/hugetlb.h @@ -14,6 +14,7 @@ #include #include +#define HAVE_ARCH_CLEAR_HUGEPAGE_FLAGS static inline void arch_clear_hugepage_flags(struct page *page) { clear_bit(PG_dcache_clean, >flags); diff --git a/arch/arm64/include/asm/hugetlb.h b/arch/arm64/include/asm/hugetlb.h index 8f58e052697a..5369cf26a05d 100644 --- a/arch/arm64/include/asm/hugetlb.h +++ b/arch/arm64/include/asm/hugetlb.h @@ -17,6 +17,7 @@ extern bool arch_hugetlb_migration_supported(struct hstate *h); #endif +#define HAVE_ARCH_CLEAR_HUGEPAGE_FLAGS static inline void arch_clear_hugepage_flags(struct page *page) { clear_bit(PG_dcache_clean, >flags); diff --git a/arch/ia64/include/asm/hugetlb.h b/arch/ia64/include/asm/hugetlb.h index f17c1e228045..2ecff18a32e5 100644 --- a/arch/ia64/include/asm/hugetlb.h +++ b/arch/ia64/include/asm/hugetlb.h @@ -28,10 +28,6 @@ static inline void huge_ptep_clear_flush(struct vm_area_struct *vma, { } -static inline void arch_clear_hugepage_flags(struct page *page) -{ -} - #include #endif /* _ASM_IA64_HUGETLB_H */ diff --git a/arch/mips/include/asm/hugetlb.h b/arch/mips/include/asm/hugetlb.h index 8b201e281f67..10e3be870df7 100644 --- a/arch/mips/include/asm/hugetlb.h +++ b/arch/mips/include/asm/hugetlb.h @@ -75,10 +75,6 @@ static inline int huge_ptep_set_access_flags(struct vm_area_struct *vma, return changed; } -static inline void arch_clear_hugepage_flags(struct page *page) -{ -} - #include #endif /* __ASM_HUGETLB_H */ diff --git a/arch/parisc/include/asm/hugetlb.h b/arch/parisc/include/asm/hugetlb.h index 411d9d867baa..a69cf9efb0c1 100644 --- a/arch/parisc/include/asm/hugetlb.h +++ b/arch/parisc/include/asm/hugetlb.h @@ -42,10 +42,6 @@ int huge_ptep_set_access_flags(struct vm_area_struct *vma, unsigned long addr, pte_t *ptep, pte_t pte, int dirty); -static inline void arch_clear_hugepage_flags(struct page *page) -{ -} - #include #endif /* _ASM_PARISC64_HUGETLB_H */ diff --git a/arch/powerpc/include/asm/hugetlb.h b/arch/powerpc/include/asm/hugetlb.h index 9dd50e1e4fe5..d259e056e6cf 100644 --- a/arch/powerpc/include/asm/hugetlb.h +++ b/arch/powerpc/include/asm/hugetlb.h @@ -61,10 +61,6 @@ int huge_ptep_set_access_flags(struct vm_area_struct *vma, unsigned long addr, pte_t *ptep, pte_t pte, int dirty); -static inline void arch_clear_hugepage_flags(struct page *page) -{ -} - #include #else /* ! CONFIG_HUGETLB_PAGE */ diff --git a/arch/riscv/include/asm/hugetlb.h b/arch/riscv/include/asm/hugetlb.h index 866f6ae6467c..a5c2ca1d1cd8 100644 --- a/arch/riscv/include/asm/hugetlb.h +++ b/arch/riscv/include/asm/hugetlb.h @@ -5,8 +5,4 @@ #include #include -static inline void arch_clear_hugepage_flags(struct page *page) -{ -} - #endif /* _ASM_RISCV_HUGETLB_H */ diff --git a/arch/s390/include/asm/hugetlb.h