Add [email protected] as this issue is in virtualization env.
The topdown metrics events became default since
commit 42641d6f4d15 ("perf stat: Add Topdown metrics events as default
events"). The perf will use 'slots' if the
/sys/bus/event_source/devices/cpu/events/slots is available.
Unfortunately, the 'slots' may not be supported in the virualization
environment. The hypervisor may not expose the 'slots' counter to the VM
in cpuid. As a result, the kernel may disable topdown slots and metrics
events in intel_pmu_init() if slots event is not in CPUID. E.g., both
c->weight and c->idxmsk64 are set to 0.
There will be below error on Icelake VM since 'slots' is the leader:
$ 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.
This is because the stat_handle_error() returns COUNTER_FATAL when the
'slots' is used as leader of events.
There are three options to fix the issue.
1. Do not expose /sys/bus/event_source/devices/cpu/events/slots to
userspace so that pmu_have_event(pmu->name, "slots") returns false.
2. Run cpuid at perf userspace and avoid using 'slots' if it is not
supported in cpuid.
3. Do not fatal perf if the leader is failed. Do not create events for an
evsel if its leader is already failed.
This RFC patch is with the 3rd option. Would you mind suggesting which
option is better?
Here is the output of patch.
$ perf stat -v
Using CPUID GenuineIntel-6-6A-6
slots -> cpu/event=0,umask=0x4/
topdown-retiring -> cpu/event=0,umask=0x80/
topdown-bad-spec -> cpu/event=0,umask=0x81/
topdown-fe-bound -> cpu/event=0,umask=0x82/
topdown-be-bound -> cpu/event=0,umask=0x83/
Control descriptor is not initialized
Warning:
slots event is not supported by the kernel.
^Ccpu-clock: 62021481051 62021480237 62021480237
context-switches: 437 62021478064 62021478064
cpu-migrations: 17 62021475294 62021475294
page-faults: 12 62021471925 62021471925
cycles: 15662273 62020909141 62020909141
instructions: 6580385 62008944246 62008944246
branches: 1446119 62008855550 62008855550
branch-misses: 30970 62008643255 62008643255
failed to read counter slots
failed to read counter topdown-retiring
failed to read counter topdown-bad-spec
failed to read counter topdown-fe-bound
failed to read counter topdown-be-bound
Performance counter stats for 'system wide':
62,021.48 msec cpu-clock # 16.006 CPUs utilized
437 context-switches # 7.046 /sec
17 cpu-migrations # 0.274 /sec
12 page-faults # 0.193 /sec
15,662,273 cycles # 0.000 GHz
6,580,385 instructions # 0.42 insn per cycle
1,446,119 branches # 23.316 K/sec
30,970 branch-misses # 2.14% of all branches
<not supported> slots
<not supported> topdown-retiring
<not supported> topdown-bad-spec
<not supported> topdown-fe-bound
<not supported> topdown-be-bound
3.874991326 seconds time elapsed
Thank you very much!
Cc: Joe Jin <[email protected]>
Signed-off-by: Dongli Zhang <[email protected]>
---
tools/perf/builtin-stat.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 0b4a62e4ff67..1053cf0886c0 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -762,9 +762,7 @@ static enum counter_recovery stat_handle_error(struct evsel *counter)
*/
counter->errored = true;
- if ((evsel__leader(counter) != counter) ||
- !(counter->core.leader->nr_members > 1))
- return COUNTER_SKIP;
+ return COUNTER_SKIP;
} else if (evsel__fallback(counter, errno, msg, sizeof(msg))) {
if (verbose > 0)
ui__warning("%s\n", msg);
@@ -843,6 +841,8 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
if (target.use_bpf)
break;
+ if (evsel__leader(counter)->errored)
+ continue;
if (counter->reset_group || counter->errored)
continue;
if (evsel__is_bpf(counter))
@@ -901,6 +901,8 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
evlist__for_each_cpu(evlist_cpu_itr, evsel_list, affinity) {
counter = evlist_cpu_itr.evsel;
+ if (evsel__leader(counter)->errored)
+ continue;
if (!counter->reset_group && !counter->errored)
continue;
if (!counter->reset_group)
--
2.17.1
On 22/9/2022 3:10 pm, Dongli Zhang wrote:
> There are three options to fix the issue.
>
> 1. Do not expose /sys/bus/event_source/devices/cpu/events/slots to
> userspace so that pmu_have_event(pmu->name, "slots") returns false.
IMO, the guest PMU driver should be fixed
since it misrepresents emulated hardware capabilities in terms of slots.
>
> 2. Run cpuid at perf userspace and avoid using 'slots' if it is not
> supported in cpuid.
>
> 3. Do not fatal perf if the leader is failed. Do not create events for an
> evsel if its leader is already failed.
We may also need this since it's easier and more agile to update the perf tool
than the kernel code or KVM emulated capabilities.
>
> This RFC patch is with the 3rd option. Would you mind suggesting which
> option is better?
On 2022-09-22 4:07 a.m., Like Xu wrote:
> On 22/9/2022 3:10 pm, Dongli Zhang wrote:
>> There are three options to fix the issue.
>>
>> 1. Do not expose /sys/bus/event_source/devices/cpu/events/slots to
>> userspace so that pmu_have_event(pmu->name, "slots") returns false.
>
> IMO, the guest PMU driver should be fixed
> since it misrepresents emulated hardware capabilities in terms of slots.
Yes, we need to fix the kernel to hide the slots event if it's not
available.
The patch as below should fix it. (Not tested yet)
diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index b20e646c8205..27ee43faba32 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -5565,6 +5565,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 in CPUID.
+ */
+ 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)
{
@@ -5600,6 +5613,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 = {
@@ -5758,6 +5772,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)
{
@@ -5784,7 +5815,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 = {
Thanks,
Kan
Hi Kan,
I have tested that the patch works by hiding 'slots' sysfs entries.
# ll /sys/bus/event_source/devices/cpu/events/
total 0
-r--r--r--. 1 root root 4096 Sep 22 17:57 branch-instructions
-r--r--r--. 1 root root 4096 Sep 22 17:57 branch-misses
-r--r--r--. 1 root root 4096 Sep 22 17:57 bus-cycles
-r--r--r--. 1 root root 4096 Sep 22 17:57 cache-misses
-r--r--r--. 1 root root 4096 Sep 22 17:57 cache-references
-r--r--r--. 1 root root 4096 Sep 22 17:57 cpu-cycles
-r--r--r--. 1 root root 4096 Sep 22 17:57 instructions
-r--r--r--. 1 root root 4096 Sep 22 17:57 ref-cycles
# perf stat
^C
Performance counter stats for 'system wide':
19,256.20 msec cpu-clock # 16.025 CPUs
utilized
179 context-switches # 9.296 /sec
17 cpu-migrations # 0.883 /sec
3 page-faults # 0.156 /sec
7,502,294 cycles # 0.000 GHz
2,512,587 instructions # 0.33 insn per
cycle
552,989 branches # 28.717 K/sec
15,999 branch-misses # 2.89% of all
branches
1.201628129 seconds time elapsed
Would you send this patch to fix at the kernel side?
Thank you very much!
Dongli Zhang
On 9/22/22 6:34 AM, Liang, Kan wrote:
>
>
> On 2022-09-22 4:07 a.m., Like Xu wrote:
>> On 22/9/2022 3:10 pm, Dongli Zhang wrote:
>>> There are three options to fix the issue.
>>>
>>> 1. Do not expose /sys/bus/event_source/devices/cpu/events/slots to
>>> userspace so that pmu_have_event(pmu->name, "slots") returns false.
>>
>> IMO, the guest PMU driver should be fixed
>> since it misrepresents emulated hardware capabilities in terms of slots.
>
> Yes, we need to fix the kernel to hide the slots event if it's not
> available.
>
> The patch as below should fix it. (Not tested yet)
>
> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> index b20e646c8205..27ee43faba32 100644
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
> @@ -5565,6 +5565,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 in CPUID.
> + */
> + 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)
> {
> @@ -5600,6 +5613,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 = {
> @@ -5758,6 +5772,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)
> {
> @@ -5784,7 +5815,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 = {
>
>
> Thanks,
> Kan
>
On 9/22/22 1:07 AM, Like Xu wrote:
> On 22/9/2022 3:10 pm, Dongli Zhang wrote:
>> There are three options to fix the issue.
>>
>> 1. Do not expose /sys/bus/event_source/devices/cpu/events/slots to
>> userspace so that pmu_have_event(pmu->name, "slots") returns false.
>
> IMO, the guest PMU driver should be fixed
> since it misrepresents emulated hardware capabilities in terms of slots.
>
>>
>> 2. Run cpuid at perf userspace and avoid using 'slots' if it is not
>> supported in cpuid.
>>
>> 3. Do not fatal perf if the leader is failed. Do not create events for an
>> evsel if its leader is already failed.
>
> We may also need this since it's easier and more agile to update the perf tool
> than the kernel code or KVM emulated capabilities.
Thank you very much for the suggestion. I may remove the RFC and re-send the
patch for perf userspace.
BTW, I used to try your prior patch at KVM side (suggested by Bert) but it does
not help in this case. The VM kernel is already aware that 'slots' is not
supported by the hypervisor.
https://lore.kernel.org/lkml/[email protected]/T/
Dongli Zhang
>
>>
>> This RFC patch is with the 3rd option. Would you mind suggesting which
>> option is better?
On 2022-09-22 2:00 p.m., Dongli Zhang wrote:
> Hi Kan,
>
> I have tested that the patch works by hiding 'slots' sysfs entries.
>
> # ll /sys/bus/event_source/devices/cpu/events/
> total 0
> -r--r--r--. 1 root root 4096 Sep 22 17:57 branch-instructions
> -r--r--r--. 1 root root 4096 Sep 22 17:57 branch-misses
> -r--r--r--. 1 root root 4096 Sep 22 17:57 bus-cycles
> -r--r--r--. 1 root root 4096 Sep 22 17:57 cache-misses
> -r--r--r--. 1 root root 4096 Sep 22 17:57 cache-references
> -r--r--r--. 1 root root 4096 Sep 22 17:57 cpu-cycles
> -r--r--r--. 1 root root 4096 Sep 22 17:57 instructions
> -r--r--r--. 1 root root 4096 Sep 22 17:57 ref-cycles
>
> # perf stat
> ^C
> Performance counter stats for 'system wide':
>
> 19,256.20 msec cpu-clock # 16.025 CPUs
> utilized
> 179 context-switches # 9.296 /sec
>
> 17 cpu-migrations # 0.883 /sec
>
> 3 page-faults # 0.156 /sec
>
> 7,502,294 cycles # 0.000 GHz
>
> 2,512,587 instructions # 0.33 insn per
> cycle
> 552,989 branches # 28.717 K/sec
>
> 15,999 branch-misses # 2.89% of all
> branches
>
> 1.201628129 seconds time elapsed
>
>
> Would you send this patch to fix at the kernel side?
Yes, I will send it out shortly.
Thanks,
Kan
>
> Thank you very much!
>
> Dongli Zhang
>
> On 9/22/22 6:34 AM, Liang, Kan wrote:
>>
>>
>> On 2022-09-22 4:07 a.m., Like Xu wrote:
>>> On 22/9/2022 3:10 pm, Dongli Zhang wrote:
>>>> There are three options to fix the issue.
>>>>
>>>> 1. Do not expose /sys/bus/event_source/devices/cpu/events/slots to
>>>> userspace so that pmu_have_event(pmu->name, "slots") returns false.
>>>
>>> IMO, the guest PMU driver should be fixed
>>> since it misrepresents emulated hardware capabilities in terms of slots.
>>
>> Yes, we need to fix the kernel to hide the slots event if it's not
>> available.
>>
>> The patch as below should fix it. (Not tested yet)
>>
>> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
>> index b20e646c8205..27ee43faba32 100644
>> --- a/arch/x86/events/intel/core.c
>> +++ b/arch/x86/events/intel/core.c
>> @@ -5565,6 +5565,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 in CPUID.
>> + */
>> + 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)
>> {
>> @@ -5600,6 +5613,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 = {
>> @@ -5758,6 +5772,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)
>> {
>> @@ -5784,7 +5815,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 = {
>>
>>
>> Thanks,
>> Kan
>>
Hello,
On Thu, Sep 22, 2022 at 12:10 AM Dongli Zhang <[email protected]> wrote:
>
> Add [email protected] as this issue is in virtualization env.
>
> The topdown metrics events became default since
> commit 42641d6f4d15 ("perf stat: Add Topdown metrics events as default
> events"). The perf will use 'slots' if the
> /sys/bus/event_source/devices/cpu/events/slots is available.
>
> Unfortunately, the 'slots' may not be supported in the virualization
> environment. The hypervisor may not expose the 'slots' counter to the VM
> in cpuid. As a result, the kernel may disable topdown slots and metrics
> events in intel_pmu_init() if slots event is not in CPUID. E.g., both
> c->weight and c->idxmsk64 are set to 0.
>
> There will be below error on Icelake VM since 'slots' is the leader:
>
> $ 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.
>
> This is because the stat_handle_error() returns COUNTER_FATAL when the
> 'slots' is used as leader of events.
>
> There are three options to fix the issue.
>
> 1. Do not expose /sys/bus/event_source/devices/cpu/events/slots to
> userspace so that pmu_have_event(pmu->name, "slots") returns false.
>
> 2. Run cpuid at perf userspace and avoid using 'slots' if it is not
> supported in cpuid.
>
> 3. Do not fatal perf if the leader is failed. Do not create events for an
> evsel if its leader is already failed.
>
> This RFC patch is with the 3rd option. Would you mind suggesting which
> option is better?
Sorry for the late reply but I think option 1 is the way to go.
The option 3 would be a transient workaround and it would affect
other events too. If it's really needed, I think option 2 is slightly better
than option 3. Or, we can add --force option to skip non-supported
events explicitly.
Thanks,
Namhyung
>
> Here is the output of patch.
>
> $ perf stat -v
> Using CPUID GenuineIntel-6-6A-6
> slots -> cpu/event=0,umask=0x4/
> topdown-retiring -> cpu/event=0,umask=0x80/
> topdown-bad-spec -> cpu/event=0,umask=0x81/
> topdown-fe-bound -> cpu/event=0,umask=0x82/
> topdown-be-bound -> cpu/event=0,umask=0x83/
> Control descriptor is not initialized
> Warning:
> slots event is not supported by the kernel.
> ^Ccpu-clock: 62021481051 62021480237 62021480237
> context-switches: 437 62021478064 62021478064
> cpu-migrations: 17 62021475294 62021475294
> page-faults: 12 62021471925 62021471925
> cycles: 15662273 62020909141 62020909141
> instructions: 6580385 62008944246 62008944246
> branches: 1446119 62008855550 62008855550
> branch-misses: 30970 62008643255 62008643255
> failed to read counter slots
> failed to read counter topdown-retiring
> failed to read counter topdown-bad-spec
> failed to read counter topdown-fe-bound
> failed to read counter topdown-be-bound
>
> Performance counter stats for 'system wide':
>
> 62,021.48 msec cpu-clock # 16.006 CPUs utilized
> 437 context-switches # 7.046 /sec
> 17 cpu-migrations # 0.274 /sec
> 12 page-faults # 0.193 /sec
> 15,662,273 cycles # 0.000 GHz
> 6,580,385 instructions # 0.42 insn per cycle
> 1,446,119 branches # 23.316 K/sec
> 30,970 branch-misses # 2.14% of all branches
> <not supported> slots
> <not supported> topdown-retiring
> <not supported> topdown-bad-spec
> <not supported> topdown-fe-bound
> <not supported> topdown-be-bound
>
> 3.874991326 seconds time elapsed
>
> Thank you very much!
>
> Cc: Joe Jin <[email protected]>
> Signed-off-by: Dongli Zhang <[email protected]>
> ---
> tools/perf/builtin-stat.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 0b4a62e4ff67..1053cf0886c0 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -762,9 +762,7 @@ static enum counter_recovery stat_handle_error(struct evsel *counter)
> */
> counter->errored = true;
>
> - if ((evsel__leader(counter) != counter) ||
> - !(counter->core.leader->nr_members > 1))
> - return COUNTER_SKIP;
> + return COUNTER_SKIP;
> } else if (evsel__fallback(counter, errno, msg, sizeof(msg))) {
> if (verbose > 0)
> ui__warning("%s\n", msg);
> @@ -843,6 +841,8 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
> if (target.use_bpf)
> break;
>
> + if (evsel__leader(counter)->errored)
> + continue;
> if (counter->reset_group || counter->errored)
> continue;
> if (evsel__is_bpf(counter))
> @@ -901,6 +901,8 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
> evlist__for_each_cpu(evlist_cpu_itr, evsel_list, affinity) {
> counter = evlist_cpu_itr.evsel;
>
> + if (evsel__leader(counter)->errored)
> + continue;
> if (!counter->reset_group && !counter->errored)
> continue;
> if (!counter->reset_group)
> --
> 2.17.1
>
Hi Namhyung,
On 10/14/22 3:16 PM, Namhyung Kim wrote:
> Hello,
>
> On Thu, Sep 22, 2022 at 12:10 AM Dongli Zhang <[email protected]> wrote:
>>
>> Add [email protected] as this issue is in virtualization env.
>>
>> The topdown metrics events became default since
>> commit 42641d6f4d15 ("perf stat: Add Topdown metrics events as default
>> events"). The perf will use 'slots' if the
>> /sys/bus/event_source/devices/cpu/events/slots is available.
>>
>> Unfortunately, the 'slots' may not be supported in the virualization
>> environment. The hypervisor may not expose the 'slots' counter to the VM
>> in cpuid. As a result, the kernel may disable topdown slots and metrics
>> events in intel_pmu_init() if slots event is not in CPUID. E.g., both
>> c->weight and c->idxmsk64 are set to 0.
>>
>> There will be below error on Icelake VM since 'slots' is the leader:
>>
>> $ 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.
>>
>> This is because the stat_handle_error() returns COUNTER_FATAL when the
>> 'slots' is used as leader of events.
>>
>> There are three options to fix the issue.
>>
>> 1. Do not expose /sys/bus/event_source/devices/cpu/events/slots to
>> userspace so that pmu_have_event(pmu->name, "slots") returns false.
>>
>> 2. Run cpuid at perf userspace and avoid using 'slots' if it is not
>> supported in cpuid.
>>
>> 3. Do not fatal perf if the leader is failed. Do not create events for an
>> evsel if its leader is already failed.
>>
>> This RFC patch is with the 3rd option. Would you mind suggesting which
>> option is better?
>
> Sorry for the late reply but I think option 1 is the way to go.
>
> The option 3 would be a transient workaround and it would affect
> other events too. If it's really needed, I think option 2 is slightly better
> than option 3. Or, we can add --force option to skip non-supported
> events explicitly.
About option 2 and 3, I will wait for more comments if anyone still prefers the
change at userspace, e.g., the "--force" option to avoid failure but just to
report non-supported.
I have already sent a version that may impact other events. Please ignore as you
think option 1 is enough.
https://lore.kernel.org/all/[email protected]/
About option 1, there is a bugfix from Ken pending for review.
https://lore.kernel.org/all/[email protected]/
Thank you very much!
Dongli Zhang
>
> Thanks,
> Namhyung
>
>>
>> Here is the output of patch.
>>
>> $ perf stat -v
>> Using CPUID GenuineIntel-6-6A-6
>> slots -> cpu/event=0,umask=0x4/
>> topdown-retiring -> cpu/event=0,umask=0x80/
>> topdown-bad-spec -> cpu/event=0,umask=0x81/
>> topdown-fe-bound -> cpu/event=0,umask=0x82/
>> topdown-be-bound -> cpu/event=0,umask=0x83/
>> Control descriptor is not initialized
>> Warning:
>> slots event is not supported by the kernel.
>> ^Ccpu-clock: 62021481051 62021480237 62021480237
>> context-switches: 437 62021478064 62021478064
>> cpu-migrations: 17 62021475294 62021475294
>> page-faults: 12 62021471925 62021471925
>> cycles: 15662273 62020909141 62020909141
>> instructions: 6580385 62008944246 62008944246
>> branches: 1446119 62008855550 62008855550
>> branch-misses: 30970 62008643255 62008643255
>> failed to read counter slots
>> failed to read counter topdown-retiring
>> failed to read counter topdown-bad-spec
>> failed to read counter topdown-fe-bound
>> failed to read counter topdown-be-bound
>>
>> Performance counter stats for 'system wide':
>>
>> 62,021.48 msec cpu-clock # 16.006 CPUs utilized
>> 437 context-switches # 7.046 /sec
>> 17 cpu-migrations # 0.274 /sec
>> 12 page-faults # 0.193 /sec
>> 15,662,273 cycles # 0.000 GHz
>> 6,580,385 instructions # 0.42 insn per cycle
>> 1,446,119 branches # 23.316 K/sec
>> 30,970 branch-misses # 2.14% of all branches
>> <not supported> slots
>> <not supported> topdown-retiring
>> <not supported> topdown-bad-spec
>> <not supported> topdown-fe-bound
>> <not supported> topdown-be-bound
>>
>> 3.874991326 seconds time elapsed
>>
>> Thank you very much!
>>
>> Cc: Joe Jin <[email protected]>
>> Signed-off-by: Dongli Zhang <[email protected]>
>> ---
>> tools/perf/builtin-stat.c | 8 +++++---
>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
>> index 0b4a62e4ff67..1053cf0886c0 100644
>> --- a/tools/perf/builtin-stat.c
>> +++ b/tools/perf/builtin-stat.c
>> @@ -762,9 +762,7 @@ static enum counter_recovery stat_handle_error(struct evsel *counter)
>> */
>> counter->errored = true;
>>
>> - if ((evsel__leader(counter) != counter) ||
>> - !(counter->core.leader->nr_members > 1))
>> - return COUNTER_SKIP;
>> + return COUNTER_SKIP;
>> } else if (evsel__fallback(counter, errno, msg, sizeof(msg))) {
>> if (verbose > 0)
>> ui__warning("%s\n", msg);
>> @@ -843,6 +841,8 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
>> if (target.use_bpf)
>> break;
>>
>> + if (evsel__leader(counter)->errored)
>> + continue;
>> if (counter->reset_group || counter->errored)
>> continue;
>> if (evsel__is_bpf(counter))
>> @@ -901,6 +901,8 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
>> evlist__for_each_cpu(evlist_cpu_itr, evsel_list, affinity) {
>> counter = evlist_cpu_itr.evsel;
>>
>> + if (evsel__leader(counter)->errored)
>> + continue;
>> if (!counter->reset_group && !counter->errored)
>> continue;
>> if (!counter->reset_group)
>> --
>> 2.17.1
>>
On 10/14/22 3:16 PM, Namhyung Kim wrote:
> Hello,
>
> On Thu, Sep 22, 2022 at 12:10 AM Dongli Zhang <[email protected]> wrote:
>>
>> Add [email protected] as this issue is in virtualization env.
>>
>> The topdown metrics events became default since
>> commit 42641d6f4d15 ("perf stat: Add Topdown metrics events as default
>> events"). The perf will use 'slots' if the
>> /sys/bus/event_source/devices/cpu/events/slots is available.
>>
>> Unfortunately, the 'slots' may not be supported in the virualization
>> environment. The hypervisor may not expose the 'slots' counter to the VM
>> in cpuid. As a result, the kernel may disable topdown slots and metrics
>> events in intel_pmu_init() if slots event is not in CPUID. E.g., both
>> c->weight and c->idxmsk64 are set to 0.
>>
>> There will be below error on Icelake VM since 'slots' is the leader:
>>
>> $ 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.
>>
>> This is because the stat_handle_error() returns COUNTER_FATAL when the
>> 'slots' is used as leader of events.
>>
>> There are three options to fix the issue.
>>
>> 1. Do not expose /sys/bus/event_source/devices/cpu/events/slots to
>> userspace so that pmu_have_event(pmu->name, "slots") returns false.
>>
>> 2. Run cpuid at perf userspace and avoid using 'slots' if it is not
>> supported in cpuid.
>>
>> 3. Do not fatal perf if the leader is failed. Do not create events for an
>> evsel if its leader is already failed.
>>
>> This RFC patch is with the 3rd option. Would you mind suggesting which
>> option is better?
>
> Sorry for the late reply but I think option 1 is the way to go.
>
> The option 3 would be a transient workaround and it would affect
> other events too. If it's really needed, I think option 2 is slightly better
> than option 3. Or, we can add --force option to skip non-supported
> events explicitly.
>
> Thanks,
> Namhyung
>
>>
>> Here is the output of patch.
>>
>> $ perf stat -v
>> Using CPUID GenuineIntel-6-6A-6
>> slots -> cpu/event=0,umask=0x4/
>> topdown-retiring -> cpu/event=0,umask=0x80/
>> topdown-bad-spec -> cpu/event=0,umask=0x81/
>> topdown-fe-bound -> cpu/event=0,umask=0x82/
>> topdown-be-bound -> cpu/event=0,umask=0x83/
>> Control descriptor is not initialized
>> Warning:
>> slots event is not supported by the kernel.
>> ^Ccpu-clock: 62021481051 62021480237 62021480237
>> context-switches: 437 62021478064 62021478064
>> cpu-migrations: 17 62021475294 62021475294
>> page-faults: 12 62021471925 62021471925
>> cycles: 15662273 62020909141 62020909141
>> instructions: 6580385 62008944246 62008944246
>> branches: 1446119 62008855550 62008855550
>> branch-misses: 30970 62008643255 62008643255
>> failed to read counter slots
>> failed to read counter topdown-retiring
>> failed to read counter topdown-bad-spec
>> failed to read counter topdown-fe-bound
>> failed to read counter topdown-be-bound
>>
>> Performance counter stats for 'system wide':
>>
>> 62,021.48 msec cpu-clock # 16.006 CPUs utilized
>> 437 context-switches # 7.046 /sec
>> 17 cpu-migrations # 0.274 /sec
>> 12 page-faults # 0.193 /sec
>> 15,662,273 cycles # 0.000 GHz
>> 6,580,385 instructions # 0.42 insn per cycle
>> 1,446,119 branches # 23.316 K/sec
>> 30,970 branch-misses # 2.14% of all branches
>> <not supported> slots
>> <not supported> topdown-retiring
>> <not supported> topdown-bad-spec
>> <not supported> topdown-fe-bound
>> <not supported> topdown-be-bound
>>
>> 3.874991326 seconds time elapsed
>>
>> Thank you very much!
>>
>> Cc: Joe Jin <[email protected]>
>> Signed-off-by: Dongli Zhang <[email protected]>
>> ---
>> tools/perf/builtin-stat.c | 8 +++++---
>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
>> index 0b4a62e4ff67..1053cf0886c0 100644
>> --- a/tools/perf/builtin-stat.c
>> +++ b/tools/perf/builtin-stat.c
>> @@ -762,9 +762,7 @@ static enum counter_recovery stat_handle_error(struct evsel *counter)
>> */
>> counter->errored = true;
>>
>> - if ((evsel__leader(counter) != counter) ||
>> - !(counter->core.leader->nr_members > 1))
>> - return COUNTER_SKIP;
>> + return COUNTER_SKIP;
>> } else if (evsel__fallback(counter, errno, msg, sizeof(msg))) {
>> if (verbose > 0)
>> ui__warning("%s\n", msg);
>> @@ -843,6 +841,8 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
>> if (target.use_bpf)
>> break;
>>
>> + if (evsel__leader(counter)->errored)
>> + continue;
>> if (counter->reset_group || counter->errored)
>> continue;
>> if (evsel__is_bpf(counter))
>> @@ -901,6 +901,8 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
>> evlist__for_each_cpu(evlist_cpu_itr, evsel_list, affinity) {
>> counter = evlist_cpu_itr.evsel;
>>
>> + if (evsel__leader(counter)->errored)
>> + continue;
>> if (!counter->reset_group && !counter->errored)
>> continue;
>> if (!counter->reset_group)
>> --
>> 2.17.1
>>
Please ignore this email. I carelessly clicked unwanted buttons on my
Thunderbird client.
Sorry for spamming.
Dongli Zhang
On 10/18/22 2:29 PM, Dongli Zhang wrote:
>
>
> On 10/14/22 3:16 PM, Namhyung Kim wrote:
>> Hello,
>>
>> On Thu, Sep 22, 2022 at 12:10 AM Dongli Zhang <[email protected]> wrote:
>>>
>>> Add [email protected] as this issue is in virtualization env.
>>>
>>> The topdown metrics events became default since
>>> commit 42641d6f4d15 ("perf stat: Add Topdown metrics events as default
>>> events"). The perf will use 'slots' if the
>>> /sys/bus/event_source/devices/cpu/events/slots is available.
>>>
>>> Unfortunately, the 'slots' may not be supported in the virualization
>>> environment. The hypervisor may not expose the 'slots' counter to the VM
>>> in cpuid. As a result, the kernel may disable topdown slots and metrics
>>> events in intel_pmu_init() if slots event is not in CPUID. E.g., both
>>> c->weight and c->idxmsk64 are set to 0.
>>>
>>> There will be below error on Icelake VM since 'slots' is the leader:
>>>
>>> $ 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.
>>>
>>> This is because the stat_handle_error() returns COUNTER_FATAL when the
>>> 'slots' is used as leader of events.
>>>
>>> There are three options to fix the issue.
>>>
>>> 1. Do not expose /sys/bus/event_source/devices/cpu/events/slots to
>>> userspace so that pmu_have_event(pmu->name, "slots") returns false.
>>>
>>> 2. Run cpuid at perf userspace and avoid using 'slots' if it is not
>>> supported in cpuid.
>>>
>>> 3. Do not fatal perf if the leader is failed. Do not create events for an
>>> evsel if its leader is already failed.
>>>
>>> This RFC patch is with the 3rd option. Would you mind suggesting which
>>> option is better?
>>
>> Sorry for the late reply but I think option 1 is the way to go.
>>
>> The option 3 would be a transient workaround and it would affect
>> other events too. If it's really needed, I think option 2 is slightly better
>> than option 3. Or, we can add --force option to skip non-supported
>> events explicitly.
>>
>> Thanks,
>> Namhyung
>>
>>>
>>> Here is the output of patch.
>>>
>>> $ perf stat -v
>>> Using CPUID GenuineIntel-6-6A-6
>>> slots -> cpu/event=0,umask=0x4/
>>> topdown-retiring -> cpu/event=0,umask=0x80/
>>> topdown-bad-spec -> cpu/event=0,umask=0x81/
>>> topdown-fe-bound -> cpu/event=0,umask=0x82/
>>> topdown-be-bound -> cpu/event=0,umask=0x83/
>>> Control descriptor is not initialized
>>> Warning:
>>> slots event is not supported by the kernel.
>>> ^Ccpu-clock: 62021481051 62021480237 62021480237
>>> context-switches: 437 62021478064 62021478064
>>> cpu-migrations: 17 62021475294 62021475294
>>> page-faults: 12 62021471925 62021471925
>>> cycles: 15662273 62020909141 62020909141
>>> instructions: 6580385 62008944246 62008944246
>>> branches: 1446119 62008855550 62008855550
>>> branch-misses: 30970 62008643255 62008643255
>>> failed to read counter slots
>>> failed to read counter topdown-retiring
>>> failed to read counter topdown-bad-spec
>>> failed to read counter topdown-fe-bound
>>> failed to read counter topdown-be-bound
>>>
>>> Performance counter stats for 'system wide':
>>>
>>> 62,021.48 msec cpu-clock # 16.006 CPUs utilized
>>> 437 context-switches # 7.046 /sec
>>> 17 cpu-migrations # 0.274 /sec
>>> 12 page-faults # 0.193 /sec
>>> 15,662,273 cycles # 0.000 GHz
>>> 6,580,385 instructions # 0.42 insn per cycle
>>> 1,446,119 branches # 23.316 K/sec
>>> 30,970 branch-misses # 2.14% of all branches
>>> <not supported> slots
>>> <not supported> topdown-retiring
>>> <not supported> topdown-bad-spec
>>> <not supported> topdown-fe-bound
>>> <not supported> topdown-be-bound
>>>
>>> 3.874991326 seconds time elapsed
>>>
>>> Thank you very much!
>>>
>>> Cc: Joe Jin <[email protected]>
>>> Signed-off-by: Dongli Zhang <[email protected]>
>>> ---
>>> tools/perf/builtin-stat.c | 8 +++++---
>>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
>>> index 0b4a62e4ff67..1053cf0886c0 100644
>>> --- a/tools/perf/builtin-stat.c
>>> +++ b/tools/perf/builtin-stat.c
>>> @@ -762,9 +762,7 @@ static enum counter_recovery stat_handle_error(struct evsel *counter)
>>> */
>>> counter->errored = true;
>>>
>>> - if ((evsel__leader(counter) != counter) ||
>>> - !(counter->core.leader->nr_members > 1))
>>> - return COUNTER_SKIP;
>>> + return COUNTER_SKIP;
>>> } else if (evsel__fallback(counter, errno, msg, sizeof(msg))) {
>>> if (verbose > 0)
>>> ui__warning("%s\n", msg);
>>> @@ -843,6 +841,8 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
>>> if (target.use_bpf)
>>> break;
>>>
>>> + if (evsel__leader(counter)->errored)
>>> + continue;
>>> if (counter->reset_group || counter->errored)
>>> continue;
>>> if (evsel__is_bpf(counter))
>>> @@ -901,6 +901,8 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
>>> evlist__for_each_cpu(evlist_cpu_itr, evsel_list, affinity) {
>>> counter = evlist_cpu_itr.evsel;
>>>
>>> + if (evsel__leader(counter)->errored)
>>> + continue;
>>> if (!counter->reset_group && !counter->errored)
>>> continue;
>>> if (!counter->reset_group)
>>> --
>>> 2.17.1
>>>