Re: [PATCH 2/3][v2] tools/power turbostat: Introduce functions to accumulate RAPL consumption

From: Chen Yu
Date: Thu Apr 16 2020 - 13:05:44 EST


Hi Doug,
Thanks for reviewing this patch.
On Wed, Apr 15, 2020 at 09:03:34PM -0700, Doug Smythies wrote:
> On 2020.04.15 05:57 Chen Yu wrote:
>
> ...
>
> > v2: According to Len's suggestion:
> > 1. Enable the accumulated RAPL mechanism by default.
>
> I am not a fan of this, but O.K.
>
> > 2. Re-use the rapl_joule_counter_range to represent the
> > the timeout of periodical timer.
>
> No, please no. It is too easy to still have an overflow.
>
> ...
> > + /*
> > + * A wraparound time is calculated early.
> > + */
> > + its.it_interval.tv_sec = rapl_joule_counter_range;
>
> Would this be o.K.?
>
> + its.it_interval.tv_sec = rapl_joule_counter_range / 2;
>
This should be okay. I've checked the defination of TDP, and
on a wiki page it has mentioned that[1]:
"Some sources state that the peak power for a microprocessor
is usually 1.5 times the TDP rating"
although the defination of TDP varies, using 2 * TDP should
be safe.
> > + its.it_interval.tv_nsec = 0;
>
> The way it was sent, this patch set does not work.
> It still overflows.
>
> Example, sample time calculated to ensure overflow:
>
> Busy% Bzy_MHz IRQ PkgTmp PkgWatt GFXWatt
> 100.00 3500 3592125 80 9.72 0.12
> 100.00 3500 3587391 79 9.77 0.12
>
> Actual package watts was around 65.
>
> However, if this additional patch is applied (I only fixed one of them):
>
> doug@s18:~/temp-k-git/linux/tools/power/x86/turbostat$ git diff
> diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
> index 29fc4069f467..4d72d9be5209 100644
> --- a/tools/power/x86/turbostat/turbostat.c
> +++ b/tools/power/x86/turbostat/turbostat.c
> @@ -1350,7 +1350,8 @@ delta_package(struct pkg_data *new, struct pkg_data *old)
>
> old->gfx_mhz = new->gfx_mhz;
>
> - DELTA_WRAP32(new->energy_pkg, old->energy_pkg);
> +/* DELTA_WRAP32(new->energy_pkg, old->energy_pkg); */
> + old->energy_pkg = new->energy_pkg - old->energy_pkg;
> DELTA_WRAP32(new->energy_cores, old->energy_cores);
> DELTA_WRAP32(new->energy_gfx, old->energy_gfx);
> DELTA_WRAP32(new->energy_dram, old->energy_dram);
>
> Then it seems to work.
>
Nice catch, I did not realize that the energy_pkg field has
already been converted into accumuted variable which does not
need to consider the wrapping(64bit should be long enough for
normal test cases).

Thanks,
Chenyu
> Example:
>
> doug@s15:~/temp-turbostat$ sudo ./turbostat --Summary --show Busy%,Bzy_MHz,PkgTmp,PkgWatt,GFXWatt,IRQ --interval 1200
> ...
> RAPL: 690 sec. Joule Counter Range, at 95 Watts
> ...
> Busy% Bzy_MHz IRQ PkgTmp PkgWatt GFXWatt
> 100.00 3500 3592328 80 64.32 0.12
> 100.00 3500 3595195 79 64.37 0.12
>
> ... Doug
>
>