Message-ID: <Ztic3BR4UUdhIVrq@google.com>
Date: Wed, 4 Sep 2024 10:46:04 -0700
From: Namhyung Kim <namhyung@...nel.org>
To: Ian Rogers <irogers@...gle.com>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>,
	Kan Liang <kan.liang@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>,
	Adrian Hunter <adrian.hunter@...el.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...nel.org>, LKML <linux-kernel@...r.kernel.org>,
	linux-perf-users@...r.kernel.org,
	Ravi Bangoria <ravi.bangoria@....com>,
	Mark Rutland <mark.rutland@....com>,
	James Clark <james.clark@....com>,
	Athira Rajeev <atrajeev@...ux.vnet.ibm.com>,
	Kajol Jain <kjain@...ux.ibm.com>,
	Thomas Richter <tmricht@...ux.ibm.com>,
	Atish Patra <atishp@...shpatra.org>,
	Palmer Dabbelt <palmer@...osinc.com>,
	Mingwei Zhang <mizhang@...gle.com>
Subject: Re: [RFC/PATCHSET 0/8] perf tools: Do not set attr.exclude_guest by
 default (v2)

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@...nel.org> 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@kernel.org/
> >
> > 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
> >

Powered by blists - more mailing lists