When we say noinstr, we mean noinstr. GCOV and PGO can both instrument
functions. Add a new function annotation __no_profile that expands to
__attribute__((__no_profile__)) and Kconfig value
CC_HAS_NO_PROFILE_FN_ATTR.
Base is
https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=for-next/clang/pgo.
Nick Desaulniers (2):
compiler_attributes.h: define __no_profile, add to noinstr
Kconfig: CC_HAS_NO_PROFILE_FN_ATTR, depend on for GCOV and PGO
include/linux/compiler_attributes.h | 12 ++++++++++++
include/linux/compiler_types.h | 2 +-
init/Kconfig | 3 +++
kernel/gcov/Kconfig | 1 +
kernel/pgo/Kconfig | 3 ++-
5 files changed, 19 insertions(+), 2 deletions(-)
base-commit: 4356bc4c0425c81e204f561acf4dd0095544a6cb
--
2.32.0.288.g62a8d224e6-goog
On 2021-06-18, Nick Desaulniers wrote:
>When we say noinstr, we mean noinstr. GCOV and PGO can both instrument
>functions. Add a new function annotation __no_profile that expands to
>__attribute__((__no_profile__)) and Kconfig value
>CC_HAS_NO_PROFILE_FN_ATTR.
>
>Base is
>https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=for-next/clang/pgo.
>
>Nick Desaulniers (2):
> compiler_attributes.h: define __no_profile, add to noinstr
> Kconfig: CC_HAS_NO_PROFILE_FN_ATTR, depend on for GCOV and PGO
>
> include/linux/compiler_attributes.h | 12 ++++++++++++
> include/linux/compiler_types.h | 2 +-
> init/Kconfig | 3 +++
> kernel/gcov/Kconfig | 1 +
> kernel/pgo/Kconfig | 3 ++-
> 5 files changed, 19 insertions(+), 2 deletions(-)
>
>
>base-commit: 4356bc4c0425c81e204f561acf4dd0095544a6cb
>--
>2.32.0.288.g62a8d224e6-goog
>
Thanks for the attribute work in clang and kernel! Hope we can use clang
PGO in 5.14... (I am a casual contributor to clang PGO/coverage)
On Fri, Jun 18, 2021 at 04:30:21PM -0700, Nick Desaulniers wrote:
> When we say noinstr, we mean noinstr. GCOV and PGO can both instrument
> functions. Add a new function annotation __no_profile that expands to
> __attribute__((__no_profile__)) and Kconfig value
> CC_HAS_NO_PROFILE_FN_ATTR.
>
> Base is
> https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=for-next/clang/pgo.
>
> Nick Desaulniers (2):
> compiler_attributes.h: define __no_profile, add to noinstr
> Kconfig: CC_HAS_NO_PROFILE_FN_ATTR, depend on for GCOV and PGO
Oh, awesome! Thanks for the fast work on this. If there are no objections,
I'll apply this in front of the PGO series and put it in -next.
-Kees
>
> include/linux/compiler_attributes.h | 12 ++++++++++++
> include/linux/compiler_types.h | 2 +-
> init/Kconfig | 3 +++
> kernel/gcov/Kconfig | 1 +
> kernel/pgo/Kconfig | 3 ++-
> 5 files changed, 19 insertions(+), 2 deletions(-)
>
>
> base-commit: 4356bc4c0425c81e204f561acf4dd0095544a6cb
> --
> 2.32.0.288.g62a8d224e6-goog
>
--
Kees Cook
noinstr implies that we would like the compiler to avoid instrumenting a
function. Add support for the compiler attribute no_profile to
compiler_attributes.h, then add __no_profile to the definition of
noinstr.
Cc: Miguel Ojeda <[email protected]>
Link: https://lore.kernel.org/lkml/[email protected]/
Link: https://reviews.llvm.org/D104475
Link: https://reviews.llvm.org/D104257
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80223
Suggested-by: Peter Zijlstra <[email protected]>
Signed-off-by: Nick Desaulniers <[email protected]>
---
include/linux/compiler_attributes.h | 12 ++++++++++++
include/linux/compiler_types.h | 2 +-
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h
index c043b8d2b17b..cf584a1908b3 100644
--- a/include/linux/compiler_attributes.h
+++ b/include/linux/compiler_attributes.h
@@ -33,6 +33,7 @@
# define __GCC4_has_attribute___externally_visible__ 1
# define __GCC4_has_attribute___no_caller_saved_registers__ 0
# define __GCC4_has_attribute___noclone__ 1
+# define __GCC4_has_attribute___no_profile 0
# define __GCC4_has_attribute___nonstring__ 0
# define __GCC4_has_attribute___no_sanitize_address__ (__GNUC_MINOR__ >= 8)
# define __GCC4_has_attribute___no_sanitize_undefined__ (__GNUC_MINOR__ >= 9)
@@ -237,6 +238,17 @@
# define __nonstring
#endif
+/*
+ * Optional: only supported since clang >= 13
+ * gcc: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80223
+ * clang: https://clang.llvm.org/docs/AttributeReference.html#no_profile
+ */
+#if __has_attribute(__no_profile__)
+# define __no_profile __attribute__((__no_profile__))
+#else
+# define __no_profile
+#endif
+
/*
* gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-noreturn-function-attribute
* clang: https://clang.llvm.org/docs/AttributeReference.html#noreturn
diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index d29bda7f6ebd..d509169860f1 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -210,7 +210,7 @@ struct ftrace_likely_data {
/* Section for code which can't be instrumented at all */
#define noinstr \
noinline notrace __attribute((__section__(".noinstr.text"))) \
- __no_kcsan __no_sanitize_address
+ __no_kcsan __no_sanitize_address __no_profile
#endif /* __KERNEL__ */
--
2.32.0.288.g62a8d224e6-goog
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 <[email protected]>
Cc: Peter Oberparleiter <[email protected]>
Link: https://lore.kernel.org/lkml/[email protected]/
Link: https://lore.kernel.org/lkml/YMcssV%[email protected]/
Suggested-by: Peter Zijlstra <[email protected]>
Signed-off-by: Nick Desaulniers <[email protected]>
---
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)
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
On Sat, Jun 19, 2021 at 1:30 AM Nick Desaulniers
<[email protected]> wrote:
>
> +/*
> + * Optional: only supported since clang >= 13
> + * gcc: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80223
> + * clang: https://clang.llvm.org/docs/AttributeReference.html#no_profile
> + */
I am not sure if it is best or not to have the GCC link in order to be
consistent with the rest of the links (they are for the docs only). Do
we know if GCC going to implement it soon?
Otherwise, it looks good to me.
Cheers,
Miguel
On Sat, Jun 19, 2021 at 1:26 PM Miguel Ojeda
<[email protected]> wrote:
>
> I am not sure if it is best or not to have the GCC link in order to be
> consistent with the rest of the links (they are for the docs only). Do
> we know if GCC going to implement it soon?
i.e. if GCC does not implement it yet we use elsewhere this kind of
marker instead:
* Optional: not supported by gcc
The first of its kind, normally it is clang/icc there ;-)
We could nevertheless have the link there, something like:
* Optional: not supported by GCC
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80223
Cheers,
Miguel
On Fri, Jun 18, 2021 at 7:45 PM Kees Cook <[email protected]> wrote:
>
> On Fri, Jun 18, 2021 at 04:30:21PM -0700, Nick Desaulniers wrote:
> > When we say noinstr, we mean noinstr. GCOV and PGO can both instrument
> > functions. Add a new function annotation __no_profile that expands to
> > __attribute__((__no_profile__)) and Kconfig value
> > CC_HAS_NO_PROFILE_FN_ATTR.
> >
> > Base is
> > https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=for-next/clang/pgo.
> >
> > Nick Desaulniers (2):
> > compiler_attributes.h: define __no_profile, add to noinstr
> > Kconfig: CC_HAS_NO_PROFILE_FN_ATTR, depend on for GCOV and PGO
>
> Oh, awesome! Thanks for the fast work on this. If there are no objections,
> I'll apply this in front of the PGO series and put it in -next.
>
That works for me! Thanks, Nick and Kees!
-bw
On Sat, Jun 19, 2021 at 4:45 AM Kees Cook <[email protected]> wrote:
>
> Oh, awesome! Thanks for the fast work on this. If there are no objections,
> I'll apply this in front of the PGO series and put it in -next.
If you are picking both patches on your tree, please see my comment on
the first commit.
With that solved, for the first commit:
Reviewed-by: Miguel Ojeda <[email protected]>
Cheers,
Miguel
On Sat, Jun 19, 2021 at 4:32 AM Miguel Ojeda
<[email protected]> wrote:
>
> On Sat, Jun 19, 2021 at 1:26 PM Miguel Ojeda
> <[email protected]> wrote:
> >
> > I am not sure if it is best or not to have the GCC link in order to be
> > consistent with the rest of the links (they are for the docs only). Do
> > we know if GCC going to implement it soon?
>
> i.e. if GCC does not implement it yet we use elsewhere this kind of
> marker instead:
>
> * Optional: not supported by gcc
>
> The first of its kind, normally it is clang/icc there ;-)
:^) GCC does have an attribute since GCC 7.1 for this.
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80223#c11
I'm moving Clang over to use that in
https://reviews.llvm.org/D104658
Once that lands, I'll send a v2 (without carrying any reviewed by tags).
>
> We could nevertheless have the link there, something like:
>
> * Optional: not supported by GCC
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80223
--
Thanks,
~Nick Desaulniers
On 2021-06-21, Nick Desaulniers wrote:
>On Sat, Jun 19, 2021 at 4:32 AM Miguel Ojeda
><[email protected]> wrote:
>>
>> On Sat, Jun 19, 2021 at 1:26 PM Miguel Ojeda
>> <[email protected]> wrote:
>> >
>> > I am not sure if it is best or not to have the GCC link in order to be
>> > consistent with the rest of the links (they are for the docs only). Do
>> > we know if GCC going to implement it soon?
>>
>> i.e. if GCC does not implement it yet we use elsewhere this kind of
>> marker instead:
>>
>> * Optional: not supported by gcc
>>
>> The first of its kind, normally it is clang/icc there ;-)
>
>:^) GCC does have an attribute since GCC 7.1 for this.
>https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80223#c11
>I'm moving Clang over to use that in
>https://reviews.llvm.org/D104658
>Once that lands, I'll send a v2 (without carrying any reviewed by tags).
Thanks! __attribute__((no_profile_instrument_function)) looks good to me.
Also a reminder that __GCC4_has_attribute___no_profile in v1 misses two
underscores. v2 no_profile_instrument_function may need to fix this.
Reviewed-by: Fangrui Song <[email protected]>
>>
>> We could nevertheless have the link there, something like:
>>
>> * Optional: not supported by GCC
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80223
>
>--
>Thanks,
>~Nick Desaulniers
On Mon, Jun 21, 2021 at 8:24 PM Fangrui Song <[email protected]> wrote:
>
> Also a reminder that __GCC4_has_attribute___no_profile in v1 misses two
> underscores. v2 no_profile_instrument_function may need to fix this.
Good catch! Yes, it is missing the last two.
Cheers,
Miguel