[v3,2/2,WIP] tools/power turbostat: Also read package power on AMD F17h (Zen)
diff mbox series

Message ID 20180817163442.10065-3-calvin.walton@kepstin.ca
State Accepted, archived
Delegated to: Len Brown
Headers show
Series
  • tools/power turbostat: Add support for AMD Fam 17h (Zen) RAPL
Related show

Commit Message

Calvin Walton Aug. 17, 2018, 4:34 p.m. UTC
The package power can also be read from an MSR. It's not clear exactly
what is included, and whether it's aggregated over all nodes or
reported separately.

It does look like this is reported separately per CCX (I get a single
value on the Ryzen R7 1700), but it might be reported separately per-
die (node?) on larger processors. If that's the case, it would have to
be recorded per node and aggregated for the socket.

Note that although Zen has these MSRs reporting power, it looks like
the actual RAPL configuration (power limits, configured TDP) is done
through PCI configuration space. I have not yet found any public
documentation for this.

Signed-off-by: Calvin Walton <calvin.walton@kepstin.ca>
---
 tools/power/x86/turbostat/turbostat.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Comments

Len Brown March 20, 2019, 11:36 p.m. UTC | #1
Hi Calvin,
I'm inclined to apply this -- because it will not break anything,
and it would at least enable testing by people, who have this hardware.

thoughts?
-Len

