From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754202AbbFOKsh (ORCPT ); Mon, 15 Jun 2015 06:48:37 -0400 Received: from bombadil.infradead.org ([198.137.202.9]:39061 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753425AbbFOKs2 (ORCPT ); Mon, 15 Jun 2015 06:48:28 -0400 Date: Mon, 15 Jun 2015 12:48:13 +0200 From: Peter Zijlstra To: Andi Kleen Cc: acme@kernel.org, linux-kernel@vger.kernel.org, jolsa@redhat.com, eranian@google.com, Andi Kleen Subject: Re: [PATCH 2/5] x86, perf: Add option to disable reading branch flags/cycles Message-ID: <20150615104813.GU3644@twins.programming.kicks-ass.net> References: <1432786398-23861-1-git-send-email-andi@firstfloor.org> <1432786398-23861-3-git-send-email-andi@firstfloor.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1432786398-23861-3-git-send-email-andi@firstfloor.org> User-Agent: Mutt/1.5.21 (2012-12-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, May 27, 2015 at 09:13:15PM -0700, Andi Kleen wrote: > From: Andi Kleen > > With LBRv5 reading the extra LBR flags like mispredict, TSX, cycles > is not free anymore, as it has moved to a separate MSR. > > For callstack mode we don't need any of this information; so we can > avoid the unnecessary MSR read. Add flags to the perf interface > where perf record can request not collecting this information. > I added sample_type flags for CYCLES and FLAGS. It's a bit unusual for > sample_types to be negative (disable), not positive (enable), but > since the legacy ABI reported the flags we need some form of explicit > disabling to avoid breaking the ABI. In theory it would be possible > to make CYCLES opt-in (as it's not deployed yet), but I also made it > opt-out to be symmetric to FLAGS. > > After we have the flags the x86 perf code can keep track if any > users need the flags. If noone needs it the information is not > collected. > diff --git a/arch/x86/kernel/cpu/perf_event_intel_lbr.c b/arch/x86/kernel/cpu/perf_event_intel_lbr.c > index 1fd8b5a..5de2048 100644 > --- a/arch/x86/kernel/cpu/perf_event_intel_lbr.c > +++ b/arch/x86/kernel/cpu/perf_event_intel_lbr.c > @@ -340,6 +340,10 @@ void intel_pmu_lbr_enable(struct perf_event *event) > } > > cpuc->lbr_users++; > + if (!(event->attr.sample_type & PERF_SAMPLE_BRANCH_NO_FLAGS)) > + cpuc->lbr_flags_users++; > + if (!(event->attr.sample_type & PERF_SAMPLE_BRANCH_NO_CYCLES)) > + cpuc->lbr_cycles_users++; > perf_sched_cb_inc(event->ctx->pmu); > } This patch seems to add an unfortunate amount of branches. And while I appreciate it'll all be cheaper than the SKL MSR read, it does add overhead to the older chips. Also, I think its broken. We can now have two events, with the 'same' LBR config but with different flags/cycles settings. __intel_shared_reg_get_constraints() will find a match, but the enable code above will disable flags/cycles for both of them. > diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h > index 1b8bd4a..8dd5765 100644 > --- a/include/uapi/linux/perf_event.h > +++ b/include/uapi/linux/perf_event.h > @@ -138,6 +138,8 @@ enum perf_event_sample_format { > PERF_SAMPLE_IDENTIFIER = 1U << 16, > PERF_SAMPLE_TRANSACTION = 1U << 17, > PERF_SAMPLE_REGS_INTR = 1U << 18, > + PERF_SAMPLE_BRANCH_NO_FLAGS = 1U << 19, > + PERF_SAMPLE_BRANCH_NO_CYCLES = 1U << 20, > > PERF_SAMPLE_MAX = 1U << 19, /* non-ABI */ > }; Should this not be part of perf_branch_sample_type instead? That seems to otherwise specify all the branch stack details.