[patch] events/amd/power add support for fam16h model30h

[patch] events/amd/power add support for fam16h model30h

[Vince Weaver]

According to the BKDG the AMD Family16h Model30h "Jaguar Mullins"
also supports the accumulated power interface.  I've tested on
hardware I have and with this patch I indeed get power readings using 
perf.

Signed-off-by: Vince Weaver <vincent.weaver@maine.edu>

diff --git a/arch/x86/events/amd/power.c b/arch/x86/events/amd/power.c
index 55a3529..778b77d 100644
--- a/arch/x86/events/amd/power.c
+++ b/arch/x86/events/amd/power.c
@@ -292,6 +292,7 @@ static struct notifier_block power_cpu_notifier_nb = {
 
 static const struct x86_cpu_id cpu_match[] = {
 	{ .vendor = X86_VENDOR_AMD, .family = 0x15 },
+	{ .vendor = X86_VENDOR_AMD, .family = 0x16 },
 	{},
 };
 

Re: [patch] events/amd/power add support for fam16h model30h

[Borislav Petkov]

On Thu, Jun 16, 2016 at 05:00:04PM -0400, Vince Weaver wrote:
> 
> According to the BKDG the AMD Family16h Model30h "Jaguar Mullins"
> also supports the accumulated power interface.  I've tested on
> hardware I have and with this patch I indeed get power readings using 
> perf.
> 
> Signed-off-by: Vince Weaver <vincent.weaver@maine.edu>
> 
> diff --git a/arch/x86/events/amd/power.c b/arch/x86/events/amd/power.c
> index 55a3529..778b77d 100644
> --- a/arch/x86/events/amd/power.c
> +++ b/arch/x86/events/amd/power.c
> @@ -292,6 +292,7 @@ static struct notifier_block power_cpu_notifier_nb = {
>  
>  static const struct x86_cpu_id cpu_match[] = {
>  	{ .vendor = X86_VENDOR_AMD, .family = 0x15 },
> +	{ .vendor = X86_VENDOR_AMD, .family = 0x16 },
>  	{},

Actually, I think we remove that table completely and rely solely on the
CPUID bit:

        if (!boot_cpu_has(X86_FEATURE_ACC_POWER))
                return -ENODEV;

Rui?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [patch] events/amd/power add support for fam16h model30h

[Huang Rui]

On Thu, Jun 16, 2016 at 11:12:18PM +0200, Borislav Petkov wrote:
> On Thu, Jun 16, 2016 at 05:00:04PM -0400, Vince Weaver wrote:
> > 
> > According to the BKDG the AMD Family16h Model30h "Jaguar Mullins"
> > also supports the accumulated power interface.  I've tested on
> > hardware I have and with this patch I indeed get power readings using 
> > perf.
> > 
> > Signed-off-by: Vince Weaver <vincent.weaver@maine.edu>
> > 
> > diff --git a/arch/x86/events/amd/power.c b/arch/x86/events/amd/power.c
> > index 55a3529..778b77d 100644
> > --- a/arch/x86/events/amd/power.c
> > +++ b/arch/x86/events/amd/power.c
> > @@ -292,6 +292,7 @@ static struct notifier_block power_cpu_notifier_nb = {
> >  
> >  static const struct x86_cpu_id cpu_match[] = {
> >  	{ .vendor = X86_VENDOR_AMD, .family = 0x15 },
> > +	{ .vendor = X86_VENDOR_AMD, .family = 0x16 },
> >  	{},
> 
> Actually, I think we remove that table completely and rely solely on the
> CPUID bit:
> 
>         if (!boot_cpu_has(X86_FEATURE_ACC_POWER))
>                 return -ENODEV;
> 
> Rui?
> 

Agree with you. If the some chips are not stable, we can add a check
to ignore them with family and model id.

Thanks,
Rui

Re: [patch] events/amd/power add support for fam16h model30h

[Borislav Petkov]

On Fri, Jun 17, 2016 at 06:07:47PM +0800, Huang Rui wrote:
> Agree with you. If the some chips are not stable, we can add a check
> to ignore them with family and model id.

So if family 0x16 is not "stable" as you say, we probably should keep
the cpu_match array too.

Actually, you could merge the feature check in there too, AFAICT, from
looking at x86_match_cpu() and if I'm not misreading it:

static const struct x86_cpu_id cpu_match[] = {
	{ .vendor = X86_VENDOR_AMD, .family = 0x15, .model = X86_MODEL_ANY, .feature = X86_FEATURE_ACC_POWER },
};

And then you can drop the boot_cpu_has() test as x86_match_cpu() does it
for you.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [patch] events/amd/power add support for fam16h model30h

[Vince Weaver]

On Fri, 17 Jun 2016, Borislav Petkov wrote:

> On Fri, Jun 17, 2016 at 06:07:47PM +0800, Huang Rui wrote:
> > Agree with you. If the some chips are not stable, we can add a check
> > to ignore them with family and model id.
> 
> So if family 0x16 is not "stable" as you say, we probably should keep
> the cpu_match array too.

Has anyone actually verified that the fam15h excavator machines are 
producing "stable" results?

I know on my fam15h piledriver machines the results returned by the 
drivers/hwmon/fam15h_power.c driver look questionable at best, which is 
what started my trying to figure this whole thing out in the first place.

Vince

Re: [patch] events/amd/power add support for fam16h model30h

[Borislav Petkov]

On Fri, Jun 17, 2016 at 11:56:31AM -0400, Vince Weaver wrote:
> Has anyone actually verified that the fam15h excavator machines are
> producing "stable" results?

Not that I know of. Rui, any ideas?

> I know on my fam15h piledriver machines the results returned by the
> drivers/hwmon/fam15h_power.c driver look questionable at best, which
> is what started my trying to figure this whole thing out in the first
> place.

Piledriver? What f/m/s is that?

Because I have a PD here - AMD FX(tm)-8350 Eight-Core Processor -
F15hM02 and sensors gives only this below. But my PD doesn't have
X86_FEATURE_ACC_POWER.

$ sensors
k10temp-pci-00c3
Adapter: PCI adapter
temp1:        +18.4°C  (high = +70.0°C)
                       (crit = +90.0°C, hyst = +87.0°C)

fam15h_power-pci-00c4
Adapter: PCI adapter
power1:       19.38 W  (crit = 125.19 W)

radeon-pci-0100
Adapter: PCI adapter
temp1:        +85.0°C
---

Do you mean this PCI adapter thing?

I can get it right up to crit when building a kernel:

power1:      124.76 W  (crit = 125.19 W)

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [patch] events/amd/power add support for fam16h model30h

[Vince Weaver]

On Fri, 17 Jun 2016, Borislav Petkov wrote:

> Piledriver? What f/m/s is that?
> 
> Because I have a PD here - AMD FX(tm)-8350 Eight-Core Processor -
> F15hM02 and sensors gives only this below. But my PD doesn't have
> X86_FEATURE_ACC_POWER.

F15hM02h

fam15h_power-pci-00c4
Adapter: PCI adapter
power1:      105.64 W  (crit = 115.17 W)

fam15h_power-pci-00d4
Adapter: PCI adapter
power1:      105.59 W  (crit = 115.17 W)

This is at idle.  The whole system only uses 167W total at idle 
(according to a wall-outlet power meter), so the two packages each drawing 
105W seems a bit off.


I also have a F15hM13h system but it doesn't seem to support TDP 
measurement.

Vince

Re: [patch] events/amd/power add support for fam16h model30h

[Borislav Petkov]

On Fri, Jun 17, 2016 at 01:38:44PM -0400, Vince Weaver wrote:
> F15hM02h
> 
> fam15h_power-pci-00c4
> Adapter: PCI adapter
> power1:      105.64 W  (crit = 115.17 W)
> 
> fam15h_power-pci-00d4
> Adapter: PCI adapter
> power1:      105.59 W  (crit = 115.17 W)

Is that a dual-socket machine?

Because I have only the fam15h_power-pci-00c4 adapter. And mine goes
between 20W and 40W in idle.

> The whole system only uses 167W total at idle (according to a
> wall-outlet power meter), so the two packages each drawing 105W seems
> a bit off.

Or is that an MCM box? I.e., a Multi-node CPU? Because:

Documentation/hwmon/fam15h_power:
"On multi-node processors the calculated value is for the entire package
and not for a single node. Thus the driver creates sysfs attributes only
for internal node0 of a multi-node processor."

And I remember Andreas did that should_load_on_this_node() thing.

If so, then 105W vs 167W should fit.

> I also have a F15hM13h system but it doesn't seem to support TDP
> measurement.

That's Trinity. That PCI device ID is not even in the kernel - I'm
guessing no one has even thought of enabling fam15_power on it. I don't
know even whether it supports the different PCI interfaces like TDP, TDP
limit, etc.

IOW, something like that might not really help.

Rui, you could take a look if you feel bored and if you have a TN system
somewhere lying around.

---
diff --git a/drivers/hwmon/fam15h_power.c b/drivers/hwmon/fam15h_power.c
index eb97a9241d17..b31ef2779aac 100644
--- a/drivers/hwmon/fam15h_power.c
+++ b/drivers/hwmon/fam15h_power.c
@@ -503,6 +503,7 @@ static int fam15h_power_probe(struct pci_dev *pdev,
 
 static const struct pci_device_id fam15h_power_id_table[] = {
 	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_15H_NB_F4) },
+	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_15H_M10H_F4) },
 	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_15H_M30H_NB_F4) },
 	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_15H_M60H_NB_F4) },
 	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_15H_M70H_NB_F4) },
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index c58752fe16c4..f7385ed3d5f1 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -521,6 +521,7 @@
 #define PCI_DEVICE_ID_AMD_11H_NB_MISC	0x1303
 #define PCI_DEVICE_ID_AMD_11H_NB_LINK	0x1304
 #define PCI_DEVICE_ID_AMD_15H_M10H_F3	0x1403
