ARM: perf: remove erroneous check on active_events
Subsystems (3)
Mark Rutland
maintainerPATCH 1/1 ARM: perf: remove erroneous check on active_events
When initialising a PMU, there is a check to protect against races with other CPUs filling all of the available event slots. Since armpmu_add checks that an event can be scheduled, we do not need to do this at initialisation time. Furthermore the current code is broken because it assumes that atomic_inc_not_zero will unconditionally increment active_counts and then tries to decrement it again on failure.
This patch removes the broken, redundant code.
--- arch/arm/kernel/perf_event.c | 5 ----- 1 files changed, 0 insertions(+), 5 deletions(-)
arch/arm/kernel/perf_event.c +0 -5
@@ -560,11 +560,6 @@ static int armpmu_event_init(struct perf_event *event) event->destroy = hw_perf_event_destroy;if (!atomic_inc_not_zero (&active_events)) { -if (atomic_read (&active_events) > armpmu->num_events) { -atomic_dec (&active_events); -return -ENOSPC ; - } -mutex_lock (&pmu_reserve_mutex);if (atomic_read (&active_events) ==0 ) { err =armpmu_reserve_hardware ();
Signature · 1 line
1.7.0.4
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH 1/1 ARM: perf: remove erroneous check on active_events
Hi Mark,
Quoted 10 lines
From: Mark Rutland <[email protected]>
When initialising a PMU, there is a check to protect against races with other CPUs filling all of the available event slots. Since
armpmu_addchecks that an event can be scheduled, we do not need to do this at initialisation time. Furthermore the current code is broken because it assumes thatatomic_inc_not_zerowill unconditionally incrementactive_countsand then tries to decrement it again on failure.This patch removes the broken, redundant code.
Nice find ! I had a slightly different solution.
Quoted 16 lines
Signed-off-by Mark Rutland <[email protected]>Acked-by Will Deacon <[email protected]>Cc Will Deacon <[email protected]>Cc Jamie Iles <[email protected]>--- arch/arm/kernel/perf_event.c | 5 ----- 1 files changed, 0 insertions(+), 5 deletions(-)arch/arm/kernel/perf_event.c +0
-0@@ -560,11 +560,6 @@ static int armpmu_event_init(struct perf_event *event) event->destroy = hw_perf_event_destroy;
arch/arm/kernel/perf_event.c static int armpmu_event_init(struct perf_event *event)
Quoted 10 lines
arch/arm/kernel/perf_event.c +0
-6if (!atomic_inc_not_zero (&active_events)) { -if (atomic_read (&active_events) > armpmu->num_events) { -atomic_dec (&active_events); -return -ENOSPC ; - } -mutex_lock (&pmu_reserve_mutex);if (atomic_read (&active_events) ==0 ) { err =armpmu_reserve_hardware (); --
While its true that we check if we can schedule an event in add(), I think we should check it here, because, if we don't have space on the PMU, returning -ENOSPC will not "allocate" a perf_event instance.
Whereas, if we skip this check in the init function, the perf_event instance will be allocated but may or may not be "armed" on the pmu in add(). This is fine, except that perf-core code "rotates" the linked list of active events. When an event is allocated but can't be "armed" on the PMU, the perf-core normalizes its output. This shows up as (scaled by) against the output of the event in perf-stat.
In practice, I've seen that when this happens, the output of the event has anywhere between 50-80% std deviation. Many users seem to not like this. So I think we should enforce support for only as many events as there are counters in the init function, and return -ENOSPC otherwise.
If the event allocation fails, it'll show up as <not-counted>, which I think is better than using an output with very high std dev.
How about something like this:-
if (!atomic_inc_not_zero(&active_events)) {
mutex_lock
err = armpmu_reserve_hardware();
mutex_unlock
if (!err)
atomic_inc(&active_events);
..
} else
if(atomic_read(&active_events) > armpmu->num_events)
atomic_dec(&active_events)
return -ENOSPC;
}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.
PATCH 1/1 ARM: perf: remove erroneous check on active_events
In reply to Mark Rutlandarch/arm/kernel/perf_event.c static int armpmu_event_init(struct perf_event *event) · line 564
Quoted 10 lines · Mark Rutland
arch/arm/kernel/perf_event.c +0
-2@@ -560,11 +560,6 @@ static int armpmu_event_init(struct perf_event *event) event->destroy = hw_perf_event_destroy;if (!atomic_inc_not_zero (&active_events)) { -if (atomic_read (&active_events) > armpmu->num_events) { -atomic_dec (&active_events);
Yuck. This is a good example of atomic_* abuse. The above does nothing to deal with the situation where two threads are running thusly:
CPU0 CPU1atomic_inc_not_zero(&active_events)
atomic_inc_not_zero(&active_events)atomic_read(&active_events)
atomic_read(&active_events)atomic_dec(&active_events)
atomic_dec(&active_events)
return -ENOSPC
return -ENOSPC
when one of those two should have succeeded. I do wish people would get out of the habbit of using atomic variables - they seem to be ripe for this kind of abuse.
The only time that atomic variables are atomic is for each individual _operation_ on the type, not across several operations, and certainly not atomic_read() or atomic_set().
What it really wants to be is a spinlock or mutex so that it can do something like this:
int err = 0;
mutex_lock(&active_events_lock);
if (++active_events > armpmu->num_events) {
active_events--;
err = -ENOSPC;
}
mutex_unlock(&active_events);
if (err)
return err;which gives well defined semantics, and ensures that when two CPUs are racing, one will at least succeed.
PATCH 1/1 ARM: perf: remove erroneous check on active_events
Quoted 25 lines · Russell King - ARM Linux
arch/arm/kernel/perf_event.c static int armpmu_event_init(struct perf_event *event) · line 564
Quoted 10 lines · Mark Rutland
arch/arm/kernel/perf_event.c +0
-2@@ -560,11 +560,6 @@ static int armpmu_event_init(struct perf_event *event) event->destroy = hw_perf_event_destroy;if (!atomic_inc_not_zero (&active_events)) { -if (atomic_read (&active_events) > armpmu->num_events) { -atomic_dec (&active_events);Yuck. This is a good example of atomic_* abuse. The above does nothing to deal with the situation where two threads are running thusly:
CPU0 CPU1
atomic_inc_not_zero(&active_events)
atomic_inc_not_zero(&active_events)atomic_read(&active_events)
atomic_read(&active_events)atomic_dec(&active_events)
atomic_dec(&active_events)
return-ENOSPC
return-ENOSPC
Quoted 3 lines
when one of those two should have succeeded. I do wish people would get out of the habbit of using atomic variables - they seem to be ripe for this kind of abuse.
Yup, I messed up there. How about the patch below that eliminates the atomic_t's entirely? This means that we always lock the mutex in event destruction rather than just for the last one but that seems acceptable to me.
The MIPS perf events code is based on this so could do with the same change.
Jamie
8<---
arch/arm/kernel/perf_event.c +14 -18
@@ -458,16 +458,16 @@ armpmu_release_hardware(void) pmu_device =NULL ; } -static atomic_t active_events =ATOMIC_INIT (0 ); -static DEFINE_MUTEX (pmu_reserve_mutex); +static int active_events; +static DEFINE_MUTEX (active_event_lock);static void hw_perf_event_destroy (struct perf_event *event) { -if (atomic_dec_and_mutex_lock (&active_events, &pmu_reserve_mutex)) { +mutex_lock (&active_event_lock); +if (--active_events ==0 )armpmu_release_hardware (); -mutex_unlock (&pmu_reserve_mutex); - } +mutex_unlock (&active_event_lock); } static int
@@ -559,22 +559,18 @@ static int armpmu_event_init(struct perf_event *event) event->destroy = hw_perf_event_destroy; -if (!atomic_inc_not_zero (&active_events)) { -if (atomic_read (&active_events) > armpmu->num_events) { -atomic_dec (&active_events); -return -ENOSPC ; - } +mutex_lock (&active_event_lock); -mutex_lock (&pmu_reserve_mutex); -if (atomic_read (&active_events) ==0 ) { - err =armpmu_reserve_hardware (); - } - -if (!err) -atomic_inc (&active_events); -mutex_unlock (&pmu_reserve_mutex); +if (++active_events > armpmu->num_events) { + --active_events; + err = -ENOSPC ; } +if (!err && active_events ==1 ) + err =armpmu_reserve_hardware (); + +mutex_unlock (&active_event_lock); +if (err)return err;
PATCH 1/1 ARM: perf: remove erroneous check on active_events
Hi guys [adding Peter Z],
Quoted 9 lines · Jamie Iles
Quoted 8 lines · Russell King - ARM Linux
Quoted 7 lines · Mark Rutland
arch/arm/kernel/perf_event.c +0
-0@@ -560,11 +560,6 @@ static int armpmu_event_init(struct perf_event *event) event->destroy = hw_perf_event_destroy;
Quoted 9 lines
Quoted 9 lines
arch/arm/kernel/perf_event.c static int armpmu_event_init(struct perf_event *event) · line 564
Quoted 3 lines
arch/arm/kernel/perf_event.c +0
-2if (!atomic_inc_not_zero (&active_events)) { -if (atomic_read (&active_events) > armpmu->num_events) { -atomic_dec (&active_events);Yuck. This is a good example of atomic_* abuse. The above does nothing to deal with the situation where two threads are running thusly:
CPU0 CPU1atomic_inc_not_zero(&active_events)
atomic_inc_not_zero(&active_events)
Agreed. Mark's patch is deleting the offending code though, so that's a plus.
Quoted 7 lines
Yup, I messed up there. How about the patch below that eliminates the atomic_t's entirely? This means that we always lock the mutex in event destruction rather than just for the last one but that seems acceptable to me.
The MIPS perf events code is based on this so could do with the same change.
Does the x86 code also need updating to address Ashwin's comments about the standard deviation? AFAICT the original patch would simply make ARM in line with x86 anyway, where -ENOSPC is returned at the add() level for individual events.
One argument for leaving the policy like it is, is that the user should be grouping the events, so that the deviation is at least consistent and the group will not be scheduled unless the whole thing can go on at once.
Will
Mark Rutland
maintainerPATCH 1/1 ARM: perf: remove erroneous check on active_events
In reply to Jamie IlesQuoted 4 lines
Yup, I messed up there. How about the patch below that eliminates the atomic_t's entirely? This means that we always lock the mutex in event destruction rather than just for the last one but that seems acceptable to me.
Whilst this solves the race condition, it still leaves behind some of the problems caused by having the counter in the first place:
- Events which can't actually be armed on the hardware will be initialised, but still take a slot that could otherwise be used.
- Events which can be armed on the hardware may be blocked by the above, or other events which are currently context-switched out (and are essentially independent).
- The maximum number of events we can initialise it that which can be supported by one core. As the number of cores scale upwards, we're going
to
waste the majority of the hardware counters.
By removing the counter, we lazily deny events we cannot allocate in the worst
case. We can still guarantee schedulability of groups if users are sensible with them.
By having the counter, we aggressively (possibly erroneously) deny events. As
we cannot check the possibility of arming single events until they hit the hardware, this can only be considered a heuristic (and not a great one).
Quoted 2 lines
The MIPS perf events code is based on this so could do with the same change.
Agreed. Deleting 4 lines is an easy patch to port. I'll post the ARM changes to Russell's patch system.
Mark.