[RFC/PATCH 0/4] perf tools: Do not set attr.exclude

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

* [RFC/PATCH 0/4] perf tools: Do not set attr.exclude_guest by default (v1)
@ 2024-09-02  1:46 Namhyung Kim
  2024-09-02  1:46 ` [PATCH 1/4] perf tools: Don't set attr.exclude_guest by default Namhyung Kim
                   ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: Namhyung Kim @ 2024-09-02  1:46 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.

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.  Please let me
know if it's ok for you.

Thanks,
Namhyung


Namhyung Kim (4):
  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

 tools/perf/Documentation/perf-stat.txt     |  7 +++++
 tools/perf/builtin-kvm.c                   |  1 +
 tools/perf/builtin-stat.c                  |  2 ++
 tools/perf/tests/attr/test-record-dummy-C0 |  2 +-
 tools/perf/tests/parse-events.c            | 30 ++++++++++------------
 tools/perf/util/evsel.c                    |  5 +---
 tools/perf/util/parse-events.c             |  6 +----
 tools/perf/util/util.c                     | 10 ++++++--
 tools/perf/util/util.h                     |  3 +++
 9 files changed, 37 insertions(+), 29 deletions(-)

-- 
2.46.0.469.g59c65b2a67-goog


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

* [PATCH 1/4] perf tools: Don't set attr.exclude_guest by default
  2024-09-02  1:46 [RFC/PATCH 0/4] perf tools: Do not set attr.exclude_guest by default (v1) Namhyung Kim
@ 2024-09-02  1:46 ` Namhyung Kim
  2024-09-02  9:15   ` James Clark
  2024-09-02  1:46 ` [PATCH 2/4] perf tools: Simplify evsel__add_modifier() Namhyung Kim
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 9+ messages in thread
From: Namhyung Kim @ 2024-09-02  1:46 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 692267b1b7e8..ca94dd3de04d 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 576ec48b3aaf..8ce6f0a5df5b 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 edc2adcf1bae..9179bf3084c3 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 fab01ba54e34..ab73b3d45f04 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 9d55a13787ce..7e3159faaa19 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 9966c21aaf04..d33ae883a54f 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] 9+ messages in thread

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



On 02/09/2024 2:46 am, 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.

Probably another case where again the real fix would be to add 
/sys/bus/event_source/devices/cpu_core/caps/exclude_guest and then we're 
able to keep the defaults.

> 
>    $ 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.

I'm reading this to assume that the default record command was always 
broken then? But what about any other command, now isn't just "cycles" 
also broken making it worse?

See 25412c036:

   ...
   (c) The Apple M1/M2 PMU requires that perf_event_attr::exclude_guest
       is set as the hardware PMU does not count while a guest is running
       (but might be extended in future to do so).
   ...

> 
> 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 692267b1b7e8..ca94dd3de04d 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 576ec48b3aaf..8ce6f0a5df5b 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 edc2adcf1bae..9179bf3084c3 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 fab01ba54e34..ab73b3d45f04 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 9d55a13787ce..7e3159faaa19 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 9966c21aaf04..d33ae883a54f 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;
>   

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

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

On Mon, Sep 02, 2024 at 10:15:57AM +0100, James Clark wrote:
> 
> 
> On 02/09/2024 2:46 am, 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.
> 
> Probably another case where again the real fix would be to add
> /sys/bus/event_source/devices/cpu_core/caps/exclude_guest and then we're
> able to keep the defaults.

Yep, I really need to work on it.  But it's another story whether it's
supported and whether it's required.

> 
> > 
> >    $ 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.
> 
> I'm reading this to assume that the default record command was always broken
> then? But what about any other command, now isn't just "cycles" also broken
> making it worse?

Hmm.. right.  Maybe we can add a detection logic to figure out the
required exclude bits for a PMU at runtime.

Thanks,
Namhyung

> 
> See 25412c036:
> 
>   ...
>   (c) The Apple M1/M2 PMU requires that perf_event_attr::exclude_guest
>       is set as the hardware PMU does not count while a guest is running
>       (but might be extended in future to do so).
>   ...
> 
> > 
> > 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 692267b1b7e8..ca94dd3de04d 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 576ec48b3aaf..8ce6f0a5df5b 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 edc2adcf1bae..9179bf3084c3 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 fab01ba54e34..ab73b3d45f04 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 9d55a13787ce..7e3159faaa19 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 9966c21aaf04..d33ae883a54f 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;

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

* [PATCH 2/4] perf tools: Simplify evsel__add_modifier()
  2024-09-02  1:46 [RFC/PATCH 0/4] perf tools: Do not set attr.exclude_guest by default (v1) Namhyung Kim
  2024-09-02  1:46 ` [PATCH 1/4] perf tools: Don't set attr.exclude_guest by default Namhyung Kim
@ 2024-09-02  1:46 ` Namhyung Kim
  2024-09-02  1:46 ` [PATCH 3/4] perf stat: Add --exclude-guest option Namhyung Kim
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Namhyung Kim @ 2024-09-02  1:46 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.

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

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 49cc71511c0c..57fe0f9b06f9 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] 9+ messages in thread

* [PATCH 3/4] perf stat: Add --exclude-guest option
  2024-09-02  1:46 [RFC/PATCH 0/4] perf tools: Do not set attr.exclude_guest by default (v1) Namhyung Kim
  2024-09-02  1:46 ` [PATCH 1/4] perf tools: Don't set attr.exclude_guest by default Namhyung Kim
  2024-09-02  1:46 ` [PATCH 2/4] perf tools: Simplify evsel__add_modifier() Namhyung Kim
@ 2024-09-02  1:46 ` Namhyung Kim
  2024-09-02  1:46 ` [PATCH 4/4] perf tools: Do not set exclude_guest for precise_ip Namhyung Kim
  2024-09-02  8:56 ` [RFC/PATCH 0/4] perf tools: Do not set attr.exclude_guest by default (v1) Peter Zijlstra
  4 siblings, 0 replies; 9+ messages in thread
