| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 1 Apr 2024 13:34:35 -0700
From: Namhyung Kim <namhyung@...nel.org>
To: weilin.wang@...el.com
Cc: Ian Rogers <irogers@...gle.com>, Arnaldo Carvalho de Melo <acme@...nel.org>,
Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>,
Adrian Hunter <adrian.hunter@...el.com>, Kan Liang <kan.liang@...ux.intel.com>,
linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
Perry Taylor <perry.taylor@...el.com>, Samantha Alt <samantha.alt@...el.com>,
Caleb Biggers <caleb.biggers@...el.com>
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@...el.com> wrote:
>
> From: Weilin Wang <weilin.wang@...el.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@...el.com>
> Reviewed-by: Ian Rogers <irogers@...gle.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;
> + }
>
Powered by blists - more mailing lists