ARM: perf: ensure overflows aren't missed due to IRQ latency
Subsystems (3)
PATCH v2 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 then we use the maximum period of the counter to calculate the elapsed events.
---
Updated following feedback from Jamie so that the code is easier to read and follows the commit log more closely in its implementation.
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
@@ -205,11 +205,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);
@@ -219,8 +217,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; + +if (overflow) + delta = armpmu->max_period - prev_raw_count + new_raw_count; +else + delta = new_raw_count - prev_raw_count;local64_add (delta, &event->count);local64_sub (delta, &hwc->period_left);
@@ -237,7 +240,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
@@ -255,7 +258,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
@@ -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
@@ -782,7 +782,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
@@ -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
PATCH v2 1/1 ARM: perf: ensure overflows aren't missed due to IRQ latency
Hi Will,
Quoted 12 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.
Quoted 8 lines
This patch adds an extra argument to
armpmu_event_updatewhich indicates whether or not an overflow has occurred. If an overflow has occurred then we use the maximum period of the counter to calculate the elapsed events.Cc Jamie Iles <[email protected]>Reported-by Ashwin Chaugule <[email protected]>Signed-off-by Will Deacon <[email protected]>
Looks nice!
Jamie
PATCH v2 1/1 ARM: perf: ensure overflows aren't missed due to IRQ latency
In reply to Will DeaconHi,
Sorry, my first attempt was regarded as suspicious. This time I shall try to avoid HTML.
A couple of nitpicks in the introduction.
Le 24/03/2011 18:07, Will Deacon a ?crit :
Quoted 11 lines
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.
a) "(0xffffffff - prev) + new"
Surely that should be "(0x100000000 - prev) + new"
b) "may be considerably smaller"
I suggest either:
"will be 0x100000000 smaller (per overflow)"
or at least:
"will be considerably smaller"
That said, something like 2^32 would be more easily readable than 0x100000000.
Cheers,
Chris
PATCH v2 1/1 ARM: perf: ensure overflows aren't missed due to IRQ latency
Quoted 1 line
Hi,
Hello Chris,
Quoted 2 lines
Sorry, my first attempt was regarded as suspicious. This time I shall try to avoid HTML.
Good idea :)
Quoted 9 lines
A couple of nitpicks in the introduction.
Le 24/03/2011 18:07, Will Deacon a ?crit :
Quoted 6 lines
If a counter overflows during a perf stat profiling run it may overtake the last known value of the counter:
0 prev new 0xffffffff |----------|-------|----------------------|
Quoted 8 lines
Quoted 5 lines
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.a) "(
0xffffffff- prev) + new"
Surely that should be "(0x100000000- prev) + new"
This has been merged now, so it's too late to change the commit log. However, I think this is also a problem in the code in that we're not counting the event through zero (Jamie - it means my original code *was* subtly different to yours, but I can't pretend I saw this issue!).
So now we're off by 1 rather than some number bounded by maxuint. At least we're converging on accuracy...
I'll post a fix.
Will