Messages in this thread | | | Subject | Re: [PATCH 1/2] perf: arm_pmu: Only show online CPUs in device's "cpus" attribute | From | Yicong Yang <> | Date | Fri, 17 May 2024 17:43:56 +0800 |
| |
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 > . >
| |