| Message ID | 20210422054323.150993-6-aneesh.kumar@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Speedup mremap on ppc64 | expand |
On Thu, Apr 22, 2021 at 11:13:19AM +0530, Aneesh Kumar K.V wrote: > No functional change in this patch > > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> > --- > .../include/asm/book3s/64/tlbflush-radix.h | 19 +++++++----- > arch/powerpc/include/asm/book3s/64/tlbflush.h | 23 ++++++++++++--- > arch/powerpc/mm/book3s64/radix_hugetlbpage.c | 4 +-- > arch/powerpc/mm/book3s64/radix_tlb.c | 29 +++++++------------ > 4 files changed, 42 insertions(+), 33 deletions(-) > > diff --git a/arch/powerpc/include/asm/book3s/64/tlbflush-radix.h b/arch/powerpc/include/asm/book3s/64/tlbflush-radix.h > index 8b33601cdb9d..171441a43b35 100644 > --- a/arch/powerpc/include/asm/book3s/64/tlbflush-radix.h > +++ b/arch/powerpc/include/asm/book3s/64/tlbflush-radix.h > @@ -56,15 +56,18 @@ static inline void radix__flush_all_lpid_guest(unsigned int lpid) > } > #endif > > -extern void radix__flush_hugetlb_tlb_range(struct vm_area_struct *vma, > - unsigned long start, unsigned long end); > -extern void radix__flush_tlb_range_psize(struct mm_struct *mm, unsigned long start, > - unsigned long end, int psize); > -extern void radix__flush_pmd_tlb_range(struct vm_area_struct *vma, > - unsigned long start, unsigned long end); > -extern void radix__flush_tlb_range(struct vm_area_struct *vma, unsigned long start, > +void radix__flush_hugetlb_tlb_range(struct vm_area_struct *vma, > + unsigned long start, unsigned long end, > + bool flush_pwc); > +void radix__flush_pmd_tlb_range(struct vm_area_struct *vma, > + unsigned long start, unsigned long end, > + bool flush_pwc); > +void radix__flush_tlb_pwc_range_psize(struct mm_struct *mm, unsigned long start, > + unsigned long end, int psize, bool flush_pwc); > +void radix__flush_tlb_range(struct vm_area_struct *vma, unsigned long start, > unsigned long end); > -extern void radix__flush_tlb_kernel_range(unsigned long start, unsigned long end); > +void radix__flush_tlb_kernel_range(unsigned long start, unsigned long end); > + > > extern void radix__local_flush_tlb_mm(struct mm_struct *mm); > extern void radix__local_flush_all_mm(struct mm_struct *mm); > diff --git a/arch/powerpc/include/asm/book3s/64/tlbflush.h b/arch/powerpc/include/asm/book3s/64/tlbflush.h > index 215973b4cb26..f9f8a3a264f7 100644 > --- a/arch/powerpc/include/asm/book3s/64/tlbflush.h > +++ b/arch/powerpc/include/asm/book3s/64/tlbflush.h > @@ -45,13 +45,30 @@ static inline void tlbiel_all_lpid(bool radix) > hash__tlbiel_all(TLB_INVAL_SCOPE_LPID); > } > > +static inline void flush_pmd_tlb_pwc_range(struct vm_area_struct *vma, ^^^^ > + unsigned long start, > + unsigned long end, > + bool flush_pwc) > +{ > + if (radix_enabled()) > + return radix__flush_pmd_tlb_range(vma, start, end, flush_pwc); > + return hash__flush_tlb_range(vma, start, end); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > +} > > #define __HAVE_ARCH_FLUSH_PMD_TLB_RANGE > static inline void flush_pmd_tlb_range(struct vm_area_struct *vma, ^^^^ > unsigned long start, unsigned long end) > +{ > + return flush_pmd_tlb_pwc_range(vma, start, end, false); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Doesn't that cause build warnings/errors all over the place ? Guenter
On Sat, 15 May 2021 09:35:25 -0700 Guenter Roeck <linux@roeck-us.net> wrote: > > > > #define __HAVE_ARCH_FLUSH_PMD_TLB_RANGE > > static inline void flush_pmd_tlb_range(struct vm_area_struct *vma, > ^^^^ > > unsigned long start, unsigned long end) > > +{ > > + return flush_pmd_tlb_pwc_range(vma, start, end, false); > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > Doesn't that cause build warnings/errors all over the place ? It will, thanks. I queued a fix.
On 5/15/21 1:41 PM, Andrew Morton wrote: > On Sat, 15 May 2021 09:35:25 -0700 Guenter Roeck <linux@roeck-us.net> wrote: > >>> >>> #define __HAVE_ARCH_FLUSH_PMD_TLB_RANGE >>> static inline void flush_pmd_tlb_range(struct vm_area_struct *vma, >> ^^^^ >>> unsigned long start, unsigned long end) >>> +{ >>> + return flush_pmd_tlb_pwc_range(vma, start, end, false); >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >> >> Doesn't that cause build warnings/errors all over the place ? > > It will, thanks. I queued a fix. > Also in mm/mremap.c, in case you didn't see it: #ifndef flush_pte_tlb_pwc_range #define flush_pte_tlb_pwc_range flush_pte_tlb_pwc_range static inline void flush_pte_tlb_pwc_range(struct vm_area_struct *vma, ^^^^ unsigned long start, unsigned long end) { return flush_tlb_range(vma, start, end); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ }
On 5/15/21 10:05 PM, Guenter Roeck wrote: > On Thu, Apr 22, 2021 at 11:13:19AM +0530, Aneesh Kumar K.V wrote: >> No functional change in this patch >> >> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> >> --- >> .../include/asm/book3s/64/tlbflush-radix.h | 19 +++++++----- >> arch/powerpc/include/asm/book3s/64/tlbflush.h | 23 ++++++++++++--- >> arch/powerpc/mm/book3s64/radix_hugetlbpage.c | 4 +-- >> arch/powerpc/mm/book3s64/radix_tlb.c | 29 +++++++------------ >> 4 files changed, 42 insertions(+), 33 deletions(-) >> >> diff --git a/arch/powerpc/include/asm/book3s/64/tlbflush-radix.h b/arch/powerpc/include/asm/book3s/64/tlbflush-radix.h >> index 8b33601cdb9d..171441a43b35 100644 >> --- a/arch/powerpc/include/asm/book3s/64/tlbflush-radix.h >> +++ b/arch/powerpc/include/asm/book3s/64/tlbflush-radix.h >> @@ -56,15 +56,18 @@ static inline void radix__flush_all_lpid_guest(unsigned int lpid) >> } >> #endif >> >> -extern void radix__flush_hugetlb_tlb_range(struct vm_area_struct *vma, >> - unsigned long start, unsigned long end); >> -extern void radix__flush_tlb_range_psize(struct mm_struct *mm, unsigned long start, >> - unsigned long end, int psize); >> -extern void radix__flush_pmd_tlb_range(struct vm_area_struct *vma, >> - unsigned long start, unsigned long end); >> -extern void radix__flush_tlb_range(struct vm_area_struct *vma, unsigned long start, >> +void radix__flush_hugetlb_tlb_range(struct vm_area_struct *vma, >> + unsigned long start, unsigned long end, >> + bool flush_pwc); >> +void radix__flush_pmd_tlb_range(struct vm_area_struct *vma, >> + unsigned long start, unsigned long end, >> + bool flush_pwc); >> +void radix__flush_tlb_pwc_range_psize(struct mm_struct *mm, unsigned long start, >> + unsigned long end, int psize, bool flush_pwc); >> +void radix__flush_tlb_range(struct vm_area_struct *vma, unsigned long start, >> unsigned long end); >> -extern void radix__flush_tlb_kernel_range(unsigned long start, unsigned long end); >> +void radix__flush_tlb_kernel_range(unsigned long start, unsigned long end); >> + >> >> extern void radix__local_flush_tlb_mm(struct mm_struct *mm); >> extern void radix__local_flush_all_mm(struct mm_struct *mm); >> diff --git a/arch/powerpc/include/asm/book3s/64/tlbflush.h b/arch/powerpc/include/asm/book3s/64/tlbflush.h >> index 215973b4cb26..f9f8a3a264f7 100644 >> --- a/arch/powerpc/include/asm/book3s/64/tlbflush.h >> +++ b/arch/powerpc/include/asm/book3s/64/tlbflush.h >> @@ -45,13 +45,30 @@ static inline void tlbiel_all_lpid(bool radix) >> hash__tlbiel_all(TLB_INVAL_SCOPE_LPID); >> } >> >> +static inline void flush_pmd_tlb_pwc_range(struct vm_area_struct *vma, > ^^^^ >> + unsigned long start, >> + unsigned long end, >> + bool flush_pwc) >> +{ >> + if (radix_enabled()) >> + return radix__flush_pmd_tlb_range(vma, start, end, flush_pwc); >> + return hash__flush_tlb_range(vma, start, end); > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > >> +} In this specific case we won't have build errors because, static inline void hash__flush_tlb_range(struct vm_area_struct *vma, unsigned long start, unsigned long end) { But I agree the below is better to read. static inline void flush_pmd_tlb_pwc_range(struct vm_area_struct *vma, unsigned long start, unsigned long end, bool flush_pwc) { if (radix_enabled()) radix__flush_pmd_tlb_range(vma, start, end, flush_pwc); else hash__flush_tlb_range(vma, start, end); return } > >> >> #define __HAVE_ARCH_FLUSH_PMD_TLB_RANGE >> static inline void flush_pmd_tlb_range(struct vm_area_struct *vma, > ^^^^ >> unsigned long start, unsigned long end) >> +{ >> + return flush_pmd_tlb_pwc_range(vma, start, end, false); > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > Doesn't that cause build warnings/errors all over the place ? > > Guenter > -aneesh
On 5/17/21 1:40 AM, Aneesh Kumar K.V wrote: > On 5/15/21 10:05 PM, Guenter Roeck wrote: >> On Thu, Apr 22, 2021 at 11:13:19AM +0530, Aneesh Kumar K.V wrote: >>> No functional change in this patch >>> >>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> >>> --- >>> .../include/asm/book3s/64/tlbflush-radix.h | 19 +++++++----- >>> arch/powerpc/include/asm/book3s/64/tlbflush.h | 23 ++++++++++++--- >>> arch/powerpc/mm/book3s64/radix_hugetlbpage.c | 4 +-- >>> arch/powerpc/mm/book3s64/radix_tlb.c | 29 +++++++------------ >>> 4 files changed, 42 insertions(+), 33 deletions(-) >>> >>> diff --git a/arch/powerpc/include/asm/book3s/64/tlbflush-radix.h b/arch/powerpc/include/asm/book3s/64/tlbflush-radix.h >>> index 8b33601cdb9d..171441a43b35 100644 >>> --- a/arch/powerpc/include/asm/book3s/64/tlbflush-radix.h >>> +++ b/arch/powerpc/include/asm/book3s/64/tlbflush-radix.h >>> @@ -56,15 +56,18 @@ static inline void radix__flush_all_lpid_guest(unsigned int lpid) >>> } >>> #endif >>> -extern void radix__flush_hugetlb_tlb_range(struct vm_area_struct *vma, >>> - unsigned long start, unsigned long end); >>> -extern void radix__flush_tlb_range_psize(struct mm_struct *mm, unsigned long start, >>> - unsigned long end, int psize); >>> -extern void radix__flush_pmd_tlb_range(struct vm_area_struct *vma, >>> - unsigned long start, unsigned long end); >>> -extern void radix__flush_tlb_range(struct vm_area_struct *vma, unsigned long start, >>> +void radix__flush_hugetlb_tlb_range(struct vm_area_struct *vma, >>> + unsigned long start, unsigned long end, >>> + bool flush_pwc); >>> +void radix__flush_pmd_tlb_range(struct vm_area_struct *vma, >>> + unsigned long start, unsigned long end, >>> + bool flush_pwc); >>> +void radix__flush_tlb_pwc_range_psize(struct mm_struct *mm, unsigned long start, >>> + unsigned long end, int psize, bool flush_pwc); >>> +void radix__flush_tlb_range(struct vm_area_struct *vma, unsigned long start, >>> unsigned long end); >>> -extern void radix__flush_tlb_kernel_range(unsigned long start, unsigned long end); >>> +void radix__flush_tlb_kernel_range(unsigned long start, unsigned long end); >>> + >>> extern void radix__local_flush_tlb_mm(struct mm_struct *mm); >>> extern void radix__local_flush_all_mm(struct mm_struct *mm); >>> diff --git a/arch/powerpc/include/asm/book3s/64/tlbflush.h b/arch/powerpc/include/asm/book3s/64/tlbflush.h >>> index 215973b4cb26..f9f8a3a264f7 100644 >>> --- a/arch/powerpc/include/asm/book3s/64/tlbflush.h >>> +++ b/arch/powerpc/include/asm/book3s/64/tlbflush.h >>> @@ -45,13 +45,30 @@ static inline void tlbiel_all_lpid(bool radix) >>> hash__tlbiel_all(TLB_INVAL_SCOPE_LPID); >>> } >>> +static inline void flush_pmd_tlb_pwc_range(struct vm_area_struct *vma, >> ^^^^ >>> + unsigned long start, >>> + unsigned long end, >>> + bool flush_pwc) >>> +{ >>> + if (radix_enabled()) >>> + return radix__flush_pmd_tlb_range(vma, start, end, flush_pwc); >>> + return hash__flush_tlb_range(vma, start, end); >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >> >>> +} > > In this specific case we won't have build errors because, > > static inline void hash__flush_tlb_range(struct vm_area_struct *vma, > unsigned long start, unsigned long end) > { > Sorry, you completely lost me. Building parisc:allnoconfig ... failed -------------- Error log: In file included from arch/parisc/include/asm/cacheflush.h:7, from include/linux/highmem.h:12, from include/linux/pagemap.h:11, from include/linux/ksm.h:13, from mm/mremap.c:14: mm/mremap.c: In function 'flush_pte_tlb_pwc_range': arch/parisc/include/asm/tlbflush.h:20:2: error: 'return' with a value, in function returning void Guenter > > But I agree the below is better to read. > > static inline void flush_pmd_tlb_pwc_range(struct vm_area_struct *vma, > unsigned long start, > unsigned long end, > bool flush_pwc) > { > if (radix_enabled()) > radix__flush_pmd_tlb_range(vma, start, end, flush_pwc); > else > hash__flush_tlb_range(vma, start, end); > return > } > >> >>> #define __HAVE_ARCH_FLUSH_PMD_TLB_RANGE >>> static inline void flush_pmd_tlb_range(struct vm_area_struct *vma, >> ^^^^ >>> unsigned long start, unsigned long end) >>> +{ >>> + return flush_pmd_tlb_pwc_range(vma, start, end, false); >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >> >> Doesn't that cause build warnings/errors all over the place ? >> >> Guenter >> > > > -aneesh
Guenter Roeck <linux@roeck-us.net> writes: > On 5/17/21 1:40 AM, Aneesh Kumar K.V wrote: >> On 5/15/21 10:05 PM, Guenter Roeck wrote: >>> On Thu, Apr 22, 2021 at 11:13:19AM +0530, Aneesh Kumar K.V wrote: ... >>> extern void radix__local_flush_all_mm(struct mm_struct *mm); >>>> diff --git a/arch/powerpc/include/asm/book3s/64/tlbflush.h b/arch/powerpc/include/asm/book3s/64/tlbflush.h >>>> index 215973b4cb26..f9f8a3a264f7 100644 >>>> --- a/arch/powerpc/include/asm/book3s/64/tlbflush.h >>>> +++ b/arch/powerpc/include/asm/book3s/64/tlbflush.h >>>> @@ -45,13 +45,30 @@ static inline void tlbiel_all_lpid(bool radix) >>>> hash__tlbiel_all(TLB_INVAL_SCOPE_LPID); >>>> } >>>> +static inline void flush_pmd_tlb_pwc_range(struct vm_area_struct *vma, >>> ^^^^ >>>> + unsigned long start, >>>> + unsigned long end, >>>> + bool flush_pwc) >>>> +{ >>>> + if (radix_enabled()) >>>> + return radix__flush_pmd_tlb_range(vma, start, end, flush_pwc); >>>> + return hash__flush_tlb_range(vma, start, end); >>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >>> >>>> +} >> >> In this specific case we won't have build errors because, >> >> static inline void hash__flush_tlb_range(struct vm_area_struct *vma, >> unsigned long start, unsigned long end) >> { >> > > Sorry, you completely lost me. > > Building parisc:allnoconfig ... failed > -------------- > Error log: > In file included from arch/parisc/include/asm/cacheflush.h:7, > from include/linux/highmem.h:12, > from include/linux/pagemap.h:11, > from include/linux/ksm.h:13, > from mm/mremap.c:14: > mm/mremap.c: In function 'flush_pte_tlb_pwc_range': > arch/parisc/include/asm/tlbflush.h:20:2: error: 'return' with a value, in function returning void As replied here https://lore.kernel.org/mm-commits/8eedb441-a612-1ec8-8bf7-b40184de9f6f@linux.ibm.com/ That was the generic header change in the patch. I was commenting about the ppc64 specific change causing build failures. -aneesh
On 5/17/21 6:55 AM, Aneesh Kumar K.V wrote: > Guenter Roeck <linux@roeck-us.net> writes: > >> On 5/17/21 1:40 AM, Aneesh Kumar K.V wrote: >>> On 5/15/21 10:05 PM, Guenter Roeck wrote: >>>> On Thu, Apr 22, 2021 at 11:13:19AM +0530, Aneesh Kumar K.V wrote: > > ... > >>>> extern void radix__local_flush_all_mm(struct mm_struct *mm); >>>>> diff --git a/arch/powerpc/include/asm/book3s/64/tlbflush.h b/arch/powerpc/include/asm/book3s/64/tlbflush.h >>>>> index 215973b4cb26..f9f8a3a264f7 100644 >>>>> --- a/arch/powerpc/include/asm/book3s/64/tlbflush.h >>>>> +++ b/arch/powerpc/include/asm/book3s/64/tlbflush.h >>>>> @@ -45,13 +45,30 @@ static inline void tlbiel_all_lpid(bool radix) >>>>> hash__tlbiel_all(TLB_INVAL_SCOPE_LPID); >>>>> } >>>>> +static inline void flush_pmd_tlb_pwc_range(struct vm_area_struct *vma, >>>> ^^^^ >>>>> + unsigned long start, >>>>> + unsigned long end, >>>>> + bool flush_pwc) >>>>> +{ >>>>> + if (radix_enabled()) >>>>> + return radix__flush_pmd_tlb_range(vma, start, end, flush_pwc); >>>>> + return hash__flush_tlb_range(vma, start, end); >>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >>>> >>>>> +} >>> >>> In this specific case we won't have build errors because, >>> >>> static inline void hash__flush_tlb_range(struct vm_area_struct *vma, >>> unsigned long start, unsigned long end) >>> { >>> >> >> Sorry, you completely lost me. >> >> Building parisc:allnoconfig ... failed >> -------------- >> Error log: >> In file included from arch/parisc/include/asm/cacheflush.h:7, >> from include/linux/highmem.h:12, >> from include/linux/pagemap.h:11, >> from include/linux/ksm.h:13, >> from mm/mremap.c:14: >> mm/mremap.c: In function 'flush_pte_tlb_pwc_range': >> arch/parisc/include/asm/tlbflush.h:20:2: error: 'return' with a value, in function returning void > > As replied here > https://lore.kernel.org/mm-commits/8eedb441-a612-1ec8-8bf7-b40184de9f6f@linux.ibm.com/ > > That was the generic header change in the patch. I was commenting about the > ppc64 specific change causing build failures. > Ah, sorry. I wasn't aware that the following is valid C code void f1() { return f2(); ^^^^^^ } as long as f2() is void as well. Confusing, but we live and learn. Guenter
Guenter Roeck <linux@roeck-us.net> writes: > On 5/17/21 6:55 AM, Aneesh Kumar K.V wrote: >> Guenter Roeck <linux@roeck-us.net> writes: >> >>> On 5/17/21 1:40 AM, Aneesh Kumar K.V wrote: >>>> On 5/15/21 10:05 PM, Guenter Roeck wrote: >>>>> On Thu, Apr 22, 2021 at 11:13:19AM +0530, Aneesh Kumar K.V wrote: >> >> ... >> >>>>> extern void radix__local_flush_all_mm(struct mm_struct *mm); >>>>>> diff --git a/arch/powerpc/include/asm/book3s/64/tlbflush.h b/arch/powerpc/include/asm/book3s/64/tlbflush.h >>>>>> index 215973b4cb26..f9f8a3a264f7 100644 >>>>>> --- a/arch/powerpc/include/asm/book3s/64/tlbflush.h >>>>>> +++ b/arch/powerpc/include/asm/book3s/64/tlbflush.h >>>>>> @@ -45,13 +45,30 @@ static inline void tlbiel_all_lpid(bool radix) >>>>>> hash__tlbiel_all(TLB_INVAL_SCOPE_LPID); >>>>>> } >>>>>> +static inline void flush_pmd_tlb_pwc_range(struct vm_area_struct *vma, >>>>> ^^^^ >>>>>> + unsigned long start, >>>>>> + unsigned long end, >>>>>> + bool flush_pwc) >>>>>> +{ >>>>>> + if (radix_enabled()) >>>>>> + return radix__flush_pmd_tlb_range(vma, start, end, flush_pwc); >>>>>> + return hash__flush_tlb_range(vma, start, end); >>>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >>>>> >>>>>> +} >>>> >>>> In this specific case we won't have build errors because, >>>> >>>> static inline void hash__flush_tlb_range(struct vm_area_struct *vma, >>>> unsigned long start, unsigned long end) >>>> { >>>> >>> >>> Sorry, you completely lost me. >>> >>> Building parisc:allnoconfig ... failed >>> -------------- >>> Error log: >>> In file included from arch/parisc/include/asm/cacheflush.h:7, >>> from include/linux/highmem.h:12, >>> from include/linux/pagemap.h:11, >>> from include/linux/ksm.h:13, >>> from mm/mremap.c:14: >>> mm/mremap.c: In function 'flush_pte_tlb_pwc_range': >>> arch/parisc/include/asm/tlbflush.h:20:2: error: 'return' with a value, in function returning void >> >> As replied here >> https://lore.kernel.org/mm-commits/8eedb441-a612-1ec8-8bf7-b40184de9f6f@linux.ibm.com/ >> >> That was the generic header change in the patch. I was commenting about the >> ppc64 specific change causing build failures. > > Ah, sorry. I wasn't aware that the following is valid C code > > void f1() > { > return f2(); > ^^^^^^ > } > > as long as f2() is void as well. Confusing, but we live and learn. It might be valid, but it's still bad IMHO. It's confusing to readers, and serves no useful purpose. cheers
On Wed, May 19, 2021 at 10:26:22AM +1000, Michael Ellerman wrote: > Guenter Roeck <linux@roeck-us.net> writes: > > Ah, sorry. I wasn't aware that the following is valid C code > > > > void f1() > > { > > return f2(); > > ^^^^^^ > > } > > > > as long as f2() is void as well. Confusing, but we live and learn. > > It might be valid, but it's still bad IMHO. > > It's confusing to readers, and serves no useful purpose. And it actually explicitly is undefined behaviour in C90 already (3.6.6.4 in C90, 6.8.6.4 in C99 and later). Segher
On 5/18/21 5:26 PM, Michael Ellerman wrote: [ ... ] >>> That was the generic header change in the patch. I was commenting about the >>> ppc64 specific change causing build failures. >> >> Ah, sorry. I wasn't aware that the following is valid C code >> >> void f1() >> { >> return f2(); >> ^^^^^^ >> } >> >> as long as f2() is void as well. Confusing, but we live and learn. > > It might be valid, but it's still bad IMHO. > > It's confusing to readers, and serves no useful purpose. > Agreed, but it is surprisingly wide-spread. Try to run the coccinelle script below, just for fun. The script doesn't even catch instances in include files, yet there are more than 450 hits. Guenter --- virtual report @d@ identifier f; expression e; position p; @@ void f(...) { <... return e@p; ...> } @script:python depends on report@ f << d.f; p << d.p; @@ print "void function %s:%s() with non-void return in line %s" % (p[0].file, f, p[0].line)
On Tue, May 18, 2021 at 07:45:14PM -0500, Segher Boessenkool wrote: > On Wed, May 19, 2021 at 10:26:22AM +1000, Michael Ellerman wrote: > > Guenter Roeck <linux@roeck-us.net> writes: > > > Ah, sorry. I wasn't aware that the following is valid C code > > > > > > void f1() > > > { > > > return f2(); > > > ^^^^^^ > > > } > > > > > > as long as f2() is void as well. Confusing, but we live and learn. > > > > It might be valid, but it's still bad IMHO. > > > > It's confusing to readers, and serves no useful purpose. > > And it actually explicitly is undefined behaviour in C90 already > (3.6.6.4 in C90, 6.8.6.4 in C99 and later). ... but there is a GCC extension that allows this by default: <https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wreturn-type> For C only, warn about a 'return' statement with an expression in a function whose return type is 'void', unless the expression type is also 'void'. As a GNU extension, the latter case is accepted without a warning unless '-Wpedantic' is used. Segher
On 5/19/21 5:03 AM, Segher Boessenkool wrote: > On Tue, May 18, 2021 at 07:45:14PM -0500, Segher Boessenkool wrote: >> On Wed, May 19, 2021 at 10:26:22AM +1000, Michael Ellerman wrote: >>> Guenter Roeck <linux@roeck-us.net> writes: >>>> Ah, sorry. I wasn't aware that the following is valid C code >>>> >>>> void f1() >>>> { >>>> return f2(); >>>> ^^^^^^ >>>> } >>>> >>>> as long as f2() is void as well. Confusing, but we live and learn. >>> >>> It might be valid, but it's still bad IMHO. >>> >>> It's confusing to readers, and serves no useful purpose. >> >> And it actually explicitly is undefined behaviour in C90 already >> (3.6.6.4 in C90, 6.8.6.4 in C99 and later). > > ... but there is a GCC extension that allows this by default: > <https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wreturn-type> > For C only, warn about a 'return' statement with an expression in a > function whose return type is 'void', unless the expression type is > also 'void'. As a GNU extension, the latter case is accepted > without a warning unless '-Wpedantic' is used. > In C99: "6.8.6.4 The return statement Constraints A return statement with an expression shall not appear in a function whose return type is void. A return statement without an expression shall only appear in a function whose return type is void." Sounds like invalid to me, not just undefined behavior. Guenter
On Wed, May 19, 2021 at 06:37:44AM -0700, Guenter Roeck wrote: > On 5/19/21 5:03 AM, Segher Boessenkool wrote: > >On Tue, May 18, 2021 at 07:45:14PM -0500, Segher Boessenkool wrote: > >>And it actually explicitly is undefined behaviour in C90 already > >>(3.6.6.4 in C90, 6.8.6.4 in C99 and later). > > > >... but there is a GCC extension that allows this by default: > ><https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wreturn-type> > > For C only, warn about a 'return' statement with an expression in a > > function whose return type is 'void', unless the expression type is > > also 'void'. As a GNU extension, the latter case is accepted > > without a warning unless '-Wpedantic' is used. > > In C99: > > "6.8.6.4 The return statement > Constraints > > A return statement with an expression shall not appear in a function whose > return type > is void. A return statement without an expression shall only appear in a > function > whose return type is void." > > Sounds like invalid to me, not just undefined behavior. I don't know what "invalid" would mean here other than UB, it isn't a specific defined term, unlike the latter, which is precisely defined in 3.4.3/1: undefined behavior behavior, upon use of a nonportable or erroneous program construct or of erroneous data, for which this International Standard imposes no requirements This is the strongest thing the standard can say, it is not Law, it does not prohibit anyone from doing anything :-) "Shall" and "shall not" X means it is undefined behaviour if X (or its inverse) is violated. See 4.2: If a ''shall'' or ''shall not'' requirement that appears outside of a constraint or runtime-constraint is violated, the behavior is undefined. Undefined behavior is otherwise indicated in this International Standard by the words ''undefined behavior'' or by the omission of any explicit definition of behavior. There is no difference in emphasis among these three; they all describe ''behavior that is undefined''. which also explains that what you call "invalid" has undefined behaviour just as well, most likely. Segher
On Wed, May 19, 2021 at 09:20:38AM -0500, Segher Boessenkool wrote: > On Wed, May 19, 2021 at 06:37:44AM -0700, Guenter Roeck wrote: > > On 5/19/21 5:03 AM, Segher Boessenkool wrote: > > >On Tue, May 18, 2021 at 07:45:14PM -0500, Segher Boessenkool wrote: > > >>And it actually explicitly is undefined behaviour in C90 already > > >>(3.6.6.4 in C90, 6.8.6.4 in C99 and later). > > > > > >... but there is a GCC extension that allows this by default: > > ><https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wreturn-type> > > > For C only, warn about a 'return' statement with an expression in a > > > function whose return type is 'void', unless the expression type is > > > also 'void'. As a GNU extension, the latter case is accepted > > > without a warning unless '-Wpedantic' is used. > > > > In C99: > > > > "6.8.6.4 The return statement > > Constraints > > > > A return statement with an expression shall not appear in a function whose > > return type > > is void. A return statement without an expression shall only appear in a > > function > > whose return type is void." > > > > Sounds like invalid to me, not just undefined behavior. > > I don't know what "invalid" would mean here other than UB, it isn't a > specific defined term, unlike the latter, which is precisely defined in > 3.4.3/1: > undefined behavior > behavior, upon use of a nonportable or erroneous program construct or > of erroneous data, for which this International Standard imposes no > requirements > > This is the strongest thing the standard can say, it is not Law, it does > not prohibit anyone from doing anything :-) > > "Shall" and "shall not" X means it is undefined behaviour if X (or its > inverse) is violated. See 4.2: > If a ''shall'' or ''shall not'' requirement that appears outside of a > constraint or runtime-constraint is violated, the behavior is > undefined. Undefined behavior is otherwise indicated in this > International Standard by the words ''undefined behavior'' or by the > omission of any explicit definition of behavior. There is no > difference in emphasis among these three; they all describe ''behavior > that is undefined''. > which also explains that what you call "invalid" has undefined behaviour > just as well, most likely. > I'd have assumed that "shall not" is syntactically wrong, but I stand corrected. Guenter
Segher Boessenkool <segher@kernel.crashing.org> writes: > On Tue, May 18, 2021 at 07:45:14PM -0500, Segher Boessenkool wrote: >> On Wed, May 19, 2021 at 10:26:22AM +1000, Michael Ellerman wrote: >> > Guenter Roeck <linux@roeck-us.net> writes: >> > > Ah, sorry. I wasn't aware that the following is valid C code >> > > >> > > void f1() >> > > { >> > > return f2(); >> > > ^^^^^^ >> > > } >> > > >> > > as long as f2() is void as well. Confusing, but we live and learn. >> > >> > It might be valid, but it's still bad IMHO. >> > >> > It's confusing to readers, and serves no useful purpose. >> >> And it actually explicitly is undefined behaviour in C90 already >> (3.6.6.4 in C90, 6.8.6.4 in C99 and later). We use gnu89, which presumably does not make it UB. > ... but there is a GCC extension that allows this by default: > <https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wreturn-type> > For C only, warn about a 'return' statement with an expression in a > function whose return type is 'void', unless the expression type is > also 'void'. As a GNU extension, the latter case is accepted > without a warning unless '-Wpedantic' is used. There's no chance we'll ever enable -Wpedantic, so I guess it's allowed for practical purposes. I guess clang must accept it too or we'd be seeing warnings from it. cheers
Guenter Roeck <linux@roeck-us.net> writes: > On 5/18/21 5:26 PM, Michael Ellerman wrote: > [ ... ] >>>> That was the generic header change in the patch. I was commenting about the >>>> ppc64 specific change causing build failures. >>> >>> Ah, sorry. I wasn't aware that the following is valid C code >>> >>> void f1() >>> { >>> return f2(); >>> ^^^^^^ >>> } >>> >>> as long as f2() is void as well. Confusing, but we live and learn. >> >> It might be valid, but it's still bad IMHO. >> >> It's confusing to readers, and serves no useful purpose. >> > > Agreed, but it is surprisingly wide-spread. Try to run the coccinelle > script below, just for fun. The script doesn't even catch instances > in include files, yet there are more than 450 hits. Yikes, that is a lot. I guess they're pretty harmless, but would be nice to clean them up eventually. Why doesn't the script work for instances in headers? cheers
On 5/20/21 4:38 AM, Michael Ellerman wrote: > Guenter Roeck <linux@roeck-us.net> writes: >> On 5/18/21 5:26 PM, Michael Ellerman wrote: >> [ ... ] >>>>> That was the generic header change in the patch. I was commenting about the >>>>> ppc64 specific change causing build failures. >>>> >>>> Ah, sorry. I wasn't aware that the following is valid C code >>>> >>>> void f1() >>>> { >>>> return f2(); >>>> ^^^^^^ >>>> } >>>> >>>> as long as f2() is void as well. Confusing, but we live and learn. >>> >>> It might be valid, but it's still bad IMHO. >>> >>> It's confusing to readers, and serves no useful purpose. >>> >> >> Agreed, but it is surprisingly wide-spread. Try to run the coccinelle >> script below, just for fun. The script doesn't even catch instances >> in include files, yet there are more than 450 hits. > > Yikes, that is a lot. > > I guess they're pretty harmless, but would be nice to clean them up > eventually. > > Why doesn't the script work for instances in headers? > Ah, that is one of those Coccinelle details. No idea. I run the script with make coccicheck COCCI="return-void.cocci" MODE=report M=. Probably there is a better way to invoke it which does handle include files. Guenter
Hi! On Thu, May 20, 2021 at 05:37:20PM +1000, Michael Ellerman wrote: > Segher Boessenkool <segher@kernel.crashing.org> writes: > > On Tue, May 18, 2021 at 07:45:14PM -0500, Segher Boessenkool wrote: > >> On Wed, May 19, 2021 at 10:26:22AM +1000, Michael Ellerman wrote: > >> > Guenter Roeck <linux@roeck-us.net> writes: > >> > > Ah, sorry. I wasn't aware that the following is valid C code > >> > > > >> > > void f1() > >> > > { > >> > > return f2(); > >> > > ^^^^^^ > >> > > } > >> > > > >> > > as long as f2() is void as well. Confusing, but we live and learn. > >> > > >> > It might be valid, but it's still bad IMHO. > >> > > >> > It's confusing to readers, and serves no useful purpose. > >> > >> And it actually explicitly is undefined behaviour in C90 already > >> (3.6.6.4 in C90, 6.8.6.4 in C99 and later). > > We use gnu89, which presumably does not make it UB. Indeed. That is kind of implied by the "as a GNU extension" below, but some explicit statement would be better, yup. > > ... but there is a GCC extension that allows this by default: > > <https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wreturn-type> > > For C only, warn about a 'return' statement with an expression in a > > function whose return type is 'void', unless the expression type is > > also 'void'. As a GNU extension, the latter case is accepted > > without a warning unless '-Wpedantic' is used. > > There's no chance we'll ever enable -Wpedantic, Good, because -pedantic adds a lot of much more annoying warnings as well. I find this extension questionable (like Guenter says it is confusing and has no purpose), so the only thing it is "good" for is it causes long email threads ;-) Other than those things it is harmless though. > so I guess it's allowed > for practical purposes. I guess clang must accept it too or we'd be > seeing warnings from it. Yup. Segher
diff --git a/arch/powerpc/include/asm/book3s/64/tlbflush-radix.h b/arch/powerpc/include/asm/book3s/64/tlbflush-radix.h index 8b33601cdb9d..171441a43b35 100644 --- a/arch/powerpc/include/asm/book3s/64/tlbflush-radix.h +++ b/arch/powerpc/include/asm/book3s/64/tlbflush-radix.h @@ -56,15 +56,18 @@ static inline void radix__flush_all_lpid_guest(unsigned int lpid) } #endif -extern void radix__flush_hugetlb_tlb_range(struct vm_area_struct *vma, - unsigned long start, unsigned long end); -extern void radix__flush_tlb_range_psize(struct mm_struct *mm, unsigned long start, - unsigned long end, int psize); -extern void radix__flush_pmd_tlb_range(struct vm_area_struct *vma, - unsigned long start, unsigned long end); -extern void radix__flush_tlb_range(struct vm_area_struct *vma, unsigned long start, +void radix__flush_hugetlb_tlb_range(struct vm_area_struct *vma, + unsigned long start, unsigned long end, + bool flush_pwc); +void radix__flush_pmd_tlb_range(struct vm_area_struct *vma, + unsigned long start, unsigned long end, + bool flush_pwc); +void radix__flush_tlb_pwc_range_psize(struct mm_struct *mm, unsigned long start, + unsigned long end, int psize, bool flush_pwc); +void radix__flush_tlb_range(struct vm_area_struct *vma, unsigned long start, unsigned long end); -extern void radix__flush_tlb_kernel_range(unsigned long start, unsigned long end); +void radix__flush_tlb_kernel_range(unsigned long start, unsigned long end); + extern void radix__local_flush_tlb_mm(struct mm_struct *mm); extern void radix__local_flush_all_mm(struct mm_struct *mm); diff --git a/arch/powerpc/include/asm/book3s/64/tlbflush.h b/arch/powerpc/include/asm/book3s/64/tlbflush.h index 215973b4cb26..f9f8a3a264f7 100644 --- a/arch/powerpc/include/asm/book3s/64/tlbflush.h +++ b/arch/powerpc/include/asm/book3s/64/tlbflush.h @@ -45,13 +45,30 @@ static inline void tlbiel_all_lpid(bool radix) hash__tlbiel_all(TLB_INVAL_SCOPE_LPID); } +static inline void flush_pmd_tlb_pwc_range(struct vm_area_struct *vma, + unsigned long start, + unsigned long end, + bool flush_pwc) +{ + if (radix_enabled()) + return radix__flush_pmd_tlb_range(vma, start, end, flush_pwc); + return hash__flush_tlb_range(vma, start, end); +} #define __HAVE_ARCH_FLUSH_PMD_TLB_RANGE static inline void flush_pmd_tlb_range(struct vm_area_struct *vma, unsigned long start, unsigned long end) +{ + return flush_pmd_tlb_pwc_range(vma, start, end, false); +} + +static inline void flush_hugetlb_tlb_pwc_range(struct vm_area_struct *vma, + unsigned long start, + unsigned long end, + bool flush_pwc) { if (radix_enabled()) - return radix__flush_pmd_tlb_range(vma, start, end); + return radix__flush_hugetlb_tlb_range(vma, start, end, flush_pwc); return hash__flush_tlb_range(vma, start, end); } @@ -60,9 +77,7 @@ static inline void flush_hugetlb_tlb_range(struct vm_area_struct *vma, unsigned long start, unsigned long end) { - if (radix_enabled()) - return radix__flush_hugetlb_tlb_range(vma, start, end); - return hash__flush_tlb_range(vma, start, end); + return flush_hugetlb_tlb_pwc_range(vma, start, end, false); } static inline void flush_tlb_range(struct vm_area_struct *vma, diff --git a/arch/powerpc/mm/book3s64/radix_hugetlbpage.c b/arch/powerpc/mm/book3s64/radix_hugetlbpage.c index cb91071eef52..e62f5679b119 100644 --- a/arch/powerpc/mm/book3s64/radix_hugetlbpage.c +++ b/arch/powerpc/mm/book3s64/radix_hugetlbpage.c @@ -26,13 +26,13 @@ void radix__local_flush_hugetlb_page(struct vm_area_struct *vma, unsigned long v } void radix__flush_hugetlb_tlb_range(struct vm_area_struct *vma, unsigned long start, - unsigned long end) + unsigned long end, bool flush_pwc) { int psize; struct hstate *hstate = hstate_file(vma->vm_file); psize = hstate_get_psize(hstate); - radix__flush_tlb_range_psize(vma->vm_mm, start, end, psize); + radix__flush_tlb_pwc_range_psize(vma->vm_mm, start, end, psize, flush_pwc); } /* diff --git a/arch/powerpc/mm/book3s64/radix_tlb.c b/arch/powerpc/mm/book3s64/radix_tlb.c index 817a02ef6032..5a59e19f9e53 100644 --- a/arch/powerpc/mm/book3s64/radix_tlb.c +++ b/arch/powerpc/mm/book3s64/radix_tlb.c @@ -1090,7 +1090,7 @@ void radix__flush_tlb_range(struct vm_area_struct *vma, unsigned long start, { #ifdef CONFIG_HUGETLB_PAGE if (is_vm_hugetlb_page(vma)) - return radix__flush_hugetlb_tlb_range(vma, start, end); + return radix__flush_hugetlb_tlb_range(vma, start, end, false); #endif __radix__flush_tlb_range(vma->vm_mm, start, end); @@ -1151,9 +1151,6 @@ void radix__flush_all_lpid_guest(unsigned int lpid) _tlbie_lpid_guest(lpid, RIC_FLUSH_ALL); } -static void radix__flush_tlb_pwc_range_psize(struct mm_struct *mm, unsigned long start, - unsigned long end, int psize); - void radix__tlb_flush(struct mmu_gather *tlb) { int psize = 0; @@ -1177,10 +1174,8 @@ void radix__tlb_flush(struct mmu_gather *tlb) else radix__flush_all_mm(mm); } else { - if (!tlb->freed_tables) - radix__flush_tlb_range_psize(mm, start, end, psize); - else - radix__flush_tlb_pwc_range_psize(mm, start, end, psize); + radix__flush_tlb_pwc_range_psize(mm, start, + end, psize, tlb->freed_tables); } } @@ -1254,16 +1249,10 @@ static void __radix__flush_tlb_range_psize(struct mm_struct *mm, preempt_enable(); } -void radix__flush_tlb_range_psize(struct mm_struct *mm, unsigned long start, - unsigned long end, int psize) +void radix__flush_tlb_pwc_range_psize(struct mm_struct *mm, unsigned long start, + unsigned long end, int psize, bool flush_pwc) { - return __radix__flush_tlb_range_psize(mm, start, end, psize, false); -} - -static void radix__flush_tlb_pwc_range_psize(struct mm_struct *mm, unsigned long start, - unsigned long end, int psize) -{ - __radix__flush_tlb_range_psize(mm, start, end, psize, true); + __radix__flush_tlb_range_psize(mm, start, end, psize, flush_pwc); } #ifdef CONFIG_TRANSPARENT_HUGEPAGE @@ -1315,9 +1304,11 @@ void radix__flush_tlb_collapsed_pmd(struct mm_struct *mm, unsigned long addr) #endif /* CONFIG_TRANSPARENT_HUGEPAGE */ void radix__flush_pmd_tlb_range(struct vm_area_struct *vma, - unsigned long start, unsigned long end) + unsigned long start, unsigned long end, + bool flush_pwc) { - radix__flush_tlb_range_psize(vma->vm_mm, start, end, MMU_PAGE_2M); + __radix__flush_tlb_range_psize(vma->vm_mm, start, + end, MMU_PAGE_2M, flush_pwc); } EXPORT_SYMBOL(radix__flush_pmd_tlb_range);
No functional change in this patch Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> --- .../include/asm/book3s/64/tlbflush-radix.h | 19 +++++++----- arch/powerpc/include/asm/book3s/64/tlbflush.h | 23 ++++++++++++--- arch/powerpc/mm/book3s64/radix_hugetlbpage.c | 4 +-- arch/powerpc/mm/book3s64/radix_tlb.c | 29 +++++++------------ 4 files changed, 42 insertions(+), 33 deletions(-)