[2/3] perf/x86/rapl: Fix energy counter detection
diff mbox series

Message ID 20210115092208.20866-2-rui.zhang@intel.com
State New, archived
Headers show
Series
  • [1/3] perf/x86/rapl: Add msr mask support
Related show

Commit Message

Zhang, Rui Jan. 15, 2021, 9:22 a.m. UTC
In the RAPL ENERGY_COUNTER MSR, only the lower 32bits represent the
energy counter, and the higher 32bits are reserved.

Add the MSR mask for these MSRs to fix a problem that the RAPL PMU events
are added erroneously when higher 32bits contain non-zero value.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
Reviewed-by: Andi Kleen <ak@linux.intel.com>
---
 arch/x86/events/rapl.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

Comments

Peter Zijlstra Jan. 15, 2021, 8:03 p.m. UTC | #1
On Fri, Jan 15, 2021 at 05:22:07PM +0800, Zhang Rui wrote:
> In the RAPL ENERGY_COUNTER MSR, only the lower 32bits represent the
> energy counter, and the higher 32bits are reserved.
> 
> Add the MSR mask for these MSRs to fix a problem that the RAPL PMU events
> are added erroneously when higher 32bits contain non-zero value.

Why would these high bits be non-zero?
Zhang, Rui Jan. 16, 2021, 8:19 a.m. UTC | #2
> -----Original Message-----
> From: Peter Zijlstra <peterz@infradead.org>
> Sent: Saturday, January 16, 2021 4:03 AM
> To: Zhang, Rui <rui.zhang@intel.com>
> Cc: mingo@redhat.com; acme@kernel.org; mark.rutland@arm.com;
> alexander.shishkin@linux.intel.com; jolsa@redhat.com;
> namhyung@kernel.org; linux-kernel@vger.kernel.org; x86@kernel.org;
> kan.liang@linux.intel.com; ak@linux.intel.com
> Subject: Re: [PATCH 2/3] perf/x86/rapl: Fix energy counter detection
> Importance: High
> 
> On Fri, Jan 15, 2021 at 05:22:07PM +0800, Zhang Rui wrote:
> > In the RAPL ENERGY_COUNTER MSR, only the lower 32bits represent the
> > energy counter, and the higher 32bits are reserved.
> >
> > Add the MSR mask for these MSRs to fix a problem that the RAPL PMU
> > events are added erroneously when higher 32bits contain non-zero value.
> 
> Why would these high bits be non-zero?

On SPR platform, the high bits of Psys energy counter are reused for other purpose.
High bits for other RAPL domains energy counters still return 0.

I didn't mention this because I thought this patch should be okay as a generic fix.

Thanks,
rui
Peter Zijlstra Jan. 16, 2021, 12:48 p.m. UTC | #3
On Sat, Jan 16, 2021 at 08:19:35AM +0000, Zhang, Rui wrote:
> 
> 
> > -----Original Message-----
> > From: Peter Zijlstra <peterz@infradead.org>
> > Sent: Saturday, January 16, 2021 4:03 AM
> > To: Zhang, Rui <rui.zhang@intel.com>
> > Cc: mingo@redhat.com; acme@kernel.org; mark.rutland@arm.com;
> > alexander.shishkin@linux.intel.com; jolsa@redhat.com;
> > namhyung@kernel.org; linux-kernel@vger.kernel.org; x86@kernel.org;
> > kan.liang@linux.intel.com; ak@linux.intel.com
> > Subject: Re: [PATCH 2/3] perf/x86/rapl: Fix energy counter detection
> > Importance: High
> > 
> > On Fri, Jan 15, 2021 at 05:22:07PM +0800, Zhang Rui wrote:
> > > In the RAPL ENERGY_COUNTER MSR, only the lower 32bits represent the
> > > energy counter, and the higher 32bits are reserved.
> > >
> > > Add the MSR mask for these MSRs to fix a problem that the RAPL PMU
> > > events are added erroneously when higher 32bits contain non-zero value.
> > 
> > Why would these high bits be non-zero?
> 
> On SPR platform, the high bits of Psys energy counter are reused for other purpose.
> High bits for other RAPL domains energy counters still return 0.
> 
> I didn't mention this because I thought this patch should be okay as a generic fix.

But it doesn't fix anything.. there's not anything broken, except on
that daft SPR thing.
Zhang, Rui Jan. 17, 2021, 2:44 p.m. UTC | #4
> -----Original Message-----
> From: Peter Zijlstra <peterz@infradead.org>
> Sent: Saturday, January 16, 2021 8:48 PM
> To: Zhang, Rui <rui.zhang@intel.com>
> Cc: mingo@redhat.com; acme@kernel.org; mark.rutland@arm.com;
> alexander.shishkin@linux.intel.com; jolsa@redhat.com;
> namhyung@kernel.org; linux-kernel@vger.kernel.org; x86@kernel.org;
> kan.liang@linux.intel.com; ak@linux.intel.com
> Subject: Re: [PATCH 2/3] perf/x86/rapl: Fix energy counter detection
> Importance: High
> 
> On Sat, Jan 16, 2021 at 08:19:35AM +0000, Zhang, Rui wrote:
> >
> >
> > > -----Original Message-----
> > > From: Peter Zijlstra <peterz@infradead.org>
> > > Sent: Saturday, January 16, 2021 4:03 AM
> > > To: Zhang, Rui <rui.zhang@intel.com>
> > > Cc: mingo@redhat.com; acme@kernel.org; mark.rutland@arm.com;
> > > alexander.shishkin@linux.intel.com; jolsa@redhat.com;
> > > namhyung@kernel.org; linux-kernel@vger.kernel.org; x86@kernel.org;
> > > kan.liang@linux.intel.com; ak@linux.intel.com
> > > Subject: Re: [PATCH 2/3] perf/x86/rapl: Fix energy counter detection
> > > Importance: High
> > >
> > > On Fri, Jan 15, 2021 at 05:22:07PM +0800, Zhang Rui wrote:
> > > > In the RAPL ENERGY_COUNTER MSR, only the lower 32bits represent
> > > > the energy counter, and the higher 32bits are reserved.
> > > >
> > > > Add the MSR mask for these MSRs to fix a problem that the RAPL PMU
> > > > events are added erroneously when higher 32bits contain non-zero
> value.
> > >
> > > Why would these high bits be non-zero?
> >
> > On SPR platform, the high bits of Psys energy counter are reused for other
> purpose.
> > High bits for other RAPL domains energy counters still return 0.
> >
> > I didn't mention this because I thought this patch should be okay as a
> generic fix.
> 
> But it doesn't fix anything.. there's not anything broken, except on that daft
> SPR thing.

Well, yes.
Before SPR, this is just a potential issue. But things on SPR suggests that this potential issue may become a real one.
So are you suggesting me to also include the SPR information as the justification of this patch?

Thanks,
rui
Peter Zijlstra Feb. 3, 2021, 2:21 p.m. UTC | #5
On Sun, Jan 17, 2021 at 02:44:04PM +0000, Zhang, Rui wrote:

> > But it doesn't fix anything.. there's not anything broken, except on that daft
> > SPR thing.
> 
> Well, yes.
> Before SPR, this is just a potential issue. But things on SPR suggests
> that this potential issue may become a real one.  So are you
> suggesting me to also include the SPR information as the justification
> of this patch?

Yes, and fix the subject. It doesn't fix anything, as there isn't
anything broken.

Just say that upcoming SPR will need this.

Patch
diff mbox series

diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
index 7dbbeaacd995..7ed25b2ba05f 100644
--- a/arch/x86/events/rapl.c
+++ b/arch/x86/events/rapl.c
@@ -523,12 +523,15 @@  static bool test_msr(int idx, void *data)
 	return test_bit(idx, (unsigned long *) data);
 }
 
+/* Only lower 32bits of the MSR represents the energy counter */
+#define RAPL_MSR_MASK 0xFFFFFFFF
+
 static struct perf_msr intel_rapl_msrs[] = {
-	[PERF_RAPL_PP0]  = { MSR_PP0_ENERGY_STATUS,      &rapl_events_cores_group, test_msr },
-	[PERF_RAPL_PKG]  = { MSR_PKG_ENERGY_STATUS,      &rapl_events_pkg_group,   test_msr },
-	[PERF_RAPL_RAM]  = { MSR_DRAM_ENERGY_STATUS,     &rapl_events_ram_group,   test_msr },
-	[PERF_RAPL_PP1]  = { MSR_PP1_ENERGY_STATUS,      &rapl_events_gpu_group,   test_msr },
-	[PERF_RAPL_PSYS] = { MSR_PLATFORM_ENERGY_STATUS, &rapl_events_psys_group,  test_msr },
+	[PERF_RAPL_PP0]  = { MSR_PP0_ENERGY_STATUS,      &rapl_events_cores_group, test_msr, false, RAPL_MSR_MASK },
+	[PERF_RAPL_PKG]  = { MSR_PKG_ENERGY_STATUS,      &rapl_events_pkg_group,   test_msr, false, RAPL_MSR_MASK },
+	[PERF_RAPL_RAM]  = { MSR_DRAM_ENERGY_STATUS,     &rapl_events_ram_group,   test_msr, false, RAPL_MSR_MASK },
+	[PERF_RAPL_PP1]  = { MSR_PP1_ENERGY_STATUS,      &rapl_events_gpu_group,   test_msr, false, RAPL_MSR_MASK },
+	[PERF_RAPL_PSYS] = { MSR_PLATFORM_ENERGY_STATUS, &rapl_events_psys_group,  test_msr, false, RAPL_MSR_MASK },
 };
 
 /*