| 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
| ||
|
Message-ID: <CAM9d7ciPkwwWkGQX9nNN=XFmtf=Wg-9uSdF_OrAh1==Re=dbgw@mail.gmail.com>
Date: Tue, 20 Feb 2024 18:21:14 -0800
From: Namhyung Kim <namhyung@...nel.org>
To: Ian Rogers <irogers@...gle.com>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
Arnaldo Carvalho de Melo <acme@...nel.org>, Mark Rutland <mark.rutland@....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>,
Kajol Jain <kjain@...ux.ibm.com>, John Garry <john.g.garry@...cle.com>, Kaige Ye <ye@...ge.org>,
K Prateek Nayak <kprateek.nayak@....com>, linux-perf-users@...r.kernel.org,
linux-kernel@...r.kernel.org, Stephane Eranian <eranian@...gle.com>
Subject: Re: [PATCH v2 1/3] perf stat: Pass fewer metric arguments
On Mon, Feb 5, 2024 at 8:32 PM Ian Rogers <irogers@...gle.com> wrote:
>
> Pass metric_expr and evsel rather than specific variables from the
> struct, thereby reducing the number of arguments. This will enable
> later fixes.
>
> To reduce the size of the diff, local variables are added to match the
> previous parameter names. This isn't done in the case of "name" as
> evsel->name is more intention revealing. A whitespace issue is also
> addressed.
>
> Signed-off-by: Ian Rogers <irogers@...gle.com>
This doesn't apply to perf-tools-next anymore. Can you please refresh?
Anyway the change looks good to me now. For the series:
Acked-by: Namhyung Kim <namhyung@...nel.org>
Thanks,
Namhyung
> ---
> tools/perf/util/stat-shadow.c | 38 +++++++++++++++++------------------
> 1 file changed, 18 insertions(+), 20 deletions(-)
>
> diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
> index e31426167852..b3a25659b9e6 100644
> --- a/tools/perf/util/stat-shadow.c
> +++ b/tools/perf/util/stat-shadow.c
> @@ -355,11 +355,12 @@ static void print_nsecs(struct perf_stat_config *config,
> print_metric(config, ctxp, NULL, NULL, "CPUs utilized", 0);
> }
>
> -static int prepare_metric(struct evsel **metric_events,
> - struct metric_ref *metric_refs,
> +static int prepare_metric(const struct metric_expr *mexp,
> struct expr_parse_ctx *pctx,
> int aggr_idx)
> {
> + struct evsel * const *metric_events = mexp->metric_events;
> + struct metric_ref *metric_refs = mexp->metric_refs;
> int i;
>
> for (i = 0; metric_events[i]; i++) {
> @@ -403,7 +404,7 @@ static int prepare_metric(struct evsel **metric_events,
> if (!aggr)
> break;
>
> - if (!metric_events[i]->supported) {
> + if (!metric_events[i]->supported) {
> /*
> * Not supported events will have a count of 0,
> * which can be confusing in a
> @@ -441,18 +442,18 @@ static int prepare_metric(struct evsel **metric_events,
> }
>
> static void generic_metric(struct perf_stat_config *config,
> - const char *metric_expr,
> - const char *metric_threshold,
> - struct evsel **metric_events,
> - struct metric_ref *metric_refs,
> - char *name,
> - const char *metric_name,
> - const char *metric_unit,
> - int runtime,
> + struct metric_expr *mexp,
> + struct evsel *evsel,
> int aggr_idx,
> struct perf_stat_output_ctx *out)
> {
> print_metric_t print_metric = out->print_metric;
> + const char *metric_name = mexp->metric_name;
> + const char *metric_expr = mexp->metric_expr;
> + const char *metric_threshold = mexp->metric_threshold;
> + const char *metric_unit = mexp->metric_unit;
> + struct evsel * const *metric_events = mexp->metric_events;
> + int runtime = mexp->runtime;
> struct expr_parse_ctx *pctx;
> double ratio, scale, threshold;
> int i;
> @@ -467,7 +468,7 @@ static void generic_metric(struct perf_stat_config *config,
> pctx->sctx.user_requested_cpu_list = strdup(config->user_requested_cpu_list);
> pctx->sctx.runtime = runtime;
> pctx->sctx.system_wide = config->system_wide;
> - i = prepare_metric(metric_events, metric_refs, pctx, aggr_idx);
> + i = prepare_metric(mexp, pctx, aggr_idx);
> if (i < 0) {
> expr__ctx_free(pctx);
> return;
> @@ -502,18 +503,18 @@ static void generic_metric(struct perf_stat_config *config,
> print_metric(config, ctxp, color, "%8.2f",
> metric_name ?
> metric_name :
> - out->force_header ? name : "",
> + out->force_header ? evsel->name : "",
> ratio);
> }
> } else {
> print_metric(config, ctxp, color, /*unit=*/NULL,
> out->force_header ?
> - (metric_name ? metric_name : name) : "", 0);
> + (metric_name ?: evsel->name) : "", 0);
> }
> } else {
> print_metric(config, ctxp, color, /*unit=*/NULL,
> out->force_header ?
> - (metric_name ? metric_name : name) : "", 0);
> + (metric_name ?: evsel->name) : "", 0);
> }
>
> expr__ctx_free(pctx);
> @@ -528,7 +529,7 @@ double test_generic_metric(struct metric_expr *mexp, int aggr_idx)
> if (!pctx)
> return NAN;
>
> - if (prepare_metric(mexp->metric_events, mexp->metric_refs, pctx, aggr_idx) < 0)
> + if (prepare_metric(mexp, pctx, aggr_idx) < 0)
> goto out;
>
> if (expr__parse(&ratio, pctx, mexp->metric_expr))
> @@ -630,10 +631,7 @@ void *perf_stat__print_shadow_stats_metricgroup(struct perf_stat_config *config,
>
> if ((*num)++ > 0)
> out->new_line(config, ctxp);
> - generic_metric(config, mexp->metric_expr, mexp->metric_threshold,
> - mexp->metric_events, mexp->metric_refs, evsel->name,
> - mexp->metric_name, mexp->metric_unit, mexp->runtime,
> - aggr_idx, out);
> + generic_metric(config, mexp, evsel, aggr_idx, out);
> }
>
> return NULL;
> --
> 2.43.0.594.gd9cf4e227d-goog
>
Powered by blists - more mailing lists