[PATCH] perf/x86: Fix overlap counter scheduling bug

[PATCH] perf/x86: Fix overlap counter scheduling bug

[Jiri Olsa]

My fuzzer testing hits following warning in the counter scheduling code:

  WARNING: CPU: 0 PID: 0 at arch/x86/events/core.c:718 perf_assign_events+0x2ae/0x2c0
  Call Trace:
   <IRQ>
   dump_stack+0x68/0x93
   __warn+0xcb/0xf0
   warn_slowpath_null+0x1d/0x20
   perf_assign_events+0x2ae/0x2c0
   uncore_assign_events+0x1a7/0x250 [intel_uncore]
   uncore_pmu_event_add+0x7a/0x3c0 [intel_uncore]
   event_sched_in.isra.104+0xf6/0x2e0
   group_sched_in+0x6e/0x190
   ...

The reason is that the counter scheduling code assumes
overlap constraints with mask weight < SCHED_STATES_MAX.

This assumption is broken with uncore cbox constraint
added for snbep in:
  3b19e4c98c03 perf/x86: Fix event constraint for SandyBridge-EP C-Box

It's also easily triggered by running following perf command
on snbep box:
   # perf stat -e 'uncore_cbox_0/event=0x1f/,uncore_cbox_0/event=0x1f/,uncore_cbox_0/event=0x1f/' -a

Fixing this by increasing the SCHED_STATES_MAX to 3
and adding build check for EVENT_CONSTRAINT_OVERLAP
macro.

Cc: Vince Weaver <vince@deater.net>
Cc: Robert Richter <rric@kernel.org>
Cc: Yan Zheng <zheng.z.yan@intel.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 arch/x86/events/core.c       |  3 ---
 arch/x86/events/perf_event.h | 10 +++++++++-
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index d31735f37ed7..c725f8854216 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -676,9 +676,6 @@ struct sched_state {
 	unsigned long used[BITS_TO_LONGS(X86_PMC_IDX_MAX)];
 };
 
