perf, x86: Add hw_watchdog_set_attr() in a sake of nmi-watchdog on P4

PATCH In review Cyrill Gorcunov 4 participants 19 messages Jun 23, 2011 – Jul 1, 2011

Subsystems (2)

x86 perf events

lore

Share & tools

lore

Cyrill Gorcunov

author

2011-06-23 12:49 UTC #0 lore raw

PATCH 1/1 perf, x86: Add hw_watchdog_set_attr() in a sake of nmi-watchdog on P4

Due to restriction and specifics of Netburst PMU we need a separated event for NMI watchdog. In particular every Netburst event consume not just a counter and config register, but also an additional ESCR register. Since ESCR registers are grouped upon counters (i.e. if ESCR is occupied for some event there is no room for another event to enter until it's released) we need to pick up "least" used ESCR (or most available) for nmi-watchdog purpose -- MSR_P4_CRU_ESCR2/3 was chosen.

With this patch nmi-watchdog and perf top should be able to run simultaneously.

v2: Add a comment about non-sleeping clockticks spotted by Ingo Molnar.
v3: Peter Zijlstra and Stephane Eranian pointed out that making new
event global visible (up to userspace) will bring problems supporting this ABI in future. So now this event is x86 specific and hidden from userspace.
v4: Stephane proposed to use __weak arch specific callback instead of
new hidden generic event.

Signed-off-by Cyrill Gorcunov <[email protected]>

CC Don Zickus <[email protected]>

CC Ingo Molnar <[email protected]>

CC Lin Ming <[email protected]>

CC Peter Zijlstra <[email protected]>

CC Arnaldo Carvalho de Melo <[email protected]>

CC Frederic Weisbecker <[email protected]>

CC Stephane Eranian <[email protected]>

---

Note, I dont have access to P4 machine at moment so help in testing would be highly appreciated.

 arch/x86/kernel/cpu/perf_event.c    |    7 +++++++
 arch/x86/kernel/cpu/perf_event_p4.c |   26 ++++++++++++++++++++++++++
 kernel/watchdog.c                   |    6 +++++-
 3 files changed, 38 insertions(+), 1 deletion(-)

arch/x86/kernel/cpu/perf_event.c +7 -0

Index: linux-2.6.git/arch/x86/kernel/cpu/perf_event.c
===================================================================
@@ -233,6 +233,7 @@ struct x86_pmu {
 	void		(*enable_all)(int added);
 	void		(*enable)(struct perf_event *);
 	void		(*disable)(struct perf_event *);
+	void		(*hw_watchdog_set_attr)(struct perf_event_attr *attr);
 	int		(*hw_config)(struct perf_event *event);
 	int		(*schedule_events)(struct cpu_hw_events *cpuc, int n, int *assign);
 	unsigned	eventsel;

@@ -315,6 +316,12 @@ static u64 __read_mostly hw_cache_extra_
 				[PERF_COUNT_HW_CACHE_OP_MAX]
 				[PERF_COUNT_HW_CACHE_RESULT_MAX];
 
+void hw_nmi_watchdog_set_attr(struct perf_event_attr *wd_attr)
+{
+	if (x86_pmu.hw_watchdog_set_attr)
+		x86_pmu.hw_watchdog_set_attr(wd_attr);
+}
+
 /*
  * Propagate event elapsed time into the generic event.
  * Can only be executed on the CPU where the event is active.

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

Index: linux-2.6.git/arch/x86/kernel/cpu/perf_event_p4.c
===================================================================
@@ -705,6 +705,31 @@ static int p4_validate_raw_event(struct 
 	return 0;
 }
 
+static void p4_hw_watchdog_set_attr(struct perf_event_attr *wd_attr)
+{
+	/*
+	 * Watchdog ticks are special on Netburst, we use
+	 * that named "non-sleeping" ticks as recommended
+	 * by Intel SDM Vol3b.
+	 */
+	WARN_ON_ONCE(wd_attr->type	!= PERF_TYPE_HARDWARE ||
+		     wd_attr->config	!= PERF_COUNT_HW_CPU_CYCLES);
+
+	wd_attr->type	= PERF_TYPE_RAW;
+	wd_attr->config	=
+		p4_config_pack_escr(P4_ESCR_EVENT(P4_EVENT_EXECUTION_EVENT)		|
+			P4_ESCR_EMASK_BIT(P4_EVENT_EXECUTION_EVENT, NBOGUS0)		|
+			P4_ESCR_EMASK_BIT(P4_EVENT_EXECUTION_EVENT, NBOGUS1)		|
+			P4_ESCR_EMASK_BIT(P4_EVENT_EXECUTION_EVENT, NBOGUS2)		|
+			P4_ESCR_EMASK_BIT(P4_EVENT_EXECUTION_EVENT, NBOGUS3)		|
+			P4_ESCR_EMASK_BIT(P4_EVENT_EXECUTION_EVENT, BOGUS0)		|
+			P4_ESCR_EMASK_BIT(P4_EVENT_EXECUTION_EVENT, BOGUS1)		|
+			P4_ESCR_EMASK_BIT(P4_EVENT_EXECUTION_EVENT, BOGUS2)		|
+			P4_ESCR_EMASK_BIT(P4_EVENT_EXECUTION_EVENT, BOGUS3))		|
+		p4_config_pack_cccr(P4_CCCR_THRESHOLD(15) | P4_CCCR_COMPLEMENT		|
+			P4_CCCR_COMPARE);
+}
+
 static int p4_hw_config(struct perf_event *event)
 {
 	int cpu = get_cpu();

@@ -1179,6 +1204,7 @@ static __initconst const struct x86_pmu 
 	.cntval_bits		= ARCH_P4_CNTRVAL_BITS,
 	.cntval_mask		= ARCH_P4_CNTRVAL_MASK,
 	.max_period		= (1ULL << (ARCH_P4_CNTRVAL_BITS - 1)) - 1,
+	.hw_watchdog_set_attr	= p4_hw_watchdog_set_attr,
 	.hw_config		= p4_hw_config,
 	.schedule_events	= p4_pmu_schedule_events,
 	/*

kernel/watchdog.c +5 -1

Index: linux-2.6.git/kernel/watchdog.c
===================================================================
@@ -200,6 +200,8 @@ static int is_softlockup(unsigned long t
 }
 
 #ifdef CONFIG_HARDLOCKUP_DETECTOR
+void __weak hw_nmi_watchdog_set_attr(struct perf_event_attr *wd_attr) { }
+
 static struct perf_event_attr wd_hw_attr = {
 	.type		= PERF_TYPE_HARDWARE,
 	.config		= PERF_COUNT_HW_CPU_CYCLES,

@@ -368,9 +370,11 @@ static int watchdog_nmi_enable(int cpu)
 	if (event != NULL)
 		goto out_enable;
 
-	/* Try to register using hardware perf events */
 	wd_attr = &wd_hw_attr;
 	wd_attr->sample_period = hw_nmi_get_sample_period(watchdog_thresh);
+	hw_nmi_watchdog_set_attr(wd_attr);
+
+	/* Try to register using hardware perf events */
 	event = perf_event_create_kernel_counter(wd_attr, cpu, NULL, watchdog_overflow_callback);
 	if (!IS_ERR(event)) {
 		printk(KERN_INFO "NMI watchdog enabled, takes one hw-pmu counter.\n");

Don Zickus

2011-06-27 19:04 UTC #1 lore raw

Quoted 7 lines · Cyrill Gorcunov

Due to restriction and specifics of Netburst PMU we need a separated event for NMI watchdog. In particular every Netburst event consume not just a counter and config register, but also an additional ESCR register. Since ESCR registers are grouped upon counters (i.e. if ESCR is occupied for some event there is no room for another event to enter until it's released) we need to pick up "least" used ESCR (or most available) for nmi-watchdog purpose -- MSR_P4_CRU_ESCR2/3 was chosen.

I have tested this on a p4 and a westmere using 'perf top' and then lkdtm's 'HARDLOCKUP' command. Both seemed to work correctly indicating that the watchdog and perf were co-existing peacefully. :-)

Thanks Cyrill.

Tested-and-reviewed-by Don Zickus <[email protected]>

kernel/watchdog.c static int is_softlockup(unsigned long t · line 204

Quoted 10 lines

Index: linux-2.6.git/kernel/watchdog.c
===================================================================
kernel/watchdog.c +2 -0
@@ -200,6 +200,8 @@ static int is_softlockup(unsigned long t
 }
 
 #ifdef CONFIG_HARDLOCKUP_DETECTOR
+void __weak hw_nmi_watchdog_set_attr(struct perf_event_attr *wd_attr) { }
+

Though I do wonder about this call in the watchdog. I thought it might be better elsewhere but not sure where.

Cheers,
Don

Stephane Eranian

2011-06-27 19:06 UTC #2 lore raw

Quoted 9 lines · Don Zickus

Quoted 7 lines · Cyrill Gorcunov
Due to restriction and specifics of Netburst PMU we need a separated event for NMI watchdog. In particular every Netburst event consume not just a counter and config register, but also an additional ESCR register. Since ESCR registers are grouped upon counters (i.e. if ESCR is occupied for some event there is no room for another event to enter until it's released) we need to pick up "least" used ESCR (or most available) for nmi-watchdog purpose -- MSR_P4_CRU_ESCR2/3 was chosen.

Quoted 4 lines

I have tested this on a p4 and a westmere using 'perf top' and then lkdtm's 'HARDLOCKUP' command. Both seemed to work correctly indicating that the watchdog and perf were co-existing peacefully. :-)

I tested the patch on a P4 and it worked for me too.

Tested-and-reviewed-by Stephane Eranian <[email protected]>

I like the patch now. It is far simpler than when we started. Thanks.

Quoted 21 lines

Thanks Cyrill.
Tested-and-reviewed-by Don Zickus <[email protected]>
kernel/watchdog.c static int is_softlockup(unsigned long t · line 204
Quoted 10 lines
Index: linux-2.6.git/kernel/watchdog.c
===================================================================
kernel/watchdog.c +2 -0
@@ -200,6 +200,8 @@ static int is_softlockup(unsigned long t
  }
 
  #ifdef CONFIG_HARDLOCKUP_DETECTOR
+void __weak hw_nmi_watchdog_set_attr(struct perf_event_attr *wd_attr) { }
+
Though I do wonder about this call in the watchdog. I thought it might be better elsewhere but not sure where.
Cheers,
Don

Peter Zijlstra

2011-06-27 19:22 UTC #3 lore raw

Quoted 3 lines · Stephane Eranian

Tested-and-reviewed-by Stephane Eranian <[email protected]>
Quoted 1 line
Tested-and-reviewed-by Don Zickus <[email protected]>

Thanks guys!

Cyrill Gorcunov

author

2011-06-28 15:23 UTC #4 lore raw

Quoted 6 lines · Peter Zijlstra

Quoted 3 lines · Stephane Eranian
Tested-and-reviewed-by Stephane Eranian <[email protected]>
Quoted 1 line
Tested-and-reviewed-by Don Zickus <[email protected]>
Thanks guys!

Thanks a huge guys!

Cyrill

Cyrill Gorcunov

author

2011-06-28 15:23 UTC #5 lore raw

In reply to Don Zickus

On Mon, Jun 27, 2011 at 03:03:58PM -0400, Don Zickus wrote:
...

Quoted 8 lines

Quoted 8 lines
Index: linux-2.6.git/kernel/watchdog.c
===================================================================
kernel/watchdog.c +0 -0
@@ -200,6 +200,8 @@ static int is_softlockup(unsigned long t
 }
 

Quoted 6 lines

kernel/watchdog.c static int is_softlockup(unsigned long t · line 204
Quoted 3 lines
kernel/watchdog.c +2 -0
 #ifdef CONFIG_HARDLOCKUP_DETECTOR
+void __weak hw_nmi_watchdog_set_attr(struct perf_event_attr *wd_attr) { }
+
Though I do wonder about this call in the watchdog. I thought it might be better elsewhere but not sure where.

Yeah, not the _best_ place. Not sure too where to put it, patches are welcome ;)

Quoted 3 lines

Cheers,
Don

Cyrill

Stephane Eranian

2011-06-28 15:27 UTC #6 lore raw

Quoted 11 lines · Cyrill Gorcunov

On Mon, Jun 27, 2011 at 03:03:58PM -0400, Don Zickus wrote:
...
Quoted 8 lines
Quoted 8 lines
Index: linux-2.6.git/kernel/watchdog.c
===================================================================
kernel/watchdog.c +0 -0
@@ -200,6 +200,8 @@ static int is_softlockup(unsigned long t
  }
 

Quoted 9 lines

Quoted 6 lines
kernel/watchdog.c static int is_softlockup(unsigned long t · line 204
Quoted 3 lines
kernel/watchdog.c +2 -0
  #ifdef CONFIG_HARDLOCKUP_DETECTOR
+void __weak hw_nmi_watchdog_set_attr(struct perf_event_attr *wd_attr) { }
+
Though I do wonder about this call in the watchdog. I thought it might be better elsewhere but not sure where.
Yeah, not the _best_ place. Not sure too where to put it, patches are welcome ;)

No matter what, this file has to remain in a file that's common to all arch.

Quoted 5 lines

Quoted 3 lines
Cheers,
Don
Cyrill

Cyrill Gorcunov

author

2011-06-28 15:31 UTC #7 lore raw

On Tue, Jun 28, 2011 at 05:24:36PM +0200, Stephane Eranian wrote:
...

Quoted 4 lines

Quoted 3 lines
Yeah, not the _best_ place. Not sure too where to put it, patches are welcome ;)
No matter what, this file has to remain in a file that's common to all arch.

well, hard to say, Stephane, nmi-watchdog is x86 specific as far as I know.

Cyrill

Stephane Eranian

2011-06-28 15:36 UTC #8 lore raw

Quoted 9 lines · Cyrill Gorcunov

On Tue, Jun 28, 2011 at 05:24:36PM +0200, Stephane Eranian wrote:
...
Quoted 4 lines
Quoted 3 lines
Yeah, not the _best_ place. Not sure too where to put it, patches are welcome ;)
No matter what, this file has to remain in a file that's common to all arch.
well, hard to say, Stephane, nmi-watchdog is x86 specific as far as I know.

Sorry I was talking about the weak function() not the file.

Quoted 2 lines

Cyrill

Cyrill Gorcunov

author

2011-06-28 15:43 UTC #9 lore raw

Quoted 4 lines · Stephane Eranian

Quoted 3 lines · Cyrill Gorcunov
On Tue, Jun 28, 2011 at 05:24:36PM +0200, Stephane Eranian wrote:
...
Quoted 1 line
Quoted 1 line

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
Quoted 2 lines
Yeah, not the _best_ place. Not sure too where to put it, patches are welcome ;)
No matter what, this file has to remain in a file that's common to all arch.
well, hard to say, Stephane, nmi-watchdog is x86 specific as far as I know.
Sorry I was talking about the weak function() not the file.

Yes, I get it ;) I mean that this __weak function is always called from x86 site and outside of CONFIG_HARDLOCKUP_DETECTOR doesn't make much sense that is why I placed it there in first place. Though it look somehow... maybe "strange" I would say, that's why I agreed with Don it's not the best place.

Cyrill

Cyrill Gorcunov

author

2011-06-28 15:57 UTC #10 lore raw

Quoted 14 lines · Cyrill Gorcunov

Quoted 4 lines · Stephane Eranian
Quoted 3 lines · Cyrill Gorcunov
On Tue, Jun 28, 2011 at 05:24:36PM +0200, Stephane Eranian wrote:
...
Quoted 1 line
Quoted 1 line
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
Quoted 2 lines
Yeah, not the _best_ place. Not sure too where to put it, patches are welcome ;)
No matter what, this file has to remain in a file that's common to all arch.
well, hard to say, Stephane, nmi-watchdog is x86 specific as far as I know.
Sorry I was talking about the weak function() not the file.

Quoted 6 lines

Yes, I get it ;) I mean that this __weak function is always called from x86 site and outside of CONFIG_HARDLOCKUP_DETECTOR doesn't make much sense that is why I placed it there in first place. Though it look somehow... maybe "strange" I would say, that's why I agreed with Don it's not the best place.
Cyrill

As Don just pointed me (it's a shame that I forget about sparc since I remember someone, maybe even Don, was already pointing me that nmi-watchdog is NOT x86 only thing! :) So yes Stephane, you're of course right, this weak definition better to live in common place. I think if decide to change its place -- better in a separate patch.

Cyrill

Don Zickus

2011-06-28 15:41 UTC #11 lore raw

In reply to Cyrill Gorcunov

Quoted 8 lines · Cyrill Gorcunov

On Tue, Jun 28, 2011 at 05:24:36PM +0200, Stephane Eranian wrote:
...
Quoted 4 lines
Quoted 3 lines
Yeah, not the _best_ place. Not sure too where to put it, patches are welcome ;)
No matter what, this file has to remain in a file that's common to all arch.
well, hard to say, Stephane, nmi-watchdog is x86 specific as far as I know.

No, other arches have similar mechanisms. Actually I think sparc copied-n-pasted an old implementation of the nmi_watchdog on x86.

It is just all we hear about is the watchdog on x86.

Cheers,
Don

Cyrill Gorcunov

author

2011-06-28 15:45 UTC #12 lore raw

Quoted 4 lines · Don Zickus

Quoted 3 lines · Cyrill Gorcunov
On Tue, Jun 28, 2011 at 05:24:36PM +0200, Stephane Eranian wrote:
...
Quoted 1 line
Quoted 1 line

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
Quoted 2 lines
Yeah, not the _best_ place. Not sure too where to put it, patches are welcome ;)
No matter what, this file has to remain in a file that's common to all arch.
well, hard to say, Stephane, nmi-watchdog is x86 specific as far as I know.
No, other arches have similar mechanisms. Actually I think sparc copied-n-pasted an old implementation of the nmi_watchdog on x86.

