| Message ID | 20221025044409.448755-6-bgray@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Use per-CPU temporary mappings for patching on Radix MMU | expand |
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 ? > > 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@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 ? > +} > + > 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 ? > 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); +} > +} > + > 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. 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. > > extern void __local_flush_tlb_page(struct mm_struct *mm, unsigned long vmaddr, > int tsize, int ind);
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@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).
On Thu, 2022-11-03 at 11:39 +1100, Benjamin Gray wrote: > > 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. This series would be a good opportunity to clean the rest of that file up; either way, as Christophe says we should avoid adding new uses given that people have started actively trying to clean up the use of the extern keyword.
On Thu, 2022-11-03 at 11:39 +1100, Benjamin Gray wrote: > On Wed, 2022-11-02 at 09:56 +0000, Christophe Leroy wrote: > > 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). From what I can see in the history, the empty hash functions were originally introduced as 64-bit compatibility definitions for the hash MMU (which I guess doesn't require any action). These empty functions were shuffled around, then eventually prefixed with hash__* to make way for the radix variants, which are hidden behind a generic 'local_flush_tlb_mm' etc. implementation as we see today. So basically, the empty hash__* functions no longer (never, really) served a purpose once the generic wrapper was added. I'll add a patch to delete them and clean up the return voids.
On Mon Nov 7, 2022 at 4:58 PM AEST, Benjamin Gray wrote: > On Thu, 2022-11-03 at 11:39 +1100, Benjamin Gray wrote: > > On Wed, 2022-11-02 at 09:56 +0000, Christophe Leroy wrote: > > > 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). > > From what I can see in the history, the empty hash functions were > originally introduced as 64-bit compatibility definitions for the hash > MMU (which I guess doesn't require any action). Yeah the hash MMU does hash PTE update and TLB flushing in the Linux pte update APIs (which end up at hash__pte_update()). By the time Linux calls flush_tlb_xxx(), the powerpc code had already done the necessary TLB flushing. > These empty functions > were shuffled around, then eventually prefixed with hash__* to make way > for the radix variants, which are hidden behind a generic > 'local_flush_tlb_mm' etc. implementation as we see today. So basically, > the empty hash__* functions no longer (never, really) served a purpose > once the generic wrapper was added. I'll add a patch to delete them and > clean up the return voids. Yeah I think you got it - the functions had to be there pre-radix because they were required by core code, and when radix was added it probably just followed a template. Removing empty hash__ functions should be fine I think. Thanks, Nick
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"); +} + 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) +{ +} + 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); +} + 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); extern void __local_flush_tlb_page(struct mm_struct *mm, unsigned long vmaddr, int tsize, int ind);
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 anything as yet. 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@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(+)