Re: [PATCH 1/2] perf: arm_pmu: Only show online CPUs in device's "cpus" attribute

[Yicong Yang <yangyicong@huawei.com>]

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
> .
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel