Re: [RESEND PATCH V2] perf/x86/intel: Avoid unnecessary PEBS_ENABLE MSR access in PMI

["Liang, Kan" <kan.liang@linux.intel.com>]

On 1/17/2020 3:54 AM, Peter Zijlstra wrote:
> On Thu, Jan 16, 2020 at 10:21:12AM -0800, kan.liang@linux.intel.com wrote:
> 
>> A PMI may land after cpuc->enabled=0 in x86_pmu_disable() and
>> PMI throttle may be triggered for the PMI. For this rare case,
>> intel_pmu_pebs_disable() will not touch PEBS_ENABLE MSR. The patch
>> explicitly disable the PEBS for this case.
> 
> intel_pmu_handle_irq()
>    pmu_enabled = cpuc->enabled;
>    cpuc->enabled = 0;
>    __intel_pmu_disable_all();
> 
>    ...
>      x86_pmu_stop()
>        intel_pmu_disable_event()
>          intel_pmu_pebs_disable()
> 	  if (cpuc->enabled) // FALSE!!!

Right, it always be false in PMI.
We force the 'enabled' to 0 when entering the PMI.

For this case, I think we may add a variable to save the pebs_enabled 
when entering PMI. If it's changed, we have to write the new value.
I will prepare the V3.

Thanks,
Kan

> 
>    cpuc->enabled = pmu_enabled;
>    if (pmu_enabled)
>      __intel_pmu_enable_all();
> 
>> @@ -2620,6 +2627,15 @@ static int handle_pmi_common(struct pt_regs *regs, u64 status)
>>   		handled++;
>>   		x86_pmu.drain_pebs(regs);
>>   		status &= x86_pmu.intel_ctrl | GLOBAL_STATUS_TRACE_TOPAPMI;
>> +
>> +		/*
>> +		 * PMI may land after cpuc->enabled=0 in x86_pmu_disable() and
>> +		 * PMI throttle may be triggered for the PMI.
>> +		 * For this rare case, intel_pmu_pebs_disable() will not touch
>> +		 * MSR_IA32_PEBS_ENABLE. Explicitly disable the PEBS here.
>> +		 */
>> +		if (unlikely(!cpuc->enabled && !cpuc->pebs_enabled))
>> +			wrmsrl(MSR_IA32_PEBS_ENABLE, 0);
>>   	}
> 
> How does that make sense? AFAICT this is all still completely broken.
> 
> Please be more careful.
>