tools/power/x86/turbostat: Fix added raw MSR output

When using --Summary mode, added MSRs in raw mode always print zeros. Print the actual register contents.

Example, with patch:

note the added column:
--add msr0x64f,u32,package,raw,REASON

Where:

0x64F is MSR_CORE_PERF_LIMIT_REASONS

# ./turbostat --quiet --Summary --show Busy%,Bzy_MHz,PkgWatt,PkgTmp,CorWatt --add msr0x64f,u32,package,raw,REASON --interval 10
Busy%   Bzy_MHz PkgTmp  PkgWatt CorWatt     REASON
0.00    4800    35      1.42    0.76    0x00000000
0.00    4801    34      1.42    0.76    0x00000000
80.08   4531    66      108.17  107.52  0x08000000
98.69   4530    66      133.21  132.54  0x08000000
99.28   4505    66      128.26  127.60  0x0c000400
99.65   4486    68      124.91  124.25  0x0c000400
99.63   4483    68      124.90  124.25  0x0c000400
79.34   4481    41      99.80   99.13   0x0c000000
0.00    4801    41      1.40    0.73    0x0c000000

Where, for the test processor (i5-10600K):

PKG Limit #1: 125.000 Watts, 8.000000 sec
MSR bit 26 = log; bit 10 = status

PKG Limit #2: 136.000 Watts, 0.002441 sec
MSR bit 27 = log; bit 11 = status

Example, without patch:

# ./turbostat --quiet --Summary --show Busy%,Bzy_MHz,PkgWatt,PkgTmp,CorWatt --add msr0x64f,u32,package,raw,REASON --interval 10
Busy%   Bzy_MHz PkgTmp  PkgWatt CorWatt     REASON
0.01    4800    35      1.43    0.77    0x00000000
0.00    4801    35      1.39    0.73    0x00000000
83.49   4531    66      112.71  112.06  0x00000000
98.69   4530    68      133.35  132.69  0x00000000
99.31   4500    67      127.96  127.30  0x00000000
99.63   4483    69      124.91  124.25  0x00000000
99.61   4481    69      124.90  124.25  0x00000000
99.61   4481    71      124.92  124.25  0x00000000
59.35   4479    42      75.03   74.37   0x00000000
0.00    4800    42      1.39    0.73    0x00000000
0.00    4801    42      1.42    0.76    0x00000000

# rdmsr 0x64f
c000000

---
 tools/power/x86/turbostat/turbostat.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

index 8a36ba5df9f9..d8d667934a23 100644
@@ -1744,8 +1744,9 @@ int sum_counters(struct thread_data *t, struct core_data *c, struct pkg_data *p)
 
 	for (i = 0, mp = sys.tp; mp; i++, mp = mp->next) {
 		if (mp->format == FORMAT_RAW)
-			continue;
-		average.threads.counter[i] += t->counter[i];
+			average.threads.counter[i] = t->counter[i];
+		else
+			average.threads.counter[i] += t->counter[i];
 	}
 
 	/* sum per-core values only for 1st thread in core */

@@ -1764,8 +1765,9 @@ int sum_counters(struct thread_data *t, struct core_data *c, struct pkg_data *p)
 
 	for (i = 0, mp = sys.cp; mp; i++, mp = mp->next) {
 		if (mp->format == FORMAT_RAW)
-			continue;
-		average.cores.counter[i] += c->counter[i];
+			average.cores.counter[i] = c->counter[i];
+		else
+			average.cores.counter[i] += c->counter[i];
 	}
 
 	/* sum per-pkg values only for 1st core in pkg */

@@ -1812,8 +1814,9 @@ int sum_counters(struct thread_data *t, struct core_data *c, struct pkg_data *p)
 
 	for (i = 0, mp = sys.pp; mp; i++, mp = mp->next) {
 		if (mp->format == FORMAT_RAW)
-			continue;
-		average.packages.counter[i] += p->counter[i];
+			average.packages.counter[i] = p->counter[i];
+		else
+			average.packages.counter[i] += p->counter[i];
 	}
 	return 0;
 }

2.25.1

Hi Doug,

I recall puzzling what to do for the system summary row for a RAW added MSR. In your example 1-package system with a package-scope MSR your patch does the trick.

But if there are N packages, the summary will ignore all but the last one, which it prints.

Similarly, if the MSR is core or CPU scope, the system summary ignores all but the last one, which is prints.

So I concluded that printing nothing, er, zero that is, on the system summary row for a RAW MSR was the least likely to confuse somebody.

I'm thinking that the first two hunks of your patch for thread and core don't make sense because of this.
I'm thinking that the last hunk, for package, makes sense, but only on a single package system?

thanks,
-Len

Len Brown, Intel

Oh, one more bit...
"turbostat --cpu cpu-set" can accept "package" as the cpu-set. This will print out one row per package, and will give you the information that you really want -- even on a multi-package system. However, it isn't as pretty as the system summary that you printed, because it pre-prints the headers (and the system summary) every interval.

