Re: [PATCH v6 2/7] perf evlist: Add evlist__findnew_tracking_event() helper

[Ian Rogers]

On Sun, Aug 20, 2023 at 6:30 PM Yang Jihong <yangjihong1@xxxxxxxxxx> wrote:
>
> Currently, intel-bts, intel-pt, and arm-spe may add a dummy event for
> tracking to the evlist. We may need to search for the dummy event for
> some settings. Therefore, add evlist__findnew_tracking_event() helper.

Given the first two sentences I don't understand why this is
evlist__findnew_tracking_event and not evlist__findnew_dummy_event?
Are you setting tracking on other events other than dummy? If so, then
the commit message isn't right. If not then I'd prefer not to use
tracking event in the function name.

>
> evlist__findnew_tracking_event() also deal with system_wide maps if
> system_wide is true.

Could you fix the explanation here, what does "deal with system_wide"
mean? A kerneldoc comment and explanation of the system_wide argument
would be useful.

Thanks,
Ian

> Signed-off-by: Yang Jihong <yangjihong1@xxxxxxxxxx>
> Acked-by: Adrian Hunter <adrian.hunter@xxxxxxxxx>
> ---
>  tools/perf/builtin-record.c | 11 +++--------
>  tools/perf/util/evlist.c    | 18 ++++++++++++++++++
>  tools/perf/util/evlist.h    |  1 +
>  3 files changed, 22 insertions(+), 8 deletions(-)
>
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 34bb31f08bb5..12edad8392cc 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -1293,14 +1293,9 @@ static int record__open(struct record *rec)
>          */
>         if (opts->target.initial_delay || target__has_cpu(&opts->target) ||
>             perf_pmus__num_core_pmus() > 1) {
> -               pos = evlist__get_tracking_event(evlist);
> -               if (!evsel__is_dummy_event(pos)) {
> -                       /* Set up dummy event. */
> -                       if (evlist__add_dummy(evlist))
> -                               return -ENOMEM;
> -                       pos = evlist__last(evlist);
> -                       evlist__set_tracking_event(evlist, pos);
> -               }
> +               pos = evlist__findnew_tracking_event(evlist, false);
> +               if (!pos)
> +                       return -ENOMEM;
>
>                 /*
>                  * Enable the dummy event when the process is forked for
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index 7ef43f72098e..25c3ebe2c2f5 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -1694,6 +1694,24 @@ void evlist__set_tracking_event(struct evlist *evlist, struct evsel *tracking_ev
>         tracking_evsel->tracking = true;
>  }
>
> +struct evsel *evlist__findnew_tracking_event(struct evlist *evlist, bool system_wide)
> +{
> +       struct evsel *evsel;
> +
> +       evsel = evlist__get_tracking_event(evlist);
> +       if (!evsel__is_dummy_event(evsel)) {
> +               evsel = evlist__add_aux_dummy(evlist, system_wide);
> +               if (!evsel)
> +                       return NULL;
> +
> +               evlist__set_tracking_event(evlist, evsel);
> +       } else if (system_wide) {
> +               perf_evlist__go_system_wide(&evlist->core, &evsel->core);
> +       }
> +
> +       return evsel;
> +}
> +
>  struct evsel *evlist__find_evsel_by_str(struct evlist *evlist, const char *str)
>  {
>         struct evsel *evsel;
> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
> index 664c6bf7b3e0..98e7ddb2bd30 100644
> --- a/tools/perf/util/evlist.h
> +++ b/tools/perf/util/evlist.h
> @@ -387,6 +387,7 @@ bool evlist_cpu_iterator__end(const struct evlist_cpu_iterator *evlist_cpu_itr);
>
>  struct evsel *evlist__get_tracking_event(struct evlist *evlist);
>  void evlist__set_tracking_event(struct evlist *evlist, struct evsel *tracking_evsel);
> +struct evsel *evlist__findnew_tracking_event(struct evlist *evlist, bool system_wide);
>
>  struct evsel *evlist__find_evsel_by_str(struct evlist *evlist, const char *str);
>
> --
> 2.30.GIT
>