Re: [PATCH v2 1/2] drivers/perf: hisi: Add driver for HiSilicon PCIe PMU

quoted

Hi John,

On 2021/4/13 1:21, John Garry wrote:
On 12/04/2021 14:34, liuqi (BA) wrote:
Hi John,

Thanks for reviewing this.
On 2021/4/9 18:22, John Garry wrote:
On 09/04/2021 10:05, 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.
side note: it would be good to mention what baseline the series is based 
on in the cover letter
Got it, will add it, thanks.

Filtering options contains:
event        - select the event.
subevent     - select the subevent.
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.
trigger_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.

Signed-off-by: Qi Liu <redacted>
---
   MAINTAINERS                                |    6 +
   drivers/perf/Kconfig                       |    2 +
   drivers/perf/Makefile                      |    1 +
   drivers/perf/pci/Kconfig                   |   16 +
   drivers/perf/pci/Makefile                  |    2 +
   drivers/perf/pci/hisilicon/Makefile        |    5 +
   drivers/perf/pci/hisilicon/hisi_pcie_pmu.c | 1011
++++++++++++++++++++++++++++
   include/linux/cpuhotplug.h                 |    1 +
   8 files changed, 1044 insertions(+)
   create mode 100644 drivers/perf/pci/Kconfig
   create mode 100644 drivers/perf/pci/Makefile
   create mode 100644 drivers/perf/pci/hisilicon/Makefile
   create mode 100644 drivers/perf/pci/hisilicon/hisi_pcie_pmu.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 3353de0..46c7861 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8023,6 +8023,12 @@ W:    http://www.hisilicon.com
   F:    Documentation/admin-guide/perf/hisi-pmu.rst
   F:    drivers/perf/hisilicon
+HISILICON PCIE PMU DRIVER
+M:    Qi Liu [off-list ref]
+S:    Maintained
+F:    Documentation/admin-guide/perf/hisi-pcie-pmu.rst
nit: this does not exist yet...
thanks, I'll move this add-maintainer-part to the second patch.
that's why I advocate the documentation first :)
ok, I'll move document as the first patch.

+F:    drivers/perf/pci/hisilicon/hisi_pcie_pmu.c
+
   HISILICON QM AND ZIP Controller DRIVER
   M:    Zhou Wang [off-list ref]
   L:    linux-crypto@vger.kernel.org
diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
index 3075cf1..99d4760 100644
--- a/drivers/perf/Kconfig
+++ b/drivers/perf/Kconfig
@@ -139,4 +139,6 @@ config ARM_DMC620_PMU
   source "drivers/perf/hisilicon/Kconfig"
+source "drivers/perf/pci/Kconfig"
+
   endmenu
diff --git a/drivers/perf/Makefile b/drivers/perf/Makefile
index 5260b11..1208c08 100644
--- a/drivers/perf/Makefile
+++ b/drivers/perf/Makefile
@@ -14,3 +14,4 @@ obj-$(CONFIG_THUNDERX2_PMU) += thunderx2_pmu.o
   obj-$(CONFIG_XGENE_PMU) += xgene_pmu.o
   obj-$(CONFIG_ARM_SPE_PMU) += arm_spe_pmu.o
   obj-$(CONFIG_ARM_DMC620_PMU) += arm_dmc620_pmu.o
+obj-y += pci/
diff --git a/drivers/perf/pci/Kconfig b/drivers/perf/pci/Kconfig
new file mode 100644
index 0000000..a119486
--- /dev/null
+++ b/drivers/perf/pci/Kconfig
@@ -0,0 +1,16 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# PCIe Performance Monitor Drivers
+#
+menu "PCIe Performance Monitor"
+
+config HISI_PCIE_PMU
+    tristate "HiSilicon PCIE PERF PMU"
+    depends on ARM64 && PCI && HISI_PMU
What from HISI_PMU is needed? I couldn't find anything here
The event_sysfs_show() and format_sysfs_show() function of
hisi_uncore_pmu.h can be reused in hisi_pcie_pmu.c, So I add path in
Makefile and include "hisi_uncore_pmu.h".
Right, but it would be nice to be able to build this under COMPILE_TEST. 
CONFIG_HISI_PMU cannot be built under COMPILE_TEST, so nice to not 
depend on it.

So you could put hisi_event_sysfs_show() as a static inline in 
hisi_uncore_pmu.h, so the dependency can be removed

Having said that, there is nothing really hisi specific in those 
functions like hisi_event_sysfs_show().

Can't we just create generic functions here?

hisi_event_sysfs_show() == cci400_pmu_cycle_event_show() == 
xgene_pmu_event_show()
Got it, will address this.


+    help
+      Provide support for HiSilicon PCIe performance monitoring unit
(PMU)
+      RCiEP devices.
+      Adds the PCIe PMU into perf events system for monitoring 
latency,
+      bandwidth etc.
+



