Re: [PATCH] perf/x86/intel/pt: Don't die on VMXON

[Alexander Shishkin]

Peter Zijlstra <peterz@xxxxxxxxxxxxx> writes:

> On Fri, Apr 01, 2016 at 07:24:14PM +0300, Alexander Shishkin wrote:
>> +static void pt_config_stop(struct perf_event *event)
>>  {
>> +	u64 ctl = READ_ONCE(event->hw.config);
>>  
>> +	/* may be already stopped by a PMI*/
>> +	if (!(ctl & RTIT_CTL_TRACEEN))
>> +		return;
>> +
>> +	ctl ^= RTIT_CTL_TRACEEN;
>
> Would that not be much less confusing when written like |= ?

This one's actually clearing TraceEn, see the if-not-set-leave in front
of it, but that just goes to prove your point I guess. :)

>> +void intel_pt_vmxon(int entry)
>> +{
>> +	struct pt *pt = this_cpu_ptr(&pt_ctx);
>> +	struct perf_event *event;
>> +	unsigned long flags;
>> +
>> +	/* PT plays nice with VMX, do nothing */
>> +	if (pt_pmu.vmx)
>> +		return;
>> +
>> +	/*
>> +	 * VMX entry will clear RTIT_CTL.TraceEn; we need to make
>> +	 * sure to not try to set it while VMX is on. Disable
>> +	 * interrupts to avoid racing with pmu callbacks;
>> +	 * concurrent PMI should be handled fine.
>> +	 */
>> +	local_irq_save(flags);
>> +	WRITE_ONCE(pt->vmx_on, entry);
>
> So you mix: "VMX is on" and "VMX entry", please pick one.
>
> Since the function is called vmxon, I find .entry a very confusing
> argument name.

I did struggle with this one indeed.

How about this then: