[PATCH 0/4] perf, x86: collection of pentium 4 fixes

[PATCH 0/4] perf, x86: collection of pentium 4 fixes

[Don Zickus]

This patch series has various clean ups for the P4 perf nmi layer.
Some of the changes touch the other platforms but those should be minimally
intrusive.

It would be nice to get these into 2.6.39 as two patches fix 'unknown' NMIs,
which is listed as a regression from 2.6.38.

Cyrill Gorcunov (3):
  perf, x86: P4 PMU -- Use perf_sample_data_init helper
  perf, x86: P4 PMU - Don't forget to clear cpuc->active_mask on
    overflow
  perf, x86: Add PERF_COUNT_HW_NMI_WATCHDOG event

Don Zickus (1):
  perf, nmi: Move LVT un-masking into irq handlers

 arch/x86/kernel/cpu/perf_event.c       |    5 ++-
 arch/x86/kernel/cpu/perf_event_amd.c   |    1 +
 arch/x86/kernel/cpu/perf_event_intel.c |    4 +++
 arch/x86/kernel/cpu/perf_event_p4.c    |   34 +++++++++++++++++++++++++------
 arch/x86/kernel/cpu/perf_event_p6.c    |    1 +
 include/linux/perf_event.h             |    1 +
 kernel/watchdog.c                      |    2 +-
 7 files changed, 38 insertions(+), 10 deletions(-)

-- 
1.7.4.2

[PATCH 1/4] perf, x86: P4 PMU -- Use perf_sample_data_init helper

[Don Zickus]

From: Cyrill Gorcunov <gorcunov@gmail.com>

Instead of opencoded assignments better to use
perf_sample_data_init helper.

Tested-by: Lin Ming <ming.m.lin@intel.com>
Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
Signed-off-by: Don Zickus <dzickus@redhat.com>
---
 arch/x86/kernel/cpu/perf_event_p4.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event_p4.c b/arch/x86/kernel/cpu/perf_event_p4.c
index c2520e1..6f2bd89 100644
--- a/arch/x86/kernel/cpu/perf_event_p4.c
+++ b/arch/x86/kernel/cpu/perf_event_p4.c
@@ -912,8 +912,7 @@ static int p4_pmu_handle_irq(struct pt_regs *regs)
 	int idx, handled = 0;
 	u64 val;
 
-	data.addr = 0;
-	data.raw = NULL;
+	perf_sample_data_init(&data, 0);
 
 	cpuc = &__get_cpu_var(cpu_hw_events);
 
-- 
1.7.4.2

Re: [PATCH 1/4] perf, x86: P4 PMU -- Use perf_sample_data_init helper

[Ingo Molnar]

* Don Zickus <dzickus@redhat.com> wrote:

> From: Cyrill Gorcunov <gorcunov@gmail.com>
> 
> Instead of opencoded assignments better to use
> perf_sample_data_init helper.
> 
> Tested-by: Lin Ming <ming.m.lin@intel.com>
> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> Signed-off-by: Don Zickus <dzickus@redhat.com>
> ---
>  arch/x86/kernel/cpu/perf_event_p4.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/perf_event_p4.c b/arch/x86/kernel/cpu/perf_event_p4.c
> index c2520e1..6f2bd89 100644
> --- a/arch/x86/kernel/cpu/perf_event_p4.c
> +++ b/arch/x86/kernel/cpu/perf_event_p4.c
> @@ -912,8 +912,7 @@ static int p4_pmu_handle_irq(struct pt_regs *regs)
>  	int idx, handled = 0;
>  	u64 val;
>  
> -	data.addr = 0;
> -	data.raw = NULL;
> +	perf_sample_data_init(&data, 0);

This one sure can wait for v2.6.40, right?

Thanks,

	Ingo

Re: [PATCH 1/4] perf, x86: P4 PMU -- Use perf_sample_data_init helper

[Cyrill Gorcunov]

yeah, it can wait

On Friday, April 22, 2011, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Don Zickus <dzickus@redhat.com> wrote:
>
>> From: Cyrill Gorcunov <gorcunov@gmail.com>
>>
>> Instead of opencoded assignments better to use
>> perf_sample_data_init helper.
>>
>> Tested-by: Lin Ming <ming.m.lin@intel.com>
>> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
>> Signed-off-by: Don Zickus <dzickus@redhat.com>
>> ---
>>  arch/x86/kernel/cpu/perf_event_p4.c |    3 +--
>>  1 files changed, 1 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/perf_event_p4.c b/arch/x86/kernel/cpu/perf_event_p4.c
>> index c2520e1..6f2bd89 100644
>> --- a/arch/x86/kernel/cpu/perf_event_p4.c
>> +++ b/arch/x86/kernel/cpu/perf_event_p4.c
>> @@ -912,8 +912,7 @@ static int p4_pmu_handle_irq(struct pt_regs *regs)
>>       int idx, handled = 0;
>>       u64 val;
>>
>> -     data.addr = 0;
>> -     data.raw = NULL;
>> +     perf_sample_data_init(&data, 0);
>
> This one sure can wait for v2.6.40, right?
>
> Thanks,
>
>         Ingo
>

