Messages in this thread | | | Date | Wed, 1 Mar 2023 14:29:13 -0500 | Subject | Re: [RFC PATCH v2 1/2] perf/x86/rapl: Fix energy-cores event | From | "Liang, Kan" <> |
| |
On 2023-03-01 1:14 p.m., Wyes Karny wrote: > For quite some time, energy-cores event is broken, because RAPL PMU > assumes all the events on this PMU are uncore and sets rapl_cpu_mask > with the first available CPU on the die. Therefore, for energy-cores > event if we read MSR form pmu->cpu, it's wrong. But the following two > changes helped to hide this issue. > > - commit 704e2f5b700d ("perf stat: Use affinity for enabling/disabling > events") > - commit e64cd6f73ff5 ("perf/x86: Use PMUEF_READ_CPU_PKG in uncore > events") > > These two changes together acted as a workaround for energy-cores event. > First change affined perf events to respective CPUs whereas the second > change helped to pick the local CPU to read the MSR. In this way, MSRs > were read from the correct CPU. This works unless it's the first > reading. For the first reading the second patch doesn't apply and we > get wrong readings. Stephane reported this issue when a patch to enable > AMD energy-cores RAPL event was posted [1]. > > The right way to fix the issue is to get rid of RAPL being considered an > uncore event. That is a larger change. To enable current RAPL usage, > work around the issue by conditionally remove the > `PERF_EV_CAP_READ_ACTIVE_PKG` flag for energy-cores event. Also, use the > event's CPU instead for PMU's CPU to read the MSR.
The current RAPL PMU aka 'power' should be die/socket scope. The energy-cores event is also defined as a die/socket scope.
* pp0 counter: consumption of all physical cores (power plane 0) * event: rapl_energy_cores * perf code: 0x1
I don't think we want to change the scope of the energy-cores event. Otherwise Intel's energy-cores event probably be broken.
It looks like you are looking for a new per-core RAPL event. I think it's better to create a new core scope RAPL PMU.
Thanks, Kan
> > [1]: https://lore.kernel.org/lkml/CABPqkBQ_bSTC-OEe_LrgUrpj2VsseX1ThvO-yLcEtF8vb4+AAw@mail.gmail.com/#t > > Fixes: e64cd6f73ff5 ("perf/x86: Use PMUEF_READ_CPU_PKG in uncore events") > Signed-off-by: Wyes Karny <wyes.karny@amd.com> > --- > arch/x86/events/rapl.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c > index 52e6e7ed4f78..e6a0c077daf5 100644 > --- a/arch/x86/events/rapl.c > +++ b/arch/x86/events/rapl.c > @@ -343,14 +343,15 @@ static int rapl_pmu_event_init(struct perf_event *event) > if (event->cpu < 0) > return -EINVAL; > > - event->event_caps |= PERF_EV_CAP_READ_ACTIVE_PKG; > - > if (!cfg || cfg >= NR_RAPL_DOMAINS + 1) > return -EINVAL; > > cfg = array_index_nospec((long)cfg, NR_RAPL_DOMAINS + 1); > bit = cfg - 1; > > + if (bit != PERF_RAPL_PP0) > + event->event_caps |= PERF_EV_CAP_READ_ACTIVE_PKG; > + > /* check event supported */ > if (!(rapl_cntr_mask & (1 << bit))) > return -EINVAL; > @@ -363,7 +364,15 @@ static int rapl_pmu_event_init(struct perf_event *event) > pmu = cpu_to_rapl_pmu(event->cpu); > if (!pmu) > return -EINVAL; > - event->cpu = pmu->cpu; > + > + /* > + * FIXME: RAPL PMU considers events are uncore and MSRs can be read from > + * the first available CPU of the die. But this is not true for energy-cores > + * event. Therefore as a workaround don't consider pmu->cpu here for PERF_RAPL_PP0. > + */ > + if (event->event_caps & PERF_EV_CAP_READ_ACTIVE_PKG) > + event->cpu = pmu->cpu; > + > event->pmu_private = pmu; > event->hw.event_base = rapl_msrs[bit].msr; > event->hw.config = cfg;
| |