Re: [PATCH 3/6] perf stat: Rename saved_value->cpu_map_idx

[Ian Rogers]

On Wed, Sep 28, 2022 at 4:57 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
>
> Hello,
>
> On Wed, Sep 28, 2022 at 3:50 AM James Clark <james.clark@xxxxxxx> wrote:
> >
> >
> >
> > On 26/09/2022 21:07, Namhyung Kim wrote:
> > > The cpu_map_idx fields is just to differentiate values from other
> > > entries.  It doesn't need to be strictly cpu map index.  Actually we can
> > > pass thread map index or aggr map index.  So rename the fields first.
> > >
> > > No functional change intended.
> > >
> > > Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
> > > ---
> > >  tools/perf/util/stat-shadow.c | 308 +++++++++++++++++-----------------
> > >  1 file changed, 154 insertions(+), 154 deletions(-)
> > >
> > > diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
> > > index 9e1eddeff21b..99d05262055c 100644
> > > --- a/tools/perf/util/stat-shadow.c
> > > +++ b/tools/perf/util/stat-shadow.c
> > > @@ -33,7 +33,7 @@ struct saved_value {
> > >       struct evsel *evsel;
> > >       enum stat_type type;
> > >       int ctx;
> > > -     int cpu_map_idx;
> > > +     int map_idx;
> >
> > Do the same variables in stat.c and stat.h also need to be updated? The
> > previous change to do this exact thing (5b1af93dbc7e) changed more than
> > just these ones.
>
> Thanks for your review!  I'll change the header too.
>
> Note that callers of perf_stat__update_shadow_stats() are free
> to use cpu_map_idx as they want.  The previous change fixed
> confusion between cpu number and map index.  Actually either
> is fine for us as long as it's used consistently.  But we use the
> cpu map index for most cases.
>
> Thanks,
> Namhyung

It is only fine to interchange CPU and CPU map index if the CPU map
contains all CPUs and not the any CPU entry. I wonder if we should
introduce a 'struct thread' to wrap the pid_t in thread maps to avoid
swapping threads and indices? Given pids are not generally in the same
range as indices (unlike CPU numbers which substantially broke
aggregation) it is much less likely this is broken. In any case it is
worth documenting map_idx to say what indices it is expected to hold.

Thanks,
Ian