[tip:perf/core] perf, x86: P4 PMU -- Use perf_sample_data_init helper

[tip-bot for Cyrill Gorcunov]

Commit-ID:  103b3934817a7c42fba6e1ef76ecb390a2837d40
Gitweb:     http://git.kernel.org/tip/103b3934817a7c42fba6e1ef76ecb390a2837d40
Author:     Cyrill Gorcunov <gorcunov@gmail.com>
AuthorDate: Thu, 21 Apr 2011 11:03:20 -0400
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Fri, 22 Apr 2011 10:20:04 +0200

perf, x86: P4 PMU -- Use perf_sample_data_init helper

Instead of opencoded assignments better to use
perf_sample_data_init helper.

Tested-by: Lin Ming <ming.m.lin@intel.com>
Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
Signed-off-by: Don Zickus <dzickus@redhat.com>
Cc: Cyrill Gorcunov <gorcunov@gmail.com>
Link: http://lkml.kernel.org/r/1303398203-2918-2-git-send-email-dzickus@redhat.com
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/kernel/cpu/perf_event_p4.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event_p4.c b/arch/x86/kernel/cpu/perf_event_p4.c
index 8ff882f..ae31e96 100644
--- a/arch/x86/kernel/cpu/perf_event_p4.c
+++ b/arch/x86/kernel/cpu/perf_event_p4.c
@@ -912,8 +912,7 @@ static int p4_pmu_handle_irq(struct pt_regs *regs)
 	int idx, handled = 0;
 	u64 val;
 
-	data.addr = 0;
-	data.raw = NULL;
+	perf_sample_data_init(&data, 0);
 
 	cpuc = &__get_cpu_var(cpu_hw_events);
 

[PATCH 2/4] perf, x86: P4 PMU - Don't forget to clear cpuc->active_mask on overflow

[Don Zickus]

From: Cyrill Gorcunov <gorcunov@gmail.com>

It's not enough to simply disable event on overflow the cpuc->active_mask
should be cleared as well otherwise counter may stall in "active" even
in real being already disabled (which potentially may lead to the situation
that user may not use this counter further).

Don pointed "I also noticed this patch fixed some unknown NMIs on a P4 when
I stressed the box".

Tested-by: Lin Ming <ming.m.lin@intel.com>
Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
Acked-by: Don Zickus <dzickus@redhat.com>
Signed-off-by: Don Zickus <dzickus@redhat.com>
---
 arch/x86/kernel/cpu/perf_event_p4.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event_p4.c b/arch/x86/kernel/cpu/perf_event_p4.c
index 6f2bd89..56ba449 100644
--- a/arch/x86/kernel/cpu/perf_event_p4.c
+++ b/arch/x86/kernel/cpu/perf_event_p4.c
@@ -946,7 +946,7 @@ static int p4_pmu_handle_irq(struct pt_regs *regs)
 		if (!x86_perf_event_set_period(event))
 			continue;
 		if (perf_event_overflow(event, 1, &data, regs))
