From: Kees Cook <keescook@chromium.org>
To: Marco Elver <elver@google.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
Bill Wendling <morbo@google.com>,
Jonathan Corbet <corbet@lwn.net>,
Masahiro Yamada <masahiroy@kernel.org>,
Linux Doc Mailing List <linux-doc@vger.kernel.org>,
LKML <linux-kernel@vger.kernel.org>,
Linux Kbuild mailing list <linux-kbuild@vger.kernel.org>,
clang-built-linux <clang-built-linux@googlegroups.com>,
Andrew Morton <akpm@linux-foundation.org>,
Nathan Chancellor <natechancellor@gmail.com>,
Nick Desaulniers <ndesaulniers@google.com>,
Sami Tolvanen <samitolvanen@google.com>,
Fangrui Song <maskray@google.com>,
"maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)"
<x86@kernel.org>, Andrey Konovalov <andreyknvl@gmail.com>,
Dmitry Vyukov <dvyukov@google.com>,
johannes.berg@intel.com, oberpar@linux.vnet.ibm.com,
linux-toolchains@vger.kernel.org
Subject: Re: [PATCH v9] pgo: add clang's Profile Guided Optimization infrastructure
Date: Mon, 14 Jun 2021 08:26:01 -0700 [thread overview]
Message-ID: <202106140817.F584D2F@keescook> (raw)
In-Reply-To: <CANpmjNNnZv7DHYaJBL7knn9P+50F+SOCvis==Utaf-avENnVsw@mail.gmail.com>
On Mon, Jun 14, 2021 at 04:16:16PM +0200, 'Marco Elver' via Clang Built Linux wrote:
> On Mon, 14 Jun 2021 at 12:45, Peter Zijlstra <peterz@infradead.org> wrote:
> [...]
> > I've also been led to believe that the KCOV data format is not in fact
> > dependent on which toolchain is used.
>
> Correct, we use KCOV with both gcc and clang. Both gcc and clang emit
> the same instrumentation for -fsanitize-coverage. Thus, the user-space
> portion and interface is indeed identical:
> https://www.kernel.org/doc/html/latest/dev-tools/kcov.html
>
> > > > I'm thinking it might be about time to build _one_ infrastructure for
> > > > that and define a kernel arc format and call it a day.
> > > >
> > > That may be nice, but it's a rather large request.
> >
> > Given GCOV just died, perhaps you can look at what KCOV does and see if
> > that can be extended to do as you want. KCOV is actively used and
> > we actually tripped over all the fun little noinstr bugs at the time.
>
> There might be a subtle mismatch between coverage instrumentation for
> testing/fuzzing and for profiling. (Disclaimer: I'm not too familiar
> with Clang-PGO's requirements.) For example, while for testing/fuzzing
> we may only require information if a code-path has been visited, for
> profiling the "hotness" might be of interest. Therefore, the
> user-space exported data format can make several trade-offs in
> complexity.
This has been my primary take-away: given that Clang's PGO is different
enough from the other things and provides more specific/actionable
results, I think it's justified to exist on its own separate from the
other parts.
> In theory, I imagine there's a limit to how generic one could make
> profiling information, because one compiler's optimizations are not
> another compiler's optimizations. On the other hand, it may be doable
> to collect unified profiling information for common stuff, but I guess
> there's little motivation for figuring out the common ground given the
> producer and consumer of the PGO data is the same compiler by design
> (unlike coverage info for testing/fuzzing).
>
> Therefore, if KCOV's exposed information does not match PGO's
> requirements today, I'm not sure what realistically can be done
> without turning KCOV into a monster. Because KCOV is optimized for
> testing/fuzzing coverage, and I'm not sure how complex we can or want
> to make it to cater to a new use-case.
>
> My intuition is that the simpler design is to have 2 subsystems for
> instrumentation-based coverage collection: one for testing/fuzzing,
> and the other for profiling.
>
> Alas, there's the problem of GCOV, which should be replaceable by KCOV
> for most use cases. But it would be good to hear from a GCOV user if
> there are some.
>
> But as we learned GCOV is broken on x86 now, I see these options:
>
> 1. Remove GCOV, make KCOV the de-facto test-coverage collection
> subsystem. Introduce PGO-instrumentation subsystem for profile
> collection only, and make it _very_ clear that KCOV != PGO data as
> hinted above. A pre-requisite is that compiler-support for PGO
> instrumentation adds selective instrumentation support, likely just
> making attribute no_instrument_function do the right thing.
Right. I can't speak to GCOV, but KCOV certainly isn't PGO.
> 2. Like (1) but also keep GCOV, given proper support for attribute
> no_instrument_function would probably fix it (?).
>
> 3. Keep GCOV (and KCOV of course). Somehow extract PGO profiles from KCOV.
>
> 4. Somehow extract PGO profiles from GCOV, or modify kernel/gcov to do so.
If there *is* a way to "combine" these, I don't think it makes sense
to do it now. PGO has users (and is expanding[1]), and trying to
optimize the design before even landing the first version seems like a
needless obstruction, and to likely not address currently undiscovered
requirements.
So, AFAICT, the original blocking issue ("PGO does not respect noinstr")
is not actually an issue (noinstr contains notrace, which IS respected
by PGO[2]), I think this is fine to move forward.
-Kees
[1] https://lore.kernel.org/lkml/20210612032425.11425-1-jarmo.tiitto@gmail.com/
[2] https://lore.kernel.org/lkml/CAGG=3QVHkkJ236mCJ8Jt_6JtgYtWHV9b4aVXnoj6ypc7GOnc0A@mail.gmail.com/
--
Kees Cook
next prev parent reply other threads:[~2021-06-14 15:28 UTC|newest]
Thread overview: 124+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-11 8:18 [PATCH] pgo: add clang's Profile Guided Optimization infrastructure Bill Wendling
2021-01-11 8:39 ` Sedat Dilek
2021-01-11 8:42 ` Sedat Dilek
2021-01-11 9:17 ` Bill Wendling
2021-01-11 9:57 ` Sedat Dilek
2021-01-11 18:28 ` Nathan Chancellor
2021-01-11 20:12 ` Fangrui Song
2021-01-11 20:23 ` Bill Wendling
2021-01-11 20:31 ` Fangrui Song
2021-01-12 0:37 ` Bill Wendling
2021-01-12 0:44 ` Fāng-ruì Sòng
2021-01-11 21:04 ` Nathan Chancellor
2021-01-11 21:17 ` Nick Desaulniers
2021-01-11 21:32 ` Bill Wendling
2021-01-12 5:14 ` [PATCH v2] " Bill Wendling
2021-01-12 5:17 ` Sedat Dilek
2021-01-12 5:31 ` [PATCH v3] " Bill Wendling
2021-01-12 9:10 ` kernel test robot
2021-01-12 9:10 ` kernel test robot
2021-01-12 17:22 ` Nathan Chancellor
2021-01-13 6:19 ` [PATCH v4] " Bill Wendling
2021-01-13 20:55 ` Nathan Chancellor
2021-01-13 21:59 ` Bill Wendling
2021-01-14 4:07 ` Nick Desaulniers
2021-01-16 0:01 ` Nick Desaulniers
2021-01-16 0:13 ` Nick Desaulniers
2021-01-16 4:30 ` Sedat Dilek
2021-01-16 5:07 ` Sedat Dilek
2021-01-16 5:18 ` Sedat Dilek
2021-01-18 0:57 ` Sedat Dilek
2021-01-13 23:01 ` Nick Desaulniers
2021-01-16 9:43 ` [PATCH v5] " Bill Wendling
2021-01-16 17:38 ` Sedat Dilek
2021-01-16 18:36 ` Sedat Dilek
2021-01-16 20:23 ` Bill Wendling
2021-01-17 10:44 ` Sedat Dilek
2021-01-17 10:53 ` Sedat Dilek
2021-01-17 11:23 ` Sedat Dilek
2021-01-17 11:42 ` Sedat Dilek
2021-01-17 11:58 ` Sedat Dilek
2021-01-17 12:05 ` Sedat Dilek
2021-01-17 17:42 ` Sedat Dilek
2021-01-17 20:34 ` Bill Wendling
2021-01-17 23:33 ` Sedat Dilek
2021-01-18 0:26 ` Sedat Dilek
2021-01-18 2:32 ` Bill Wendling
2021-01-18 12:39 ` Sedat Dilek
2021-01-18 17:26 ` Sedat Dilek
2021-01-18 21:56 ` Bill Wendling
2021-01-18 23:29 ` Sedat Dilek
2021-01-21 2:03 ` Sedat Dilek
2021-01-21 22:44 ` Sedat Dilek
2021-01-22 1:42 ` Nick Desaulniers
2021-01-22 1:49 ` Sedat Dilek
2021-01-22 1:52 ` Nick Desaulniers
2021-01-22 1:54 ` Sedat Dilek
2021-01-20 1:02 ` Nick Desaulniers
2021-01-21 0:51 ` Nick Desaulniers
2021-01-21 8:24 ` Bill Wendling
2021-01-21 8:24 ` [PATCH v6] " Bill Wendling
2021-01-21 10:34 ` Sedat Dilek
2021-01-22 1:44 ` Nick Desaulniers
2021-01-22 1:51 ` Sedat Dilek
2021-01-22 0:14 ` Sedat Dilek
2021-01-22 0:58 ` Sedat Dilek
2021-01-22 1:29 ` Nick Desaulniers
2021-01-22 10:11 ` [PATCH v7] " Bill Wendling
2021-01-22 11:31 ` Sedat Dilek
2021-01-22 18:41 ` Nick Desaulniers
2021-01-28 20:46 ` Sedat Dilek
2021-01-28 21:12 ` Nick Desaulniers
2021-01-28 21:19 ` Sedat Dilek
2021-01-28 21:24 ` Nick Desaulniers
2021-01-28 21:39 ` Sedat Dilek
2021-01-29 7:43 ` Sedat Dilek
2021-01-29 21:48 ` Nick Desaulniers
2021-02-10 23:25 ` Bill Wendling
2021-02-22 21:52 ` Bill Wendling
2021-02-26 22:20 ` [PATCH v8] " Bill Wendling
2021-02-26 22:55 ` Bill Wendling
2021-02-28 18:52 ` Fangrui Song
2021-02-28 21:50 ` Fangrui Song
2021-01-12 17:37 ` [PATCH v2] " Nick Desaulniers
2021-01-12 17:45 ` Fāng-ruì Sòng
2021-01-21 2:21 ` [PATCH] " Sedat Dilek
2021-01-22 1:34 ` Nick Desaulniers
2021-01-22 1:43 ` Sedat Dilek
2021-04-07 21:17 ` [PATCH v9] " Bill Wendling
2021-04-07 21:22 ` Kees Cook
2021-04-07 21:44 ` Fāng-ruì Sòng
2021-04-07 21:47 ` Nathan Chancellor
2021-04-07 21:58 ` Bill Wendling
2021-05-19 21:37 ` Kees Cook
2021-05-22 23:51 ` Bill Wendling
2021-05-31 21:12 ` Nathan Chancellor
2021-06-01 17:31 ` Nick Desaulniers
2021-06-12 16:59 ` Peter Zijlstra
2021-06-12 17:25 ` Bill Wendling
2021-06-12 18:15 ` Peter Zijlstra
2021-06-12 19:10 ` Bill Wendling
2021-06-12 19:28 ` Bill Wendling
2021-06-12 20:25 ` Peter Zijlstra
2021-06-12 20:56 ` Bill Wendling
2021-06-12 22:47 ` Bill Wendling
2021-06-13 18:07 ` Bill Wendling
2021-06-14 9:43 ` Peter Zijlstra
2021-06-14 10:18 ` Peter Zijlstra
2021-06-14 7:51 ` Peter Zijlstra
2021-06-14 9:01 ` Peter Zijlstra
2021-06-14 9:39 ` Bill Wendling
2021-06-14 10:44 ` Peter Zijlstra
2021-06-14 11:41 ` Bill Wendling
2021-06-14 11:43 ` Bill Wendling
2021-06-14 14:16 ` Marco Elver
2021-06-14 15:26 ` Kees Cook [this message]
2021-06-14 15:35 ` Peter Zijlstra
2021-06-14 16:22 ` Kees Cook
2021-06-14 18:07 ` Nick Desaulniers
2021-06-14 20:49 ` Nick Desaulniers
2021-06-14 15:46 ` Peter Zijlstra
2021-06-14 16:03 ` Nick Desaulniers
2021-06-12 20:20 ` Fangrui Song
2021-06-12 20:31 ` Peter Zijlstra
-- strict thread matches above, loose matches on Subject: below --
2021-06-01 18:53 Kees Cook
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=202106140817.F584D2F@keescook \
--to=keescook@chromium.org \
--cc=akpm@linux-foundation.org \
--cc=andreyknvl@gmail.com \
--cc=clang-built-linux@googlegroups.com \
--cc=corbet@lwn.net \
--cc=dvyukov@google.com \
--cc=elver@google.com \
--cc=johannes.berg@intel.com \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kbuild@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-toolchains@vger.kernel.org \
--cc=masahiroy@kernel.org \
--cc=maskray@google.com \
--cc=morbo@google.com \
--cc=natechancellor@gmail.com \
--cc=ndesaulniers@google.com \
--cc=oberpar@linux.vnet.ibm.com \
--cc=peterz@infradead.org \
--cc=samitolvanen@google.com \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.