perf, x86: Add PERF_COUNT_HW_NMI_WATCHDOG event v2

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 the room 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.

Note that on all other PMUs which support relocation of events between counters this event is a simple alias for PERF_COUNT_HW_CPU_CYCLES.

v2: Add a comment about non-sleeping clockticks.

N.B: An attempts to make an alternate encodings for events didn't make situation better because we would need to track how exactly we substitute the particular event -- hw::config knows nothing from where the event came, from user-space as a raw event or as pre-configured general event. If it comes as raw event we have to track every single bit of ESCR mask and find out if new event would count exactly the same thing as the former event was supposed to. So I found such way inconvenient for users and adding a single code snippet seems to be a way more clean approach.

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

Acked-by 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]>

---

To PeterZ: Peter, I've tried various ways to implement an alternate encoding (Don even tried one which didn't work because of ESCR conflict ;) but all them introduced much code which makes the whole picture more complex I think and there is no 1:1 map between even single event (initially I thought we have something but eventually found they are not). So even new NMI-WATCHDOG event is *not* the same as "power events" were before but they are not supposed to be "exactly" precise in compare with cpu-clocks we use for perf top. So I think it's acceptable trade off -- less precise events for nmi-watchdog and more precise for perf top and friends.

Don, I put your Ack here because the only thing I've changed (in compare with previous tested verion) is PERF_COUNT_HW_NMI_WATCHDOG = 8 (was 7 before), please re-test it again, I've tested it already but still.

Comments are welcome as usuall ;)

 arch/x86/kernel/cpu/perf_event_amd.c   |    1 +
 arch/x86/kernel/cpu/perf_event_intel.c |    1 +
 arch/x86/kernel/cpu/perf_event_p4.c    |   18 ++++++++++++++++++
 arch/x86/kernel/cpu/perf_event_p6.c    |    1 +
 include/linux/perf_event.h             |    1 +
 kernel/watchdog.c                      |    2 +-
 6 files changed, 23 insertions(+), 1 deletion(-)

Index: linux-2.6.git/arch/x86/kernel/cpu/perf_event_amd.c
=====================================================================

arch/x86/kernel/cpu/perf_event_amd.c +1 -0

@@ -102,6 +102,7 @@ static const u64 amd_perfmon_event_map[]
   [PERF_COUNT_HW_CACHE_MISSES]		= 0x0081,
   [PERF_COUNT_HW_BRANCH_INSTRUCTIONS]	= 0x00c2,
   [PERF_COUNT_HW_BRANCH_MISSES]		= 0x00c3,
+  [PERF_COUNT_HW_NMI_WATCHDOG]		= 0x0076,
 };
 
 static u64 amd_pmu_event_map(int hw_event)

Index: linux-2.6.git/arch/x86/kernel/cpu/perf_event_intel.c
=====================================================================

arch/x86/kernel/cpu/perf_event_intel.c +1 -0

@@ -34,6 +34,7 @@ static u64 intel_perfmon_event_map[PERF_
   [PERF_COUNT_HW_BRANCH_INSTRUCTIONS]	= 0x00c4,
   [PERF_COUNT_HW_BRANCH_MISSES]		= 0x00c5,
   [PERF_COUNT_HW_BUS_CYCLES]		= 0x013c,
+  [PERF_COUNT_HW_NMI_WATCHDOG]		= 0x003c,
 };
 
 static struct event_constraint intel_core_event_constraints[] __read_mostly =

Index: linux-2.6.git/arch/x86/kernel/cpu/perf_event_p4.c
=====================================================================

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

@@ -607,6 +607,24 @@ static u64 p4_general_events[PERF_COUNT_
 		P4_ESCR_EMASK_BIT(P4_EVENT_FSB_DATA_ACTIVITY, DRDY_DRV)		|
 		P4_ESCR_EMASK_BIT(P4_EVENT_FSB_DATA_ACTIVITY, DRDY_OWN))	|
 	p4_config_pack_cccr(P4_CCCR_EDGE | P4_CCCR_COMPARE),
+
+  /*
+   * This is a specific way to count non-halted clockticks as SDM Vol.3B
+   * "30.11.2 Non-Sleep Clockticks" suggest. We set threshold and complement
+   * flag as result every tick is accounted and delivered to the counter.
+   */
+  [PERF_COUNT_HW_NMI_WATCHDOG] =
+	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 struct p4_event_bind *p4_config_get_bind(u64 config)

Index: linux-2.6.git/arch/x86/kernel/cpu/perf_event_p6.c
=====================================================================

arch/x86/kernel/cpu/perf_event_p6.c +1 -0