-/* Total max is X86_PMC_IDX_MAX, but we are O(n!) limited */
-#define	SCHED_STATES_MAX	2
-
 struct perf_sched {
 	int			max_weight;
 	int			max_events;
diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
index 5874d8de1f8d..4553275b37d4 100644
--- a/arch/x86/events/perf_event.h
+++ b/arch/x86/events/perf_event.h
@@ -277,8 +277,16 @@ struct cpu_hw_events {
  * dramatically.  The number of such EVENT_CONSTRAINT_OVERLAP() macros
  * and its counter masks must be kept at a minimum.
  */
+
+/* Total max is X86_PMC_IDX_MAX, but we are O(n!) limited */
+#define SCHED_STATES_MAX 3
+
+/* Check we dont overlap beyond the states max. */
+#define OVERLAP_CHECK(n)   (!!sizeof(char[1 - 2*!!(HWEIGHT(n) > SCHED_STATES_MAX)]))
+#define OVERLAP_HWEIGHT(n) (OVERLAP_CHECK(n)*HWEIGHT(n))
+
 #define EVENT_CONSTRAINT_OVERLAP(c, n, m)	\
-	__EVENT_CONSTRAINT(c, n, m, HWEIGHT(n), 1, 0)
+	__EVENT_CONSTRAINT(c, n, m, OVERLAP_HWEIGHT(n), 1, 0)
 
 /*
  * Constraint on the Event code.
-- 
2.7.4

Re: [PATCH] perf/x86: Fix overlap counter scheduling bug

[Peter Zijlstra]

On Tue, Nov 01, 2016 at 04:44:28PM +0100, Jiri Olsa wrote:
> My fuzzer testing hits following warning in the counter scheduling code:
> 
>   WARNING: CPU: 0 PID: 0 at arch/x86/events/core.c:718 perf_assign_events+0x2ae/0x2c0
>   Call Trace:
>    <IRQ>
>    dump_stack+0x68/0x93
>    __warn+0xcb/0xf0
>    warn_slowpath_null+0x1d/0x20
>    perf_assign_events+0x2ae/0x2c0
>    uncore_assign_events+0x1a7/0x250 [intel_uncore]
>    uncore_pmu_event_add+0x7a/0x3c0 [intel_uncore]
>    event_sched_in.isra.104+0xf6/0x2e0
>    group_sched_in+0x6e/0x190
>    ...
> 
> The reason is that the counter scheduling code assumes
> overlap constraints with mask weight < SCHED_STATES_MAX.
> 
> This assumption is broken with uncore cbox constraint
> added for snbep in:


>   3b19e4c98c03 perf/x86: Fix event constraint for SandyBridge-EP C-Box

   3b19e4c98c03 ("perf/x86: Fix event constraint for SandyBridge-EP C-Box")

Is the right form.

> It's also easily triggered by running following perf command
> on snbep box:
>    # perf stat -e 'uncore_cbox_0/event=0x1f/,uncore_cbox_0/event=0x1f/,uncore_cbox_0/event=0x1f/' -a
> 
> Fixing this by increasing the SCHED_STATES_MAX to 3 and adding build
> check for EVENT_CONSTRAINT_OVERLAP macro.


> -#define	SCHED_STATES_MAX	2
> +#define SCHED_STATES_MAX 3

Us having to increase this is ff'ing sad :/ That's seriously challenged
hardware :/

> +
> +/* Check we dont overlap beyond the states max. */
> +#define OVERLAP_CHECK(n)   (!!sizeof(char[1 - 2*!!(HWEIGHT(n) > SCHED_STATES_MAX)]))
> +#define OVERLAP_HWEIGHT(n) (OVERLAP_CHECK(n)*HWEIGHT(n))

I'm not sure I get how this is correct at all. You cannot tell by a
single mask what the overlap is. You need all the masks.

The point is that that PMU has constraints like:

 0x01 - 0001
 0x03 - 0011
 0x0e - 1110
 0x0c - 1100

Which gets us a total of 4 overlapping counter masks, and that would
indeed lead to max 3 retries I think.

Now, I would much rather solve this by changing the constraint like the
below, that yields:

 0x01 - 0001
 0x03 - 0011

 0x0c - 1100

Which is two distinct groups, only one of which has overlap. And the one
with overlap only has 2 overlapping masks, giving a max reties of 1.


diff --git a/arch/x86/events/intel/uncore_snbep.c b/arch/x86/events/intel/uncore_snbep.c
index 272427700d48..71bc348736bd 100644
--- a/arch/x86/events/intel/uncore_snbep.c
+++ b/arch/x86/events/intel/uncore_snbep.c
@@ -669,7 +669,7 @@ static struct event_constraint snbep_uncore_cbox_constraints[] = {
 	UNCORE_EVENT_CONSTRAINT(0x1c, 0xc),
 	UNCORE_EVENT_CONSTRAINT(0x1d, 0xc),
 	UNCORE_EVENT_CONSTRAINT(0x1e, 0xc),
-	EVENT_CONSTRAINT_OVERLAP(0x1f, 0xe, 0xff),
+	UNCORE_EVENT_CONSTRAINT(0x1f, 0xc); /* should be 0x0e but that gives scheduling pain */
 	UNCORE_EVENT_CONSTRAINT(0x21, 0x3),
 	UNCORE_EVENT_CONSTRAINT(0x23, 0x3),
 	UNCORE_EVENT_CONSTRAINT(0x31, 0x3),

Re: [PATCH] perf/x86: Fix overlap counter scheduling bug

[Jiri Olsa]

On Tue, Nov 08, 2016 at 01:20:39PM +0100, Peter Zijlstra wrote:

SNIP

> Now, I would much rather solve this by changing the constraint like the
> below, that yields:
> 
>  0x01 - 0001
>  0x03 - 0011
> 
>  0x0c - 1100
> 
> Which is two distinct groups, only one of which has overlap. And the one
> with overlap only has 2 overlapping masks, giving a max reties of 1.

works for me.. 

thanks,
jirka

> 
> 
> diff --git a/arch/x86/events/intel/uncore_snbep.c b/arch/x86/events/intel/uncore_snbep.c
> index 272427700d48..71bc348736bd 100644
> --- a/arch/x86/events/intel/uncore_snbep.c
> +++ b/arch/x86/events/intel/uncore_snbep.c
> @@ -669,7 +669,7 @@ static struct event_constraint snbep_uncore_cbox_constraints[] = {
>  	UNCORE_EVENT_CONSTRAINT(0x1c, 0xc),
>  	UNCORE_EVENT_CONSTRAINT(0x1d, 0xc),
>  	UNCORE_EVENT_CONSTRAINT(0x1e, 0xc),
> -	EVENT_CONSTRAINT_OVERLAP(0x1f, 0xe, 0xff),
> +	UNCORE_EVENT_CONSTRAINT(0x1f, 0xc); /* should be 0x0e but that gives scheduling pain */
>  	UNCORE_EVENT_CONSTRAINT(0x21, 0x3),
>  	UNCORE_EVENT_CONSTRAINT(0x23, 0x3),
>  	UNCORE_EVENT_CONSTRAINT(0x31, 0x3),

Re: [PATCH] perf/x86: Fix overlap counter scheduling bug

[Andi Kleen]

Adding Kan who actually maintains the uncore drivers these days.

> Which is two distinct groups, only one of which has overlap. And the one
> with overlap only has 2 overlapping masks, giving a max reties of 1.

Looks reasonable to limit the mask.

Are we sure this problem isn't in the other 0xc masks too ?

-Andi

> 
> 
> diff --git a/arch/x86/events/intel/uncore_snbep.c b/arch/x86/events/intel/uncore_snbep.c
> index 272427700d48..71bc348736bd 100644
> --- a/arch/x86/events/intel/uncore_snbep.c
> +++ b/arch/x86/events/intel/uncore_snbep.c
> @@ -669,7 +669,7 @@ static struct event_constraint snbep_uncore_cbox_constraints[] = {
>  	UNCORE_EVENT_CONSTRAINT(0x1c, 0xc),
>  	UNCORE_EVENT_CONSTRAINT(0x1d, 0xc),
>  	UNCORE_EVENT_CONSTRAINT(0x1e, 0xc),
> -	EVENT_CONSTRAINT_OVERLAP(0x1f, 0xe, 0xff),
> +	UNCORE_EVENT_CONSTRAINT(0x1f, 0xc); /* should be 0x0e but that gives scheduling pain */
>  	UNCORE_EVENT_CONSTRAINT(0x21, 0x3),
>  	UNCORE_EVENT_CONSTRAINT(0x23, 0x3),
>  	UNCORE_EVENT_CONSTRAINT(0x31, 0x3),
> 

RE: [PATCH] perf/x86: Fix overlap counter scheduling bug

[Liang, Kan]

> >
> >
> > diff --git a/arch/x86/events/intel/uncore_snbep.c
> > b/arch/x86/events/intel/uncore_snbep.c
> > index 272427700d48..71bc348736bd 100644
> > --- a/arch/x86/events/intel/uncore_snbep.c
> > +++ b/arch/x86/events/intel/uncore_snbep.c
> > @@ -669,7 +669,7 @@ static struct event_constraint
> snbep_uncore_cbox_constraints[] = {
> >  	UNCORE_EVENT_CONSTRAINT(0x1c, 0xc),
> >  	UNCORE_EVENT_CONSTRAINT(0x1d, 0xc),
> >  	UNCORE_EVENT_CONSTRAINT(0x1e, 0xc),
> > -	EVENT_CONSTRAINT_OVERLAP(0x1f, 0xe, 0xff),
> > +	UNCORE_EVENT_CONSTRAINT(0x1f, 0xc); /* should be 0x0e but that
> gives
> > +scheduling pain */

I think the crash is caused by the overlap bit.
Why not just revert the previous patch?

If overlap bit is removed, the perf_sched_save_state will never be touched.
Why we have to reduce a counter?

Thanks,
Kan

> >  	UNCORE_EVENT_CONSTRAINT(0x21, 0x3),
> >  	UNCORE_EVENT_CONSTRAINT(0x23, 0x3),
> >  	UNCORE_EVENT_CONSTRAINT(0x31, 0x3),
> >

Re: [PATCH] perf/x86: Fix overlap counter scheduling bug

[Peter Zijlstra]

On Tue, Nov 08, 2016 at 04:22:13PM +0000, Liang, Kan wrote:
> 
> 
> > >
> > >
> > > diff --git a/arch/x86/events/intel/uncore_snbep.c
> > > b/arch/x86/events/intel/uncore_snbep.c
> > > index 272427700d48..71bc348736bd 100644
> > > --- a/arch/x86/events/intel/uncore_snbep.c
> > > +++ b/arch/x86/events/intel/uncore_snbep.c
> > > @@ -669,7 +669,7 @@ static struct event_constraint
> > snbep_uncore_cbox_constraints[] = {
> > >  	UNCORE_EVENT_CONSTRAINT(0x1c, 0xc),
> > >  	UNCORE_EVENT_CONSTRAINT(0x1d, 0xc),
> > >  	UNCORE_EVENT_CONSTRAINT(0x1e, 0xc),
> > > -	EVENT_CONSTRAINT_OVERLAP(0x1f, 0xe, 0xff),
> > > +	UNCORE_EVENT_CONSTRAINT(0x1f, 0xc); /* should be 0x0e but that
> > gives
> > > +scheduling pain */
> 
> I think the crash is caused by the overlap bit.
> Why not just revert the previous patch?
> 
> If overlap bit is removed, the perf_sched_save_state will never be touched.
> Why we have to reduce a counter?

By simply removing the overlap bit you'll still get bad scheduling
(we'll just not crash).

I think all the 0x3 mask need the overlap flag set, since they clearly
overlap with the 0x1 masks. That would improve the scheduling.

But as Jiri noted, you cannot do 0x1 + 0x3 + 0xc + 0xe without also
raising the retry limit, because that are 4 overlapping masks, you'll
have to, worst case, pop 3 attempts.

By reducing 0xe to 0xc you'll not have 4 overlapping masks anymore.

In any case, overlapping masks stink (because they make scheduling
O(n!)) and ideally hardware would not do this.

RE: [PATCH] perf/x86: Fix overlap counter scheduling bug

[Liang, Kan]

> >
> > > >
> > > >
> > > > diff --git a/arch/x86/events/intel/uncore_snbep.c
> > > > b/arch/x86/events/intel/uncore_snbep.c
> > > > index 272427700d48..71bc348736bd 100644
> > > > --- a/arch/x86/events/intel/uncore_snbep.c
> > > > +++ b/arch/x86/events/intel/uncore_snbep.c
> > > > @@ -669,7 +669,7 @@ static struct event_constraint
> > > snbep_uncore_cbox_constraints[] = {
> > > >  	UNCORE_EVENT_CONSTRAINT(0x1c, 0xc),
> > > >  	UNCORE_EVENT_CONSTRAINT(0x1d, 0xc),
> > > >  	UNCORE_EVENT_CONSTRAINT(0x1e, 0xc),
> > > > -	EVENT_CONSTRAINT_OVERLAP(0x1f, 0xe, 0xff),
> > > > +	UNCORE_EVENT_CONSTRAINT(0x1f, 0xc); /* should be 0x0e but that
> > > gives
> > > > +scheduling pain */
> >
> > I think the crash is caused by the overlap bit.
> > Why not just revert the previous patch?
> >
> > If overlap bit is removed, the perf_sched_save_state will never be
> touched.
> > Why we have to reduce a counter?
> 
> By simply removing the overlap bit you'll still get bad scheduling (we'll just
> not crash).

But in some cases, it may be good to have 0xe.
For example,
uncore_cbox_0/event=0x1d/,uncore_cbox_0/event=0x1e/,uncore_cbox_0/event=0x1f/
events 1d and 1e have constraint 0xc.

> 
> I think all the 0x3 mask need the overlap flag set, since they clearly overlap
> with the 0x1 masks. That would improve the scheduling.
>

How much the overlap hint can improve the scheduling?
Because there is not only snbep_uncore_cbox, but also other uncore events
which have overlapping masks.
If it's a significant improvement, I need to set overlap flag for all of them.  

 
Thanks,
Kan
> But as Jiri noted, you cannot do 0x1 + 0x3 + 0xc + 0xe without also raising
> the retry limit, because that are 4 overlapping masks, you'll have to, worst
> case, pop 3 attempts.
> 
> By reducing 0xe to 0xc you'll not have 4 overlapping masks anymore.
> 
> In any case, overlapping masks stink (because they make scheduling
> O(n!)) and ideally hardware would not do this.

Re: [PATCH] perf/x86: Fix overlap counter scheduling bug

[Peter Zijlstra]

On Tue, Nov 08, 2016 at 05:25:34PM +0000, Liang, Kan wrote:

> > I think all the 0x3 mask need the overlap flag set, since they clearly overlap
> > with the 0x1 masks. That would improve the scheduling.
> >
> 
> How much the overlap hint can improve the scheduling?
> Because there is not only snbep_uncore_cbox, but also other uncore events
> which have overlapping masks.


Hurm, not much. We're saved by the fact that we schedule from wwin to
wmax, which means that we first place the 0x01 events, and then try and
fit the 0x03 events on top. That should already be good.

/me ponders more..

The comment with EVENT_CONSTRAINT_OVERLAP states: "This is the case if
the counter mask of such an event is not a subset of any other counter
mask of a constraint with an equal or higher weight".

Esp. that latter part is of interest here I think, our overlapping mask
is 0x0e, that has 3 bits set and is the highest weight mask in on the
PMU, therefore it will be placed last. Can we still create a scenario
where we would need to rewind that?

The scenario for AMD Fam15h is we're having masks like:

	0x3F -- 111111
	0x38 -- 111000
	0x07 -- 000111

	0x09 -- 001001

And we mark 0x09 as overlapping, because it is not a direct subset of
0x38 or 0x07 and has less weight than either of those. This means we'll
first try and place the 0x09 event, then try and place 0x38/0x07 events.
Now imagine we have:

	3 * 0x07 + 0x09

and the initial pick for the 0x09 event is counter 0, then we'll fail to
place all 0x07 events. So we'll pop back, try counter 4 for the 0x09
event, and then re-try all 0x07 events, which will now work.



But given, that in the uncore case, the overlapping event is the
heaviest mask, I don't think this can happen. Or did I overlook
something.... takes a bit to page all this back in.

Re: [PATCH] perf/x86: Fix overlap counter scheduling bug

[Robert Richter]

On 08.11.16 19:27:39, Peter Zijlstra wrote:
> The comment with EVENT_CONSTRAINT_OVERLAP states: "This is the case if
> the counter mask of such an event is not a subset of any other counter
> mask of a constraint with an equal or higher weight".
> 
> Esp. that latter part is of interest here I think, our overlapping mask
> is 0x0e, that has 3 bits set and is the highest weight mask in on the
> PMU, therefore it will be placed last. Can we still create a scenario
> where we would need to rewind that?
> 
> The scenario for AMD Fam15h is we're having masks like:
> 
> 	0x3F -- 111111
> 	0x38 -- 111000
> 	0x07 -- 000111
> 
> 	0x09 -- 001001
> 
> And we mark 0x09 as overlapping, because it is not a direct subset of
> 0x38 or 0x07 and has less weight than either of those. This means we'll
> first try and place the 0x09 event, then try and place 0x38/0x07 events.
> Now imagine we have:
> 
> 	3 * 0x07 + 0x09
> 
> and the initial pick for the 0x09 event is counter 0, then we'll fail to
> place all 0x07 events. So we'll pop back, try counter 4 for the 0x09
> event, and then re-try all 0x07 events, which will now work.
> 
> 
> 
> But given, that in the uncore case, the overlapping event is the
> heaviest mask, I don't think this can happen. Or did I overlook
> something.... takes a bit to page all this back in.

Right, IMO 0xE mask may not be marked as overlapping. It is placed
last and if there is no space left we are lost. There is no other
combination or state we could try then. So the fix is to remove the
overlapping bit for that counter, the state is then never saved.

This assumes there are no other counters than 0x3 and 0xc that overlap
with 0xe. It becomes a bit tricky if there is another counter with the
same or higher weight that overlaps with 0xe, e.g. 0x7.

-Robert

Re: [PATCH] perf/x86: Fix overlap counter scheduling bug

[Peter Zijlstra]

On Wed, Nov 09, 2016 at 03:25:15PM +0100, Robert Richter wrote:
> On 08.11.16 19:27:39, Peter Zijlstra wrote:
> > The comment with EVENT_CONSTRAINT_OVERLAP states: "This is the case if
> > the counter mask of such an event is not a subset of any other counter
> > mask of a constraint with an equal or higher weight".
> > 
> > Esp. that latter part is of interest here I think, our overlapping mask
> > is 0x0e, that has 3 bits set and is the highest weight mask in on the
> > PMU, therefore it will be placed last. Can we still create a scenario
> > where we would need to rewind that?
> > 
> > The scenario for AMD Fam15h is we're having masks like:
> > 
> > 	0x3F -- 111111
> > 	0x38 -- 111000
> > 	0x07 -- 000111
> > 
> > 	0x09 -- 001001
> > 
> > And we mark 0x09 as overlapping, because it is not a direct subset of
> > 0x38 or 0x07 and has less weight than either of those. This means we'll
> > first try and place the 0x09 event, then try and place 0x38/0x07 events.
> > Now imagine we have:
> > 
> > 	3 * 0x07 + 0x09
> > 
> > and the initial pick for the 0x09 event is counter 0, then we'll fail to
> > place all 0x07 events. So we'll pop back, try counter 4 for the 0x09
> > event, and then re-try all 0x07 events, which will now work.
> > 
> > 
> > 
> > But given, that in the uncore case, the overlapping event is the
> > heaviest mask, I don't think this can happen. Or did I overlook
> > something.... takes a bit to page all this back in.
> 
> Right, IMO 0xE mask may not be marked as overlapping. It is placed
> last and if there is no space left we are lost. There is no other
> combination or state we could try then. So the fix is to remove the
> overlapping bit for that counter, the state is then never saved.
> 
> This assumes there are no other counters than 0x3 and 0xc that overlap
> with 0xe. It becomes a bit tricky if there is another counter with the
> same or higher weight that overlaps with 0xe, e.g. 0x7.

As per a prior mail, the masks on the PMU in question are:

 0x01 - 0001
 0x03 - 0011
 0x0e - 1110
 0x0c - 1100

But since all the masks that have overlap (0xe -> {0xc,0x3}) and (0x3 ->
0x1) are of heavier weight, it should all work out I think.

So yes, something like the below (removing the OVERLAP bit) looks like
its sufficient.

---
 arch/x86/events/intel/uncore_snbep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/events/intel/uncore_snbep.c b/arch/x86/events/intel/uncore_snbep.c
index 272427700d48..e6832be714bc 100644
--- a/arch/x86/events/intel/uncore_snbep.c
+++ b/arch/x86/events/intel/uncore_snbep.c
@@ -669,7 +669,7 @@ static struct event_constraint snbep_uncore_cbox_constraints[] = {
 	UNCORE_EVENT_CONSTRAINT(0x1c, 0xc),
 	UNCORE_EVENT_CONSTRAINT(0x1d, 0xc),
 	UNCORE_EVENT_CONSTRAINT(0x1e, 0xc),
-	EVENT_CONSTRAINT_OVERLAP(0x1f, 0xe, 0xff),
+	UNCORE_EVENT_CONSTRAINT(0x1f, 0xe),
 	UNCORE_EVENT_CONSTRAINT(0x21, 0x3),
 	UNCORE_EVENT_CONSTRAINT(0x23, 0x3),
 	UNCORE_EVENT_CONSTRAINT(0x31, 0x3),

Re: [PATCH] perf/x86: Fix overlap counter scheduling bug

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Wed, Nov 09, 2016 at 03:25:15PM +0100, Robert Richter wrote:
> > On 08.11.16 19:27:39, Peter Zijlstra wrote:
> > > The comment with EVENT_CONSTRAINT_OVERLAP states: "This is the case if
> > > the counter mask of such an event is not a subset of any other counter
> > > mask of a constraint with an equal or higher weight".
> > > 
> > > Esp. that latter part is of interest here I think, our overlapping mask
> > > is 0x0e, that has 3 bits set and is the highest weight mask in on the
> > > PMU, therefore it will be placed last. Can we still create a scenario
> > > where we would need to rewind that?
> > > 
> > > The scenario for AMD Fam15h is we're having masks like:
> > > 
> > > 	0x3F -- 111111
> > > 	0x38 -- 111000
> > > 	0x07 -- 000111
> > > 
> > > 	0x09 -- 001001
> > > 
> > > And we mark 0x09 as overlapping, because it is not a direct subset of
> > > 0x38 or 0x07 and has less weight than either of those. This means we'll
> > > first try and place the 0x09 event, then try and place 0x38/0x07 events.
> > > Now imagine we have:
> > > 
> > > 	3 * 0x07 + 0x09
> > > 
> > > and the initial pick for the 0x09 event is counter 0, then we'll fail to
> > > place all 0x07 events. So we'll pop back, try counter 4 for the 0x09
> > > event, and then re-try all 0x07 events, which will now work.
> > > 
> > > 
> > > 
> > > But given, that in the uncore case, the overlapping event is the
> > > heaviest mask, I don't think this can happen. Or did I overlook
> > > something.... takes a bit to page all this back in.
> > 
> > Right, IMO 0xE mask may not be marked as overlapping. It is placed
> > last and if there is no space left we are lost. There is no other
> > combination or state we could try then. So the fix is to remove the
> > overlapping bit for that counter, the state is then never saved.
> > 
> > This assumes there are no other counters than 0x3 and 0xc that overlap
> > with 0xe. It becomes a bit tricky if there is another counter with the
> > same or higher weight that overlaps with 0xe, e.g. 0x7.
> 
> As per a prior mail, the masks on the PMU in question are:
> 
>  0x01 - 0001
>  0x03 - 0011
>  0x0e - 1110
>  0x0c - 1100
> 
> But since all the masks that have overlap (0xe -> {0xc,0x3}) and (0x3 ->
> 0x1) are of heavier weight, it should all work out I think.
> 
> So yes, something like the below (removing the OVERLAP bit) looks like
> its sufficient.

Would it be possible to also add debug code (or some other mechanism) to disallow 
such buggy EVENT_CONSTRAINT_OVERLAP() definitions?

Thanks,

	Ingo

Re: [PATCH] perf/x86: Fix overlap counter scheduling bug

[Peter Zijlstra]

On Thu, Nov 10, 2016 at 09:00:13AM +0100, Ingo Molnar wrote:
> Would it be possible to also add debug code (or some other mechanism) to disallow
> such buggy EVENT_CONSTRAINT_OVERLAP() definitions?

Should certainly be possible if someone has the time for it I think. The
rules are fairly straight forward, the only tricky bit is detectoring
the actual overlap condition.

Re: [PATCH] perf/x86: Fix overlap counter scheduling bug

[Jiri Olsa]

On Wed, Nov 09, 2016 at 04:51:53PM +0100, Peter Zijlstra wrote:

SNIP

> 
> As per a prior mail, the masks on the PMU in question are:
> 
>  0x01 - 0001
>  0x03 - 0011
>  0x0e - 1110
>  0x0c - 1100
> 
> But since all the masks that have overlap (0xe -> {0xc,0x3}) and (0x3 ->
> 0x1) are of heavier weight, it should all work out I think.
> 
> So yes, something like the below (removing the OVERLAP bit) looks like
> its sufficient.

Peter,
could you please take this one?

thanks,
jirka

> 
> ---
>  arch/x86/events/intel/uncore_snbep.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/events/intel/uncore_snbep.c b/arch/x86/events/intel/uncore_snbep.c
> index 272427700d48..e6832be714bc 100644
> --- a/arch/x86/events/intel/uncore_snbep.c
> +++ b/arch/x86/events/intel/uncore_snbep.c
> @@ -669,7 +669,7 @@ static struct event_constraint snbep_uncore_cbox_constraints[] = {
>  	UNCORE_EVENT_CONSTRAINT(0x1c, 0xc),
>  	UNCORE_EVENT_CONSTRAINT(0x1d, 0xc),
>  	UNCORE_EVENT_CONSTRAINT(0x1e, 0xc),
> -	EVENT_CONSTRAINT_OVERLAP(0x1f, 0xe, 0xff),
> +	UNCORE_EVENT_CONSTRAINT(0x1f, 0xe),
>  	UNCORE_EVENT_CONSTRAINT(0x21, 0x3),
>  	UNCORE_EVENT_CONSTRAINT(0x23, 0x3),
>  	UNCORE_EVENT_CONSTRAINT(0x31, 0x3),

[tip:perf/urgent] perf/x86: Fix overlap counter scheduling bug

[tip-bot for Peter Zijlstra]

Commit-ID:  1134c2b5cb840409ffd966d8c2a9468f64e6a494
Gitweb:     http://git.kernel.org/tip/1134c2b5cb840409ffd966d8c2a9468f64e6a494
Author:     Peter Zijlstra <peterz@infradead.org>
AuthorDate: Wed, 9 Nov 2016 16:51:53 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 22 Dec 2016 17:45:43 +0100

perf/x86: Fix overlap counter scheduling bug

Jiri reported the overlap scheduling exceeding its max stack.

Looking at the constraint that triggered this, it turns out the
overlap marker isn't needed.

The comment with EVENT_CONSTRAINT_OVERLAP states: "This is the case if
the counter mask of such an event is not a subset of any other counter
mask of a constraint with an equal or higher weight".

Esp. that latter part is of interest here I think, our overlapping mask
is 0x0e, that has 3 bits set and is the highest weight mask in on the
PMU, therefore it will be placed last. Can we still create a scenario
where we would need to rewind that?

The scenario for AMD Fam15h is we're having masks like:

	0x3F -- 111111
	0x38 -- 111000
	0x07 -- 000111

	0x09 -- 001001

And we mark 0x09 as overlapping, because it is not a direct subset of
0x38 or 0x07 and has less weight than either of those. This means we'll
first try and place the 0x09 event, then try and place 0x38/0x07 events.
Now imagine we have:

	3 * 0x07 + 0x09

and the initial pick for the 0x09 event is counter 0, then we'll fail to
place all 0x07 events. So we'll pop back, try counter 4 for the 0x09
event, and then re-try all 0x07 events, which will now work.

The masks on the PMU in question are:

  0x01 - 0001
  0x03 - 0011
  0x0e - 1110
  0x0c - 1100

But since all the masks that have overlap (0xe -> {0xc,0x3}) and (0x3 ->
0x1) are of heavier weight, it should all work out.

Reported-by: Jiri Olsa <jolsa@kernel.org>
Tested-by: Jiri Olsa <jolsa@kernel.org>
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: Jiri Olsa <jolsa@redhat.com>
Cc: Liang Kan <kan.liang@intel.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Robert Richter <rric@kernel.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/20161109155153.GQ3142@twins.programming.kicks-ass.net
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/events/intel/uncore_snbep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/events/intel/uncore_snbep.c b/arch/x86/events/intel/uncore_snbep.c
index 2724277..e6832be 100644
--- a/arch/x86/events/intel/uncore_snbep.c
+++ b/arch/x86/events/intel/uncore_snbep.c
@@ -669,7 +669,7 @@ static struct event_constraint snbep_uncore_cbox_constraints[] = {
 	UNCORE_EVENT_CONSTRAINT(0x1c, 0xc),
 	UNCORE_EVENT_CONSTRAINT(0x1d, 0xc),
 	UNCORE_EVENT_CONSTRAINT(0x1e, 0xc),
-	EVENT_CONSTRAINT_OVERLAP(0x1f, 0xe, 0xff),
+	UNCORE_EVENT_CONSTRAINT(0x1f, 0xe),
 	UNCORE_EVENT_CONSTRAINT(0x21, 0x3),
 	UNCORE_EVENT_CONSTRAINT(0x23, 0x3),
 	UNCORE_EVENT_CONSTRAINT(0x31, 0x3),