Re: [RFC/PATCHSET 0/8] perf tools: Do not set attr.exclude_guest by default (v2)

From: Namhyung Kim
Date: Wed Sep 04 2024 - 13:46:27 EST


Hi Ian,

On Wed, Sep 04, 2024 at 09:36:02AM -0700, Ian Rogers wrote:
> On Tue, Sep 3, 2024 at 11:41 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
> >
> > Hello,
> >
> > I found perf tools set exclude_guest bit inconsistently. It used to
> > set the bit but now the default event for perf record doesn't. So I'm
> > wondering why we want the bit in the first place.
> >
> > Actually it's not good for PMUs don't support any exclusion like AMD
> > IBS because it disables new features after the exclude_guest due to
> > the missing feature detection logic.
>
> I think trying to clean this up is good but there's a wac-a-moie
> problem whenever a default or fallback is changed - it can break a
> hard to test platform in unthought of ways. I wonder if we can expand
> PMU testing to at least capture the differences in behavior. For

Right, that's why I use a software event to test kernel features and
separate branch stack, precise_ip and excludes testing.


> example, pick an event that works, like legacy cycles, then increase
> the precision to 3 and the event should either open again or fail with
> EINVAL, if it opens then it should count. Similarly for the exclude_*

But sometimes precision and exclude_* interfere each other. AMD IBS
accepts precise_ip up to 2 but it'd fail if exclude_guest is set. And
sometimes the same PMU requires different exclude bits. For instance,
Intel branch stack sampling needs exclude_hv as well as exclude_kernel
for regular users, but normally it is enough to have exclude_kernel
for other events.

I agree that we need to save this info to per-PMU and reuse it for
other events. Actually I tried this but failed to get it working.
IIRC sometimes legacy events don't have evsel->pmu and looking up a
PMU for the evsel failed to get a correct PMU. Also I couldn't come
up with an absolute way to make sure if it checks the PMU features
100% correct. I think I need to add something to the kernel to show
supported (and required?) PMU exclude_* capabilities.

Maybe we can revisit this later, but having the missing kernel feature
checks separate from the PMU specific checks is an improvement IMHO.
Also having another fallback for exclude_guest would fix an existing
problem on Apple M1.


> bits. I think some PMUs don't behave like they should and we can add
> ifs to the test to capture these behaviours - for example an
> exclude_.. is accepted for an event but then the event doesn't count
> if the bit is set. There are many cases where a large group of events

So I want to start from no exclude_* and to add a bit at a time until
the PMU accepts the event so that it won't have unnecessary bits.

Thanks,
Namhyung


> will cause the group to stop counting, in metrics we work around this
> with grouping flags for the metric:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/pmu-events/pmu-events.h?h=perf-tools-next#n16
> but these shouldn't be necessary as the PMU kernel driver should
> reject the perf_event_open.
>
> Thanks,
> Ian
>
> > v2 changes)
> > * update the missing feature detection logic
> > * separate exclude_hv fallback
> > * add new fallback for exclude_guest
> >
> > v1) https://lore.kernel.org/lkml/20240902014621.2002343-1-namhyung@xxxxxxxxxx/
> >
> > AFAIK it doesn't matter for the most cases but perf kvm. If users
> > need to set the bit, they can still use :H modifier. For vPMU pass-
> > through or Apple M1, it'd add the exclude_guest during the fallback
> > logic. Please let me know if it's ok for you.
> >
> > The code is available at 'perf/exclude-v2' branch in
> > git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
> >
> > Thanks,
> > Namhyung
> >
> >
> > Namhyung Kim (8):
> > perf tools: Don't set attr.exclude_guest by default
> > perf tools: Simplify evsel__add_modifier()
> > perf stat: Add --exclude-guest option
> > perf tools: Do not set exclude_guest for precise_ip
> > perf tools: Detect missing kernel features properly
> > perf tools: Separate exclude_hv fallback
> > perf tools: Add fallback for exclude_guest
> > perf tools: Check fallback error and order
> >
> > tools/perf/Documentation/perf-stat.txt | 7 +
> > tools/perf/builtin-kvm.c | 1 +
> > tools/perf/builtin-stat.c | 2 +
> > tools/perf/dlfilters/dlfilter-test-api-v0.c | 2 +-
> > tools/perf/dlfilters/dlfilter-test-api-v2.c | 2 +-
> > tools/perf/tests/attr/test-record-dummy-C0 | 2 +-
> > tools/perf/tests/parse-events.c | 30 +-
> > tools/perf/util/evsel.c | 393 ++++++++++++++------
> > tools/perf/util/evsel.h | 1 -
> > tools/perf/util/parse-events.c | 6 +-
> > tools/perf/util/util.c | 10 +-
> > tools/perf/util/util.h | 3 +
> > 12 files changed, 322 insertions(+), 137 deletions(-)
> >
> > --
> > 2.46.0.469.g59c65b2a67-goog
> >