Mind to point me where it lives?

Quoted 5 lines

It is just all we hear about is the watchdog on x86.
Cheers,
Don

Cyrill

Cyrill Gorcunov

author

2011-06-28 15:47 UTC #13 lore raw

Quoted 7 lines · Cyrill Gorcunov

Quoted 6 lines · Don Zickus
Quoted 5 lines · Cyrill Gorcunov
On Tue, Jun 28, 2011 at 05:24:36PM +0200, Stephane Eranian wrote:
...
Quoted 3 lines
Quoted 3 lines
Yeah, not the _best_ place. Not sure too where to put it, patches are welcome ;)

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
Quoted 1 line
No matter what, this file has to remain in a file that's common to all arch.
well, hard to say, Stephane, nmi-watchdog is x86 specific as far as I know.
No, other arches have similar mechanisms. Actually I think sparc copied-n-pasted an old implementation of the nmi_watchdog on x86.
Mind to point me where it lives?

Indeed, at least SPARC has it as well. Thanks for clarification!

Quoted 8 lines

Quoted 6 lines
It is just all we hear about is the watchdog on x86.
Cheers,
Don
Cyrill

Cyrill

Stephane Eranian

2011-06-28 15:55 UTC #14 lore raw

Cyril,

#ifdef CONFIG_HARDLOCKUP_DETECTOR
+void __weak hw_nmi_watchdog_set_attr(struct perf_event_attr *wd_attr) { }

