Re: [PATCH 2/2] perf/x86/intel/pt: Allow disabling branch tracing

[Alexander Shishkin]

Ingo Molnar <mingo@xxxxxxxxxx> writes:

> * Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx> 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: */

Will do.

>> +	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...

Agreed.

> 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. )

Most features (all but BranchEn) are at the moment set-to-enable. It is
only this unfortunate default of BranchEn=1 that is a pain. Another
relatively painless thing we can do is add a 'passthrough' bit, which
will allow the user to (not) set BranchEn bit when passthrough=1 and
behave like it did before, when passthrough=0. Like below: