Re: [PATCH 4/8] perf tools: Do not set exclude_guest for precise_ip
From: Ian Rogers
Date: Tue Oct 01 2024 - 13:46:36 EST
On Mon, Sep 30, 2024 at 5:20 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
>
> It seems perf sets the exclude_guest bit because of Intel PEBS
> implementation which uses a virtual address. IIUC now kernel disables
> PEBS when it goes to the guest mode regardless of this bit so we don't
> need to set it explicitly. At least for the other archs/vendors.
>
> I found the commit 1342798cc13e set the exclude_guest for precise_ip
> in the tool and the commit 20b279ddb38c added kernel side enforcement
> which was reverted by commit a706d965dcfd later.
>
> Actually it doesn't set the exclude_guest for the default event
> (cycles:P) already.
>
> $ grep -m1 vendor /proc/cpuinfo
> vendor_id : GenuineIntel
>
> $ perf record -e cycles:P true
> [ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 0.002 MB perf.data (9 samples) ]
>
> $ perf evlist -v | tr ',' '\n' | grep -e exclude -e precise
> precise_ip: 3
>
> But having lower 'p' modifier set the bit for some reason.
>
> $ perf record -e cycles:pp true
> [ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 0.002 MB perf.data (9 samples) ]
>
> $ perf evlist -v | tr ',' '\n' | grep -e exclude -e precise
> precise_ip: 2
> exclude_guest: 1
>
> Actually AMD IBS suffers from this because it doesn't support excludes
> and having this bit effectively disables new features in the current
> implementation (due to the missing feature check).
>
> $ grep -m1 vendor /proc/cpuinfo
> vendor_id : AuthenticAMD
>
> $ perf record -W -e cycles:p -vv true 2>&1 | grep switching
> switching off PERF_FORMAT_LOST support
> switching off weight struct support
> switching off bpf_event
> switching off ksymbol
> switching off cloexec flag
> switching off mmap2
> switching off exclude_guest, exclude_host
>
> By not setting exclude_guest, we can fix this inconsistency and the
> troubles.
>
> Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
Reviewed-by: Ian Rogers <irogers@xxxxxxxxxx>
Thanks,
Ian
> ---
> tools/perf/tests/parse-events.c | 12 ++++--------
> tools/perf/util/parse-events.c | 4 ----
> 2 files changed, 4 insertions(+), 12 deletions(-)
>
> diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
> index 727683f249f66f5a..82a19674a38f774e 100644
> --- a/tools/perf/tests/parse-events.c
> +++ b/tools/perf/tests/parse-events.c
> @@ -898,8 +898,7 @@ static int test__group1(struct evlist *evlist)
> TEST_ASSERT_VAL("wrong exclude_user", !evsel->core.attr.exclude_user);
> TEST_ASSERT_VAL("wrong exclude_kernel", evsel->core.attr.exclude_kernel);
> TEST_ASSERT_VAL("wrong exclude_hv", evsel->core.attr.exclude_hv);
> - /* use of precise requires exclude_guest */
> - TEST_ASSERT_VAL("wrong exclude guest", evsel->core.attr.exclude_guest);
> + TEST_ASSERT_VAL("wrong exclude guest", !evsel->core.attr.exclude_guest);
> TEST_ASSERT_VAL("wrong exclude host", !evsel->core.attr.exclude_host);
> TEST_ASSERT_VAL("wrong precise_ip", evsel->core.attr.precise_ip == 2);
> TEST_ASSERT_VAL("wrong leader", evsel__has_leader(evsel, leader));
> @@ -1016,9 +1015,8 @@ static int test__group3(struct evlist *evlist __maybe_unused)
> TEST_ASSERT_VAL("wrong exclude_kernel",
> !evsel->core.attr.exclude_kernel);
> TEST_ASSERT_VAL("wrong exclude_hv", evsel->core.attr.exclude_hv);
> - /* use of precise requires exclude_guest */
> TEST_ASSERT_VAL("wrong exclude guest",
> - evsel->core.attr.exclude_guest);
> + !evsel->core.attr.exclude_guest);
> TEST_ASSERT_VAL("wrong exclude host",
> !evsel->core.attr.exclude_host);
> TEST_ASSERT_VAL("wrong precise_ip",
> @@ -1103,8 +1101,7 @@ static int test__group4(struct evlist *evlist __maybe_unused)
> TEST_ASSERT_VAL("wrong exclude_user", !evsel->core.attr.exclude_user);
> TEST_ASSERT_VAL("wrong exclude_kernel", evsel->core.attr.exclude_kernel);
> TEST_ASSERT_VAL("wrong exclude_hv", evsel->core.attr.exclude_hv);
> - /* use of precise requires exclude_guest */
> - TEST_ASSERT_VAL("wrong exclude guest", evsel->core.attr.exclude_guest);
> + TEST_ASSERT_VAL("wrong exclude guest", !evsel->core.attr.exclude_guest);
> TEST_ASSERT_VAL("wrong exclude host", !evsel->core.attr.exclude_host);
> TEST_ASSERT_VAL("wrong precise_ip", evsel->core.attr.precise_ip == 1);
> TEST_ASSERT_VAL("wrong group name", !evsel->group_name);
> @@ -1122,8 +1119,7 @@ static int test__group4(struct evlist *evlist __maybe_unused)
> TEST_ASSERT_VAL("wrong exclude_user", evsel->core.attr.exclude_user);
> TEST_ASSERT_VAL("wrong exclude_kernel", !evsel->core.attr.exclude_kernel);
> TEST_ASSERT_VAL("wrong exclude_hv", evsel->core.attr.exclude_hv);
> - /* use of precise requires exclude_guest */
> - TEST_ASSERT_VAL("wrong exclude guest", evsel->core.attr.exclude_guest);
> + TEST_ASSERT_VAL("wrong exclude guest", !evsel->core.attr.exclude_guest);
> TEST_ASSERT_VAL("wrong exclude host", !evsel->core.attr.exclude_host);
> TEST_ASSERT_VAL("wrong precise_ip", evsel->core.attr.precise_ip == 2);
> TEST_ASSERT_VAL("wrong leader", evsel__has_leader(evsel, leader));
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index ff67213d6e887169..63da105ba914ef29 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -1769,10 +1769,6 @@ static int parse_events__modifier_list(struct parse_events_state *parse_state,
> int exclude = eu | ek | eh;
> int exclude_GH = group ? evsel->exclude_GH : 0;
>
> - if (mod.precise) {
> - /* use of precise requires exclude_guest */
> - eG = 1;
> - }
> if (mod.user) {
> if (!exclude)
> exclude = eu = ek = eh = 1;
> --
> 2.46.1.824.gd892dcdcdd-goog
>