Messages in this thread | | | From | Namhyung Kim <> | Date | Mon, 1 Apr 2024 13:34:35 -0700 | Subject | Re: [RFC PATCH v6 1/5] perf stat: Parse and find tpebs events when parsing metrics to prepare for perf record sampling |
| |
Hello Weilin,
On Fri, Mar 29, 2024 at 12:12 PM <weilin.wang@intel.com> wrote: > > From: Weilin Wang <weilin.wang@intel.com> > > 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@intel.com> > Reviewed-by: Ian Rogers <irogers@google.com> > --- > 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; > + } >
| |