[...]
+static bool hisi_pcie_pmu_valid_filter(struct perf_event *event,
+                       struct hisi_pcie_pmu *pcie_pmu)
+{
+    u32 subev_idx = hisi_pcie_get_subevent(event);
+    u32 event_idx = hisi_pcie_get_event(event);
+    u32 requester_id = hisi_pcie_get_bdf(event);
+
+    if (subev_idx > HISI_PCIE_SUBEVENT_MAX ||
+        event_idx > HISI_PCIE_EVENT_MAX) {
+        pci_err(pcie_pmu->pdev,
+            "Max event index and max subevent index is: %d, %d.\n",
+            HISI_PCIE_EVENT_MAX, HISI_PCIE_SUBEVENT_MAX);
if this is just going to be fed back to userspace, I don't see why we
need a kernel log

and the only caller also triggers an error message, which I doubt is 
needed
Print out the HISI_PCIE_EVENT_MAX and HISI_PCIE_SUBEVENT_MAX here may be
more convenient for users to get the right value.
If you this is redundant I'll remove it. :)
Don't we already tell this to userspace?
"event" is a 8-bit filter, its max value is 0xff, but PCIE PMU only have 
0xa2 events， So if users input "event=0xa3", userspace only printout 
"<not supported>".
Perhaps driver could tell users the max value of event index here.
If you think this is redundant I'll remove it. :)


+        return false;
+    }
+
+    if (hisi_pcie_get_thr_len(event) > HISI_PCIE_THR_MAX_VAL)
+        return false;
+
+    if (hisi_pcie_get_trig_len(event) > HISI_PCIE_TRIG_MAX_VAL)
+        return false;
+
+    if (requester_id) {
+        if (!hisi_pcie_pmu_valid_requester_id(pcie_pmu, 
requester_id)) {
+            pci_err(pcie_pmu->pdev, "Invalid requester id.\n");
see previous comments

+            return false;
+        }
+    }
+
+    return true;
+}
+
+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;
+}
+
+static int hisi_pcie_pmu_event_init(struct perf_event *event)
+{
+    struct hisi_pcie_pmu *pcie_pmu = to_pcie_pmu(event->pmu);
+
+    event->cpu = cpumask_first(&pcie_pmu->cpumask);
+
+    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;
+
+    /* Per-task mode is not supported. */
+    if (event->cpu < 0)
cpumask_first() gives an unsigned int - this can never happen
please fix this!
Sorry, missed it yesterday. I'll fix this next version.

+        return -EINVAL;
+
+    if (!hisi_pcie_pmu_valid_filter(event, pcie_pmu)) {
+        pci_err(pcie_pmu->pdev, "Invalid filter!\n");
+        return -EINVAL;
+    }
+
+    if (!hisi_pcie_pmu_validate_event_group(event))
+        return -EINVAL;
+
+    return 0;
+}
+
+static u64 hisi_pcie_pmu_process_data(struct perf_event *event, u64 
val,

...

+
+static void hisi_pcie_pmu_irq_unregister(struct pci_dev *pdev,
+                     struct hisi_pcie_pmu *pcie_pmu)
+{
+    free_irq(pcie_pmu->irq, pcie_pmu);
+    pci_free_irq_vectors(pdev);
+}
+
+static int hisi_pcie_pmu_online_cpu(unsigned int cpu, struct
hlist_node *node)
+{
+    struct hisi_pcie_pmu *pcie_pmu = hlist_entry_safe(node,
+                     struct hisi_pcie_pmu, node);
+
+    if (cpumask_empty(&pcie_pmu->cpumask)) {
+        cpumask_set_cpu(cpu, &pcie_pmu->cpumask);
+        WARN_ON(irq_set_affinity_hint(pcie_pmu->irq, 
cpumask_of(cpu)));
+    }
+
+    return 0;
+}
+
+static int hisi_pcie_pmu_offline_cpu(unsigned int cpu, struct
hlist_node *node)
+{
+    struct hisi_pcie_pmu *pcie_pmu = hlist_entry_safe(node,
+                     struct hisi_pcie_pmu, node);
+    unsigned int target;
+
+    if (!cpumask_test_and_clear_cpu(cpu, &pcie_pmu->cpumask))
I do wonder why we even need maintain pcie_pmu->cpumask

Can't we just use cpu_online_mask as appropiate instead?
?
Sorry, missed it yesterday.
It seems that cpumask is always same as cpu_online_mask, So do we need 
to reserve the cpumask sysfs interface?

Thanks,
Qi


+        return 0;
+
+    /* Choose a new CPU from all online cpus. */
+    target = cpumask_any_but(cpu_online_mask, cpu);
+    if (target >= nr_cpu_ids)
+        return 0;
+
+    perf_pmu_migrate_context(&pcie_pmu->pmu, cpu, target);
+    WARN_ON(irq_set_affinity_hint(pcie_pmu->irq, cpumask_of(target)));
+
+    return 0;
+}
+
.

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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help