* [PATCH] perf parse-events: Set default GH modifier properly
@ 2025-06-06 22:54 Namhyung Kim
2025-06-06 23:50 ` Ian Rogers
2025-06-21 18:01 ` Namhyung Kim
0 siblings, 2 replies; 4+ messages in thread
From: Namhyung Kim @ 2025-06-06 22:54 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
Commit 7b100989b4f6bce7 ("perf evlist: Remove __evlist__add_default")
changed to use "cycles:P" as a default event. But the problem is it
cannot set other default modifiers correctly.
perf kvm needs to set attr.exclude_host by default but it didn't work
because of the logic in the parse_events__modifier_list(). Also the
exclude_GH_default was applied only if ":u" modifier was specified -
which is strange. Move it out after handling the ":GH" and check
perf_host and perf_guest properly.
Before:
$ ./perf kvm record -vv true |& grep exclude
(nothing)
But specifying an event (without a modifier) works:
$ ./perf kvm record -vv -e cycles true |& grep exclude
exclude_host 1
After:
It now works for the both cases:
$ ./perf kvm record -vv true |& grep exclude
exclude_host 1
$ ./perf kvm record -vv -e cycles true |& grep exclude
exclude_host 1
Fixes: 35c8d21371e9b342 ("perf tools: Don't set attr.exclude_guest by default")
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
tools/perf/util/parse-events.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 7f34e602fc080881..d1965a7b97ed6b97 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1830,13 +1830,11 @@ static int parse_events__modifier_list(struct parse_events_state *parse_state,
int eH = group ? evsel->core.attr.exclude_host : 0;
int eG = group ? evsel->core.attr.exclude_guest : 0;
int exclude = eu | ek | eh;
- int exclude_GH = group ? evsel->exclude_GH : 0;
+ int exclude_GH = eG | eH;
if (mod.user) {
if (!exclude)
exclude = eu = ek = eh = 1;
- if (!exclude_GH && !perf_guest && exclude_GH_default)
- eG = 1;
eu = 0;
}
if (mod.kernel) {
@@ -1859,6 +1857,13 @@ static int parse_events__modifier_list(struct parse_events_state *parse_state,
exclude_GH = eG = eH = 1;
eH = 0;
}
+ if (!exclude_GH && exclude_GH_default) {
+ if (perf_host)
+ eG = 1;
+ else if (perf_guest)
+ eH = 1;
+ }
+
evsel->core.attr.exclude_user = eu;
evsel->core.attr.exclude_kernel = ek;
evsel->core.attr.exclude_hv = eh;
--
2.50.0.rc0.604.gd4ff7b7c86-goog
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] perf parse-events: Set default GH modifier properly
2025-06-06 22:54 [PATCH] perf parse-events: Set default GH modifier properly Namhyung Kim
@ 2025-06-06 23:50 ` Ian Rogers
2025-06-20 20:31 ` Namhyung Kim
2025-06-21 18:01 ` Namhyung Kim
1 sibling, 1 reply; 4+ messages in thread
From: Ian Rogers @ 2025-06-06 23:50 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
On Fri, Jun 6, 2025 at 3:54 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Commit 7b100989b4f6bce7 ("perf evlist: Remove __evlist__add_default")
> changed to use "cycles:P" as a default event. But the problem is it
> cannot set other default modifiers correctly.
>
> perf kvm needs to set attr.exclude_host by default but it didn't work
> because of the logic in the parse_events__modifier_list(). Also the
> exclude_GH_default was applied only if ":u" modifier was specified -
> which is strange. Move it out after handling the ":GH" and check
> perf_host and perf_guest properly.
>
> Before:
> $ ./perf kvm record -vv true |& grep exclude
> (nothing)
>
> But specifying an event (without a modifier) works:
>
> $ ./perf kvm record -vv -e cycles true |& grep exclude
> exclude_host 1
>
> After:
> It now works for the both cases:
>
> $ ./perf kvm record -vv true |& grep exclude
> exclude_host 1
>
> $ ./perf kvm record -vv -e cycles true |& grep exclude
> exclude_host 1
>
> Fixes: 35c8d21371e9b342 ("perf tools: Don't set attr.exclude_guest by default")
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Code is more understandable than before and reads as correct.
Reviewed-by: Ian Rogers <irogers@google.com>
Perhaps consider adding test coverage in tools/perf/tests/parse-events.c ?
Thanks,
Ian
> ---
> tools/perf/util/parse-events.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 7f34e602fc080881..d1965a7b97ed6b97 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -1830,13 +1830,11 @@ static int parse_events__modifier_list(struct parse_events_state *parse_state,
> int eH = group ? evsel->core.attr.exclude_host : 0;
> int eG = group ? evsel->core.attr.exclude_guest : 0;
> int exclude = eu | ek | eh;
> - int exclude_GH = group ? evsel->exclude_GH : 0;
> + int exclude_GH = eG | eH;
>
> if (mod.user) {
> if (!exclude)
> exclude = eu = ek = eh = 1;
> - if (!exclude_GH && !perf_guest && exclude_GH_default)
> - eG = 1;
> eu = 0;
> }
> if (mod.kernel) {
> @@ -1859,6 +1857,13 @@ static int parse_events__modifier_list(struct parse_events_state *parse_state,
> exclude_GH = eG = eH = 1;
> eH = 0;
> }
> + if (!exclude_GH && exclude_GH_default) {
> + if (perf_host)
> + eG = 1;
> + else if (perf_guest)
> + eH = 1;
> + }
> +
> evsel->core.attr.exclude_user = eu;
> evsel->core.attr.exclude_kernel = ek;
> evsel->core.attr.exclude_hv = eh;
> --
> 2.50.0.rc0.604.gd4ff7b7c86-goog
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] perf parse-events: Set default GH modifier properly
2025-06-06 23:50 ` Ian Rogers
@ 2025-06-20 20:31 ` Namhyung Kim
0 siblings, 0 replies; 4+ messages in thread
From: Namhyung Kim @ 2025-06-20 20:31 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
On Fri, Jun 06, 2025 at 04:50:13PM -0700, Ian Rogers wrote:
> On Fri, Jun 6, 2025 at 3:54 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > Commit 7b100989b4f6bce7 ("perf evlist: Remove __evlist__add_default")
> > changed to use "cycles:P" as a default event. But the problem is it
> > cannot set other default modifiers correctly.
> >
> > perf kvm needs to set attr.exclude_host by default but it didn't work
> > because of the logic in the parse_events__modifier_list(). Also the
> > exclude_GH_default was applied only if ":u" modifier was specified -
> > which is strange. Move it out after handling the ":GH" and check
> > perf_host and perf_guest properly.
> >
> > Before:
> > $ ./perf kvm record -vv true |& grep exclude
> > (nothing)
> >
> > But specifying an event (without a modifier) works:
> >
> > $ ./perf kvm record -vv -e cycles true |& grep exclude
> > exclude_host 1
> >
> > After:
> > It now works for the both cases:
> >
> > $ ./perf kvm record -vv true |& grep exclude
> > exclude_host 1
> >
> > $ ./perf kvm record -vv -e cycles true |& grep exclude
> > exclude_host 1
> >
> > Fixes: 35c8d21371e9b342 ("perf tools: Don't set attr.exclude_guest by default")
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>
> Code is more understandable than before and reads as correct.
>
> Reviewed-by: Ian Rogers <irogers@google.com>
Thanks for your review!
>
> Perhaps consider adding test coverage in tools/perf/tests/parse-events.c ?
Yep, will add it later.
Thanks,
Namhyung
> > ---
> > tools/perf/util/parse-events.c | 11 ++++++++---
> > 1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> > index 7f34e602fc080881..d1965a7b97ed6b97 100644
> > --- a/tools/perf/util/parse-events.c
> > +++ b/tools/perf/util/parse-events.c
> > @@ -1830,13 +1830,11 @@ static int parse_events__modifier_list(struct parse_events_state *parse_state,
> > int eH = group ? evsel->core.attr.exclude_host : 0;
> > int eG = group ? evsel->core.attr.exclude_guest : 0;
> > int exclude = eu | ek | eh;
> > - int exclude_GH = group ? evsel->exclude_GH : 0;
> > + int exclude_GH = eG | eH;
> >
> > if (mod.user) {
> > if (!exclude)
> > exclude = eu = ek = eh = 1;
> > - if (!exclude_GH && !perf_guest && exclude_GH_default)
> > - eG = 1;
> > eu = 0;
> > }
> > if (mod.kernel) {
> > @@ -1859,6 +1857,13 @@ static int parse_events__modifier_list(struct parse_events_state *parse_state,
> > exclude_GH = eG = eH = 1;
> > eH = 0;
> > }
> > + if (!exclude_GH && exclude_GH_default) {
> > + if (perf_host)
> > + eG = 1;
> > + else if (perf_guest)
> > + eH = 1;
> > + }
> > +
> > evsel->core.attr.exclude_user = eu;
> > evsel->core.attr.exclude_kernel = ek;
> > evsel->core.attr.exclude_hv = eh;
> > --
> > 2.50.0.rc0.604.gd4ff7b7c86-goog
> >
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] perf parse-events: Set default GH modifier properly
2025-06-06 22:54 [PATCH] perf parse-events: Set default GH modifier properly Namhyung Kim
2025-06-06 23:50 ` Ian Rogers
@ 2025-06-21 18:01 ` Namhyung Kim
1 sibling, 0 replies; 4+ messages in thread
From: Namhyung Kim @ 2025-06-21 18:01 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Ian Rogers, Kan Liang, Namhyung Kim
Cc: Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
linux-perf-users
On Fri, 06 Jun 2025 15:54:31 -0700, Namhyung Kim wrote:
> Commit 7b100989b4f6bce7 ("perf evlist: Remove __evlist__add_default")
> changed to use "cycles:P" as a default event. But the problem is it
> cannot set other default modifiers correctly.
>
> perf kvm needs to set attr.exclude_host by default but it didn't work
> because of the logic in the parse_events__modifier_list(). Also the
> exclude_GH_default was applied only if ":u" modifier was specified -
> which is strange. Move it out after handling the ":GH" and check
> perf_host and perf_guest properly.
>
> [...]
Applied to perf-tools-next, thanks!
Best regards,
Namhyung
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-06-21 18:01 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-06 22:54 [PATCH] perf parse-events: Set default GH modifier properly Namhyung Kim
2025-06-06 23:50 ` Ian Rogers
2025-06-20 20:31 ` Namhyung Kim
2025-06-21 18:01 ` 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).