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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 8 Nov 2016 17:25:34 +0000
From:   "Liang, Kan" <kan.liang@...el.com>
To:     Peter Zijlstra <peterz@...radead.org>
CC:     Andi Kleen <andi@...stfloor.org>, Jiri Olsa <jolsa@...nel.org>,
        "Vince Weaver" <vince@...ter.net>,
        Robert Richter <rric@...nel.org>,
        lkml <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>
Subject: RE: [PATCH] perf/x86: Fix overlap counter scheduling bug

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