* Re: [PATCH] perf/x86/uncore: Allow single pmu/box within events group
2016-11-18 12:53 ` Peter Zijlstra
@ 2016-11-18 13:23 ` Jiri Olsa
2016-11-18 13:48 ` Jiri Olsa
2016-11-22 12:30 ` [tip:perf/urgent] perf/x86/intel/uncore: Allow only a single PMU/box within an " tip-bot for Peter Zijlstra
2 siblings, 0 replies; 10+ messages in thread
From: Jiri Olsa @ 2016-11-18 13:23 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Liang, Kan, Andi Kleen, Vince Weaver, lkml, Ingo Molnar, Thomas Gleixner
On Fri, Nov 18, 2016 at 01:53:54PM +0100, Peter Zijlstra wrote:
> On Fri, Nov 18, 2016 at 01:33:25PM +0100, Jiri Olsa wrote:
> > On Fri, Nov 18, 2016 at 12:28:50PM +0100, Peter Zijlstra wrote:
> > > On Fri, Nov 18, 2016 at 01:15:28AM +0100, Jiri Olsa wrote:
> > > > Current uncore_validate_group code expects all events within
> > > > the group to have same pmu.
> > > >
> > > > This leads to constraint code using wrong boxes which leads
> > > > in my case to touching uninitialized spinlocks, but could
> > > > be probably worse.. depends on type and box details.
> > > >
> > > > I get lockdep warning below for following perf stat:
> > > > # perf stat -vv -e '{uncore_cbox_0/config=0x0334/,uncore_qpi_0/event=1/}' -a sleep 1
> > >
> > > Hurm, we shouldn't be allowing that in the first place I think.
> > >
> > >
> > > Let me stare at the generic group code, the intent was to only allow
> > > software events to mix with hw events, nothing else.
> >
> > yep, that's what's happening now.. but after the event_init callback
>
> Ah yes indeed. Its the is_uncore_event() test in uncore_collect_event()
> that's too lenient, that allows us to mix events from various uncore
> boxes.
>
> Would something like so fix things too? Because that is the point of
> is_uncore_event() in collect(), to only collect events for _that_ pmu.
that looks ok.. I'll run the test
jirka
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] perf/x86/uncore: Allow single pmu/box within events group
2016-11-18 12:53 ` Peter Zijlstra
2016-11-18 13:23 ` Jiri Olsa
@ 2016-11-18 13:48 ` Jiri Olsa
2016-11-18 14:06 ` Peter Zijlstra
2016-11-22 12:30 ` [tip:perf/urgent] perf/x86/intel/uncore: Allow only a single PMU/box within an " tip-bot for Peter Zijlstra
2 siblings, 1 reply; 10+ messages in thread
From: Jiri Olsa @ 2016-11-18 13:48 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Liang, Kan, Andi Kleen, Vince Weaver, lkml, Ingo Molnar, Thomas Gleixner
On Fri, Nov 18, 2016 at 01:53:54PM +0100, Peter Zijlstra wrote:
SNIP
> ---
> arch/x86/events/intel/uncore.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
> index efca2685d876..7b1b34576886 100644
> --- a/arch/x86/events/intel/uncore.c
> +++ b/arch/x86/events/intel/uncore.c
> @@ -319,9 +319,9 @@ static struct intel_uncore_box *uncore_alloc_box(struct intel_uncore_type *type,
> */
> static int uncore_pmu_event_init(struct perf_event *event);
>
> -static bool is_uncore_event(struct perf_event *event)
> +static bool is_box_event(struct intel_uncore_box *box, struct perf_event *event)
> {
> - return event->pmu->event_init == uncore_pmu_event_init;
> + return box->pmu == event->pmu;
this one needs to be:
+ return box->pmu == uncore_event_to_pmu(event);
and it works.. ;-)
Tested-by: Jiri Olsa <jolsa@redhat.com>
thanks,
jirka
> }
>
> static int
> @@ -340,7 +340,7 @@ uncore_collect_events(struct intel_uncore_box *box, struct perf_event *leader,
>
> n = box->n_events;
>
> - if (is_uncore_event(leader)) {
> + if (is_box_event(box, leader)) {
> box->event_list[n] = leader;
> n++;
> }
> @@ -349,7 +349,7 @@ uncore_collect_events(struct intel_uncore_box *box, struct perf_event *leader,
> return n;
>
> list_for_each_entry(event, &leader->sibling_list, group_entry) {
> - if (!is_uncore_event(event) ||
> + if (!is_box_event(box, event) ||
> event->state <= PERF_EVENT_STATE_OFF)
> continue;
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] perf/x86/uncore: Allow single pmu/box within events group
2016-11-18 13:48 ` Jiri Olsa
@ 2016-11-18 14:06 ` Peter Zijlstra
2016-11-18 14:30 ` Jiri Olsa
0 siblings, 1 reply; 10+ messages in thread
From: Peter Zijlstra @ 2016-11-18 14:06 UTC (permalink / raw)
To: Jiri Olsa
Cc: Liang, Kan, Andi Kleen, Vince Weaver, lkml, Ingo Molnar, Thomas Gleixner
On Fri, Nov 18, 2016 at 02:48:59PM +0100, Jiri Olsa wrote:
> On Fri, Nov 18, 2016 at 01:53:54PM +0100, Peter Zijlstra wrote:
>
> SNIP
>
> > ---
> > arch/x86/events/intel/uncore.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
> > index efca2685d876..7b1b34576886 100644
> > --- a/arch/x86/events/intel/uncore.c
> > +++ b/arch/x86/events/intel/uncore.c
> > @@ -319,9 +319,9 @@ static struct intel_uncore_box *uncore_alloc_box(struct intel_uncore_type *type,
> > */
> > static int uncore_pmu_event_init(struct perf_event *event);
> >
> > -static bool is_uncore_event(struct perf_event *event)
> > +static bool is_box_event(struct intel_uncore_box *box, struct perf_event *event)
> > {
> > - return event->pmu->event_init == uncore_pmu_event_init;
> > + return box->pmu == event->pmu;
>
> this one needs to be:
>
> + return box->pmu == uncore_event_to_pmu(event);
>
> and it works.. ;-)
Will that not explode if we fudge a software event in there?
Wouldn't:
return box->pmu.pmu == event->pmu;
be the safer option?
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] perf/x86/uncore: Allow single pmu/box within events group
2016-11-18 14:06 ` Peter Zijlstra
@ 2016-11-18 14:30 ` Jiri Olsa
2016-11-18 14:45 ` Peter Zijlstra
0 siblings, 1 reply; 10+ messages in thread
From: Jiri Olsa @ 2016-11-18 14:30 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Liang, Kan, Andi Kleen, Vince Weaver, lkml, Ingo Molnar, Thomas Gleixner
On Fri, Nov 18, 2016 at 03:06:05PM +0100, Peter Zijlstra wrote:
> On Fri, Nov 18, 2016 at 02:48:59PM +0100, Jiri Olsa wrote:
> > On Fri, Nov 18, 2016 at 01:53:54PM +0100, Peter Zijlstra wrote:
> >
> > SNIP
> >
> > > ---
> > > arch/x86/events/intel/uncore.c | 8 ++++----
> > > 1 file changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
> > > index efca2685d876..7b1b34576886 100644
> > > --- a/arch/x86/events/intel/uncore.c
> > > +++ b/arch/x86/events/intel/uncore.c
> > > @@ -319,9 +319,9 @@ static struct intel_uncore_box *uncore_alloc_box(struct intel_uncore_type *type,
> > > */
> > > static int uncore_pmu_event_init(struct perf_event *event);
> > >
> > > -static bool is_uncore_event(struct perf_event *event)
> > > +static bool is_box_event(struct intel_uncore_box *box, struct perf_event *event)
> > > {
> > > - return event->pmu->event_init == uncore_pmu_event_init;
> > > + return box->pmu == event->pmu;
> >
> > this one needs to be:
> >
> > + return box->pmu == uncore_event_to_pmu(event);
> >
> > and it works.. ;-)
>
> Will that not explode if we fudge a software event in there?
>
> Wouldn't:
>
> return box->pmu.pmu == event->pmu;
>
> be the safer option?
hum right.. but for some reason I can't crash it nor even fuzzer complains
jirka
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] perf/x86/uncore: Allow single pmu/box within events group
2016-11-18 14:30 ` Jiri Olsa
@ 2016-11-18 14:45 ` Peter Zijlstra
0 siblings, 0 replies; 10+ messages in thread
From: Peter Zijlstra @ 2016-11-18 14:45 UTC (permalink / raw)
To: Jiri Olsa
Cc: Liang, Kan, Andi Kleen, Vince Weaver, lkml, Ingo Molnar, Thomas Gleixner
On Fri, Nov 18, 2016 at 03:30:48PM +0100, Jiri Olsa wrote:
> > > > +static bool is_box_event(struct intel_uncore_box *box, struct perf_event *event)
> > > > {
> > > > - return event->pmu->event_init == uncore_pmu_event_init;
> > > > + return box->pmu == event->pmu;
> > >
> > > this one needs to be:
> > >
> > > + return box->pmu == uncore_event_to_pmu(event);
> > >
> > > and it works.. ;-)
> >
> > Will that not explode if we fudge a software event in there?
> >
> > Wouldn't:
> >
> > return box->pmu.pmu == event->pmu;
> >
> > be the safer option?
>
> hum right.. but for some reason I can't crash it nor even fuzzer complains
Its because pmu is the first member, so the pointer is the exact same,
just the type changes. But that is a 'happy' accident of implementation.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [tip:perf/urgent] perf/x86/intel/uncore: Allow only a single PMU/box within an events group
2016-11-18 12:53 ` Peter Zijlstra
2016-11-18 13:23 ` Jiri Olsa
2016-11-18 13:48 ` Jiri Olsa
@ 2016-11-22 12:30 ` tip-bot for Peter Zijlstra
2 siblings, 0 replies; 10+ messages in thread
From: tip-bot for Peter Zijlstra @ 2016-11-22 12:30 UTC (permalink / raw)
To: linux-tip-commits
Cc: alexander.shishkin, vince, peterz, hpa, vincent.weaver, torvalds,
eranian, acme, kan.liang, linux-kernel, jolsa, mingo, tglx
Commit-ID: 033ac60c7f21f9996a0fab2fd04f334afbf77b33
Gitweb: http://git.kernel.org/tip/033ac60c7f21f9996a0fab2fd04f334afbf77b33
Author: Peter Zijlstra <peterz@infradead.org>
AuthorDate: Fri, 18 Nov 2016 13:53:54 +0100
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 22 Nov 2016 12:36:59 +0100
perf/x86/intel/uncore: Allow only a single PMU/box within an events group
Group validation expects all events to be of the same PMU; however
is_uncore_pmu() is too wide, it matches _all_ uncore events, even
across PMUs.
This triggers failure when we group different events from different
uncore PMUs, like:
perf stat -vv -e '{uncore_cbox_0/config=0x0334/,uncore_qpi_0/event=1/}' -a sleep 1
Fix is_uncore_pmu() by only matching events to the box at hand.
Note that generic code; ran after this step; will disallow this
mixture of PMU events.
Reported-by: Jiri Olsa <jolsa@redhat.com>
Tested-by: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Kan Liang <kan.liang@intel.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Vince Weaver <vince@deater.net>
Cc: Vince Weaver <vincent.weaver@maine.edu>
Link: http://lkml.kernel.org/r/20161118125354.GQ3117@twins.programming.kicks-ass.net
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
arch/x86/events/intel/uncore.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
index efca268..dbaaf7dc 100644
--- a/arch/x86/events/intel/uncore.c
+++ b/arch/x86/events/intel/uncore.c
@@ -319,9 +319,9 @@ static struct intel_uncore_box *uncore_alloc_box(struct intel_uncore_type *type,
*/
static int uncore_pmu_event_init(struct perf_event *event);
-static bool is_uncore_event(struct perf_event *event)
+static bool is_box_event(struct intel_uncore_box *box, struct perf_event *event)
{
- return event->pmu->event_init == uncore_pmu_event_init;
+ return &box->pmu->pmu == event->pmu;
}
static int
@@ -340,7 +340,7 @@ uncore_collect_events(struct intel_uncore_box *box, struct perf_event *leader,
n = box->n_events;
- if (is_uncore_event(leader)) {
+ if (is_box_event(box, leader)) {
box->event_list[n] = leader;
n++;
}
@@ -349,7 +349,7 @@ uncore_collect_events(struct intel_uncore_box *box, struct perf_event *leader,
return n;
list_for_each_entry(event, &leader->sibling_list, group_entry) {
- if (!is_uncore_event(event) ||
+ if (!is_box_event(box, event) ||
event->state <= PERF_EVENT_STATE_OFF)
continue;
^ permalink raw reply related [flat|nested] 10+ messages in thread