lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5c6b52ec-e903-42be-aa57-675abc350241@linux.intel.com>
Date: Fri, 14 Mar 2025 09:48:35 -0400
From: "Liang, Kan" <kan.liang@...ux.intel.com>
To: Peter Zijlstra <peterz@...radead.org>
Cc: mingo@...nel.org, acme@...nel.org, namhyung@...nel.org,
 irogers@...gle.com, adrian.hunter@...el.com, ak@...ux.intel.com,
 linux-kernel@...r.kernel.org, eranian@...gle.com, thomas.falcon@...el.com
Subject: Re: [PATCH V2 3/3] perf/x86/intel: Support auto counter reload



On 2025-03-14 6:20 a.m., Peter Zijlstra wrote:
> On Thu, Oct 10, 2024 at 12:28:44PM -0700, kan.liang@...ux.intel.com 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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