Re: [RESEND Patch 2/2] perf/x86/intel: Add missing branch counters constraint apply

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

On 3/12/2026 4:16 AM, Peter Zijlstra wrote:
> On Sat, Feb 28, 2026 at 01:33:20PM +0800, Dapeng Mi wrote:
>> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
>> index 4768236c054b..4b042d71104f 100644
>> --- a/arch/x86/events/intel/core.c
>> +++ b/arch/x86/events/intel/core.c
>> @@ -4628,6 +4628,19 @@ static inline void intel_pmu_set_acr_caused_constr(struct perf_event *event,
>>  		event->hw.dyn_constraint &= hybrid(event->pmu, acr_cause_mask64);
>>  }
>>  
>> +static inline int intel_set_branch_counter_constr(struct perf_event *event,
>> +						  int *num)
>> +{
>> +	if (branch_sample_call_stack(event))
>> +		return -EINVAL;
>> +	if (branch_sample_counters(event)) {
>> +		(*num)++;
>> +		event->hw.dyn_constraint &= x86_pmu.lbr_counters;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  static int intel_pmu_hw_config(struct perf_event *event)
>>  {
>>  	int ret = x86_pmu_hw_config(event);
>> @@ -4698,21 +4711,18 @@ static int intel_pmu_hw_config(struct perf_event *event)
>>  		 * group, which requires the extra space to store the counters.
>>  		 */
>>  		leader = event->group_leader;
>> +		if (intel_set_branch_counter_constr(leader, &num))
>>  			return -EINVAL;
>>  		leader->hw.flags |= PERF_X86_EVENT_BRANCH_COUNTERS;
>>  
>>  		for_each_sibling_event(sibling, leader) {
>> +			if (intel_set_branch_counter_constr(sibling, &num))
>> +				return -EINVAL;
>> +		}
>> +
> Do the new bit is this, right?

Actually not, the key change is the below one. The last event in the group
is not applied the branch counter constraint.

Assume we have a event group {cycles,instructions,branches}. When the 3rd
event "branches" is created and the function intel_pmu_hw_config() is
called for the "branches" event to check the config.  The event leader is
"cycles" and the sibling event has only the "instructions" event at that
time since the 3rd event "branches" is in creation and still not added into
the sibling_list. So for_each_sibling_event() can't really iterate the
"branches" event.


>
>> +		if (event != leader) {
>> +			if (intel_set_branch_counter_constr(event, &num))
>>  				return -EINVAL;
>>  		}
> The point being that for_each_sibling_event() will not have iterated the
> event because its not on the list yet?

Yes. 


>
> That wasn't really clear from the changelog and I think that deserves a
> comment as well.

Sure. I would add comment and enhance the changelog to make it clearer. Thanks.


>
> Let me go fix that.