Messages in this thread | | | From | Namhyung Kim <> | Date | Tue, 11 Oct 2022 16:08:38 -0700 | Subject | Re: [PATCH 04/19] perf stat: Add aggr id for global mode |
| |
On Mon, Oct 10, 2022 at 3:46 PM Ian Rogers <irogers@google.com> wrote: > > On Sun, Oct 9, 2022 at 10:36 PM Namhyung Kim <namhyung@kernel.org> 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@kernel.org> > > --- > > 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
| |