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

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

Hi Peter,

Sorry for the late response. I was on vacation.

On 2024-12-20 9:22 a.m., Peter Zijlstra wrote:
> On Wed, Dec 18, 2024 at 07:16:43AM -0800, kan.liang@linux.intel.com 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?

This was to specially handle the perf metric topdown group.

> 
>> +
>> +		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.

Right

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

Yes, that was the design.

> 
> 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.

Right. It should be good enough to only set the flag for the
event->group_leader, since there is only one sampling event for a
SAMPLE_READ group.

The hw_config check can be simplified as below.

	if ((event->attr.sample_type & PERF_SAMPLE_READ) &&
	    (x86_pmu.intel_cap.pebs_format >= 6) &&
	    is_sampling_event(event) &&
	    event->attr.precise_ip)
		event->group_leader->hw.flags |= PERF_X86_EVENT_PEBS_CNTR;

Also, Only need to check the leader's flag to indicate the event in a
SAMPLE_READ group.

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


Thanks,
Kan