Re: [PATCH 04/19] perf stat: Add aggr id for global mode

[Namhyung Kim]

On Mon, Oct 10, 2022 at 3:46 PM Ian Rogers <irogers@xxxxxxxxxx> wrote:
>
> On Sun, Oct 9, 2022 at 10:36 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
> >
> > To make the code simpler, I'd like to use the same aggregation code for
> > the global mode.  We can simply add an id function to return cpu 0 and
> > use print_aggr().
> >
> > No functional change intended.
> >
> > Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
> > ---
> >  tools/perf/builtin-stat.c      | 39 ++++++++++++++++++++++++++++++++--
> >  tools/perf/util/cpumap.c       | 10 +++++++++
> >  tools/perf/util/cpumap.h       |  6 +++++-
> >  tools/perf/util/stat-display.c |  9 ++------
> >  4 files changed, 54 insertions(+), 10 deletions(-)
> >
> > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> > index 265b05157972..144bb3a657f2 100644
> > --- a/tools/perf/builtin-stat.c
> > +++ b/tools/perf/builtin-stat.c
> > @@ -1330,6 +1330,15 @@ static struct aggr_cpu_id perf_stat__get_node(struct perf_stat_config *config __
> >         return aggr_cpu_id__node(cpu, /*data=*/NULL);
> >  }
> >
> > +static struct aggr_cpu_id perf_stat__get_global(struct perf_stat_config *config __maybe_unused,
> > +                                               struct perf_cpu cpu __maybe_unused)
> > +{
> > +       struct aggr_cpu_id id = aggr_cpu_id__empty();
> > +
> > +       id.cpu = (struct perf_cpu){ .cpu = 0 };
> > +       return id;
> > +}
> > +
>
> See below, I think this should just return aggr_cpu_id__global or just
> call that directly.

Ok, will do.

>
> >  static struct aggr_cpu_id perf_stat__get_aggr(struct perf_stat_config *config,
> >                                               aggr_get_id_t get_id, struct perf_cpu cpu)
> >  {
> > @@ -1366,6 +1375,12 @@ static struct aggr_cpu_id perf_stat__get_node_cached(struct perf_stat_config *co
> >         return perf_stat__get_aggr(config, perf_stat__get_node, cpu);
> >  }
> >
> > +static struct aggr_cpu_id perf_stat__get_global_cached(struct perf_stat_config *config,
> > +                                                      struct perf_cpu cpu)
> > +{
> > +       return perf_stat__get_aggr(config, perf_stat__get_global, cpu);
> > +}
> > +
> >  static bool term_percore_set(void)
> >  {
> >         struct evsel *counter;
> > @@ -1395,6 +1410,7 @@ static aggr_cpu_id_get_t aggr_mode__get_aggr(enum aggr_mode aggr_mode)
> >
> >                 return NULL;
> >         case AGGR_GLOBAL:
> > +               return aggr_cpu_id__global;
> >         case AGGR_THREAD:
> >         case AGGR_UNSET:
> >         case AGGR_MAX:
> > @@ -1420,6 +1436,7 @@ static aggr_get_id_t aggr_mode__get_id(enum aggr_mode aggr_mode)
> >                 }
> >                 return NULL;
> >         case AGGR_GLOBAL:
> > +               return perf_stat__get_global_cached;
> >         case AGGR_THREAD:
> >         case AGGR_UNSET:
> >         case AGGR_MAX:
> > @@ -1535,6 +1552,16 @@ static struct aggr_cpu_id perf_env__get_node_aggr_by_cpu(struct perf_cpu cpu, vo
> >         return id;
> >  }
> >
> > +static struct aggr_cpu_id perf_env__get_global_aggr_by_cpu(struct perf_cpu cpu __maybe_unused,
> > +                                                          void *data __maybe_unused)
> > +{
> > +       struct aggr_cpu_id id = aggr_cpu_id__empty();
> > +
> > +       /* it always aggregates to the cpu 0 */
> > +       id.cpu = (struct perf_cpu){ .cpu = 0 };
> > +       return id;
> > +}
> > +
> >  static struct aggr_cpu_id perf_stat__get_socket_file(struct perf_stat_config *config __maybe_unused,
> >                                                      struct perf_cpu cpu)
> >  {
> > @@ -1558,6 +1585,12 @@ static struct aggr_cpu_id perf_stat__get_node_file(struct perf_stat_config *conf
> >         return perf_env__get_node_aggr_by_cpu(cpu, &perf_stat.session->header.env);
> >  }
> >
> > +static struct aggr_cpu_id perf_stat__get_global_file(struct perf_stat_config *config __maybe_unused,
> > +                                                    struct perf_cpu cpu)
> > +{
> > +       return perf_env__get_global_aggr_by_cpu(cpu, &perf_stat.session->header.env);
> > +}
> > +
> >  static aggr_cpu_id_get_t aggr_mode__get_aggr_file(enum aggr_mode aggr_mode)
> >  {
> >         switch (aggr_mode) {
> > @@ -1569,8 +1602,9 @@ static aggr_cpu_id_get_t aggr_mode__get_aggr_file(enum aggr_mode aggr_mode)
> >                 return perf_env__get_core_aggr_by_cpu;
> >         case AGGR_NODE:
> >                 return perf_env__get_node_aggr_by_cpu;
> > -       case AGGR_NONE:
> >         case AGGR_GLOBAL:
> > +               return perf_env__get_global_aggr_by_cpu;
> > +       case AGGR_NONE:
> >         case AGGR_THREAD:
> >         case AGGR_UNSET:
> >         case AGGR_MAX:
> > @@ -1590,8 +1624,9 @@ static aggr_get_id_t aggr_mode__get_id_file(enum aggr_mode aggr_mode)
> >                 return perf_stat__get_core_file;
> >         case AGGR_NODE:
> >                 return perf_stat__get_node_file;
> > -       case AGGR_NONE:
> >         case AGGR_GLOBAL:
> > +               return perf_stat__get_global_file;
> > +       case AGGR_NONE:
> >         case AGGR_THREAD:
> >         case AGGR_UNSET:
> >         case AGGR_MAX:
> > diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
> > index 8486ca3bec75..60209fe87456 100644
> > --- a/tools/perf/util/cpumap.c
> > +++ b/tools/perf/util/cpumap.c
> > @@ -354,6 +354,16 @@ struct aggr_cpu_id aggr_cpu_id__node(struct perf_cpu cpu, void *data __maybe_unu
> >         return id;
> >  }
> >
> > +struct aggr_cpu_id aggr_cpu_id__global(struct perf_cpu cpu, void *data __maybe_unused)
>
> Is this a duplicate of aggr_cpu_id perf_stat__get_global? Could we
> replace all uses of the former with this one?

They are very similar but used for different purposes.
I'll think about how to simplify this code more.

Thanks,
Namhyung