Hi, Will On 2021/8/24 22:31, Will Deacon wrote:
Hi, On Wed, Aug 18, 2021 at 01:12:46PM +0800, Qi Liu wrote:PCIe PMU Root Complex Integrated End Point(RCiEP) device is supported to sample bandwidth, latency, buffer occupation etc. Each PMU RCiEP device monitors multiple Root Ports, and each RCiEP is registered as a PMU in /sys/bus/event_source/devices, so users can select target PMU, and use filter to do further sets. Filtering options contains: event - select the event. port - select target Root Ports. Information of Root Ports are shown under sysfs. bdf - select requester_id of target EP device. trig_len - set trigger condition for starting event statistics. trig_mode - set trigger mode. 0 means starting to statistic when bigger than trigger condition, and 1 means smaller. thr_len - set threshold for statistics. thr_mode - set threshold mode. 0 means count when bigger than threshold, and 1 means smaller.I think this is getting there now, thanks for sticking with it. Just a couple of comments below..+static bool hisi_pcie_pmu_validate_event_group(struct perf_event *event) +{ + struct perf_event *sibling, *leader = event->group_leader; + int counters = 1; + + if (!is_software_event(leader)) { + if (leader->pmu != event->pmu) + return false; + + if (leader != event) + counters++; + } + + for_each_sibling_event(sibling, event->group_leader) { + if (is_software_event(sibling)) + continue; + + if (sibling->pmu != event->pmu) + return false; + + counters++; + } + + return counters <= HISI_PCIE_MAX_COUNTERS; +}Given that this function doesn't look at the event numbers, doesn't this over-provision the counter registers? For example, if I create a group containing 4 of the same event, then we'll allocate four counters but only use one. Similarly, if I create a group containing two events, one for the normal counter and one for the extended counter, then we'll again allocate two counters instead of one.
Yes, we should add some check in hisi_pcie_pmu_validate_event_group() function to avoid over-provision. I'll use a array to record events and do this check.
Thanks for your review, I'll fix this.
Have I misunderstood?+static int hisi_pcie_pmu_event_init(struct perf_event *event) +{ + struct hisi_pcie_pmu *pcie_pmu = to_pcie_pmu(event->pmu); + struct hw_perf_event *hwc = &event->hw; + + event->cpu = pcie_pmu->on_cpu; + + if (EXT_COUNTER_IS_USED(hisi_pcie_get_event(event))) + hwc->event_base = HISI_PCIE_EXT_CNT; + else + hwc->event_base = HISI_PCIE_CNT; + + if (event->attr.type != event->pmu->type) + return -ENOENT; + + /* Sampling is not supported. */ + if (is_sampling_event(event) || event->attach_state & PERF_ATTACH_TASK) + return -EOPNOTSUPP; + + if (!hisi_pcie_pmu_valid_filter(event, pcie_pmu)) { + pci_err(pcie_pmu->pdev, "Invalid filter!\n");Please remove this message, as it's triggerable from userspace.
got it, will remove this. Thanks, Qi
Will .
