* [PATCH] perf/x86: Fix overlap counter scheduling bug @ 2016-11-01 15:44 Jiri Olsa 2016-11-08 12:20 ` Peter Zijlstra 0 siblings, 1 reply; 14+ messages in thread From: Jiri Olsa @ 2016-11-01 15:44 UTC (permalink / raw) To: Peter Zijlstra Cc: Vince Weaver, Robert Richter, Yan Zheng, lkml, Ingo Molnar, Andi Kleen 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] perf/x86: Fix overlap counter scheduling bug 2016-11-01 15:44 [PATCH] perf/x86: Fix overlap counter scheduling bug Jiri Olsa @ 2016-11-08 12:20 ` Peter Zijlstra 2016-11-08 13:14 ` Jiri Olsa 2016-11-08 15:09 ` Andi Kleen 0 siblings, 2 replies; 14+ messages in thread From: Peter Zijlstra @ 2016-11-08 12:20 UTC (permalink / raw) To: Jiri Olsa Cc: Vince Weaver, Robert Richter, Yan Zheng, lkml, Ingo Molnar, Andi Kleen 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), ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] perf/x86: Fix overlap counter scheduling bug 2016-11-08 12:20 ` Peter Zijlstra @ 2016-11-08 13:14 ` Jiri Olsa 2016-11-08 15:09 ` Andi Kleen 1 sibling, 0 replies; 14+ messages in thread From: Jiri Olsa @ 2016-11-08 13:14 UTC (permalink / raw) To: Peter Zijlstra Cc: Jiri Olsa, Vince Weaver, Robert Richter, Yan Zheng, lkml, Ingo Molnar, Andi Kleen 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), ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] perf/x86: Fix overlap counter scheduling bug 2016-11-08 12:20 ` Peter Zijlstra 2016-11-08 13:14 ` Jiri Olsa @ 2016-11-08 15:09 ` Andi Kleen 2016-11-08 16:22 ` Liang, Kan 1 sibling, 1 reply; 14+ messages in thread From: Andi Kleen @ 2016-11-08 15:09 UTC (permalink / raw) To: Peter Zijlstra Cc: Jiri Olsa, Vince Weaver, Robert Richter, lkml, Ingo Molnar, Andi Kleen, kan.liang 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), > ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH] perf/x86: Fix overlap counter scheduling bug 2016-11-08 15:09 ` Andi Kleen @ 2016-11-08 16:22 ` Liang, Kan 2016-11-08 16:57 ` Peter Zijlstra 0 siblings, 1 reply; 14+ messages in thread From: Liang, Kan @ 2016-11-08 16:22 UTC (permalink / raw) To: Andi Kleen, Peter Zijlstra Cc: Jiri Olsa, Vince Weaver, Robert Richter, lkml, Ingo Molnar > > > > > > 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), > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] perf/x86: Fix overlap counter scheduling bug 2016-11-08 16:22 ` Liang, Kan @ 2016-11-08 16:57 ` Peter Zijlstra 2016-11-08 17:25 ` Liang, Kan 0 siblings, 1 reply; 14+ messages in thread From: Peter Zijlstra @ 2016-11-08 16:57 UTC (permalink / raw) To: Liang, Kan Cc: Andi Kleen, Jiri Olsa, Vince Weaver, Robert Richter, lkml, Ingo Molnar 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH] perf/x86: Fix overlap counter scheduling bug 2016-11-08 16:57 ` Peter Zijlstra @ 2016-11-08 17:25 ` Liang, Kan 2016-11-08 18:27 ` Peter Zijlstra 0 siblings, 1 reply; 14+ messages in thread From: Liang, Kan @ 2016-11-08 17:25 UTC (permalink / raw) To: Peter Zijlstra Cc: Andi Kleen, Jiri Olsa, Vince Weaver, Robert Richter, lkml, Ingo Molnar > > > > > > > > > > > > > > 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] perf/x86: Fix overlap counter scheduling bug 2016-11-08 17:25 ` Liang, Kan @ 2016-11-08 18:27 ` Peter Zijlstra 2016-11-09 14:25 ` Robert Richter 0 siblings, 1 reply; 14+ messages in thread From: Peter Zijlstra @ 2016-11-08 18:27 UTC (permalink / raw) To: Liang, Kan Cc: Andi Kleen, Jiri Olsa, Vince Weaver, Robert Richter, lkml, Ingo Molnar 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] perf/x86: Fix overlap counter scheduling bug 2016-11-08 18:27 ` Peter Zijlstra @ 2016-11-09 14:25 ` Robert Richter 2016-11-09 15:51 ` Peter Zijlstra 0 siblings, 1 reply; 14+ messages in thread From: Robert Richter @ 2016-11-09 14:25 UTC (permalink / raw) To: Peter Zijlstra Cc: Liang, Kan, Andi Kleen, Jiri Olsa, Vince Weaver, lkml, Ingo Molnar 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] perf/x86: Fix overlap counter scheduling bug 2016-11-09 14:25 ` Robert Richter @ 2016-11-09 15:51 ` Peter Zijlstra 2016-11-10 8:00 ` Ingo Molnar ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: Peter Zijlstra @ 2016-11-09 15:51 UTC (permalink / raw) To: Robert Richter Cc: Liang, Kan, Andi Kleen, Jiri Olsa, Vince Weaver, lkml, Ingo Molnar 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), ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] perf/x86: Fix overlap counter scheduling bug 2016-11-09 15:51 ` Peter Zijlstra @ 2016-11-10 8:00 ` Ingo Molnar 2016-11-10 16:41 ` Peter Zijlstra 2016-12-14 15:59 ` Jiri Olsa 2016-12-22 16:50 ` [tip:perf/urgent] " tip-bot for Peter Zijlstra 2 siblings, 1 reply; 14+ messages in thread From: Ingo Molnar @ 2016-11-10 8:00 UTC (permalink / raw) To: Peter Zijlstra Cc: Robert Richter, Liang, Kan, Andi Kleen, Jiri Olsa, Vince Weaver, lkml * 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] perf/x86: Fix overlap counter scheduling bug 2016-11-10 8:00 ` Ingo Molnar @ 2016-11-10 16:41 ` Peter Zijlstra 0 siblings, 0 replies; 14+ messages in thread From: Peter Zijlstra @ 2016-11-10 16:41 UTC (permalink / raw) To: Ingo Molnar Cc: Robert Richter, Liang, Kan, Andi Kleen, Jiri Olsa, Vince Weaver, lkml 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] perf/x86: Fix overlap counter scheduling bug 2016-11-09 15:51 ` Peter Zijlstra 2016-11-10 8:00 ` Ingo Molnar @ 2016-12-14 15:59 ` Jiri Olsa 2016-12-22 16:50 ` [tip:perf/urgent] " tip-bot for Peter Zijlstra 2 siblings, 0 replies; 14+ messages in thread From: Jiri Olsa @ 2016-12-14 15:59 UTC (permalink / raw) To: Peter Zijlstra Cc: Robert Richter, Liang, Kan, Andi Kleen, Jiri Olsa, Vince Weaver, lkml, Ingo Molnar 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), ^ permalink raw reply [flat|nested] 14+ messages in thread
* [tip:perf/urgent] perf/x86: Fix overlap counter scheduling bug 2016-11-09 15:51 ` Peter Zijlstra 2016-11-10 8:00 ` Ingo Molnar 2016-12-14 15:59 ` Jiri Olsa @ 2016-12-22 16:50 ` tip-bot for Peter Zijlstra 2 siblings, 0 replies; 14+ messages in thread From: tip-bot for Peter Zijlstra @ 2016-12-22 16:50 UTC (permalink / raw) To: linux-tip-commits Cc: torvalds, jolsa, mingo, peterz, jolsa, vincent.weaver, eranian, vince, kan.liang, alexander.shishkin, linux-kernel, hpa, acme, tglx, rric 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), ^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2016-12-22 16:53 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-11-01 15:44 [PATCH] perf/x86: Fix overlap counter scheduling bug Jiri Olsa 2016-11-08 12:20 ` Peter Zijlstra 2016-11-08 13:14 ` Jiri Olsa 2016-11-08 15:09 ` Andi Kleen 2016-11-08 16:22 ` Liang, Kan 2016-11-08 16:57 ` Peter Zijlstra 2016-11-08 17:25 ` Liang, Kan 2016-11-08 18:27 ` Peter Zijlstra 2016-11-09 14:25 ` Robert Richter 2016-11-09 15:51 ` Peter Zijlstra 2016-11-10 8:00 ` Ingo Molnar 2016-11-10 16:41 ` Peter Zijlstra 2016-12-14 15:59 ` Jiri Olsa 2016-12-22 16:50 ` [tip:perf/urgent] " tip-bot for Peter Zijlstra
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).