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
Date: Fri, 17 Apr 2020 01:06:11 +0800 [thread overview]
Message-ID: <20200416170611.GA23628@chenyu-office.sh.intel.com> (raw)
In-Reply-To: <001901d613a4$010e0a70$032a1f50$@net>
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
>
>
next prev parent reply other threads:[~2020-04-16 17:05 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-14 12:56 [PATCH 0/3][RFC v2] tools/power turbostat: Enable accumulated energy consumption for long time sampling Chen Yu
2020-04-14 12:56 ` [PATCH 1/3][RFC v2] tools/power turbostat: Make the energy variable to be 64 bit Chen Yu
2020-04-14 12:56 ` [PATCH 2/3][v2] tools/power turbostat: Introduce functions to accumulate RAPL consumption Chen Yu
2020-04-16 4:03 ` Doug Smythies
2020-04-16 17:06 ` Chen Yu [this message]
2020-04-16 20:40 ` Doug Smythies
2020-04-17 4:23 ` Chen Yu
2020-04-14 12:57 ` [PATCH 3/3][RFC v2] tools/power turbostat: Enable accumulate RAPL display Chen Yu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200416170611.GA23628@chenyu-office.sh.intel.com \
--to=yu.c.chen@intel.com \
--cc=dsmythies@telus.net \
--cc=lenb@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=rjw@rjwysocki.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.