* Re: [PATCH 2/2] Kconfig: CC_HAS_NO_PROFILE_FN_ATTR, depend on for GCOV and PGO [not found] ` <CANpmjNNK-iYXucjz7Degh1kJPF_Z_=8+2vNLtUW17x0UnfgtPg@mail.gmail.com> @ 2021-06-21 18:22 ` Nick Desaulniers 2021-06-21 18:50 ` Bill Wendling 0 siblings, 1 reply; 7+ messages in thread From: Nick Desaulniers @ 2021-06-21 18:22 UTC (permalink / raw) To: Will Deacon, Catalin Marinas, Mark Rutland, Ard Biesheuvel, Heiko Carstens, Vasily Gorbik, Christian Borntraeger, Peter Oberparleiter Cc: Kees Cook, Peter Zijlstra, Bill Wendling, Sami Tolvanen, Masahiro Yamada, Miguel Ojeda, Nathan Chancellor, Luc Van Oostenryck, Arnd Bergmann, Andrew Morton, Rasmus Villemoes, LKML, clang-built-linux, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), Borislav Petkov, Martin Liska, Jonathan Corbet, Fangrui Song, Linux Doc Mailing List, Linux Kbuild mailing list, Dmitry Vyukov, Johannes Berg, linux-toolchains, Marco Elver, Linux ARM, linux-s390 On Fri, Jun 18, 2021 at 11:23 PM Marco Elver <elver@google.com> wrote: > > On Sat, 19 Jun 2021 at 01:30, Nick Desaulniers <ndesaulniers@google.com> wrote: > > > > We don't want compiler instrumentation to touch noinstr functions, which > > are annotated with the no_profile function attribute. Add a Kconfig test > > for this and make PGO and GCOV depend on it. > > > > Cc: Masahiro Yamada <masahiroy@kernel.org> > > Cc: Peter Oberparleiter <oberpar@linux.ibm.com> > > Link: https://lore.kernel.org/lkml/YMTn9yjuemKFLbws@hirez.programming.kicks-ass.net/ > > Link: https://lore.kernel.org/lkml/YMcssV%2Fn5IBGv4f0@hirez.programming.kicks-ass.net/ > > Suggested-by: Peter Zijlstra <peterz@infradead.org> > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > > --- > > init/Kconfig | 3 +++ > > kernel/gcov/Kconfig | 1 + > > kernel/pgo/Kconfig | 3 ++- > > 3 files changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/init/Kconfig b/init/Kconfig > > index 1ea12c64e4c9..540f862b40c6 100644 > > --- a/init/Kconfig > > +++ b/init/Kconfig > > @@ -83,6 +83,9 @@ config TOOLS_SUPPORT_RELR > > config CC_HAS_ASM_INLINE > > def_bool $(success,echo 'void foo(void) { asm inline (""); }' | $(CC) -x c - -c -o /dev/null) > > > > +config CC_HAS_NO_PROFILE_FN_ATTR > > + def_bool $(success,echo '__attribute__((no_profile)) int x();' | $(CC) -x c - -c -o /dev/null -Werror) > > + > > config CONSTRUCTORS > > bool > > > > diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig > > index 58f87a3092f3..19facd4289cd 100644 > > --- a/kernel/gcov/Kconfig > > +++ b/kernel/gcov/Kconfig > > @@ -5,6 +5,7 @@ config GCOV_KERNEL > > bool "Enable gcov-based kernel profiling" > > depends on DEBUG_FS > > depends on !CC_IS_CLANG || CLANG_VERSION >= 110000 > > + depends on !X86 || (X86 && CC_HAS_NO_PROFILE_FN_ATTR) > > [+Cc Mark] > > arm64 is also starting to rely on noinstr working properly. Sure, Will, Catalin, other arm64 folks: Any thoughts on requiring GCC 7.1+/Clang 13.0+ for GCOV support? That way we can better guarantee that GCOV (and eventually, PGO) don't touch noinstr functions? If that's ok, I'll add modify the above like: + depends on !ARM64 || (ARM64 && CC_HAS_NO_PROFILE_FN_ATTR) to the above hunk in v2. Oh, looks like arch/s390 also uses noinstr. Same question applies then: + depends on !S390 || (S390 && CC_HAS_NO_PROFILE_FN_ATTR) Or, we could just do + depends on CC_HAS_NO_PROFILE_FN_ATTR Though that will penalize architectures not using noinstr, that still would like to use GCOV with versions of GCC older than 7.1. Perhaps there are no such such users, or they should consider upgrading their tools to we can stick with the simpler Kconfig? Thoughts? > > This should probably be a 'select ARCH_HAS_GCOV_PROFILE_ALL if > CC_HAS_NO_PROFILE_FN_ATTR' in the relevant arch/../Kconfig. > > Alternatively, using: > https://lkml.kernel.org/r/YMcssV/n5IBGv4f0@hirez.programming.kicks-ass.net > > But I'd probably not overcomplicate things at this point and just use > ARCH_HAS_GCOV_PROFILE_ALL, because GCOV seems to be a) rarely used, > and b) if someone decides to selectively instrument stuff like entry > code, we can just say it's user error. > > > > select CONSTRUCTORS > > default n > > help > > diff --git a/kernel/pgo/Kconfig b/kernel/pgo/Kconfig > > index d2053df1111c..26f75ac4c6c1 100644 > > --- a/kernel/pgo/Kconfig > > +++ b/kernel/pgo/Kconfig > > @@ -8,7 +8,8 @@ config PGO_CLANG > > bool "Enable clang's PGO-based kernel profiling" > > depends on DEBUG_FS > > depends on ARCH_SUPPORTS_PGO_CLANG > > - depends on CC_IS_CLANG && CLANG_VERSION >= 120000 > > + depends on CC_IS_CLANG > > + depends on CC_HAS_NO_PROFILE_FN_ATTR > > help > > This option enables clang's PGO (Profile Guided Optimization) based > > code profiling to better optimize the kernel. > > -- > > 2.32.0.288.g62a8d224e6-goog > > -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] Kconfig: CC_HAS_NO_PROFILE_FN_ATTR, depend on for GCOV and PGO 2021-06-21 18:22 ` [PATCH 2/2] Kconfig: CC_HAS_NO_PROFILE_FN_ATTR, depend on for GCOV and PGO Nick Desaulniers @ 2021-06-21 18:50 ` Bill Wendling 2021-06-21 20:43 ` Nick Desaulniers 0 siblings, 1 reply; 7+ messages in thread From: Bill Wendling @ 2021-06-21 18:50 UTC (permalink / raw) To: Nick Desaulniers Cc: Will Deacon, Catalin Marinas, Mark Rutland, Ard Biesheuvel, Heiko Carstens, Vasily Gorbik, Christian Borntraeger, Peter Oberparleiter, Kees Cook, Peter Zijlstra, Bill Wendling, Sami Tolvanen, Masahiro Yamada, Miguel Ojeda, Nathan Chancellor, Luc Van Oostenryck, Arnd Bergmann, Andrew Morton, Rasmus Villemoes, LKML, clang-built-linux, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), Borislav Petkov, Martin Liska, Jonathan Corbet, Fangrui Song, Linux Doc Mailing List, Linux Kbuild mailing list, Dmitry Vyukov, Johannes Berg, linux-toolchains, Marco Elver, Linux ARM, linux-s390 On Mon, Jun 21, 2021 at 11:22 AM Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Fri, Jun 18, 2021 at 11:23 PM Marco Elver <elver@google.com> wrote: > > > > On Sat, 19 Jun 2021 at 01:30, Nick Desaulniers <ndesaulniers@google.com> wrote: > > > > > > We don't want compiler instrumentation to touch noinstr functions, which > > > are annotated with the no_profile function attribute. Add a Kconfig test > > > for this and make PGO and GCOV depend on it. > > > > > > Cc: Masahiro Yamada <masahiroy@kernel.org> > > > Cc: Peter Oberparleiter <oberpar@linux.ibm.com> > > > Link: https://lore.kernel.org/lkml/YMTn9yjuemKFLbws@hirez.programming.kicks-ass.net/ > > > Link: https://lore.kernel.org/lkml/YMcssV%2Fn5IBGv4f0@hirez.programming.kicks-ass.net/ > > > Suggested-by: Peter Zijlstra <peterz@infradead.org> > > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > > > --- > > > init/Kconfig | 3 +++ > > > kernel/gcov/Kconfig | 1 + > > > kernel/pgo/Kconfig | 3 ++- > > > 3 files changed, 6 insertions(+), 1 deletion(-) > > > > > > diff --git a/init/Kconfig b/init/Kconfig > > > index 1ea12c64e4c9..540f862b40c6 100644 > > > --- a/init/Kconfig > > > +++ b/init/Kconfig > > > @@ -83,6 +83,9 @@ config TOOLS_SUPPORT_RELR > > > config CC_HAS_ASM_INLINE > > > def_bool $(success,echo 'void foo(void) { asm inline (""); }' | $(CC) -x c - -c -o /dev/null) > > > > > > +config CC_HAS_NO_PROFILE_FN_ATTR > > > + def_bool $(success,echo '__attribute__((no_profile)) int x();' | $(CC) -x c - -c -o /dev/null -Werror) > > > + > > > config CONSTRUCTORS > > > bool > > > > > > diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig > > > index 58f87a3092f3..19facd4289cd 100644 > > > --- a/kernel/gcov/Kconfig > > > +++ b/kernel/gcov/Kconfig > > > @@ -5,6 +5,7 @@ config GCOV_KERNEL > > > bool "Enable gcov-based kernel profiling" > > > depends on DEBUG_FS > > > depends on !CC_IS_CLANG || CLANG_VERSION >= 110000 > > > + depends on !X86 || (X86 && CC_HAS_NO_PROFILE_FN_ATTR) > > > > [+Cc Mark] > > > > arm64 is also starting to rely on noinstr working properly. > > Sure, > Will, Catalin, other arm64 folks: > Any thoughts on requiring GCC 7.1+/Clang 13.0+ for GCOV support? That > way we can better guarantee that GCOV (and eventually, PGO) don't > touch noinstr functions? > > If that's ok, I'll add modify the above like: > > + depends on !ARM64 || (ARM64 && CC_HAS_NO_PROFILE_FN_ATTR) > Wouldn't "!ARM64 || CC_HAS_NO_PROFILE_FN_ATTR" be more succinct? > to the above hunk in v2. Oh, looks like arch/s390 also uses noinstr. > Same question applies then: > > + depends on !S390 || (S390 && CC_HAS_NO_PROFILE_FN_ATTR) > > Or, we could just do > > + depends on CC_HAS_NO_PROFILE_FN_ATTR > > Though that will penalize architectures not using noinstr, that still > would like to use GCOV with versions of GCC older than 7.1. Perhaps > there are no such such users, or they should consider upgrading their > tools to we can stick with the simpler Kconfig? Thoughts? > > > > > This should probably be a 'select ARCH_HAS_GCOV_PROFILE_ALL if > > CC_HAS_NO_PROFILE_FN_ATTR' in the relevant arch/../Kconfig. > > > > Alternatively, using: > > https://lkml.kernel.org/r/YMcssV/n5IBGv4f0@hirez.programming.kicks-ass.net > > > > But I'd probably not overcomplicate things at this point and just use > > ARCH_HAS_GCOV_PROFILE_ALL, because GCOV seems to be a) rarely used, > > and b) if someone decides to selectively instrument stuff like entry > > code, we can just say it's user error. > > > > > > > select CONSTRUCTORS > > > default n > > > help > > > diff --git a/kernel/pgo/Kconfig b/kernel/pgo/Kconfig > > > index d2053df1111c..26f75ac4c6c1 100644 > > > --- a/kernel/pgo/Kconfig > > > +++ b/kernel/pgo/Kconfig > > > @@ -8,7 +8,8 @@ config PGO_CLANG > > > bool "Enable clang's PGO-based kernel profiling" > > > depends on DEBUG_FS > > > depends on ARCH_SUPPORTS_PGO_CLANG > > > - depends on CC_IS_CLANG && CLANG_VERSION >= 120000 > > > + depends on CC_IS_CLANG > > > + depends on CC_HAS_NO_PROFILE_FN_ATTR > > > help > > > This option enables clang's PGO (Profile Guided Optimization) based > > > code profiling to better optimize the kernel. > > > -- > > > 2.32.0.288.g62a8d224e6-goog > > > > > > > -- > Thanks, > ~Nick Desaulniers ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] Kconfig: CC_HAS_NO_PROFILE_FN_ATTR, depend on for GCOV and PGO 2021-06-21 18:50 ` Bill Wendling @ 2021-06-21 20:43 ` Nick Desaulniers 2021-06-21 21:15 ` Nathan Chancellor 2021-06-22 9:25 ` Catalin Marinas 0 siblings, 2 replies; 7+ messages in thread From: Nick Desaulniers @ 2021-06-21 20:43 UTC (permalink / raw) To: Bill Wendling, Masahiro Yamada Cc: Will Deacon, Catalin Marinas, Mark Rutland, Ard Biesheuvel, Heiko Carstens, Vasily Gorbik, Christian Borntraeger, Peter Oberparleiter, Kees Cook, Peter Zijlstra, Bill Wendling, Sami Tolvanen, Miguel Ojeda, Nathan Chancellor, Luc Van Oostenryck, Arnd Bergmann, Andrew Morton, Rasmus Villemoes, LKML, clang-built-linux, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), Borislav Petkov, Martin Liska, Jonathan Corbet, Fangrui Song, Linux Doc Mailing List, Linux Kbuild mailing list, Dmitry Vyukov, Johannes Berg, linux-toolchains, Marco Elver, Linux ARM, linux-s390 On Mon, Jun 21, 2021 at 11:50 AM Bill Wendling <morbo@google.com> wrote: > > On Mon, Jun 21, 2021 at 11:22 AM Nick Desaulniers > <ndesaulniers@google.com> wrote: > > > > On Fri, Jun 18, 2021 at 11:23 PM Marco Elver <elver@google.com> wrote: > > > > > > On Sat, 19 Jun 2021 at 01:30, Nick Desaulniers <ndesaulniers@google.com> wrote: > > > > > > > > We don't want compiler instrumentation to touch noinstr functions, which > > > > are annotated with the no_profile function attribute. Add a Kconfig test > > > > for this and make PGO and GCOV depend on it. > > > > > > > > Cc: Masahiro Yamada <masahiroy@kernel.org> > > > > Cc: Peter Oberparleiter <oberpar@linux.ibm.com> > > > > Link: https://lore.kernel.org/lkml/YMTn9yjuemKFLbws@hirez.programming.kicks-ass.net/ > > > > Link: https://lore.kernel.org/lkml/YMcssV%2Fn5IBGv4f0@hirez.programming.kicks-ass.net/ > > > > Suggested-by: Peter Zijlstra <peterz@infradead.org> > > > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > > > > --- > > > > init/Kconfig | 3 +++ > > > > kernel/gcov/Kconfig | 1 + > > > > kernel/pgo/Kconfig | 3 ++- > > > > 3 files changed, 6 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/init/Kconfig b/init/Kconfig > > > > index 1ea12c64e4c9..540f862b40c6 100644 > > > > --- a/init/Kconfig > > > > +++ b/init/Kconfig > > > > @@ -83,6 +83,9 @@ config TOOLS_SUPPORT_RELR > > > > config CC_HAS_ASM_INLINE > > > > def_bool $(success,echo 'void foo(void) { asm inline (""); }' | $(CC) -x c - -c -o /dev/null) > > > > > > > > +config CC_HAS_NO_PROFILE_FN_ATTR > > > > + def_bool $(success,echo '__attribute__((no_profile)) int x();' | $(CC) -x c - -c -o /dev/null -Werror) > > > > + > > > > config CONSTRUCTORS > > > > bool > > > > > > > > diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig > > > > index 58f87a3092f3..19facd4289cd 100644 > > > > --- a/kernel/gcov/Kconfig > > > > +++ b/kernel/gcov/Kconfig > > > > @@ -5,6 +5,7 @@ config GCOV_KERNEL > > > > bool "Enable gcov-based kernel profiling" > > > > depends on DEBUG_FS > > > > depends on !CC_IS_CLANG || CLANG_VERSION >= 110000 > > > > + depends on !X86 || (X86 && CC_HAS_NO_PROFILE_FN_ATTR) > > > > > > [+Cc Mark] > > > > > > arm64 is also starting to rely on noinstr working properly. > > > > Sure, > > Will, Catalin, other arm64 folks: > > Any thoughts on requiring GCC 7.1+/Clang 13.0+ for GCOV support? That > > way we can better guarantee that GCOV (and eventually, PGO) don't > > touch noinstr functions? > > > > If that's ok, I'll add modify the above like: > > > > + depends on !ARM64 || (ARM64 && CC_HAS_NO_PROFILE_FN_ATTR) > > > Wouldn't "!ARM64 || CC_HAS_NO_PROFILE_FN_ATTR" be more succinct? We need to be able to express via Kconfig "GCOV should not be enabled for architectures that use noinstr when the toolchain does not support __attribute__((no_profile_instrument_function))." Where "architectures that use noinstr" are currently arm64, s390, and x86. So I guess we could do: + depends on !ARM64 || !S390 || !X86 || CC_HAS_NO_PROFILE_FN_ATTR (We could add a Kconfig for ARCH_WANTS_NO_INSTR, which might be more informative than listed out architectures which might be non-obvious to passers-by). It would be most succinct to raise the requirements to: "GCOV should not be enabled when the toolchain does not support __attribute__((no_profile_instrument_function))." Then we could do: + depends on CC_HAS_NO_PROFILE_FN_ATTR Assuming no one has the requirement to support GCOV on PPC with GCC < 7.1, for example. > > > to the above hunk in v2. Oh, looks like arch/s390 also uses noinstr. > > Same question applies then: > > > > + depends on !S390 || (S390 && CC_HAS_NO_PROFILE_FN_ATTR) > > > > Or, we could just do > > > > + depends on CC_HAS_NO_PROFILE_FN_ATTR > > > > Though that will penalize architectures not using noinstr, that still > > would like to use GCOV with versions of GCC older than 7.1. Perhaps > > there are no such such users, or they should consider upgrading their > > tools to we can stick with the simpler Kconfig? Thoughts? > > > > > > > > This should probably be a 'select ARCH_HAS_GCOV_PROFILE_ALL if > > > CC_HAS_NO_PROFILE_FN_ATTR' in the relevant arch/../Kconfig. > > > > > > Alternatively, using: > > > https://lkml.kernel.org/r/YMcssV/n5IBGv4f0@hirez.programming.kicks-ass.net > > > > > > But I'd probably not overcomplicate things at this point and just use > > > ARCH_HAS_GCOV_PROFILE_ALL, because GCOV seems to be a) rarely used, > > > and b) if someone decides to selectively instrument stuff like entry > > > code, we can just say it's user error. > > > > > > > > > > select CONSTRUCTORS > > > > default n > > > > help > > > > diff --git a/kernel/pgo/Kconfig b/kernel/pgo/Kconfig > > > > index d2053df1111c..26f75ac4c6c1 100644 > > > > --- a/kernel/pgo/Kconfig > > > > +++ b/kernel/pgo/Kconfig > > > > @@ -8,7 +8,8 @@ config PGO_CLANG > > > > bool "Enable clang's PGO-based kernel profiling" > > > > depends on DEBUG_FS > > > > depends on ARCH_SUPPORTS_PGO_CLANG > > > > - depends on CC_IS_CLANG && CLANG_VERSION >= 120000 > > > > + depends on CC_IS_CLANG > > > > + depends on CC_HAS_NO_PROFILE_FN_ATTR > > > > help > > > > This option enables clang's PGO (Profile Guided Optimization) based > > > > code profiling to better optimize the kernel. > > > > -- > > > > 2.32.0.288.g62a8d224e6-goog > > > > > > > > > > > > -- > > Thanks, > > ~Nick Desaulniers -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] Kconfig: CC_HAS_NO_PROFILE_FN_ATTR, depend on for GCOV and PGO 2021-06-21 20:43 ` Nick Desaulniers @ 2021-06-21 21:15 ` Nathan Chancellor 2021-06-22 9:25 ` Catalin Marinas 1 sibling, 0 replies; 7+ messages in thread From: Nathan Chancellor @ 2021-06-21 21:15 UTC (permalink / raw) To: Nick Desaulniers, Bill Wendling, Masahiro Yamada Cc: Will Deacon, Catalin Marinas, Mark Rutland, Ard Biesheuvel, Heiko Carstens, Vasily Gorbik, Christian Borntraeger, Peter Oberparleiter, Kees Cook, Peter Zijlstra, Bill Wendling, Sami Tolvanen, Miguel Ojeda, Luc Van Oostenryck, Arnd Bergmann, Andrew Morton, Rasmus Villemoes, LKML, clang-built-linux, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), Borislav Petkov, Martin Liska, Jonathan Corbet, Fangrui Song, Linux Doc Mailing List, Linux Kbuild mailing list, Dmitry Vyukov, Johannes Berg, linux-toolchains, Marco Elver, Linux ARM, linux-s390 On 6/21/2021 1:43 PM, Nick Desaulniers wrote: > On Mon, Jun 21, 2021 at 11:50 AM Bill Wendling <morbo@google.com> wrote: >> >> On Mon, Jun 21, 2021 at 11:22 AM Nick Desaulniers >> <ndesaulniers@google.com> wrote: >>> >>> On Fri, Jun 18, 2021 at 11:23 PM Marco Elver <elver@google.com> wrote: >>>> >>>> On Sat, 19 Jun 2021 at 01:30, Nick Desaulniers <ndesaulniers@google.com> wrote: >>>>> >>>>> We don't want compiler instrumentation to touch noinstr functions, which >>>>> are annotated with the no_profile function attribute. Add a Kconfig test >>>>> for this and make PGO and GCOV depend on it. >>>>> >>>>> Cc: Masahiro Yamada <masahiroy@kernel.org> >>>>> Cc: Peter Oberparleiter <oberpar@linux.ibm.com> >>>>> Link: https://lore.kernel.org/lkml/YMTn9yjuemKFLbws@hirez.programming.kicks-ass.net/ >>>>> Link: https://lore.kernel.org/lkml/YMcssV%2Fn5IBGv4f0@hirez.programming.kicks-ass.net/ >>>>> Suggested-by: Peter Zijlstra <peterz@infradead.org> >>>>> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> >>>>> --- >>>>> init/Kconfig | 3 +++ >>>>> kernel/gcov/Kconfig | 1 + >>>>> kernel/pgo/Kconfig | 3 ++- >>>>> 3 files changed, 6 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/init/Kconfig b/init/Kconfig >>>>> index 1ea12c64e4c9..540f862b40c6 100644 >>>>> --- a/init/Kconfig >>>>> +++ b/init/Kconfig >>>>> @@ -83,6 +83,9 @@ config TOOLS_SUPPORT_RELR >>>>> config CC_HAS_ASM_INLINE >>>>> def_bool $(success,echo 'void foo(void) { asm inline (""); }' | $(CC) -x c - -c -o /dev/null) >>>>> >>>>> +config CC_HAS_NO_PROFILE_FN_ATTR >>>>> + def_bool $(success,echo '__attribute__((no_profile)) int x();' | $(CC) -x c - -c -o /dev/null -Werror) >>>>> + >>>>> config CONSTRUCTORS >>>>> bool >>>>> >>>>> diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig >>>>> index 58f87a3092f3..19facd4289cd 100644 >>>>> --- a/kernel/gcov/Kconfig >>>>> +++ b/kernel/gcov/Kconfig >>>>> @@ -5,6 +5,7 @@ config GCOV_KERNEL >>>>> bool "Enable gcov-based kernel profiling" >>>>> depends on DEBUG_FS >>>>> depends on !CC_IS_CLANG || CLANG_VERSION >= 110000 >>>>> + depends on !X86 || (X86 && CC_HAS_NO_PROFILE_FN_ATTR) >>>> >>>> [+Cc Mark] >>>> >>>> arm64 is also starting to rely on noinstr working properly. >>> >>> Sure, >>> Will, Catalin, other arm64 folks: >>> Any thoughts on requiring GCC 7.1+/Clang 13.0+ for GCOV support? That >>> way we can better guarantee that GCOV (and eventually, PGO) don't >>> touch noinstr functions? >>> >>> If that's ok, I'll add modify the above like: >>> >>> + depends on !ARM64 || (ARM64 && CC_HAS_NO_PROFILE_FN_ATTR) >>> >> Wouldn't "!ARM64 || CC_HAS_NO_PROFILE_FN_ATTR" be more succinct? > > We need to be able to express via Kconfig "GCOV should not be enabled > for architectures that use noinstr when the toolchain does not support > __attribute__((no_profile_instrument_function))." > > Where "architectures that use noinstr" are currently arm64, s390, and > x86. So I guess we could do: > > + depends on !ARM64 || !S390 || !X86 || CC_HAS_NO_PROFILE_FN_ATTR > > (We could add a Kconfig for ARCH_WANTS_NO_INSTR, which might be more > informative than listed out architectures which might be non-obvious > to passers-by). I agree that spelling this out might be nicer for the future, in case instances like this crop up again. ARCH_REQUIRES_NO_INSTR might be a better wording? > It would be most succinct to raise the requirements to: "GCOV should > not be enabled when the toolchain does not support > __attribute__((no_profile_instrument_function))." Then we could do: > > + depends on CC_HAS_NO_PROFILE_FN_ATTR Then this could become depends on !ARCH_REQUIRES_NO_INSTR || (ARCH_REQUIRES_NO_INSTR && CC_HAS_NO_PROFILE_FN_ATTR) (sorry for the potential wrap). Cheers, Nathan > Assuming no one has the requirement to support GCOV on PPC with GCC < > 7.1, for example. > >> >>> to the above hunk in v2. Oh, looks like arch/s390 also uses noinstr. >>> Same question applies then: >>> >>> + depends on !S390 || (S390 && CC_HAS_NO_PROFILE_FN_ATTR) >>> >>> Or, we could just do >>> >>> + depends on CC_HAS_NO_PROFILE_FN_ATTR >>> >>> Though that will penalize architectures not using noinstr, that still >>> would like to use GCOV with versions of GCC older than 7.1. Perhaps >>> there are no such such users, or they should consider upgrading their >>> tools to we can stick with the simpler Kconfig? Thoughts? >>> >>>> >>>> This should probably be a 'select ARCH_HAS_GCOV_PROFILE_ALL if >>>> CC_HAS_NO_PROFILE_FN_ATTR' in the relevant arch/../Kconfig. >>>> >>>> Alternatively, using: >>>> https://lkml.kernel.org/r/YMcssV/n5IBGv4f0@hirez.programming.kicks-ass.net >>>> >>>> But I'd probably not overcomplicate things at this point and just use >>>> ARCH_HAS_GCOV_PROFILE_ALL, because GCOV seems to be a) rarely used, >>>> and b) if someone decides to selectively instrument stuff like entry >>>> code, we can just say it's user error. >>>> >>>> >>>>> select CONSTRUCTORS >>>>> default n >>>>> help >>>>> diff --git a/kernel/pgo/Kconfig b/kernel/pgo/Kconfig >>>>> index d2053df1111c..26f75ac4c6c1 100644 >>>>> --- a/kernel/pgo/Kconfig >>>>> +++ b/kernel/pgo/Kconfig >>>>> @@ -8,7 +8,8 @@ config PGO_CLANG >>>>> bool "Enable clang's PGO-based kernel profiling" >>>>> depends on DEBUG_FS >>>>> depends on ARCH_SUPPORTS_PGO_CLANG >>>>> - depends on CC_IS_CLANG && CLANG_VERSION >= 120000 >>>>> + depends on CC_IS_CLANG >>>>> + depends on CC_HAS_NO_PROFILE_FN_ATTR >>>>> help >>>>> This option enables clang's PGO (Profile Guided Optimization) based >>>>> code profiling to better optimize the kernel. >>>>> -- >>>>> 2.32.0.288.g62a8d224e6-goog >>>>> >>> >>> >>> >>> -- >>> Thanks, >>> ~Nick Desaulniers > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] Kconfig: CC_HAS_NO_PROFILE_FN_ATTR, depend on for GCOV and PGO 2021-06-21 20:43 ` Nick Desaulniers 2021-06-21 21:15 ` Nathan Chancellor @ 2021-06-22 9:25 ` Catalin Marinas 2021-06-22 9:29 ` Mark Rutland 1 sibling, 1 reply; 7+ messages in thread From: Catalin Marinas @ 2021-06-22 9:25 UTC (permalink / raw) To: Nick Desaulniers Cc: Bill Wendling, Masahiro Yamada, Will Deacon, Mark Rutland, Ard Biesheuvel, Heiko Carstens, Vasily Gorbik, Christian Borntraeger, Peter Oberparleiter, Kees Cook, Peter Zijlstra, Bill Wendling, Sami Tolvanen, Miguel Ojeda, Nathan Chancellor, Luc Van Oostenryck, Arnd Bergmann, Andrew Morton, Rasmus Villemoes, LKML, clang-built-linux, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), Borislav Petkov, Martin Liska, Jonathan Corbet, Fangrui Song, Linux Doc Mailing List, Linux Kbuild mailing list, Dmitry Vyukov, Johannes Berg, linux-toolchains, Marco Elver, Linux ARM, linux-s390 On Mon, Jun 21, 2021 at 01:43:54PM -0700, Nick Desaulniers wrote: > On Mon, Jun 21, 2021 at 11:50 AM Bill Wendling <morbo@google.com> wrote: > > On Mon, Jun 21, 2021 at 11:22 AM Nick Desaulniers > > <ndesaulniers@google.com> wrote: > > > On Fri, Jun 18, 2021 at 11:23 PM Marco Elver <elver@google.com> wrote: > > > > On Sat, 19 Jun 2021 at 01:30, Nick Desaulniers <ndesaulniers@google.com> wrote: > > > > > We don't want compiler instrumentation to touch noinstr functions, which > > > > > are annotated with the no_profile function attribute. Add a Kconfig test > > > > > for this and make PGO and GCOV depend on it. > > > > > > > > > > Cc: Masahiro Yamada <masahiroy@kernel.org> > > > > > Cc: Peter Oberparleiter <oberpar@linux.ibm.com> > > > > > Link: https://lore.kernel.org/lkml/YMTn9yjuemKFLbws@hirez.programming.kicks-ass.net/ > > > > > Link: https://lore.kernel.org/lkml/YMcssV%2Fn5IBGv4f0@hirez.programming.kicks-ass.net/ > > > > > Suggested-by: Peter Zijlstra <peterz@infradead.org> > > > > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > > > > > --- > > > > > init/Kconfig | 3 +++ > > > > > kernel/gcov/Kconfig | 1 + > > > > > kernel/pgo/Kconfig | 3 ++- > > > > > 3 files changed, 6 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/init/Kconfig b/init/Kconfig > > > > > index 1ea12c64e4c9..540f862b40c6 100644 > > > > > --- a/init/Kconfig > > > > > +++ b/init/Kconfig > > > > > @@ -83,6 +83,9 @@ config TOOLS_SUPPORT_RELR > > > > > config CC_HAS_ASM_INLINE > > > > > def_bool $(success,echo 'void foo(void) { asm inline (""); }' | $(CC) -x c - -c -o /dev/null) > > > > > > > > > > +config CC_HAS_NO_PROFILE_FN_ATTR > > > > > + def_bool $(success,echo '__attribute__((no_profile)) int x();' | $(CC) -x c - -c -o /dev/null -Werror) > > > > > + > > > > > config CONSTRUCTORS > > > > > bool > > > > > > > > > > diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig > > > > > index 58f87a3092f3..19facd4289cd 100644 > > > > > --- a/kernel/gcov/Kconfig > > > > > +++ b/kernel/gcov/Kconfig > > > > > @@ -5,6 +5,7 @@ config GCOV_KERNEL > > > > > bool "Enable gcov-based kernel profiling" > > > > > depends on DEBUG_FS > > > > > depends on !CC_IS_CLANG || CLANG_VERSION >= 110000 > > > > > + depends on !X86 || (X86 && CC_HAS_NO_PROFILE_FN_ATTR) > > > > > > > > [+Cc Mark] > > > > > > > > arm64 is also starting to rely on noinstr working properly. > > > > > > Sure, > > > Will, Catalin, other arm64 folks: > > > Any thoughts on requiring GCC 7.1+/Clang 13.0+ for GCOV support? That > > > way we can better guarantee that GCOV (and eventually, PGO) don't > > > touch noinstr functions? > > > > > > If that's ok, I'll add modify the above like: > > > > > > + depends on !ARM64 || (ARM64 && CC_HAS_NO_PROFILE_FN_ATTR) > > > > Wouldn't "!ARM64 || CC_HAS_NO_PROFILE_FN_ATTR" be more succinct? > > We need to be able to express via Kconfig "GCOV should not be enabled > for architectures that use noinstr when the toolchain does not support > __attribute__((no_profile_instrument_function))." > > Where "architectures that use noinstr" are currently arm64, s390, and > x86. So I guess we could do: > > + depends on !ARM64 || !S390 || !X86 || CC_HAS_NO_PROFILE_FN_ATTR I think you want: depends on !(ARM64 || S390 || X86) || CC_HAS_NO_PROFILE_FN_ATTR > (We could add a Kconfig for ARCH_WANTS_NO_INSTR, which might be more > informative than listed out architectures which might be non-obvious > to passers-by). That would probably look better. -- Catalin ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] Kconfig: CC_HAS_NO_PROFILE_FN_ATTR, depend on for GCOV and PGO 2021-06-22 9:25 ` Catalin Marinas @ 2021-06-22 9:29 ` Mark Rutland 2021-06-22 9:32 ` Catalin Marinas 0 siblings, 1 reply; 7+ messages in thread From: Mark Rutland @ 2021-06-22 9:29 UTC (permalink / raw) To: Catalin Marinas Cc: Nick Desaulniers, Bill Wendling, Masahiro Yamada, Will Deacon, Ard Biesheuvel, Heiko Carstens, Vasily Gorbik, Christian Borntraeger, Peter Oberparleiter, Kees Cook, Peter Zijlstra, Bill Wendling, Sami Tolvanen, Miguel Ojeda, Nathan Chancellor, Luc Van Oostenryck, Arnd Bergmann, Andrew Morton, Rasmus Villemoes, LKML, clang-built-linux, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), Borislav Petkov, Martin Liska, Jonathan Corbet, Fangrui Song, Linux Doc Mailing List, Linux Kbuild mailing list, Dmitry Vyukov, Johannes Berg, linux-toolchains, Marco Elver, Linux ARM, linux-s390 On Tue, Jun 22, 2021 at 10:25:34AM +0100, Catalin Marinas wrote: > On Mon, Jun 21, 2021 at 01:43:54PM -0700, Nick Desaulniers wrote: > > We need to be able to express via Kconfig "GCOV should not be enabled > > for architectures that use noinstr when the toolchain does not support > > __attribute__((no_profile_instrument_function))." > > > > Where "architectures that use noinstr" are currently arm64, s390, and > > x86. So I guess we could do: > > > > + depends on !ARM64 || !S390 || !X86 || CC_HAS_NO_PROFILE_FN_ATTR > > I think you want: > > depends on !(ARM64 || S390 || X86) || CC_HAS_NO_PROFILE_FN_ATTR > > > (We could add a Kconfig for ARCH_WANTS_NO_INSTR, which might be more > > informative than listed out architectures which might be non-obvious > > to passers-by). > > That would probably look better. It does; see: https://lore.kernel.org/r/20210621231822.2848305-1-ndesaulniers@google.com :) Thanks, Mark. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] Kconfig: CC_HAS_NO_PROFILE_FN_ATTR, depend on for GCOV and PGO 2021-06-22 9:29 ` Mark Rutland @ 2021-06-22 9:32 ` Catalin Marinas 0 siblings, 0 replies; 7+ messages in thread From: Catalin Marinas @ 2021-06-22 9:32 UTC (permalink / raw) To: Mark Rutland Cc: Nick Desaulniers, Bill Wendling, Masahiro Yamada, Will Deacon, Ard Biesheuvel, Heiko Carstens, Vasily Gorbik, Christian Borntraeger, Peter Oberparleiter, Kees Cook, Peter Zijlstra, Bill Wendling, Sami Tolvanen, Miguel Ojeda, Nathan Chancellor, Luc Van Oostenryck, Arnd Bergmann, Andrew Morton, Rasmus Villemoes, LKML, clang-built-linux, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), Borislav Petkov, Martin Liska, Jonathan Corbet, Fangrui Song, Linux Doc Mailing List, Linux Kbuild mailing list, Dmitry Vyukov, Johannes Berg, linux-toolchains, Marco Elver, Linux ARM, linux-s390 On Tue, Jun 22, 2021 at 10:29:37AM +0100, Mark Rutland wrote: > On Tue, Jun 22, 2021 at 10:25:34AM +0100, Catalin Marinas wrote: > > On Mon, Jun 21, 2021 at 01:43:54PM -0700, Nick Desaulniers wrote: > > > We need to be able to express via Kconfig "GCOV should not be enabled > > > for architectures that use noinstr when the toolchain does not support > > > __attribute__((no_profile_instrument_function))." > > > > > > Where "architectures that use noinstr" are currently arm64, s390, and > > > x86. So I guess we could do: > > > > > > + depends on !ARM64 || !S390 || !X86 || CC_HAS_NO_PROFILE_FN_ATTR > > > > I think you want: > > > > depends on !(ARM64 || S390 || X86) || CC_HAS_NO_PROFILE_FN_ATTR > > > > > (We could add a Kconfig for ARCH_WANTS_NO_INSTR, which might be more > > > informative than listed out architectures which might be non-obvious > > > to passers-by). > > > > That would probably look better. > > It does; see: > > https://lore.kernel.org/r/20210621231822.2848305-1-ndesaulniers@google.com I'm getting there, eventually ;). -- Catalin ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-06-22 9:32 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20210618233023.1360185-1-ndesaulniers@google.com>
[not found] ` <20210618233023.1360185-3-ndesaulniers@google.com>
[not found] ` <CANpmjNNK-iYXucjz7Degh1kJPF_Z_=8+2vNLtUW17x0UnfgtPg@mail.gmail.com>
2021-06-21 18:22 ` [PATCH 2/2] Kconfig: CC_HAS_NO_PROFILE_FN_ATTR, depend on for GCOV and PGO Nick Desaulniers
2021-06-21 18:50 ` Bill Wendling
2021-06-21 20:43 ` Nick Desaulniers
2021-06-21 21:15 ` Nathan Chancellor
2021-06-22 9:25 ` Catalin Marinas
2021-06-22 9:29 ` Mark Rutland
2021-06-22 9:32 ` Catalin Marinas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).