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

linux-perf-users.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [RFC/PATCHSET 0/8] perf tools: Do not set attr.exclude_guest by default (v2)
@ 2024-09-04  6:41 Namhyung Kim
  2024-09-04  6:41 ` [PATCH 1/8] perf tools: Don't set attr.exclude_guest by default Namhyung Kim
                   ` (8 more replies)
  0 siblings, 9 replies; 19+ messages in thread
From: Namhyung Kim @ 2024-09-04  6:41 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Ian Rogers, Kan Liang
  Cc: Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
	linux-perf-users, Ravi Bangoria, Mark Rutland, James Clark,
	Athira Rajeev, Kajol Jain, Thomas Richter, Atish Patra,
	Palmer Dabbelt, Mingwei Zhang

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.

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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH 1/8] perf tools: Don't set attr.exclude_guest by default
  2024-09-04  6:41 [RFC/PATCHSET 0/8] perf tools: Do not set attr.exclude_guest by default (v2) Namhyung Kim
@ 2024-09-04  6:41 ` Namhyung Kim
  2024-09-04 13:07   ` Arnaldo Carvalho de Melo
  2024-09-04  6:41 ` [PATCH 2/8] perf tools: Simplify evsel__add_modifier() Namhyung Kim
                   ` (7 subsequent siblings)
  8 siblings, 1 reply; 19+ messages in thread
From: Namhyung Kim @ 2024-09-04  6:41 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Ian Rogers, Kan Liang
  Cc: Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
	linux-perf-users, Ravi Bangoria, Mark Rutland, James Clark,
	Athira Rajeev, Kajol Jain, Thomas Richter, Atish Patra,
	Palmer Dabbelt, Mingwei Zhang, James Clark

The exclude_guest in the event attribute is to limit profiling in the
host environment.  But I'm not sure why we want to set it by default
cause we don't care about it in most cases and I feel like it just
makes new PMU implementation complicated.

Of course it's useful for perf kvm command so I added the
exclude_GH_default variable to preserve the old behavior for perf kvm
and other commands like perf record and stat won't set the exclude bit.
This is helpful for AMD IBS case since having exclude_guest bit will
clear new feature bit due to the missing feature check logic.

  $ sysctl kernel.perf_event_paranoid
  kernel.perf_event_paranoid = 0

  $ perf record -W -e ibs_op// -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

Maybe Apple M1 users will scream but actually the default event for
perf record was converted to "cycles:P" which doesn't set the
exclude_guest bit already.  So they need to specify the necessary
modifier manually like "cycles:PH" and I think it's ok.

Intestingly, I found it sets the exclude_bit if "u" modifier is used.
I don't know why but it's neither intuitive nor consistent.  Let's
remove the bit there too.

Cc: Mark Rutland <mark.rutland@arm.com>
Cc: James Clark <james.clark@linaro.org>
Cc: Ravi Bangoria <ravi.bangoria@amd.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-kvm.c                   |  1 +
 tools/perf/tests/attr/test-record-dummy-C0 |  2 +-
 tools/perf/tests/parse-events.c            | 18 +++++++++---------
 tools/perf/util/parse-events.c             |  2 +-
 tools/perf/util/util.c                     | 10 ++++++++--
 tools/perf/util/util.h                     |  3 +++
 6 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index 692267b1b7e8311f..ca94dd3de04d34a3 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -2147,6 +2147,7 @@ int cmd_kvm(int argc, const char **argv)
 						"buildid-list", "stat", NULL };
 	const char *kvm_usage[] = { NULL, NULL };
 
+	exclude_HG_default = true;
 	perf_host  = 0;
 	perf_guest = 1;
 
diff --git a/tools/perf/tests/attr/test-record-dummy-C0 b/tools/perf/tests/attr/test-record-dummy-C0
index 576ec48b3aafaa6a..8ce6f0a5df5b7013 100644
--- a/tools/perf/tests/attr/test-record-dummy-C0
+++ b/tools/perf/tests/attr/test-record-dummy-C0
@@ -37,7 +37,7 @@ precise_ip=0
 mmap_data=0
 sample_id_all=1
 exclude_host=0
-exclude_guest=1
+exclude_guest=0
 exclude_callchain_kernel=0
 exclude_callchain_user=0
 mmap2=1
diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
index edc2adcf1baed195..9179bf3084c302c1 100644
--- a/tools/perf/tests/parse-events.c
+++ b/tools/perf/tests/parse-events.c
@@ -932,7 +932,7 @@ static int test__group2(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);
-			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);
 			TEST_ASSERT_VAL("wrong leader", evsel__is_group_leader(evsel));
@@ -947,7 +947,7 @@ static int test__group2(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);
-			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);
 			if (evsel__has_leader(evsel, leader))
@@ -1072,7 +1072,7 @@ static int test__group3(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);
-		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);
 		TEST_ASSERT_VAL("wrong leader", evsel__is_group_leader(evsel));
@@ -1222,7 +1222,7 @@ static int test__group5(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);
-		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);
 		TEST_ASSERT_VAL("wrong leader", evsel__is_group_leader(evsel));
@@ -1437,7 +1437,7 @@ static int test__leader_sample1(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);
-		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);
 		TEST_ASSERT_VAL("wrong group name", !evsel->group_name);
@@ -1453,7 +1453,7 @@ static int test__leader_sample1(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);
-		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);
 		TEST_ASSERT_VAL("wrong leader", evsel__has_leader(evsel, leader));
@@ -1468,7 +1468,7 @@ static int test__leader_sample1(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);
-		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);
 		TEST_ASSERT_VAL("wrong group name", !evsel->group_name);
@@ -1497,7 +1497,7 @@ static int test__leader_sample2(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);
-		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);
 		TEST_ASSERT_VAL("wrong group name", !evsel->group_name);
@@ -1513,7 +1513,7 @@ static int test__leader_sample2(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);
-		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);
 		TEST_ASSERT_VAL("wrong group name", !evsel->group_name);
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index fab01ba54e34da7d..ab73b3d45f0467f9 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1739,7 +1739,7 @@ static int parse_events__modifier_list(struct parse_events_state *parse_state,
 		if (mod.user) {
 			if (!exclude)
 				exclude = eu = ek = eh = 1;
-			if (!exclude_GH && !perf_guest)
+			if (!exclude_GH && !perf_guest && exclude_HG_default)
 				eG = 1;
 			eu = 0;
 		}
diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
index 9d55a13787ce3c05..7e3159faaa1991df 100644
--- a/tools/perf/util/util.c
+++ b/tools/perf/util/util.c
@@ -78,17 +78,23 @@ bool sysctl__nmi_watchdog_enabled(void)
 
 bool test_attr__enabled;
 
+bool exclude_HG_default;
+
 bool perf_host  = true;
 bool perf_guest = false;
 
 void event_attr_init(struct perf_event_attr *attr)
 {
+	/* to capture ABI version */
+	attr->size = sizeof(*attr);
+
+	if (!exclude_HG_default)
+		return;
+
 	if (!perf_host)
 		attr->exclude_host  = 1;
 	if (!perf_guest)
 		attr->exclude_guest = 1;
-	/* to capture ABI version */
-	attr->size = sizeof(*attr);
 }
 
 int mkdir_p(char *path, mode_t mode)
diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
index 9966c21aaf048479..d33ae883a54f2f2f 100644
--- a/tools/perf/util/util.h
+++ b/tools/perf/util/util.h
@@ -21,6 +21,9 @@ extern const char perf_more_info_string[];
 
 extern const char *input_name;
 
+/* This will control if perf_{host,guest} will set attr.exclude_{host,guest}. */
+extern bool exclude_HG_default;
+
 extern bool perf_host;
 extern bool perf_guest;
 
-- 
2.46.0.469.g59c65b2a67-goog


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 2/8] perf tools: Simplify evsel__add_modifier()
  2024-09-04  6:41 [RFC/PATCHSET 0/8] perf tools: Do not set attr.exclude_guest by default (v2) Namhyung Kim
  2024-09-04  6:41 ` [PATCH 1/8] perf tools: Don't set attr.exclude_guest by default Namhyung Kim
@ 2024-09-04  6:41 ` Namhyung Kim
  2024-09-04  6:41 ` [PATCH 3/8] perf stat: Add --exclude-guest option Namhyung Kim
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 19+ messages in thread
From: Namhyung Kim @ 2024-09-04  6:41 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Ian Rogers, Kan Liang
  Cc: Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
	linux-perf-users, Ravi Bangoria, Mark Rutland, James Clark,
	Athira Rajeev, Kajol Jain, Thomas Richter, Atish Patra,
	Palmer Dabbelt, Mingwei Zhang

Since it doesn't set the exclude_guest, no need to special handle the
bit and simply show only if one of host or guest bit is set.  Now the
default event name might not have :H prefix anymore so change the
dlfilter test not to compare the ":" at the end.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/dlfilters/dlfilter-test-api-v0.c | 2 +-
 tools/perf/dlfilters/dlfilter-test-api-v2.c | 2 +-
 tools/perf/util/evsel.c                     | 5 +----
 3 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/tools/perf/dlfilters/dlfilter-test-api-v0.c b/tools/perf/dlfilters/dlfilter-test-api-v0.c
index 4083b1abeaabe605..4ca2d7b2ea6c8200 100644
--- a/tools/perf/dlfilters/dlfilter-test-api-v0.c
+++ b/tools/perf/dlfilters/dlfilter-test-api-v0.c
@@ -220,7 +220,7 @@ static int check_sample(struct filter_data *d, const struct perf_dlfilter_sample
 	CHECK_SAMPLE(raw_callchain_nr);
 	CHECK(!sample->raw_callchain);
 
-#define EVENT_NAME "branches:"
+#define EVENT_NAME "branches"
 	CHECK(!strncmp(sample->event, EVENT_NAME, strlen(EVENT_NAME)));
 
 	return 0;
diff --git a/tools/perf/dlfilters/dlfilter-test-api-v2.c b/tools/perf/dlfilters/dlfilter-test-api-v2.c
index 32ff619e881caa50..00d73a16c4fdaece 100644
--- a/tools/perf/dlfilters/dlfilter-test-api-v2.c
+++ b/tools/perf/dlfilters/dlfilter-test-api-v2.c
@@ -235,7 +235,7 @@ static int check_sample(struct filter_data *d, const struct perf_dlfilter_sample
 	CHECK_SAMPLE(raw_callchain_nr);
 	CHECK(!sample->raw_callchain);
 
-#define EVENT_NAME "branches:"
+#define EVENT_NAME "branches"
 	CHECK(!strncmp(sample->event, EVENT_NAME, strlen(EVENT_NAME)));
 
 	return 0;
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 49cc71511c0c8ce8..57fe0f9b06f9a8bb 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -548,7 +548,6 @@ static int evsel__add_modifiers(struct evsel *evsel, char *bf, size_t size)
 {
 	int colon = 0, r = 0;
 	struct perf_event_attr *attr = &evsel->core.attr;
-	bool exclude_guest_default = false;
 
 #define MOD_PRINT(context, mod)	do {					\
 		if (!attr->exclude_##context) {				\
@@ -560,17 +559,15 @@ static int evsel__add_modifiers(struct evsel *evsel, char *bf, size_t size)
 		MOD_PRINT(kernel, 'k');
 		MOD_PRINT(user, 'u');
 		MOD_PRINT(hv, 'h');
-		exclude_guest_default = true;
 	}
 
 	if (attr->precise_ip) {
 		if (!colon)
 			colon = ++r;
 		r += scnprintf(bf + r, size - r, "%.*s", attr->precise_ip, "ppp");
-		exclude_guest_default = true;
 	}
 
-	if (attr->exclude_host || attr->exclude_guest == exclude_guest_default) {
+	if (attr->exclude_host || attr->exclude_guest) {
 		MOD_PRINT(host, 'H');
 		MOD_PRINT(guest, 'G');
 	}
-- 
2.46.0.469.g59c65b2a67-goog


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 3/8] perf stat: Add --exclude-guest option
  2024-09-04  6:41 [RFC/PATCHSET 0/8] perf tools: Do not set attr.exclude_guest by default (v2) Namhyung Kim
  2024-09-04  6:41 ` [PATCH 1/8] perf tools: Don't set attr.exclude_guest by default Namhyung Kim
  2024-09-04  6:41 ` [PATCH 2/8] perf tools: Simplify evsel__add_modifier() Namhyung Kim
@ 2024-09-04  6:41 ` Namhyung Kim
  2024-09-04  6:41 ` [PATCH 4/8] perf tools: Do not set exclude_guest for precise_ip Namhyung Kim
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 19+ messages in thread
From: Namhyung Kim @ 2024-09-04  6:41 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Ian Rogers, Kan Liang
  Cc: Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
	linux-perf-users, Ravi Bangoria, Mark Rutland, James Clark,
	Athira Rajeev, Kajol Jain, Thomas Richter, Atish Patra,
	Palmer Dabbelt, Mingwei Zhang

This option is to support the old behavior of setting exclude_guest by
default.  Now it doesn't set the bit so users want the old behavior can
use this option.

  $ perf stat true

   Performance counter stats for 'true':

                0.86 msec task-clock:u                     #    0.443 CPUs utilized
                   0      context-switches:u               #    0.000 /sec
                   0      cpu-migrations:u                 #    0.000 /sec
                  49      page-faults:u                    #   56.889 K/sec
                 ...

  $ perf stat --exclude-guest true

   Performance counter stats for 'true':

                0.79 msec task-clock:Hu                    #    0.490 CPUs utilized
                   0      context-switches:Hu              #    0.000 /sec
                   0      cpu-migrations:Hu                #    0.000 /sec
                  49      page-faults:Hu                   #   62.078 K/sec
                 ...

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/Documentation/perf-stat.txt | 7 +++++++
 tools/perf/builtin-stat.c              | 2 ++
 2 files changed, 9 insertions(+)

diff --git a/tools/perf/Documentation/perf-stat.txt b/tools/perf/Documentation/perf-stat.txt
index 2bc06367248691dd..d28d8370a856598f 100644
--- a/tools/perf/Documentation/perf-stat.txt
+++ b/tools/perf/Documentation/perf-stat.txt
@@ -382,6 +382,13 @@ color the metric's computed value.
 Don't print output, warnings or messages. This is useful with perf stat
 record below to only write data to the perf.data file.
 
+--exclude-guest::
+Don't count event in the guest mode.  It was the old behavior but the
+default is changed to count guest events also.  Use this option if you
+want the old behavior (host only).  Note that this option needs to be
+before other events in case you added -e/--event option in the command
+line.
+
 STAT RECORD
 -----------
 Stores stat data into perf data file.
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index cf985cdb9a6ee588..8b9889873d3efe40 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -2492,6 +2492,8 @@ int cmd_stat(int argc, const char **argv)
 		OPT_BOOLEAN_FLAG(0, "all-user", &stat_config.all_user,
 				"Configure all used events to run in user space.",
 				PARSE_OPT_EXCLUSIVE),
+		OPT_BOOLEAN(0, "exclude-guest", &exclude_HG_default,
+			"Don't count events in the guest mode"),
 		OPT_BOOLEAN(0, "percore-show-thread", &stat_config.percore_show_thread,
 			"Use with 'percore' event qualifier to show the event "
 			"counts of one hardware thread by sum up total hardware "
-- 
2.46.0.469.g59c65b2a67-goog


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 4/8] perf tools: Do not set exclude_guest for precise_ip
  2024-09-04  6:41 [RFC/PATCHSET 0/8] perf tools: Do not set attr.exclude_guest by default (v2) Namhyung Kim
                   ` (2 preceding siblings ...)
  2024-09-04  6:41 ` [PATCH 3/8] perf stat: Add --exclude-guest option Namhyung Kim
@ 2024-09-04  6:41 ` Namhyung Kim
  2024-09-04  6:41 ` [PATCH 5/8] perf tools: Detect missing kernel features properly Namhyung Kim
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 19+ messages in thread
From: Namhyung Kim @ 2024-09-04  6:41 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Ian Rogers, Kan Liang
  Cc: Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
	linux-perf-users, Ravi Bangoria, Mark Rutland, James Clark,
	Athira Rajeev, Kajol Jain, Thomas Richter, Atish Patra,
	Palmer Dabbelt, Mingwei Zhang

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@kernel.org>
---
 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 9179bf3084c302c1..900107a171ee9a09 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 ab73b3d45f0467f9..4fa46ef7213c5d5b 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1732,10 +1732,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.0.469.g59c65b2a67-goog


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 5/8] perf tools: Detect missing kernel features properly
  2024-09-04  6:41 [RFC/PATCHSET 0/8] perf tools: Do not set attr.exclude_guest by default (v2) Namhyung Kim
                   ` (3 preceding siblings ...)
  2024-09-04  6:41 ` [PATCH 4/8] perf tools: Do not set exclude_guest for precise_ip Namhyung Kim
@ 2024-09-04  6:41 ` Namhyung Kim
  2024-09-04  6:41 ` [PATCH 6/8] perf tools: Separate exclude_hv fallback Namhyung Kim
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 19+ messages in thread
From: Namhyung Kim @ 2024-09-04  6:41 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Ian Rogers, Kan Liang
  Cc: Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
	linux-perf-users, Ravi Bangoria, Mark Rutland, James Clark,
	Athira Rajeev, Kajol Jain, Thomas Richter, Atish Patra,
	Palmer Dabbelt, Mingwei Zhang

The evsel__detect_missing_features() is to check if the attributes of
the evsel is supported or not.  But it checks the attribute based on the
given evsel, it might miss something if the attr doesn't have the bit or
give incorrect results if the event is special.

Also it maintains the order of the feature that was added to the kernel
which means it can assume older features should be supported once it
detects the current feature is working.  To minimized the confusion and
to accurately check the kernel features, I think it's better to use a
software event and go through all the features at once.

Also make the function static since it's only used in evsel.c.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/evsel.c | 345 +++++++++++++++++++++++++++++-----------
 tools/perf/util/evsel.h |   1 -
 2 files changed, 249 insertions(+), 97 deletions(-)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 57fe0f9b06f9a8bb..6456c28326144528 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -20,6 +20,7 @@
 #include <linux/zalloc.h>
 #include <sys/ioctl.h>
 #include <sys/resource.h>
+#include <sys/syscall.h>
 #include <sys/types.h>
 #include <dirent.h>
 #include <stdlib.h>
@@ -2135,120 +2136,272 @@ int evsel__prepare_open(struct evsel *evsel, struct perf_cpu_map *cpus,
 	return err;
 }
 
-bool evsel__detect_missing_features(struct evsel *evsel)
+static bool has_attr_feature(struct perf_event_attr *attr, unsigned long flags)
 {
+	int fd = syscall(SYS_perf_event_open, attr, /*pid=*/0, /*cpu=*/-1,
+			 /*group_fd=*/-1, flags);
+	close(fd);
+
+	if (fd < 0) {
+		attr->exclude_kernel = 1;
+
+		fd = syscall(SYS_perf_event_open, attr, /*pid=*/0, /*cpu=*/-1,
+			     /*group_fd=*/-1, flags);
+		close(fd);
+	}
+
+	if (fd < 0) {
+		attr->exclude_hv = 1;
+
+		fd = syscall(SYS_perf_event_open, attr, /*pid=*/0, /*cpu=*/-1,
+			     /*group_fd=*/-1, flags);
+		close(fd);
+	}
+
+	if (fd < 0) {
+		attr->exclude_guest = 1;
+
+		fd = syscall(SYS_perf_event_open, attr, /*pid=*/0, /*cpu=*/-1,
+			     /*group_fd=*/-1, flags);
+		close(fd);
+	}
+
+	attr->exclude_kernel = 0;
+	attr->exclude_guest = 0;
+	attr->exclude_hv = 0;
+
+	return fd >= 0;
+}
+
+static void evsel__detect_missing_brstack_features(struct evsel *evsel)
+{
+	static bool detection_done = false;
+	struct perf_event_attr attr = {
+		.type = evsel->core.attr.type,
+		.config = evsel->core.attr.config,
+		.disabled = 1,
+		.sample_type = PERF_SAMPLE_BRANCH_STACK,
+		.sample_period = 1000,
+	};
+	int old_errno;
+
+	if (detection_done)
+		return;
+
+	old_errno = errno;
+
 	/*
 	 * Must probe features in the order they were added to the
-	 * perf_event_attr interface.
+	 * perf_event_attr interface.  These are PMU specific limitation
+	 * so we can detect with the given hardware event and stop on the
+	 * first one succeeded.
 	 */
-	if (!perf_missing_features.branch_counters &&
-	    (evsel->core.attr.branch_sample_type & PERF_SAMPLE_BRANCH_COUNTERS)) {
-		perf_missing_features.branch_counters = true;
-		pr_debug2("switching off branch counters support\n");
-		return true;
-	} else if (!perf_missing_features.read_lost &&
-	    (evsel->core.attr.read_format & PERF_FORMAT_LOST)) {
-		perf_missing_features.read_lost = true;
-		pr_debug2("switching off PERF_FORMAT_LOST support\n");
+
+	/* Please add new feature detection here. */
+
+	attr.branch_sample_type = PERF_SAMPLE_BRANCH_COUNTERS;
+	if (has_attr_feature(&attr, /*flags=*/0))
+		goto found;
+	perf_missing_features.branch_counters = true;
+	pr_debug2("switching off branch counters support\n");
+
+	attr.branch_sample_type = PERF_SAMPLE_BRANCH_HW_INDEX;
+	if (has_attr_feature(&attr, /*flags=*/0))
+		goto found;
+	perf_missing_features.branch_hw_idx = true;
+	pr_debug2("switching off branch HW index support\n");
+
+	attr.branch_sample_type = PERF_SAMPLE_BRANCH_NO_CYCLES | PERF_SAMPLE_BRANCH_NO_FLAGS;
+	if (has_attr_feature(&attr, /*flags=*/0))
+		goto found;
+	perf_missing_features.lbr_flags = true;
+	pr_debug2_peo("switching off branch sample type no (cycles/flags)\n");
+
+found:
+	detection_done = true;
+	errno = old_errno;
+}
+
+static bool evsel__detect_missing_features(struct evsel *evsel)
+{
+	static bool detection_done = false;
+	struct perf_event_attr attr = {
+		.type = PERF_TYPE_SOFTWARE,
+		.config = PERF_COUNT_SW_TASK_CLOCK,
+		.disabled = 1,
+	};
+	int old_errno;
+
+	if (evsel__has_br_stack(evsel))
+		evsel__detect_missing_brstack_features(evsel);
+
+	if (detection_done)
+		goto check;
+
+	old_errno = errno;
+
+	/*
+	 * Must probe features in the order they were added to the
+	 * perf_event_attr interface.  These are kernel core limitation
+	 * not PMU-specific so we can detect with a software event and
+	 * stop on the first one succeeded.
+	 */
+
+	/* Please add new feature detection here. */
+
+	attr.read_format = PERF_FORMAT_LOST;
+	if (has_attr_feature(&attr, /*flags=*/0))
+		goto found;
+	perf_missing_features.read_lost = true;
+	pr_debug2("switching off PERF_FORMAT_LOST support\n");
+	attr.read_format = 0;
+
+	attr.sample_type = PERF_SAMPLE_WEIGHT_STRUCT;
+	if (has_attr_feature(&attr, /*flags=*/0))
+		goto found;
+	perf_missing_features.weight_struct = true;
+	pr_debug2("switching off weight struct support\n");
+	attr.sample_type = 0;
+
+	attr.sample_type = PERF_SAMPLE_CODE_PAGE_SIZE;
+	if (has_attr_feature(&attr, /*flags=*/0))
+		goto found;
+	perf_missing_features.code_page_size = true;
+	pr_debug2_peo("Kernel has no PERF_SAMPLE_CODE_PAGE_SIZE support\n");
+	attr.sample_type = 0;
+
+	attr.sample_type = PERF_SAMPLE_DATA_PAGE_SIZE;
+	if (has_attr_feature(&attr, /*flags=*/0))
+		goto found;
+	perf_missing_features.data_page_size = true;
+	pr_debug2_peo("Kernel has no PERF_SAMPLE_DATA_PAGE_SIZE support\n");
+	attr.sample_type = 0;
+
+	attr.cgroup = 1;
+	if (has_attr_feature(&attr, /*flags=*/0))
+		goto found;
+	perf_missing_features.cgroup = true;
+	pr_debug2_peo("Kernel has no cgroup sampling support\n");
+	attr.cgroup = 0;
+
+	attr.aux_output = 1;
+	if (has_attr_feature(&attr, /*flags=*/0))
+		goto found;
+	perf_missing_features.aux_output = true;
+	pr_debug2_peo("Kernel has no attr.aux_output support\n");
+	attr.aux_output = 0;
+
+	attr.bpf_event = 1;
+	if (has_attr_feature(&attr, /*flags=*/0))
+		goto found;
+	perf_missing_features.bpf = true;
+	pr_debug2_peo("switching off bpf_event\n");
+	attr.bpf_event = 0;
+
+	attr.ksymbol = 1;
+	if (has_attr_feature(&attr, /*flags=*/0))
+		goto found;
+	perf_missing_features.ksymbol = true;
+	pr_debug2_peo("switching off ksymbol\n");
+	attr.ksymbol = 0;
+
+	attr.write_backward = 1;
+	if (has_attr_feature(&attr, /*flags=*/0))
+		goto found;
+	perf_missing_features.write_backward = true;
+	pr_debug2_peo("switching off write_backward\n");
+	attr.write_backward = 0;
+
+	attr.use_clockid = 1;
+	attr.clockid = CLOCK_MONOTONIC;
+	if (has_attr_feature(&attr, /*flags=*/0))
+		goto found;
+	perf_missing_features.clockid = true;
+	pr_debug2_peo("switching off clockid\n");
+	attr.use_clockid = 0;
+	attr.clockid = 0;
+
+	if (has_attr_feature(&attr, /*flags=*/PERF_FLAG_FD_CLOEXEC))
+		goto found;
+	perf_missing_features.cloexec = true;
+	pr_debug2_peo("switching off cloexec flag\n");
+
+	attr.mmap2 = 1;
+	if (has_attr_feature(&attr, /*flags=*/0))
+		goto found;
+	perf_missing_features.mmap2 = true;
+	pr_debug2_peo("switching off mmap2\n");
+	attr.mmap2 = 0;
+
+	/* set this unconditionally? */
+	perf_missing_features.sample_id_all = true;
+	pr_debug2_peo("switching off sample_id_all\n");
+
+	attr.inherit = 1;
+	attr.read_format = PERF_FORMAT_GROUP;
+	if (has_attr_feature(&attr, /*flags=*/0))
+		goto found;
+	perf_missing_features.group_read = true;
+	pr_debug2_peo("switching off group read\n");
+	attr.inherit = 0;
+	attr.read_format = 0;
+
+found:
+	detection_done = true;
+	errno = old_errno;
+
+check:
+	if ((evsel->core.attr.branch_sample_type & PERF_SAMPLE_BRANCH_COUNTERS) &&
+	    perf_missing_features.branch_counters)
 		return true;
-	} else if (!perf_missing_features.weight_struct &&
-	    (evsel->core.attr.sample_type & PERF_SAMPLE_WEIGHT_STRUCT)) {
-		perf_missing_features.weight_struct = true;
-		pr_debug2("switching off weight struct support\n");
+
+	if ((evsel->core.attr.read_format & PERF_FORMAT_LOST) &&
+	    perf_missing_features.read_lost)
 		return true;
-	} else if (!perf_missing_features.code_page_size &&
-	    (evsel->core.attr.sample_type & PERF_SAMPLE_CODE_PAGE_SIZE)) {
-		perf_missing_features.code_page_size = true;
-		pr_debug2_peo("Kernel has no PERF_SAMPLE_CODE_PAGE_SIZE support, bailing out\n");
-		return false;
-	} else if (!perf_missing_features.data_page_size &&
-	    (evsel->core.attr.sample_type & PERF_SAMPLE_DATA_PAGE_SIZE)) {
-		perf_missing_features.data_page_size = true;
-		pr_debug2_peo("Kernel has no PERF_SAMPLE_DATA_PAGE_SIZE support, bailing out\n");
-		return false;
-	} else if (!perf_missing_features.cgroup && evsel->core.attr.cgroup) {
-		perf_missing_features.cgroup = true;
-		pr_debug2_peo("Kernel has no cgroup sampling support, bailing out\n");
-		return false;
-	} else if (!perf_missing_features.branch_hw_idx &&
-	    (evsel->core.attr.branch_sample_type & PERF_SAMPLE_BRANCH_HW_INDEX)) {
-		perf_missing_features.branch_hw_idx = true;
-		pr_debug2("switching off branch HW index support\n");
+
+	if ((evsel->core.attr.sample_type & PERF_SAMPLE_WEIGHT_STRUCT) &&
+	    perf_missing_features.weight_struct)
 		return true;
-	} else if (!perf_missing_features.aux_output && evsel->core.attr.aux_output) {
-		perf_missing_features.aux_output = true;
-		pr_debug2_peo("Kernel has no attr.aux_output support, bailing out\n");
-		return false;
-	} else if (!perf_missing_features.bpf && evsel->core.attr.bpf_event) {
-		perf_missing_features.bpf = true;
-		pr_debug2_peo("switching off bpf_event\n");
+
+	if (evsel->core.attr.use_clockid && evsel->core.attr.clockid != CLOCK_MONOTONIC &&
+	    !perf_missing_features.clockid) {
+		perf_missing_features.clockid_wrong = true;
 		return true;
-	} else if (!perf_missing_features.ksymbol && evsel->core.attr.ksymbol) {
-		perf_missing_features.ksymbol = true;
-		pr_debug2_peo("switching off ksymbol\n");
+	}
+
+	if (evsel->core.attr.use_clockid && perf_missing_features.clockid)
 		return true;
-	} else if (!perf_missing_features.write_backward && evsel->core.attr.write_backward) {
-		perf_missing_features.write_backward = true;
-		pr_debug2_peo("switching off write_backward\n");
-		return false;
-	} else if (!perf_missing_features.clockid_wrong && evsel->core.attr.use_clockid) {
-		perf_missing_features.clockid_wrong = true;
-		pr_debug2_peo("switching off clockid\n");
+
+	if ((evsel->open_flags & PERF_FLAG_FD_CLOEXEC) &&
+	    perf_missing_features.cloexec)
 		return true;
-	} else if (!perf_missing_features.clockid && evsel->core.attr.use_clockid) {
-		perf_missing_features.clockid = true;
-		pr_debug2_peo("switching off use_clockid\n");
+
+	if (evsel->core.attr.mmap2 && perf_missing_features.mmap2)
 		return true;
-	} else if (!perf_missing_features.cloexec && (evsel->open_flags & PERF_FLAG_FD_CLOEXEC)) {
-		perf_missing_features.cloexec = true;
-		pr_debug2_peo("switching off cloexec flag\n");
+
+	if ((evsel->core.attr.branch_sample_type & (PERF_SAMPLE_BRANCH_NO_FLAGS |
+						    PERF_SAMPLE_BRANCH_NO_CYCLES)) &&
+	    perf_missing_features.lbr_flags)
 		return true;
-	} else if (!perf_missing_features.mmap2 && evsel->core.attr.mmap2) {
-		perf_missing_features.mmap2 = true;
-		pr_debug2_peo("switching off mmap2\n");
+
+	if (evsel->core.attr.inherit && (evsel->core.attr.read_format & PERF_FORMAT_GROUP) &&
+	    perf_missing_features.group_read)
 		return true;
-	} else if (evsel->core.attr.exclude_guest || evsel->core.attr.exclude_host) {
-		if (evsel->pmu == NULL)
-			evsel->pmu = evsel__find_pmu(evsel);
-
-		if (evsel->pmu)
-			evsel->pmu->missing_features.exclude_guest = true;
-		else {
-			/* we cannot find PMU, disable attrs now */
-			evsel->core.attr.exclude_host = false;
-			evsel->core.attr.exclude_guest = false;
-		}
 
-		if (evsel->exclude_GH) {
-			pr_debug2_peo("PMU has no exclude_host/guest support, bailing out\n");
-			return false;
-		}
-		if (!perf_missing_features.exclude_guest) {
-			perf_missing_features.exclude_guest = true;
-			pr_debug2_peo("switching off exclude_guest, exclude_host\n");
-		}
+	if (evsel->core.attr.ksymbol && perf_missing_features.ksymbol)
 		return true;
-	} else if (!perf_missing_features.sample_id_all) {
-		perf_missing_features.sample_id_all = true;
-		pr_debug2_peo("switching off sample_id_all\n");
+
+	if (evsel->core.attr.bpf_event && perf_missing_features.bpf)
 		return true;
-	} else if (!perf_missing_features.lbr_flags &&
-			(evsel->core.attr.branch_sample_type &
-			 (PERF_SAMPLE_BRANCH_NO_CYCLES |
-			  PERF_SAMPLE_BRANCH_NO_FLAGS))) {
-		perf_missing_features.lbr_flags = true;
-		pr_debug2_peo("switching off branch sample type no (cycles/flags)\n");
+
+	if ((evsel->core.attr.branch_sample_type & PERF_SAMPLE_BRANCH_HW_INDEX) &&
+	    perf_missing_features.branch_hw_idx)
 		return true;
-	} else if (!perf_missing_features.group_read &&
-		    evsel->core.attr.inherit &&
-		   (evsel->core.attr.read_format & PERF_FORMAT_GROUP) &&
-		   evsel__is_group_leader(evsel)) {
-		perf_missing_features.group_read = true;
-		pr_debug2_peo("switching off group read\n");
+
+	if (evsel->core.attr.sample_id_all && perf_missing_features.sample_id_all)
 		return true;
-	} else {
-		return false;
-	}
+
+	return false;
 }
 
 static int evsel__open_cpu(struct evsel *evsel, struct perf_cpu_map *cpus,
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 15acf293e12af713..098aa0d8eeb65466 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -363,7 +363,6 @@ int evsel__open(struct evsel *evsel, struct perf_cpu_map *cpus,
 void evsel__close(struct evsel *evsel);
 int evsel__prepare_open(struct evsel *evsel, struct perf_cpu_map *cpus,
 		struct perf_thread_map *threads);
-bool evsel__detect_missing_features(struct evsel *evsel);
 
 bool evsel__precise_ip_fallback(struct evsel *evsel);
 
-- 
2.46.0.469.g59c65b2a67-goog


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 6/8] perf tools: Separate exclude_hv fallback
  2024-09-04  6:41 [RFC/PATCHSET 0/8] perf tools: Do not set attr.exclude_guest by default (v2) Namhyung Kim
                   ` (4 preceding siblings ...)
  2024-09-04  6:41 ` [PATCH 5/8] perf tools: Detect missing kernel features properly Namhyung Kim
@ 2024-09-04  6:41 ` Namhyung Kim
  2024-09-04  6:41 ` [PATCH 7/8] perf tools: Add fallback for exclude_guest Namhyung Kim
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 19+ messages in thread
From: Namhyung Kim @ 2024-09-04  6:41 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Ian Rogers, Kan Liang
  Cc: Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
	linux-perf-users, Ravi Bangoria, Mark Rutland, James Clark,
	Athira Rajeev, Kajol Jain, Thomas Richter, Atish Patra,
	Palmer Dabbelt, Mingwei Zhang

The exclude_hv was added to address branch stack samples on Intel PMUs.
As some other PMUs might not support that, let's separate the bit from
exclude_kernel to make sure it can add the bit only if required.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/evsel.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 6456c28326144528..0de0a72947db3f10 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -3389,10 +3389,16 @@ bool evsel__fallback(struct evsel *evsel, struct target *target, int err,
 		free(evsel->name);
 		evsel->name = new_name;
 		scnprintf(msg, msgsize, "kernel.perf_event_paranoid=%d, trying "
-			  "to fall back to excluding kernel and hypervisor "
-			  " samples", paranoid);
+			  "to fall back to excluding kernel samples", paranoid);
 		evsel->core.attr.exclude_kernel = 1;
-		evsel->core.attr.exclude_hv     = 1;
+
+		return true;
+	} else if (err == EACCES && !evsel->core.attr.exclude_hv &&
+		   (paranoid = perf_event_paranoid()) > 1) {
+		/* Intel branch stack requires exclude_hv */
+		scnprintf(msg, msgsize, "kernel.perf_event_paranoid=%d, trying "
+			  "to fall back to excluding hypervisor samples", paranoid);
+		evsel->core.attr.exclude_hv = 1;
 
 		return true;
 	}
-- 
2.46.0.469.g59c65b2a67-goog


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 7/8] perf tools: Add fallback for exclude_guest
  2024-09-04  6:41 [RFC/PATCHSET 0/8] perf tools: Do not set attr.exclude_guest by default (v2) Namhyung Kim
                   ` (5 preceding siblings ...)
  2024-09-04  6:41 ` [PATCH 6/8] perf tools: Separate exclude_hv fallback Namhyung Kim
@ 2024-09-04  6:41 ` Namhyung Kim
  2024-09-04 13:28   ` James Clark
  2024-09-04  6:41 ` [PATCH 8/8] perf tools: Check fallback error and order Namhyung Kim
  2024-09-04 16:36 ` [RFC/PATCHSET 0/8] perf tools: Do not set attr.exclude_guest by default (v2) Ian Rogers
  8 siblings, 1 reply; 19+ messages in thread
From: Namhyung Kim @ 2024-09-04  6:41 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Ian Rogers, Kan Liang
  Cc: Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
	linux-perf-users, Ravi Bangoria, Mark Rutland, James Clark,
	Athira Rajeev, Kajol Jain, Thomas Richter, Atish Patra,
	Palmer Dabbelt, Mingwei Zhang, James Clark

It seems Apple M1 PMU requires exclude_guest set and returns EOPNOTSUPP
if not.  Let's add a fallback so that it can work with default events.

Cc: Mark Rutland <mark.rutland@arm.com>
Cc: James Clark <james.clark@linaro.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/evsel.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 0de0a72947db3f10..8c4d70f7b2f5b880 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -3400,6 +3400,27 @@ bool evsel__fallback(struct evsel *evsel, struct target *target, int err,
 			  "to fall back to excluding hypervisor samples", paranoid);
 		evsel->core.attr.exclude_hv = 1;
 
+		return true;
+	} else if (err == EOPNOTSUPP && !evsel->core.attr.exclude_guest &&
+		   !evsel->exclude_GH) {
+		const char *name = evsel__name(evsel);
+		char *new_name;
+		const char *sep = ":";
+
+		/* Is there already the separator in the name. */
+		if (strchr(name, '/') ||
+		    (strchr(name, ':') && !evsel->is_libpfm_event))
+			sep = "";
+
+		if (asprintf(&new_name, "%s%su", name, sep) < 0)
+			return false;
+
+		free(evsel->name);
+		evsel->name = new_name;
+		/* Apple M1 requires exclude_guest */
+		scnprintf(msg, msgsize, "trying to fall back to excluding guest samples");
+		evsel->core.attr.exclude_guest = 1;
+
 		return true;
 	}
 
-- 
2.46.0.469.g59c65b2a67-goog


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 8/8] perf tools: Check fallback error and order
  2024-09-04  6:41 [RFC/PATCHSET 0/8] perf tools: Do not set attr.exclude_guest by default (v2) Namhyung Kim
                   ` (6 preceding siblings ...)
  2024-09-04  6:41 ` [PATCH 7/8] perf tools: Add fallback for exclude_guest Namhyung Kim
@ 2024-09-04  6:41 ` Namhyung Kim
  2024-09-04 16:19   ` Ian Rogers
  2024-09-04 16:36 ` [RFC/PATCHSET 0/8] perf tools: Do not set attr.exclude_guest by default (v2) Ian Rogers
  8 siblings, 1 reply; 19+ messages in thread