+#define PCI_DEVICE_ID_AMD_15H_M10H_F4	0x1404
 #define PCI_DEVICE_ID_AMD_15H_M30H_NB_F3 0x141d
 #define PCI_DEVICE_ID_AMD_15H_M30H_NB_F4 0x141e
 #define PCI_DEVICE_ID_AMD_15H_M60H_NB_F3 0x1573


-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [patch] events/amd/power add support for fam16h model30h

[Vince Weaver]

On Fri, 17 Jun 2016, Borislav Petkov wrote:

> On Fri, Jun 17, 2016 at 01:38:44PM -0400, Vince Weaver wrote:
> > F15hM02h
> > 
> > fam15h_power-pci-00c4
> > Adapter: PCI adapter
> > power1:      105.64 W  (crit = 115.17 W)
> > 
> > fam15h_power-pci-00d4
> > Adapter: PCI adapter
> > power1:      105.59 W  (crit = 115.17 W)
> 
> Is that a dual-socket machine?
> 
> Because I have only the fam15h_power-pci-00c4 adapter. And mine goes
> between 20W and 40W in idle.

It's a dual-socket server machine.
AMD Opteron(tm) Processor 6376
2 packages, 16 cores each.


> That's Trinity. That PCI device ID is not even in the kernel - I'm
> guessing no one has even thought of enabling fam15_power on it. I don't
> know even whether it supports the different PCI interfaces like TDP, TDP
> limit, etc.
> 
> IOW, something like that might not really help.

