Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: ZtviEUcwAw04AQ6vGY2G6qGqhVhluMbOfUY9MOTyDKQ/HBJlrfEQCTppp6icNF6lCYwwlEBLwe
 lgH+ydAGIQXQ==
IronPort-SDR: r2p37llV8oTWEX2XJNI4HUD191O2WCdTioOapylF9eDQt2rnWXEtqiqUUUL7PgwyfxBRBlAi4M
 pxaUQq64DoUw==
Date:   Fri, 17 Apr 2020 01:06:11 +0800
From:   Chen Yu <yu.c.chen@intel.com>
To:     Doug Smythies <dsmythies@telus.net>
Cc:     'Len Brown' <lenb@kernel.org>,
        "'Rafael J. Wysocki'" <rjw@rjwysocki.net>,
        linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org
Subject: Re: [PATCH 2/3][v2] tools/power turbostat: Introduce functions to
 accumulate RAPL consumption
Message-ID: <20200416170611.GA23628@chenyu-office.sh.intel.com>
References: <cover.1586782089.git.yu.c.chen@intel.com>
 <db96fd31afd0ff65e4041665293b96c984e675bc.1586782089.git.yu.c.chen@intel.com>
 <001901d613a4$010e0a70$032a1f50$@net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <001901d613a4$010e0a70$032a1f50$@net>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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