All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] Use pmc_overflow() to detect rolled back events
@ 2012-08-08  1:07 Sukadev Bhattiprolu
  2012-09-21  0:38 ` Sukadev Bhattiprolu
  0 siblings, 1 reply; 11+ messages in thread
From: Sukadev Bhattiprolu @ 2012-08-08  1:07 UTC (permalink / raw)
  To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, Anton Blanchard, Maynard Johnson


>From 21e9d1775f0c6f37a39e5d682ff74693fa9a4004 Mon Sep 17 00:00:00 2001
From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Date: Tue, 7 Aug 2012 17:53:24 -0700
Subject: [PATCH] Use pmc_overflow to detect rolled back events.

For certain speculative events on Power7, 'perf stat' reports far higher
event count than 'perf record' for the same event.

As described in following commit, a performance monitor exception is raised
even when the the performance events are rolled back.

        commit 0837e3242c73566fc1c0196b4ec61779c25ffc93
        Author: Anton Blanchard <anton@samba.org>
        Date:   Wed Mar 9 14:38:42 2011 +1100

perf_event_interrupt() records an event only when an overflow occurs. But
this check for overflow is a simple 'if (val < 0)'.

Because the events are rolled back, this check for overflow fails and the
event is not recorded. perf_event_interrupt() later uses pmc_overflow() to
detect the overflow and resets the counters and the events are lost completely.

To properly detect the overflow of rolled back events, use pmc_overflow()
even when recording events.

To reproduce:
        $ cat strcpy.c
        #include <stdio.h>
        #include <string.h>
        main()
        {
                char buf[256];

                alarm(5);
                while(1)
                        strcpy(buf, "string1");
        }

        $ perf record -e r20014 ./strcpy
        $ perf report -n > report.1
        $ perf stat -e r20014 > report.2
        # Compare report.1 and report.2

Reported-by: Maynard Johnson <mpjohn@us.ibm.com>
Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
---
 arch/powerpc/perf/core-book3s.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index 8f84bcb..f74b90d 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -1394,7 +1394,7 @@ static void perf_event_interrupt(struct pt_regs *regs)
 		if (!event->hw.idx || is_limited_pmc(event->hw.idx))
 			continue;
 		val = read_pmc(event->hw.idx);
-		if ((int)val < 0) {
+		if (pmc_overflow(val)) {
 			/* event has overflowed */
 			found = 1;
 			record_and_restart(event, val, regs);
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] Use pmc_overflow() to detect rolled back events
  2012-08-08  1:07 [PATCH] Use pmc_overflow() to detect rolled back events Sukadev Bhattiprolu
@ 2012-09-21  0:38 ` Sukadev Bhattiprolu
  2012-11-06  1:08   ` [PATCH 1/2] powerpc/perf: Fix finding overflowed PMC in interrupt Michael Neuling
  0 siblings, 1 reply; 11+ messages in thread
From: Sukadev Bhattiprolu @ 2012-09-21  0:38 UTC (permalink / raw)
  To: Benjamin Herrenschmidt; +Cc: Maynard Johnson, Anton Blanchard, linuxppc-dev


Sukadev Bhattiprolu [sukadev@linux.vnet.ibm.com] wrote:
| 
| >From 21e9d1775f0c6f37a39e5d682ff74693fa9a4004 Mon Sep 17 00:00:00 2001
| From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
| Date: Tue, 7 Aug 2012 17:53:24 -0700
| Subject: [PATCH] Use pmc_overflow to detect rolled back events.
| 
| For certain speculative events on Power7, 'perf stat' reports far higher
| event count than 'perf record' for the same event.
| 
| As described in following commit, a performance monitor exception is raised
| even when the the performance events are rolled back.
| 
|         commit 0837e3242c73566fc1c0196b4ec61779c25ffc93
|         Author: Anton Blanchard <anton@samba.org>
|         Date:   Wed Mar 9 14:38:42 2011 +1100
| 
| perf_event_interrupt() records an event only when an overflow occurs. But
| this check for overflow is a simple 'if (val < 0)'.
| 
| Because the events are rolled back, this check for overflow fails and the
| event is not recorded. perf_event_interrupt() later uses pmc_overflow() to
| detect the overflow and resets the counters and the events are lost completely.
| 
| To properly detect the overflow of rolled back events, use pmc_overflow()
| even when recording events.

Ben,

Sorry for the noise, but please revert this patch (following commit):

	commit 813312110bede27bffd082c25cd31730bd567beb
	Author: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
	Date:   Tue Aug 7 15:07:19 2012 +0000

While it does fix the problem described above and works for the limit-pmc
events, it seems to break on Power7 for other events and when the sample
period is low.

I am still investigating the problem and will follow up with a separate
mail.

Sukadev

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/2] powerpc/perf: Fix finding overflowed PMC in interrupt
  2012-09-21  0:38 ` Sukadev Bhattiprolu