The weak function has to remain inside the #ifdef. Remember it's just an override in case you cannot using the generic cycle event. It is only needed when you have the HARDLOCK detector, i.e., are using the PMU to detect deadlocks.

I suspect P4 may be the only one so far which exhibited a problem there.

Quoted 30 lines · Cyrill Gorcunov

Quoted 7 lines · Cyrill Gorcunov
Quoted 6 lines · Don Zickus
Quoted 5 lines · Cyrill Gorcunov
On Tue, Jun 28, 2011 at 05:24:36PM +0200, Stephane Eranian wrote:
...
Quoted 3 lines
Quoted 3 lines
Yeah, not the _best_ place. Not sure too where to put it, patches are welcome ;)
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
No matter what, this file has to remain in a file that's common to all arch.
well, hard to say, Stephane, nmi-watchdog is x86 specific as far as I know.
No, other arches have similar mechanisms. Actually I think sparc copied-n-pasted an old implementation of the nmi_watchdog on x86.
Mind to point me where it lives?
Indeed, at least SPARC has it as well. Thanks for clarification!
Quoted 8 lines
Quoted 6 lines
It is just all we hear about is the watchdog on x86.
Cheers,
Don
Cyrill
Cyrill

Cyrill Gorcunov

author

2011-06-28 16:13 UTC #15 lore raw

