mbox series

[v2,0/3] no_profile fn attr and Kconfig for GCOV+PGO

Message ID 20210621231822.2848305-1-ndesaulniers@google.com (mailing list archive)
Headers show
Series no_profile fn attr and Kconfig for GCOV+PGO | expand

Message

Nick Desaulniers June 21, 2021, 11:18 p.m. UTC
The kernel has been using noinstr for correctness to politely request
that the compiler avoid adding various forms of instrumentation to
certain functions.

GCOV and PGO can both instrument functions, yet the function attribute
to disable such instrumentation (no_profile_instrument_function) was not
being used to suppress such implementation. Also, clang only just
recently gained support for no_profile_instrument_function. GCC has
supported that since 7.1+.

Add a new function annotation __no_profile that expands to
__attribute__((__no_profile_instrument_function__)) and Kconfig values
CC_HAS_NO_PROFILE_FN_ATTR and ARCH_WANTS_NO_INSTR. Make GCOV and PGO
depend on either !ARCH_WANTS_NO_INSTR or CC_HAS_NO_PROFILE_FN_ATTR.

Changes V1 -> V2:
* s/no_profile/no_profile_instrument_function/
* fix trailing double underscore on GCC 4 define, as per Fangrui+Miguel.
* Pick up Fangrui + Miguel's reviewed-by tag.
* Add link to GCC's doc.
* Fix clang's doc format; will appear once clang-13 is released.
* New cleanup patch 2/3. Orthogonal to the series, but while I'm here...

Base is
https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=for-next/clang/pgo.

Nick Desaulniers (3):
  compiler_attributes.h: define __no_profile, add to noinstr
  compiler_attributes.h: cleanups for GCC 4.9+
  Kconfig: add ARCH_WANTS_NO_INSTR+CC_HAS_NO_PROFILE_FN_ATTR, depend on
    for GCOV and PGO

 arch/Kconfig                        |  7 +++++++
 arch/arm64/Kconfig                  |  1 +
 arch/s390/Kconfig                   |  1 +
 arch/x86/Kconfig                    |  1 +
 include/linux/compiler_attributes.h | 19 ++++++++++++++++---
 include/linux/compiler_types.h      |  2 +-
 init/Kconfig                        |  3 +++
 kernel/gcov/Kconfig                 |  1 +
 kernel/pgo/Kconfig                  |  3 ++-
 9 files changed, 33 insertions(+), 5 deletions(-)


base-commit: 4356bc4c0425c81e204f561acf4dd0095544a6cb

Comments

Peter Zijlstra June 22, 2021, 7:25 a.m. UTC | #1
On Mon, Jun 21, 2021 at 04:18:19PM -0700, Nick Desaulniers wrote:
> Nick Desaulniers (3):
>   compiler_attributes.h: define __no_profile, add to noinstr
>   compiler_attributes.h: cleanups for GCC 4.9+
>   Kconfig: add ARCH_WANTS_NO_INSTR+CC_HAS_NO_PROFILE_FN_ATTR, depend on
>     for GCOV and PGO

Thanks for sorting this Nick!

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Kees Cook June 22, 2021, 6:19 p.m. UTC | #2
On Mon, Jun 21, 2021 at 04:18:19PM -0700, Nick Desaulniers wrote:
> Add a new function annotation __no_profile that expands to
> __attribute__((__no_profile_instrument_function__)) and Kconfig values
> CC_HAS_NO_PROFILE_FN_ATTR and ARCH_WANTS_NO_INSTR. Make GCOV and PGO
> depend on either !ARCH_WANTS_NO_INSTR or CC_HAS_NO_PROFILE_FN_ATTR.

Awesome; thanks everyone! I'm doing a Clang rebuild now, and will do
kernel testing and push this to my for-next/clang/features tree shortly.
Kees Cook June 23, 2021, 6:15 a.m. UTC | #3
On Mon, 21 Jun 2021 16:18:19 -0700, Nick Desaulniers wrote:
> The kernel has been using noinstr for correctness to politely request
> that the compiler avoid adding various forms of instrumentation to
> certain functions.
> 
> GCOV and PGO can both instrument functions, yet the function attribute
> to disable such instrumentation (no_profile_instrument_function) was not
> being used to suppress such implementation. Also, clang only just
> recently gained support for no_profile_instrument_function. GCC has
> supported that since 7.1+.
> 
> [...]

Applied to for-next/clang/features, thanks!

[1/3] compiler_attributes.h: define __no_profile, add to noinstr
      https://git.kernel.org/kees/c/380d53c45ff2
[2/3] compiler_attributes.h: cleanups for GCC 4.9+
      https://git.kernel.org/kees/c/ae4d682dfd33
[3/3] Kconfig: add ARCH_WANTS_NO_INSTR+CC_HAS_NO_PROFILE_FN_ATTR, depend on for GCOV and PGO
      https://git.kernel.org/kees/c/51c2ee6d121c

Note that I've tweaked the series slightly to move the PGO Kconfig change into
the PGO patch.
Nick Desaulniers June 24, 2021, 7:36 p.m. UTC | #4
On Tue, Jun 22, 2021 at 11:17 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Mon, 21 Jun 2021 16:18:19 -0700, Nick Desaulniers wrote:
> > The kernel has been using noinstr for correctness to politely request
> > that the compiler avoid adding various forms of instrumentation to
> > certain functions.
> >
> > GCOV and PGO can both instrument functions, yet the function attribute
> > to disable such instrumentation (no_profile_instrument_function) was not
> > being used to suppress such implementation. Also, clang only just
> > recently gained support for no_profile_instrument_function. GCC has
> > supported that since 7.1+.
> >
> > [...]
>
> Applied to for-next/clang/features, thanks!
>
> [1/3] compiler_attributes.h: define __no_profile, add to noinstr
>       https://git.kernel.org/kees/c/380d53c45ff2
> [2/3] compiler_attributes.h: cleanups for GCC 4.9+
>       https://git.kernel.org/kees/c/ae4d682dfd33
> [3/3] Kconfig: add ARCH_WANTS_NO_INSTR+CC_HAS_NO_PROFILE_FN_ATTR, depend on for GCOV and PGO
>       https://git.kernel.org/kees/c/51c2ee6d121c
>
> Note that I've tweaked the series slightly to move the PGO Kconfig change into
> the PGO patch.

Ok, LGTM.