[PATCH] perf_events: fix validate_event bug

[PATCH] perf_events: fix validate_event bug

[Stephane Eranian]

	The validate_event() was failing on valid event
	combinations. The function was assuming that if
	x86_schedule_event() returned 0, it meant error.
	But x86_schedule_event() returns the counter index
	and 0 is a perfectly valid value. An error is returned
	if the function returns a negative value.

	Furthermore, validate_event() was also failing for
	event groups because the event->pmu was not set until
	after hw_perf_pmu_init().
	
	Signed-off-by: Stephane Eranian <eranian@gmail.com>
--
 arch/x86/kernel/cpu/perf_event.c |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
index 2e20bca..d321ff7 100644
--- a/arch/x86/kernel/cpu/perf_event.c
+++ b/arch/x86/kernel/cpu/perf_event.c
@@ -2229,10 +2229,7 @@ validate_event(struct cpu_hw_events *cpuc, struct perf_event *event)
 {
 	struct hw_perf_event fake_event = event->hw;
 
-	if (event->pmu != &pmu)
-		return 0;
-
-	return x86_schedule_event(cpuc, &fake_event);
+	return x86_schedule_event(cpuc, &fake_event) >= 0;
 }
 
 static int validate_group(struct perf_event *event)

Re: [PATCH] perf_events: fix validate_event bug

[Peter Zijlstra]

On Thu, 2009-10-22 at 16:51 +0200, Stephane Eranian wrote:
> 	The validate_event() was failing on valid event
> 	combinations. The function was assuming that if
> 	x86_schedule_event() returned 0, it meant error.
> 	But x86_schedule_event() returns the counter index
> 	and 0 is a perfectly valid value. An error is returned
> 	if the function returns a negative value.

Good point.

> 	Furthermore, validate_event() was also failing for
> 	event groups because the event->pmu was not set until
> 	after hw_perf_pmu_init().

(hw_perf_event_init, right?)

Won't this give very funny results for mixed pmu groups?

How about something like:

 if (event->pmu && event->pmu != &pmu)
	return 0;

That should deal with new events, who do not yet have their pmu set and
for those we know they're for us, but exclude events for other PMUs.

> 	Signed-off-by: Stephane Eranian <eranian@gmail.com>
> --
>  arch/x86/kernel/cpu/perf_event.c |    5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
> index 2e20bca..d321ff7 100644
> --- a/arch/x86/kernel/cpu/perf_event.c
> +++ b/arch/x86/kernel/cpu/perf_event.c
> @@ -2229,10 +2229,7 @@ validate_event(struct cpu_hw_events *cpuc, struct perf_event *event)
>  {
>  	struct hw_perf_event fake_event = event->hw;
>  
> -	if (event->pmu != &pmu)
> -		return 0;
> -
> -	return x86_schedule_event(cpuc, &fake_event);
> +	return x86_schedule_event(cpuc, &fake_event) >= 0;
>  }
>  
>  static int validate_group(struct perf_event *event)

Re: [PATCH] perf_events: fix validate_event bug

[stephane eranian]

Hi,

Sorry for the delay,

On Wed, Nov 18, 2009 at 5:46 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Thu, 2009-10-22 at 16:51 +0200, Stephane Eranian wrote:
>>       The validate_event() was failing on valid event
>>       combinations. The function was assuming that if
>>       x86_schedule_event() returned 0, it meant error.
>>       But x86_schedule_event() returns the counter index
>>       and 0 is a perfectly valid value. An error is returned
>>       if the function returns a negative value.
>
> Good point.
>
>>       Furthermore, validate_event() was also failing for
>>       event groups because the event->pmu was not set until
>>       after hw_perf_pmu_init().
>
> (hw_perf_event_init, right?)
>
Yes.

> Won't this give very funny results for mixed pmu groups?
>

What do you mean by 'mixed pmu groups'?

> How about something like:
>
>  if (event->pmu && event->pmu != &pmu)
>        return 0;
>
> That should deal with new events, who do not yet have their pmu set and
> for those we know they're for us, but exclude events for other PMUs.
>
>>       Signed-off-by: Stephane Eranian <eranian@gmail.com>
>> --
>>  arch/x86/kernel/cpu/perf_event.c |    5 +----
>>  1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
>> index 2e20bca..d321ff7 100644
>> --- a/arch/x86/kernel/cpu/perf_event.c
>> +++ b/arch/x86/kernel/cpu/perf_event.c
>> @@ -2229,10 +2229,7 @@ validate_event(struct cpu_hw_events *cpuc, struct perf_event *event)
>>  {
>>       struct hw_perf_event fake_event = event->hw;
>>
>> -     if (event->pmu != &pmu)
>> -             return 0;
>> -
>> -     return x86_schedule_event(cpuc, &fake_event);
>> +     return x86_schedule_event(cpuc, &fake_event) >= 0;
>>  }
>>
>>  static int validate_group(struct perf_event *event)
>
>
>

