Date:   Thu, 10 Sep 2020 14:51:13 +0900
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@...hat.com>, Kajol Jain <kjain@...ux.ibm.com>,
        Kan Liang <kan.liang@...ux.intel.com>,
        Jin Yao <yao.jin@...ux.intel.com>,
        Thomas Richter <tmricht@...ux.ibm.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Stephane Eranian <eranian@...gle.com>
Subject: Re: [PATCH v2 3/3] perf metricgroup: Fix uncore metric expressions

On Thu, Sep 10, 2020 at 12:26 PM Ian Rogers <irogers@...gle.com> wrote:
>
> A metric like DRAM_BW_Use has on SkylakeX events uncore_imc/cas_count_read/
> and uncore_imc/case_count_write/. These events open 6 events per socket
> with pmu names of uncore_imc_[0-5]. The current metric setup code in
> find_evsel_group assumes one ID will map to 1 event to be recorded in
> metric_events. For events with multiple matches, the first event is
> recorded in metric_events (avoiding matching >1 event with the same
> name) and the evlist_used updated so that duplicate events aren't
> removed when the evlist has unused events removed.
>
> Before this change:
> $ /tmp/perf/perf stat -M DRAM_BW_Use -a -- sleep 1
>
>  Performance counter stats for 'system wide':
>
>              41.14 MiB  uncore_imc/cas_count_read/
>      1,002,614,251 ns   duration_time
>
>        1.002614251 seconds time elapsed
>
> After this change:
> $ /tmp/perf/perf stat -M DRAM_BW_Use -a -- sleep 1
>
>  Performance counter stats for 'system wide':
>
>             157.47 MiB  uncore_imc/cas_count_read/ #     0.00 DRAM_BW_Use
>             126.97 MiB  uncore_imc/cas_count_write/
>      1,003,019,728 ns   duration_time
>
> v2. avoids iterating over the whole evlist as suggested by
>     namhyung@...nel.org. It also fixes the metric_leader computation
>     that was broken in the same commits.
>
> Erroneous duplication introduced in:
> commit 2440689d62e9 ("perf metricgroup: Remove duped metric group events").
>
> Fixes: ded80bda8bc9 ("perf expr: Migrate expr ids table to a hashmap").
> Reported-by: Jin Yao <yao.jin@...ux.intel.com>
> Signed-off-by: Ian Rogers <irogers@...gle.com>
> ---
>  tools/perf/util/metricgroup.c | 45 ++++++++++++++++++++++++++++++++---
>  1 file changed, 42 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 662f4e8777d5..79080de9217d 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -206,6 +206,18 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
>                                 sizeof(struct evsel *) * idnum);
>                         current_leader = ev->leader;
>                 }
> +               /*
> +                * Check for duplicate events with the same name. For example,
> +                * uncore_imc/cas_count_read/ will turn into 6 events per socket
> +                * on skylakex. Only the first such event is placed in
> +                * metric_events.
> +                */
> +               for (i = 0; i < matched_events; i++) {
> +                       if (!strcmp(metric_events[i]->name, ev->name))
> +                               break;
> +               }
> +               if (i != matched_events)
> +                       continue;

We have the same logic in the below.  Maybe it'd better to factor out..


>                 if (hashmap__find(&pctx->ids, ev->name, (void **)&val_ptr)) {
>                         if (has_constraint) {
>                                 /*
> @@ -245,9 +257,36 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
>         metric_events[idnum] = NULL;
>
>         for (i = 0; i < idnum; i++) {
> -               ev = metric_events[i];
> -               ev->metric_leader = ev;
> -               set_bit(ev->idx, evlist_used);
> +               /* Don't free used events. */
> +               set_bit(metric_events[i]->idx, evlist_used);
> +               /*
> +                * The metric leader points to the identically named event in
> +                * metric_events.
> +                */
> +               metric_events[i]->metric_leader = metric_events[i];
> +               /*
> +                * Mark two events with identical names in the same group (or
> +                * globally) as being in use as uncore events may be duplicated
> +                * for each pmu. Set the metric leader to be the event that
> +                * appears in metric_events.
> +                */

I thought this again, and it's not guaranteed that the metric leader is
a group leader so below won't work IMHO.  Instead we should iterate
evlist always, but started from the metric leader with the
evlist__for_each_entry_continue.

Thanks
Namhyung


> +               if (!has_constraint) {
> +                       for_each_group_evsel(ev, metric_events[i]->leader) {
> +                               if (ev != metric_events[i] &&
> +                                   !strcmp(metric_events[i]->name, ev->name)) {
> +                                       set_bit(ev->idx, evlist_used);
> +                                       ev->metric_leader = metric_events[i];
> +                               }
> +                       }
> +               } else {
> +                       evlist__for_each_entry(perf_evlist, ev) {
> +                               if (ev != metric_events[i] &&
> +                                   !strcmp(metric_events[i]->name, ev->name)) {
> +                                       set_bit(ev->idx, evlist_used);
> +                                       ev->metric_leader = metric_events[i];
> +                               }
> +                       }
> +               }
>         }
>
>         return metric_events[0];
> --
> 2.28.0.526.ge36021eeef-goog
>

Powered by blists - more mailing lists