| Message ID | 20240410095833.63934-1-yangyicong@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] perf: arm_pmu: Only show online CPUs in device's "cpus" attribute | expand |
On Wed, Apr 10, 2024 at 05:58:32PM +0800, Yicong Yang wrote: > From: Yicong Yang <yangyicong@hisilicon.com> > > When there're CPUs offline after system booting, perf will failed: > [root@localhost ~]# /home/yang/perf stat -a -e armv8_pmuv3_0/cycles/ > Error: > The sys_perf_event_open() syscall returned with 19 (No such device) for event (cpu-clock). > /bin/dmesg | grep -i perf may provide additional information. > > This is due to PMU's "cpus" is not updated and still contains offline > CPUs and perf will try to open perf event on the offlined CPUs. > > Make "cpus" attribute only shows online CPUs and introduced a new > "supported_cpus" where users can get the range of the CPUs this > PMU supported monitoring. > > Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> > --- > drivers/perf/arm_pmu.c | 24 +++++++++++++++++++++++- > 1 file changed, 23 insertions(+), 1 deletion(-) Hmm. Is the complexity in the driver really worth it here? CPUs can be onlined and offlined after the perf_event_open() syscall has been executed, so this feels like something userspace should be aware of and handle on a best-effort basis anyway. Does x86 get away with this because CPU0 is never offlined? Will
On 2024/4/10 23:34, Will Deacon wrote: > On Wed, Apr 10, 2024 at 05:58:32PM +0800, Yicong Yang wrote: >> From: Yicong Yang <yangyicong@hisilicon.com> >> >> When there're CPUs offline after system booting, perf will failed: >> [root@localhost ~]# /home/yang/perf stat -a -e armv8_pmuv3_0/cycles/ >> Error: >> The sys_perf_event_open() syscall returned with 19 (No such device) for event (cpu-clock). >> /bin/dmesg | grep -i perf may provide additional information. >> >> This is due to PMU's "cpus" is not updated and still contains offline >> CPUs and perf will try to open perf event on the offlined CPUs. >> >> Make "cpus" attribute only shows online CPUs and introduced a new >> "supported_cpus" where users can get the range of the CPUs this >> PMU supported monitoring. >> >> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> >> --- >> drivers/perf/arm_pmu.c | 24 +++++++++++++++++++++++- >> 1 file changed, 23 insertions(+), 1 deletion(-) > > Hmm. Is the complexity in the driver really worth it here? CPUs can be > onlined and offlined after the perf_event_open() syscall has been > executed, Yes. So we have cpuhp callbacks to handle the cpu online/offline and migrate the perf context. > so this feels like something userspace should be aware of and > handle on a best-effort basis anyway. > Looks like it's a convention for a PMU device to provide a "cpus" attribute (for core PMUs) or "cpumask" attribute (for uncore PMUs) to indicates the CPUs on which the events can be opened. If no such attributes provided, all online CPUs indicated. Perf will check this and if user doesn't specify a certian range of CPUs the events will be opened on all the CPUs PMU indicated. > Does x86 get away with this because CPU0 is never offlined? > Checked on my x86 server there's no "cpus" or "cpumask" provided so perf will try to open the events on all the online CPUs if no CPU range specified. But for their hybrid platform there do have a "cpus" attribute[1] and it'll be updated when CPU offline[2]. The arm-cspmu also provides a "cpumask" to indicate supported online CPUs and an "associated_cpus" to indicated the CPUs related to the PMU. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/events/intel/core.c?h=v6.9-rc1#n5931 [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/events/intel/core.c?h=v6.9-rc1#n4949 Thanks.
On 4/11/24 01:55, Yicong Yang wrote: > On 2024/4/10 23:34, Will Deacon wrote: >> On Wed, Apr 10, 2024 at 05:58:32PM +0800, Yicong Yang wrote: >>> From: Yicong Yang <yangyicong@hisilicon.com> >>> >>> When there're CPUs offline after system booting, perf will failed: >>> [root@localhost ~]# /home/yang/perf stat -a -e armv8_pmuv3_0/cycles/ >>> Error: >>> The sys_perf_event_open() syscall returned with 19 (No such device) for event (cpu-clock). >>> /bin/dmesg | grep -i perf may provide additional information. >>> >>> This is due to PMU's "cpus" is not updated and still contains offline >>> CPUs and perf will try to open perf event on the offlined CPUs. >>> >>> Make "cpus" attribute only shows online CPUs and introduced a new >>> "supported_cpus" where users can get the range of the CPUs this >>> PMU supported monitoring. >>> >>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> >>> --- >>> drivers/perf/arm_pmu.c | 24 +++++++++++++++++++++++- >>> 1 file changed, 23 insertions(+), 1 deletion(-) >> >> Hmm. Is the complexity in the driver really worth it here? CPUs can be >> onlined and offlined after the perf_event_open() syscall has been >> executed, > > Yes. So we have cpuhp callbacks to handle the cpu online/offline > and migrate the perf context. > >> so this feels like something userspace should be aware of and >> handle on a best-effort basis anyway. >> > > Looks like it's a convention for a PMU device to provide a "cpus" attribute (for core > PMUs) or "cpumask" attribute (for uncore PMUs) to indicates the CPUs on which the > events can be opened. If no such attributes provided, all online CPUs indicated. Perf > will check this and if user doesn't specify a certian range of CPUs the events will > be opened on all the CPUs PMU indicated. > >> Does x86 get away with this because CPU0 is never offlined? >> > > Checked on my x86 server there's no "cpus" or "cpumask" provided so perf will try > to open the events on all the online CPUs if no CPU range specified. But for their > hybrid platform there do have a "cpus" attribute[1] and it'll be updated when CPU > offline[2]. > > The arm-cspmu also provides a "cpumask" to indicate supported online CPUs and an > "associated_cpus" to indicated the CPUs related to the PMU. > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/events/intel/core.c?h=v6.9-rc1#n5931 > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/events/intel/core.c?h=v6.9-rc1#n4949 > > Thanks. > > The arm_dsu has the concepts of 'cpumask' as well. It also has 'associated_cpus'. When the current cpumask offline, the cpuhp handler will migrate the cpumask to other associated_cpus. # cat /sys/devices/arm_dsu_26/associated_cpus 4-5 [root@lse-aarch64-bm-ol8 opc]# cat /sys/devices/arm_dsu_26/cpumask 4 812 static int dsu_pmu_cpu_online(unsigned int cpu, struct hlist_node *node) 813 { 814 struct dsu_pmu *dsu_pmu = hlist_entry_safe(node, struct dsu_pmu, 815 cpuhp_node); 816 817 if (!cpumask_test_cpu(cpu, &dsu_pmu->associated_cpus)) 818 return 0; 819 820 /* If the PMU is already managed, there is nothing to do */ 821 if (!cpumask_empty(&dsu_pmu->active_cpu)) 822 return 0; 823 824 dsu_pmu_init_pmu(dsu_pmu); 825 dsu_pmu_set_active_cpu(cpu, dsu_pmu); 826 827 return 0; 828 } 829 830 static int dsu_pmu_cpu_teardown(unsigned int cpu, struct hlist_node *node) 831 { 832 int dst; 833 struct dsu_pmu *dsu_pmu = hlist_entry_safe(node, struct dsu_pmu, 834 cpuhp_node); 835 836 if (!cpumask_test_and_clear_cpu(cpu, &dsu_pmu->active_cpu)) 837 return 0; 838 839 dst = dsu_pmu_get_online_cpu_any_but(dsu_pmu, cpu); 840 /* If there are no active CPUs in the DSU, leave IRQ disabled */ 841 if (dst >= nr_cpu_ids) 842 return 0; 843 844 perf_pmu_migrate_context(&dsu_pmu->pmu, cpu, dst); 845 dsu_pmu_set_active_cpu(dst, dsu_pmu); 846 847 return 0; 848 } However, I think the userspace perf tool looks more friendly (just return <not supported>) in this case when I offline all CPUs from cpumask of a DSU. Perhaps because it is NULL now. # perf stat -e arm_dsu_26/l3d_cache_wb/ ^C Performance counter stats for 'system wide': <not supported> arm_dsu_26/l3d_cache_wb/ 0.553294766 seconds time elapsed # cat /sys/devices/arm_dsu_26/associated_cpus 4-5 # cat /sys/devices/arm_dsu_26/cpumask 4 # echo 0 > /sys/devices/system/cpu/cpu4/online # cat /sys/devices/arm_dsu_26/cpumask 5 # echo 0 > /sys/devices/system/cpu/cpu5/online # cat /sys/devices/arm_dsu_26/cpumask # Dongli Zhang
Ping? Is there any plan to move forward with the patch from Yicong? Thank you very much! Dongli Zhang On 4/18/24 9:32 AM, Dongli Zhang wrote: > > > On 4/11/24 01:55, Yicong Yang wrote: >> On 2024/4/10 23:34, Will Deacon wrote: >>> On Wed, Apr 10, 2024 at 05:58:32PM +0800, Yicong Yang wrote: >>>> From: Yicong Yang <yangyicong@hisilicon.com> >>>> >>>> When there're CPUs offline after system booting, perf will failed: >>>> [root@localhost ~]# /home/yang/perf stat -a -e armv8_pmuv3_0/cycles/ >>>> Error: >>>> The sys_perf_event_open() syscall returned with 19 (No such device) for event (cpu-clock). >>>> /bin/dmesg | grep -i perf may provide additional information. >>>> >>>> This is due to PMU's "cpus" is not updated and still contains offline >>>> CPUs and perf will try to open perf event on the offlined CPUs. >>>> >>>> Make "cpus" attribute only shows online CPUs and introduced a new >>>> "supported_cpus" where users can get the range of the CPUs this >>>> PMU supported monitoring. >>>> >>>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> >>>> --- >>>> drivers/perf/arm_pmu.c | 24 +++++++++++++++++++++++- >>>> 1 file changed, 23 insertions(+), 1 deletion(-) >>> >>> Hmm. Is the complexity in the driver really worth it here? CPUs can be >>> onlined and offlined after the perf_event_open() syscall has been >>> executed, >> >> Yes. So we have cpuhp callbacks to handle the cpu online/offline >> and migrate the perf context. >> >>> so this feels like something userspace should be aware of and >>> handle on a best-effort basis anyway. >>> >> >> Looks like it's a convention for a PMU device to provide a "cpus" attribute (for core >> PMUs) or "cpumask" attribute (for uncore PMUs) to indicates the CPUs on which the >> events can be opened. If no such attributes provided, all online CPUs indicated. Perf >> will check this and if user doesn't specify a certian range of CPUs the events will >> be opened on all the CPUs PMU indicated. >> >>> Does x86 get away with this because CPU0 is never offlined? >>> >> >> Checked on my x86 server there's no "cpus" or "cpumask" provided so perf will try >> to open the events on all the online CPUs if no CPU range specified. But for their >> hybrid platform there do have a "cpus" attribute[1] and it'll be updated when CPU >> offline[2]. >> >> The arm-cspmu also provides a "cpumask" to indicate supported online CPUs and an >> "associated_cpus" to indicated the CPUs related to the PMU. >> >> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/events/intel/core.c?h=v6.9-rc1#n5931 >> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/events/intel/core.c?h=v6.9-rc1#n4949 >> >> Thanks. >> >> > > > The arm_dsu has the concepts of 'cpumask' as well. It also has 'associated_cpus'. > > When the current cpumask offline, the cpuhp handler will migrate the cpumask to > other associated_cpus. > > # cat /sys/devices/arm_dsu_26/associated_cpus > 4-5 > [root@lse-aarch64-bm-ol8 opc]# cat /sys/devices/arm_dsu_26/cpumask > 4 > > 812 static int dsu_pmu_cpu_online(unsigned int cpu, struct hlist_node *node) > 813 { > 814 struct dsu_pmu *dsu_pmu = hlist_entry_safe(node, struct dsu_pmu, > 815 cpuhp_node); > 816 > 817 if (!cpumask_test_cpu(cpu, &dsu_pmu->associated_cpus)) > 818 return 0; > 819 > 820 /* If the PMU is already managed, there is nothing to do */ > 821 if (!cpumask_empty(&dsu_pmu->active_cpu)) > 822 return 0; > 823 > 824 dsu_pmu_init_pmu(dsu_pmu); > 825 dsu_pmu_set_active_cpu(cpu, dsu_pmu); > 826 > 827 return 0; > 828 } > 829 > 830 static int dsu_pmu_cpu_teardown(unsigned int cpu, struct hlist_node *node) > 831 { > 832 int dst; > 833 struct dsu_pmu *dsu_pmu = hlist_entry_safe(node, struct dsu_pmu, > 834 cpuhp_node); > 835 > 836 if (!cpumask_test_and_clear_cpu(cpu, &dsu_pmu->active_cpu)) > 837 return 0; > 838 > 839 dst = dsu_pmu_get_online_cpu_any_but(dsu_pmu, cpu); > 840 /* If there are no active CPUs in the DSU, leave IRQ disabled */ > 841 if (dst >= nr_cpu_ids) > 842 return 0; > 843 > 844 perf_pmu_migrate_context(&dsu_pmu->pmu, cpu, dst); > 845 dsu_pmu_set_active_cpu(dst, dsu_pmu); > 846 > 847 return 0; > 848 } > > > However, I think the userspace perf tool looks more friendly (just return <not > supported>) in this case when I offline all CPUs from cpumask of a DSU. Perhaps > because it is NULL now. > > # perf stat -e arm_dsu_26/l3d_cache_wb/ > ^C > Performance counter stats for 'system wide': > > <not supported> arm_dsu_26/l3d_cache_wb/ > > 0.553294766 seconds time elapsed > > > # cat /sys/devices/arm_dsu_26/associated_cpus > 4-5 > # cat /sys/devices/arm_dsu_26/cpumask > 4 > # echo 0 > /sys/devices/system/cpu/cpu4/online > # cat /sys/devices/arm_dsu_26/cpumask > 5 > # echo 0 > /sys/devices/system/cpu/cpu5/online > # cat /sys/devices/arm_dsu_26/cpumask > > # > > Dongli Zhang
Hi Dongli, Since it's merge window now, I can resend this along with the userspace perf handling in next cycle. We can continue the discussion then. Thanks. On 2024/5/16 6:10, Dongli Zhang wrote: > Ping? Is there any plan to move forward with the patch from Yicong? > > Thank you very much! > > Dongli Zhang > > On 4/18/24 9:32 AM, Dongli Zhang wrote: >> >> >> On 4/11/24 01:55, Yicong Yang wrote: >>> On 2024/4/10 23:34, Will Deacon wrote: >>>> On Wed, Apr 10, 2024 at 05:58:32PM +0800, Yicong Yang wrote: >>>>> From: Yicong Yang <yangyicong@hisilicon.com> >>>>> >>>>> When there're CPUs offline after system booting, perf will failed: >>>>> [root@localhost ~]# /home/yang/perf stat -a -e armv8_pmuv3_0/cycles/ >>>>> Error: >>>>> The sys_perf_event_open() syscall returned with 19 (No such device) for event (cpu-clock). >>>>> /bin/dmesg | grep -i perf may provide additional information. >>>>> >>>>> This is due to PMU's "cpus" is not updated and still contains offline >>>>> CPUs and perf will try to open perf event on the offlined CPUs. >>>>> >>>>> Make "cpus" attribute only shows online CPUs and introduced a new >>>>> "supported_cpus" where users can get the range of the CPUs this >>>>> PMU supported monitoring. >>>>> >>>>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> >>>>> --- >>>>> drivers/perf/arm_pmu.c | 24 +++++++++++++++++++++++- >>>>> 1 file changed, 23 insertions(+), 1 deletion(-) >>>> >>>> Hmm. Is the complexity in the driver really worth it here? CPUs can be >>>> onlined and offlined after the perf_event_open() syscall has been >>>> executed, >>> >>> Yes. So we have cpuhp callbacks to handle the cpu online/offline >>> and migrate the perf context. >>> >>>> so this feels like something userspace should be aware of and >>>> handle on a best-effort basis anyway. >>>> >>> >>> Looks like it's a convention for a PMU device to provide a "cpus" attribute (for core >>> PMUs) or "cpumask" attribute (for uncore PMUs) to indicates the CPUs on which the >>> events can be opened. If no such attributes provided, all online CPUs indicated. Perf >>> will check this and if user doesn't specify a certian range of CPUs the events will >>> be opened on all the CPUs PMU indicated. >>> >>>> Does x86 get away with this because CPU0 is never offlined? >>>> >>> >>> Checked on my x86 server there's no "cpus" or "cpumask" provided so perf will try >>> to open the events on all the online CPUs if no CPU range specified. But for their >>> hybrid platform there do have a "cpus" attribute[1] and it'll be updated when CPU >>> offline[2]. >>> >>> The arm-cspmu also provides a "cpumask" to indicate supported online CPUs and an >>> "associated_cpus" to indicated the CPUs related to the PMU. >>> >>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/events/intel/core.c?h=v6.9-rc1#n5931 >>> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/events/intel/core.c?h=v6.9-rc1#n4949 >>> >>> Thanks. >>> >>> >> >> >> The arm_dsu has the concepts of 'cpumask' as well. It also has 'associated_cpus'. >> >> When the current cpumask offline, the cpuhp handler will migrate the cpumask to >> other associated_cpus. >> >> # cat /sys/devices/arm_dsu_26/associated_cpus >> 4-5 >> [root@lse-aarch64-bm-ol8 opc]# cat /sys/devices/arm_dsu_26/cpumask >> 4 >> >> 812 static int dsu_pmu_cpu_online(unsigned int cpu, struct hlist_node *node) >> 813 { >> 814 struct dsu_pmu *dsu_pmu = hlist_entry_safe(node, struct dsu_pmu, >> 815 cpuhp_node); >> 816 >> 817 if (!cpumask_test_cpu(cpu, &dsu_pmu->associated_cpus)) >> 818 return 0; >> 819 >> 820 /* If the PMU is already managed, there is nothing to do */ >> 821 if (!cpumask_empty(&dsu_pmu->active_cpu)) >> 822 return 0; >> 823 >> 824 dsu_pmu_init_pmu(dsu_pmu); >> 825 dsu_pmu_set_active_cpu(cpu, dsu_pmu); >> 826 >> 827 return 0; >> 828 } >> 829 >> 830 static int dsu_pmu_cpu_teardown(unsigned int cpu, struct hlist_node *node) >> 831 { >> 832 int dst; >> 833 struct dsu_pmu *dsu_pmu = hlist_entry_safe(node, struct dsu_pmu, >> 834 cpuhp_node); >> 835 >> 836 if (!cpumask_test_and_clear_cpu(cpu, &dsu_pmu->active_cpu)) >> 837 return 0; >> 838 >> 839 dst = dsu_pmu_get_online_cpu_any_but(dsu_pmu, cpu); >> 840 /* If there are no active CPUs in the DSU, leave IRQ disabled */ >> 841 if (dst >= nr_cpu_ids) >> 842 return 0; >> 843 >> 844 perf_pmu_migrate_context(&dsu_pmu->pmu, cpu, dst); >> 845 dsu_pmu_set_active_cpu(dst, dsu_pmu); >> 846 >> 847 return 0; >> 848 } >> >> >> However, I think the userspace perf tool looks more friendly (just return <not >> supported>) in this case when I offline all CPUs from cpumask of a DSU. Perhaps >> because it is NULL now. >> >> # perf stat -e arm_dsu_26/l3d_cache_wb/ >> ^C >> Performance counter stats for 'system wide': >> >> <not supported> arm_dsu_26/l3d_cache_wb/ >> >> 0.553294766 seconds time elapsed >> >> >> # cat /sys/devices/arm_dsu_26/associated_cpus >> 4-5 >> # cat /sys/devices/arm_dsu_26/cpumask >> 4 >> # echo 0 > /sys/devices/system/cpu/cpu4/online >> # cat /sys/devices/arm_dsu_26/cpumask >> 5 >> # echo 0 > /sys/devices/system/cpu/cpu5/online >> # cat /sys/devices/arm_dsu_26/cpumask >> >> # >> >> Dongli Zhang > . >
diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c index 8458fe2cebb4..acbb0e1d0414 100644 --- a/drivers/perf/arm_pmu.c +++ b/drivers/perf/arm_pmu.c @@ -558,13 +558,35 @@ static ssize_t cpus_show(struct device *dev, struct device_attribute *attr, char *buf) { struct arm_pmu *armpmu = to_arm_pmu(dev_get_drvdata(dev)); - return cpumap_print_to_pagebuf(true, buf, &armpmu->supported_cpus); + cpumask_var_t mask; + ssize_t n; + + /* If allocation failed then show the supported_cpus */ + if (!alloc_cpumask_var(&mask, GFP_KERNEL)) + return cpumap_print_to_pagebuf(true, buf, &armpmu->supported_cpus); + + cpumask_and(mask, &armpmu->supported_cpus, cpu_online_mask); + n = cpumap_print_to_pagebuf(true, buf, mask); + free_cpumask_var(mask); + + return n; } static DEVICE_ATTR_RO(cpus); +static ssize_t supported_cpus_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct arm_pmu *armpmu = to_arm_pmu(dev_get_drvdata(dev)); + + return cpumap_print_to_pagebuf(true, buf, &armpmu->supported_cpus); +} + +static DEVICE_ATTR_RO(supported_cpus); + static struct attribute *armpmu_common_attrs[] = { &dev_attr_cpus.attr, + &dev_attr_supported_cpus.attr, NULL, };