Re: [PATCH] perf_events: fix validate_event bug

[Peter Zijlstra]

On Mon, 2009-11-23 at 14:34 +0100, stephane eranian wrote:

> > Won't this give very funny results for mixed pmu groups?
> >
> 
> What do you mean by 'mixed pmu groups'?

We currently have a number of struct pmu objects:

 perf_ops_generic
 perf_ops_cpu_clock
 perf_ops_task_clock

which are all software based PMUs, and one of:

 pmu        (arch/x86/kernel/cpu/perf_event.c)
 power_pmu  (arch/powerpc/kernel/perf_event.c)

To represent the hardware PMU.

Now say you mix software events and hardware events into a single group,
the loop in validate_group:

  list_for_each_entry(sibling, &leader->sibling_list, group_entry) {
	if (!validate_event(&fake_pmu, sibling))
			return -ENOSPC;
  }

could pass a !hardware event into validate_event(), which currently
ignores it because event->pmu won't be &pmu, however if you remove that
check, it'll try and call x86 routines on a software event, which is
bound to go funny.

Now Frederic is going to make things more interesting by representing HW
breakpoints as another HW PMU (the distinction between hw/sw pmu is in
scheduling, you can always schedule a software event).

This weakens the !is_software_event(), in that !software doesn't tell
you which hardware event it is -- something which needs mending in your
more complex x86 constraints scheduling patch.

Re: [PATCH] perf_events: fix validate_event bug

[stephane eranian]

On Mon, Nov 23, 2009 at 2:45 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Mon, 2009-11-23 at 14:34 +0100, stephane eranian wrote:
>
>> > Won't this give very funny results for mixed pmu groups?
>> >
>>
>> What do you mean by 'mixed pmu groups'?
>
> We currently have a number of struct pmu objects:
>
>  perf_ops_generic
>  perf_ops_cpu_clock
>  perf_ops_task_clock
>
> which are all software based PMUs, and one of:
>
>  pmu        (arch/x86/kernel/cpu/perf_event.c)
>  power_pmu  (arch/powerpc/kernel/perf_event.c)
>
> To represent the hardware PMU.
>
> Now say you mix software events and hardware events into a single group,
> the loop in validate_group:
>
>  list_for_each_entry(sibling, &leader->sibling_list, group_entry) {
>        if (!validate_event(&fake_pmu, sibling))
>                        return -ENOSPC;
>  }
>
> could pass a !hardware event into validate_event(), which currently
> ignores it because event->pmu won't be &pmu, however if you remove that
> check, it'll try and call x86 routines on a software event, which is
> bound to go funny.
>
Ok, so it seems the only valid test to check if the event is related to the
HW PMU is to compare event->pmu with pmu (arch/x86/.../perf_event.c).

In that case you first suggestion is fine.

> Now Frederic is going to make things more interesting by representing HW
> breakpoints as another HW PMU (the distinction between hw/sw pmu is in
> scheduling, you can always schedule a software event).
>
> This weakens the !is_software_event(), in that !software doesn't tell
> you which hardware event it is -- something which needs mending in your
> more complex x86 constraints scheduling patch.
>
That means we can drop is_software_event() in this code and instead
define locally
in x86 a is_hw_pmu_event() function as event->pmu == &pmu.

[PATCH] perf_events: fix validate_event bug

[Stephane Eranian]

	The validate_event() was failing on valid event
	combinations. The function was assuming that if
	x86_schedule_event() returned 0, it meant error.
	But x86_schedule_event() returns the counter index
	and 0 is a perfectly valid value. An error is returned
	if the function returns a negative value.

	Furthermore, validate_event() was also failing for
	event groups because the event->pmu was not set until
	after hw_perf_event_init().
	
	Signed-off-by: Stephane Eranian <eranian@google.com>
--
 arch/x86/kernel/cpu/perf_event.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
index bd87430..c1bbed1 100644
--- a/arch/x86/kernel/cpu/perf_event.c
+++ b/arch/x86/kernel/cpu/perf_event.c
@@ -2229,10 +2229,10 @@ validate_event(struct cpu_hw_events *cpuc, struct perf_event *event)
 {
 	struct hw_perf_event fake_event = event->hw;
 
-	if (event->pmu != &pmu)
+	if (event->pmu && event->pmu != &pmu)
 		return 0;
 
-	return x86_schedule_event(cpuc, &fake_event);
+	return x86_schedule_event(cpuc, &fake_event) >= 0;
 }
 
 static int validate_group(struct perf_event *event)

[tip:perf/core] perf_events, x86: Fix validate_event bug

[tip-bot for Stephane Eranian]

