linux-kernel - Re: [RFC/PATCHSET 0/8] perf tools: Do not set attr.exclude

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAP-5=fU5UeHER5hPAUhq_9_fSUR9okp_i0sOk_E7Q_aZkvo+DQ@mail.gmail.com>
Date: Wed, 4 Sep 2024 09:36:02 -0700
From: Ian Rogers <irogers@...gle.com>
To: Namhyung Kim <namhyung@...nel.org>
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)

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
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_*
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
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
>