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

[Peter Zijlstra]

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.