Re: [PATCH 07/15] perf/x86/intel: Reject SAMPLE_READ for no-counter-snapshot ACR events

["Mi, Dapeng" <dapeng1.mi@linux.intel.com>]

On 9/30/2026 3:02 AM, Falcon, Thomas wrote:
> On Mon, 2026-09-28 at 15:43 +0800, Dapeng Mi wrote:
>> ACR events cannot always provide a reliable value through SAMPLE_READ.
>> For non-PEBS ACR events, another ACR overflow can auto-reload the counter
>> before software reads it, so software cannot sample the exact count before
>> the hardware reload.
>>
>> PEBS-backed ACR events are safe only when counter snapshot support is
>> available, because the value is captured in the PEBS record before the
>> counter is reloaded.
>>
>> Reject SAMPLE_READ for ACR event groups unless the event has PEBS
>> counter snapshot support, so perf does not report invalid counts.
>>
>> Reported-by: Andi Kleen <ak@linux.intel.com>
>> Fixes: ec980e4facef ("perf/x86/intel: Support auto counter reload")
>> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
>> ---
>>  arch/x86/events/intel/core.c | 59 ++++++++++++++++++++++++++++++++++++
>>  1 file changed, 59 insertions(+)
>>
>> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
>> index 377ff3912420..3fc3795534bf 100644
>> --- a/arch/x86/events/intel/core.c
>> +++ b/arch/x86/events/intel/core.c
>> @@ -4974,6 +4974,53 @@ static inline int intel_set_branch_counter_constr(struct perf_event *event,
>>  	return 0;
>>  }
>>  
>> +static inline bool is_acr_sample_read_allowed(struct perf_event *event,
>> +					      bool group_has_sample_read)
>> +{
>> +	/*
>> +	 * ACR events cannot report an accurate count for non-PEBS events
>> +	 * or for PEBS events without counter snapshots: another ACR event
>> +	 * may overflow andauto-reload the counter before software can read
> Just wanted to point out the typo here. Other than that, this patch and the rest of the series look good to me...

Good catch. Thanks.

I'd like to wait one or two weeks to see if Peter and other guys have other
comments, and fix them all in version 2. 


>
> Reviewed-by: Thomas Falcon <thomas.falcon@intel.com>
>
>> +	 * the precise value.
>> +	 *
>> +	 * We keep the check simple and do not validate the acr_mask precisely
>> +	 * to determine whether the SAMPLE_READ event is actually auto-reloaded
>> +	 * by another ACR event. If a SAMPLE_READ event is in the group, the
>> +	 * ACR event must be a PEBS event with counter snapshots; otherwise it
>> +	 * is rejected.
>> +	 */
>> +	if (group_has_sample_read && is_sampling_event(event) &&
>> +	    (!event->attr.precise_ip || !is_pebs_counter_event_group(event)))
>> +		return false;
>> +
>> +	return true;
>> +}
>> +
>> +static bool intel_pmu_allow_acr_sample_read(struct perf_event *event,
>> +					    bool group_has_sample_read)
>> +{
>> +	struct perf_event *leader = event->group_leader;
>> +	struct perf_event *sibling;
>> +
>> +	if (!is_acr_sample_read_allowed(leader, group_has_sample_read))
>> +		return false;
>> +
>> +	if (leader->nr_siblings) {
>> +		for_each_sibling_event(sibling, leader) {
>> +			if (!is_acr_sample_read_allowed(sibling,
>> +						group_has_sample_read))
>> +				return false;
>> +		}
>> +	}
>> +
>> +	/* event isn't installed as a sibling yet. */
>> +	if ((event != leader) &&
>> +	    !is_acr_sample_read_allowed(event, group_has_sample_read))
>> +		return false;
>> +
>> +	return true;
>> +}
>> +
>>  static int intel_pmu_hw_config(struct perf_event *event)
>>  {
>>  	int ret = x86_pmu_hw_config(event);
>> @@ -5117,6 +5164,7 @@ static int intel_pmu_hw_config(struct perf_event *event)
>>  		struct perf_event *sibling, *leader = event->group_leader;
>>  		struct pmu *pmu = event->pmu;
>>  		bool has_sw_event = false;
>> +		bool has_sample_read = false;
>>  		int num = 0, idx = 0;
>>  		u64 cause_mask = 0;
>>  
>> @@ -5162,8 +5210,14 @@ static int intel_pmu_hw_config(struct perf_event *event)
>>  		if (leader->attr.config2)
>>  			intel_pmu_set_acr_cntr_constr(leader, &cause_mask, &num);
>>  
>> +		if ((leader->attr.sample_type & PERF_SAMPLE_READ) ||
>> +		    (event->attr.sample_type & PERF_SAMPLE_READ))
>> +			has_sample_read = true;
>> +
>>  		if (leader->nr_siblings) {
>>  			for_each_sibling_event(sibling, leader) {
>> +				if (sibling->attr.sample_type & PERF_SAMPLE_READ)
>> +					has_sample_read = true;
>>  				if (!is_x86_event(sibling)) {
>>  					has_sw_event = true;
>>  					continue;
>> @@ -5175,6 +5229,7 @@ static int intel_pmu_hw_config(struct perf_event *event)
>>  				intel_pmu_set_acr_cntr_constr(sibling, &cause_mask, &num);
>>  			}
>>  		}
>> +
>>  		if (leader != event && event->attr.config2) {
>>  			if (has_sw_event)
>>  				return -EINVAL;
>> @@ -5184,6 +5239,10 @@ static int intel_pmu_hw_config(struct perf_event *event)
>>  		if (hweight64(cause_mask) > hweight64(hybrid(pmu, acr_cause_mask64)) ||
>>  		    num > hweight64(hybrid(event->pmu, acr_cntr_mask64)))
>>  			return -EINVAL;
>> +
>> +		if (!intel_pmu_allow_acr_sample_read(event, has_sample_read))
>> +			return -EINVAL;
>> +
>>  		/*
>>  		 * In the second round, apply the counter-constraints for
>>  		 * the events which can cause other events reload.