From: Namhyung Kim @ 2024-09-04  6:41 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Ian Rogers, Kan Liang
  Cc: Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
	linux-perf-users, Ravi Bangoria, Mark Rutland, James Clark,
	Athira Rajeev, Kajol Jain, Thomas Richter, Atish Patra,
	Palmer Dabbelt, Mingwei Zhang

The perf_event_open might fail due to various reasons, so blindly
reducing precise_ip level might not be the best way to deal with it.

It seems the kernel return -EOPNOTSUPP when PMU doesn't support the
given precise level.  Let's try again with the correct error code.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/evsel.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 8c4d70f7b2f5b880..0133c9ad3ce07a24 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -2565,9 +2565,6 @@ static int evsel__open_cpu(struct evsel *evsel, struct perf_cpu_map *cpus,
 	return 0;
 
 try_fallback:
-	if (evsel__precise_ip_fallback(evsel))
-		goto retry_open;
-
 	if (evsel__ignore_missing_thread(evsel, perf_cpu_map__nr(cpus),
 					 idx, threads, thread, err)) {
 		/* We just removed 1 thread, so lower the upper nthreads limit. */
@@ -2584,11 +2581,12 @@ static int evsel__open_cpu(struct evsel *evsel, struct perf_cpu_map *cpus,
 	if (err == -EMFILE && rlimit__increase_nofile(&set_rlimit))
 		goto retry_open;
 
-	if (err != -EINVAL || idx > 0 || thread > 0)
-		goto out_close;
+	if (err == -EOPNOTSUPP && evsel__precise_ip_fallback(evsel))
+		goto retry_open;
 
-	if (evsel__detect_missing_features(evsel))
+	if (err == -EINVAL && evsel__detect_missing_features(evsel))
 		goto fallback_missing_features;
+
 out_close:
 	if (err)
 		threads->err_thread = thread;
-- 
2.46.0.469.g59c65b2a67-goog


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/8] perf tools: Don't set attr.exclude_guest by default
  2024-09-04  6:41 ` [PATCH 1/8] perf tools: Don't set attr.exclude_guest by default Namhyung Kim
@ 2024-09-04 13:07   ` Arnaldo Carvalho de Melo
  2024-09-04 15:43     ` Namhyung Kim
  0 siblings, 1 reply; 19+ messages in thread
From: Arnaldo Carvalho de Melo @ 2024-09-04 13:07 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Ian Rogers, Kan Liang, Jiri Olsa, Adrian Hunter, Peter Zijlstra,
	Ingo Molnar, LKML, linux-perf-users, Ravi Bangoria, Mark Rutland,
	James Clark, Athira Rajeev, Kajol Jain, Thomas Richter,
	Atish Patra, Palmer Dabbelt, Mingwei Zhang, James Clark

On Tue, Sep 03, 2024 at 11:41:24PM -0700, Namhyung Kim wrote:
> The exclude_guest in the event attribute is to limit profiling in the
> host environment.  But I'm not sure why we want to set it by default
> cause we don't care about it in most cases and I feel like it just
> makes new PMU implementation complicated.
> 
> Of course it's useful for perf kvm command so I added the
> exclude_GH_default variable to preserve the old behavior for perf kvm
> and other commands like perf record and stat won't set the exclude bit.
> This is helpful for AMD IBS case since having exclude_guest bit will
> clear new feature bit due to the missing feature check logic.
> 
>   $ sysctl kernel.perf_event_paranoid
>   kernel.perf_event_paranoid = 0
> 
>   $ perf record -W -e ibs_op// -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
> 
> Maybe Apple M1 users will scream but actually the default event for
> perf record was converted to "cycles:P" which doesn't set the
> exclude_guest bit already.  So they need to specify the necessary
> modifier manually like "cycles:PH" and I think it's ok.

Would be really, really good to have a Tested-by: Someone # on an Apple M1

I'll reinstall my rk-3399-pc and try it on that hybrid aarch64 board.

I have access to Ampere Altra systems, will try there as well.

- Arnaldo
 
> Intestingly, I found it sets the exclude_bit if "u" modifier is used.
> I don't know why but it's neither intuitive nor consistent.  Let's
> remove the bit there too.
> 
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: James Clark <james.clark@linaro.org>
> Cc: Ravi Bangoria <ravi.bangoria@amd.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/builtin-kvm.c                   |  1 +
>  tools/perf/tests/attr/test-record-dummy-C0 |  2 +-
>  tools/perf/tests/parse-events.c            | 18 +++++++++---------
>  tools/perf/util/parse-events.c             |  2 +-
>  tools/perf/util/util.c                     | 10 ++++++++--
>  tools/perf/util/util.h                     |  3 +++
>  6 files changed, 23 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
> index 692267b1b7e8311f..ca94dd3de04d34a3 100644
> --- a/tools/perf/builtin-kvm.c
> +++ b/tools/perf/builtin-kvm.c
> @@ -2147,6 +2147,7 @@ int cmd_kvm(int argc, const char **argv)
>  						"buildid-list", "stat", NULL };
>  	const char *kvm_usage[] = { NULL, NULL };
>  
> +	exclude_HG_default = true;
>  	perf_host  = 0;
>  	perf_guest = 1;
>  
> diff --git a/tools/perf/tests/attr/test-record-dummy-C0 b/tools/perf/tests/attr/test-record-dummy-C0
> index 576ec48b3aafaa6a..8ce6f0a5df5b7013 100644
> --- a/tools/perf/tests/attr/test-record-dummy-C0
> +++ b/tools/perf/tests/attr/test-record-dummy-C0
> @@ -37,7 +37,7 @@ precise_ip=0
>  mmap_data=0
>  sample_id_all=1
>  exclude_host=0
> -exclude_guest=1
> +exclude_guest=0
>  exclude_callchain_kernel=0
>  exclude_callchain_user=0
>  mmap2=1
> diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
> index edc2adcf1baed195..9179bf3084c302c1 100644
> --- a/tools/perf/tests/parse-events.c
> +++ b/tools/perf/tests/parse-events.c
> @@ -932,7 +932,7 @@ static int test__group2(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);
> -			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);
>  			TEST_ASSERT_VAL("wrong leader", evsel__is_group_leader(evsel));
> @@ -947,7 +947,7 @@ static int test__group2(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);
> -			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);
>  			if (evsel__has_leader(evsel, leader))
> @@ -1072,7 +1072,7 @@ static int test__group3(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);
> -		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);
>  		TEST_ASSERT_VAL("wrong leader", evsel__is_group_leader(evsel));
> @@ -1222,7 +1222,7 @@ static int test__group5(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);
> -		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);
>  		TEST_ASSERT_VAL("wrong leader", evsel__is_group_leader(evsel));
> @@ -1437,7 +1437,7 @@ static int test__leader_sample1(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);
> -		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);
>  		TEST_ASSERT_VAL("wrong group name", !evsel->group_name);
> @@ -1453,7 +1453,7 @@ static int test__leader_sample1(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);
> -		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);
>  		TEST_ASSERT_VAL("wrong leader", evsel__has_leader(evsel, leader));
> @@ -1468,7 +1468,7 @@ static int test__leader_sample1(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);
> -		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);
>  		TEST_ASSERT_VAL("wrong group name", !evsel->group_name);
> @@ -1497,7 +1497,7 @@ static int test__leader_sample2(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);
> -		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);
>  		TEST_ASSERT_VAL("wrong group name", !evsel->group_name);
> @@ -1513,7 +1513,7 @@ static int test__leader_sample2(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);
> -		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);
>  		TEST_ASSERT_VAL("wrong group name", !evsel->group_name);
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index fab01ba54e34da7d..ab73b3d45f0467f9 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -1739,7 +1739,7 @@ static int parse_events__modifier_list(struct parse_events_state *parse_state,
>  		if (mod.user) {
>  			if (!exclude)
>  				exclude = eu = ek = eh = 1;
> -			if (!exclude_GH && !perf_guest)
> +			if (!exclude_GH && !perf_guest && exclude_HG_default)
>  				eG = 1;
>  			eu = 0;
>  		}
> diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
> index 9d55a13787ce3c05..7e3159faaa1991df 100644
> --- a/tools/perf/util/util.c
> +++ b/tools/perf/util/util.c
> @@ -78,17 +78,23 @@ bool sysctl__nmi_watchdog_enabled(void)
>  
>  bool test_attr__enabled;
>  
> +bool exclude_HG_default;
> +
>  bool perf_host  = true;
>  bool perf_guest = false;
>  
>  void event_attr_init(struct perf_event_attr *attr)
>  {
> +	/* to capture ABI version */
> +	attr->size = sizeof(*attr);
> +
> +	if (!exclude_HG_default)
> +		return;
> +
>  	if (!perf_host)
>  		attr->exclude_host  = 1;
>  	if (!perf_guest)
>  		attr->exclude_guest = 1;
> -	/* to capture ABI version */
> -	attr->size = sizeof(*attr);
>  }
>  
>  int mkdir_p(char *path, mode_t mode)
> diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
> index 9966c21aaf048479..d33ae883a54f2f2f 100644
> --- a/tools/perf/util/util.h
> +++ b/tools/perf/util/util.h
> @@ -21,6 +21,9 @@ extern const char perf_more_info_string[];
>  
>  extern const char *input_name;
>  
> +/* This will control if perf_{host,guest} will set attr.exclude_{host,guest}. */
> +extern bool exclude_HG_default;
> +
>  extern bool perf_host;
>  extern bool perf_guest;
>  
> -- 
> 2.46.0.469.g59c65b2a67-goog

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 7/8] perf tools: Add fallback for exclude_guest
  2024-09-04  6:41 ` [PATCH 7/8] perf tools: Add fallback for exclude_guest Namhyung Kim
@ 2024-09-04 13:28   ` James Clark
  2024-09-04 13:29     ` James Clark
  2024-09-04 13:36     ` James Clark
  0 siblings, 2 replies; 19+ messages in thread
From: James Clark @ 2024-09-04 13:28 UTC (permalink / raw)
  To: Namhyung Kim, Arnaldo Carvalho de Melo
  Cc: Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
	linux-perf-users, Ravi Bangoria, Mark Rutland, James Clark,
	Athira Rajeev, Kajol Jain, Thomas Richter, Atish Patra,
	Palmer Dabbelt, Mingwei Zhang, Ian Rogers, Kan Liang



On 04/09/2024 7:41 am, Namhyung Kim wrote:
> It seems Apple M1 PMU requires exclude_guest set and returns EOPNOTSUPP
> if not.  Let's add a fallback so that it can work with default events.
> 
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: James Clark <james.clark@linaro.org>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>   tools/perf/util/evsel.c | 21 +++++++++++++++++++++
>   1 file changed, 21 insertions(+)
> 
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 0de0a72947db3f10..8c4d70f7b2f5b880 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -3400,6 +3400,27 @@ bool evsel__fallback(struct evsel *evsel, struct target *target, int err,
>   			  "to fall back to excluding hypervisor samples", paranoid);
>   		evsel->core.attr.exclude_hv = 1;
>   
> +		return true;
> +	} else if (err == EOPNOTSUPP && !evsel->core.attr.exclude_guest &&
> +		   !evsel->exclude_GH) {
> +		const char *name = evsel__name(evsel);
> +		char *new_name;
> +		const char *sep = ":";
> +
> +		/* Is there already the separator in the name. */
> +		if (strchr(name, '/') ||
> +		    (strchr(name, ':') && !evsel->is_libpfm_event))
> +			sep = "";
> +
> +		if (asprintf(&new_name, "%s%su", name, sep) < 0)
> +			return false;
> +
> +		free(evsel->name);
> +		evsel->name = new_name;
> +		/* Apple M1 requires exclude_guest */
> +		scnprintf(msg, msgsize, "trying to fall back to excluding guest samples");
> +		evsel->core.attr.exclude_guest = 1;
> +
>   		return true;
>   	}
>   

Not sure if this is working, for some reason it doesn't try the 
fallback. With exclude guest made mandatory in the Arm PMU, then:

  $ perf stat -e cycles -vvv -- true

   Control descriptor is not initialized
   Opening: cycles
   ------------------------------------------------------------
   perf_event_attr:
     type                             0 (PERF_TYPE_HARDWARE)
     size                             136
     config                           0xb00000000
     sample_type                      IDENTIFIER
     read_format                      TOTAL_TIME_ENABLED|TOTAL_TIME_RUNNING
     disabled                         1
     inherit                          1
     enable_on_exec                   1
   ------------------------------------------------------------
   sys_perf_event_open: pid 698  cpu -1  group_fd -1  flags 0x8
   sys_perf_event_open failed, error -95
   Warning:
   cycles event is not supported by the kernel.
   Opening: cycles
   ------------------------------------------------------------
   perf_event_attr:
     type                             0 (PERF_TYPE_HARDWARE)
     size                             136
     config                           0xa00000000
     sample_type                      IDENTIFIER
     read_format                      TOTAL_TIME_ENABLED|TOTAL_TIME_RUNNING
     disabled                         1
     inherit                          1
     enable_on_exec                   1
   ------------------------------------------------------------
   sys_perf_event_open: pid 698  cpu -1  group_fd -1  flags 0x8
   sys_perf_event_open failed, error -95
   Warning:
   cycles event is not supported by the kernel.
   failed to read counter cycles
   failed to read counter cycles

    Performance counter stats for 'true':

      <not supported>      armv8_cortex_a53/cycles/ 

      <not supported>      armv8_cortex_a57/cycles/ 




Other than that, all the tests are passing on Juno (without the 
exclude_guest requirement).

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 7/8] perf tools: Add fallback for exclude_guest
  2024-09-04 13:28   ` James Clark
@ 2024-09-04 13:29     ` James Clark
  2024-09-04 13:36     ` James Clark
  1 sibling, 0 replies; 19+ messages in thread
From: James Clark @ 2024-09-04 13:29 UTC (permalink / raw)
  To: Namhyung Kim, Arnaldo Carvalho de Melo
  Cc: Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
	linux-perf-users, Ravi Bangoria, Mark Rutland, James Clark,
	Athira Rajeev, Kajol Jain, Thomas Richter, Atish Patra,
	Palmer Dabbelt, Mingwei Zhang, Ian Rogers, Kan Liang



On 04/09/2024 2:28 pm, James Clark wrote:
> 
> 
> On 04/09/2024 7:41 am, Namhyung Kim wrote:
>> It seems Apple M1 PMU requires exclude_guest set and returns EOPNOTSUPP
>> if not.  Let's add a fallback so that it can work with default events.
>>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: James Clark <james.clark@linaro.org>
>> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>> ---
>>   tools/perf/util/evsel.c | 21 +++++++++++++++++++++
>>   1 file changed, 21 insertions(+)
>>
>> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
>> index 0de0a72947db3f10..8c4d70f7b2f5b880 100644
>> --- a/tools/perf/util/evsel.c
>> +++ b/tools/perf/util/evsel.c
>> @@ -3400,6 +3400,27 @@ bool evsel__fallback(struct evsel *evsel, 
>> struct target *target, int err,
>>                 "to fall back to excluding hypervisor samples", 
>> paranoid);
>>           evsel->core.attr.exclude_hv = 1;
>> +        return true;
>> +    } else if (err == EOPNOTSUPP && !evsel->core.attr.exclude_guest &&
>> +           !evsel->exclude_GH) {
>> +        const char *name = evsel__name(evsel);
>> +        char *new_name;
>> +        const char *sep = ":";
>> +
>> +        /* Is there already the separator in the name. */
>> +        if (strchr(name, '/') ||
>> +            (strchr(name, ':') && !evsel->is_libpfm_event))
>> +            sep = "";
>> +
>> +        if (asprintf(&new_name, "%s%su", name, sep) < 0)
>> +            return false;
>> +
>> +        free(evsel->name);
>> +        evsel->name = new_name;
>> +        /* Apple M1 requires exclude_guest */
>> +        scnprintf(msg, msgsize, "trying to fall back to excluding 
>> guest samples");
>> +        evsel->core.attr.exclude_guest = 1;
>> +
>>           return true;
>>       }
> 
> Not sure if this is working, for some reason it doesn't try the 
> fallback. With exclude guest made mandatory in the Arm PMU, then:
> 
>   $ perf stat -e cycles -vvv -- true
> 
>    Control descriptor is not initialized
>    Opening: cycles
>    ------------------------------------------------------------
>    perf_event_attr:
>      type                             0 (PERF_TYPE_HARDWARE)
>      size                             136
>      config                           0xb00000000
>      sample_type                      IDENTIFIER
>      read_format                      TOTAL_TIME_ENABLED|TOTAL_TIME_RUNNING
>      disabled                         1
>      inherit                          1
>      enable_on_exec                   1
>    ------------------------------------------------------------
>    sys_perf_event_open: pid 698  cpu -1  group_fd -1  flags 0x8
>    sys_perf_event_open failed, error -95
>    Warning:
>    cycles event is not supported by the kernel.
>    Opening: cycles
>    ------------------------------------------------------------
>    perf_event_attr:
>      type                             0 (PERF_TYPE_HARDWARE)
>      size                             136
>      config                           0xa00000000
>      sample_type                      IDENTIFIER
>      read_format                      TOTAL_TIME_ENABLED|TOTAL_TIME_RUNNING
>      disabled                         1
>      inherit                          1
>      enable_on_exec                   1
>    ------------------------------------------------------------
>    sys_perf_event_open: pid 698  cpu -1  group_fd -1  flags 0x8
>    sys_perf_event_open failed, error -95
>    Warning:
>    cycles event is not supported by the kernel.
>    failed to read counter cycles
>    failed to read counter cycles
> 
>     Performance counter stats for 'true':
> 
>       <not supported>      armv8_cortex_a53/cycles/
>       <not supported>      armv8_cortex_a57/cycles/
> 
> 
> 
> Other than that, all the tests are passing on Juno (without the 
> exclude_guest requirement).

Sorry one other thing, I think this commit would also need to come 
before the change to the exclude_guest default to keep bisect working.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 7/8] perf tools: Add fallback for exclude_guest
  2024-09-04 13:28   ` James Clark
  2024-09-04 13:29     ` James Clark
