perf, p4: Add format attributes

PATCH In review Peter Zijlstra 3 participants 10 messages Mar 27, 2012 – Apr 3, 2012

Subsystems (2)

x86 perf events

lore

Share & tools

lore

Peter Zijlstra

author maintainer

2012-03-27 14:50 UTC #0 lore raw

PATCH 1/1 perf, p4: Add format attributes

Subject: perf, p4: Add format attributes
From: Peter Zijlstra <[email protected]>
Date: Tue Mar 27 16:19:25 CEST 2012

Steven reported his P4 not booting properly, the missing format attributes cause a NULL ptr deref. Cure this by adding the missing format specification.

I took the format description out of the comment near p4_config_pack*() and hope that comment is still relatively accurate.

Cc Cyrill Gorcunov <[email protected]>

Cc Lin Ming <[email protected]>

Reported-by Steven Rostedt <[email protected]>

Tested-by Steven Rostedt <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

---
 arch/x86/kernel/cpu/perf_event_p4.c |   13 +++++++++++++
 1 file changed, 13 insertions(+)

arch/x86/kernel/cpu/perf_event_p4.c +13 -0

@@ -1271,6 +1271,17 @@ static int p4_pmu_schedule_events(struct
 	return num ? -EINVAL : 0;
 }
 
+PMU_FORMAT_ATTR(cccr, "config:0-31" );
+PMU_FORMAT_ATTR(escr, "config:32-62");
+PMU_FORMAT_ATTR(ht,   "config:63"   );
+
+static struct attribute *intel_p4_formats_attr[] = {
+	&format_attr_cccr.attr,
+	&format_attr_escr.attr,
+	&format_attr_ht.attr,
+	NULL,
+};
+
 static __initconst const struct x86_pmu p4_pmu = {
 	.name			= "Netburst P4/Xeon",
 	.handle_irq		= p4_pmu_handle_irq,

@@ -1305,6 +1316,8 @@ static __initconst const struct x86_pmu
 	 * the former idea is taken from OProfile code
 	 */
 	.perfctr_second_write	= 1,
+
+	.format_attrs		= intel_p4_formats_attr,
 };
 
 __init int p4_pmu_init(void)

Cyrill Gorcunov

Reviewed-by on v1 1/1

2012-03-27 14:57 UTC #1 lore raw

Quoted 12 lines · Peter Zijlstra

Subject: perf, p4: Add format attributes
From: Peter Zijlstra <[email protected]>
Date: Tue Mar 27 16:19:25 CEST 2012
Steven reported his P4 not booting properly, the missing format attributes cause a NULL ptr deref. Cure this by adding the missing format specification.
I took the format description out of the comment near p4_config_pack*() and hope that comment is still relatively accurate.

Quoted 5 lines

Cc Cyrill Gorcunov <[email protected]>
Cc Lin Ming <[email protected]>
Reported-by Steven Rostedt <[email protected]>
Tested-by Steven Rostedt <[email protected]>
Signed-off-by Peter Zijlstra <[email protected]>

