Re: [RFC PATCH v6 1/5] perf stat: Parse and find tpebs events when parsing metrics to prepare for perf record sampling

[Namhyung Kim]

Hello Weilin,

On Fri, Mar 29, 2024 at 12:12 PM <weilin.wang@xxxxxxxxx> wrote:
>
> From: Weilin Wang <weilin.wang@xxxxxxxxx>
>
> Metrics that use tpebs values would use the R as retire_latency modifier in
> formulas. We put all these events into a list and pass the list to perf
> record to collect their retire latency value.
>
> Signed-off-by: Weilin Wang <weilin.wang@xxxxxxxxx>
> Reviewed-by: Ian Rogers <irogers@xxxxxxxxxx>
> ---
>  tools/perf/builtin-stat.c     | 38 +++++++++++++--
>  tools/perf/util/metricgroup.c | 88 +++++++++++++++++++++++++++++------
>  tools/perf/util/metricgroup.h | 10 +++-
>  tools/perf/util/stat.h        |  2 +
>  4 files changed, 119 insertions(+), 19 deletions(-)
>
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 6bba1a89d030..6291e1e24535 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -162,6 +162,7 @@ static struct perf_stat_config stat_config = {
>         .ctl_fd                 = -1,
>         .ctl_fd_ack             = -1,
>         .iostat_run             = false,
> +       .tpebs_events           = LIST_HEAD_INIT(stat_config.tpebs_events),
>  };
>
>  static bool cpus_map_matched(struct evsel *a, struct evsel *b)
> @@ -686,6 +687,12 @@ static enum counter_recovery stat_handle_error(struct evsel *counter)
>         return COUNTER_FATAL;
>  }
>
> +static int __run_perf_record(void)
> +{
> +       pr_debug("Prepare perf record for retire_latency\n");
> +       return 0;
> +}
> +
>  static int __run_perf_stat(int argc, const char **argv, int run_idx)
>  {
>         int interval = stat_config.interval;
> @@ -703,6 +710,16 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
>         int err;
>         bool second_pass = false;
>
> +       /* Prepare perf record for sampling event retire_latency before fork and
> +        * prepare workload */
> +       if (stat_config.tpebs_event_size > 0) {
> +               int ret;
> +
> +               ret = __run_perf_record();
> +               if (ret)
> +                       return ret;
> +       }
> +
>         if (forks) {
>                 if (evlist__prepare_workload(evsel_list, &target, argv, is_pipe, workload_exec_failed_signal) < 0) {
>                         perror("failed to prepare workload");
> @@ -2106,7 +2123,9 @@ static int add_default_attributes(void)
>                                                 stat_config.metric_no_threshold,
>                                                 stat_config.user_requested_cpu_list,
>                                                 stat_config.system_wide,
> -                                               &stat_config.metric_events);
> +                                               &stat_config.metric_events,
> +                                               &stat_config.tpebs_events,
> +                                               &stat_config.tpebs_event_size);

Maybe it'd be better to pass the stat_config, but it can be done later.


>         }
>
>         if (smi_cost) {
> @@ -2139,7 +2158,9 @@ static int add_default_attributes(void)
>                                                 stat_config.metric_no_threshold,
>                                                 stat_config.user_requested_cpu_list,
>                                                 stat_config.system_wide,
> -                                               &stat_config.metric_events);
> +                                               &stat_config.metric_events,
> +                                               &stat_config.tpebs_events,
> +                                               &stat_config.tpebs_event_size);
>         }
>
>         if (topdown_run) {
> @@ -2173,7 +2194,9 @@ static int add_default_attributes(void)
>                                                 /*metric_no_threshold=*/true,
>                                                 stat_config.user_requested_cpu_list,
>                                                 stat_config.system_wide,
> -                                               &stat_config.metric_events) < 0)
> +                                               &stat_config.metric_events,
> +                                               &stat_config.tpebs_events,
> +                                               &stat_config.tpebs_event_size) < 0)
>                         return -1;
>         }
>
> @@ -2214,7 +2237,9 @@ static int add_default_attributes(void)
>                                                         /*metric_no_threshold=*/true,
>                                                         stat_config.user_requested_cpu_list,
>                                                         stat_config.system_wide,
> -                                                       &stat_config.metric_events) < 0)
> +                                                       &stat_config.metric_events,
> +                                                       /*&stat_config.tpebs_events=*/NULL,
> +                                                       /*stat_config.tpebs_event_size=*/0) < 0)
>                                 return -1;
>
>                         evlist__for_each_entry(metric_evlist, metric_evsel) {
> @@ -2736,6 +2761,7 @@ int cmd_stat(int argc, const char **argv)
>                 }
>         }
>
> +
>         /*
>          * Metric parsing needs to be delayed as metrics may optimize events
>          * knowing the target is system-wide.
> @@ -2748,7 +2774,9 @@ int cmd_stat(int argc, const char **argv)
>                                                 stat_config.metric_no_threshold,
>                                                 stat_config.user_requested_cpu_list,
>                                                 stat_config.system_wide,
> -                                               &stat_config.metric_events);
> +                                               &stat_config.metric_events,
> +                                               &stat_config.tpebs_events,
> +                                               &stat_config.tpebs_event_size);
>
>                 zfree(&metrics);
>                 if (ret) {
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 79ef6095ab28..8e007d60af91 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -277,7 +277,8 @@ static bool contains_metric_id(struct evsel **metric_events, int num_events,
>   */
>  static int setup_metric_events(const char *pmu, struct hashmap *ids,
>                                struct evlist *metric_evlist,
> -                              struct evsel ***out_metric_events)
> +                              struct evsel ***out_metric_events,
> +                              size_t tpebs_event_size)
>  {
>         struct evsel **metric_events;
>         const char *metric_id;
> @@ -286,7 +287,7 @@ static int setup_metric_events(const char *pmu, struct hashmap *ids,
>         bool all_pmus = !strcmp(pmu, "all") || perf_pmus__num_core_pmus() == 1 || !is_pmu_core(pmu);
>
>         *out_metric_events = NULL;
> -       ids_size = hashmap__size(ids);
> +       ids_size = hashmap__size(ids) - tpebs_event_size;
>
>         metric_events = calloc(ids_size + 1, sizeof(void *));
>         if (!metric_events)
> @@ -323,6 +324,7 @@ static int setup_metric_events(const char *pmu, struct hashmap *ids,
>                 }
>         }
>         if (matched_events < ids_size) {
> +               pr_debug("Error: matched_events = %lu, ids_size = %lu\n", matched_events, ids_size);
>                 free(metric_events);
>                 return -EINVAL;
>         }
> @@ -668,7 +670,9 @@ static int decode_all_metric_ids(struct evlist *perf_evlist, const char *modifie
>  static int metricgroup__build_event_string(struct strbuf *events,
>                                            const struct expr_parse_ctx *ctx,
>                                            const char *modifier,
> -                                          bool group_events)
> +                                          bool group_events,
> +                                          struct list_head *tpebs_events __maybe_unused,
> +                                          size_t *tpebs_event_size)
>  {
>         struct hashmap_entry *cur;
>         size_t bkt;
> @@ -681,8 +685,56 @@ static int metricgroup__build_event_string(struct strbuf *events,
>         hashmap__for_each_entry(ctx->ids, cur, bkt) {
>                 const char *sep, *rsep, *id = cur->pkey;
>                 enum perf_tool_event ev;
> +               /*
> +                * Parse and search for event name with retire_latency modifier R.
> +                * If found, put event name into the tpebs_events list. This list
> +                * of events will be passed to perf record for sampling to get
> +                * their reitre_latency value.
> +                * Search for ":R" in event name without "@". Search for the
> +                * last "@R" in event name with "@".

Hmm.. it seems you look for an 'R' modifier and then change it to 'p', right?
Why not use strrchr to check ':' or '@' and if it's followed by 'R'?

Is the 'R' modifier only used in the metric expressions?  Also please mention
why some events have "@" in the name and others don't.


> +                */
> +               char *p = strstr(id, ":R");
> +               char *p1 = strstr(id, "@R");
> +
> +               if (p == NULL && p1) {
> +                       p = strstr(p1+1, "@R");
> +                       if (p == NULL)
> +                               p = p1;
> +                       p = p+1;
> +               }
> +
> +               if (p) {
> +                       char *name;
> +                       char *at;
> +                       struct tpebs_event *new_event = malloc(sizeof(struct tpebs_event));
>
> -               pr_debug("found event %s\n", id);
> +                       if (!new_event)
> +                               return -ENOMEM;
> +
> +                       new_event->tpebs_name = strdup(id);
> +                       *p = '\0';

I think 'p' points to the 'id' string ("cur->pkey").  Is it ok to
change it here?
I guess you may want to do it on the tpebs_name.

Thanks,
Namhyung


> +                       name = malloc(strlen(id) + 2);
> +                       if (!name)
> +                               return -ENOMEM;
> +
> +                       at = strchr(id, '@');
> +                       if (at != NULL) {
> +                               *at = '/';
> +                               at = strchr(id, '@');
> +                               *at = '/';
> +                               strcpy(name, id);
> +                               strcat(name, "p");
> +                       } else {
> +                               strcpy(name, id);
> +                               strcat(name, ":p");
> +                       }
> +                       new_event->name = name;
> +                       *tpebs_event_size += 1;
> +                       pr_debug("retire_latency required, tpebs_event_size=%lu, new_event=%s\n",
> +                               *tpebs_event_size, new_event->name);
> +                       list_add_tail(&new_event->nd, tpebs_events);
> +                       continue;
> +               }
>