@ 2012-11-06  1:08   ` Michael Neuling
  2012-11-06  1:08     ` [PATCH 2/2] powerpc/perf: Fix for PMCs not making progress Michael Neuling
                       ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Michael Neuling @ 2012-11-06  1:08 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: Linux PPC dev, Michael Neuling, Sukadev Bhattiprolu,
	Paul Mackerras, Anton Blanchard

If a PMC is about to overflow on a counter that's on an active perf event
(ie. less than 256 from the end) and a _different_ PMC overflows just at this
time (a PMC that's not on an active perf event), we currently mark the event as
found, but in reality it's not as it's likely the other PMC that caused the
IRQ.  Since we mark it as found the second catch all for overflows doesn't run,
and we don't reset the overflowing PMC ever.  Hence we keep hitting that same
PMC IRQ over and over and don't reset the actual overflowing counter.

This is a rewrite of the perf interrupt handler for book3s to get around this.
We now check to see if any of the PMCs have actually overflowed (ie >=
0x80000000).  If yes, record it for active counters and just reset it for
inactive counters.  If it's not overflowed, then we check to see if it's one of
the buggy power7 counters and if it is, record it and continue.  If none of the
PMCs match this, then we make note that we couldn't find the PMC that caused
the IRQ.

Signed-off-by: Michael Neuling <mikey@neuling.org>
Reviewed-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
cc: Paul Mackerras <paulus@samba.org>
cc: Anton Blanchard <anton@samba.org>
cc: Linux PPC dev <linuxppc-dev@ozlabs.org>
---
 arch/powerpc/perf/core-book3s.c |   83 +++++++++++++++++++++++++--------------
 1 file changed, 54 insertions(+), 29 deletions(-)

diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index aa2465e..3575def 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -1412,11 +1412,8 @@ unsigned long perf_instruction_pointer(struct pt_regs *regs)
 		return regs->nip;
 }
 