@@ -12,6 +12,7 @@ static const u64 p6_perfmon_event_map[]
   [PERF_COUNT_HW_BRANCH_INSTRUCTIONS]	= 0x00c4,
   [PERF_COUNT_HW_BRANCH_MISSES]		= 0x00c5,
   [PERF_COUNT_HW_BUS_CYCLES]		= 0x0062,
+  [PERF_COUNT_HW_NMI_WATCHDOG]		= 0x0079,
 };
 
 static u64 p6_pmu_event_map(int hw_event)

Index: linux-2.6.git/include/linux/perf_event.h
=====================================================================

include/linux/perf_event.h +1 -0

@@ -53,6 +53,7 @@ enum perf_hw_id {
 	PERF_COUNT_HW_BRANCH_MISSES		= 5,
 	PERF_COUNT_HW_BUS_CYCLES		= 6,
 	PERF_COUNT_HW_STALLED_CYCLES		= 7,
+	PERF_COUNT_HW_NMI_WATCHDOG		= 8,
 
 	PERF_COUNT_HW_MAX,			/* non-ABI */
 };

Index: linux-2.6.git/kernel/watchdog.c
=====================================================================

kernel/watchdog.c +1 -1

@@ -191,7 +191,7 @@ static int is_softlockup(unsigned long t
 #ifdef CONFIG_HARDLOCKUP_DETECTOR
 static struct perf_event_attr wd_hw_attr = {
 	.type		= PERF_TYPE_HARDWARE,
-	.config		= PERF_COUNT_HW_CPU_CYCLES,
+	.config		= PERF_COUNT_HW_NMI_WATCHDOG,
 	.size		= sizeof(struct perf_event_attr),
 	.pinned		= 1,
 	.disabled	= 1,

Signature · 7 lines

-- 
    Cyrill
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Quoted 10 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 the room 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.

Note that on all other PMUs which support relocation of events between counters this event is a simple alias for PERF_COUNT_HW_CPU_CYCLES.

Cyril,

What ever happened with this patch?

Cheers,
Don

Quoted 16 lines · Don Zickus

Quoted 10 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 the room 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.

Note that on all other PMUs which support relocation of events between counters this event is a simple alias for PERF_COUNT_HW_CPU_CYCLES.

Cyril,

What ever happened with this patch?

Quoted 3 lines

Cheers,
Don

Well, it's flowing around, I didn't get any more Ack's other than your. Since it introduces kind of abi I presume more ack's would be welcome before it (possibly) get merged.

Quoted 17 lines · Cyrill Gorcunov

Quoted 16 lines · Don Zickus

Quoted 10 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 the room 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.

Note that on all other PMUs which support relocation of events between counters this event is a simple alias for PERF_COUNT_HW_CPU_CYCLES.

Cyril,

What ever happened with this patch?

Quoted 7 lines

Quoted 3 lines

Cheers,
Don

Well, it's flowing around, I didn't get any more Ack's other than your. Since it introduces kind of abi I presume more ack's would be welcome before it (possibly) get merged.

we could avoid that abi thing by simply not making it an exposed event

Its not like userspace ever should be using it anyway

Ok, I'll take a look (sorry for top posting, ISP problem)

Quoted 30 lines · Peter Zijlstra

Quoted 17 lines · Cyrill Gorcunov

Quoted 16 lines · Don Zickus

Quoted 10 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 the room 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.

Note that on all other PMUs which support relocation of events between counters this event is a simple alias for PERF_COUNT_HW_CPU_CYCLES.

Cyril,

What ever happened with this patch?

Quoted 7 lines

Quoted 3 lines

Cheers,
Don

Well, it's flowing around, I didn't get any more Ack's other than your. Since it introduces kind of abi I presume more ack's would be welcome before it (possibly) get merged.

we could avoid that abi thing by simply not making it an exposed event

Its not like userspace ever should be using it anyway

Cyrill,
[repost because of MIME crap]

I admit I don't quite understand how this patch works around the limitation. In the end you are still going to program some cycle event into a P4 PMU register. So how it is going to free more counters for regular users?

The key here is that we use that named non-sleeping ticks (as oprofile
did) for nmi-watchdog and it allows us to free "cpu-cycles" counter for user needs. Of course we pick up one counter for this but it doesn't intersect with "cpu-cycles" counter (because counters are grouped and can count only specified events in each group). Stepane, should I post more details?

Cyrill

Quoted 2 lines · Stephane Eranian

Cyrill,
[repost because of MIME crap]

I saw first as well, sorry for delay, my isp is broken today :/

Quoted 7 lines

I admit I don't quite understand how this patch works around the limitation. In the end you are still going to program some cycle event into a P4 PMU register. So how it is going to free more counters for regular users?

 The key here is that we use that named non-sleeping ticks (as oprofile did) for nmi-watchdog and it allows us to free "cpu-cycles" counter for user needs. Of course we pick up one counter for this but it doesn't intersect with "cpu-cycles" counter (because counters are grouped and can count only specified events in each group). Stepane, should I post more details?

Quoted 7 lines

 The key here is that we use that named non-sleeping ticks (as oprofile did) for nmi-watchdog and it allows us to free "cpu-cycles" counter for user needs. Of course we pick up one counter for this but it doesn't intersect with "cpu-cycles" counter (because counters are grouped and can count only specified events in each group). Stepane, should I post more details?

Ok, so you're using a different PMU event for the watchdog. But then, in this case why not simply change kernel/watchdog.c to hardcode something specific for P4, i.e, change the wd_hw_attr.config/type fields to match the event you want.

You could create a arch specific callback to setup config/type.

I don't think it makes sense to expose yet another generic PMU event, especially given the name you gave to it and what it actually does. People might wonder what good is that for?

On Tue, Jun 21, 2011 at 07:10:05PM +0200, Stephane Eranian wrote:
...

Quoted 11 lines

Quoted 7 lines

 The key here is that we use that named non-sleeping ticks (as oprofile did) for nmi-watchdog and it allows us to free "cpu-cycles" counter for user needs. Of course we pick up one counter for this but it doesn't intersect with "cpu-cycles" counter (because counters are grouped and can count only specified events in each group). Stepane, should I post more details?

Ok, so you're using a different PMU event for the watchdog. But then, in this case why not simply change kernel/watchdog.c to hardcode something specific for P4, i.e, change the wd_hw_attr.config/type fields to match the event you want.

 It doesn't change things much. Of course I can put some specifics into .config but where is the guarantee some new generic event would not ever intersect with it. I know (for example) we could reserve -1ULL for this event but again where is the guarantee that it will never ever be used system wide in future for some different event?

So I need some event-id in which I may be sure it will never be used for different purpose.

Quoted 5 lines

You could create a arch specific callback to setup config/type.

I don't think it makes sense to expose yet another generic PMU event, especially given the name you gave to it and what it actually does. People might wonder what good is that for?

       Cyrill

Quoted 7 lines

 It doesn't change things much. Of course I can put some specifics into .config but where is the guarantee some new generic event would not ever intersect with it. I know (for example) we could reserve -1ULL for this event but again where is the guarantee that it will never ever be used system wide in future for some different event?

I am not talking about a new generic PMU event. I am talking about you hardcoding a raw event in the callback: type = PERF_TYPE_RAW, config=0x003c.

On Tue, Jun 21, 2011 at 07:26:16PM +0200, Stephane Eranian wrote:
...

Quoted 11 lines

Quoted 7 lines

 It doesn't change things much. Of course I can put some specifics into .config but where is the guarantee some new generic event would not ever intersect with it. I know (for example) we could reserve -1ULL for this event but again where is the guarantee that it will never ever be used system wide in future for some different event?

I am not talking about a new generic PMU event. I am talking about you hardcoding a raw event in the callback: type = PERF_TYPE_RAW, config=0x003c.

watchdog.c is system-wide and even if it's not used by archs other than x86 doesn't mean it'll not in future ;) So we could encode this watchdog event as you mention (and I would check for this values and map it internally to bits I need, this btw will require me to check if this bits do not intersect with some already valid bits combination but it's a different problem) but than we _must_ be sure it never bring problems as new arch or consumer appear. So, no, I don't like this approach, it's fragile. But gimme some time -- I'll re-check all bits, probably there is some workaround.

       Cyrill

If in arch/x86/kernel/cpu/perf_event.c I add a callback which checks:

   if (netburst_host) {
       attr.type = PERF_TYPE_RAW
       attr.config = 0x3c
   } else {
      attr.type = PERF_TYPE_HARDWARE
      attr.config = PERF_COUNT_HW__CPU_CYCLES
  }

I simply don't think PERF_COUNT_HW_NMI_WATCHDOG should ever be exposed to users.

On Tue, Jun 21, 2011 at 08:06:54PM +0200, Stephane Eranian wrote:
...

Quoted 12 lines

If in arch/x86/kernel/cpu/perf_event.c I add a callback which checks:

   if (netburst_host) {
       attr.type = PERF_TYPE_RAW
       attr.config = 0x3c
   } else {
      attr.type = PERF_TYPE_HARDWARE
      attr.config = PERF_COUNT_HW__CPU_CYCLES
  }

Quoted 2 lines

I simply don't think PERF_COUNT_HW_NMI_WATCHDOG should ever be exposed to users.

yes, and peterz pointed it out as well, i agree of course.

Quoted 3 lines

yes, and peterz pointed it out as well, i agree of course.

What about something like below?

Cyrill

Note that I've not tested it but rather need approval/rejecting on idea in general.

On Wed, Jun 22, 2011 at 01:21:34PM +0400, Cyrill Gorcunov wrote:
...

Quoted 2 lines

Note that I've not tested it but rather need approval/rejecting on idea in general.

The final version is below. Stephane, note that it's almost the same idea as you proposed except it uses explicit namings to mark out that watchdog cycles are special.

Quoted 9 lines · Cyrill Gorcunov

On Wed, Jun 22, 2011 at 01:21:34PM +0400, Cyrill Gorcunov wrote:
...

Quoted 2 lines

Note that I've not tested it but rather need approval/rejecting on idea in general.

The final version is below. Stephane, note that it's almost the same idea as you proposed except it uses explicit namings to mark out that watchdog cycles are special.

This looks okay.

The only alternative I see (This is wha I had in mind at the beginning) that would
not require this new hidden generic event would be to have watchdog.c invoke an arch-specific callback to fill out the attr.type, attr->config fields directly:

No new hidden event, just a x86_pmu + a per-arch callbacks.

Quoted 2 lines

No new hidden event, just a x86_pmu + a per-arch callbacks.

Looks quite good for me, Don? (i'll cook some draft patch for review meanwhile).

Cyrill

On Thu, Jun 23, 2011 at 01:54:39PM +0400, Cyrill Gorcunov wrote:
...

Quoted 6 lines

Quoted 2 lines

No new hidden event, just a x86_pmu + a per-arch callbacks.

Looks quite good for me, Don? (i'll cook some draft patch for review meanwhile).

Cyrill

Since we are going to make __weak linking anyway maybe something like below fit even beter? (untested)

Cyrill

---
 arch/x86/kernel/cpu/perf_event_p4.c |   26 ++++++++++++++++++++++++++
 kernel/watchdog.c                   |    3 +++
 2 files changed, 29 insertions(+)

Index: linux-2.6.git/arch/x86/kernel/cpu/perf_event_p4.c
===================================================================

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

@@ -705,6 +705,32 @@ static int p4_validate_raw_event(struct 
 	return 0;
 }
 
+void hw_nmi_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.
+	 */
+	if (wd_attr->type	!= PERF_TYPE_HARDWARE ||
+	   wd_attr->attr.config	!= PERF_COUNT_HW_CPU_CYCLES)
+		return;
+
+	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();

Index: linux-2.6.git/kernel/watchdog.c
===================================================================

kernel/watchdog.c +3 -0

@@ -355,6 +355,8 @@ static int watchdog(void *unused)
 
 
 #ifdef CONFIG_HARDLOCKUP_DETECTOR
+void __weak hw_nmi_watchdog_set_attr(struct perf_event_attr *wd_attr) { }
+
 static int watchdog_nmi_enable(int cpu)
 {
 	struct perf_event_attr *wd_attr;

@@ -371,6 +373,7 @@ static int watchdog_nmi_enable(int cpu)
 	/* 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);
 	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");

Quoted 7 lines · Cyrill Gorcunov

On Thu, Jun 23, 2011 at 01:54:39PM +0400, Cyrill Gorcunov wrote:
...

Quoted 5 lines

Quoted 2 lines

No new hidden event, just a x86_pmu + a per-arch callbacks.

Looks quite good for me, Don? (i'll cook some draft patch for review meanwhile).

Quoted 4 lines

Quoted 1 line

    Cyrill

Since we are going to make __weak linking anyway maybe something like below fit even beter? (untested)

I don't think the compiler knows what platform you are running on and may just blindly link your new p4 function for all x86s, which is probably not what you want.

Cheers,
Don

Quoted 65 lines

      Cyrill
---
 arch/x86/kernel/cpu/perf_event_p4.c |   26 ++++++++++++++++++++++++++ kernel/watchdog.c                   |    3 +++

 2 files changed, 29 insertions(+)

Index: linux-2.6.git/arch/x86/kernel/cpu/perf_event_p4.c
===================================================================

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

@@ -705,6 +705,32 @@ static int p4_validate_raw_event(struct
       return 0;
  }
 
+void hw_nmi_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.
+      */
+     if (wd_attr->type       != PERF_TYPE_HARDWARE ||
+        wd_attr->attr.config != PERF_COUNT_HW_CPU_CYCLES)
+             return;
+
+     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();

Index: linux-2.6.git/kernel/watchdog.c
===================================================================

kernel/watchdog.c +3 -0

@@ -355,6 +355,8 @@ static int watchdog(void *unused)
 
 
  #ifdef CONFIG_HARDLOCKUP_DETECTOR
+void __weak hw_nmi_watchdog_set_attr(struct perf_event_attr *wd_attr) { }
+
  static int watchdog_nmi_enable(int cpu)
  {
       struct perf_event_attr *wd_attr;

@@ -371,6 +373,7 @@ static int watchdog_nmi_enable(int cpu)
       /* 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);
       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");

Quoted 8 lines · Don Zickus

Quoted 7 lines · Cyrill Gorcunov

On Thu, Jun 23, 2011 at 01:54:39PM +0400, Cyrill Gorcunov wrote:
...

Quoted 5 lines

Quoted 2 lines

No new hidden event, just a x86_pmu + a per-arch callbacks.

Looks quite good for me, Don? (i'll cook some draft patch for review meanwhile).

Quoted 9 lines

Quoted 4 lines

Quoted 1 line

    Cyrill

Since we are going to make __weak linking anyway maybe something like below fit even beter? (untested)

I don't think the compiler knows what platform you are running on and may just blindly link your new p4 function for all x86s, which is probably not what you want.

Don, is right. You need the level of indirection I had in my outline patch.

You also don't need the:

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

+       if (wd_attr->type       != PERF_TYPE_HARDWARE ||
+          wd_attr->attr.config != PERF_COUNT_HW_CPU_CYCLES)
+               return;

In the p4 callback given you know your coming in for the watchdog.

Quoted 25 lines · Stephane Eranian

Quoted 8 lines · Don Zickus

Quoted 7 lines · Cyrill Gorcunov

On Thu, Jun 23, 2011 at 01:54:39PM +0400, Cyrill Gorcunov wrote:
...

Quoted 5 lines

Quoted 2 lines

No new hidden event, just a x86_pmu + a per-arch callbacks.

Looks quite good for me, Don? (i'll cook some draft patch for review meanwhile).

Quoted 9 lines

Quoted 4 lines

Quoted 1 line

    Cyrill

Since we are going to make __weak linking anyway maybe something like below fit even beter? (untested)

I don't think the compiler knows what platform you are running on and may just blindly link your new p4 function for all x86s, which is probably not what you want.

Don, is right. You need the level of indirection I had in my outline patch.

You also don't need the:
+       if (wd_attr->type       != PERF_TYPE_HARDWARE ||
+          wd_attr->attr.config != PERF_COUNT_HW_CPU_CYCLES)
+               return;

Quoted 2 lines

In the p4 callback given you know your coming in for the watchdog.

Yes, that is why in __weak implementation I dropped it. So guys, what we stick with -- __weak with second level indirection?

       Cyrill

Quoted 1 line

No, first level in watchdog.c, the other callback has to be implemented from x86_pmu as you had it.

On Thu, Jun 23, 2011 at 02:03:50PM +0200, Stephane Eranian wrote:
...

Quoted 4 lines

Quoted 1 line

No, first level in watchdog.c, the other callback has to be implemented from x86_pmu as you had it.

Quoted 6 lines · Cyrill Gorcunov

On Thu, Jun 23, 2011 at 02:03:50PM +0200, Stephane Eranian wrote:
...

Quoted 4 lines

Quoted 1 line

No, first level in watchdog.c, the other callback has to be implemented from x86_pmu as you had it.

Something like below I suppose

       Cyrill
---
 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(-)

Index: linux-2.6.git/arch/x86/kernel/cpu/perf_event.c
===================================================================

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

@@ -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];

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

+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.
Index: linux-2.6.git/arch/x86/kernel/cpu/perf_event_p4.c
===================================================================

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

@@ -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,
        /*

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) { }
+
  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;

Yes.

Quoted 7 lines · Cyrill Gorcunov

On Thu, Jun 23, 2011 at 01:54:39PM +0400, Cyrill Gorcunov wrote:
...

Quoted 5 lines

Quoted 2 lines

No new hidden event, just a x86_pmu + a per-arch callbacks.

Looks quite good for me, Don? (i'll cook some draft patch for review meanwhile).

Quoted 4 lines

Quoted 1 line

Cyrill

Since we are going to make __weak linking anyway maybe something like below fit even beter? (untested)

I don't think the compiler knows what platform you are running on and may just blindly link your new p4 function for all x86s, which is probably not what you want.