Re: [PATCH V6 3/3] perf/x86/intel: Support PEBS counters snapshotting

[Peter Zijlstra]

On Wed, Dec 18, 2024 at 07:16:43AM -0800, kan.liang@xxxxxxxxxxxxxxx wrote:

> @@ -3109,6 +3116,27 @@ static int handle_pmi_common(struct pt_regs *regs, u64 status)
>  		if (!test_bit(bit, cpuc->active_mask))
>  			continue;
>  

> +		if (is_pebs_counter_event(event))
> +			x86_pmu.drain_pebs(regs, &data);
> +
>  		if (!intel_pmu_save_and_restart(event))
>  			continue;
>  
> @@ -4056,6 +4084,23 @@ static int intel_pmu_hw_config(struct perf_event *event)
>  		event->hw.flags |= PERF_X86_EVENT_PEBS_VIA_PT;
>  	}
>  
> +	if ((event->attr.sample_type & PERF_SAMPLE_READ) &&
> +	    (x86_pmu.intel_cap.pebs_format >= 6)) {

Right, so the event that has SAMPLE_READ on is 'event'

> +		struct perf_event *leader = event->group_leader;
> +		bool slots_leader = is_slots_event(leader);
> +
> +		if (slots_leader)
> +			leader = list_next_entry(leader, sibling_list);

Uh, what, why?

> +
> +		if (leader->attr.precise_ip) {
> +			event->hw.flags |= PERF_X86_EVENT_PEBS_CNTR;
> +			if (slots_leader) {
> +				leader->hw.flags |= PERF_X86_EVENT_PEBS_CNTR;
> +				event->group_leader->hw.flags |= PERF_X86_EVENT_PEBS_CNTR;
> +			}
> +		}

And this is more confusion. You want event to be a PEBS event, not the
leader, you don't care about the leader.


> +	}
> +
>  	if ((event->attr.type == PERF_TYPE_HARDWARE) ||
>  	    (event->attr.type == PERF_TYPE_HW_CACHE))
>  		return 0;

> +static inline bool is_pebs_counter_event(struct perf_event *event)
> +{
> +	return event->hw.flags & PERF_X86_EVENT_PEBS_CNTR;
> +}

For that drain_pebs() thing, you want all group members to have
PEBS_CNTR set.

That is, if PEBS>=6 and event is PEBS and event has SAMPLE_READ, then
mark the whole group with PEBS_CNTR

SAMPLE_READ doesn't particularly care who's the leader, the event that
has SAMPLE_READ will read the whole group. Heck they could all have
SAMPLE_READ and then all their samples will read each-other.