Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754256AbbEZNYg (ORCPT ); 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 ); 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> <20150526101237.GK3644@twins.programming.kicks-ass.net> <20150526121619.GN3644@twins.programming.kicks-ass.net> Date: Tue, 26 May 2015 05:25:59 -0700 Message-ID: Subject: Re: [PATCH v2 01/11] perf,x86: Fix event/group validation From: Stephane Eranian To: Peter Zijlstra Cc: Ingo Molnar , Vince Weaver , Jiri Olsa , "Liang, Kan" , LKML , Andrew Hunter , Maria Dimakopoulou Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2363 Lines: 50 On Tue, May 26, 2015 at 5:16 AM, Peter Zijlstra 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 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 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. > -- 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/