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

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

[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.

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 <gorcunov@openvz.org>
CC: Don Zickus <dzickus@redhat.com>
CC: Ingo Molnar <mingo@redhat.com>
CC: Lin Ming <ming.m.lin@intel.com>
CC: Peter Zijlstra <a.p.zijlstra@chello.nl>
CC: Arnaldo Carvalho de Melo <acme@redhat.com>
CC: Frederic Weisbecker <fweisbec@gmail.com>
CC: Stephane Eranian <eranian@google.com>
---

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

Index: linux-2.6.git/arch/x86/kernel/cpu/perf_event.c
===================================================================
--- linux-2.6.git.orig/arch/x86/kernel/cpu/perf_event.c
+++ 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.
Index: linux-2.6.git/arch/x86/kernel/cpu/perf_event_p4.c
===================================================================
--- linux-2.6.git.orig/arch/x86/kernel/cpu/perf_event_p4.c
+++ 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,
 	/*
Index: linux-2.6.git/kernel/watchdog.c
===================================================================
--- linux-2.6.git.orig/kernel/watchdog.c
+++ 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");

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

[Don Zickus]

On Thu, Jun 23, 2011 at 04:49:18PM +0400, Cyrill Gorcunov wrote:
> 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 <dzickus@redhat.com>

> Index: linux-2.6.git/kernel/watchdog.c
> ===================================================================
> --- linux-2.6.git.orig/kernel/watchdog.c
> +++ 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) { }
> +

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

Cheers,
Don

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

[Stephane Eranian]

On Mon, Jun 27, 2011 at 9:03 PM, Don Zickus <dzickus@redhat.com> wrote:
> On Thu, Jun 23, 2011 at 04:49:18PM +0400, Cyrill Gorcunov wrote:
>> 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. :-)
>

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

Tested-and-reviewed-by: Stephane Eranian <eranian@google.com>

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

> Thanks Cyrill.
>
> Tested-and-reviewed-by: Don Zickus <dzickus@redhat.com>
>
>> Index: linux-2.6.git/kernel/watchdog.c
>> ===================================================================
>> --- linux-2.6.git.orig/kernel/watchdog.c
>> +++ 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) { }
>> +
>
> Though I do wonder about this call in the watchdog.  I thought it might be
> better elsewhere but not sure where.
>
> Cheers,
> Don
>

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

[Peter Zijlstra]

On Mon, 2011-06-27 at 21:06 +0200, Stephane Eranian wrote:
> Tested-and-reviewed-by: Stephane Eranian <eranian@google.com>

> > Tested-and-reviewed-by: Don Zickus <dzickus@redhat.com> 

Thanks guys!

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

[Cyrill Gorcunov]

On Mon, Jun 27, 2011 at 09:21:15PM +0200, Peter Zijlstra wrote:
> On Mon, 2011-06-27 at 21:06 +0200, Stephane Eranian wrote:
> > Tested-and-reviewed-by: Stephane Eranian <eranian@google.com>
> 
> > > Tested-and-reviewed-by: Don Zickus <dzickus@redhat.com> 
> 
> Thanks guys!

Thanks a huge guys!

	Cyrill

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

[Cyrill Gorcunov]

On Mon, Jun 27, 2011 at 03:03:58PM -0400, Don Zickus wrote:
...
> 
> > Index: linux-2.6.git/kernel/watchdog.c
> > ===================================================================
> > --- linux-2.6.git.orig/kernel/watchdog.c
> > +++ 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) { }
> > +
> 
> 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 ;)

> 
> Cheers,
> Don

	Cyrill

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

[Stephane Eranian]

On Mon, Jun 27, 2011 at 9:32 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>
> On Mon, Jun 27, 2011 at 03:03:58PM -0400, Don Zickus wrote:
> ...
> >
> > > Index: linux-2.6.git/kernel/watchdog.c
> > > ===================================================================
> > > --- linux-2.6.git.orig/kernel/watchdog.c
> > > +++ 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) { }
> > > +
> >
> > 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.

> >
> > Cheers,
> > Don
>
>        Cyrill

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

[Cyrill Gorcunov]

On Tue, Jun 28, 2011 at 05:24:36PM +0200, Stephane Eranian wrote:
...
> >
> > 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

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

[Stephane Eranian]

On Tue, Jun 28, 2011 at 5:28 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> On Tue, Jun 28, 2011 at 05:24:36PM +0200, Stephane Eranian wrote:
> ...
>> >
>> > 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.

>        Cyrill
>

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

[Don Zickus]

On Tue, Jun 28, 2011 at 07:28:27PM +0400, Cyrill Gorcunov wrote:
> On Tue, Jun 28, 2011 at 05:24:36PM +0200, Stephane Eranian wrote:
> ...
> > >
> > > 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

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

[Cyrill Gorcunov]

On Tue, Jun 28, 2011 at 05:32:27PM +0200, Stephane Eranian wrote:
> On Tue, Jun 28, 2011 at 5:28 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> > On Tue, Jun 28, 2011 at 05:24:36PM +0200, Stephane Eranian wrote:
> > ...
> >> >
> >> > 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

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

[Cyrill Gorcunov]

On Tue, Jun 28, 2011 at 11:37:31AM -0400, Don Zickus wrote:
> On Tue, Jun 28, 2011 at 07:28:27PM +0400, Cyrill Gorcunov wrote:
> > On Tue, Jun 28, 2011 at 05:24:36PM +0200, Stephane Eranian wrote:
> > ...
> > > >
> > > > 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?

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

	Cyrill

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

[Cyrill Gorcunov]

On Tue, Jun 28, 2011 at 07:44:22PM +0400, Cyrill Gorcunov wrote:
> On Tue, Jun 28, 2011 at 11:37:31AM -0400, Don Zickus wrote:
> > On Tue, Jun 28, 2011 at 07:28:27PM +0400, Cyrill Gorcunov wrote:
> > > On Tue, Jun 28, 2011 at 05:24:36PM +0200, Stephane Eranian wrote:
> > > ...
> > > > >
> > > > > 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?

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

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

	Cyrill

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

[Stephane Eranian]

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.


On Tue, Jun 28, 2011 at 5:46 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> On Tue, Jun 28, 2011 at 07:44:22PM +0400, Cyrill Gorcunov wrote:
>> On Tue, Jun 28, 2011 at 11:37:31AM -0400, Don Zickus wrote:
>> > On Tue, Jun 28, 2011 at 07:28:27PM +0400, Cyrill Gorcunov wrote:
>> > > On Tue, Jun 28, 2011 at 05:24:36PM +0200, Stephane Eranian wrote:
>> > > ...
>> > > > >
>> > > > > 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?
>
> Indeed, at least SPARC has it as well. Thanks for clarification!
>
>>
>> >
>> > It is just all we hear about is the watchdog on x86.
>> >
>> > Cheers,
>> > Don
>>
>>       Cyrill
>
>        Cyrill
>

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

[Cyrill Gorcunov]

On Tue, Jun 28, 2011 at 07:40:44PM +0400, Cyrill Gorcunov wrote:
> On Tue, Jun 28, 2011 at 05:32:27PM +0200, Stephane Eranian wrote:
> > On Tue, Jun 28, 2011 at 5:28 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> > > On Tue, Jun 28, 2011 at 05:24:36PM +0200, Stephane Eranian wrote:
> > > ...
> > >> >
> > >> > 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

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

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

[Cyrill Gorcunov]

On Tue, Jun 28, 2011 at 05:53:31PM +0200, Stephane Eranian wrote:
> Cyrill,
> 
> #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.
> 

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

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

[Don Zickus]

On Tue, Jun 28, 2011 at 08:11:10PM +0400, Cyrill Gorcunov wrote:
> 
>  	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

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

[Cyrill Gorcunov]

On Tue, Jun 28, 2011 at 12:27:14PM -0400, Don Zickus wrote:
> On Tue, Jun 28, 2011 at 08:11:10PM +0400, Cyrill Gorcunov wrote:
> > 
> >  	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.

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

> 
> 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:perf/core] perf, x86: Add hw_watchdog_set_attr() in a sake of nmi-watchdog on P4

[tip-bot for Cyrill Gorcunov]

Commit-ID:  1880c4ae182afb5650c5678949ecfe7ff66a724e
Gitweb:     http://git.kernel.org/tip/1880c4ae182afb5650c5678949ecfe7ff66a724e
Author:     Cyrill Gorcunov <gorcunov@gmail.com>
AuthorDate: Thu, 23 Jun 2011 16:49:18 +0400
Committer:  Ingo Molnar <mingo@elte.hu>
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 <gorcunov@openvz.org>
CC: Lin Ming <ming.m.lin@intel.com>
CC: Arnaldo Carvalho de Melo <acme@redhat.com>
CC: Frederic Weisbecker <fweisbec@gmail.com>
Tested-and-reviewed-by: Don Zickus <dzickus@redhat.com>
Tested-and-reviewed-by: Stephane Eranian <eranian@google.com>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/20110623124918.GC13050@sun
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 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(-)

diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
index 3a0338b..8a57f9a 100644
--- a/arch/x86/kernel/cpu/perf_event.c
+++ b/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_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.
diff --git a/arch/x86/kernel/cpu/perf_event_p4.c b/arch/x86/kernel/cpu/perf_event_p4.c
index ead584f..f76fddf 100644
--- a/arch/x86/kernel/cpu/perf_event_p4.c
+++ b/arch/x86/kernel/cpu/perf_event_p4.c
@@ -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,
 	/*
diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index 3d0c56a..752b75b 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -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");