[PATCH] ARM: perf: remove erroneous check on active_events

[PATCH] ARM: perf: remove erroneous check on active_events

[Mark Rutland]

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.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Acked-by: Will Deacon <will.deacon@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Jamie Iles <jamie@jamieiles.com>
---
 arch/arm/kernel/perf_event.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/arch/arm/kernel/perf_event.c b/arch/arm/kernel/perf_event.c
index fd6403c..29a0cf8 100644
--- a/arch/arm/kernel/perf_event.c
+++ b/arch/arm/kernel/perf_event.c
@@ -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();
-- 
1.7.0.4

[PATCH] ARM: perf: remove erroneous check on active_events

[Ashwin Chaugule]

Hi Mark,

> From: Mark Rutland <mark.rutland@arm.com>
>
> 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.

Nice find ! I had a slightly different solution.

> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Acked-by: Will Deacon <will.deacon@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Jamie Iles <jamie@jamieiles.com>
> ---
>  arch/arm/kernel/perf_event.c |    5 -----
>  1 files changed, 0 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/kernel/perf_event.c b/arch/arm/kernel/perf_event.c
> index fd6403c..29a0cf8 100644
> --- a/arch/arm/kernel/perf_event.c
> +++ b/arch/arm/kernel/perf_event.c
> @@ -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();
> --

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


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

[PATCH] ARM: perf: remove erroneous check on active_events

[Russell King - ARM Linux]

On Wed, Apr 27, 2011 at 10:57:16AM +0100, Mark Rutland wrote:
> diff --git a/arch/arm/kernel/perf_event.c b/arch/arm/kernel/perf_event.c
> index fd6403c..29a0cf8 100644
> --- a/arch/arm/kernel/perf_event.c
> +++ b/arch/arm/kernel/perf_event.c
> @@ -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

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] ARM: perf: remove erroneous check on active_events

[Jamie Iles]

On Thu, Apr 28, 2011 at 04:41:08PM +0100, Russell King - ARM Linux wrote:
> On Wed, Apr 27, 2011 at 10:57:16AM +0100, Mark Rutland wrote:
> > diff --git a/arch/arm/kernel/perf_event.c b/arch/arm/kernel/perf_event.c
> > index fd6403c..29a0cf8 100644
> > --- a/arch/arm/kernel/perf_event.c
> > +++ b/arch/arm/kernel/perf_event.c
> > @@ -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
> 
> 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<---

diff --git a/arch/arm/kernel/perf_event.c b/arch/arm/kernel/perf_event.c
index 69cfee0..439f006 100644
--- a/arch/arm/kernel/perf_event.c
+++ b/arch/arm/kernel/perf_event.c
@@ -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] ARM: perf: remove erroneous check on active_events

[Will Deacon]

Hi guys [adding Peter Z],

On Thu, 2011-04-28 at 17:12 +0100, Jamie Iles wrote:
> On Thu, Apr 28, 2011 at 04:41:08PM +0100, Russell King - ARM Linux wrote:
> > On Wed, Apr 27, 2011 at 10:57:16AM +0100, Mark Rutland wrote:
> > > diff --git a/arch/arm/kernel/perf_event.c b/arch/arm/kernel/perf_event.c
> > > index fd6403c..29a0cf8 100644
> > > --- a/arch/arm/kernel/perf_event.c
> > > +++ b/arch/arm/kernel/perf_event.c
> > > @@ -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)

Agreed. Mark's patch is deleting the offending code though, so that's a
plus.

> 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

[PATCH] ARM: perf: remove erroneous check on active_events

[Mark Rutland]

> 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).

> 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.