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

[Mi, Dapeng]

On 3/12/2026 2:41 PM, Peter Zijlstra wrote:
> On Thu, Mar 12, 2026 at 10:31:28AM +0800, Mi, Dapeng wrote:
>> 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.
>>
> I already fixed everything up. Should be in queue/perf/urgent.

Thanks. 

Peter, As Ian points out, the patch "perf/x86: Update cap_user_rdpmc base
on rdpmc user disable state" has a bug
(https://lore.kernel.org/all/CAP-5=fWr2L6miiFZ6Km3HYEdmqp3T0NBL=WY3buKdKztW+HvmA@xxxxxxxxxxxxxx/).
I would posted a patch to fix the issue. Thanks.



>