Quoted 5 lines · Stephane Eranian

Cyrill,
#ifdef CONFIG_HARDLOCKUP_DETECTOR
+void __weak hw_nmi_watchdog_set_attr(struct perf_event_attr *wd_attr) { }

Quoted 7 lines

The weak function has to remain inside the #ifdef. Remember it's just an override in case you cannot using the generic cycle event. It is only needed when you have the HARDLOCK detector, i.e., are using the PMU to detect deadlocks.
I suspect P4 may be the only one so far which exhibited a problem there.

heh ;) Guys, maybe we're talking about different things? Don, when you said "Though I do wonder about this call in the watchdog" you meant the

wd_attr->sample_period = hw_nmi_get_sample_period(watchdog_thresh);
--> hw_nmi_watchdog_set_attr(wd_attr);

itself? Ie you suspect some different point where to call it?

When I said not a "best place" I meant about __weak function bare implementation placed that near to call (which is looked somehow suspicious for me from overall code structure), but I didn't mean the call sequence itself ;)

Cyrill

Don Zickus

2011-06-28 16:29 UTC #16 lore raw

Quoted 9 lines · Cyrill Gorcunov

wd_attr->sample_period = hw_nmi_get_sample_period(watchdog_thresh);
--> hw_nmi_watchdog_set_attr(wd_attr);
itself? Ie you suspect some different point where to call it?
When I said not a "best place" I meant about __weak function bare implementation placed that near to call (which is looked somehow suspicious for me from overall code structure), but I didn't mean the call sequence itself ;)