On Fri, Aug 17, 2018 at 12:35 PM Calvin Walton <calvin.walton@kepstin.ca> wrote:
>
> The package power can also be read from an MSR. It's not clear exactly
> what is included, and whether it's aggregated over all nodes or
> reported separately.
>
> It does look like this is reported separately per CCX (I get a single
> value on the Ryzen R7 1700), but it might be reported separately per-
> die (node?) on larger processors. If that's the case, it would have to
> be recorded per node and aggregated for the socket.
>
> Note that although Zen has these MSRs reporting power, it looks like
> the actual RAPL configuration (power limits, configured TDP) is done
> through PCI configuration space. I have not yet found any public
> documentation for this.
>
> Signed-off-by: Calvin Walton <calvin.walton@kepstin.ca>
> ---
>  tools/power/x86/turbostat/turbostat.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
> index 89d4e2e75774..675c894b8595 100644
> --- a/tools/power/x86/turbostat/turbostat.c
> +++ b/tools/power/x86/turbostat/turbostat.c
> @@ -1973,6 +1973,11 @@ int get_counters(struct thread_data *t, struct core_data *c, struct pkg_data *p)
>                         return -16;
>                 p->rapl_dram_perf_status = msr & 0xFFFFFFFF;
>         }
> +       if (do_rapl & RAPL_AMD_F17H) {
> +               if (get_msr(cpu, MSR_PKG_ENERGY_STAT, &msr))
> +                       return -13;
> +               p->energy_pkg = msr & 0xFFFFFFFF;
> +       }
>         if (DO_BIC(BIC_PkgTmp)) {
>                 if (get_msr(cpu, MSR_IA32_PACKAGE_THERM_STATUS, &msr))
>                         return -17;
> @@ -3986,10 +3991,13 @@ void rapl_probe_amd(unsigned int family, unsigned int model)
>         switch (family) {
>         case 0x17: /* Zen, Zen+ */
>                 do_rapl = RAPL_AMD_F17H | RAPL_PER_CORE_ENERGY;
> -               if (rapl_joules)
> +               if (rapl_joules) {
> +                       BIC_PRESENT(BIC_Pkg_J);
>                         BIC_PRESENT(BIC_Cor_J);
> -               else
> +               } else {
> +                       BIC_PRESENT(BIC_PkgWatt);
>                         BIC_PRESENT(BIC_CorWatt);
> +               }
>                 break;
>         default:
>                 return;
> --
> 2.18.0
>
Calvin Walton March 21, 2019, 1:35 a.m. UTC | #2
(Whoops, resending this message. Forgot that Gmail defaulted to
HTML...)

On Wed, 2019-03-20 at 19:36 -0400, Len Brown wrote:
> Hi Calvin,
> I'm inclined to apply this -- because it will not break anything,
> and it would at least enable testing by people, who have this
> hardware.

Hi Len,

By all means go ahead and apply the package power patch as well. I've
tested it on both Summit (single-die desktop) and Raven (desktop/laptop
apu) with good results. The worst case as it stands is that if the
multi-die packages (EPYC/TR) don't aggregate the value per package,
then package power will be under-reported.

(I previously wrote that this would be easier to test with the patch
applied, because the package power is reported with the -Dump option,
but I'm not sure that's actually the case - the value might still only
be collected once per package?)

Calvin.

> 
> On Fri, Aug 17, 2018 at 12:35 PM Calvin Walton <
> calvin.walton@kepstin.ca> wrote:
> > The package power can also be read from an MSR. It's not clear
> > exactly
> > what is included, and whether it's aggregated over all nodes or
> > reported separately.
> > 
> > It does look like this is reported separately per CCX (I get a
> > single
> > value on the Ryzen R7 1700), but it might be reported separately
> > per-
> > die (node?) on larger processors. If that's the case, it would have
> > to
> > be recorded per node and aggregated for the socket.
> > 
> > Note that although Zen has these MSRs reporting power, it looks
> > like
> > the actual RAPL configuration (power limits, configured TDP) is
> > done
> > through PCI configuration space. I have not yet found any public
> > documentation for this.
> > 
> > Signed-off-by: Calvin Walton <calvin.walton@kepstin.ca>
> > ---
> >  tools/power/x86/turbostat/turbostat.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/power/x86/turbostat/turbostat.c
> > b/tools/power/x86/turbostat/turbostat.c
> > index 89d4e2e75774..675c894b8595 100644
> > --- a/tools/power/x86/turbostat/turbostat.c
> > +++ b/tools/power/x86/turbostat/turbostat.c
> > @@ -1973,6 +1973,11 @@ int get_counters(struct thread_data *t,
> > struct core_data *c, struct pkg_data *p)
> >                         return -16;
> >                 p->rapl_dram_perf_status = msr & 0xFFFFFFFF;
> >         }
> > +       if (do_rapl & RAPL_AMD_F17H) {
> > +               if (get_msr(cpu, MSR_PKG_ENERGY_STAT, &msr))
> > +                       return -13;
> > +               p->energy_pkg = msr & 0xFFFFFFFF;
> > +       }
> >         if (DO_BIC(BIC_PkgTmp)) {
> >                 if (get_msr(cpu, MSR_IA32_PACKAGE_THERM_STATUS,
> > &msr))
> >                         return -17;
> > @@ -3986,10 +3991,13 @@ void rapl_probe_amd(unsigned int family,
> > unsigned int model)
> >         switch (family) {
> >         case 0x17: /* Zen, Zen+ */
> >                 do_rapl = RAPL_AMD_F17H | RAPL_PER_CORE_ENERGY;
> > -               if (rapl_joules)
> > +               if (rapl_joules) {
> > +                       BIC_PRESENT(BIC_Pkg_J);
> >                         BIC_PRESENT(BIC_Cor_J);
> > -               else
> > +               } else {
> > +                       BIC_PRESENT(BIC_PkgWatt);
> >                         BIC_PRESENT(BIC_CorWatt);
> > +               }
> >                 break;
> >         default:
> >                 return;
> > --
> > 2.18.0
> > 
> 
>
Len Brown March 21, 2019, 3:09 a.m. UTC | #3
> (I previously wrote that this would be easier to test with the patch
> applied, because the package power is reported with the -Dump option,
> but I'm not sure that's actually the case - the value might still only
> be collected once per package?)

Right -- we'd have to move it to a per-core or per-CPU data structure
to capture it on a finer granularity than per package.

Of course, if you want to see if you get different values on the cpus,
you can always dump the raw values from each cpu using rdmsr(1).

Unfortunately, I don't have that AMD hardware -- somebody with an
interest in it can test it out.

thanks,
Len Brown, Intel Open Source Technology Center

Patch
diff mbox series

diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
index 89d4e2e75774..675c894b8595 100644
--- a/tools/power/x86/turbostat/turbostat.c
+++ b/tools/power/x86/turbostat/turbostat.c
@@ -1973,6 +1973,11 @@  int get_counters(struct thread_data *t, struct core_data *c, struct pkg_data *p)
 			return -16;
 		p->rapl_dram_perf_status = msr & 0xFFFFFFFF;
 	}
+	if (do_rapl & RAPL_AMD_F17H) {
+		if (get_msr(cpu, MSR_PKG_ENERGY_STAT, &msr))
+			return -13;
+		p->energy_pkg = msr & 0xFFFFFFFF;
+	}
 	if (DO_BIC(BIC_PkgTmp)) {
 		if (get_msr(cpu, MSR_IA32_PACKAGE_THERM_STATUS, &msr))
 			return -17;
@@ -3986,10 +3991,13 @@  void rapl_probe_amd(unsigned int family, unsigned int model)
 	switch (family) {
 	case 0x17: /* Zen, Zen+ */
 		do_rapl = RAPL_AMD_F17H | RAPL_PER_CORE_ENERGY;
-		if (rapl_joules)
+		if (rapl_joules) {
+			BIC_PRESENT(BIC_Pkg_J);
 			BIC_PRESENT(BIC_Cor_J);
-		else
+		} else {
+			BIC_PRESENT(BIC_PkgWatt);
 			BIC_PRESENT(BIC_CorWatt);
+		}
 		break;
 	default:
 		return;