Messages in this thread | | | Date | Sat, 25 Mar 2023 18:21:59 +0800 | Subject | Re: [RFC PATCH v1 2/4] drivers/perf: hisi: Add driver support for HiSilicon PMCU | From | Jie Zhan <> |
| |
On 17/03/2023 22:52, Jonathan Cameron wrote: > On Mon, 6 Feb 2023 14:51:44 +0800 > Jie Zhan <zhanjie9@hisilicon.com> wrote: > >> HiSilicon Performance Monitor Control Unit (PMCU) is a device that offloads >> PMU accesses from CPUs, handling the configuration, event switching, and >> counter reading of core PMUs on Kunpeng SoC. It facilitates fine-grained >> and multi-PMU-event CPU profiling, in which scenario the current 'perf' >> scheme may lose events or drop sampling frequency. With PMCU, users can >> reliably obtain the data of up to 240 PMU events with the sample interval >> of events down to 1ms, while the software overhead of accessing PMUs, as >> well as its impact on target workloads, is reduced. >> >> This driver enables the usage of PMCU through the perf_event framework. >> PMCU is registered as a PMU device and utilises the AUX buffer to dump data >> directly. Users can start PMCU sampling through 'perf-record'. Event >> numbers are passed by a sysfs interface. >> >> Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com> > Hi Jie, > > A few minor comments inline. > Whilst I looked at this internally, that was a while back so I've > found a few new things to point out in what I think is a pretty good/clean driver. > The main thing here is the RFC questions you've raised in the cover letter > of course - particularly the one around mediating who has the counters between > this and the normal PMU driver. > > Thanks, > > Jonathan Hi Jonathan,
Many thanks for the review again.
Happy to accept all the comments. I have updated the driver based on them.
One reply below.
Jie
... >> +static const struct attribute_group hisi_pmcu_format_attr_group = { >> + .name = "format", >> + .attrs = hisi_pmcu_format_attrs, >> +}; >> + >> +static ssize_t monitored_cpus_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct hisi_pmcu *hisi_pmcu = to_hisi_pmcu(dev_get_drvdata(dev)); >> + >> + return sysfs_emit(buf, "%d-%d\n", >> + cpumask_first(&hisi_pmcu->cpus), >> + cpumask_last(&hisi_pmcu->cpus)); > What does this do about offline CPUs? > Should it include them or not? PMCU takes care of offline CPUs as well, and the event counts from offline CPUs should show as zeroes in the output.
hisi_pmcu->cpus contains only the online CPUs monitored by the PMCU, so something should be improved with the "monitored_cpus" interface here.
"monitored_cpus" should actually show alll the online/offline CPUs monitored, or, if it is meant to show only online CPUs, it show be a comma separated list representing the hisi_pmcu->cpus mask rather than a range that may ignore some offline CPUs in the middle.
Will fix this in V2.
| |