It may make more sense to tweak (or simply filter) the output of "--cpu package" to produce the output you are looking for....

Len Brown, Intel

Hi Len,

Thank you for having a look at this.

I played around with it, and simply can not recall why I did the first 2 hunks. The 3rd hunk seems good enough, but I have yet to figure out how to determine if it is a one package system.

Thanks for that.
Yes, the format of the information isn't pretty but it is probably good enough. I use --add MSRs rarely.

... Doug

The format for "--cpu package" comes with a caveat too. The system summary is generated from the info from all the CPUs sampled, eg. sum or average, depending on the metric.

But the per-package row is *not* a combination of all the CPUs in the package, it is the values for just the CPU that has been anointed as 1st in that package.

This is fine for your per-package MSR output, but the other metrics, say %Busy, may be quite different on that first CPU in the package vs the average of the whole package.

For this reason, an accurate picture really does need the system summary and the "first CPU in package" per-package row.

Fundamentally, we can't put both per-system and per-CPU info onto the same row except under limited conditions.
Your example of a package-scope raw MSR on a single package system seems to be such a valid condition.

Maybe in your 3rd hunk simply add...
if (topo.num_packages == 1)

thanks,
Len Brown, Intel

turns out that is topo.num_packages == 0...

Doug,
Let me know if this tweak to your patch doesn't do the trick.

thanks,
-Len

From 4db629665d8524f705b86c8529c69f7589b39c7b Mon Sep 17 00:00:00 2001
Message-Id: <4db629665d8524f705b86c8529c69f7589b39c7b.1701307225.git.len.brown@intel.com>
In-Reply-To: <7f93100257ed9f426c5c3656686a5f57d7d9800e.1701307225.git.len.brown@intel.com>
References: <7f93100257ed9f426c5c3656686a5f57d7d9800e.1701307225.git.len.brown@intel.com>
From: Doug Smythies <[email protected]>
Date: Mon, 3 Apr 2023 14:11:38 -0700
Subject: [PATCH 4/4] tools/power turbostat: Fix added raw MSR output
Reply-To: Len Brown <[email protected]>
Organization: Intel Open Source Technology Center

When using --Summary mode, added MSRs in raw mode always print zeros. Print the actual register contents.

Example, with patch:

note the added column:
--add msr0x64f,u32,package,raw,REASON

Where:

0x64F is MSR_CORE_PERF_LIMIT_REASONS

Busy%   Bzy_MHz PkgTmp  PkgWatt CorWatt     REASON
0.00    4800    35      1.42    0.76    0x00000000
0.00    4801    34      1.42    0.76    0x00000000
80.08   4531    66      108.17  107.52  0x08000000
98.69   4530    66      133.21  132.54  0x08000000
99.28   4505    66      128.26  127.60  0x0c000400
99.65   4486    68      124.91  124.25  0x0c000400
99.63   4483    68      124.90  124.25  0x0c000400
79.34   4481    41      99.80   99.13   0x0c000000
0.00    4801    41      1.40    0.73    0x0c000000

Where, for the test processor (i5-10600K):

PKG Limit #1: 125.000 Watts, 8.000000 sec
MSR bit 26 = log; bit 10 = status

PKG Limit #2: 136.000 Watts, 0.002441 sec
MSR bit 27 = log; bit 11 = status

Example, without patch:

Busy%   Bzy_MHz PkgTmp  PkgWatt CorWatt     REASON
0.01    4800    35      1.43    0.77    0x00000000
0.00    4801    35      1.39    0.73    0x00000000
83.49   4531    66      112.71  112.06  0x00000000
98.69   4530    68      133.35  132.69  0x00000000
99.31   4500    67      127.96  127.30  0x00000000
99.63   4483    69      124.91  124.25  0x00000000
99.61   4481    69      124.90  124.25  0x00000000
99.61   4481    71      124.92  124.25  0x00000000
59.35   4479    42      75.03   74.37   0x00000000
0.00    4800    42      1.39    0.73    0x00000000
0.00    4801    42      1.42    0.76    0x00000000

c000000

---
 tools/power/x86/turbostat/turbostat.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

index c5bd9de25ac7..ed43872afcc2 100644
@@ -2454,9 +2454,10 @@ int sum_counters(struct thread_data *t, struct core_data *c, struct pkg_data *p)
 	average.packages.rapl_dram_perf_status += p->rapl_dram_perf_status;
 
 	for (i = 0, mp = sys.pp; mp; i++, mp = mp->next) {
-		if (mp->format == FORMAT_RAW)
-			continue;
-		average.packages.counter[i] += p->counter[i];
+		if ((mp->format == FORMAT_RAW) && (topo.num_packages == 0))
+			average.packages.counter[i] = p->counter[i];
+		else
+			average.packages.counter[i] += p->counter[i];
 	}
 	return 0;
 }

2.39.2

Hi Len,

It works great, and is a much better solution, for all the reasons you listed in your previous email.

Thanks.

... Doug