Re: [PATCH 4/5] perf evlist: Add evlist__{en/dis}able_non_dummy()

[Ian Rogers]

On Wed, Aug 24, 2022 at 12:28 AM Adrian Hunter <adrian.hunter@xxxxxxxxx> wrote:
>
> Dummy events are used to provide sideband information like MMAP events that
> are always needed even when main events are disabled. Add functions that
> take that into account.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@xxxxxxxxx>

Acked-by: Ian Rogers <irogers@xxxxxxxxxx>

Thanks,
Ian

> ---
>  tools/perf/util/evlist.c | 30 ++++++++++++++++++++++++------
>  tools/perf/util/evlist.h |  2 ++
>  2 files changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index 4c5e6e9f8d11..3cfe730c12b8 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -480,7 +480,7 @@ static int evlist__is_enabled(struct evlist *evlist)
>         return false;
>  }
>
> -static void __evlist__disable(struct evlist *evlist, char *evsel_name)
> +static void __evlist__disable(struct evlist *evlist, char *evsel_name, bool excl_dummy)
>  {
>         struct evsel *pos;
>         struct evlist_cpu_iterator evlist_cpu_itr;
> @@ -502,6 +502,8 @@ static void __evlist__disable(struct evlist *evlist, char *evsel_name)
>                                 continue;
>                         if (pos->disabled || !evsel__is_group_leader(pos) || !pos->core.fd)
>                                 continue;
> +                       if (excl_dummy && evsel__is_dummy_event(pos))
> +                               continue;
>                         if (pos->immediate)
>                                 has_imm = true;
>                         if (pos->immediate != imm)
> @@ -518,6 +520,8 @@ static void __evlist__disable(struct evlist *evlist, char *evsel_name)
>                         continue;
>                 if (!evsel__is_group_leader(pos) || !pos->core.fd)
>                         continue;
> +               if (excl_dummy && evsel__is_dummy_event(pos))
> +                       continue;
>                 pos->disabled = true;
>         }
>
> @@ -533,15 +537,20 @@ static void __evlist__disable(struct evlist *evlist, char *evsel_name)
>
>  void evlist__disable(struct evlist *evlist)
>  {
> -       __evlist__disable(evlist, NULL);
> +       __evlist__disable(evlist, NULL, false);
> +}
> +
> +void evlist__disable_non_dummy(struct evlist *evlist)
> +{
> +       __evlist__disable(evlist, NULL, true);

nit: I think it is better to  prefer to name the argument when passing
constants, it can avoid errors if later constants are introduced, etc.
It can also be checked by compiler warnings. As this is limited to the
same file it is not hugely critical.

>  }
>
>  void evlist__disable_evsel(struct evlist *evlist, char *evsel_name)
>  {
> -       __evlist__disable(evlist, evsel_name);
> +       __evlist__disable(evlist, evsel_name, false);
>  }
>
> -static void __evlist__enable(struct evlist *evlist, char *evsel_name)
> +static void __evlist__enable(struct evlist *evlist, char *evsel_name, bool excl_dummy)
>  {
>         struct evsel *pos;
>         struct evlist_cpu_iterator evlist_cpu_itr;
> @@ -560,6 +569,8 @@ static void __evlist__enable(struct evlist *evlist, char *evsel_name)
>                         continue;
>                 if (!evsel__is_group_leader(pos) || !pos->core.fd)
>                         continue;
> +               if (excl_dummy && evsel__is_dummy_event(pos))
> +                       continue;
>                 evsel__enable_cpu(pos, evlist_cpu_itr.cpu_map_idx);
>         }
>         affinity__cleanup(affinity);
> @@ -568,6 +579,8 @@ static void __evlist__enable(struct evlist *evlist, char *evsel_name)
>                         continue;
>                 if (!evsel__is_group_leader(pos) || !pos->core.fd)
>                         continue;
> +               if (excl_dummy && evsel__is_dummy_event(pos))
> +                       continue;
>                 pos->disabled = false;
>         }
>
> @@ -581,12 +594,17 @@ static void __evlist__enable(struct evlist *evlist, char *evsel_name)
>
>  void evlist__enable(struct evlist *evlist)
>  {
> -       __evlist__enable(evlist, NULL);
> +       __evlist__enable(evlist, NULL, false);
> +}
> +
> +void evlist__enable_non_dummy(struct evlist *evlist)
> +{
> +       __evlist__enable(evlist, NULL, true);
>  }
>
>  void evlist__enable_evsel(struct evlist *evlist, char *evsel_name)
>  {
> -       __evlist__enable(evlist, evsel_name);
> +       __evlist__enable(evlist, evsel_name, false);
>  }
>
>  void evlist__toggle_enable(struct evlist *evlist)
> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
> index 3a464585d397..3a8474406738 100644
> --- a/tools/perf/util/evlist.h
> +++ b/tools/perf/util/evlist.h
> @@ -205,6 +205,8 @@ void evlist__enable(struct evlist *evlist);
>  void evlist__toggle_enable(struct evlist *evlist);
>  void evlist__disable_evsel(struct evlist *evlist, char *evsel_name);
>  void evlist__enable_evsel(struct evlist *evlist, char *evsel_name);
> +void evlist__disable_non_dummy(struct evlist *evlist);
> +void evlist__enable_non_dummy(struct evlist *evlist);
>
>  void evlist__set_selected(struct evlist *evlist, struct evsel *evsel);
>
> --
> 2.25.1
>