perf: Add Arm Network-on-Chip PMU driver
Subsystems (4)
Robin Murphy
maintainerv2 cover perf: Add Arm Network-on-Chip PMU driver
v1: https://lore.kernel.org/linux-arm-kernel/[email protected]/
Hi all,
Probably a bit late now, but here's a v2 addressing Will's review comments, and hoping to attract the DT maintainers' attention properly this time...
Thanks,
Robin.
Robin Murphy (3):
dt-bindings/perf: Add Arm NI-700 PMU
perf: Add driver for Arm NI-700 interconnect PMU
MAINTAINERS: List Arm interconnect PMUs as supported
Documentation/admin-guide/perf/arm-ni.rst | 17 + Documentation/admin-guide/perf/index.rst | 1 + .../devicetree/bindings/perf/arm,ni.yaml | 30 + MAINTAINERS | 11 + drivers/perf/Kconfig | 7 + drivers/perf/Makefile | 1 + drivers/perf/arm-ni.c | 778 ++++++++++++++++++ 7 files changed, 845 insertions(+) create mode 100644 Documentation/admin-guide/perf/arm-ni.rst create mode 100644 Documentation/devicetree/bindings/perf/arm,ni.yaml create mode 100644 drivers/perf/arm-ni.c
Signature · 1 line
2.39.2.101.g768bb238c484.dirty
Robin Murphy
maintainerPATCH v2 1/3 dt-bindings/perf: Add Arm NI-700 PMU
Add an initial binding for the Arm NI-700 interconnect PMU. As with the Arm CMN family, there are already future NI products on the roadmap, so the overall binding is named generically just in case any non-discoverable incompatibility between generations crops up.
---
v2: No change
.../devicetree/bindings/perf/arm,ni.yaml | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 Documentation/devicetree/bindings/perf/arm,ni.yaml
Documentation/devicetree/bindings/perf/arm,ni.yaml new file +30 -0
@@ -0,0 +1,30 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/perf/arm,ni.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Arm NI (Network-on-Chip Interconnect) Performance Monitors + +maintainers: + - Robin Murphy <[email protected]> + +properties: + compatible: + const: arm,ni-700 + + reg: + items: + - description: Complete configuration register space + + interrupts: + minItems: 1 + maxItems: 32 + description: Overflow interrupts, one per clock domain, in order of domain ID + +required: + - compatible + - reg + - interrupts + +additionalProperties: false
Signature · 1 line
2.39.2.101.g768bb238c484.dirty
Re: [PATCH v2 1/3] dt-bindings/perf: Add Arm NI-700 PMU
Quoted 7 lines · Robin Murphy
Add an initial binding for the Arm NI-700 interconnect PMU. As with the Arm CMN family, there are already future NI products on the roadmap, so the overall binding is named generically just in case any non-discoverable incompatibility between generations crops up.
Cc <[email protected]>Signed-off-by Robin Murphy <[email protected]>
Quoted 8 lines
---
v2: No change
.../devicetree/bindings/perf/arm,ni.yaml | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 Documentation/devicetree/bindings/perf/arm,ni.yaml
Robin Murphy
maintainerPATCH v2 2/3 perf: Add driver for Arm NI-700 interconnect PMU
In reply to Robin MurphyThe Arm NI-700 Network-on-Chip Interconnect has a relatively straightforward design with a hierarchy of voltage, power, and clock domains, where each clock domain then contains a number of interface units and a PMU which can monitor events thereon. As such, it begets a relatively straightforward driver to interface those PMUs with perf.
Even more so than with arm-cmn, users will require detailed knowledge of the wider system topology in order to meaningfully analyse anything, since the interconnect itself cannot know what lies beyond the boundary of each inscrutably-numbered interface. Given that, for now they are also expected to refer to the NI-700 documentation for the relevant event IDs to provide as well. An identifier is implemented so we can come back and add jevents if anyone really wants to.
---
v2:
- Add basic usage documentation
- Use __counted_by attribute
- Make group validation logic clearer (and drop PMU type check which perf_event_open() already takes care of)
- Add retry limit to arm_ni_read_ccnt()
--- Documentation/admin-guide/perf/arm-ni.rst | 17 + Documentation/admin-guide/perf/index.rst | 1 + drivers/perf/Kconfig | 7 + drivers/perf/Makefile | 1 + drivers/perf/arm-ni.c | 778 ++++++++++++++++++++++ 5 files changed, 804 insertions(+) create mode 100644 Documentation/admin-guide/perf/arm-ni.rst create mode 100644 drivers/perf/arm-ni.c
Documentation/admin-guide/perf/arm-ni.rst new file +17 -0
@@ -0,0 +1,17 @@ +==================================== +Arm Network-on Chip Interconnect PMU +==================================== + +NI-700 and friends implement a distinct PMU for each clock domain within the +interconnect. Correspondingly, the driver exposes multiple PMU devices named +arm_ni_<x>_cd_<y>, where <x> is an (abritrary) instance identifier and <y> is +the clock domain ID within that particular instance. If multiple NI instances +exist within a system, the PMU devices can be correlated with the underlying +hardware instance via sysfs parentage. + +Each PMU exposes base event aliases for the interface types present in its clock +domain. These require qualifying with the "eventid" and "nodeid" parameters +to specify the event code to count and the interface at which to count it +(per the configured hardware ID as reflected in the xxNI_NODE_INFO register). +The exception is the "cycles" alias for the PMU cycle counter, which is encoded +with the PMU node type and needs no further qualification.
Documentation/admin-guide/perf/index.rst +1 -0
@@ -16,6 +16,7 @@ Performance monitor support starfive_starlink_pmu arm-ccn arm-cmn + arm-ni xgene-pmu arm_dsu_pmu thunderx2-pmu
drivers/perf/Kconfig +7 -0
@@ -48,6 +48,13 @@ config ARM_CMN Support for PMU events monitoring on the Arm CMN-600 Coherent Mesh Network interconnect. +config ARM_NI + tristate "Arm NI-700 PMU support" + depends on ARM64 || COMPILE_TEST + help + Support for PMU events monitoring on the Arm NI-700 Network-on-Chip + interconnect and family. + config ARM_PMU depends on ARM || ARM64 bool "ARM PMU framework"
drivers/perf/Makefile +1 -0
@@ -3,6 +3,7 @@ obj-$(CONFIG_ARM_CCI_PMU) += arm-cci.o obj-$(CONFIG_ARM_CCN) += arm-ccn.o obj-$(CONFIG_ARM_CMN) += arm-cmn.o obj-$(CONFIG_ARM_DSU_PMU) += arm_dsu_pmu.o +obj-$(CONFIG_ARM_NI) += arm-ni.o obj-$(CONFIG_ARM_PMU) += arm_pmu.o arm_pmu_platform.o obj-$(CONFIG_ARM_PMU_ACPI) += arm_pmu_acpi.o obj-$(CONFIG_ARM_PMUV3) += arm_pmuv3.o
drivers/perf/arm-ni.c new file +778 -0
778 lines not on this page · raw message
Signature · 1 line
2.39.2.101.g768bb238c484.dirty
Mark Rutland
maintainerRe: [PATCH v2 2/3] perf: Add driver for Arm NI-700 interconnect PMU
Quoted 6 lines · Robin Murphy
The Arm NI-700 Network-on-Chip Interconnect has a relatively straightforward design with a hierarchy of voltage, power, and clock domains, where each clock domain then contains a number of interface units and a PMU which can monitor events thereon. As such, it begets a relatively straightforward driver to interface those PMUs with perf.
Quoted 7 lines
Even more so than with arm-cmn, users will require detailed knowledge of the wider system topology in order to meaningfully analyse anything, since the interconnect itself cannot know what lies beyond the boundary of each inscrutably-numbered interface. Given that, for now they are also expected to refer to the NI-700 documentation for the relevant event IDs to provide as well. An identifier is implemented so we can come back and add jevents if anyone really wants to.
IIUC the relevant documentation would be the NI-700 TRM:
https://developer.arm.com/documentation/101566/0203/?lang=en
... right?
[...]
Documentation/admin-guide/perf/arm-ni.rst line 10
Quoted 10 lines
+====================================
Documentation/admin-guide/perf/arm-ni.rst new file +9
-0+Arm Network-on Chip Interconnect PMU +==================================== + +NI-700 and friends implement a distinct PMU for each clock domain within the +interconnect. Correspondingly, the driver exposes multiple PMU devices named +arm_ni_<x>_cd_<y>, where <x> is an (abritrary) instance identifier and <y> is +the clock domain ID within that particular instance. If multiple NI instances +exist within a system, the PMU devices can be correlated with the underlying +hardware instance via sysfs parentage.
I suspect that name suffixing *might* confuse userspace in some cases, since IIUC the perf tool tries to aggregate some_pmu_<number> instances, and here it would presumably aggregate all the clock domains as a arm_ni_<x>_cd PMU.
We should check how that behaves and speak to the userspace folk about this; taking a step back we probably need a better way for determining when things can or should be aggregated.
I assume we don't have HW to test on, but we should be able to fake up the sysfs hierarchy and see how "perf list" treats that.
[...]
Quoted 7 lines
drivers/perf/arm-ni.c new file +7
-0+static int arm_ni_validate_group (struct perf_event *event) +{ +struct perf_event *sibling, *leader; +struct arm_ni_val val = {0 }; + +arm_ni_validate_event (event, &val); +
drivers/perf/arm-ni.c line 306
Quoted 10 lines
drivers/perf/arm-ni.c new file +10
-0+ leader = event->group_leader; +if (leader != event && !arm_ni_validate_event (leader, &val)) +return -EINVAL ; + +for_each_sibling_event (sibling, leader) { +if (!arm_ni_validate_event (sibling, &val)) +return -EINVAL ; + } +return 0 ; +}
As a trivial nit, something has gone wrong with spacing and you have:
return - EINVAL;
... rather than:
return -EINVAL;
As a more substantial thing, this will trigger splats when lockdep is enabled and an event is opened where event == event->group_leader, because for_each_sibling_event(..., event) checks event->ctx->mutex is held, and event->ctx isn't initialised until pmu::event_init() returns.
That's a latent bug in many PMU drivers at the moment, and is on my TODO list to fix elsewhere. For now, can you make the above:
| static int arm_ni_validate_group(struct perf_event *event)
| {
| struct perf_event *sibling, *leader = event->group_leader;
| struct arm_ni_val val = { 0 };
|
| if (event == leader)
| return 0;
|
| arm_ni_validate_event(event, &val);
|
| if (!arm_ni_validate_event(leader, &val))
| return -EINVAL;
|
| for_each_sibling_event(sibling, leader) {
| if (!arm_ni_validate_event(sibling, &val))
| return -EINVAL;
| }
|
| return 0;
| }... where the early exit for the (event == leader) case will avoid the bad call.
[...]
drivers/perf/arm-ni.c
Quoted 7 lines
drivers/perf/arm-ni.c new file +6
-0+static int arm_ni_init_cd (struct arm_ni *ni,struct arm_ni_node *node,u64 res_start) +{ +struct arm_ni_cd *cd = ni->cds + node->id; +const char *name; +int err; + cd->cpu =cpumask_local_spread (0 ,dev_to_node (ni->dev));
Can dev_to_node(ni->dev) return NUMA_NO_NODE, and if so, do we need to error out here? ...
drivers/perf/arm-ni.c line 726
Quoted 11 lines
drivers/perf/arm-ni.c new file +11
-0+static int arm_ni_pmu_online_cpu (unsigned int cpu,struct hlist_node *cpuhp_node) +{ +struct arm_ni_cd *cd; +int node; + + cd = hlist_entry_safe(cpuhp_node, struct arm_ni_cd, cpuhp_node); + node =dev_to_node (cd_to_ni (cd)->dev); +if (node !=NUMA_NO_NODE &&cpu_to_node (cd->cpu) != node &&cpu_to_node (cpu) == node) +arm_ni_pmu_migrate (cd, cpu); +return 0 ; +}
... since we expect to handle similar here ...
Quoted 10 lines
drivers/perf/arm-ni.c new file +10
-0+ +static int arm_ni_pmu_offline_cpu (unsigned int cpu,struct hlist_node *cpuhp_node) +{ +struct arm_ni_cd *cd; +unsigned int target; +int node; + + cd = hlist_entry_safe(cpuhp_node, struct arm_ni_cd, cpuhp_node); +if (cpu != cd->cpu) +return 0 ;
drivers/perf/arm-ni.c line 746
Quoted 10 lines
drivers/perf/arm-ni.c new file +10
-0+ + node =dev_to_node (cd_to_ni (cd)->dev); + target =cpumask_any_and_but (cpumask_of_node (node), cpu_online_mask, cpu); +if (target >= nr_cpu_ids) + target =cpumask_any_but (cpu_online_mask, cpu); + +if (target < nr_cpu_ids) +arm_ni_pmu_migrate (cd, target); +return 0 ; +}
... though not here, and overall that seems inconsistent.
Mark.
Robin Murphy
maintainerRe: [PATCH v2 2/3] perf: Add driver for Arm NI-700 interconnect PMU
Quoted 15 lines · Mark Rutland
Quoted 6 lines · Robin Murphy
The Arm NI-700 Network-on-Chip Interconnect has a relatively straightforward design with a hierarchy of voltage, power, and clock domains, where each clock domain then contains a number of interface units and a PMU which can monitor events thereon. As such, it begets a relatively straightforward driver to interface those PMUs with perf.
Quoted 7 lines
Even more so than with arm-cmn, users will require detailed knowledge of the wider system topology in order to meaningfully analyse anything, since the interconnect itself cannot know what lies beyond the boundary of each inscrutably-numbered interface. Given that, for now they are also expected to refer to the NI-700 documentation for the relevant event IDs to provide as well. An identifier is implemented so we can come back and add jevents if anyone really wants to.
Quoted 5 lines
IIUC the relevant documentation would be the NI-700 TRM:
https://developer.arm.com/documentation/101566/0203/?lang=en... right?
That's the one.
Quoted 17 lines
[...]
Documentation/admin-guide/perf/arm-ni.rst line 10
Quoted 10 lines
+====================================
Documentation/admin-guide/perf/arm-ni.rst new file +9
-0+Arm Network-on Chip Interconnect PMU +==================================== + +NI-700 and friends implement a distinct PMU for each clock domain within the +interconnect. Correspondingly, the driver exposes multiple PMU devices named +arm_ni_<x>_cd_<y>, where <x> is an (abritrary) instance identifier and <y> is +the clock domain ID within that particular instance. If multiple NI instances +exist within a system, the PMU devices can be correlated with the underlying +hardware instance via sysfs parentage.I suspect that name suffixing *might* confuse userspace in some cases, since IIUC the perf tool tries to aggregate some_pmu_<number> instances, and here it would presumably aggregate all the clock domains as a arm_ni_<x>_cd PMU.
Indeed that is liable to be particularly funky since different domains may well contain different sets of interface types and thus not even necessarily share the same set of base events. Either way, events are targeted at specific interfaces so aggregation at any level would generally be meaningless.
Quoted 5 lines
We should check how that behaves and speak to the userspace folk about this; taking a step back we probably need a better way for determining when things can or should be aggregated. >
I assume we don't have HW to test on, but we should be able to fake up the sysfs hierarchy and see how "perf list" treats that.
I see there is an available FVP for the Total Compute TC2 system[1] which includes an NI-700 instance - it doesn't model the PMU counters, but the rest should work enough for discovery. I might have a go with that next week...
Quoted 24 lines
[...]
Quoted 7 lines
drivers/perf/arm-ni.c new file +7
-0+static int arm_ni_validate_group (struct perf_event *event) +{ +struct perf_event *sibling, *leader; +struct arm_ni_val val = {0 }; + +arm_ni_validate_event (event, &val); +drivers/perf/arm-ni.c line 306
Quoted 10 lines
drivers/perf/arm-ni.c new file +10
-0+ leader = event->group_leader; +if (leader != event && !arm_ni_validate_event (leader, &val)) +return -EINVAL ; + +for_each_sibling_event (sibling, leader) { +if (!arm_ni_validate_event (sibling, &val)) +return -EINVAL ; + } +return 0 ; +}As a trivial nit, something has gone wrong with spacing and you have:
return - EINVAL;
Quoted 8 lines
... rather than:
return -EINVAL;As a more substantial thing, this will trigger splats when lockdep is enabled and an event is opened where event ==
event->group_leader, becausefor_each_sibling_event(..., event)checksevent->ctx->mutexis held, andevent->ctxisn't initialised untilpmu::event_init()returns.
Oof, I even remember looking at that report, so I don't really have any excuse!
Quoted 24 lines
That's a latent bug in many PMU drivers at the moment, and is on my TODO list to fix elsewhere. For now, can you make the above:
| static int arm_ni_validate_group(struct perf_event *event) | { | struct perf_event *sibling, *leader = event->group_leader; | struct arm_ni_val val = { 0 }; | | if (event == leader) | return 0; | | arm_ni_validate_event(event, &val); | | if (!arm_ni_validate_event(leader, &val)) | return -EINVAL; | | for_each_sibling_event(sibling, leader) { | if (!arm_ni_validate_event(sibling, &val)) | return -EINVAL; | } | | return 0; | }
Quoted 2 lines
... where the early exit for the (event == leader) case will avoid the bad call.
Took me a moment to see why it's OK to skip "validating" the event as leader itself, but that's arguably just as much down to my naming. I'll polish that up a bit further.
Quoted 8 lines
[...]
Quoted 5 lines
drivers/perf/arm-ni.c new file +5
-0+static int arm_ni_init_cd (struct arm_ni *ni,struct arm_ni_node *node,u64 res_start) +{ +struct arm_ni_cd *cd = ni->cds + node->id; +const char *name; +int err;
Quoted 4 lines
drivers/perf/arm-ni.c line 547
Quoted 1 line
drivers/perf/arm-ni.c new file +1
-0+ cd->cpu =cpumask_local_spread (0 ,dev_to_node (ni->dev));Can
dev_to_node(ni->dev)returnNUMA_NO_NODE, and if so, do we need to error out here? ...
It can, and cpumask_local_spread() just picks from cpu_online_mask in that case. It's not an error either way, the intent is just that if we do have NUMA, then it makes sense to try to associate with a physically-close CPU.
Quoted 5 lines
Quoted 5 lines
drivers/perf/arm-ni.c new file +5
-0+static int arm_ni_pmu_online_cpu (unsigned int cpu,struct hlist_node *cpuhp_node) +{ +struct arm_ni_cd *cd; +int node; +
Quoted 8 lines
drivers/perf/arm-ni.c line 726
Quoted 6 lines
drivers/perf/arm-ni.c new file +6
-0+ cd = hlist_entry_safe(cpuhp_node, struct arm_ni_cd, cpuhp_node); + node =dev_to_node (cd_to_ni (cd)->dev); +if (node !=NUMA_NO_NODE &&cpu_to_node (cd->cpu) != node &&cpu_to_node (cpu) == node) +arm_ni_pmu_migrate (cd, cpu); +return 0 ; +}... since we expect to handle similar here ...
...and conversely, without NUMA there's no point even asking the question of whether a new CPU might be more local than our current one.
Quoted 14 lines
Quoted 14 lines
drivers/perf/arm-ni.c new file +14
-0+ +static int arm_ni_pmu_offline_cpu (unsigned int cpu,struct hlist_node *cpuhp_node) +{ +struct arm_ni_cd *cd; +unsigned int target; +int node; + + cd = hlist_entry_safe(cpuhp_node, struct arm_ni_cd, cpuhp_node); +if (cpu != cd->cpu) +return 0 ; + + node =dev_to_node (cd_to_ni (cd)->dev); + target =cpumask_any_and_but (cpumask_of_node (node), cpu_online_mask, cpu); +if (target >= nr_cpu_ids)
Quoted 8 lines
drivers/perf/arm-ni.c line 746
Quoted 6 lines
drivers/perf/arm-ni.c new file +6
-0+ target =cpumask_any_but (cpu_online_mask, cpu); + +if (target < nr_cpu_ids) +arm_ni_pmu_migrate (cd, target); +return 0 ; +}... though not here, and overall that seems inconsistent.
I did start down that rabbit hole, but gave up in despair around the point of the "#ifdef CONFIG_NUMA" inside the "#ifndef CONFIG_NUMA". My conclusion was that the majority of cpumask_of_node() implementations do handle NUMA_NO_NODE, making it appear expected, and there is already other code assuming so, so the ones which don't were not my problem.
Thanks,
Robin.
Robin Murphy
maintainerPATCH v2 3/3 MAINTAINERS: List Arm interconnect PMUs as supported
In reply to Robin MurphyWhatever I may or may not have hoped for, looking after these drivers seems to have firmly stuck as one of the responsibilities of the job Arm pays me for, and I would still like to be aware of any other patches, so make it official.
---
v2: Add new arm-ni.rst path
MAINTAINERS | 11 +++++++++++ 1 file changed, 11 insertions(+)
MAINTAINERS +11 -0
@@ -1724,6 +1724,17 @@ F: drivers/mtd/maps/physmap-versatile.* F: drivers/power/reset/arm-versatile-reboot.c F: drivers/soc/versatile/ +ARM INTERCONNECT PMU DRIVERS +M: Robin Murphy <[email protected]> +S: Supported +F: Documentation/admin-guide/perf/arm-cmn.rst +F: Documentation/admin-guide/perf/arm-ni.rst +F: Documentation/devicetree/bindings/perf/arm,cmn.yaml +F: Documentation/devicetree/bindings/perf/arm,ni.yaml +F: drivers/perf/arm-cmn.c +F: drivers/perf/arm-ni.c +F: tools/perf/pmu-events/arch/arm64/arm/cmn/ + ARM KOMEDA DRM-KMS DRIVER M: Liviu Dudau <[email protected]> S: Supported
Signature · 1 line
2.39.2.101.g768bb238c484.dirty