Re: [Patch v3 12/22] perf/x86/intel: Update dyn_constranit base on PEBS event precise level

[Liang, Kan]

On 2025-04-15 9:46 p.m., Mi, Dapeng wrote:
> 
> On 4/16/2025 12:31 AM, Liang, Kan wrote:
>>
>> On 2025-04-15 9:53 a.m., Peter Zijlstra wrote:
>>> On Tue, Apr 15, 2025 at 11:44:18AM +0000, Dapeng Mi wrote:
>>>> arch-PEBS provides CPUIDs to enumerate which counters support PEBS
>>>> sampling and precise distribution PEBS sampling. Thus PEBS constraints
>>>> should be dynamically configured base on these counter and precise
>>>> distribution bitmap instead of defining them statically.
>>>>
>>>> Update event dyn_constraint base on PEBS event precise level.
>>> What if any constraints are there on this? 
>> Do you mean the static constraints defined in the
>> event_constraints/pebs_constraints?
>>
>>> CPUID is virt host
>>> controlled, right, so these could be the most horrible masks ever.
>>>
>> Yes, it could be changed by VMM. A sanity check should be required if
>> abad mask is given.
> 
> Yes, we need a check to restrict the PEBS counter mask into the valid
> counter mask, and just realized that we can't use hybrid(event->pmu,
> intel_ctrl) to check counter mask and need a minor tweak since it includes
> the GLOBAL_CTRL_EN_PERF_METRICS bit.
> 
> How about this?
> 
>         if (x86_pmu.arch_pebs) {
>             u64 cntr_mask = hybrid(event->pmu, intel_ctrl) &
>                         ~GLOBAL_CTRL_EN_PERF_METRICS;
>             u64 pebs_mask = event->attr.precise_ip >= 3 ?
>                         pebs_cap.pdists : pebs_cap.counters;
>             if (pebs_mask != cntr_mask)
>                 event->hw.dyn_constraint = pebs_mask & cntr_mask;
>         }
> 

The mask isn't changed after boot. The sanity check should only be done
once. I think it can be done in the update_pmu_cap() when perf retrieves
the value. If a bad mask is detected, the PEBS should be disabled.

Thanks,
Kan>
>>
>>> This can land us in EVENT_CONSTRAINT_OVERLAP territory, no?The dyn_constraint is a supplement of the static constraints. It doesn't
>> overwrite the static constraints.
>>
>> In the intel_get_event_constraints(), perf always gets the static
>> constraints first. If the dyn_constraint is defined, it gets the common
>> mask of the static constraints and the dynamic constraints. All
>> constraint rules will be complied.
>>
>> 	if (event->hw.dyn_constraint != ~0ULL) {
>> 		c2 = dyn_constraint(cpuc, c2, idx);
>> 		c2->idxmsk64 &= event->hw.dyn_constraint;
>> 		c2->weight = hweight64(c2->idxmsk64);
>> 	}
>>
>> Thanks,
>> Kan
>>
>