Re: [Patch v4 09/13] perf/x86/intel: Setup PEBS data configuration and enable legacy groups

["Mi, Dapeng" <dapeng1.mi@linux.intel.com>]

On 6/21/2025 5:34 PM, Peter Zijlstra wrote:
> On Fri, Jun 20, 2025 at 10:39:05AM +0000, Dapeng Mi wrote:
>
>> +static void intel_pmu_enable_event_ext(struct perf_event *event)
>> +{
>> +	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>> +	struct hw_perf_event *hwc = &event->hw;
>> +	union arch_pebs_index cached, index;
>> +	struct arch_pebs_cap cap;
>> +	u64 ext = 0;
>> +
>> +	if (!x86_pmu.arch_pebs)
>> +		return;
>> +
>> +	cap = hybrid(cpuc->pmu, arch_pebs_cap);
>> +
>> +	if (event->attr.precise_ip) {
>> +		u64 pebs_data_cfg = intel_get_arch_pebs_data_config(event);
>> +
>> +		ext |= ARCH_PEBS_EN;
>> +		if (hwc->flags & PERF_X86_EVENT_AUTO_RELOAD)
>> +			ext |= (-hwc->sample_period) & ARCH_PEBS_RELOAD;
>> +
>> +		if (pebs_data_cfg && cap.caps) {
>> +			if (pebs_data_cfg & PEBS_DATACFG_MEMINFO)
>> +				ext |= ARCH_PEBS_AUX & cap.caps;
>> +
>> +			if (pebs_data_cfg & PEBS_DATACFG_GP)
>> +				ext |= ARCH_PEBS_GPR & cap.caps;
>> +
>> +			if (pebs_data_cfg & PEBS_DATACFG_XMMS)
>> +				ext |= ARCH_PEBS_VECR_XMM & cap.caps;
>> +
>> +			if (pebs_data_cfg & PEBS_DATACFG_LBRS)
>> +				ext |= ARCH_PEBS_LBR & cap.caps;
>> +		}
>> +
>> +		if (cpuc->n_pebs == cpuc->n_large_pebs)
>> +			index.split.thresh = ARCH_PEBS_THRESH_MUL;
>> +		else
>> +			index.split.thresh = ARCH_PEBS_THRESH_SINGLE;
>> +
>> +		rdmsrl(MSR_IA32_PEBS_INDEX, cached.full);
> Its unclear to me we need this rdmrsl(); does anything actually change
> in there or is it just the value we wrote last? The naming seems to
> suggested you want it cached instead of re-read. Most confusing.

HW could write MSR_IA32_PEBS_INDEX as well, so strictly speaking it's not
we wrote last. The aim of this part of code is to check if large PEBS is
used by last time, if so we need to drain up the PEBS buffer before
re-enable PEBS. 

Yes, the "cache" is some kind of misleading. I would change it.


>
> Also, if you do:
>
> union arch_perf_index {
> 	u64 full;
> 	struct {
> 		u64 foo:1,
> 		    bar:2;
> 	};
> };
>
> Then you can get rid of that .split naming.

Sure. Thanks.


>
>> +		if (index.split.thresh != cached.split.thresh || !cached.split.en) {
>> +			if (cached.split.thresh == ARCH_PEBS_THRESH_MUL &&
>> +			    cached.split.wr > 0) {
>> +				/*
>> +				 * Large PEBS was enabled.
>> +				 * Drain PEBS buffer before applying the single PEBS.
>> +				 */
>> +				intel_pmu_drain_pebs_buffer();
>> +			} else {
>> +				index.split.wr = 0;
>> +				index.split.full = 0;
>> +				index.split.en = 1;
>> +				wrmsrq(MSR_IA32_PEBS_INDEX, index.full);
>> +			}
>> +		}
>> +	}
>> +
>> +	if (cpuc->cfg_c_val[hwc->idx] != ext)
>> +		__intel_pmu_update_event_ext(hwc->idx, ext);
>> +}