MIME-Version: 1.0
In-Reply-To: <20150526132221.GP3644@twins.programming.kicks-ass.net>
References: <20150522132905.416122812@infradead.org>
	<20150522133135.353044581@infradead.org>
	<20150522134056.GG3644@twins.programming.kicks-ass.net>
	<CABPqkBTzQ0z6erv4dnotFKu=KystkSRJG2ovxBhsp5vFFFufqg@mail.gmail.com>
	<20150526101237.GK3644@twins.programming.kicks-ass.net>
	<CABPqkBQcKLoHCS3=_1cj4-JCTiOzWDc1=vGp85T-nnoAJQd5-Q@mail.gmail.com>
	<20150526121619.GN3644@twins.programming.kicks-ass.net>
	<CABPqkBQbw=vx+hFX+gAFQRkHpkpzz6meucpdo4eMfQ6Xu_XzvA@mail.gmail.com>
	<20150526132221.GP3644@twins.programming.kicks-ass.net>
Date: Tue, 26 May 2015 06:44:31 -0700
Message-ID: <CABPqkBTm7L-dqjEz6ECuXsJNLReC+zuu1uu6ceZkpksTUTaycg@mail.gmail.com>
Subject: Re: [PATCH v2 01/11] perf,x86: Fix event/group validation
From: Stephane Eranian <eranian@google.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@kernel.org>, Vince Weaver <vincent.weaver@maine.edu>,
        Jiri Olsa <jolsa@redhat.com>, "Liang, Kan" <kan.liang@intel.com>,
        LKML <linux-kernel@vger.kernel.org>, Andrew Hunter <ahh@google.com>,
        Maria Dimakopoulou <maria.n.dimakopoulou@gmail.com>
Content-Type: text/plain; charset=UTF-8
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1914
Lines: 40

On Tue, May 26, 2015 at 6:22 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Tue, May 26, 2015 at 05:25:59AM -0700, Stephane Eranian wrote:
>> > IIRC the problem was that the copy from c2 into c1:
>> >
>> >         if (c1 && (c1->flags & PERF_X86_EVENT_DYNAMIC)) {
>> >                 bitmap_copy(c1->idxmsk, c2->idxmsk, X86_PMC_IDX_MAX);
>> >                 c1->weight = c2->weight;
>> >                 c2 = c1;
>> >         }
>> >
>> > is incomplete. For instance, flags is not copied, and some code down the
>> > line might check that and get wrong flags.
>> >
>> Ok, now I remember this code. It has to do with incremental scheduling.
>> Suppose E1, E2, E3 events where E1 is exclusive. The first call is
>> for scheduling E1. It gets to get_event_constraint() which "allocates" a
>> dynamic constraint. The second call  tries to schedule E1, E2. But the
>> second time for E1, you already have the dynamic constraint allocated, so
>> this code is reusing the constraint storage and just updates the bitmask
>> and weight.
>>
>> Now, that the storage is not actually dynamic (kmalloc'd), but taken from a
>> fixed size array in cpuc, I believe we can simplify this and "re-allocate"
>> the constraint for each incremental call to intel_get_event_constraints().
>> Do you agree?
>
> That would probably work, the whole incremental thing seems superfluous
> to me.

Well, you could pass the whole lists (4)  of events in one call and
let the arch level
sort it out. It could do one pass and stop at first error to get a
fixed bound like
today. But it would need to deal with groups as well...so not so easy and why
repeat the group processing for each arch?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/