-			p4_pmu_disable_event(event);
+			x86_pmu_stop(event, 0);
 	}
 
 	if (handled) {
-- 
1.7.4.2

[tip:perf/urgent] perf, x86: P4 PMU - Don't forget to clear cpuc->active_mask on overflow

[tip-bot for Cyrill Gorcunov]

Commit-ID:  1ea5a6afd95a4803900c97ed63a47a883ebe7b3e
Gitweb:     http://git.kernel.org/tip/1ea5a6afd95a4803900c97ed63a47a883ebe7b3e
Author:     Cyrill Gorcunov <gorcunov@gmail.com>
AuthorDate: Thu, 21 Apr 2011 11:03:21 -0400
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Fri, 22 Apr 2011 10:21:34 +0200

perf, x86: P4 PMU - Don't forget to clear cpuc->active_mask on overflow

It's not enough to simply disable event on overflow the
cpuc->active_mask should be cleared as well otherwise counter
may stall in "active" even in real being already disabled (which
potentially may lead to the situation that user may not use this
counter further).

Don pointed out that:

 " I also noticed this patch fixed some unknown NMIs
   on a P4 when I stressed the box".

Tested-by: Lin Ming <ming.m.lin@intel.com>
Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
Acked-by: Don Zickus <dzickus@redhat.com>
Signed-off-by: Don Zickus <dzickus@redhat.com>
Cc: Cyrill Gorcunov <gorcunov@gmail.com>
Link: http://lkml.kernel.org/r/1303398203-2918-3-git-send-email-dzickus@redhat.com
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/kernel/cpu/perf_event_p4.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event_p4.c b/arch/x86/kernel/cpu/perf_event_p4.c
index c2520e1..d1f77e2 100644
--- a/arch/x86/kernel/cpu/perf_event_p4.c
+++ b/arch/x86/kernel/cpu/perf_event_p4.c
@@ -947,7 +947,7 @@ static int p4_pmu_handle_irq(struct pt_regs *regs)
 		if (!x86_perf_event_set_period(event))
 			continue;
 		if (perf_event_overflow(event, 1, &data, regs))
-			p4_pmu_disable_event(event);
+			x86_pmu_stop(event, 0);
 	}
 
 	if (handled) {

[PATCH 3/4] perf, nmi: Move LVT un-masking into irq handlers

[Don Zickus]

It was noticed that P4 machines were generating double NMIs for each
perf event.  These extra NMIs lead to 'Dazed and confused' messages on
the screen.

I tracked this down to a P4 quirk that said the overflow bit had to be
cleared before re-enabling the apic LVT mask.  My first attempt was
to move the un-masking inside the perf nmi handler from before the
chipset NMI handler to after.

This broke Nehalem boxes that seem to like the unmasking before the
counters themselves are re-enabled.

In order to keep this change simple for 2.6.39, I decided to just
simply move the apic LVT un-masking to the beginning of all the
chipset NMI handlers, with the exeption of Pentium4's to fix the
double NMI issue.

Later on we can move the un-masking to later in the handlers to save
a number of 'extra' NMIs on those particular chipsets.

I tested this change on a P4 machine, an AMD machine, a Nehalem box,
and a core2quad box.  'perf top' worked correctly along with various
other small 'perf record' runs.  Anything high stress breaks all the
machines but that is a different problem.

Thanks to various people for testing different versions of this patch.

https://bugzilla.kernel.org/show_bug.cgi?id=33252

Reported-and-tested-by: Shaun Ruffell <sruffell@digium.com>
Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>
CC: Peter Zijlstra <peterz@infradead.org>
CC: Robert Richter <robert.richter@amd.com>
CC: Maciej Rutecki <maciej.rutecki@gmail.com>
CC: George Spelvin <linux@horizon.com>
CC: Stephane Eranian <eranian@google.com>
Signed-off-by: Don Zickus <dzickus@redhat.com>
---
 arch/x86/kernel/cpu/perf_event.c       |    5 +++--
 arch/x86/kernel/cpu/perf_event_intel.c |    3 +++
 arch/x86/kernel/cpu/perf_event_p4.c    |   14 ++++++++++----
 3 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
index eed3673a..97c6c44 100644
--- a/arch/x86/kernel/cpu/perf_event.c
+++ b/arch/x86/kernel/cpu/perf_event.c
@@ -1284,6 +1284,9 @@ static int x86_pmu_handle_irq(struct pt_regs *regs)
 
 	cpuc = &__get_cpu_var(cpu_hw_events);
 
+	/* chipsets have their own quirks when to unmask */
+	apic_write(APIC_LVTPC, APIC_DM_NMI);
+
 	for (idx = 0; idx < x86_pmu.num_counters; idx++) {
 		if (!test_bit(idx, cpuc->active_mask)) {
 			/*
@@ -1370,8 +1373,6 @@ perf_event_nmi_handler(struct notifier_block *self,
 		return NOTIFY_DONE;
 	}
 
-	apic_write(APIC_LVTPC, APIC_DM_NMI);
-
 	handled = x86_pmu.handle_irq(args->regs);
 	if (!handled)
 		return NOTIFY_DONE;
diff --git a/arch/x86/kernel/cpu/perf_event_intel.c b/arch/x86/kernel/cpu/perf_event_intel.c
index 8fc2b2c..d2326e1 100644
--- a/arch/x86/kernel/cpu/perf_event_intel.c
+++ b/arch/x86/kernel/cpu/perf_event_intel.c
@@ -933,6 +933,9 @@ static int intel_pmu_handle_irq(struct pt_regs *regs)
 
 	cpuc = &__get_cpu_var(cpu_hw_events);
 
+	/* chipsets have their own quirks when to unmask */
+	apic_write(APIC_LVTPC, APIC_DM_NMI);
+
 	intel_pmu_disable_all();
 	handled = intel_pmu_drain_bts_buffer();
 	status = intel_pmu_get_status();
diff --git a/arch/x86/kernel/cpu/perf_event_p4.c b/arch/x86/kernel/cpu/perf_event_p4.c
index 56ba449..2d0ac91 100644
--- a/arch/x86/kernel/cpu/perf_event_p4.c
+++ b/arch/x86/kernel/cpu/perf_event_p4.c
@@ -949,11 +949,17 @@ static int p4_pmu_handle_irq(struct pt_regs *regs)
 			x86_pmu_stop(event, 0);
 	}
 
-	if (handled) {
-		/* p4 quirk: unmask it again */
-		apic_write(APIC_LVTPC, apic_read(APIC_LVTPC) & ~APIC_LVT_MASKED);
+	if (handled)
 		inc_irq_stat(apic_perf_irqs);
-	}
+
+        /*
+	 * P4 quirks:
+	 * - An overflown perfctr will assert its interrupt
+	 *   until the OVF flag in its CCCR is cleared.
+	 * - LVTPC is masked on interrupt and must be
+	 *   unmasked by the LVTPC handler.
+	 */
+	apic_write(APIC_LVTPC, APIC_DM_NMI);
 
 	return handled;
 }
-- 
1.7.4.2

Re: [PATCH 3/4] perf, nmi: Move LVT un-masking into irq handlers

[Ingo Molnar]

* Don Zickus <dzickus@redhat.com> wrote:

> --- a/arch/x86/kernel/cpu/perf_event.c
> +++ b/arch/x86/kernel/cpu/perf_event.c
> @@ -1284,6 +1284,9 @@ static int x86_pmu_handle_irq(struct pt_regs *regs)
>  
>  	cpuc = &__get_cpu_var(cpu_hw_events);
>  
> +	/* chipsets have their own quirks when to unmask */
> +	apic_write(APIC_LVTPC, APIC_DM_NMI);
> +

What sense does this comment make in this place?

Yes, chipsets have their own quirks - but the generic handler is not one of 
them. So a more appropriate comment would be to point out why we want to unmask 
there - before PMU handling or after it, etc.

Like the P4 quirk is documented a bit better:

> +        /*
> +	 * P4 quirks:
> +	 * - An overflown perfctr will assert its interrupt
> +	 *   until the OVF flag in its CCCR is cleared.
> +	 * - LVTPC is masked on interrupt and must be
> +	 *   unmasked by the LVTPC handler.
> +	 */
> +	apic_write(APIC_LVTPC, APIC_DM_NMI);

(btw., there's whitespace damage above as well.)

Furthermore, the P4 comment should *explain* the quirk coherently, not just 
list random facts. What happens, why, where, and why do we unmask the LVTPC in 
that spot.

Thanks,

	Ingo

Re: [PATCH 3/4] perf, nmi: Move LVT un-masking into irq handlers

[Don Zickus]

On Fri, Apr 22, 2011 at 10:26:36AM +0200, Ingo Molnar wrote:
> 
> * Don Zickus <dzickus@redhat.com> wrote:
> 
> > --- a/arch/x86/kernel/cpu/perf_event.c
> > +++ b/arch/x86/kernel/cpu/perf_event.c
> > @@ -1284,6 +1284,9 @@ static int x86_pmu_handle_irq(struct pt_regs *regs)
> >  
> >  	cpuc = &__get_cpu_var(cpu_hw_events);
> >  
> > +	/* chipsets have their own quirks when to unmask */
> > +	apic_write(APIC_LVTPC, APIC_DM_NMI);
> > +
> 
> What sense does this comment make in this place?
> 
> Yes, chipsets have their own quirks - but the generic handler is not one of 
> them. So a more appropriate comment would be to point out why we want to unmask 
> there - before PMU handling or after it, etc.

Yup.  I rework that.

> 
> Like the P4 quirk is documented a bit better:
> 
> > +        /*
> > +	 * P4 quirks:
> > +	 * - An overflown perfctr will assert its interrupt
> > +	 *   until the OVF flag in its CCCR is cleared.
> > +	 * - LVTPC is masked on interrupt and must be
> > +	 *   unmasked by the LVTPC handler.
> > +	 */
> > +	apic_write(APIC_LVTPC, APIC_DM_NMI);
> 
> (btw., there's whitespace damage above as well.)
> 
> Furthermore, the P4 comment should *explain* the quirk coherently, not just 
> list random facts. What happens, why, where, and why do we unmask the LVTPC in 
> that spot.

Yeah, I copied that from the old watchdog code.  I'll revise that too and
repost.

Cheers,
Don

Re: [PATCH 3/4] perf, nmi: Move LVT un-masking into irq handlers

[Cyrill Gorcunov]

On 04/25/2011 05:39 PM, Don Zickus wrote:
> On Fri, Apr 22, 2011 at 10:26:36AM +0200, Ingo Molnar wrote:
>>
>> * Don Zickus <dzickus@redhat.com> wrote:
>>
>>> --- a/arch/x86/kernel/cpu/perf_event.c
>>> +++ b/arch/x86/kernel/cpu/perf_event.c
>>> @@ -1284,6 +1284,9 @@ static int x86_pmu_handle_irq(struct pt_regs *regs)
>>>  
>>>  	cpuc = &__get_cpu_var(cpu_hw_events);
>>>  
>>> +	/* chipsets have their own quirks when to unmask */
>>> +	apic_write(APIC_LVTPC, APIC_DM_NMI);
>>> +
>>
>> What sense does this comment make in this place?
>>
>> Yes, chipsets have their own quirks - but the generic handler is not one of 
>> them. So a more appropriate comment would be to point out why we want to unmask 
>> there - before PMU handling or after it, etc.
> 
> Yup.  I rework that.

I suspect the comment from one of my draft version might fulfit, Don do you have it?
/me looking in sent mbox

+	/*
+	 * We have to unmask LVTPC *before* enabling counters,
+	 * the key moment is that LVTPC is getting masked on
+	 * PMI arrival and if PMU enabled before APIC unmasked
+	 * a new oveflow signal may reach it but not propagated
+	 * as NMI (due to transition timings) and LVTPC eventually
+	 * stick masked forever dropping any further PMI signals.
+	 */

it was for pmu-intel but i think same applies to general pmu handler.

> 
>>
>> Like the P4 quirk is documented a bit better:
>>
>>> +        /*
>>> +	 * P4 quirks:
>>> +	 * - An overflown perfctr will assert its interrupt
>>> +	 *   until the OVF flag in its CCCR is cleared.
>>> +	 * - LVTPC is masked on interrupt and must be
>>> +	 *   unmasked by the LVTPC handler.
>>> +	 */
>>> +	apic_write(APIC_LVTPC, APIC_DM_NMI);
>>
>> (btw., there's whitespace damage above as well.)
>>
>> Furthermore, the P4 comment should *explain* the quirk coherently, not just 
>> list random facts. What happens, why, where, and why do we unmask the LVTPC in 
>> that spot.

  Ingo, actually I don't understand what else could be added here, should we post
the backtrace calls there? Or plain "we reach this point with OVF bit set" is enough?

  Don't get me wrong please but the whole picture of what is happening can be seen only when
all caller sequence is taken into account and once (for some reason) the sequence
get changed the "detailed" comment would simply mess the comment reader so I think
the former comment is detailed enough and what is more important it's "general" enough
so it doesn't depend on when code is called but points a reader on hw details and it's
up to a reader to check "current" calling sequence because kernel code changes too
damn fast ;)

> 
> Yeah, I copied that from the old watchdog code.  I'll revise that too and
> repost.
> 
> Cheers,
> Don


-- 
    Cyrill

Re: [PATCH 3/4] perf, nmi: Move LVT un-masking into irq handlers

[Cyrill Gorcunov]

On 04/25/2011 06:18 PM, lina wrote:
> sorry,
> 
> How do I solve the problem of delivery failed?
> 
> Thanks,
> 
> Delivery to the following recipient failed permanently:
> 
>     linux-kernel@vger.kernel.org <mailto:linux-kernel@vger.kernel.org>
> 
> Technical details of permanent failure:
> Google tried to deliver your message, but it was rejected by the recipient domain. We recommend contacting the other email provider for further information about the cause of this error. The error
> that the other server returned was: 550 550 5.7.1 Content-Policy reject msg: The message contains HTML subpart, therefore we consider it SPAM or Outlook Virus.  TEXT/PLAIN is accepted.! BF:<U
> 0.499653>; S1758441Ab1DYOLi (state 18).

It's written in the tech details of the failure

"Content-Policy reject msg: The message contains HTML subpart",
so no HTML messages should be sent to LKML, plain text messaging only.

-- 
    Cyrill

Re: [PATCH 3/4] perf, nmi: Move LVT un-masking into irq handlers

[Don Zickus]

On Mon, Apr 25, 2011 at 06:15:25PM +0400, Cyrill Gorcunov wrote:
> 
>   Don't get me wrong please but the whole picture of what is happening can be seen only when
> all caller sequence is taken into account and once (for some reason) the sequence
> get changed the "detailed" comment would simply mess the comment reader so I think
> the former comment is detailed enough and what is more important it's "general" enough
> so it doesn't depend on when code is called but points a reader on hw details and it's
> up to a reader to check "current" calling sequence because kernel code changes too
> damn fast ;)

I think Ingo is looking for is something like

/*
 * It has been observed that quirks in the P4 perf hw has forced the
 * following sequence of events to happen in the order below
 *
 * - clear the OVF bit (as it will continue to assert the NMI line)
 * - unmask the apic LVTPC bit to allow NMIs from the PMU again
 * - optionally re-enable the PMU to count events again
 *
 * Un-masking the apic prematurely (before clearing the OVF bit) has led
 * to the creation of a second NMI event (which led to the unknown NMI
 * warnings) due to the fact that the PMU will continue to generate an
 * interrupt until its OVF bit is cleared.
 */

Something that specifically documents what we saw, why we saw it and what
we are doing to avoid it.

Cheers,
Don

Re: [PATCH 3/4] perf, nmi: Move LVT un-masking into irq handlers

[Cyrill Gorcunov]

On 04/25/2011 06:50 PM, Don Zickus wrote:
> On Mon, Apr 25, 2011 at 06:15:25PM +0400, Cyrill Gorcunov wrote:
>>
>>   Don't get me wrong please but the whole picture of what is happening can be seen only when
>> all caller sequence is taken into account and once (for some reason) the sequence
>> get changed the "detailed" comment would simply mess the comment reader so I think
>> the former comment is detailed enough and what is more important it's "general" enough
>> so it doesn't depend on when code is called but points a reader on hw details and it's
>> up to a reader to check "current" calling sequence because kernel code changes too
>> damn fast ;)
> 
> I think Ingo is looking for is something like
> 
> /*
>  * It has been observed that quirks in the P4 perf hw has forced the
>  * following sequence of events to happen in the order below
>  *
>  * - clear the OVF bit (as it will continue to assert the NMI line)
>  * - unmask the apic LVTPC bit to allow NMIs from the PMU again
>  * - optionally re-enable the PMU to count events again
>  *
>  * Un-masking the apic prematurely (before clearing the OVF bit) has led
>  * to the creation of a second NMI event (which led to the unknown NMI
>  * warnings) due to the fact that the PMU will continue to generate an
>  * interrupt until its OVF bit is cleared.
>  */
> 
> Something that specifically documents what we saw, why we saw it and what
> we are doing to avoid it.
> 
> Cheers,
> Don

Excellent, thanks a huge Don!

-- 
    Cyrill

[PATCH 4/4] perf, x86: Add PERF_COUNT_HW_NMI_WATCHDOG event

[Don Zickus]

From: Cyrill Gorcunov <gorcunov@gmail.com>

Due to restriction and specifics of Netburst PMU we need
a separated event for NMI watchdog. Note that on all other
than P4 PMU cpus this event is a simple alias for PERF_COUNT_HW_CPU_CYCLES.

This event is only used by x86 for now so no other archs involved.

Tested-by: Lin Ming <ming.m.lin@intel.com>
Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
Acked-by: Don Zickus <dzickus@redhat.com>
CC: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Don Zickus <dzickus@redhat.com>
---
 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    |   15 +++++++++++++++
 arch/x86/kernel/cpu/perf_event_p6.c    |    1 +
 include/linux/perf_event.h             |    1 +
 kernel/watchdog.c                      |    2 +-
 6 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event_amd.c b/arch/x86/kernel/cpu/perf_event_amd.c
index 461f62b..248d8d7 100644
--- a/arch/x86/kernel/cpu/perf_event_amd.c
+++ b/arch/x86/kernel/cpu/perf_event_amd.c
@@ -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)
diff --git a/arch/x86/kernel/cpu/perf_event_intel.c b/arch/x86/kernel/cpu/perf_event_intel.c
index d2326e1..92351ec 100644
--- a/arch/x86/kernel/cpu/perf_event_intel.c
+++ b/arch/x86/kernel/cpu/perf_event_intel.c
@@ -34,6 +34,7 @@ static const u64 intel_perfmon_event_map[] =
   [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[] =
diff --git a/arch/x86/kernel/cpu/perf_event_p4.c b/arch/x86/kernel/cpu/perf_event_p4.c
index 2d0ac91..9e13864 100644
--- a/arch/x86/kernel/cpu/perf_event_p4.c
+++ b/arch/x86/kernel/cpu/perf_event_p4.c
@@ -557,6 +557,7 @@ static __initconst const u64 p4_hw_cache_event_ids
 };
 
 static u64 p4_general_events[PERF_COUNT_HW_MAX] = {
+
   /* non-halted CPU clocks */
   [PERF_COUNT_HW_CPU_CYCLES] =
 	p4_config_pack_escr(P4_ESCR_EVENT(P4_EVENT_GLOBAL_POWER_EVENTS)		|
@@ -607,6 +608,20 @@ static u64 p4_general_events[PERF_COUNT_HW_MAX] = {
 		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),
+
+  /* we use that named non-sleeping calls */
+  [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)
diff --git a/arch/x86/kernel/cpu/perf_event_p6.c b/arch/x86/kernel/cpu/perf_event_p6.c
index 20c097e..3daeb18 100644
--- a/arch/x86/kernel/cpu/perf_event_p6.c
+++ b/arch/x86/kernel/cpu/perf_event_p6.c
@@ -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)
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 311b4dc..b67564d 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -52,6 +52,7 @@ enum perf_hw_id {
 	PERF_COUNT_HW_BRANCH_INSTRUCTIONS	= 4,
 	PERF_COUNT_HW_BRANCH_MISSES		= 5,
 	PERF_COUNT_HW_BUS_CYCLES		= 6,
+	PERF_COUNT_HW_NMI_WATCHDOG		= 7,
 
 	PERF_COUNT_HW_MAX,			/* non-ABI */
 };
diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index 14733d4..3b6e0af 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -191,7 +191,7 @@ static int is_softlockup(unsigned long touch_ts)
 #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,
-- 
1.7.4.2

Re: [PATCH 4/4] perf, x86: Add PERF_COUNT_HW_NMI_WATCHDOG event

[Ingo Molnar]

* Don Zickus <dzickus@redhat.com> wrote:

> From: Cyrill Gorcunov <gorcunov@gmail.com>
> 
> Due to restriction and specifics of Netburst PMU we need
> a separated event for NMI watchdog. Note that on all other
> than P4 PMU cpus this event is a simple alias for PERF_COUNT_HW_CPU_CYCLES.
> 
> This event is only used by x86 for now so no other archs involved.

hm, there's no explanation exactly why this is needed and what the alternatives 
are.

This sentence:

> +  /* we use that named non-sleeping calls */

does not parse for me.

Thanks,

	Ingo

Re: [PATCH 4/4] perf, x86: Add PERF_COUNT_HW_NMI_WATCHDOG event

[Cyrill Gorcunov]

P4 cant move events between counters in compare with architectural
event so when nmi-watchdog is used perf top is blocked because it
needs same counters but the counters already borrowed for watchdog. as
result we need a different event and counters for watchdog.

Ingo, nonsleeping cycles is from intel sdm, so when someone start
reading this code in future will find this term in sdm fast. or i
could put the reference from sdm itself here, hm?

On Friday, April 22, 2011, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Don Zickus <dzickus@redhat.com> wrote:
>
>> From: Cyrill Gorcunov <gorcunov@gmail.com>
>>
>> Due to restriction and specifics of Netburst PMU we need
>> a separated event for NMI watchdog. Note that on all other
>> than P4 PMU cpus this event is a simple alias for PERF_COUNT_HW_CPU_CYCLES.
>>
>> This event is only used by x86 for now so no other archs involved.
>
> hm, there's no explanation exactly why this is needed and what the alternatives
> are.
>
> This sentence:
>
>> +  /* we use that named non-sleeping calls */
>
> does not parse for me.
>
> Thanks,
>
>         Ingo
>

Re: [PATCH 4/4] perf, x86: Add PERF_COUNT_HW_NMI_WATCHDOG event

[Peter Zijlstra]

On Fri, 2011-04-22 at 12:43 +0400, Cyrill Gorcunov wrote:
> we need a different event and counters for watchdog.
> 
Does it count the same thing though? If so we could look at having
alternative encodings support (we might need something like that for
offcore too).

Re: [PATCH 4/4] perf, x86: Add PERF_COUNT_HW_NMI_WATCHDOG event

[Cyrill Gorcunov]

On Fri, Apr 22, 2011 at 12:54 PM, Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> On Fri, 2011-04-22 at 12:43 +0400, Cyrill Gorcunov wrote:
>> we need a different event and counters for watchdog.
>>
> Does it count the same thing though? If so we could look at having
> alternative encodings support (we might need something like that for
> offcore too).
>

Yes, they count the same thing (well some tech details are different but
not much).By using different counters we are allowed to run them
simultaneously which makes perf top works again.

Could you elaborate what you have in mind with "alternative encodings"
here?

Re: [PATCH 4/4] perf, x86: Add PERF_COUNT_HW_NMI_WATCHDOG event

[Peter Zijlstra]

On Fri, 2011-04-22 at 13:24 +0400, Cyrill Gorcunov wrote:
> On Fri, Apr 22, 2011 at 12:54 PM, Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> > On Fri, 2011-04-22 at 12:43 +0400, Cyrill Gorcunov wrote:
> >> we need a different event and counters for watchdog.
> >>
> > Does it count the same thing though? If so we could look at having
> > alternative encodings support (we might need something like that for
> > offcore too).
> >
> 
> Yes, they count the same thing (well some tech details are different but
> not much).By using different counters we are allowed to run them
> simultaneously which makes perf top works again.
> 
> Could you elaborate what you have in mind with "alternative encodings"
> here?

Right, so the idea is that one event (concept) has two (or more)
representations in the config space. And when scheduling the events we
find we have a conflict, we simply try the alternative encoding(s).

For example, with the offcore bits, we have two events: r1b7 and r1bb
that both have an extra MSR to fill (01A6 and 01A7 resp). Now if you
were to write the same value to these MSRs these events would count the
exact same thing.

So eg. for counting remote dram hits, you can use: r1b7:20ff or
r1bb:20ff, both count the same. So if we want to support a generic event
that counts remote dram hits we must pick one. Now if that one is taken
for measuring some L3 event, we'll have a conflict and not schedule the
thing, even though the other counter might be available.

POWER has something similar and has already implemented this alternative
encoding scheme.

Re: [PATCH 4/4] perf, x86: Add PERF_COUNT_HW_NMI_WATCHDOG event

[Cyrill Gorcunov]

Thanks! Looks interesting. Need to think if duch scheme is possible for p4 case.

On Friday, April 22, 2011, Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> On Fri, 2011-04-22 at 13:24 +0400, Cyrill Gorcunov wrote:
>> On Fri, Apr 22, 2011 at 12:54 PM, Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
>> > On Fri, 2011-04-22 at 12:43 +0400, Cyrill Gorcunov wrote:
>> >> we need a different event and counters for watchdog.
>> >>
>> > Does it count the same thing though? If so we could look at having
>> > alternative encodings support (we might need something like that for
>> > offcore too).
>> >
>>
>> Yes, they count the same thing (well some tech details are different but
>> not much).By using different counters we are allowed to run them
>> simultaneously which makes perf top works again.
>>
>> Could you elaborate what you have in mind with "alternative encodings"
>> here?
>
> Right, so the idea is that one event (concept) has two (or more)
> representations in the config space. And when scheduling the events we
> find we have a conflict, we simply try the alternative encoding(s).
>
> For example, with the offcore bits, we have two events: r1b7 and r1bb
> that both have an extra MSR to fill (01A6 and 01A7 resp). Now if you
> were to write the same value to these MSRs these events would count the
> exact same thing.
>
> So eg. for counting remote dram hits, you can use: r1b7:20ff or
> r1bb:20ff, both count the same. So if we want to support a generic event
> that counts remote dram hits we must pick one. Now if that one is taken
> for measuring some L3 event, we'll have a conflict and not schedule the
> thing, even though the other counter might be available.
>
> POWER has something similar and has already implemented this alternative
> encoding scheme.
>
>
>
>

Re: [PATCH 4/4] perf, x86: Add PERF_COUNT_HW_NMI_WATCHDOG event

[Cyrill Gorcunov]

On 04/22/2011 01:45 PM, Peter Zijlstra wrote:
...
> 
> Right, so the idea is that one event (concept) has two (or more)
> representations in the config space. And when scheduling the events we
> find we have a conflict, we simply try the alternative encoding(s).
> 
...

  Yeah, Peter, I think it's quite possible to use alternative encoding for p4 as well,
not sure tho if some "generalized" approach for all perf would be applied to p4 but
we can make event "aliases" being per-pmu eventually.

  So Ingo, drop this patch then ;)

