| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 11 Oct 2022 16:08:38 -0700
From: Namhyung Kim <namhyung@...nel.org>
To: Ian Rogers <irogers@...gle.com>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>,
Jiri Olsa <jolsa@...nel.org>, Ingo Molnar <mingo@...nel.org>,
Peter Zijlstra <peterz@...radead.org>,
LKML <linux-kernel@...r.kernel.org>,
Adrian Hunter <adrian.hunter@...el.com>,
linux-perf-users <linux-perf-users@...r.kernel.org>,
Kan Liang <kan.liang@...ux.intel.com>,
Leo Yan <leo.yan@...aro.org>, Andi Kleen <ak@...ux.intel.com>,
Athira Rajeev <atrajeev@...ux.vnet.ibm.com>,
James Clark <james.clark@....com>,
Xing Zhengjun <zhengjun.xing@...ux.intel.com>
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@...gle.com> wrote:
>
> On Sun, Oct 9, 2022 at 10:36 PM Namhyung Kim <namhyung@...nel.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@...nel.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
Powered by blists - more mailing lists