From: Namhyung Kim @ 2024-09-02  1:46 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 2bc063672486..d28d8370a856 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 cf985cdb9a6e..8b9889873d3e 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] 9+ messages in thread

* [PATCH 4/4] perf tools: Do not set exclude_guest for precise_ip
  2024-09-02  1:46 [RFC/PATCH 0/4] perf tools: Do not set attr.exclude_guest by default (v1) Namhyung Kim
                   ` (2 preceding siblings ...)
  2024-09-02  1:46 ` [PATCH 3/4] perf stat: Add --exclude-guest option Namhyung Kim
@ 2024-09-02  1:46 ` Namhyung Kim
  2024-09-02  8:56 ` [RFC/PATCH 0/4] perf tools: Do not set attr.exclude_guest by default (v1) Peter Zijlstra
  4 siblings, 0 replies; 9+ messages in thread
From: Namhyung Kim @ 2024-09-02  1:46 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 9179bf3084c3..900107a171ee 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 ab73b3d45f04..4fa46ef7213c 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] 9+ messages in thread

* Re: [RFC/PATCH 0/4] perf tools: Do not set attr.exclude_guest by default (v1)
  2024-09-02  1:46 [RFC/PATCH 0/4] perf tools: Do not set attr.exclude_guest by default (v1) Namhyung Kim
                   ` (3 preceding siblings ...)
  2024-09-02  1:46 ` [PATCH 4/4] perf tools: Do not set exclude_guest for precise_ip Namhyung Kim
@ 2024-09-02  8:56 ` Peter Zijlstra
       [not found]   ` <CAM9d7cg13isO4rmwUpDAm9pL7qLYDOfXsBnGhr6eLMuUyM2Z2w@mail.gmail.com>
  4 siblings, 1 reply; 9+ messages in thread
From: Peter Zijlstra @ 2024-09-02  8:56 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, Ian Rogers, Kan Liang, Jiri Olsa,
	Adrian Hunter, 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 Sun, Sep 01, 2024 at 06:46:17PM -0700, Namhyung Kim 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.
> 
> 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.  Please let me
> know if it's ok for you.

IIRc the point of setting exclude_guest by default was so that default
perf keeps working in the precense of that PMU pass through mess, no?

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

[parent not found: <CAM9d7cg13isO4rmwUpDAm9pL7qLYDOfXsBnGhr6eLMuUyM2Z2w@mail.gmail.com>]

* Re: [RFC/PATCH 0/4] perf tools: Do not set attr.exclude_guest by default (v1)
       [not found]   ` <CAM9d7cg13isO4rmwUpDAm9pL7qLYDOfXsBnGhr6eLMuUyM2Z2w@mail.gmail.com>
@ 2024-09-03  8:39     ` Peter Zijlstra
  0 siblings, 0 replies; 9+ messages in thread
From: Peter Zijlstra @ 2024-09-03  8:39 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, Ian Rogers, Kan Liang, Jiri Olsa,
	Adrian Hunter, 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 Mon, Sep 02, 2024 at 10:20:41AM -0700, Namhyung Kim wrote:
> Hi Peter,
> 
> On Mon, Sep 2, 2024 at 1:56 AM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Sun, Sep 01, 2024 at 06:46:17PM -0700, Namhyung Kim 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.
> > >
> > > 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.  Please let me
> > > know if it's ok for you.
> >
> > IIRc the point of setting exclude_guest by default was so that default
> > perf keeps working in the precense of that PMU pass through mess, no?
> 
> Sorry I haven't followed the vPMU pass through threads.  It makes
> sense to keep the exclude_guest for the kernels with it.  But I guess
> it's configurable via kernel config and/or boot option.

Yeah, you need KVM build with the passthrough thing on, and then at
runtime enable it or something -- I too have to still read the latest
posting.

> A question, with PMU pass through would the kernel reject host events
> which don't set the exclude_guest?

Yes.

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

end of thread, other threads:[~2024-09-03  8:39 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-09-02  1:46 [RFC/PATCH 0/4] perf tools: Do not set attr.exclude_guest by default (v1) Namhyung Kim
2024-09-02  1:46 ` [PATCH 1/4] perf tools: Don't set attr.exclude_guest by default Namhyung Kim
2024-09-02  9:15   ` James Clark
2024-09-02 17:38     ` Namhyung Kim
2024-09-02  1:46 ` [PATCH 2/4] perf tools: Simplify evsel__add_modifier() Namhyung Kim
2024-09-02  1:46 ` [PATCH 3/4] perf stat: Add --exclude-guest option Namhyung Kim
2024-09-02  1:46 ` [PATCH 4/4] perf tools: Do not set exclude_guest for precise_ip Namhyung Kim
2024-09-02  8:56 ` [RFC/PATCH 0/4] perf tools: Do not set attr.exclude_guest by default (v1) Peter Zijlstra
     [not found]   ` <CAM9d7cg13isO4rmwUpDAm9pL7qLYDOfXsBnGhr6eLMuUyM2Z2w@mail.gmail.com>
2024-09-03  8:39     ` Peter Zijlstra

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).