Messages in this thread | | | Date | Fri, 17 Apr 2020 01:06:11 +0800 | From | Chen Yu <> | Subject | Re: [PATCH 2/3][v2] tools/power turbostat: Introduce functions to accumulate RAPL consumption |
| |
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 > >
| |