@ 2024-09-04 13:36     ` James Clark
  2024-09-04 15:52       ` Namhyung Kim
  1 sibling, 1 reply; 19+ messages in thread
From: James Clark @ 2024-09-04 13:36 UTC (permalink / raw)
  To: Namhyung Kim, Arnaldo Carvalho de Melo
  Cc: Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
	linux-perf-users, Ravi Bangoria, Mark Rutland, James Clark,
	Athira Rajeev, Kajol Jain, Thomas Richter, Atish Patra,
	Palmer Dabbelt, Mingwei Zhang, Ian Rogers, Kan Liang



On 04/09/2024 2:28 pm, James Clark wrote:
> 
> 
> On 04/09/2024 7:41 am, Namhyung Kim wrote:
>> It seems Apple M1 PMU requires exclude_guest set and returns EOPNOTSUPP
>> if not.  Let's add a fallback so that it can work with default events.
>>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: James Clark <james.clark@linaro.org>
>> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>> ---
>>   tools/perf/util/evsel.c | 21 +++++++++++++++++++++
>>   1 file changed, 21 insertions(+)
>>
>> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
>> index 0de0a72947db3f10..8c4d70f7b2f5b880 100644
>> --- a/tools/perf/util/evsel.c
>> +++ b/tools/perf/util/evsel.c
>> @@ -3400,6 +3400,27 @@ bool evsel__fallback(struct evsel *evsel, 
>> struct target *target, int err,
>>                 "to fall back to excluding hypervisor samples", 
>> paranoid);
>>           evsel->core.attr.exclude_hv = 1;
>> +        return true;
>> +    } else if (err == EOPNOTSUPP && !evsel->core.attr.exclude_guest &&
>> +           !evsel->exclude_GH) {
>> +        const char *name = evsel__name(evsel);
>> +        char *new_name;
>> +        const char *sep = ":";
>> +
>> +        /* Is there already the separator in the name. */
>> +        if (strchr(name, '/') ||
>> +            (strchr(name, ':') && !evsel->is_libpfm_event))
>> +            sep = "";
>> +
>> +        if (asprintf(&new_name, "%s%su", name, sep) < 0)
>> +            return false;
>> +
>> +        free(evsel->name);
>> +        evsel->name = new_name;
>> +        /* Apple M1 requires exclude_guest */
>> +        scnprintf(msg, msgsize, "trying to fall back to excluding 
>> guest samples");
>> +        evsel->core.attr.exclude_guest = 1;
>> +
>>           return true;
>>       }
> 
> Not sure if this is working, for some reason it doesn't try the 
> fallback. With exclude guest made mandatory in the Arm PMU, then:
> 

Looks like you change this, but it's not obvious why the stat behavior 
is different to perf record anyway:

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 8b9889873d3e..6f2ee3032f5f 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -639,7 +639,7 @@ static enum counter_recovery 
stat_handle_error(struct evsel *counter)
          * (behavior changed with commit b0a873e).
          */
         if (errno == EINVAL || errno == ENOSYS ||
-           errno == ENOENT || errno == EOPNOTSUPP ||
+           errno == ENOENT ||
             errno == ENXIO) {
                 if (verbose > 0)
                         ui__warning("%s event is not supported by the 
kernel.\n",



^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/8] perf tools: Don't set attr.exclude_guest by default
  2024-09-04 13:07   ` Arnaldo Carvalho de Melo
@ 2024-09-04 15:43     ` Namhyung Kim
  0 siblings, 0 replies; 19+ messages in thread
From: Namhyung Kim @ 2024-09-04 15:43 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Ian Rogers, Kan Liang, Jiri Olsa, Adrian Hunter, Peter Zijlstra,
	Ingo Molnar, LKML, linux-perf-users, Ravi Bangoria, Mark Rutland,
	James Clark, Athira Rajeev, Kajol Jain, Thomas Richter,
	Atish Patra, Palmer Dabbelt, Mingwei Zhang, James Clark

Hi Arnaldo,

On Wed, Sep 4, 2024 at 6:07 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
>
> On Tue, Sep 03, 2024 at 11:41:24PM -0700, Namhyung Kim wrote:
> > The exclude_guest in the event attribute is to limit profiling in the
> > host environment.  But I'm not sure why we want to set it by default
> > cause we don't care about it in most cases and I feel like it just
> > makes new PMU implementation complicated.
> >
> > Of course it's useful for perf kvm command so I added the
> > exclude_GH_default variable to preserve the old behavior for perf kvm
> > and other commands like perf record and stat won't set the exclude bit.
> > This is helpful for AMD IBS case since having exclude_guest bit will
> > clear new feature bit due to the missing feature check logic.
> >
> >   $ sysctl kernel.perf_event_paranoid
> >   kernel.perf_event_paranoid = 0
> >
> >   $ perf record -W -e ibs_op// -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
> >
> > Maybe Apple M1 users will scream but actually the default event for
> > perf record was converted to "cycles:P" which doesn't set the
> > exclude_guest bit already.  So they need to specify the necessary
> > modifier manually like "cycles:PH" and I think it's ok.
>
> Would be really, really good to have a Tested-by: Someone # on an Apple M1

Yep, and other non-x86 platforms too.

>
> I'll reinstall my rk-3399-pc and try it on that hybrid aarch64 board.
>
> I have access to Ampere Altra systems, will try there as well.

Great, thanks!
Namhyung

>
> - Arnaldo
>
> > Intestingly, I found it sets the exclude_bit if "u" modifier is used.
> > I don't know why but it's neither intuitive nor consistent.  Let's
> > remove the bit there too.
> >
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: James Clark <james.clark@linaro.org>
> > Cc: Ravi Bangoria <ravi.bangoria@amd.com>
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/builtin-kvm.c                   |  1 +
> >  tools/perf/tests/attr/test-record-dummy-C0 |  2 +-
> >  tools/perf/tests/parse-events.c            | 18 +++++++++---------
> >  tools/perf/util/parse-events.c             |  2 +-
> >  tools/perf/util/util.c                     | 10 ++++++++--
> >  tools/perf/util/util.h                     |  3 +++
> >  6 files changed, 23 insertions(+), 13 deletions(-)
> >
> > diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
> > index 692267b1b7e8311f..ca94dd3de04d34a3 100644
> > --- a/tools/perf/builtin-kvm.c
> > +++ b/tools/perf/builtin-kvm.c
> > @@ -2147,6 +2147,7 @@ int cmd_kvm(int argc, const char **argv)
> >                                               "buildid-list", "stat", NULL };
> >       const char *kvm_usage[] = { NULL, NULL };
> >
> > +     exclude_HG_default = true;
> >       perf_host  = 0;
> >       perf_guest = 1;
> >
> > diff --git a/tools/perf/tests/attr/test-record-dummy-C0 b/tools/perf/tests/attr/test-record-dummy-C0
> > index 576ec48b3aafaa6a..8ce6f0a5df5b7013 100644
> > --- a/tools/perf/tests/attr/test-record-dummy-C0
> > +++ b/tools/perf/tests/attr/test-record-dummy-C0
> > @@ -37,7 +37,7 @@ precise_ip=0
> >  mmap_data=0
> >  sample_id_all=1
> >  exclude_host=0
> > -exclude_guest=1
> > +exclude_guest=0
> >  exclude_callchain_kernel=0
> >  exclude_callchain_user=0
> >  mmap2=1
> > diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
> > index edc2adcf1baed195..9179bf3084c302c1 100644
> > --- a/tools/perf/tests/parse-events.c
> > +++ b/tools/perf/tests/parse-events.c
> > @@ -932,7 +932,7 @@ static int test__group2(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);
> > -                     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);
> >                       TEST_ASSERT_VAL("wrong leader", evsel__is_group_leader(evsel));
> > @@ -947,7 +947,7 @@ static int test__group2(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);
> > -                     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);
> >                       if (evsel__has_leader(evsel, leader))
> > @@ -1072,7 +1072,7 @@ static int test__group3(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);
> > -             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);
> >               TEST_ASSERT_VAL("wrong leader", evsel__is_group_leader(evsel));
> > @@ -1222,7 +1222,7 @@ static int test__group5(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);
> > -             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);
> >               TEST_ASSERT_VAL("wrong leader", evsel__is_group_leader(evsel));
> > @@ -1437,7 +1437,7 @@ static int test__leader_sample1(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);
> > -             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);
> >               TEST_ASSERT_VAL("wrong group name", !evsel->group_name);
> > @@ -1453,7 +1453,7 @@ static int test__leader_sample1(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);
> > -             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);
> >               TEST_ASSERT_VAL("wrong leader", evsel__has_leader(evsel, leader));
> > @@ -1468,7 +1468,7 @@ static int test__leader_sample1(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);
> > -             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);
> >               TEST_ASSERT_VAL("wrong group name", !evsel->group_name);
> > @@ -1497,7 +1497,7 @@ static int test__leader_sample2(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);
> > -             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);
> >               TEST_ASSERT_VAL("wrong group name", !evsel->group_name);
> > @@ -1513,7 +1513,7 @@ static int test__leader_sample2(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);
> > -             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);
> >               TEST_ASSERT_VAL("wrong group name", !evsel->group_name);
> > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> > index fab01ba54e34da7d..ab73b3d45f0467f9 100644
> > --- a/tools/perf/util/parse-events.c
> > +++ b/tools/perf/util/parse-events.c
> > @@ -1739,7 +1739,7 @@ static int parse_events__modifier_list(struct parse_events_state *parse_state,
> >               if (mod.user) {
> >                       if (!exclude)
> >                               exclude = eu = ek = eh = 1;
> > -                     if (!exclude_GH && !perf_guest)
> > +                     if (!exclude_GH && !perf_guest && exclude_HG_default)
> >                               eG = 1;
> >                       eu = 0;
> >               }
> > diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
> > index 9d55a13787ce3c05..7e3159faaa1991df 100644
> > --- a/tools/perf/util/util.c
> > +++ b/tools/perf/util/util.c
> > @@ -78,17 +78,23 @@ bool sysctl__nmi_watchdog_enabled(void)
> >
> >  bool test_attr__enabled;
> >
> > +bool exclude_HG_default;
> > +
> >  bool perf_host  = true;
> >  bool perf_guest = false;
> >
> >  void event_attr_init(struct perf_event_attr *attr)
> >  {
> > +     /* to capture ABI version */
> > +     attr->size = sizeof(*attr);
> > +
> > +     if (!exclude_HG_default)
> > +             return;
> > +
> >       if (!perf_host)
> >               attr->exclude_host  = 1;
> >       if (!perf_guest)
> >               attr->exclude_guest = 1;
> > -     /* to capture ABI version */
> > -     attr->size = sizeof(*attr);
> >  }
> >
> >  int mkdir_p(char *path, mode_t mode)
> > diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
> > index 9966c21aaf048479..d33ae883a54f2f2f 100644
> > --- a/tools/perf/util/util.h
> > +++ b/tools/perf/util/util.h
> > @@ -21,6 +21,9 @@ extern const char perf_more_info_string[];
> >
> >  extern const char *input_name;
> >
> > +/* This will control if perf_{host,guest} will set attr.exclude_{host,guest}. */
> > +extern bool exclude_HG_default;
> > +
> >  extern bool perf_host;
> >  extern bool perf_guest;
> >
> > --
> > 2.46.0.469.g59c65b2a67-goog

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 7/8] perf tools: Add fallback for exclude_guest
  2024-09-04 13:36     ` James Clark
@ 2024-09-04 15:52       ` Namhyung Kim
  0 siblings, 0 replies; 19+ messages in thread