Sorry I was probably vague. What I meant to say is that the call 'hw_nmi_watchdog_set_attr' is really x86 specific and thought we could bury it down there somehow. Yeah the __weak symbol cleverly gets around it.

I was thinking it would be nice to stick it in hw_nmi_get_sample_period as that is arch specific. But it really wouldn't make sense there.

It's probably fine for now and maybe someday we can come up with a better idea where to put it.

I don't want to waste to much time thinking about it as I have other issues I am dealing with. I just wanted to get this resolved so I can push this patch into RHEL-6.

Cheers,
Don

Cyrill Gorcunov

author

2011-06-28 16:43 UTC #17 lore raw

Quoted 16 lines · Don Zickus

Quoted 9 lines · Cyrill Gorcunov
wd_attr->sample_period = hw_nmi_get_sample_period(watchdog_thresh);
--> hw_nmi_watchdog_set_attr(wd_attr);
itself? Ie you suspect some different point where to call it?
When I said not a "best place" I meant about __weak function bare implementation placed that near to call (which is looked somehow suspicious for me from overall code structure), but I didn't mean the call sequence itself ;)
Sorry I was probably vague. What I meant to say is that the call 'hw_nmi_watchdog_set_attr' is really x86 specific and thought we could bury it down there somehow. Yeah the __weak symbol cleverly gets around it.

