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

[Chen Yu <yu.c.chen@intel.com>]

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