From: Namhyung Kim @ 2024-09-04 15:52 UTC (permalink / raw)
  To: James Clark
  Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Adrian Hunter,
	Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users,
	Ravi Bangoria, Mark Rutland, James Clark, Athira Rajeev,
	Kajol Jain, Thomas Richter, Atish Patra, Palmer Dabbelt,
	Mingwei Zhang, Ian Rogers, Kan Liang

Hello James,

On Wed, Sep 4, 2024 at 6:36 AM James Clark <james.clark@linaro.org> wrote:
>
>
>
> On 04/09/2024 2:28 pm, James Clark wrote:
> >
> >
> > On 04/09/2024 7:41 am, Namhyung Kim wrote:
> >> It seems Apple M1 PMU requires exclude_guest set and returns EOPNOTSUPP
> >> if not.  Let's add a fallback so that it can work with default events.
> >>
> >> Cc: Mark Rutland <mark.rutland@arm.com>
> >> Cc: James Clark <james.clark@linaro.org>
> >> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> >> ---
> >>   tools/perf/util/evsel.c | 21 +++++++++++++++++++++
> >>   1 file changed, 21 insertions(+)
> >>
> >> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> >> index 0de0a72947db3f10..8c4d70f7b2f5b880 100644
> >> --- a/tools/perf/util/evsel.c
> >> +++ b/tools/perf/util/evsel.c
> >> @@ -3400,6 +3400,27 @@ bool evsel__fallback(struct evsel *evsel,
> >> struct target *target, int err,
> >>                 "to fall back to excluding hypervisor samples",
> >> paranoid);
> >>           evsel->core.attr.exclude_hv = 1;
> >> +        return true;
> >> +    } else if (err == EOPNOTSUPP && !evsel->core.attr.exclude_guest &&
> >> +           !evsel->exclude_GH) {
> >> +        const char *name = evsel__name(evsel);
> >> +        char *new_name;
> >> +        const char *sep = ":";
> >> +
> >> +        /* Is there already the separator in the name. */
> >> +        if (strchr(name, '/') ||
> >> +            (strchr(name, ':') && !evsel->is_libpfm_event))
> >> +            sep = "";
> >> +
> >> +        if (asprintf(&new_name, "%s%su", name, sep) < 0)
> >> +            return false;
> >> +
> >> +        free(evsel->name);
> >> +        evsel->name = new_name;
> >> +        /* Apple M1 requires exclude_guest */
> >> +        scnprintf(msg, msgsize, "trying to fall back to excluding
> >> guest samples");
> >> +        evsel->core.attr.exclude_guest = 1;
> >> +
> >>           return true;
> >>       }
> >
> > Not sure if this is working, for some reason it doesn't try the
> > fallback. With exclude guest made mandatory in the Arm PMU, then:
> >
>
> Looks like you change this, but it's not obvious why the stat behavior
> is different to perf record anyway:

Right, I think we should consolidate the open code to be used by
both perf record and perf stat.  I'll reorder my patchset to have
exclude_guest fallback first.

Anyway thanks for the test and the fix.  With this, perf stat and
perf record work well on your setup?

Thanks,
Namhyung

>
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 8b9889873d3e..6f2ee3032f5f 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -639,7 +639,7 @@ static enum counter_recovery
> stat_handle_error(struct evsel *counter)
>           * (behavior changed with commit b0a873e).
>           */
>          if (errno == EINVAL || errno == ENOSYS ||
> -           errno == ENOENT || errno == EOPNOTSUPP ||
> +           errno == ENOENT ||
>              errno == ENXIO) {
>                  if (verbose > 0)
>                          ui__warning("%s event is not supported by the
> kernel.\n",
>
>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 8/8] perf tools: Check fallback error and order
  2024-09-04  6:41 ` [PATCH 8/8] perf tools: Check fallback error and order Namhyung Kim
@ 2024-09-04 16:19   ` Ian Rogers
  2024-09-04 18:15     ` Namhyung Kim
  0 siblings, 1 reply; 19+ messages in thread
From: Ian Rogers @ 2024-09-04 16:19 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, Kan Liang, Jiri Olsa, Adrian Hunter,
	Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users,
	Ravi Bangoria, Mark Rutland, James Clark, Athira Rajeev,
	Kajol Jain, Thomas Richter, Atish Patra, Palmer Dabbelt,
	Mingwei Zhang

On Tue, Sep 3, 2024 at 11:41 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> The perf_event_open might fail due to various reasons, so blindly
> reducing precise_ip level might not be the best way to deal with it.
>
> It seems the kernel return -EOPNOTSUPP when PMU doesn't support the
> given precise level.  Let's try again with the correct error code.

We also have pmu's max_precise:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/pmu.h?h=perf-tools-next#n91
The reducing the precision approach was iirc taken for AMD who will
forward some precise events to IBS, but the max_precise on the cpu PMU
is 0. I think because of this, reducing the precision below
evsel->pmu->max_precise shouldn't be necessary and another fallback
may help better.

Thanks,
Ian

> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/evsel.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 8c4d70f7b2f5b880..0133c9ad3ce07a24 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -2565,9 +2565,6 @@ static int evsel__open_cpu(struct evsel *evsel, struct perf_cpu_map *cpus,
>         return 0;
>
>  try_fallback:
> -       if (evsel__precise_ip_fallback(evsel))
> -               goto retry_open;
> -
>         if (evsel__ignore_missing_thread(evsel, perf_cpu_map__nr(cpus),
>                                          idx, threads, thread, err)) {
>                 /* We just removed 1 thread, so lower the upper nthreads limit. */
> @@ -2584,11 +2581,12 @@ static int evsel__open_cpu(struct evsel *evsel, struct perf_cpu_map *cpus,
>         if (err == -EMFILE && rlimit__increase_nofile(&set_rlimit))
>                 goto retry_open;
>
> -       if (err != -EINVAL || idx > 0 || thread > 0)
> -               goto out_close;
> +       if (err == -EOPNOTSUPP && evsel__precise_ip_fallback(evsel))
> +               goto retry_open;
>
> -       if (evsel__detect_missing_features(evsel))
> +       if (err == -EINVAL && evsel__detect_missing_features(evsel))
>                 goto fallback_missing_features;
> +
>  out_close:
>         if (err)
>                 threads->err_thread = thread;
> --
> 2.46.0.469.g59c65b2a67-goog
>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFC/PATCHSET 0/8] perf tools: Do not set attr.exclude_guest by default (v2)
  2024-09-04  6:41 [RFC/PATCHSET 0/8] perf tools: Do not set attr.exclude_guest by default (v2) Namhyung Kim
                   ` (7 preceding siblings ...)
  2024-09-04  6:41 ` [PATCH 8/8] perf tools: Check fallback error and order Namhyung Kim
@ 2024-09-04 16:36 ` Ian Rogers
  2024-09-04 17:46   ` Namhyung Kim
  8 siblings, 1 reply; 19+ messages in thread
From: Ian Rogers @ 2024-09-04 16:36 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, Kan Liang, Jiri Olsa, Adrian Hunter,
	Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users,
	Ravi Bangoria, Mark Rutland, James Clark, Athira Rajeev,
	Kajol Jain, Thomas Richter, Atish Patra, Palmer Dabbelt,
	Mingwei Zhang

On Tue, Sep 3, 2024 at 11:41 PM Namhyung Kim <namhyung@kernel.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
>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFC/PATCHSET 0/8] perf tools: Do not set attr.exclude_guest by default (v2)
  2024-09-04 16:36 ` [RFC/PATCHSET 0/8] perf tools: Do not set attr.exclude_guest by default (v2) Ian Rogers
@ 2024-09-04 17:46   ` Namhyung Kim
  0 siblings, 0 replies; 19+ messages in thread
From: Namhyung Kim @ 2024-09-04 17:46 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Arnaldo Carvalho de Melo, Kan Liang, Jiri Olsa, Adrian Hunter,
	Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users,
	Ravi Bangoria, Mark Rutland, James Clark, Athira Rajeev,
	Kajol Jain, Thomas Richter, Atish Patra, Palmer Dabbelt,
	Mingwei Zhang

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@kernel.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
> >

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 8/8] perf tools: Check fallback error and order
  2024-09-04 16:19   ` Ian Rogers
@ 2024-09-04 18:15     ` Namhyung Kim
  0 siblings, 0 replies; 19+ messages in thread
From: Namhyung Kim @ 2024-09-04 18:15 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Arnaldo Carvalho de Melo, Kan Liang, Jiri Olsa, Adrian Hunter,
	Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users,
	Ravi Bangoria, Mark Rutland, James Clark, Athira Rajeev,
	Kajol Jain, Thomas Richter, Atish Patra, Palmer Dabbelt,
	Mingwei Zhang

On Wed, Sep 04, 2024 at 09:19:25AM -0700, Ian Rogers wrote:
> On Tue, Sep 3, 2024 at 11:41 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > The perf_event_open might fail due to various reasons, so blindly
> > reducing precise_ip level might not be the best way to deal with it.
> >
> > It seems the kernel return -EOPNOTSUPP when PMU doesn't support the
> > given precise level.  Let's try again with the correct error code.
> 
> We also have pmu's max_precise:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/pmu.h?h=perf-tools-next#n91
> The reducing the precision approach was iirc taken for AMD who will
> forward some precise events to IBS, but the max_precise on the cpu PMU
> is 0. I think because of this, reducing the precision below
> evsel->pmu->max_precise shouldn't be necessary and another fallback
> may help better.

Internally IBS has max_precise of 2 and I think it should have that in
the sysfs.

But I found a problem with this code.  Now cycles:P would stop at 2
because after that it won't return EOPNOTSUPP.  Instead, it returns
EINVAL because of exclude_kernel and PERF_PMU_CAP_NO_EXCLUDE.

Maybe we need something like this.. :(

Thanks,
Namhyung


diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 0133c9ad3ce07a24..6157dc68044eb389 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -2587,6 +2587,13 @@ static int evsel__open_cpu(struct evsel *evsel, struct perf_cpu_map *cpus,
        if (err == -EINVAL && evsel__detect_missing_features(evsel))
                goto fallback_missing_features;
 
+       /* HACK: AMD IBS doesn't accept exclude_*, forwarding it back to core PMU */
+       if (err == -EINVAL && evsel->precise_max && evsel->core.attr.precise_ip &&
+                       evsel->core.attr.exclude_kernel) {
+               evsel->core.attr.precise_ip = 0;
+               goto fallback_missing_features;
+       }
+
 out_close:
        if (err)
                threads->err_thread = thread;

^ permalink raw reply related	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2024-09-04 18:15 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-09-04  6:41 [RFC/PATCHSET 0/8] perf tools: Do not set attr.exclude_guest by default (v2) Namhyung Kim
2024-09-04  6:41 ` [PATCH 1/8] perf tools: Don't set attr.exclude_guest by default Namhyung Kim
2024-09-04 13:07   ` Arnaldo Carvalho de Melo
2024-09-04 15:43     ` Namhyung Kim
2024-09-04  6:41 ` [PATCH 2/8] perf tools: Simplify evsel__add_modifier() Namhyung Kim
2024-09-04  6:41 ` [PATCH 3/8] perf stat: Add --exclude-guest option Namhyung Kim
2024-09-04  6:41 ` [PATCH 4/8] perf tools: Do not set exclude_guest for precise_ip Namhyung Kim
2024-09-04  6:41 ` [PATCH 5/8] perf tools: Detect missing kernel features properly Namhyung Kim
2024-09-04  6:41 ` [PATCH 6/8] perf tools: Separate exclude_hv fallback Namhyung Kim
2024-09-04  6:41 ` [PATCH 7/8] perf tools: Add fallback for exclude_guest Namhyung Kim
2024-09-04 13:28   ` James Clark
2024-09-04 13:29     ` James Clark
2024-09-04 13:36     ` James Clark
2024-09-04 15:52       ` Namhyung Kim
2024-09-04  6:41 ` [PATCH 8/8] perf tools: Check fallback error and order Namhyung Kim
2024-09-04 16:19   ` Ian Rogers
2024-09-04 18:15     ` Namhyung Kim
2024-09-04 16:36 ` [RFC/PATCHSET 0/8] perf tools: Do not set attr.exclude_guest by default (v2) Ian Rogers
2024-09-04 17:46   ` Namhyung Kim

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).