Thanks a lot, Peter! This format description is somewhat new to me (I think I've missed when it was introduced first time).

Reviewed-by Cyrill Gorcunov <[email protected]>

Cyrill

Cyrill Gorcunov

2012-03-27 15:05 UTC #2 lore raw

Quoted 4 lines · Cyrill Gorcunov

Thanks a lot, Peter! This format description is somewhat new to me (I think I've missed when it was introduced first time).

By format description I mean of course PMU_FORMAT_ATTR :)

Cyrill

Peter Zijlstra

author maintainer

2012-03-27 15:06 UTC #3 lore raw

In reply to Cyrill Gorcunov

Quoted 19 lines · Cyrill Gorcunov

Quoted 12 lines · Peter Zijlstra
Subject: perf, p4: Add format attributes
From: Peter Zijlstra <[email protected]>
Date: Tue Mar 27 16:19:25 CEST 2012
Steven reported his P4 not booting properly, the missing format attributes cause a NULL ptr deref. Cure this by adding the missing format specification.
I took the format description out of the comment near p4_config_pack*() and hope that comment is still relatively accurate.
Quoted 5 lines
Cc Cyrill Gorcunov <[email protected]>
Cc Lin Ming <[email protected]>
Reported-by Steven Rostedt <[email protected]>
Tested-by Steven Rostedt <[email protected]>
Signed-off-by Peter Zijlstra <[email protected]>

Quoted 3 lines

Thanks a lot, Peter! This format description is somewhat new to me (I think I've missed when it was introduced first time).

Yeah, its all brand-spanking new stuff.. see
https://lkml.org/lkml/2012/1/16/146 , Jiri added a full fledged bison/yacc parser to perf so we can now write events like:

perf stat -e cpu/event=0xb7,umask=0x01,offcore_rsp=0x500b/

and it uses /sys/bus/event_source/devices/cpu/format/* to figure out how to convert that into perf_event_attr::config{,1,2} values.

So with this patch you could, on your P4, write:

perf stat -e cpu/cccr=0x1234,escr=0x4321,ht/

and it would construct the perf_event_attr::config value 0x92344321 for you.

I've still not actually read the P4-PMU specs, so I don't know if there's anything more convenient we can do on P4. If there's a better/more useful format representation possible, don't hesitate to send a patch! :-)

Quoted 1 line

Reviewed-by Cyrill Gorcunov <[email protected]>

Thanks!

Peter Zijlstra

author maintainer

2012-03-27 15:07 UTC #4 lore raw

Quoted 5 lines · Peter Zijlstra

perf stat -e cpu/cccr=0x1234,escr=0x4321,ht/
and it would construct the perf_event_attr::config value 0x92344321 for
you.

Or rather, 0xc3211234, I got cccr and escr reversed.

Cyrill Gorcunov

2012-03-27 15:12 UTC #5 lore raw

In reply to Peter Zijlstra

Quoted 14 lines · Peter Zijlstra

Quoted 4 lines
Thanks a lot, Peter! This format description is somewhat new to me (I think I've missed when it was introduced first time).
Yeah, its all brand-spanking new stuff.. see
https://lkml.org/lkml/2012/1/16/146 , Jiri added a full fledged bison/yacc parser to perf so we can now write events like:
perf stat -e cpu/event=0xb7,umask=0x01,offcore_rsp=0x500b/
and it uses /sys/bus/event_source/devices/cpu/format/* to figure out how to convert that into perf_event_attr::config{,1,2} values.

Quoted 7 lines

So with this patch you could, on your P4, write:
perf stat -e cpu/cccr=0x1234,escr=0x4321,ht/
and it would construct the perf_event_attr::config value 0x92344321 for you.

Ah, I see thanks for the reference, Peter!

Quoted 4 lines

I've still not actually read the P4-PMU specs, so I don't know if there's anything more convenient we can do on P4. If there's a better/more useful format representation possible, don't hesitate to send a patch! :-)

Sure, I'll try to find some time tonight to read this brand new stuff. And if I find something -- will send out. At moment your patch should be more than enough ;)

Cyrill

Cyrill Gorcunov

2012-03-27 22:03 UTC #6 lore raw

Quoted 11 lines · Cyrill Gorcunov

Ah, I see thanks for the reference, Peter!
Quoted 4 lines
I've still not actually read the P4-PMU specs, so I don't know if there's anything more convenient we can do on P4. If there's a better/more useful format representation possible, don't hesitate to send a patch! :-)
Sure, I'll try to find some time tonight to read this brand new stuff. And if I find something -- will send out. At moment your patch should be more than enough ;)

Hi, Peter, sorry for delay,

after looking some more I think ht bit might be dropped from the format, it's used internally by the kernel in attmept to eliminate redundant msr poking if new event runs on the same thread as previous one, and it's always cleared before real write to msr happens, which means user-space might set or clear it, but it never be considered by the kernel.

(still my earlier reviewed-by is valid because this ht bit
is harmless ;)

Cyrill

Peter Zijlstra

author maintainer

2012-03-27 22:05 UTC #7 lore raw

Quoted 22 lines · Cyrill Gorcunov

Quoted 11 lines · Cyrill Gorcunov
Ah, I see thanks for the reference, Peter!
Quoted 4 lines
I've still not actually read the P4-PMU specs, so I don't know if there's anything more convenient we can do on P4. If there's a better/more useful format representation possible, don't hesitate to send a patch! :-)
Sure, I'll try to find some time tonight to read this brand new stuff. And if I find something -- will send out. At moment your patch should be more than enough ;)
Hi, Peter, sorry for delay,
after looking some more I think ht bit might be dropped from the format, it's used internally by the kernel in attmept to eliminate redundant msr poking if new event runs on the same thread as previous one, and it's always cleared before real write to msr happens, which means user-space might set or clear it, but it never be considered by the kernel.

Quoted 2 lines

(still my earlier reviewed-by is valid because this ht bit
is harmless ;)

So does that make escr:32-62 or should that become 32-63 ?

Cyrill Gorcunov

2012-03-27 22:11 UTC #8 lore raw

Quoted 2 lines · Peter Zijlstra

So does that make escr:32-62 or should that become 32-63 ?

32-63 I think will be better, because it's raw format for escr, simply 63 bit is never used in hw level so we borrowed it for own needs.

Cyrill

tip-bot for Peter Zijlstra

2012-04-03 08:12 UTC #9 lore raw

[tip:perf/urgent] perf/x86/p4: Add format attributes

In reply to Peter Zijlstra

Commit-ID:  7b8e6da46b921d30ac1553cac56d8fb74f0b431d
Gitweb:     http://git.kernel.org/tip/7b8e6da46b921d30ac1553cac56d8fb74f0b431d
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Tue, 27 Mar 2012 16:50:42 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Tue, 3 Apr 2012 08:33:38 +0200

perf/x86/p4: Add format attributes

Steven reported his P4 not booting properly, the missing format attributes cause a NULL ptr deref. Cure this by adding the missing format specification.

I took the format description out of the comment near p4_config_pack*() and hope that comment is still relatively accurate.

Reported-by Steven Rostedt <[email protected]>

Reported-by Bruno Prémont <[email protected]>

Tested-by Steven Rostedt <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

Cc Jiri Olsa <[email protected]>

Cc Cyrill Gorcunov <[email protected]>

Cc Lin Ming <[email protected]>

Cc Stephane Eranian <[email protected]>

Link http://lkml.kernel.org/r/1332859842.16159.227.camel@twins open here

Signed-off-by Ingo Molnar <[email protected]>

---
 arch/x86/kernel/cpu/perf_event_p4.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

arch/x86/kernel/cpu/perf_event_p4.c +13 -0

index ef484d9..a2dfacf 100644
@@ -1271,6 +1271,17 @@ done:
 	return num ? -EINVAL : 0;
 }
 
+PMU_FORMAT_ATTR(cccr, "config:0-31" );
+PMU_FORMAT_ATTR(escr, "config:32-62");
+PMU_FORMAT_ATTR(ht,   "config:63"   );
+
+static struct attribute *intel_p4_formats_attr[] = {
+	&format_attr_cccr.attr,
+	&format_attr_escr.attr,
+	&format_attr_ht.attr,
+	NULL,
+};
+
 static __initconst const struct x86_pmu p4_pmu = {
 	.name			= "Netburst P4/Xeon",
 	.handle_irq		= p4_pmu_handle_irq,

@@ -1305,6 +1316,8 @@ static __initconst const struct x86_pmu p4_pmu = {
 	 * the former idea is taken from OProfile code
 	 */
 	.perfctr_second_write	= 1,
+
+	.format_attrs		= intel_p4_formats_attr,
 };
 
 __init int p4_pmu_init(void)