ARM: perf: ensure overflows aren't missed due to IRQ latency

PATCH New Will Deacon 3 participants 4 messages Mar 23, 2011 1 message not in the archive

Subsystems (3)

Arm (32-bit) perf events Arm PMU profiling and debugging

lore

Share & tools

lore

Will Deacon

author

2011-03-23 15:52 UTC #0 lore raw

PATCH 1/1 ARM: perf: ensure overflows aren't missed due to IRQ latency

If a counter overflows during a perf stat profiling run it may overtake the last known value of the counter:

    0        prev     new                0xffffffff
    |----------|-------|----------------------|

In this case, the number of events that have occurred is (0xffffffff - prev) + new. Unfortunately, the event update code will not realise an overflow has occurred and will instead report the event delta as (new - prev) which may be considerably smaller than the real count.

This patch adds an extra argument to armpmu_event_update which indicates whether or not an overflow has occurred. If an overflow has occurred and the new count value is greater than the previous one, then we add on the remaining event period (which must have expired for the overflow to occur) and the previous count value.

Cc Jamie Iles <[email protected]>

Reported-by Ashwin Chaugule <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---

Jamie, I've changed quite a bit of the delta calculation code in armpmu_event_update so that we can detect the problem above.

Please let me know if I've missed/broken anything!

Will

 arch/arm/kernel/perf_event.c        |   19 +++++++++++--------
 arch/arm/kernel/perf_event_v6.c     |    2 +-
 arch/arm/kernel/perf_event_v7.c     |    2 +-
 arch/arm/kernel/perf_event_xscale.c |    4 ++--
 4 files changed, 15 insertions(+), 12 deletions(-)

arch/arm/kernel/perf_event.c +11 -8

index d150ad1..53f6068 100644
@@ -204,11 +204,9 @@ armpmu_event_set_period(struct perf_event *event,
 static u64
 armpmu_event_update(struct perf_event *event,
 		    struct hw_perf_event *hwc,
-		    int idx)
+		    int idx, int overflow)
 {
-	int shift = 64 - 32;
-	s64 prev_raw_count, new_raw_count;
-	u64 delta;
+	u64 delta, prev_raw_count, new_raw_count;
 
 again:
 	prev_raw_count = local64_read(&hwc->prev_count);

@@ -218,8 +216,13 @@ again:
 			     new_raw_count) != prev_raw_count)
 		goto again;
 
-	delta = (new_raw_count << shift) - (prev_raw_count << shift);
-	delta >>= shift;
+	new_raw_count &= armpmu->max_period;
+	prev_raw_count &= armpmu->max_period;
+
+	delta = (new_raw_count - prev_raw_count);
+
+	if (overflow && new_raw_count > prev_raw_count)
+		delta += local64_read(&hwc->period_left) + prev_raw_count;
 
 	local64_add(delta, &event->count);
 	local64_sub(delta, &hwc->period_left);

@@ -236,7 +239,7 @@ armpmu_read(struct perf_event *event)
 	if (hwc->idx < 0)
 		return;
 
-	armpmu_event_update(event, hwc, hwc->idx);
+	armpmu_event_update(event, hwc, hwc->idx, 0);
 }
 
 static void

@@ -254,7 +257,7 @@ armpmu_stop(struct perf_event *event, int flags)
 	if (!(hwc->state & PERF_HES_STOPPED)) {
 		armpmu->disable(hwc, hwc->idx);
 		barrier(); /* why? */
-		armpmu_event_update(event, hwc, hwc->idx);
+		armpmu_event_update(event, hwc, hwc->idx, 0);
 		hwc->state |= PERF_HES_STOPPED | PERF_HES_UPTODATE;
 	}
 }

arch/arm/kernel/perf_event_v6.c +1 -1

