From: "Liang, Kan" <kan.liang@intel.com>
To: Peter Zijlstra <peterz@infradead.org>
CC: Andi Kleen <andi@firstfloor.org>, Jiri Olsa <jolsa@kernel.org>,
        "Vince Weaver" <vince@deater.net>, Robert Richter <rric@kernel.org>,
        lkml <linux-kernel@vger.kernel.org>, Ingo Molnar <mingo@kernel.org>
Subject: RE: [PATCH] perf/x86: Fix overlap counter scheduling bug
Thread-Topic: [PATCH] perf/x86: Fix overlap counter scheduling bug
Thread-Index: AQHSOdIsiqKg0s6PyEOKEiPKy1JRqqDPPNCQ//+L6ICAAIh+wA==
Date: Tue, 8 Nov 2016 17:25:34 +0000
Message-ID: <37D7C6CF3E00A74B8858931C1DB2F07750C8DA4F@SHSMSX103.ccr.corp.intel.com>
References: <1478015068-14052-1-git-send-email-jolsa@kernel.org>
 <20161108122039.GP3142@twins.programming.kicks-ass.net>
 <20161108150949.GM26852@two.firstfloor.org>
 <37D7C6CF3E00A74B8858931C1DB2F07750C8D9D0@SHSMSX103.ccr.corp.intel.com>
 <20161108165749.GJ3117@twins.programming.kicks-ass.net>
In-Reply-To: <20161108165749.GJ3117@twins.programming.kicks-ass.net>
Accept-Language: zh-CN, en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 8BIT
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1952
Lines: 54

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