-static bool pmc_overflow(unsigned long val)
+static bool pmc_overflow_power7(unsigned long val)
 {
-	if ((int)val < 0)
-		return true;
-
 	/*
 	 * Events on POWER7 can roll back if a speculative event doesn't
 	 * eventually complete. Unfortunately in some rare cases they will
@@ -1428,7 +1425,15 @@ static bool pmc_overflow(unsigned long val)
 	 * PMCs because a user might set a period of less than 256 and we
 	 * don't want to mistakenly reset them.
 	 */
-	if (pvr_version_is(PVR_POWER7) && ((0x80000000 - val) <= 256))
+	if ((0x80000000 - val) <= 256)
+		return true;
+
+	return false;
+}
+
+static bool pmc_overflow(unsigned long val)
+{
+	if ((int)val < 0)
 		return true;
 
 	return false;
@@ -1439,11 +1444,11 @@ static bool pmc_overflow(unsigned long val)
  */
 static void perf_event_interrupt(struct pt_regs *regs)
 {
-	int i;
+	int i, j;
 	struct cpu_hw_events *cpuhw = &__get_cpu_var(cpu_hw_events);
 	struct perf_event *event;
-	unsigned long val;
-	int found = 0;
+	unsigned long val[8];
+	int found, active;
 	int nmi;
 
 	if (cpuhw->n_limited)
@@ -1458,33 +1463,53 @@ static void perf_event_interrupt(struct pt_regs *regs)
 	else
 		irq_enter();
 
-	for (i = 0; i < cpuhw->n_events; ++i) {
-		event = cpuhw->event[i];
-		if (!event->hw.idx || is_limited_pmc(event->hw.idx))
+	/* Read all the PMCs since we'll need them a bunch of times */
+	for (i = 0; i < ppmu->n_counter; ++i)
+		val[i] = read_pmc(i + 1);
+
+	/* Try to find what caused the IRQ */
+	found = 0;
+	for (i = 0; i < ppmu->n_counter; ++i) {
+		if (!pmc_overflow(val[i]))
 			continue;
-		val = read_pmc(event->hw.idx);
-		if ((int)val < 0) {
-			/* event has overflowed */
-			found = 1;
-			record_and_restart(event, val, regs);
+		if (is_limited_pmc(i + 1))
+			continue; /* these won't generate IRQs */
+		/*
+		 * We've found one that's overflowed.  For active
+		 * counters we need to log this.  For inactive
+		 * counters, we need to reset it anyway
+		 */
+		found = 1;
+		active = 0;
+		for (j = 0; j < cpuhw->n_events; ++j) {
+			event = cpuhw->event[j];
+			if (event->hw.idx == (i + 1)) {
+				active = 1;
+				record_and_restart(event, val[i], regs);
+				break;
+			}
 		}
+		if (!active)
+			/* reset non active counters that have overflowed */
+			write_pmc(i + 1, 0);
 	}
-
-	/*
-	 * In case we didn't find and reset the event that caused
-	 * the interrupt, scan all events and reset any that are
-	 * negative, to avoid getting continual interrupts.
-	 * Any that we processed in the previous loop will not be negative.
-	 */
-	if (!found) {
-		for (i = 0; i < ppmu->n_counter; ++i) {
-			if (is_limited_pmc(i + 1))
+	if (!found && pvr_version_is(PVR_POWER7)) {
+		/* check active counters for special buggy p7 overflow */
+		for (i = 0; i < cpuhw->n_events; ++i) {
+			event = cpuhw->event[i];
+			if (!event->hw.idx || is_limited_pmc(event->hw.idx))
 				continue;
-			val = read_pmc(i + 1);
-			if (pmc_overflow(val))
-				write_pmc(i + 1, 0);
+			if (pmc_overflow_power7(val[event->hw.idx - 1])) {
+				/* event has overflowed in a buggy way*/
+				found = 1;
+				record_and_restart(event,
+						   val[event->hw.idx - 1],
+						   regs);
+			}
 		}
 	}
+	if (!found)
+		printk(KERN_WARNING "Can't find PMC that caused IRQ\n");
 
 	/*
 	 * Reset MMCR0 to its normal value.  This will set PMXE and
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/2] powerpc/perf: Fix for PMCs not making progress
  2012-11-06  1:08   ` [PATCH 1/2] powerpc/perf: Fix finding overflowed PMC in interrupt Michael Neuling
@ 2012-11-06  1:08     ` Michael Neuling
  2012-11-06  1:25     ` [PATCH 1/2] powerpc/perf: Fix finding overflowed PMC in interrupt Anton Blanchard
  2012-12-22  1:07     ` Sukadev Bhattiprolu
  2 siblings, 0 replies; 11+ messages in thread
From: Michael Neuling @ 2012-11-06  1:08 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: Linux PPC dev, Michael Neuling, Sukadev Bhattiprolu,
	Paul Mackerras, Anton Blanchard

On POWER7 when we have really small counts left before overflow, we can take a
PMU IRQ, but the PMC gets wound back to just before the overflow.

If the kernel is setting the PMC to a value just before the overflow, we can
get interrupted again without the PMC making any progress (ie another buggy
overflow).  In this case, we can end up making no forward progress, with the
PMC interrupt returning us to the same count over and over.

The below detects when we are making no forward progress (ie. delta = 0) and
then increases the amount left before the overflow.  This stops us from locking
up.

Signed-off-by: Michael Neuling <mikey@neuling.org>
Reviewed-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
cc: Paul Mackerras <paulus@samba.org>
cc: Anton Blanchard <anton@samba.org>
cc: Linux PPC dev <linuxppc-dev@ozlabs.org>
---
 arch/powerpc/perf/core-book3s.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index 3575def..f1018c8 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -1349,6 +1349,8 @@ static void record_and_restart(struct perf_event *event, unsigned long val,
 	 */
 	val = 0;
 	left = local64_read(&event->hw.period_left) - delta;
+	if (delta == 0)
+		left++;
 	if (period) {
 		if (left <= 0) {
 			left += period;
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] powerpc/perf: Fix finding overflowed PMC in interrupt
  2012-11-06  1:08   ` [PATCH 1/2] powerpc/perf: Fix finding overflowed PMC in interrupt Michael Neuling
  2012-11-06  1:08     ` [PATCH 2/2] powerpc/perf: Fix for PMCs not making progress Michael Neuling
@ 2012-11-06  1:25     ` Anton Blanchard
  2012-11-06  1:53       ` Michael Neuling
  2012-11-06  1:53       ` Michael Neuling
  2012-12-22  1:07     ` Sukadev Bhattiprolu
  2 siblings, 2 replies; 11+ messages in thread
From: Anton Blanchard @ 2012-11-06  1:25 UTC (permalink / raw)
  To: Michael Neuling
  Cc: Linux PPC dev, Sukadev Bhattiprolu, Paul Mackerras,
	Benjamin Herrenschmidt


Hi,

Thanks for looking into this mess. One small thing we can fix in a
follow up patch:

> +	if (!found)
> +		printk(KERN_WARNING "Can't find PMC that caused

I think it would be worth making that a printk_ratelimited. We are
probably dead at this stage but we shouldn't spam the console
at ludicrous speed in the process.

Anton

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] powerpc/perf: Fix finding overflowed PMC in interrupt
  2012-11-06  1:25     ` [PATCH 1/2] powerpc/perf: Fix finding overflowed PMC in interrupt Anton Blanchard
@ 2012-11-06  1:53       ` Michael Neuling
  2012-11-06  1:53       ` Michael Neuling
  1 sibling, 0 replies; 11+ messages in thread
From: Michael Neuling @ 2012-11-06  1:53 UTC (permalink / raw)
  To: Anton Blanchard; +Cc: Sukadev Bhattiprolu, Paul Mackerras, Linux PPC dev

> Thanks for looking into this mess. One small thing we can fix in a
> follow up patch:
> > +	if (!found)
> > +		printk(KERN_WARNING "Can't find PMC that caused
> 
> I think it would be worth making that a printk_ratelimited. We are
> probably dead at this stage but we shouldn't spam the console
> at ludicrous speed in the process.

Thanks, good point.

Mikey

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/2] powerpc/perf: Fix finding overflowed PMC in interrupt
  2012-11-06  1:25     ` [PATCH 1/2] powerpc/perf: Fix finding overflowed PMC in interrupt Anton Blanchard
  2012-11-06  1:53       ` Michael Neuling
@ 2012-11-06  1:53       ` Michael Neuling
  2012-11-06  6:47         ` Anshuman Khandual
  1 sibling, 1 reply; 11+ messages in thread
From: Michael Neuling @ 2012-11-06  1:53 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: Linux PPC dev, Michael Neuling, Sukadev Bhattiprolu,
	Paul Mackerras, Anton Blanchard

If a PMC is about to overflow on a counter that's on an active perf event
(ie. less than 256 from the end) and a _different_ PMC overflows just at this
time (a PMC that's not on an active perf event), we currently mark the event as
found, but in reality it's not as it's likely the other PMC that caused the
IRQ.  Since we mark it as found the second catch all for overflows doesn't run,
and we don't reset the overflowing PMC ever.  Hence we keep hitting that same
PMC IRQ over and over and don't reset the actual overflowing counter.

This is a rewrite of the perf interrupt handler for book3s to get around this.
We now check to see if any of the PMCs have actually overflowed (ie >=
0x80000000).  If yes, record it for active counters and just reset it for
inactive counters.  If it's not overflowed, then we check to see if it's one of
the buggy power7 counters and if it is, record it and continue.  If none of the
PMCs match this, then we make note that we couldn't find the PMC that caused
the IRQ.

Signed-off-by: Michael Neuling <mikey@neuling.org>
Reviewed-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
cc: Paul Mackerras <paulus@samba.org>
cc: Anton Blanchard <anton@samba.org>
cc: Linux PPC dev <linuxppc-dev@ozlabs.org>
---
 arch/powerpc/perf/core-book3s.c |   83 +++++++++++++++++++++++++--------------
 1 file changed, 54 insertions(+), 29 deletions(-)

diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index aa2465e..53fc7b8 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -1412,11 +1412,8 @@ unsigned long perf_instruction_pointer(struct pt_regs *regs)
 		return regs->nip;
 }
 
-static bool pmc_overflow(unsigned long val)
+static bool pmc_overflow_power7(unsigned long val)
 {
-	if ((int)val < 0)
-		return true;
-
 	/*
 	 * Events on POWER7 can roll back if a speculative event doesn't
 	 * eventually complete. Unfortunately in some rare cases they will
@@ -1428,7 +1425,15 @@ static bool pmc_overflow(unsigned long val)
 	 * PMCs because a user might set a period of less than 256 and we
 	 * don't want to mistakenly reset them.
 	 */
-	if (pvr_version_is(PVR_POWER7) && ((0x80000000 - val) <= 256))
+	if ((0x80000000 - val) <= 256)
+		return true;
+
+	return false;
+}
+
+static bool pmc_overflow(unsigned long val)
+{
+	if ((int)val < 0)
 		return true;
 
 	return false;
@@ -1439,11 +1444,11 @@ static bool pmc_overflow(unsigned long val)
  */
 static void perf_event_interrupt(struct pt_regs *regs)
 {
-	int i;
+	int i, j;
 	struct cpu_hw_events *cpuhw = &__get_cpu_var(cpu_hw_events);
 	struct perf_event *event;
-	unsigned long val;
-	int found = 0;
+	unsigned long val[8];
+	int found, active;
 	int nmi;
 
 	if (cpuhw->n_limited)
@@ -1458,33 +1463,53 @@ static void perf_event_interrupt(struct pt_regs *regs)
 	else
 		irq_enter();
 
-	for (i = 0; i < cpuhw->n_events; ++i) {
-		event = cpuhw->event[i];
-		if (!event->hw.idx || is_limited_pmc(event->hw.idx))
+	/* Read all the PMCs since we'll need them a bunch of times */
+	for (i = 0; i < ppmu->n_counter; ++i)
+		val[i] = read_pmc(i + 1);
+
+	/* Try to find what caused the IRQ */
+	found = 0;
+	for (i = 0; i < ppmu->n_counter; ++i) {
+		if (!pmc_overflow(val[i]))
 			continue;
-		val = read_pmc(event->hw.idx);
-		if ((int)val < 0) {
-			/* event has overflowed */
-			found = 1;
-			record_and_restart(event, val, regs);
+		if (is_limited_pmc(i + 1))
+			continue; /* these won't generate IRQs */
+		/*
+		 * We've found one that's overflowed.  For active
+		 * counters we need to log this.  For inactive
+		 * counters, we need to reset it anyway
+		 */
+		found = 1;
+		active = 0;
+		for (j = 0; j < cpuhw->n_events; ++j) {
+			event = cpuhw->event[j];
+			if (event->hw.idx == (i + 1)) {
+				active = 1;
+				record_and_restart(event, val[i], regs);
+				break;
+			}
 		}
+		if (!active)
+			/* reset non active counters that have overflowed */
+			write_pmc(i + 1, 0);
 	}
-
-	/*
-	 * In case we didn't find and reset the event that caused
-	 * the interrupt, scan all events and reset any that are
-	 * negative, to avoid getting continual interrupts.
-	 * Any that we processed in the previous loop will not be negative.
-	 */
-	if (!found) {
-		for (i = 0; i < ppmu->n_counter; ++i) {
-			if (is_limited_pmc(i + 1))
+	if (!found && pvr_version_is(PVR_POWER7)) {
+		/* check active counters for special buggy p7 overflow */
+		for (i = 0; i < cpuhw->n_events; ++i) {
+			event = cpuhw->event[i];
+			if (!event->hw.idx || is_limited_pmc(event->hw.idx))
 				continue;
-			val = read_pmc(i + 1);
-			if (pmc_overflow(val))
-				write_pmc(i + 1, 0);
+			if (pmc_overflow_power7(val[event->hw.idx - 1])) {
+				/* event has overflowed in a buggy way*/
+				found = 1;
+				record_and_restart(event,
+						   val[event->hw.idx - 1],
+						   regs);
+			}
 		}
 	}
+	if ((!found) && printk_ratelimit())
+		printk(KERN_WARNING "Can't find PMC that caused IRQ\n");
 
 	/*
 	 * Reset MMCR0 to its normal value.  This will set PMXE and
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] powerpc/perf: Fix finding overflowed PMC in interrupt
  2012-11-06  1:53       ` Michael Neuling
@ 2012-11-06  6:47         ` Anshuman Khandual
  2012-11-06 10:19           ` Michael Neuling
  0 siblings, 1 reply; 11+ messages in thread
From: Anshuman Khandual @ 2012-11-06  6:47 UTC (permalink / raw)
  To: Michael Neuling
  Cc: Sukadev Bhattiprolu, Paul Mackerras, Anton Blanchard, Linux PPC dev

On 11/06/2012 07:23 AM, Michael Neuling wrote:

> +	if (!found && pvr_version_is(PVR_POWER7)) {
> +		/* check active counters for special buggy p7 overflow */
> +		for (i = 0; i < cpuhw->n_events; ++i) {
> +			event = cpuhw->event[i];
> +			if (!event->hw.idx || is_limited_pmc(event->hw.idx))
>  				continue;
> -			val = read_pmc(i + 1);
> -			if (pmc_overflow(val))
> -				write_pmc(i + 1, 0);
> +			if (pmc_overflow_power7(val[event->hw.idx - 1])) {


I have couple of questions. 

Can the buggy overflow happen on any of the available counters PMC1-PMC4 ?
Will this approach never reset an actual user defined event (with sample period < 256) ?
Is this related to the counter or the event which it is counting ? Just wondering if we
have to do something more than checking for the count < 256. Just a thought.

Regards
Anshuman

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] powerpc/perf: Fix finding overflowed PMC in interrupt
  2012-11-06  6:47         ` Anshuman Khandual
@ 2012-11-06 10:19           ` Michael Neuling
  2012-11-06 10:42             ` Anshuman Khandual
  0 siblings, 1 reply; 11+ messages in thread
From: Michael Neuling @ 2012-11-06 10:19 UTC (permalink / raw)
  To: Anshuman Khandual
  Cc: Sukadev Bhattiprolu, Paul Mackerras, Anton Blanchard, Linux PPC dev

Anshuman Khandual <khandual@linux.vnet.ibm.com> wrote:

> On 11/06/2012 07:23 AM, Michael Neuling wrote:
> 
> > +	if (!found && pvr_version_is(PVR_POWER7)) {
> > +		/* check active counters for special buggy p7 overflow */
> > +		for (i = 0; i < cpuhw->n_events; ++i) {
> > +			event = cpuhw->event[i];
> > +			if (!event->hw.idx || is_limited_pmc(event->hw.idx))
> >  				continue;
> > -			val = read_pmc(i + 1);
> > -			if (pmc_overflow(val))
> > -				write_pmc(i + 1, 0);
> > +			if (pmc_overflow_power7(val[event->hw.idx - 1])) {
> 
> 
> I have couple of questions. 
> 
> Can the buggy overflow happen on any of the available counters PMC1-PMC4 ?

No.  It's limited to certain events and I believe it can only happen on
PMC2 and 4.  This code doesn't bother trying to make this distinction
though.

> Will this approach never reset an actual user defined event (with
> sample period < 256) ? Is this related to the counter or the event
> which it is counting ? Just wondering if we have to do something more
> than checking for the count < 256. Just a thought.


I don't understand what you mean by these questions.  Can you explain a
bit more?

Mikey

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] powerpc/perf: Fix finding overflowed PMC in interrupt
  2012-11-06 10:19           ` Michael Neuling
@ 2012-11-06 10:42             ` Anshuman Khandual
  0 siblings, 0 replies; 11+ messages in thread
From: Anshuman Khandual @ 2012-11-06 10:42 UTC (permalink / raw)
  To: Michael Neuling
  Cc: Linux PPC dev, Sukadev Bhattiprolu, Paul Mackerras, Anton Blanchard

On 11/06/2012 03:49 PM, Michael Neuling wrote:
>>
>>
>> I have couple of questions. 
>>
>> Can the buggy overflow happen on any of the available counters PMC1-PMC4 ?
> 
> No.  It's limited to certain events and I believe it can only happen on
> PMC2 and 4.  This code doesn't bother trying to make this distinction
> though.
> 
>> Will this approach never reset an actual user defined event (with
>> sample period < 256) ? Is this related to the counter or the event
>> which it is counting ? Just wondering if we have to do something more
>> than checking for the count < 256. Just a thought.
> 
> 
> I don't understand what you mean by these questions.  Can you explain a
> bit more?
> 

Thats fine. The previous answer explains it well. Thanks !

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] powerpc/perf: Fix finding overflowed PMC in interrupt
  2012-11-06  1:08   ` [PATCH 1/2] powerpc/perf: Fix finding overflowed PMC in interrupt Michael Neuling
  2012-11-06  1:08     ` [PATCH 2/2] powerpc/perf: Fix for PMCs not making progress Michael Neuling
  2012-11-06  1:25     ` [PATCH 1/2] powerpc/perf: Fix finding overflowed PMC in interrupt Anton Blanchard
@ 2012-12-22  1:07     ` Sukadev Bhattiprolu
  2 siblings, 0 replies; 11+ messages in thread
From: Sukadev Bhattiprolu @ 2012-12-22  1:07 UTC (permalink / raw)
  To: Michael Neuling
  Cc: Linux PPC dev, Paul Mackerras, Anton Blanchard, Benjamin Herrenschmidt

Ben

Will these two patches be pushed upstream or are they waiting for
review/test ?

They fix a hang that we get with some perf events.

Thanks,

Sukadev


Michael Neuling [mikey@neuling.org] wrote:
| If a PMC is about to overflow on a counter that's on an active perf event
| (ie. less than 256 from the end) and a _different_ PMC overflows just at this
| time (a PMC that's not on an active perf event), we currently mark the event as
| found, but in reality it's not as it's likely the other PMC that caused the
| IRQ.  Since we mark it as found the second catch all for overflows doesn't run,
| and we don't reset the overflowing PMC ever.  Hence we keep hitting that same
| PMC IRQ over and over and don't reset the actual overflowing counter.
| 
| This is a rewrite of the perf interrupt handler for book3s to get around this.
| We now check to see if any of the PMCs have actually overflowed (ie >=
| 0x80000000).  If yes, record it for active counters and just reset it for
| inactive counters.  If it's not overflowed, then we check to see if it's one of
| the buggy power7 counters and if it is, record it and continue.  If none of the
| PMCs match this, then we make note that we couldn't find the PMC that caused
| the IRQ.
| 
| Signed-off-by: Michael Neuling <mikey@neuling.org>
| Reviewed-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
| cc: Paul Mackerras <paulus@samba.org>
| cc: Anton Blanchard <anton@samba.org>
| cc: Linux PPC dev <linuxppc-dev@ozlabs.org>
| ---
|  arch/powerpc/perf/core-book3s.c |   83 +++++++++++++++++++++++++--------------
|  1 file changed, 54 insertions(+), 29 deletions(-)
| 
| diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
| index aa2465e..3575def 100644
| --- a/arch/powerpc/perf/core-book3s.c
| +++ b/arch/powerpc/perf/core-book3s.c
| @@ -1412,11 +1412,8 @@ unsigned long perf_instruction_pointer(struct pt_regs *regs)
|  		return regs->nip;
|  }
| 
| -static bool pmc_overflow(unsigned long val)
| +static bool pmc_overflow_power7(unsigned long val)
|  {
| -	if ((int)val < 0)
| -		return true;
| -
|  	/*
|  	 * Events on POWER7 can roll back if a speculative event doesn't
|  	 * eventually complete. Unfortunately in some rare cases they will
| @@ -1428,7 +1425,15 @@ static bool pmc_overflow(unsigned long val)
|  	 * PMCs because a user might set a period of less than 256 and we
|  	 * don't want to mistakenly reset them.
|  	 */
| -	if (pvr_version_is(PVR_POWER7) && ((0x80000000 - val) <= 256))
| +	if ((0x80000000 - val) <= 256)
| +		return true;
| +
| +	return false;
| +}
| +
| +static bool pmc_overflow(unsigned long val)
| +{
| +	if ((int)val < 0)
|  		return true;
| 
|  	return false;
| @@ -1439,11 +1444,11 @@ static bool pmc_overflow(unsigned long val)
|   */
|  static void perf_event_interrupt(struct pt_regs *regs)
|  {
| -	int i;
| +	int i, j;
|  	struct cpu_hw_events *cpuhw = &__get_cpu_var(cpu_hw_events);
|  	struct perf_event *event;
| -	unsigned long val;
| -	int found = 0;
| +	unsigned long val[8];
| +	int found, active;
|  	int nmi;
| 
|  	if (cpuhw->n_limited)
| @@ -1458,33 +1463,53 @@ static void perf_event_interrupt(struct pt_regs *regs)
|  	else
|  		irq_enter();
| 
| -	for (i = 0; i < cpuhw->n_events; ++i) {
| -		event = cpuhw->event[i];
| -		if (!event->hw.idx || is_limited_pmc(event->hw.idx))
| +	/* Read all the PMCs since we'll need them a bunch of times */
| +	for (i = 0; i < ppmu->n_counter; ++i)
| +		val[i] = read_pmc(i + 1);
| +
| +	/* Try to find what caused the IRQ */
| +	found = 0;
| +	for (i = 0; i < ppmu->n_counter; ++i) {
| +		if (!pmc_overflow(val[i]))
|  			continue;
| -		val = read_pmc(event->hw.idx);
| -		if ((int)val < 0) {
| -			/* event has overflowed */
| -			found = 1;
| -			record_and_restart(event, val, regs);
| +		if (is_limited_pmc(i + 1))
| +			continue; /* these won't generate IRQs */
| +		/*
| +		 * We've found one that's overflowed.  For active
| +		 * counters we need to log this.  For inactive
| +		 * counters, we need to reset it anyway
| +		 */
| +		found = 1;
| +		active = 0;
| +		for (j = 0; j < cpuhw->n_events; ++j) {
| +			event = cpuhw->event[j];
| +			if (event->hw.idx == (i + 1)) {
| +				active = 1;
| +				record_and_restart(event, val[i], regs);
| +				break;
| +			}
|  		}
| +		if (!active)
| +			/* reset non active counters that have overflowed */
| +			write_pmc(i + 1, 0);
|  	}
| -
| -	/*
| -	 * In case we didn't find and reset the event that caused
| -	 * the interrupt, scan all events and reset any that are
| -	 * negative, to avoid getting continual interrupts.
| -	 * Any that we processed in the previous loop will not be negative.
| -	 */
| -	if (!found) {
| -		for (i = 0; i < ppmu->n_counter; ++i) {
| -			if (is_limited_pmc(i + 1))
| +	if (!found && pvr_version_is(PVR_POWER7)) {
| +		/* check active counters for special buggy p7 overflow */
| +		for (i = 0; i < cpuhw->n_events; ++i) {
| +			event = cpuhw->event[i];
| +			if (!event->hw.idx || is_limited_pmc(event->hw.idx))
|  				continue;
| -			val = read_pmc(i + 1);
| -			if (pmc_overflow(val))
| -				write_pmc(i + 1, 0);
| +			if (pmc_overflow_power7(val[event->hw.idx - 1])) {
| +				/* event has overflowed in a buggy way*/
| +				found = 1;
| +				record_and_restart(event,
| +						   val[event->hw.idx - 1],
| +						   regs);
| +			}
|  		}
|  	}
| +	if (!found)
| +		printk(KERN_WARNING "Can't find PMC that caused IRQ\n");
| 
|  	/*
|  	 * Reset MMCR0 to its normal value.  This will set PMXE and
| -- 
| 1.7.9.5

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2012-12-22  1:08 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-08-08  1:07 [PATCH] Use pmc_overflow() to detect rolled back events Sukadev Bhattiprolu
2012-09-21  0:38 ` Sukadev Bhattiprolu
2012-11-06  1:08   ` [PATCH 1/2] powerpc/perf: Fix finding overflowed PMC in interrupt Michael Neuling
2012-11-06  1:08     ` [PATCH 2/2] powerpc/perf: Fix for PMCs not making progress Michael Neuling
2012-11-06  1:25     ` [PATCH 1/2] powerpc/perf: Fix finding overflowed PMC in interrupt Anton Blanchard
2012-11-06  1:53       ` Michael Neuling
2012-11-06  1:53       ` Michael Neuling
2012-11-06  6:47         ` Anshuman Khandual
2012-11-06 10:19           ` Michael Neuling
2012-11-06 10:42             ` Anshuman Khandual
2012-12-22  1:07     ` Sukadev Bhattiprolu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.