[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAP-5=fXHGGCFw_5zcXboFUYeNMMC5K0fbAXwPn0HRw91pt6A0A@mail.gmail.com>
Date: Wed, 15 Jul 2020 10:00:09 -0700
From: Ian Rogers <irogers@...gle.com>
To: Jiri Olsa <jolsa@...nel.org>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>,
lkml <linux-kernel@...r.kernel.org>,
Ingo Molnar <mingo@...nel.org>,
Namhyung Kim <namhyung@...nel.org>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Peter Zijlstra <a.p.zijlstra@...llo.nl>,
Michael Petlan <mpetlan@...hat.com>,
Andi Kleen <ak@...ux.intel.com>,
Kajol Jain <kjain@...ux.ibm.com>,
John Garry <john.garry@...wei.com>,
"Paul A. Clarke" <pc@...ibm.com>,
Stephane Eranian <eranian@...gle.com>
Subject: Re: [PATCH 13/18] perf metric: Add events for the current group
On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <jolsa@...nel.org> wrote:
>
> There's no need to iterate the whole list of groups,
> when adding new events. The currently created group
> is the one we want to add.
>
> Signed-off-by: Jiri Olsa <jolsa@...nel.org>
> ---
> tools/perf/util/metricgroup.c | 22 ++++++++++++----------
> 1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 8cbcc5e05fef..66f25362702d 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -811,17 +811,19 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
Could we add a function comment to describe the arguments here?
Currently events is an empty list out argument that is built up by
this code, now it will be incrementally updated. Except I don't think
I'm capturing the current state correctly, it is confusing that there
is the loop in the current code. It looks like events will be added
multiple times redundantly.
> if (ret)
> return ret;
>
> - list_for_each_entry(eg, group_list, nd) {
> - if (events->len > 0)
> - strbuf_addf(events, ",");
> + if (events->len > 0)
> + strbuf_addf(events, ",");
>
> - if (eg->has_constraint) {
> - metricgroup__add_metric_non_group(events,
> - &eg->pctx);
> - } else {
> - metricgroup__add_metric_weak_group(events,
> - &eg->pctx);
> - }
> + /*
> + * Even if we add multiple groups through the runtime
> + * param, they share same events.
> + */
I'm not clear what runtime param is here. Is it the \? arch runtime parameter?
Thanks,
Ian
> + if (eg->has_constraint) {
> + metricgroup__add_metric_non_group(events,
> + &eg->pctx);
> + } else {
> + metricgroup__add_metric_weak_group(events,
> + &eg->pctx);
> }
> return 0;
> }
> --
> 2.25.4
>
Powered by blists - more mailing lists