Re: [PATCH V2 3/3] perf/x86/intel: Support auto counter reload

[Liang, Kan]

On 2025-03-14 6:20 a.m., Peter Zijlstra wrote:
> On Thu, Oct 10, 2024 at 12:28:44PM -0700, kan.liang@xxxxxxxxxxxxxxx wrote:
> 
>> @@ -4159,6 +4332,49 @@ static int intel_pmu_hw_config(struct perf_event *event)
>>  	return 0;
>>  }
>>  
>> +static void intel_pmu_update_acr_mask(struct cpu_hw_events *cpuc, int n, int *assign)
>> +{
>> +	struct perf_event *event;
>> +	int n0, i, off;
>> +
>> +	if (cpuc->txn_flags & PERF_PMU_TXN_ADD)
>> +		n0 = cpuc->n_events - cpuc->n_txn;
>> +	else
>> +		n0 = cpuc->n_events;
>> +
>> +	for (i = n0; i < n; i++) {
>> +		event = cpuc->event_list[i];
>> +		event->hw.config1 = 0;
>> +
>> +		/* Convert the group index into the counter index */
>> +		for_each_set_bit(off, (unsigned long *)&event->attr.config2, n - n0)
>> +			set_bit(assign[n0 + off], (unsigned long *)&event->hw.config1);
> 
> Atomic set_bit() is required?

I don't think so. Will change it to __set_bit().

> 
>> +	}
>> +}
>> +
>> +static int intel_pmu_schedule_events(struct cpu_hw_events *cpuc, int n, int *assign)
>> +{
>> +	struct perf_event *event;
>> +	int ret = x86_schedule_events(cpuc, n, assign);
>> +
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (cpuc->is_fake)
>> +		return ret;
>> +
>> +	event = cpuc->event_list[n - 1];
> 
> ISTR seeing this pattern before somewhere and then argued it was all
> sorts of broken. Why is it sane to look at the last event here?

The schedule_events() is invoked for only two cases, a new event or a
new group. Since the event_list[] is in enabled order, the last event
should be either the new event or the last event of the new group.

The is_acr_event_group() always checks the leader's flag. It doesn't
matter which event in the ACR group is used to do the check.

Checking the last event should be good enough to cover both cases.

> 
>> +	/*
>> +	 * The acr_mask(config2) is the event-enabling order.
>> +	 * Update it to the counter order after the counters are assigned.
>> +	 */
>> +	if (event && is_acr_event_group(event))
>> +		intel_pmu_update_acr_mask(cpuc, n, assign);
>> +
>> +	return 0;
>> +}
>> +
>> +
>>  /*
>>   * Currently, the only caller of this function is the atomic_switch_perf_msrs().
>>   * The host perf context helps to prepare the values of the real hardware for
>> @@ -5305,7 +5521,7 @@ static __initconst const struct x86_pmu intel_pmu = {
>>  	.set_period		= intel_pmu_set_period,
>>  	.update			= intel_pmu_update,
>>  	.hw_config		= intel_pmu_hw_config,
>> -	.schedule_events	= x86_schedule_events,
>> +	.schedule_events	= intel_pmu_schedule_events,
>>  	.eventsel		= MSR_ARCH_PERFMON_EVENTSEL0,
>>  	.perfctr		= MSR_ARCH_PERFMON_PERFCTR0,
>>  	.fixedctr		= MSR_ARCH_PERFMON_FIXED_CTR0,
> 
> How about only setting that function if the PMU actually support ACR ?

Sure. I will also address the other comments which I didn't reply above
in the email.

Thanks,
Kan