Messages in this thread |  | | Date | Wed, 1 Feb 2017 11:17:04 +0100 | From | Ingo Molnar <> | Subject | Re: [PATCH 2/2] perf/x86/intel/pt: Allow disabling branch tracing |
| |
* Alexander Shishkin <alexander.shishkin@linux.intel.com> 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
|  |