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

[Peter Zijlstra <peterz@infradead.org>]

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.

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.

> +		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);
> +}