I'll try applying the patch.  My code that probes the msrs/pci directly 
from userspace seems to think it has support.

Vince

Re: [patch] events/amd/power add support for fam16h model30h

[Vince Weaver]

[-- Attachment #1: Type: text/plain, Size: 587 bytes --]

On Fri, 17 Jun 2016, Vince Weaver wrote:

> I'll try applying the patch.  My code that probes the msrs/pci directly 
> from userspace seems to think it has support.

FWIW on the trinity machine (F15hM13h) with your patch applied I get the 
following:

vince@a10:~$ sensors
k10temp-pci-00c3
Adapter: PCI adapter
temp1:         +0.0°C  (high = +70.0°C)
                       (crit = +70.0°C, hyst = +69.0°C)

fam15h_power-pci-00c4
Adapter: PCI adapter
power1:           N/A  (crit = 100.04 W)

radeon-pci-0008
Adapter: PCI adapter
temp1:         -3.0°C  (crit = +120.0°C, hyst = +90.0°C)

Re: [patch] events/amd/power add support for fam16h model30h

[Borislav Petkov]

On Fri, Jun 17, 2016 at 02:58:41PM -0400, Vince Weaver wrote:
> It's a dual-socket server machine.
> AMD Opteron(tm) Processor 6376
> 2 packages, 16 cores each.

Right, so this is 2 MCMs. You have 2 physical nodes and each physical is
separated into 2 logical nodes additionally.

So provided the wall-outlet power meter is correct (I have one here
which is not really reliable but it was dirt cheap so I shouldn't
wonder) then those numbers look funny, yap.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [patch] events/amd/power add support for fam16h model30h

[Borislav Petkov]

On Fri, Jun 17, 2016 at 03:41:38PM -0400, Vince Weaver wrote:
> FWIW on the trinity machine (F15hM13h) with your patch applied I get the 
> following:
> 
> vince@a10:~$ sensors
> k10temp-pci-00c3
> Adapter: PCI adapter
> temp1:         +0.0°C  (high = +70.0°C)
>                        (crit = +70.0°C, hyst = +69.0°C)
> 
> fam15h_power-pci-00c4
> Adapter: PCI adapter
> power1:           N/A  (crit = 100.04 W)

Yeah, I didn't expect to be that easy.

Provided hw support on TN is present, someone needs to sit down and
audit the algorithm in show_power() and sanity-check all those values
coming from the PCI regs.

But I'd wait for Rui first to confirm whether it is even worth to chase
this or whether TN is a lost cause because this TDP mechanism is not
supported there or whatever other reason there might be...

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.