[PATCH] perf parse-events: Set default GH modifier properly

[PATCH] perf parse-events: Set default GH modifier properly

[Namhyung Kim]

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

Re: [PATCH] perf parse-events: Set default GH modifier properly

[Ian Rogers]

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
>

Re: [PATCH] perf parse-events: Set default GH modifier properly

[Namhyung Kim]

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

Re: [PATCH] perf parse-events: Set default GH modifier properly

[Namhyung Kim]

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