perf: qcom: Add L3 cache PMU driver
Subsystems (3)
PATCH 1/1 perf: qcom: Add L3 cache PMU driver
This adds a new dynamic PMU to the Perf Events framework to program and control the L3 cache PMUs in some Qualcomm Technologies SOCs.
The driver supports a distributed cache architecture where the overall cache is comprised of multiple slices each with its own PMU. The driver aggregates counts across the whole system to provide a global picture of the metrics selected by the user.
The driver exports formatting and event information to sysfs so it can be used by the perf user space tools with the syntaxes:
perf stat -a -e l3cache/read-miss/
perf stat -a -e l3cache/event=0x21/
--- arch/arm64/kernel/Makefile | 4 + arch/arm64/kernel/perf_event_qcom_l3_cache.c | 816 +++++++++++++++++++++++++++ 2 files changed, 820 insertions(+) create mode 100644 arch/arm64/kernel/perf_event_qcom_l3_cache.c
arch/arm64/kernel/Makefile +4 -0
@@ -43,6 +43,10 @@ arm64-obj-$(CONFIG_ARMV8_DEPRECATED) += armv8_deprecated.o arm64-obj-$(CONFIG_ACPI) += acpi.o arm64-obj-$(CONFIG_PARAVIRT) += paravirt.o +ifeq ($(CONFIG_ARCH_QCOM), y) +arm64-obj-$(CONFIG_HW_PERF_EVENTS) += perf_event_qcom_l3_cache.o +endif + obj-y += $(arm64-obj-y) vdso/ obj-m += $(arm64-obj-m) head-y := head.o
arch/arm64/kernel/perf_event_qcom_l3_cache.c new file +816 -0
816 lines not on this page · raw message
Signature · 3 lines
Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Peter Zijlstra
maintainerPATCH 1/1 perf: qcom: Add L3 cache PMU driver
Quoted 7 lines · Agustin Vega-Frias
This adds a new dynamic PMU to the Perf Events framework to program and control the L3 cache PMUs in some Qualcomm Technologies SOCs.
The driver supports a distributed cache architecture where the overall cache is comprised of multiple slices each with its own PMU. The driver aggregates counts across the whole system to provide a global picture of the metrics selected by the user.
So is there never a situation where you want to profile just a single slice?
It userspace at all aware of these slices through other means?
That is; typically we do not aggregate in-kernel like this but simply expose each slice as a separate PMU and let userspace sort things.
PATCH 1/1 perf: qcom: Add L3 cache PMU driver
Quoted 4 lines · Peter Zijlstra
Quoted 3 lines · Agustin Vega-Frias
This adds a new dynamic PMU to the Perf Events framework to program and control the L3 cache PMUs in some Qualcomm Technologies SOCs.
Quoted 8 lines
Quoted 5 lines
The driver supports a distributed cache architecture where the overall cache is comprised of multiple slices each with its own PMU. The driver
aggregates counts across the whole system to provide a global picture of the metrics selected by the user.So is there never a situation where you want to profile just a single slice?
No, access to each individual slice is determined by hashing based on the target address.
Quoted 2 lines
It userspace at all aware of these slices through other means?
Userspace is not aware of the actual topology.
Quoted 3 lines
That is; typically we do not aggregate in-kernel like this but simply expose each slice as a separate PMU and let userspace sort things.
My decision of single vs. multiple PMUs was based on reducing the overhead required of retrieving the system-wide counts, which would require multiple system calls in the multiple-PMU case.
Peter Zijlstra
maintainerPATCH 1/1 perf: qcom: Add L3 cache PMU driver
Quoted 22 lines · agustinv at codeaurora.org
Quoted 11 lines · Peter Zijlstra
Quoted 7 lines · Agustin Vega-Frias
This adds a new dynamic PMU to the Perf Events framework to program and control the L3 cache PMUs in some Qualcomm Technologies SOCs.
The driver supports a distributed cache architecture where the overall cache is comprised of multiple slices each with its own PMU. The driver aggregates counts across the whole system to provide a global picture of the metrics selected by the user.
So is there never a situation where you want to profile just a single slice?
No, access to each individual slice is determined by hashing based on the target address.
Quoted 2 lines
It userspace at all aware of these slices through other means?
Userspace is not aware of the actual topology.
Quoted 1 line
Quoted 6 lines
Quoted 2 lines
That is; typically we do not aggregate in-kernel like this but simply expose each slice as a separate PMU and let userspace sort things.
My decision of single vs. multiple PMUs was based on reducing the overhead required of retrieving the system-wide counts, which would require multiple system calls in the multiple-PMU case.
OK. A bit weird your hardware has a PMU per slice if its otherwise completely hidden. In any case, put a comment somewhere describing how access to a single slice never makes sense.
Mark Rutland
maintainerPATCH 1/1 perf: qcom: Add L3 cache PMU driver
In reply to Agustin Vega-FriasQuoted 13 lines · Agustin Vega-Frias
This adds a new dynamic PMU to the Perf Events framework to program and control the L3 cache PMUs in some Qualcomm Technologies SOCs.
The driver supports a distributed cache architecture where the overall cache is comprised of multiple slices each with its own PMU. The driver aggregates counts across the whole system to provide a global picture of the metrics selected by the user.
The driver exports formatting and event information to sysfs so it can be used by the perf user space tools with the syntaxes:
perf stat -a -e l3cache/read-miss/
perf stat -a -e l3cache/event=0x21/
Quoted 6 lines
Signed-off-by Agustin Vega-Frias <[email protected]>--- arch/arm64/kernel/Makefile | 4 + arch/arm64/kernel/perf_event_qcom_l3_cache.c | 816 +++++++++++++++++++++++++++ 2 files changed, 820 insertions(+) create mode 100644 arch/arm64/kernel/perf_event_qcom_l3_cache.c
Move this to drivers/bus (where the CCI and CCN PMU drivers live), or drivers/perf (where some common infrastructure lives).
This isn't architectural, and isn't CPU-specific, so it has no reason to live in arch/arm64.
Quoted 22 lines
arch/arm64/kernel/perf_event_qcom_l3_cache.c new file +22
-0+static +int qcom_l3_cache__event_init (struct perf_event *event) +{ +struct hw_perf_event *hwc = &event->hw; + +if (event->attr.type != l3cache_pmu.pmu.type) +return -ENOENT ; + +/* +* There are no per-counter mode filters in the PMU. +*/ +if (event->attr.exclude_user || event->attr.exclude_kernel || + event->attr.exclude_hv || event->attr.exclude_idle) +return -EINVAL ; + + hwc->idx =-1 ; + +/* +* Sampling not supported since these events are not core-attributable. +*/ +if (hwc->sample_period) +return -EINVAL ;
arch/arm64/kernel/perf_event_qcom_l3_cache.c line 456
Quoted 10 lines
arch/arm64/kernel/perf_event_qcom_l3_cache.c new file +10
-0+ +/* +* Task mode not available, we run the counters as system counters, +* not attributable to any CPU and therefore cannot attribute per-task. +*/ +if (event->cpu <0 ) +return -EINVAL ; + +return 0 ; +}
Please follow what the other system PMUs do and (forcefully) ensure that all events share the same CPU in event_init.
Otherwise, events can exist in multiple percpu contexts, and management thereof can race on things line pmu_{enable,disable}.
You'll also want to expose a cpumask to userspace for that, to ensure that it only opens events on a single CPU.
For an example, see drivers/bus/arm-ccn.c. That also handles migrating events to a new CPU upon hotplug.
Quoted 18 lines
arch/arm64/kernel/perf_event_qcom_l3_cache.c new file +18
-0+static +int qcom_l3_cache__event_add (struct perf_event *event,int flags) +{ +struct l3cache_pmu *system =to_l3cache_pmu (event->pmu); +struct hw_perf_event *hwc = &event->hw; +int idx; +int prev_cpu; +int err =0 ; + +/* +* We need to disable the pmu while adding the event, otherwise +* the perf tick might kick-in and re-add this event. +*/ +perf_pmu_disable (event->pmu); + +/* +* This ensures all events are on the same CPU context. No need to open +* these on all CPUs since they are system events. The strategy here is
arch/arm64/kernel/perf_event_qcom_l3_cache.c line 536
Quoted 10 lines
arch/arm64/kernel/perf_event_qcom_l3_cache.c new file +10
-0+* to set system->cpu when the first event is created and from that +* point on, only events in the same CPU context will be allowed to be +* active. system->cpu will get reset back to -1 when the last event +* is deleted, please see qcom_l3_cache__event_del below. +*/ + prev_cpu =atomic_cmpxchg (&system->cpu,-1 , event->cpu); +if ((event->cpu != prev_cpu) && (prev_cpu !=-1 )) { + err = -EAGAIN ; + goto out; + }
As above, handle this in event_init, as other system PMUs do.
Handling it here is rather late, and unnecessarily fragile.
Quoted 20 lines
arch/arm64/kernel/perf_event_qcom_l3_cache.c new file +20
-0+/* +* In some platforms interrupt resources might not come directly from the GIC, +* but from separate IRQ circuitry that signals a summary IRQ to the GIC and +* is handled by a secondary IRQ controller. This is problematic under ACPI boot +* because the ACPI core does not use the Resource Source field on the Extended +* Interrupt Descriptor, which in theory could be used to specify an alternative +* IRQ controller. + +* For this reason in these platforms we implement the secondary IRQ controller +* using the gpiolib and specify the IRQs as GpioInt resources, so when getting +* an IRQ from the device we first try platform_get_irq and if it fails we try +* devm_gpiod_get_index/gpiod_to_irq. +*/ +static +int qcom_l3_cache_pmu_get_slice_irq (struct platform_device *pdev, +struct platform_device *sdev) +{ +int virq =platform_get_irq (sdev,0 ); +struct gpio_desc *desc; +
arch/arm64/kernel/perf_event_qcom_l3_cache.c line 693
Quoted 10 lines
arch/arm64/kernel/perf_event_qcom_l3_cache.c new file +10
-0+if (virq >=0 ) +return virq; + + desc =devm_gpiod_get_index (&sdev->dev,NULL ,0 ,GPIOD_ASIS ); +if (IS_ERR (desc)) +return -ENOENT ; + +return gpiod_to_irq (desc); +} +
As Marc Zyngier pointed out in another thread, you should represent your interrupt controller as an interrupt controller rather than pretending it is a GPIO controller.
Drivers should be able to remain blissfully unaware what the other end of their interrupt line is wired up to, and shouldn't have to jump through hoops like the above.
Thanks,
Mark.
PATCH 1/1 perf: qcom: Add L3 cache PMU driver
Quoted 21 lines · Mark Rutland
Quoted 13 lines · Agustin Vega-Frias
This adds a new dynamic PMU to the Perf Events framework to program and control the L3 cache PMUs in some Qualcomm Technologies SOCs.
The driver supports a distributed cache architecture where the overall cache is comprised of multiple slices each with its own PMU. The driver aggregates counts across the whole system to provide a global picture of the metrics selected by the user.
The driver exports formatting and event information to sysfs so it can be used by the perf user space tools with the syntaxes:
perf stat -a -e l3cache/read-miss/
perf stat -a -e l3cache/event=0x21/Quoted 6 lines
Signed-off-by Agustin Vega-Frias <[email protected]>--- arch/arm64/kernel/Makefile | 4 + arch/arm64/kernel/perf_event_qcom_l3_cache.c | 816 +++++++++++++++++++++++++++ 2 files changed, 820 insertions(+) create mode 100644 arch/arm64/kernel/perf_event_qcom_l3_cache.c
Quoted 2 lines
Move this to
drivers/bus(where the CCI and CCN PMU drivers live), ordrivers/perf(where some common infrastructure lives).
Please stick to drivers/perf, as I have a vague plan to move the CCI/CCN PMU code out of drivers/bus and into drivers/perf (which didn't exist when they were originally upstreamed).
Will
Peter Zijlstra
maintainerPATCH 1/1 perf: qcom: Add L3 cache PMU driver
In reply to Mark RutlandQuoted 6 lines · Mark Rutland
Quoted 6 lines
arch/arm64/kernel/perf_event_qcom_l3_cache.c new file +6
-0+static +int qcom_l3_cache__event_add (struct perf_event *event,int flags) +{ +struct l3cache_pmu *system =to_l3cache_pmu (event->pmu); +struct hw_perf_event *hwc = &event->hw; +int idx;
Quoted 8 lines
arch/arm64/kernel/perf_event_qcom_l3_cache.c line 522
Quoted 8 lines
arch/arm64/kernel/perf_event_qcom_l3_cache.c new file +8
-0+int prev_cpu; +int err =0 ; + +/* +* We need to disable the pmu while adding the event, otherwise +* the perf tick might kick-in and re-add this event. +*/ +perf_pmu_disable (event->pmu);
Why did you write that? If you really need this you did something seriously wrong elsewhere, because:
kernel/events/core.c:event_sched_in() is the only place calling pmu::add() and that explicitly already does this.
PATCH 1/1 perf: qcom: Add L3 cache PMU driver
Quoted 19 lines · Peter Zijlstra
Quoted 6 lines · Mark Rutland
Quoted 6 lines
arch/arm64/kernel/perf_event_qcom_l3_cache.c new file +6
-0+static +int qcom_l3_cache__event_add (struct perf_event *event,int flags) +{ +struct l3cache_pmu *system =to_l3cache_pmu (event->pmu); +struct hw_perf_event *hwc = &event->hw; +int idx;Quoted 8 lines
arch/arm64/kernel/perf_event_qcom_l3_cache.c line 522
Quoted 8 lines
arch/arm64/kernel/perf_event_qcom_l3_cache.c new file +8
-0+int prev_cpu; +int err =0 ; + +/* +* We need to disable the pmu while adding the event, otherwise +* the perf tick might kick-in and re-add this event. +*/ +perf_pmu_disable (event->pmu);Why did you write that? If you really need this you did something seriously wrong elsewhere, because:
Quoted 2 lines
kernel/events/core.c:event_sched_in() is the only place calling
pmu::add()and that explicitly already does this.
This might have been before I added the restriction that only one CPU can open the events, but I will double-check and remove this as it is unnecessary.
Thanks,
Agustin
PATCH 1/1 perf: qcom: Add L3 cache PMU driver
In reply to Mark RutlandQuoted 23 lines · Mark Rutland
Quoted 14 lines · Agustin Vega-Frias
This adds a new dynamic PMU to the Perf Events framework to program and control the L3 cache PMUs in some Qualcomm Technologies SOCs.
The driver supports a distributed cache architecture where the overall cache is comprised of multiple slices each with its own PMU. The driver
aggregates counts across the whole system to provide a global picture of the metrics selected by the user.The driver exports formatting and event information to sysfs so it can be used by the perf user space tools with the syntaxes:
perf stat -a -e l3cache/read-miss/
perf stat -a -e l3cache/event=0x21/Quoted 7 lines
Signed-off-by Agustin Vega-Frias <[email protected]>--- arch/arm64/kernel/Makefile | 4 + arch/arm64/kernel/perf_event_qcom_l3_cache.c | 816 +++++++++++++++++++++++++++ 2 files changed, 820 insertions(+) create mode 100644 arch/arm64/kernel/perf_event_qcom_l3_cache.c
Quoted 7 lines
Move this to
drivers/bus(where the CCI and CCN PMU drivers live), ordrivers/perf(where some common infrastructure lives).This isn't architectural, and isn't CPU-specific, so it has no reason to
live inarch/arm64.
I will move it to drivers/perf. Thanks.
Quoted 42 lines
Quoted 28 lines
arch/arm64/kernel/perf_event_qcom_l3_cache.c new file +19
-0+static +int qcom_l3_cache__event_init (struct perf_event *event) +{ +struct hw_perf_event *hwc = &event->hw; + +if (event->attr.type != l3cache_pmu.pmu.type) +return -ENOENT ; + +/* +* There are no per-counter mode filters in the PMU. +*/ +if (event->attr.exclude_user || event->attr.exclude_kernel || + event->attr.exclude_hv || event->attr.exclude_idle) +return -EINVAL ; + + hwc->idx =-1 ; + +/* +* Sampling not supported since these events are not core-attributable.
arch/arm64/kernel/perf_event_qcom_l3_cache.c new file +7
-0+*/ +if (hwc->sample_period) +return -EINVAL ; + +/* +* Task mode not available, we run the counters as system counters, +* not attributable to any CPU and therefore cannot attribute per-task.
arch/arm64/kernel/perf_event_qcom_l3_cache.c line 456
Quoted 6 lines
arch/arm64/kernel/perf_event_qcom_l3_cache.c new file +6
-0+*/ +if (event->cpu <0 ) +return -EINVAL ; + +return 0 ; +}Please follow what the other system PMUs do and (forcefully) ensure that all
events share the same CPU inevent_init.Otherwise, events can exist in multiple percpu contexts, and management thereof can race on things line pmu_{enable,disable}.
Quoted 6 lines
You'll also want to expose a cpumask to userspace for that, to ensure that it
only opens events on a single CPU.For an example, see
drivers/bus/arm-ccn.c. That also handles migrating events to a new CPU upon hotplug.
Understood. I will look at the CCN implementation as reference,
Quoted 35 lines
Quoted 24 lines
arch/arm64/kernel/perf_event_qcom_l3_cache.c new file +17
-0+static +int qcom_l3_cache__event_add (struct perf_event *event,int flags) +{ +struct l3cache_pmu *system =to_l3cache_pmu (event->pmu); +struct hw_perf_event *hwc = &event->hw; +int idx; +int prev_cpu; +int err =0 ; + +/* +* We need to disable the pmu while adding the event, otherwise +* the perf tick might kick-in and re-add this event. +*/ +perf_pmu_disable (event->pmu); + +/* +* This ensures all events are on the same CPU context. No need to open
(quoted diff) +1
-0+ * these on all CPUs since they are system events. The strategy hereis
(quoted diff) +2
-0+ * to set system->cpu when the first event is created and from that + * point on, only events in the same CPU context will be allowed tobe
arch/arm64/kernel/perf_event_qcom_l3_cache.c line 536
Quoted 8 lines
arch/arm64/kernel/perf_event_qcom_l3_cache.c new file +8
-0+* active. system->cpu will get reset back to -1 when the last event +* is deleted, please see qcom_l3_cache__event_del below. +*/ + prev_cpu =atomic_cmpxchg (&system->cpu,-1 , event->cpu); +if ((event->cpu != prev_cpu) && (prev_cpu !=-1 )) { + err = -EAGAIN ; + goto out; + }As above, handle this in
event_init, as other system PMUs do.
Quoted 1 line
Handling it here is rather late, and unnecessarily fragile.
Understood. I will look at the CCN implementation as reference,
Quoted 46 lines
Quoted 29 lines
+/*
+ * In some platforms interrupt resources might not come directly from the GIC,
+ * but from separate IRQ circuitry that signals a summary IRQ to the GIC and
+ * is handled by a secondary IRQ controller. This is problematic under ACPI boot
+ * because the ACPI core does not use the Resource Source field on the Extended
+ * Interrupt Descriptor, which in theory could be used to specify an alternative
+ * IRQ controller.
+
+ * For this reason in these platforms we implement the secondary IRQ controller
+ * using the gpiolib and specify the IRQs as GpioInt resources, so when getting
+ * an IRQ from the device we first tryplatform_get_irqand if it fails we try
+ * devm_gpiod_get_index/gpiod_to_irq.
+ */arch/arm64/kernel/perf_event_qcom_l3_cache.c new file +7
-0+static +int qcom_l3_cache_pmu_get_slice_irq (struct platform_device *pdev, +struct platform_device *sdev) +{ +int virq =platform_get_irq (sdev,0 ); +struct gpio_desc *desc; +arch/arm64/kernel/perf_event_qcom_l3_cache.c line 693
Quoted 10 lines
arch/arm64/kernel/perf_event_qcom_l3_cache.c new file +10
-0+if (virq >=0 ) +return virq; + + desc =devm_gpiod_get_index (&sdev->dev,NULL ,0 ,GPIOD_ASIS ); +if (IS_ERR (desc)) +return -ENOENT ; + +return gpiod_to_irq (desc); +} +As Marc Zyngier pointed out in another thread, you should represent your
interrupt controller as an interrupt controller rather than pretending it is a
GPIO controller.
Quoted 5 lines
Drivers should be able to remain blissfully unaware what the other end of their
interrupt line is wired up to, and shouldn't have to jump through hoops like
the above.
Understood. We need to close the loop with Rafael J. Wysocki w.r.t. ACPI support for multiple regular IRQ controllers similar to DT.
Quoted 3 lines
Thanks,
Mark.
Thanks,
Agustin
PATCH 1/1 perf: qcom: Add L3 cache PMU driver
In reply to Mark RutlandQuoted 46 lines · Mark Rutland
Quoted 28 lines · Agustin Vega-Frias
+/*
+ * In some platforms interrupt resources might not come directly from the GIC,
+ * but from separate IRQ circuitry that signals a summary IRQ to the GIC and
+ * is handled by a secondary IRQ controller. This is problematic under ACPI boot
+ * because the ACPI core does not use the Resource Source field on the Extended
+ * Interrupt Descriptor, which in theory could be used to specify an alternative
+ * IRQ controller.
+
+ * For this reason in these platforms we implement the secondary IRQ controller
+ * using the gpiolib and specify the IRQs as GpioInt resources, so when getting
+ * an IRQ from the device we first tryplatform_get_irqand if it fails we try
+ * devm_gpiod_get_index/gpiod_to_irq.
+ */arch/arm64/kernel/perf_event_qcom_l3_cache.c new file +7
-0+static +int qcom_l3_cache_pmu_get_slice_irq (struct platform_device *pdev, +struct platform_device *sdev) +{ +int virq =platform_get_irq (sdev,0 ); +struct gpio_desc *desc; +arch/arm64/kernel/perf_event_qcom_l3_cache.c line 693
Quoted 10 lines
arch/arm64/kernel/perf_event_qcom_l3_cache.c new file +10
-0+if (virq >=0 ) +return virq; + + desc =devm_gpiod_get_index (&sdev->dev,NULL ,0 ,GPIOD_ASIS ); +if (IS_ERR (desc)) +return -ENOENT ; + +return gpiod_to_irq (desc); +} +As Marc Zyngier pointed out in another thread, you should represent your
interrupt controller as an interrupt controller rather than pretending it is a
GPIO controller.
Quoted 8 lines
Drivers should be able to remain blissfully unaware what the other end of their
interrupt line is wired up to, and shouldn't have to jump through hoops like
the above.Thanks,
Mark.
Given that this driver is ACPI-only we are leaning toward implementing overflow signalling as ACPI events.
This will hide these details from the driver and use standard ACPI APIs.
Thoughts?
Thanks,
Agustin
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH 1/1 perf: qcom: Add L3 cache PMU driver
Quoted 88 lines · Marc Zyngier
Quoted 63 lines · agustinv at codeaurora.org
Quoted 59 lines · Mark Rutland
Quoted 32 lines · Agustin Vega-Frias
+/*
+ * In some platforms interrupt resources might not come directly from
the GIC,
+ * but from separate IRQ circuitry that signals a summary IRQ to the
GIC and
+ * is handled by a secondary IRQ controller. This is problematic under ACPI boot
+ * because the ACPI core does not use the Resource Source field on the Extended
+ * Interrupt Descriptor, which in theory could be used to specify an
alternative
+ * IRQ controller.
+
+ * For this reason in these platforms we implement the secondary IRQ
controller
+ * using the gpiolib and specify the IRQs as GpioInt resources, so when getting
+ * an IRQ from the device we first tryplatform_get_irqand if it fails we try
+ * devm_gpiod_get_index/gpiod_to_irq.
+ */arch/arm64/kernel/perf_event_qcom_l3_cache.c new file +7
-0+static +int qcom_l3_cache_pmu_get_slice_irq (struct platform_device *pdev, +struct platform_device *sdev) +{ +int virq =platform_get_irq (sdev,0 ); +struct gpio_desc *desc; +arch/arm64/kernel/perf_event_qcom_l3_cache.c line 693
Quoted 10 lines
arch/arm64/kernel/perf_event_qcom_l3_cache.c new file +10
-0+if (virq >=0 ) +return virq; + + desc =devm_gpiod_get_index (&sdev->dev,NULL ,0 ,GPIOD_ASIS ); +if (IS_ERR (desc)) +return -ENOENT ; + +return gpiod_to_irq (desc); +} +As Marc Zyngier pointed out in another thread, you should represent your
interrupt controller as an interrupt controller rather than pretending
it is a
GPIO controller.Drivers should be able to remain blissfully unaware what the other end
of their
interrupt line is wired up to, and shouldn't have to jump through hoops
like
the above.Quoted 2 lines
Thanks,
Mark.Quoted 6 lines
Given that this driver is ACPI-only we are leaning toward implementing overflow signalling as ACPI events.
This will hide these details from the driver and use standard ACPI APIs.Thoughts?
Please don't do that. The HW (whatever that is, and whatever the firmware is) provides you with an interrupt line, not an ACPI event. "Hiding the details" is always the wrong thing to do. How do we cope with CPU affinity, for example? How do we selectively disable this interrupt at the controller level?
Also, ACPI events will be signalled by (guess what?) an interrupt. So what are we actually gaining here?
I'd *really* advise you to stick to existing abstractions, as they:
- accurately describe the way the HW works
- are already supported in the kernelThe existing shortcomings of the ACPI layer should be addressed pretty easily (if the feature is not supported on x86, let's find out why - we can make it an arm64 feature if we really have to).
Quoted 5 lines
Thanks,
M.--
Jazz is not dead. It just smells funny...
ACPI events *are* existing abstractions within ACPI-based systems and the mechanism works across OSes.
If there was a one to one relationship between IRQs and events this would be hiding for the sake of hiding.
In this case I am proposing to demultiplex the IRQ in the ACPI layer just as a secondary IRQ driver/domain does in DT.
I have reached out to Rafael and Jeremy Pieralisi w.r.t. support of multiple IRQ domains via the Resource Source field.
On the other hand we had implemented the IRQ combiner driver as a GPIO chip because the hardware is not an interrupt controller, it is an "interrupt
combiner", which is very similar to a GPIO controller.
Consider how a GPIO controller works: it sends a summary interrupt when a GPIO is toggled, software reads a status register to determine which pin was toggled. These combiners do the *exact* same thing except it's based on internal chip wires instead of external pins: it sends a summary interrupt when any wire is toggled and software checks a status register to determine which wire was toggled. That's pretty similar if you ask me.
Given that we need to support other operating systems representing it as a GPIO controller was the most OS agnostic way to
implement something like this on ACPI based systems.
Thanks,
Agustin.
Signature · 4 lines
Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Peter Zijlstra
maintainerPATCH 1/1 perf: qcom: Add L3 cache PMU driver
Quoted 2 lines · agustinv at codeaurora.org
ACPI events *are* existing abstractions within ACPI-based systems and the mechanism works across OSes.
Correction; it never works anywhere. Its a nice idea on paper, but when you're stuck with a firmware that hands you random values pulled from a crack monkey's behind you have to then hack around, you wish you had done the sane thing and stuffed it in code you can actually fix.
Peter Zijlstra
maintainerPATCH 1/1 perf: qcom: Add L3 cache PMU driver
In reply to Agustin Vega-Friasarch/arm64/kernel/perf_event_qcom_l3_cache.c line 747
Quoted 9 lines · Agustin Vega-Frias
arch/arm64/kernel/perf_event_qcom_l3_cache.c new file +9
-0+static int qcom_l3_cache_pmu_probe (struct platform_device *pdev) +{ +int result; + +INIT_LIST_HEAD (&l3cache_pmu.pmus); + +atomic_set (&l3cache_pmu.cpu,-1 ); + l3cache_pmu.pmu = (struct pmu ) { + .task_ctx_nr = perf_hw_context,
This cannot be right. There should only be a single perf_hw_context driver in the system and that is typically the core pmu.
Also, since this is an L3 PMU, it seems very unlikely that these events are actually per logical CPU, which is required for per-task events.
If these events are indeed not per logical CPU but per L3 cluster, you should designate a single logical CPU per cluster to handle these events. See for example arch/x86/events/intel/rapl.c for a relatively simple PMU driver that has similar constraints.
arch/arm64/kernel/perf_event_qcom_l3_cache.c line 759
Quoted 11 lines
arch/arm64/kernel/perf_event_qcom_l3_cache.c new file +11
-0+ .name = "l3cache", + .pmu_enable = qcom_l3_cache__pmu_enable, + .pmu_disable = qcom_l3_cache__pmu_disable, + .event_init = qcom_l3_cache__event_init, + .add = qcom_l3_cache__event_add, + .del = qcom_l3_cache__event_del, + .start = qcom_l3_cache__event_start, + .stop = qcom_l3_cache__event_stop, + .read = qcom_l3_cache__event_read, + + .event_idx = dummy_event_idx,
perf_event_idx_default() wasn't good enough?
Quoted 2 lines
(quoted diff) +2
-0+ .attr_groups = qcom_l3_cache_pmu_attr_grps, + };