Message-ID: <CAP-5=fWtziP7HzXKZHKxZdQuQ=sTEyNVKNkPVnhYNF-BcX8eKw@mail.gmail.com>
Date:   Fri, 24 Feb 2023 21:47:22 -0800
From:   Ian Rogers <irogers@...gle.com>
To:     Namhyung Kim <namhyung@...nel.org>
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>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        Alexandre Torgue <alexandre.torgue@...s.st.com>,
        Zhengjun Xing <zhengjun.xing@...ux.intel.com>,
        Sandipan Das <sandipan.das@....com>,
        James Clark <james.clark@....com>,
        Kajol Jain <kjain@...ux.ibm.com>,
        John Garry <john.g.garry@...cle.com>,
        Kan Liang <kan.liang@...ux.intel.com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Andrii Nakryiko <andrii@...nel.org>,
        Eduard Zingerman <eddyz87@...il.com>,
        Suzuki Poulouse <suzuki.poulose@....com>,
        Leo Yan <leo.yan@...aro.org>,
        Florian Fischer <florian.fischer@...q.space>,
        Ravi Bangoria <ravi.bangoria@....com>,
        Jing Zhang <renyu.zj@...ux.alibaba.com>,
        Sean Christopherson <seanjc@...gle.com>,
        Athira Rajeev <atrajeev@...ux.vnet.ibm.com>,
        linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org,
        linux-stm32@...md-mailman.stormreply.com,
        linux-arm-kernel@...ts.infradead.org,
        Perry Taylor <perry.taylor@...el.com>,
        Caleb Biggers <caleb.biggers@...el.com>,
        Stephane Eranian <eranian@...gle.com>
Subject: Re: [PATCH v1 50/51] perf stat: Use counts rather than saved_value

On Fri, Feb 24, 2023 at 2:48 PM Namhyung Kim <namhyung@...nel.org> wrote:
>
> On Sun, Feb 19, 2023 at 01:28:47AM -0800, Ian Rogers wrote:
> > Switch the hard coded metrics to use the aggregate value rather than
> > from saved_value. When computing a metric like IPC the aggregate count
> > comes from instructions then cycles is looked up and if present IPC
> > computed. Rather than lookup from the saved_value rbtree, search the
> > counter's evlist for the desired counter.
> >
> > A new helper evsel__stat_type is used to both quickly find a metric
> > function and to identify when a counter is the one being sought. So
> > that both total and miss counts can be sought, the stat_type enum is
> > expanded. The ratio functions are rewritten to share a common helper
> > with the ratios being directly passed rather than computed from an
> > enum value.
> >
> > Signed-off-by: Ian Rogers <irogers@...gle.com>
> > ---
> [SNIP]
> > -static double runtime_stat_avg(enum stat_type type, int aggr_idx,
> > -                            struct runtime_stat_data *rsd)
> > +static double find_stat(const struct evsel *evsel, int aggr_idx, enum stat_type type)
> >  {
> > -     struct saved_value *v;
> > -
> > -     v = saved_value_lookup(NULL, aggr_idx, false, type, rsd->ctx, rsd->cgrp);
> > -     if (!v)
> > -             return 0.0;
> > -
> > -     return avg_stats(&v->stats);
> > +     const struct evsel *cur;
> > +     int evsel_ctx = evsel_context(evsel);
> > +
> > +     evlist__for_each_entry(evsel->evlist, cur) {
> > +             struct perf_stat_aggr *aggr;
> > +
> > +             /* Ignore the evsel that is being searched from. */
> > +             if (evsel == cur)
> > +                     continue;
> > +
> > +             /* Ignore evsels that are part of different groups. */
> > +             if (evsel->core.leader->nr_members &&
> > +                 evsel->core.leader != cur->core.leader)
>
> The evsel->nr_members is somewhat confusing in that it counts itself
> as a member.  I'm not sure it resets the nr_members to 0 for standalone
> events.  You'd better checking nr_members greater than 1 for group
> events.

Agreed. The code is correct as the nr_members is only set when the
group is closed by the call to parse_events_set_leader, standalone
events don't close a group and so have nr_members of 0, but I agree
that's confusing.

I'm actually looking at a related bug where telling metrics not to
group events breaks the topdown events that must be grouped with
slots.

One thing that bugs me is the libperf evsel idx variable. When an
evsel is added to an evlist the idx is that number of elements in the
evlist. However, we reorganize the list in parse-events and so the idx
is just a hopefully unique value in the list. In places in parse
events we use the idx for computing the length of the evlist by
subtracting the last idx from the first and adding 1. Removing the idx
isn't straightforward though as later on it is used for mmaps.

Thanks,
Ian

> Thanks,
> Namhyung
>
>
> > +                     continue;
> > +             /* Ignore evsels with mismatched modifiers. */
> > +             if (evsel_ctx != evsel_context(cur))
> > +                     continue;
> > +             /* Ignore if not the cgroup we're looking for. */
> > +             if (evsel->cgrp != cur->cgrp)
> > +                     continue;
> > +             /* Ignore if not the stat we're looking for. */
> > +             if (type != evsel__stat_type(cur))
> > +                     continue;
> > +
> > +             aggr = &cur->stats->aggr[aggr_idx];
> > +             if (type == STAT_NSECS)
> > +                     return aggr->counts.val;
> > +             return aggr->counts.val * cur->scale;
> > +     }
> > +     return 0.0;
> >  }

Powered by blists - more mailing lists