Re: [PATCH] perf/x86/intel: Hide Topdown metrics events if slots is not enumerated

[Liang, Kan]

On 2023-12-19 8:22 a.m., Dongli Zhang wrote:
> I can still reproduce this issue with the most recent mainline linux kernel (at
> least when the KVM is not the most recent).
> 
> Any chance to have this patch in the linux kernel?
> 
> (We may need to rename 'cpu_type' due to commit b0560bfd4b70 ("perf/x86/intel:
> Clean up the hybrid CPU type handling code"))

I have re-based the patch on top of the latest 6.7-rc.
https://lore.kernel.org/lkml/20231219150109.1596634-1-kan.liang@xxxxxxxxxxxxxxx/

Besides the 'cpu_type', I also use the intel_cap.perf_metrics to do the
check, which should be more accurate than the slots event.

Please give it a try and let us know if the V2 works for you.

Thanks,
Kan
> 
> Thank you very much!
> 
> Dongli Zhang
> 
> On 10/27/22 09:17, Dongli Zhang wrote:
>> Ping? Any plan for this patch? Currently "perf stat" will fail on Icelake VMs
>> (without the topdown metric). The user will need to manually specify the events
>> to trace.
>>
>> Thank you very much!
>>
>> Dongli Zhang
>>
>> On 10/9/22 10:03 PM, Dongli Zhang wrote:
>>> Ping?
>>>
>>> Currently the default "perf stat" may fail on all Icelake KVM VMs.
>>>
>>> Thank you very much!
>>>
>>> Dongli Zhang
>>>
>>> On 9/22/22 13:15, kan.liang@xxxxxxxxxxxxxxx wrote:
>>>> From: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
>>>>
>>>> The below error is observed on Ice Lake VM.
>>>>
>>>> $ perf stat
>>>> Error:
>>>> The sys_perf_event_open() syscall returned with 22 (Invalid argument)
>>>> for event (slots).
>>>> /bin/dmesg | grep -i perf may provide additional information.
>>>>
>>>> In a virtualization env, the Topdown metrics and the slots event haven't
>>>> been supported yet. The guest CPUID doesn't enumerate them. However, the
>>>> current kernel unconditionally exposes the slots event and the Topdown
>>>> metrics events to sysfs, which misleads the perf tool and triggers the
>>>> error.
>>>>
>>>> Hide the perf metrics topdown events and the slots event if the slots
>>>> event is not enumerated.
>>>>
>>>> The big core of a hybrid platform can also supports the perf-metrics
>>>> feature. Fix the hybrid platform as well.
>>>>
>>>> Reported-by: Dongli Zhang <dongli.zhang@xxxxxxxxxx>
>>>> Tested-by: Dongli Zhang <dongli.zhang@xxxxxxxxxx>
>>>> Signed-off-by: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
>>>> ---
>>>>  arch/x86/events/intel/core.c | 33 ++++++++++++++++++++++++++++++++-
>>>>  1 file changed, 32 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
>>>> index b16c91ac9219..a0a62b67c440 100644
>>>> --- a/arch/x86/events/intel/core.c
>>>> +++ b/arch/x86/events/intel/core.c
>>>> @@ -5335,6 +5335,19 @@ static struct attribute *intel_pmu_attrs[] = {
>>>>  	NULL,
>>>>  };
>>>>  
>>>> +static umode_t
>>>> +td_is_visible(struct kobject *kobj, struct attribute *attr, int i)
>>>> +{
>>>> +	/*
>>>> +	 * Hide the perf metrics topdown events
>>>> +	 * if the slots is not enumerated.
>>>> +	 */
>>>> +	if (x86_pmu.num_topdown_events)
>>>> +		return (x86_pmu.intel_ctrl & INTEL_PMC_MSK_FIXED_SLOTS) ? attr->mode : 0;
>>>> +
>>>> +	return attr->mode;
>>>> +}
>>>> +
>>>>  static umode_t
>>>>  tsx_is_visible(struct kobject *kobj, struct attribute *attr, int i)
>>>>  {
>>>> @@ -5370,6 +5383,7 @@ default_is_visible(struct kobject *kobj, struct attribute *attr, int i)
>>>>  
>>>>  static struct attribute_group group_events_td  = {
>>>>  	.name = "events",
>>>> +	.is_visible = td_is_visible,
>>>>  };
>>>>  
>>>>  static struct attribute_group group_events_mem = {
>>>> @@ -5522,6 +5536,23 @@ static inline int hybrid_find_supported_cpu(struct x86_hybrid_pmu *pmu)
>>>>  	return (cpu >= nr_cpu_ids) ? -1 : cpu;
>>>>  }
>>>>  
>>>> +static umode_t hybrid_td_is_visible(struct kobject *kobj,
>>>> +					struct attribute *attr, int i)
>>>> +{
>>>> +	struct device *dev = kobj_to_dev(kobj);
>>>> +	struct x86_hybrid_pmu *pmu =
>>>> +		 container_of(dev_get_drvdata(dev), struct x86_hybrid_pmu, pmu);
>>>> +
>>>> +	if (!is_attr_for_this_pmu(kobj, attr))
>>>> +		return 0;
>>>> +
>>>> +	/* Only check the big core which supports perf metrics */
>>>> +	if (pmu->cpu_type == hybrid_big)
>>>> +		return (pmu->intel_ctrl & INTEL_PMC_MSK_FIXED_SLOTS) ? attr->mode : 0;
>>>> +
>>>> +	return attr->mode;
>>>> +}
>>>> +
>>>>  static umode_t hybrid_tsx_is_visible(struct kobject *kobj,
>>>>  				     struct attribute *attr, int i)
>>>>  {
>>>> @@ -5548,7 +5579,7 @@ static umode_t hybrid_format_is_visible(struct kobject *kobj,
>>>>  
>>>>  static struct attribute_group hybrid_group_events_td  = {
>>>>  	.name		= "events",
>>>> -	.is_visible	= hybrid_events_is_visible,
>>>> +	.is_visible	= hybrid_td_is_visible,
>>>>  };
>>>>  
>>>>  static struct attribute_group hybrid_group_events_mem = {
>