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

From: Liang, Kan
Date: Tue Dec 19 2023 - 10:04:31 EST




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 = {
>