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]

Message-ID: <20161108165749.GJ3117@twins.programming.kicks-ass.net>
Date:   Tue, 8 Nov 2016 17:57:49 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     "Liang, Kan" <kan.liang@...el.com>
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

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.