| Message ID | 00d201d96670$e15ab9d0$a4102d70$@telus.net (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Delegated to: | Len Brown |
| Headers | show |
| Series | tools/power/x86/turbostat: Fix added raw MSR output | expand |
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
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; }
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(-)