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

Signed-off-by: Doug Smythies <dsmythies@telus.net>
---
 tools/power/x86/turbostat/turbostat.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

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



On Mon, Apr 3, 2023 at 5:11 PM Doug Smythies <dsmythies@telus.net> wrote:
>
>
> 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
>
> Signed-off-by: Doug Smythies <dsmythies@telus.net>
> ---
>  tools/power/x86/turbostat/turbostat.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
> index 8a36ba5df9f9..d8d667934a23 100644
> --- a/tools/power/x86/turbostat/turbostat.c
> +++ b/tools/power/x86/turbostat/turbostat.c
> @@ -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
>
>

On Mon, Nov 27, 2023 at 9:31 PM Len Brown <lenb@kernel.org> wrote:
>
> 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?

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....

Hi Len,

Thank you for having a look at this.

On 2023.11.27 18:44 Len wrote:
> On Mon, Nov 27, 2023 at 9:31 PM Len Brown <lenb@kernel.org> wrote:
>>
>> 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?

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.

>
> 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....

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

... Doug

On Wed, Nov 29, 2023 at 10:58 AM Doug Smythies <dsmythies@telus.net> wrote:

> > "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....
>
> Thanks for that.
> Yes, the format of the information isn't pretty but it is probably
> good enough. I use --add MSRs rarely.

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

> if (topo.num_packages == 1)

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

On 2023.11.29 17:24 Len wrote:

>> if (topo.num_packages == 1)
>
> 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

Hi Len,

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

Thanks.

... Doug