Quoted 5 lines

I was thinking it would be nice to stick it in hw_nmi_get_sample_period as that is arch specific. But it really wouldn't make sense there.
It's probably fine for now and maybe someday we can come up with a better idea where to put it.

OK, clear enough, lets stick with other issues. Thanks!

Quoted 7 lines

I don't want to waste to much time thinking about it as I have other issues I am dealing with. I just wanted to get this resolved so I can push this patch into RHEL-6.
Cheers,
Don

Cyrill

tip-bot for Cyrill Gorcunov

author

2011-07-01 15:21 UTC #18 lore raw

In reply to Cyrill Gorcunov

Commit-ID:  1880c4ae182afb5650c5678949ecfe7ff66a724e
Gitweb:     http://git.kernel.org/tip/1880c4ae182afb5650c5678949ecfe7ff66a724e
Author:     Cyrill Gorcunov <[email protected]>
AuthorDate: Thu, 23 Jun 2011 16:49:18 +0400
Committer:  Ingo Molnar <[email protected]>
CommitDate: Fri, 1 Jul 2011 11:06:34 +0200

perf, x86: Add hw_watchdog_set_attr() in a sake of nmi-watchdog on P4

Due to restriction and specifics of Netburst PMU we need a separated event for NMI watchdog. In particular every Netburst event consumes not just a counter and a config register, but also an additional ESCR register.

