[PATCH v9 5/7] powerpc/tlb: Add local flush for page given mm_struct and psize
Benjamin Gray
bgray at linux.ibm.com
Thu Nov 3 11:39:19 AEDT 2022
On Wed, 2022-11-02 at 09:56 +0000, Christophe Leroy wrote:
>
>
> Le 25/10/2022 à 06:44, Benjamin Gray a écrit :
> > Adds a local TLB flush operation that works given an mm_struct, VA
> > to
> > flush, and page size representation. Most implementations mirror
> > the
> > surrounding code. The book3s/32/tlbflush.h implementation is left
> > as
> > a WARN_ONCE_ON because it is more complicated and not required for
>
> s/ONCE_ON/ONCE
>
> > anything as yet.
>
> Is a WARN_ONCE() really needed ? Can't a BUILD_BUG() be used instead
> ?
Looks like BUILD_BUG is safe. Tested with 83xx/mpc834x_mds_defconfig,
it doesn't break the build.
> >
> > This removes the need to create a vm_area_struct, which the
> > temporary
> > patching mm work does not need.
> >
> > Signed-off-by: Benjamin Gray <bgray at linux.ibm.com>
> > ---
> > v9: * Replace book3s/32/tlbflush.h implementation with warning
> > ---
> > arch/powerpc/include/asm/book3s/32/tlbflush.h | 9 +++++++++
> > arch/powerpc/include/asm/book3s/64/tlbflush-hash.h | 5 +++++
> > arch/powerpc/include/asm/book3s/64/tlbflush.h | 8 ++++++++
> > arch/powerpc/include/asm/nohash/tlbflush.h | 8 ++++++++
> > 4 files changed, 30 insertions(+)
> >
> > diff --git a/arch/powerpc/include/asm/book3s/32/tlbflush.h
> > b/arch/powerpc/include/asm/book3s/32/tlbflush.h
> > index ba1743c52b56..14d989d41f75 100644
> > --- a/arch/powerpc/include/asm/book3s/32/tlbflush.h
> > +++ b/arch/powerpc/include/asm/book3s/32/tlbflush.h
> > @@ -2,6 +2,8 @@
> > #ifndef _ASM_POWERPC_BOOK3S_32_TLBFLUSH_H
> > #define _ASM_POWERPC_BOOK3S_32_TLBFLUSH_H
> >
> > +#include <asm/bug.h>
> > +
> > #define MMU_NO_CONTEXT (0)
> > /*
> > * TLB flushing for "classic" hash-MMU 32-bit CPUs, 6xx, 7xx,
> > 7xxx
> > @@ -74,6 +76,13 @@ static inline void local_flush_tlb_page(struct
> > vm_area_struct *vma,
> > {
> > flush_tlb_page(vma, vmaddr);
> > }
> > +
> > +static inline void local_flush_tlb_page_psize(struct mm_struct
> > *mm,
> > + unsigned long vmaddr,
> > int psize)
> > +{
> > + WARN_ONCE(true, "local TLB flush not implemented");
>
> Is it possible to use BUILD_BUG() instead ?
Yep, above
> > +}
> > +
> > static inline void local_flush_tlb_mm(struct mm_struct *mm)
> > {
> > flush_tlb_mm(mm);
> > diff --git a/arch/powerpc/include/asm/book3s/64/tlbflush-hash.h
> > b/arch/powerpc/include/asm/book3s/64/tlbflush-hash.h
> > index fab8332fe1ad..8fd9dc49b2a1 100644
> > --- a/arch/powerpc/include/asm/book3s/64/tlbflush-hash.h
> > +++ b/arch/powerpc/include/asm/book3s/64/tlbflush-hash.h
> > @@ -94,6 +94,11 @@ static inline void
> > hash__local_flush_tlb_page(struct vm_area_struct *vma,
> > {
> > }
> >
> > +static inline void hash__local_flush_tlb_page_psize(struct
> > mm_struct *mm,
> > + unsigned long
> > vmaddr, int psize)
> > +{
> > +}
> > +
>
> Is it worth an empty function ?
See end
> > static inline void hash__flush_tlb_page(struct vm_area_struct
> > *vma,
> > unsigned long vmaddr)
> > {
> > diff --git a/arch/powerpc/include/asm/book3s/64/tlbflush.h
> > b/arch/powerpc/include/asm/book3s/64/tlbflush.h
> > index 67655cd60545..2d839dd5c08c 100644
> > --- a/arch/powerpc/include/asm/book3s/64/tlbflush.h
> > +++ b/arch/powerpc/include/asm/book3s/64/tlbflush.h
> > @@ -92,6 +92,14 @@ static inline void local_flush_tlb_page(struct
> > vm_area_struct *vma,
> > return hash__local_flush_tlb_page(vma, vmaddr);
> > }
> >
> > +static inline void local_flush_tlb_page_psize(struct mm_struct
> > *mm,
> > + unsigned long vmaddr,
> > int psize)
> > +{
> > + if (radix_enabled())
> > + return radix__local_flush_tlb_page_psize(mm,
> > vmaddr, psize);
> > + return hash__local_flush_tlb_page_psize(mm, vmaddr, psize);
>
> Those functions are 'void', shouldn't need the "return".
>
> Could just be:
>
> +static inline void local_flush_tlb_page_psize(struct mm_struct *mm,
> + unsigned long vmaddr,
> int psize)
> +{
> + if (radix_enabled())
> + radix__local_flush_tlb_page_psize(mm, vmaddr, psize);
> +}
See end
> > +}
> > +
> > static inline void local_flush_all_mm(struct mm_struct *mm)
> > {
> > if (radix_enabled())
> > diff --git a/arch/powerpc/include/asm/nohash/tlbflush.h
> > b/arch/powerpc/include/asm/nohash/tlbflush.h
> > index bdaf34ad41ea..432bca4cac62 100644
> > --- a/arch/powerpc/include/asm/nohash/tlbflush.h
> > +++ b/arch/powerpc/include/asm/nohash/tlbflush.h
> > @@ -45,6 +45,12 @@ static inline void local_flush_tlb_page(struct
> > vm_area_struct *vma, unsigned lon
> > asm volatile ("tlbie %0; sync" : : "r" (vmaddr) :
> > "memory");
> > }
> >
> > +static inline void local_flush_tlb_page_psize(struct mm_struct
> > *mm,
> > + unsigned long vmaddr,
> > int psize)
> > +{
> > + asm volatile ("tlbie %0; sync" : : "r" (vmaddr) :
> > "memory");
> > +}
> > +
> > static inline void flush_tlb_kernel_range(unsigned long start,
> > unsigned long end)
> > {
> > start &= PAGE_MASK;
> > @@ -58,6 +64,8 @@ static inline void
> > flush_tlb_kernel_range(unsigned long start, unsigned long end
> > extern void flush_tlb_kernel_range(unsigned long start, unsigned
> > long end);
> > extern void local_flush_tlb_mm(struct mm_struct *mm);
> > extern void local_flush_tlb_page(struct vm_area_struct *vma,
> > unsigned long vmaddr);
> > +extern void local_flush_tlb_page_psize(struct mm_struct *mm,
> > + unsigned long vmaddr, int
> > psize);
>
> That function doesn't seem to be defined anywhere. Is this prototype
> just to make compiler happy ? If so a static inline with a BUILD_BUG
> would likely be better, it would allow detection of a build problem
> at
> compile time instead of link time.
I think I just missed the implementation. It's easy to implement though
based on local_flush_tlb_page in mm/nohash/tlb.c, so I'll just add it
void local_flush_tlb_page_psize(struct mm_struct *mm,
unsigned long vmaddr, int psize)
{
__local_flush_tlb_page(mm, vmaddr, mmu_get_tsize(psize), 0);
}
EXPORT_SYMBOL(local_flush_tlb_page_psize);
>
> By the way, 'extern' keyword is pointless and deprecated for
> functions
> prototypes, please don't add new ones, even if other historical
> prototypes have one.
This and the above commented parts match the style of the surrounding
implementations. For example,
static inline void local_flush_tlb_mm(struct mm_struct *mm)
{
if (radix_enabled())
return radix__local_flush_tlb_mm(mm);
return hash__local_flush_tlb_mm(mm);
}
I am not going to add code that is inconsistent with the surrounding
code. That just causes confusion later down the line when readers
wonder why this function is special compared to the others. If it needs
to use modern style, then I would be happy to include a patch that
modernises the surrounding code first.
Though why the hash__* functions are empty I'm not sure... If they were
not implemented I would have expected a BUILD_BUG(). If they are
unnecessary due to hash itself, it's odd that they exist (as you point
out for the new one).
More information about the Linuxppc-dev
mailing list