Re: [PATCH 03/22] perf/x86/intel: Support adaptive PEBSv4

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

On 3/19/2019 10:47 AM, Peter Zijlstra wrote:
>> @@ -933,6 +998,19 @@ pebs_update_state(bool needed_cb, struct cpu_hw_events *cpuc, struct pmu *pmu)
>>   		update = true;
>>   	}
>>   
>> +	if (x86_pmu.intel_cap.pebs_baseline && add) {
>> +		u64 pebs_data_cfg;
>> +
>> +		pebs_data_cfg = pebs_update_adaptive_cfg(event);
>> +
>> +		/* Update pebs_record_size if new event requires more data. */
>> +		if (pebs_data_cfg & ~cpuc->pebs_data_cfg) {
>> +			cpuc->pebs_data_cfg |= pebs_data_cfg;
>> +			adaptive_pebs_record_size_update();
>> +			update = true;
>> +		}
>> +	}
>> +
>>   	if (update)
>>   		pebs_update_threshold(cpuc);
>>   }
> Hurmph.. this only grows the PEBS record.
>

Yes, the PEBS record doesn't shrink on the del. Because we have to go 
through all the existing pebs events for an accurate config. I think it 
doesn't worth it. There is no harmful for a bigger PEBS record, except 
little performance impacts. But that's rare case. For most cases, we 
usually apply the same pebs config for all pebs events.

> 
>> @@ -947,7 +1025,7 @@ void intel_pmu_pebs_add(struct perf_event *event)
>>   	if (hwc->flags & PERF_X86_EVENT_LARGE_PEBS)
>>   		cpuc->n_large_pebs++;
>>   
>> -	pebs_update_state(needed_cb, cpuc, event->ctx->pmu);
>> +	pebs_update_state(needed_cb, cpuc, event, true);
>>   }
>>   
>>   void intel_pmu_pebs_enable(struct perf_event *event)
>> @@ -965,6 +1043,14 @@ void intel_pmu_pebs_enable(struct perf_event *event)
>>   	else if (event->hw.flags & PERF_X86_EVENT_PEBS_ST)
>>   		cpuc->pebs_enabled |= 1ULL << 63;
>>   
>> +	if (x86_pmu.intel_cap.pebs_baseline) {
>> +		hwc->config |= ICL_EVENTSEL_ADAPTIVE;
>> +		if (cpuc->pebs_data_cfg != cpuc->active_pebs_data_cfg) {
>> +			wrmsrl(MSR_PEBS_DATA_CFG, cpuc->pebs_data_cfg);
>> +			cpuc->active_pebs_data_cfg = cpuc->pebs_data_cfg;
>> +		}
>> +	}
>> +
>>   	/*
>>   	 * Use auto-reload if possible to save a MSR write in the PMI.
>>   	 * This must be done in pmu::start(), because PERF_EVENT_IOC_PERIOD.
>> @@ -991,7 +1077,12 @@ void intel_pmu_pebs_del(struct perf_event *event)
>>   	if (hwc->flags & PERF_X86_EVENT_LARGE_PEBS)
>>   		cpuc->n_large_pebs--;
>>   
>> -	pebs_update_state(needed_cb, cpuc, event->ctx->pmu);
>> +	/* Clear both pebs_data_cfg and pebs_record_size for first PEBS. */
> Weird comment..
> 
>> +	if (x86_pmu.intel_cap.pebs_baseline && !cpuc->n_pebs) {
>> +		cpuc->pebs_data_cfg = 0;
>> +		cpuc->pebs_record_size = sizeof(struct pebs_basic);
>> +	}
>> +	pebs_update_state(needed_cb, cpuc, event, false);
> Why do we have to reset record_size? That'll be updated in
> pebs_update_state() on the next add.
> 

The record_size should be reset for the first PEBS events.
Right, I can move the reset in pebs_update_state().


Thanks,
Kan