diff mbox series

[v4] tools/power turbostat: Fix RAPL summary collection on AMD processors

Message ID 20210330213825.77294-1-terry.bowman@amd.com
State Superseded
Headers show
Series [v4] tools/power turbostat: Fix RAPL summary collection on AMD processors | expand

Commit Message

Terry Bowman March 30, 2021, 9:38 p.m. UTC
Turbostat fails to correctly collect and display RAPL summary information
on Family 17h and 19h AMD processors. Running turbostat on these processors
returns immediately. If turbostat is working correctly then RAPL summary
data is displayed until the user provided command completes. If a command
is not provided by the user then turbostat is designed to continuously
display RAPL information until interrupted.

The issue is due to offset_to_idx() and idx_to_offset() missing support for
AMD MSR addresses/offsets. offset_to_idx()'s switch statement is missing
cases for AMD MSRs and idx_to_offset() does not include a path to return
AMD MSR(s) for any idx.

The solution is add AMD MSR support to offset_to_idx() and idx_to_offset().
These functions are split-out and renamed along architecture vendor lines
for supporting both AMD and Intel MSRs.

Fixes: 9972d5d84d76 ("tools/power turbostat: Enable accumulate RAPL display")
Signed-off-by: Terry Bowman <terry.bowman@amd.com>
Cc: Len Brown <lenb@kernel.org>
Cc: linux-pm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 tools/power/x86/turbostat/turbostat.c | 61 ++++++++++++++++++++++++---
 1 file changed, 56 insertions(+), 5 deletions(-)

Comments

Calvin Walton April 15, 2021, 2:13 a.m. UTC | #1
On Tue, 2021-03-30 at 21:38 +0000, Terry Bowman wrote:
> 

> +int idx_valid_amd(int idx)

> +{

> +       switch (idx) {

> +       case IDX_PKG_ENERGY:

> +               return do_rapl & MSR_PKG_ENERGY_STAT;


This isn't correct - MSR_PKG_ENERGY_STAT is the MSR offset, not a bit
mask for the do_rapl bit field.

The presence of MSR_PKG_ENERGY_STAT (along with MSR_RAPL_PWR_UNIT and
MSR_CORE_ENERGY_STAT) is indicated by the RAPL_AMD_F17H bit in do_rapl,
and can be checked with:
	do_rapl & RAPL_AMD_F17H


-- 
Calvin Walton <calvin.walton@kepstin.ca>
Terry Bowman April 16, 2021, 11:42 a.m. UTC | #2
Hi Calvin,

Thanks for the feedback. I'll begin making the change and testing. I'll 
respond with V2 patch in this thread.

Regards,
Terry


On 4/14/21 9:13 PM, Calvin Walton wrote:
> On Tue, 2021-03-30 at 21:38 +0000, Terry Bowman wrote:

>> +int idx_valid_amd(int idx)

>> +{

>> +       switch (idx) {

>> +       case IDX_PKG_ENERGY:

>> +               return do_rapl & MSR_PKG_ENERGY_STAT;

> This isn't correct - MSR_PKG_ENERGY_STAT is the MSR offset, not a bit

> mask for the do_rapl bit field.

>

> The presence of MSR_PKG_ENERGY_STAT (along with MSR_RAPL_PWR_UNIT and

> MSR_CORE_ENERGY_STAT) is indicated by the RAPL_AMD_F17H bit in do_rapl,

> and can be checked with:

> 	do_rapl & RAPL_AMD_F17H

>

>
Calvin Walton April 16, 2021, 2:50 p.m. UTC | #3
On Fri, 2021-04-16 at 06:42 -0500, Terry Bowman wrote:
> 

> Hi Calvin,

> 

> Thanks for the feedback. I'll begin making the change and testing.

> I'll 

> respond with V2 patch in this thread.

> 

> Regards,

> Terry


It looks like there might already be a patch in the pipeline for this
issue; see Chen Yu's response to my patch here:
https://lkml.org/lkml/2021/4/14/1322
I'm hoping we get some clarification of the status soon.

While you're respinning your patch, there's one other issue that I
noticed - all the idx/offset-related functions pass the offset value in
a variable of type "int" (32bit signed integer), but the offset of the
AMD MSR_CORE_ENERGY_STAT MSR is 0xC001029A, which exceeds INT_MAX. The
offsets should all use "off_t" to get a 64bit type and avoid wrapping
or sign extension issues.

-- 
Calvin Walton <calvin.walton@kepstin.ca>
diff mbox series

Patch

diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
index a7c4f0772e53..24c7f380485f 100644
--- a/tools/power/x86/turbostat/turbostat.c
+++ b/tools/power/x86/turbostat/turbostat.c
@@ -291,7 +291,7 @@  struct msr_sum_array {
 /* The percpu MSR sum array.*/
 struct msr_sum_array *per_cpu_msr_sum;
 
-int idx_to_offset(int idx)
+int idx_to_offset_intel(int idx)
 {
 	int offset;
 
@@ -320,7 +320,7 @@  int idx_to_offset(int idx)
 	return offset;
 }
 
-int offset_to_idx(int offset)
+int offset_to_idx_intel(int offset)
 {
 	int idx;
 
@@ -349,7 +349,7 @@  int offset_to_idx(int offset)
 	return idx;
 }
 
-int idx_valid(int idx)
+int idx_valid_intel(int idx)
 {
 	switch (idx) {
 	case IDX_PKG_ENERGY:
@@ -368,6 +368,51 @@  int idx_valid(int idx)
 		return 0;
 	}
 }
+
+int (*idx_to_offset)(int idx) = idx_to_offset_intel;
+int (*offset_to_idx)(int offset) = offset_to_idx_intel;
+int (*idx_valid)(int idx) = idx_valid_intel;
+
+int idx_to_offset_amd(int idx)
+{
+	int offset;
+
+	switch (idx) {
+	case IDX_PKG_ENERGY:
+		offset = MSR_PKG_ENERGY_STAT;
+		break;
+	default:
+		offset = -1;
+	}
+
+	return offset;
+}
+
+int offset_to_idx_amd(int offset)
+{
+	int idx;
+
+	switch (offset) {
+	case MSR_PKG_ENERGY_STAT:
+		idx = IDX_PKG_ENERGY;
+		break;
+	default:
+		idx = -1;
+	}
+
+	return idx;
+}
+
+int idx_valid_amd(int idx)
+{
+	switch (idx) {
+	case IDX_PKG_ENERGY:
+		return do_rapl & MSR_PKG_ENERGY_STAT;
+	default:
+		return 0;
+	}
+}
+
 struct sys_counters {
 	unsigned int added_thread_counters;
 	unsigned int added_core_counters;
@@ -3249,7 +3294,7 @@  int get_msr_sum(int cpu, off_t offset, unsigned long long *msr)
 		return 1;
 
 	idx = offset_to_idx(offset);
-	if (idx < 0)
+	if (idx == -1)
 		return idx;
 	/* get_msr_sum() = sum + (get_msr() - last) */
 	ret = get_msr(cpu, offset, &msr_cur);
@@ -3277,7 +3322,7 @@  static int update_msr_sum(struct thread_data *t, struct core_data *c, struct pkg
 		if (!idx_valid(i))
 			continue;
 		offset = idx_to_offset(i);
-		if (offset < 0)
+		if (offset == -1)
 			continue;
 		ret = get_msr(cpu, offset, &msr_cur);
 		if (ret) {
@@ -5348,6 +5393,12 @@  void process_cpuid()
 	if (!quiet)
 		decode_misc_feature_control();
 
+	if (authentic_amd || hygon_genuine) {
+		idx_to_offset = idx_to_offset_amd;
+		offset_to_idx = offset_to_idx_amd;
+		idx_valid = idx_valid_amd;
+	}
+
 	return;
 }