From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1754256AbbEZNYg (ORCPT <rfc822;w@1wt.eu>);
	Tue, 26 May 2015 09:24:36 -0400
Received: from mail-oi0-f48.google.com ([209.85.218.48]:36570 "EHLO
	mail-oi0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1753802AbbEZNYb (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Tue, 26 May 2015 09:24:31 -0400
MIME-Version: 1.0
In-Reply-To: <20150526121619.GN3644@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>
Date: Tue, 26 May 2015 05:25:59 -0700
Message-ID: <CABPqkBQbw=vx+hFX+gAFQRkHpkpzz6meucpdo4eMfQ6Xu_XzvA@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
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, May 26, 2015 at 5:16 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Tue, May 26, 2015 at 04:46:07AM -0700, Stephane Eranian wrote:
>> On Tue, May 26, 2015 at 3:12 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>> > On Tue, May 26, 2015 at 02:24:38AM -0700, Stephane Eranian wrote:
>> >> On Fri, May 22, 2015 at 6:40 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>> >> > On Fri, May 22, 2015 at 03:29:06PM +0200, Peter Zijlstra wrote:
>> >> >> @@ -788,9 +788,9 @@ int x86_schedule_events(struct cpu_hw_ev
>> >> >>               x86_pmu.start_scheduling(cpuc);
>> >> >>
>> >> >>       for (i = 0, wmin = X86_PMC_IDX_MAX, wmax = 0; i < n; i++) {
>> >> >> +             cpuc->event_constraint[i] = NULL;
>> >> >
>
>> But where is the code that says: skip reinstalling the constraint
>> in intel_get_event_constraints() because there is already a (stale)
>> one? I don't see where that is.
>
> 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?


> I'm not entirely sure I saw misbehaviour, but I figured I'd better close
> that hole and rule out this is contributing to fail.
>