From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934098Ab2FENFD (ORCPT ); Tue, 5 Jun 2012 09:05:03 -0400 Received: from merlin.infradead.org ([205.233.59.134]:37462 "EHLO merlin.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932429Ab2FENFA convert rfc822-to-8bit (ORCPT ); Tue, 5 Jun 2012 09:05:00 -0400 Message-ID: <1338901490.28282.167.camel@twins> Subject: Re: [PATCH] perf: Fix intel shared extra msr allocation From: Peter Zijlstra To: Stephane Eranian Cc: "Yan, Zheng" , "Yan, Zheng" , linux-kernel@vger.kernel.org Date: Tue, 05 Jun 2012 15:04:50 +0200 In-Reply-To: References: <1338520856-21020-1-git-send-email-zheng.z.yan@intel.com> <1338891271.28282.155.camel@twins> <1338892071.28282.157.camel@twins> <1338898024.28282.160.camel@twins> <1338899999.28282.164.camel@twins> Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7BIT X-Mailer: Evolution 3.2.2- Mime-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 2012-06-05 at 14:51 +0200, Stephane Eranian wrote: > So I think if you say in: > __intel_shared_reg_put_constraints() > > if (!cpuc->is_fake) > reg->alloc = 1; > > That should do it given that: > > __intel_shared_reg_put_constraints(struct cpu_hw_events *cpuc, > struct hw_perf_event_extra *reg) > { > struct er_account *era; > if (!reg->alloc) > return; > > } > > But then, if reg->alloc was already set to 1, you will have a problem > if you leave > it as is. So I think in __intel_shared_reg_put_constraints(), you still need: > > __intel_shared_reg_put_constraints(struct cpu_hw_events *cpuc, > struct hw_perf_event_extra *reg) > { > if (!reg->alloc || cpuc->is_fake) > return; > > Or something like that. Right, and then something slightly less hideous for intel_try_alt_er(). How does something like this look? --- arch/x86/kernel/cpu/perf_event_intel.c | 43 +++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 15 deletions(-) diff --git a/arch/x86/kernel/cpu/perf_event_intel.c b/arch/x86/kernel/cpu/perf_event_intel.c index 166546e..a3b7eb3 100644 --- a/arch/x86/kernel/cpu/perf_event_intel.c +++ b/arch/x86/kernel/cpu/perf_event_intel.c @@ -1119,27 +1119,33 @@ intel_bts_constraints(struct perf_event *event) return NULL; } -static bool intel_try_alt_er(struct perf_event *event, int orig_idx) +static int intel_alt_er(int idx) { if (!(x86_pmu.er_flags & ERF_HAS_RSP_1)) - return false; + return idx; - if (event->hw.extra_reg.idx == EXTRA_REG_RSP_0) { + if (idx == EXTRA_REG_RSP_0) + return EXTRA_REG_RSP_1; + + if (idx == EXTRA_REG_RSP_1) + return EXTRA_REG_RSP_0; + + return idx; +} + +static void intel_fixup_er(struct perf_event *event, int idx) +{ + if (idx == EXTRA_REG_RSP_0) { event->hw.config &= ~INTEL_ARCH_EVENT_MASK; event->hw.config |= 0x01bb; event->hw.extra_reg.idx = EXTRA_REG_RSP_1; event->hw.extra_reg.reg = MSR_OFFCORE_RSP_1; - } else if (event->hw.extra_reg.idx == EXTRA_REG_RSP_1) { + } else if (idx == EXTRA_REG_RSP_1) { event->hw.config &= ~INTEL_ARCH_EVENT_MASK; event->hw.config |= 0x01b7; event->hw.extra_reg.idx = EXTRA_REG_RSP_0; event->hw.extra_reg.reg = MSR_OFFCORE_RSP_0; } - - if (event->hw.extra_reg.idx == orig_idx) - return false; - - return true; } /* @@ -1157,14 +1163,14 @@ __intel_shared_reg_get_constraints(struct cpu_hw_events *cpuc, struct event_constraint *c = &emptyconstraint; struct er_account *era; unsigned long flags; - int orig_idx = reg->idx; + int idx = reg->idx; /* already allocated shared msr */ if (reg->alloc) return NULL; /* call x86_get_event_constraint() */ again: - era = &cpuc->shared_regs->regs[reg->idx]; + era = &cpuc->shared_regs->regs[idx]; /* * we use spin_lock_irqsave() to avoid lockdep issues when * passing a fake cpuc @@ -1181,16 +1187,23 @@ __intel_shared_reg_get_constraints(struct cpu_hw_events *cpuc, atomic_inc(&era->ref); /* no need to reallocate during incremental event scheduling */ - reg->alloc = 1; + if (!cpuc->is_fake) { + if (idx != reg->idx) + intel_fixup_er(event, idx); + reg->alloc = 1; + } /* * need to call x86_get_event_constraint() * to check if associated event has constraints */ c = NULL; - } else if (intel_try_alt_er(event, orig_idx)) { - raw_spin_unlock_irqrestore(&era->lock, flags); - goto again; + } else { + idx = intel_alt_er(idx); + if (idx != reg->idx) { + raw_spin_unlock_irqrestore(&era->lock, flags); + goto again; + } } raw_spin_unlock_irqrestore(&era->lock, flags);