index c058bfc..66ce900 100644
@@ -474,7 +474,7 @@ armv6pmu_handle_irq(int irq_num,
 			continue;
 
 		hwc = &event->hw;
-		armpmu_event_update(event, hwc, idx);
+		armpmu_event_update(event, hwc, idx, 1);
 		data.period = event->hw.last_period;
 		if (!armpmu_event_set_period(event, hwc, idx))
 			continue;

arch/arm/kernel/perf_event_v7.c +1 -1

index 2e14025..e144b8c 100644
@@ -780,7 +780,7 @@ static irqreturn_t armv7pmu_handle_irq(int irq_num, void *dev)
 			continue;
 
 		hwc = &event->hw;
-		armpmu_event_update(event, hwc, idx);
+		armpmu_event_update(event, hwc, idx, 1);
 		data.period = event->hw.last_period;
 		if (!armpmu_event_set_period(event, hwc, idx))
 			continue;

arch/arm/kernel/perf_event_xscale.c +2 -2

index 28cd3b0..39affbe 100644
@@ -246,7 +246,7 @@ xscale1pmu_handle_irq(int irq_num, void *dev)
 			continue;
 
 		hwc = &event->hw;
-		armpmu_event_update(event, hwc, idx);
+		armpmu_event_update(event, hwc, idx, 1);
 		data.period = event->hw.last_period;
 		if (!armpmu_event_set_period(event, hwc, idx))
 			continue;

@@ -578,7 +578,7 @@ xscale2pmu_handle_irq(int irq_num, void *dev)
 			continue;
 
 		hwc = &event->hw;
-		armpmu_event_update(event, hwc, idx);
+		armpmu_event_update(event, hwc, idx, 1);
 		data.period = event->hw.last_period;
 		if (!armpmu_event_set_period(event, hwc, idx))
 			continue;

Signature · 1 line

1.7.0.4

Jamie Iles

2011-03-23 18:13 UTC #1 lore raw

PATCH 1/1 ARM: perf: ensure overflows aren't missed due to IRQ latency

Hi Will,

Quoted 60 lines · Will Deacon

If a counter overflows during a perf stat profiling run it may overtake the last known value of the counter:
    0        prev     new                0xffffffff
    |----------|-------|----------------------|
In this case, the number of events that have occurred is (0xffffffff - prev) + new. Unfortunately, the event update code will not realise an overflow has occurred and will instead report the event delta as (new - prev) which may be considerably smaller than the real count.
This patch adds an extra argument to armpmu_event_update which indicates whether or not an overflow has occurred. If an overflow has occurred and the new count value is greater than the previous one, then we add on the remaining event period (which must have expired for the overflow to occur) and the previous count value.
Cc Jamie Iles <[email protected]>
Reported-by Ashwin Chaugule <[email protected]>
Signed-off-by Will Deacon <[email protected]>
---
Jamie, I've changed quite a bit of the delta calculation code in armpmu_event_update so that we can detect the problem above.
Please let me know if I've missed/broken anything!
Will
 arch/arm/kernel/perf_event.c        |   19 +++++++++++--------
 arch/arm/kernel/perf_event_v6.c     |    2 +-
 arch/arm/kernel/perf_event_v7.c     |    2 +-
 arch/arm/kernel/perf_event_xscale.c |    4 ++--
 4 files changed, 15 insertions(+), 12 deletions(-)
arch/arm/kernel/perf_event.c +2 -6
index d150ad1..53f6068 100644
@@ -204,11 +204,9 @@ armpmu_event_set_period(struct perf_event *event,
 static u64
 armpmu_event_update(struct perf_event *event,
 		    struct hw_perf_event *hwc,
-		    int idx)
+		    int idx, int overflow)
 {
-	int shift = 64 - 32;
-	s64 prev_raw_count, new_raw_count;
-	u64 delta;
+	u64 delta, prev_raw_count, new_raw_count;
 
 again:
 	prev_raw_count = local64_read(&hwc->prev_count);
@@ -218,8 +216,13 @@ again:
 			     new_raw_count) != prev_raw_count)
 		goto again;
 
-	delta = (new_raw_count << shift) - (prev_raw_count << shift);
-	delta >>= shift;

arch/arm/kernel/perf_event.c again: · line 228

Quoted 10 lines

arch/arm/kernel/perf_event.c +7 -0

+	new_raw_count &= armpmu->max_period;
+	prev_raw_count &= armpmu->max_period;
+
+	delta = (new_raw_count - prev_raw_count);
+
+	if (overflow && new_raw_count > prev_raw_count)
+		delta += local64_read(&hwc->period_left) + prev_raw_count;
 
 	local64_add(delta, &event->count);
 	local64_sub(delta, &hwc->period_left);

Hmm, I'm not really sure I follow that and see how it works for an overflow when new < prev. Why doesn't the naive:

	new_raw_count &= armpmu->max_period;
	prev_raw_count &= armpmu->max_period;
	if (overflow)
		delta = (armpmu->max_period - prev_raw_count) + 
			new_raw_count
	else
		delta = new_raw_count - prev_raw_count;

work? I'm sure I'm missing something here!

Jamie

A message that is not in the archive (sent off-list or not yet received): [email protected]

Ashwin Chaugule

2011-03-23 18:35 UTC #3 lore raw

PATCH 1/1 ARM: perf: ensure overflows aren't missed due to IRQ latency

Quoted 11 lines · Jamie Iles

Hmm, I'm not really sure I follow that and see how it works for an overflow when new < prev. Why doesn't the naive:

       new_raw_count &= armpmu->max_period;
       prev_raw_count &= armpmu->max_period;
       if (overflow)
               delta = (armpmu->max_period - prev_raw_count) +
                       new_raw_count
       else
               delta = new_raw_count - prev_raw_count;

Quoted 3 lines

work? I'm sure I'm missing something here!
Jamie

When new < prev, delta will be greater than 2^32. So we'll have accounted for the overflow condition anyway.

The naive solution you mention above is what we discussed earlier and works too. I guess the additional:

Quoted 2 lines

       new_raw_count &= armpmu->max_period;
       prev_raw_count &= armpmu->max_period;

just makes it cleaner than the shifting to upper 32bits, subtracting and then shifting back again.

Cheers,
Ashwin

Signature · 2 lines

Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Will Deacon

author

2011-03-23 18:44 UTC #4 lore raw

PATCH 1/1 ARM: perf: ensure overflows aren't missed due to IRQ latency

Quoted 12 lines · Jamie Iles

Quoted 11 lines
Hmm, I'm not really sure I follow that and see how it works for an overflow when new < prev. Why doesn't the naive:
       new_raw_count &= armpmu->max_period;
       prev_raw_count &= armpmu->max_period;
       if (overflow)
               delta = (armpmu->max_period - prev_raw_count) +
                       new_raw_count
       else
               delta = new_raw_count - prev_raw_count;

Quoted 8 lines

Quoted 3 lines
work? I'm sure I'm missing something here!
Jamie
When new < prev, delta will be greater than 2^32. So we'll have accounted for the overflow condition anyway.
The naive solution you mention above is what we discussed earlier and works too.

Yup, they both work but I think Jamie's version is easier to read so I'll roll another patch...

Quoted 6 lines

I guess the additional:
Quoted 2 lines
       new_raw_count &= armpmu->max_period;
       prev_raw_count &= armpmu->max_period;
just makes it cleaner than the shifting to upper 32bits, subtracting and then shifting back again.

... but I'm having second thoughts about using max_period as the mask. If the period was anything other than 0xffffffff then it wouldn't work. I'll add another field to the PMU structure in case somebody in the future has counters with strange period constraints.

Will