| 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: Wed, 1 Feb 2023 22:31:00 -0300
From: Arnaldo Carvalho de Melo <acme@...nel.org>
To: Ian Rogers <irogers@...gle.com>
Cc: Namhyung Kim <namhyung@...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@...r.kernel.org,
Athira Jajeev <atrajeev@...ux.vnet.ibm.com>,
Michael Petlan <mpetlan@...hat.com>
Subject: Re: [PATCH v2] perf stat: Hide invalid uncore event output for aggr
mode
Em Wed, Jan 25, 2023 at 04:10:39PM -0800, Ian Rogers escreveu:
> On Wed, Jan 25, 2023 at 11:24 AM Namhyung Kim <namhyung@...nel.org> wrote:
> >
> > The current display code for perf stat iterates given cpus and build the
> > aggr map to collect the event data for the aggregation mode.
> >
> > But uncore events have their own cpu maps and it won't guarantee that
> > it'd match to the aggr map. For example, per-package uncore events
> > would generate a single value for each socket. When user asks per-core
> > aggregation mode, the output would contain 0 values for other cores.
Thanks, applied.
- Arnaldo
>
> > Thus it needs to check the uncore PMU's cpumask and if it matches to the
> > current aggregation id.
> >
> > Before:
> > $ sudo ./perf stat -a --per-core -e power/energy-pkg/ sleep 1
> >
> > Performance counter stats for 'system wide':
> >
> > S0-D0-C0 1 3.73 Joules power/energy-pkg/
> > S0-D0-C1 0 <not counted> Joules power/energy-pkg/
> > S0-D0-C2 0 <not counted> Joules power/energy-pkg/
> > S0-D0-C3 0 <not counted> Joules power/energy-pkg/
> >
> > 1.001404046 seconds time elapsed
> >
> > Some events weren't counted. Try disabling the NMI watchdog:
> > echo 0 > /proc/sys/kernel/nmi_watchdog
> > perf stat ...
> > echo 1 > /proc/sys/kernel/nmi_watchdog
> >
> > The core 1, 2 and 3 should not be printed because the event is handled
> > in a cpu in the core 0 only. With this change, the output becomes like
> > below.
> >
> > After:
> > $ sudo ./perf stat -a --per-core -e power/energy-pkg/ sleep 1
> >
> > Performance counter stats for 'system wide':
> >
> > S0-D0-C0 1 2.09 Joules power/energy-pkg/
> >
> > Fixes: b89761351089 ("perf stat: Update event skip condition for system-wide per-thread mode and merged uncore and hybrid events")
> > Cc: Athira Jajeev <atrajeev@...ux.vnet.ibm.com>
> > Cc: Michael Petlan <mpetlan@...hat.com>
> > Tested-by: Ian Rogers <irogers@...gle.com>
> > Signed-off-by: Namhyung Kim <namhyung@...nel.org>
>
> Acked-by: Ian Rogers <irogers@...gle.com>
>
> Thanks,
> Ian
>
> > ---
> > * rename to 'should_skip_zero_counter'
> > * check pmu->cpus instead
> > * add kernel-doc style comments
> > * add Ian's Tested-by tag
> >
> > tools/perf/util/stat-display.c | 51 ++++++++++++++++++++++++++++++----
> > 1 file changed, 46 insertions(+), 5 deletions(-)
> >
> > diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> > index 8bd8b0142630..1b5cb20efd23 100644
> > --- a/tools/perf/util/stat-display.c
> > +++ b/tools/perf/util/stat-display.c
> > @@ -787,6 +787,51 @@ static void uniquify_counter(struct perf_stat_config *config, struct evsel *coun
> > uniquify_event_name(counter);
> > }
> >
> > +/**
> > + * should_skip_zero_count() - Check if the event should print 0 values.
> > + * @config: The perf stat configuration (including aggregation mode).
> > + * @counter: The evsel with its associated cpumap.
> > + * @id: The aggregation id that is being queried.
> > + *
> > + * Due to mismatch between the event cpumap or thread-map and the
> > + * aggregation mode, sometimes it'd iterate the counter with the map
> > + * which does not contain any values.
> > + *
> > + * For example, uncore events have dedicated CPUs to manage them,
> > + * result for other CPUs should be zero and skipped.
> > + *
> > + * Return: %true if the value should NOT be printed, %false if the value
> > + * needs to be printed like "<not counted>" or "<not supported>".
> > + */
> > +static bool should_skip_zero_counter(struct perf_stat_config *config,
> > + struct evsel *counter,
> > + const struct aggr_cpu_id *id)
> > +{
> > + struct perf_cpu cpu;
> > + int idx;
> > +
> > + /*
> > + * Skip value 0 when enabling --per-thread globally,
> > + * otherwise it will have too many 0 output.
> > + */
> > + if (config->aggr_mode == AGGR_THREAD && config->system_wide)
> > + return true;
> > + /*
> > + * Skip value 0 when it's an uncore event and the given aggr id
> > + * does not belong to the PMU cpumask.
> > + */
> > + if (!counter->pmu || !counter->pmu->is_uncore)
> > + return false;
> > +
> > + perf_cpu_map__for_each_cpu(cpu, idx, counter->pmu->cpus) {
> > + struct aggr_cpu_id own_id = config->aggr_get_id(config, cpu);
> > +
> > + if (aggr_cpu_id__equal(id, &own_id))
> > + return false;
> > + }
> > + return true;
> > +}
> > +
> > static void print_counter_aggrdata(struct perf_stat_config *config,
> > struct evsel *counter, int s,
> > struct outstate *os)
> > @@ -814,11 +859,7 @@ static void print_counter_aggrdata(struct perf_stat_config *config,
> > ena = aggr->counts.ena;
> > run = aggr->counts.run;
> >
> > - /*
> > - * Skip value 0 when enabling --per-thread globally, otherwise it will
> > - * have too many 0 output.
> > - */
> > - if (val == 0 && config->aggr_mode == AGGR_THREAD && config->system_wide)
> > + if (val == 0 && should_skip_zero_counter(config, counter, &id))
> > return;
> >
> > if (!metric_only) {
> > --
> > 2.39.1.456.gfc5497dd1b-goog
> >
--
- Arnaldo
Powered by blists - more mailing lists