Since ESCR registers are grouped upon counters (i.e. if ESCR is occupied for some event there is no room for another event to enter until its released) we need to pick up the "least" used ESCR (or the most available one) for nmi-watchdog purposes -- so MSR_P4_CRU_ESCR2/3 was chosen.

With this patch nmi-watchdog and perf top should be able to run simultaneously.

Signed-off-by Cyrill Gorcunov <[email protected]>

CC Lin Ming <[email protected]>

CC Arnaldo Carvalho de Melo <[email protected]>

CC Frederic Weisbecker <[email protected]>

Tested-and-reviewed-by Don Zickus <[email protected]>

Tested-and-reviewed-by Stephane Eranian <[email protected]>

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

Link http://lkml.kernel.org/r/20110623124918.GC13050@sun open here

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

---
 arch/x86/kernel/cpu/perf_event.c    |    7 +++++++
 arch/x86/kernel/cpu/perf_event_p4.c |   26 ++++++++++++++++++++++++++
 kernel/watchdog.c                   |    6 +++++-
 3 files changed, 38 insertions(+), 1 deletions(-)

arch/x86/kernel/cpu/perf_event.c +7 -0

index 3a0338b..8a57f9a 100644
@@ -233,6 +233,7 @@ struct x86_pmu {
 	void		(*enable_all)(int added);
 	void		(*enable)(struct perf_event *);
 	void		(*disable)(struct perf_event *);
+	void		(*hw_watchdog_set_attr)(struct perf_event_attr *attr);
 	int		(*hw_config)(struct perf_event *event);
 	int		(*schedule_events)(struct cpu_hw_events *cpuc, int n, int *assign);
 	unsigned	eventsel;

@@ -315,6 +316,12 @@ static u64 __read_mostly hw_cache_extra_regs
 				[PERF_COUNT_HW_CACHE_OP_MAX]
 				[PERF_COUNT_HW_CACHE_RESULT_MAX];
 
+void hw_nmi_watchdog_set_attr(struct perf_event_attr *wd_attr)
+{
+	if (x86_pmu.hw_watchdog_set_attr)
+		x86_pmu.hw_watchdog_set_attr(wd_attr);
+}
+
 /*
  * Propagate event elapsed time into the generic event.
  * Can only be executed on the CPU where the event is active.

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

index ead584f..f76fddf 100644
@@ -705,6 +705,31 @@ static int p4_validate_raw_event(struct perf_event *event)
 	return 0;
 }
 
+static void p4_hw_watchdog_set_attr(struct perf_event_attr *wd_attr)
+{
+	/*
+	 * Watchdog ticks are special on Netburst, we use
+	 * that named "non-sleeping" ticks as recommended
+	 * by Intel SDM Vol3b.
+	 */
+	WARN_ON_ONCE(wd_attr->type	!= PERF_TYPE_HARDWARE ||
+		     wd_attr->config	!= PERF_COUNT_HW_CPU_CYCLES);
+
+	wd_attr->type	= PERF_TYPE_RAW;
+	wd_attr->config	=
+		p4_config_pack_escr(P4_ESCR_EVENT(P4_EVENT_EXECUTION_EVENT)		|
+			P4_ESCR_EMASK_BIT(P4_EVENT_EXECUTION_EVENT, NBOGUS0)		|
+			P4_ESCR_EMASK_BIT(P4_EVENT_EXECUTION_EVENT, NBOGUS1)		|
+			P4_ESCR_EMASK_BIT(P4_EVENT_EXECUTION_EVENT, NBOGUS2)		|
+			P4_ESCR_EMASK_BIT(P4_EVENT_EXECUTION_EVENT, NBOGUS3)		|
+			P4_ESCR_EMASK_BIT(P4_EVENT_EXECUTION_EVENT, BOGUS0)		|
+			P4_ESCR_EMASK_BIT(P4_EVENT_EXECUTION_EVENT, BOGUS1)		|
+			P4_ESCR_EMASK_BIT(P4_EVENT_EXECUTION_EVENT, BOGUS2)		|
+			P4_ESCR_EMASK_BIT(P4_EVENT_EXECUTION_EVENT, BOGUS3))		|
+		p4_config_pack_cccr(P4_CCCR_THRESHOLD(15) | P4_CCCR_COMPLEMENT		|
+			P4_CCCR_COMPARE);
+}
+
 static int p4_hw_config(struct perf_event *event)
 {
 	int cpu = get_cpu();

@@ -1179,6 +1204,7 @@ static __initconst const struct x86_pmu p4_pmu = {
 	.cntval_bits		= ARCH_P4_CNTRVAL_BITS,
 	.cntval_mask		= ARCH_P4_CNTRVAL_MASK,
 	.max_period		= (1ULL << (ARCH_P4_CNTRVAL_BITS - 1)) - 1,
+	.hw_watchdog_set_attr	= p4_hw_watchdog_set_attr,
 	.hw_config		= p4_hw_config,
 	.schedule_events	= p4_pmu_schedule_events,
 	/*

kernel/watchdog.c +5 -1

index 3d0c56a..752b75b 100644
@@ -200,6 +200,8 @@ static int is_softlockup(unsigned long touch_ts)
 }
 
 #ifdef CONFIG_HARDLOCKUP_DETECTOR
+void __weak hw_nmi_watchdog_set_attr(struct perf_event_attr *wd_attr) { }
+
 static struct perf_event_attr wd_hw_attr = {
 	.type		= PERF_TYPE_HARDWARE,
 	.config		= PERF_COUNT_HW_CPU_CYCLES,

@@ -368,9 +370,11 @@ static int watchdog_nmi_enable(int cpu)
 	if (event != NULL)
 		goto out_enable;
 
-	/* Try to register using hardware perf events */
 	wd_attr = &wd_hw_attr;
 	wd_attr->sample_period = hw_nmi_get_sample_period(watchdog_thresh);
+	hw_nmi_watchdog_set_attr(wd_attr);
+
+	/* Try to register using hardware perf events */
 	event = perf_event_create_kernel_counter(wd_attr, cpu, NULL, watchdog_overflow_callback);
 	if (!IS_ERR(event)) {
 		printk(KERN_INFO "NMI watchdog enabled, takes one hw-pmu counter.\n");