From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751714AbdBAKRL (ORCPT ); Wed, 1 Feb 2017 05:17:11 -0500 Received: from mail-wj0-f195.google.com ([209.85.210.195]:35750 "EHLO mail-wj0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751259AbdBAKRI (ORCPT ); Wed, 1 Feb 2017 05:17:08 -0500 Date: Wed, 1 Feb 2017 11:17:04 +0100 From: Ingo Molnar To: Alexander Shishkin Cc: Peter Zijlstra , Ingo Molnar , linux-kernel@vger.kernel.org, vince@deater.net, eranian@google.com, Arnaldo Carvalho de Melo , Borislav Petkov , Thomas Gleixner Subject: Re: [PATCH 2/2] perf/x86/intel/pt: Allow disabling branch tracing Message-ID: <20170201101704.GA450@gmail.com> References: <20170127151644.8585-1-alexander.shishkin@linux.intel.com> <20170127151644.8585-3-alexander.shishkin@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170127151644.8585-3-alexander.shishkin@linux.intel.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Alexander Shishkin wrote: > Now that Intel PT supports more types of trace content than just branch > tracing, it may be useful to allow the user to disable branch tracing > when it is not needed. > > The special case is BDW, where not setting BranchEn is not supported. > > This patch adds 'no_branch' event format string to PT events, which > will disable setting BranchEn bit in the hardware trace configuration. > + /* trying to unset BRANCH_EN where it is not supported */ Please capitalize comments consistently and use the typical tense. This one should be something like: /* Try to unset BRANCH_EN where it is not supported: */ > > reg = pt_config_filters(event); > - reg |= RTIT_CTL_TOPA | RTIT_CTL_BRANCH_EN | RTIT_CTL_TRACEEN; > + reg |= RTIT_CTL_TOPA | RTIT_CTL_TRACEEN; > + > + /* > + * Previously, we had BRANCH_EN on by default, but now that PT has > + * grown features outside of branch tracing, it is useful to allow > + * the user to disable it. So, to keep compatibility, setting > + * BRANCH_EN bit in the event config (no_branch=1) will have the > + * reverse effect and *not* set BRANCH_EN in the hardware > + * configuration. > + */ > + if (!(event->attr.config & RTIT_CTL_BRANCH_EN)) > + reg |= RTIT_CTL_BRANCH_EN; > + else > + event->attr.config &= ~RTIT_CTL_BRANCH_EN; So I really hate this ABI hack - it's these unclean approaches how ABIs degrade over time, by death of a thousand cuts... Any reason why we couldn't add a separate pt_feature_branch_disable and pt_feature_trace_disable bits and interpret them in a straightforward way, or something like that? ( This means two more bits, but that's our punishment for not anticipating extensions to the hardware feature. ) Also, rename "RTIT_CTL_BRANCH_EN" to "RTIT_CTL_PT_EN" (but without changing the ABI), to more clearly express what that bit realy does. I.e. we'd have a hierarchy of flags: - the old RTIT_CTL_BRANCH_EN bit (now RTIT_CTL_PT_EN) enables all of PT, with all features - individual feature disabling bits, which default to 0 (i.e. the feature is enabled) in the attr structure control the finegrained enabling/disabling of PT features. Currently there are two bits: pt_feature_branch_disable and pt_feature_branch_enable. More are added in the future if PT grows even more features. or so? Thanks, Ingo