* [PATCH] perf stat: Fix the hard-coded metrics calculation on the hybrid
@ 2024-06-05 16:08 kan.liang
2024-06-05 17:21 ` Ian Rogers
0 siblings, 1 reply; 4+ messages in thread
From: kan.liang @ 2024-06-05 16:08 UTC (permalink / raw)
To: acme, namhyung, irogers, jolsa, adrian.hunter, linux-perf-users,
linux-kernel
Cc: Kan Liang, Khalil, Amiri, stable
From: Kan Liang <kan.liang@linux.intel.com>
The hard-coded metrics is wrongly calculated on the hybrid machine.
$ perf stat -e cycles,instructions -a sleep 1
Performance counter stats for 'system wide':
18,205,487 cpu_atom/cycles/
9,733,603 cpu_core/cycles/
9,423,111 cpu_atom/instructions/ # 0.52 insn per cycle
4,268,965 cpu_core/instructions/ # 0.23 insn per cycle
The insn per cycle for cpu_core should be 4,268,965 / 9,733,603 = 0.44.
When finding the metric events, the find_stat() doesn't take the PMU
type into account. The cpu_atom/cycles/ is wrongly used to calculate
the IPC of the cpu_core.
Fixes: 0a57b910807a ("perf stat: Use counts rather than saved_value")
Reported-by: "Khalil, Amiri" <amiri.khalil@intel.com>
Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
Cc: stable@vger.kernel.org
---
tools/perf/util/stat-shadow.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
index 3466aa952442..4d0edc061f1a 100644
--- a/tools/perf/util/stat-shadow.c
+++ b/tools/perf/util/stat-shadow.c
@@ -176,6 +176,10 @@ static double find_stat(const struct evsel *evsel, int aggr_idx, enum stat_type
if (type != evsel__stat_type(cur))
continue;
+ /* Ignore if not the PMU we're looking for. */
+ if (evsel->pmu != cur->pmu)
+ continue;
+
aggr = &cur->stats->aggr[aggr_idx];
if (type == STAT_NSECS)
return aggr->counts.val;
--
2.35.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] perf stat: Fix the hard-coded metrics calculation on the hybrid 2024-06-05 16:08 [PATCH] perf stat: Fix the hard-coded metrics calculation on the hybrid kan.liang @ 2024-06-05 17:21 ` Ian Rogers 2024-06-06 7:34 ` Namhyung Kim 0 siblings, 1 reply; 4+ messages in thread From: Ian Rogers @ 2024-06-05 17:21 UTC (permalink / raw) To: kan.liang Cc: acme, namhyung, jolsa, adrian.hunter, linux-perf-users, linux-kernel, Khalil, Amiri, stable On Wed, Jun 5, 2024 at 9:10 AM <kan.liang@linux.intel.com> wrote: > > From: Kan Liang <kan.liang@linux.intel.com> > > The hard-coded metrics is wrongly calculated on the hybrid machine. > > $ perf stat -e cycles,instructions -a sleep 1 > > Performance counter stats for 'system wide': > > 18,205,487 cpu_atom/cycles/ > 9,733,603 cpu_core/cycles/ > 9,423,111 cpu_atom/instructions/ # 0.52 insn per cycle > 4,268,965 cpu_core/instructions/ # 0.23 insn per cycle > > The insn per cycle for cpu_core should be 4,268,965 / 9,733,603 = 0.44. > > When finding the metric events, the find_stat() doesn't take the PMU > type into account. The cpu_atom/cycles/ is wrongly used to calculate > the IPC of the cpu_core. > > Fixes: 0a57b910807a ("perf stat: Use counts rather than saved_value") > Reported-by: "Khalil, Amiri" <amiri.khalil@intel.com> > Signed-off-by: Kan Liang <kan.liang@linux.intel.com> Reviewed-by: Ian Rogers <irogers@google.com> Thanks, Ian > Cc: stable@vger.kernel.org > --- > tools/perf/util/stat-shadow.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c > index 3466aa952442..4d0edc061f1a 100644 > --- a/tools/perf/util/stat-shadow.c > +++ b/tools/perf/util/stat-shadow.c > @@ -176,6 +176,10 @@ static double find_stat(const struct evsel *evsel, int aggr_idx, enum stat_type > if (type != evsel__stat_type(cur)) > continue; > > + /* Ignore if not the PMU we're looking for. */ > + if (evsel->pmu != cur->pmu) > + continue; > + > aggr = &cur->stats->aggr[aggr_idx]; > if (type == STAT_NSECS) > return aggr->counts.val; > -- > 2.35.1 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] perf stat: Fix the hard-coded metrics calculation on the hybrid 2024-06-05 17:21 ` Ian Rogers @ 2024-06-06 7:34 ` Namhyung Kim 2024-06-06 13:44 ` Liang, Kan 0 siblings, 1 reply; 4+ messages in thread From: Namhyung Kim @ 2024-06-06 7:34 UTC (permalink / raw) To: Ian Rogers Cc: kan.liang, acme, jolsa, adrian.hunter, linux-perf-users, linux-kernel, Khalil, Amiri, stable On Wed, Jun 5, 2024 at 10:21 AM Ian Rogers <irogers@google.com> wrote: > > On Wed, Jun 5, 2024 at 9:10 AM <kan.liang@linux.intel.com> wrote: > > > > From: Kan Liang <kan.liang@linux.intel.com> > > > > The hard-coded metrics is wrongly calculated on the hybrid machine. > > > > $ perf stat -e cycles,instructions -a sleep 1 > > > > Performance counter stats for 'system wide': > > > > 18,205,487 cpu_atom/cycles/ > > 9,733,603 cpu_core/cycles/ > > 9,423,111 cpu_atom/instructions/ # 0.52 insn per cycle > > 4,268,965 cpu_core/instructions/ # 0.23 insn per cycle > > > > The insn per cycle for cpu_core should be 4,268,965 / 9,733,603 = 0.44. > > > > When finding the metric events, the find_stat() doesn't take the PMU > > type into account. The cpu_atom/cycles/ is wrongly used to calculate > > the IPC of the cpu_core. > > > > Fixes: 0a57b910807a ("perf stat: Use counts rather than saved_value") > > Reported-by: "Khalil, Amiri" <amiri.khalil@intel.com> > > Signed-off-by: Kan Liang <kan.liang@linux.intel.com> > > Reviewed-by: Ian Rogers <irogers@google.com> > > Thanks, > Ian > > > Cc: stable@vger.kernel.org > > --- > > tools/perf/util/stat-shadow.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c > > index 3466aa952442..4d0edc061f1a 100644 > > --- a/tools/perf/util/stat-shadow.c > > +++ b/tools/perf/util/stat-shadow.c > > @@ -176,6 +176,10 @@ static double find_stat(const struct evsel *evsel, int aggr_idx, enum stat_type > > if (type != evsel__stat_type(cur)) > > continue; > > > > + /* Ignore if not the PMU we're looking for. */ > > + if (evsel->pmu != cur->pmu) > > + continue; Hmm.. Don't some metrics need events from different PMU? Like cycles per sec or branch instructions per sec.. Thanks, Namhyung > > + > > aggr = &cur->stats->aggr[aggr_idx]; > > if (type == STAT_NSECS) > > return aggr->counts.val; > > -- > > 2.35.1 > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] perf stat: Fix the hard-coded metrics calculation on the hybrid 2024-06-06 7:34 ` Namhyung Kim @ 2024-06-06 13:44 ` Liang, Kan 0 siblings, 0 replies; 4+ messages in thread From: Liang, Kan @ 2024-06-06 13:44 UTC (permalink / raw) To: Namhyung Kim, Ian Rogers Cc: acme, jolsa, adrian.hunter, linux-perf-users, linux-kernel, Khalil, Amiri, stable On 2024-06-06 3:34 a.m., Namhyung Kim wrote: > On Wed, Jun 5, 2024 at 10:21 AM Ian Rogers <irogers@google.com> wrote: >> >> On Wed, Jun 5, 2024 at 9:10 AM <kan.liang@linux.intel.com> wrote: >>> >>> From: Kan Liang <kan.liang@linux.intel.com> >>> >>> The hard-coded metrics is wrongly calculated on the hybrid machine. >>> >>> $ perf stat -e cycles,instructions -a sleep 1 >>> >>> Performance counter stats for 'system wide': >>> >>> 18,205,487 cpu_atom/cycles/ >>> 9,733,603 cpu_core/cycles/ >>> 9,423,111 cpu_atom/instructions/ # 0.52 insn per cycle >>> 4,268,965 cpu_core/instructions/ # 0.23 insn per cycle >>> >>> The insn per cycle for cpu_core should be 4,268,965 / 9,733,603 = 0.44. >>> >>> When finding the metric events, the find_stat() doesn't take the PMU >>> type into account. The cpu_atom/cycles/ is wrongly used to calculate >>> the IPC of the cpu_core. >>> >>> Fixes: 0a57b910807a ("perf stat: Use counts rather than saved_value") >>> Reported-by: "Khalil, Amiri" <amiri.khalil@intel.com> >>> Signed-off-by: Kan Liang <kan.liang@linux.intel.com> >> >> Reviewed-by: Ian Rogers <irogers@google.com> >> >> Thanks, >> Ian >> >>> Cc: stable@vger.kernel.org >>> --- >>> tools/perf/util/stat-shadow.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c >>> index 3466aa952442..4d0edc061f1a 100644 >>> --- a/tools/perf/util/stat-shadow.c >>> +++ b/tools/perf/util/stat-shadow.c >>> @@ -176,6 +176,10 @@ static double find_stat(const struct evsel *evsel, int aggr_idx, enum stat_type >>> if (type != evsel__stat_type(cur)) >>> continue; >>> >>> + /* Ignore if not the PMU we're looking for. */ >>> + if (evsel->pmu != cur->pmu) >>> + continue; > > Hmm.. Don't some metrics need events from different PMU? > Like cycles per sec or branch instructions per sec.. > Right. In the hard-coded metrics, the events from a different PMU are SW_CPU_CLOCK and SW_TASK_CLOCK. They both have the stat type, STAT_NSECS. Perf should ignore the PMU checking for the type as below. I will send a V2 to fix it. diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c index 3466aa952442..d01335f18808 100644 --- a/tools/perf/util/stat-shadow.c +++ b/tools/perf/util/stat-shadow.c @@ -176,6 +176,9 @@ static double find_stat(const struct evsel *evsel, int aggr_idx, enum stat_type if (type != evsel__stat_type(cur)) continue; + if ((type != STAT_NSECS) && (evsel->pmu != cur->pmu)) + continue; + aggr = &cur->stats->aggr[aggr_idx]; if (type == STAT_NSECS) return aggr->counts.val; Thanks, Kan > Thanks, > Namhyung > > >>> + >>> aggr = &cur->stats->aggr[aggr_idx]; >>> if (type == STAT_NSECS) >>> return aggr->counts.val; >>> -- >>> 2.35.1 >>> > ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-06-06 13:44 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-06-05 16:08 [PATCH] perf stat: Fix the hard-coded metrics calculation on the hybrid kan.liang 2024-06-05 17:21 ` Ian Rogers 2024-06-06 7:34 ` Namhyung Kim 2024-06-06 13:44 ` Liang, Kan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).