Commit-ID:  1261a02a0c0ab8e643125705f0d1d83e5090e4d1
Gitweb:     http://git.kernel.org/tip/1261a02a0c0ab8e643125705f0d1d83e5090e4d1
Author:     Stephane Eranian <eranian@google.com>
AuthorDate: Tue, 24 Nov 2009 05:27:18 -0800
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Tue, 24 Nov 2009 19:23:48 +0100

perf_events, x86: Fix validate_event bug

The validate_event() was failing on valid event combinations. The
function was assuming that if x86_schedule_event() returned 0, it
meant error. But x86_schedule_event() returns the counter index and
0 is a perfectly valid value. An error is returned if the function
returns a negative value.

Furthermore, validate_event() was also failing for event groups
because the event->pmu was not set until after
hw_perf_event_init().

Signed-off-by: Stephane Eranian <eranian@google.com>
Cc: peterz@infradead.org
Cc: paulus@samba.org
Cc: perfmon2-devel@lists.sourceforge.net
Cc: eranian@gmail.com
LKML-Reference: <4b0bdf36.1818d00a.07cc.25ae@mx.google.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
--
 arch/x86/kernel/cpu/perf_event.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
---
 arch/x86/kernel/cpu/perf_event.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
index bd87430..c1bbed1 100644
--- a/arch/x86/kernel/cpu/perf_event.c
+++ b/arch/x86/kernel/cpu/perf_event.c
@@ -2229,10 +2229,10 @@ validate_event(struct cpu_hw_events *cpuc, struct perf_event *event)
 {
 	struct hw_perf_event fake_event = event->hw;
 
-	if (event->pmu != &pmu)
+	if (event->pmu && event->pmu != &pmu)
 		return 0;
 
-	return x86_schedule_event(cpuc, &fake_event);
+	return x86_schedule_event(cpuc, &fake_event) >= 0;
 }
 
 static int validate_group(struct perf_event *event)

Re: [PATCH] perf_events: fix validate_event bug

[Paul Mackerras]

stephane eranian writes:

> That means we can drop is_software_event() in this code and instead
> define locally
> in x86 a is_hw_pmu_event() function as event->pmu == &pmu.

I'd have to see the patch, but that doesn't feel entirely right,
because there is a unique characteristic of software events, compared
to hardware or breakpoint events: they are never capacity
constrained.  In the past, only hardware events were capacity
constrained, which meant that all the decisions about whether a group
could go on could be done in the hardware PMU backend.  Now we have
two sources of capacity constraints, so it may be that a group can't
go on even if the hardware PMU has capacity.  That's going to be
somewhat interesting to get completely right, I think.

Paul.

Re: [PATCH] perf_events: fix validate_event bug

[stephane eranian]

On Tue, Nov 24, 2009 at 11:00 PM, Paul Mackerras <paulus@samba.org> wrote:
> stephane eranian writes:
>
>> That means we can drop is_software_event() in this code and instead
>> define locally
>> in x86 a is_hw_pmu_event() function as event->pmu == &pmu.
>
> I'd have to see the patch, but that doesn't feel entirely right,
> because there is a unique characteristic of software events, compared
> to hardware or breakpoint events: they are never capacity
> constrained.  In the past, only hardware events were capacity
> constrained, which meant that all the decisions about whether a group
> could go on could be done in the hardware PMU backend.  Now we have
> two sources of capacity constraints, so it may be that a group can't
> go on even if the hardware PMU has capacity.  That's going to be
> somewhat interesting to get completely right, I think.
>
I was talking of is_software_event() in the context of the hardware PMU
code. The reason you were using this function is simply to skip
SW events because, as you said, they are never constrained
and also because they are not related to HW PMU.

You are right about HW breakpoints, because now you have a new source of
constraints. Only the breakpoint code knows about those constraints.

It seems to me you have two ways of solving this:
  1- push the algorithm to assign events to counters up in the generic code
  2- have the generic code invoke all possible constraint sources on each group

I have already said that I would not recommend initially going with 1- because
constraints are very diverse in their nature. It is not as simple as 1 event = 1
bitmask of valid counters. Things can be more dynamic than that, e.g., on AMD64,
whereby for certain events the bitmask depends on what is going on in the other
cores on the socket. There are also similar constraints on advanced
Intel features.
So unlike what I heard early on, constraints are not going away,
instead they are
changing in nature and to something much more complex to deal with.

I believe that until all PMU event assignment logic is implemented in the PMU
specific code, it would be very presumptuous to try and design that
generic algorithm.
So I would go with 2, at least initially.

Re: [PATCH] perf_events: fix validate_event bug

[Peter Zijlstra]

On Wed, 2009-11-25 at 06:47 +0100, stephane eranian wrote:
> It seems to me you have two ways of solving this:
>   1- push the algorithm to assign events to counters up in the generic code
>   2- have the generic code invoke all possible constraint sources on each group

2, is what it currently does. 

Since we can only add a single event to a group at a time, and we know
the previous group was schedulable, we only need to validate the
constraints on the pmu of the new event.