-- 
    Cyrill

Re: [PATCH 4/4] perf, x86: Add PERF_COUNT_HW_NMI_WATCHDOG event

[Don Zickus]

On Fri, Apr 22, 2011 at 07:15:12PM +0400, Cyrill Gorcunov wrote:
> On 04/22/2011 01:45 PM, Peter Zijlstra wrote:
> ...
> > 
> > Right, so the idea is that one event (concept) has two (or more)
> > representations in the config space. And when scheduling the events we
> > find we have a conflict, we simply try the alternative encoding(s).
> > 
> ...
> 
>   Yeah, Peter, I think it's quite possible to use alternative encoding for p4 as well,
> not sure tho if some "generalized" approach for all perf would be applied to p4 but
> we can make event "aliases" being per-pmu eventually.
> 
>   So Ingo, drop this patch then ;)

Eek, when can you have an alternate patch for this Cyril?  I just want to
make sure this can still eventually find its way into 2.6.40.

Cheers,
Don

Re: [PATCH 4/4] perf, x86: Add PERF_COUNT_HW_NMI_WATCHDOG event

[Cyrill Gorcunov]

On 04/25/2011 05:41 PM, Don Zickus wrote:
> On Fri, Apr 22, 2011 at 07:15:12PM +0400, Cyrill Gorcunov wrote:
>> On 04/22/2011 01:45 PM, Peter Zijlstra wrote:
>> ...
>>>
>>> Right, so the idea is that one event (concept) has two (or more)
>>> representations in the config space. And when scheduling the events we
>>> find we have a conflict, we simply try the alternative encoding(s).
>>>
>> ...
>>
>>   Yeah, Peter, I think it's quite possible to use alternative encoding for p4 as well,
>> not sure tho if some "generalized" approach for all perf would be applied to p4 but
>> we can make event "aliases" being per-pmu eventually.
>>
>>   So Ingo, drop this patch then ;)
> 
> Eek, when can you have an alternate patch for this Cyril?  I just want to
> make sure this can still eventually find its way into 2.6.40.
> 
> Cheers,
> Don

  I need kinda aliases for events (or alternative scheme as PeterZ mentions),
Don I'll try to make something tomorrow, ok? With aliases we would not need
to even modify sources outside of p4 pmu which is good i think. I've some spare
bits in low hw:config bits so this aliases could go there as a start.

-- 
    Cyrill