diff mbox

[RFC,5/5] arm: perf: Use FIQ to handle PMU events.

Message ID 1421166931-14134-6-git-send-email-daniel.thompson@linaro.org
State New
Headers show

Commit Message

Daniel Thompson Jan. 13, 2015, 4:35 p.m. UTC
Using FIQ (if it is available) gives perf a better insight into the
system by allowing code run with interrupts disabled to be profiled.

Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
---
 arch/arm/include/asm/pmu.h       |  4 ++++
 arch/arm/kernel/perf_event.c     |  2 +-
 arch/arm/kernel/perf_event_cpu.c | 35 ++++++++++++++++++++++++++++++++---
 arch/arm/kernel/traps.c          |  3 ++-
 4 files changed, 39 insertions(+), 5 deletions(-)

Comments

Daniel Thompson Jan. 20, 2015, 10:04 a.m. UTC | #1
On 19/01/15 17:48, Russell King - ARM Linux wrote:
> On Tue, Jan 13, 2015 at 04:35:31PM +0000, Daniel Thompson wrote:
>> +/*
>> + * This handler is called *unconditionally* from the default NMI/FIQ
>> + * handler. The irq may not be anything to do with us so the main
>> + * job of this function is to figure out if the irq passed in is ours
>> + * or not.
>> + */
>> +void cpu_pmu_handle_fiq(int irq)
>> +{
>> +	int cpu = smp_processor_id();
> 
> This can be either debug_smp_processor_id() or raw_smp_processor_id().
> raw_smp_processor_id() is fine from FIQ contexts, as seems to be
> debug_smp_processor_id(), but only because we guarantee that
> irqs_disabled() in there will be true.

Curiously I was looking at exactly this yesterday (because I was
intrigued why the NMI-safe bits of kgdb use raw_smp_processor_id() but
the x86 arch_trigger_all_cpu_backtrace() implementation uses
smp_processor_id()).

Given the comments make clear smp_processor_id() is the preferred
variant except for false positives I concluded I would continue with
smp_processor_id() for any code I write hanging off the default FIQ
handler. No objections?


>> +
>> +	if (irq != get_cpu_var(cpu_pmu_irqs))
>> +		return;
> 
> get_cpu_var() needs put_cpu_var() to undo its effects.  get_cpu_var()
> calls preempt_disable(), which calls into lockdep...  I think we
> determined that was fine last time we went digging?

Yes. We reviewed lockdep from the point-of-view of RCU and found that
lockdep disabled most of itself when in_nmi() is true.

> put_cpu_var()
> would call preempt_enable() which I'd hope would be safe in FIQ/NMI
> contexts?

Yes.

preempt_count_add/sub form part of the work done by nmi_enter() and
nmi_exit().

However this code gets no benefit from calling get_cpu_var(). I think it
would be better to switch it to this_cpu_ptr.


>> +
>> +	(void)armpmu_dispatch_irq(irq,
>> +				  get_cpu_ptr(&cpu_pmu->hw_events->percpu_pmu));
> 
> Again, get_cpu_xxx() needs to be balanced with a put_cpu_xxx().
> 
>
diff mbox

Patch

diff --git a/arch/arm/include/asm/pmu.h b/arch/arm/include/asm/pmu.h
index b1596bd59129..2a7ea97a4a14 100644
--- a/arch/arm/include/asm/pmu.h
+++ b/arch/arm/include/asm/pmu.h
@@ -123,6 +123,8 @@  struct arm_pmu {
 
 extern const struct dev_pm_ops armpmu_dev_pm_ops;
 
+irqreturn_t armpmu_dispatch_irq(int irq, void *dev);
+
 int armpmu_register(struct arm_pmu *armpmu, int type);
 
 u64 armpmu_event_update(struct perf_event *event);
@@ -136,6 +138,8 @@  int armpmu_map_event(struct perf_event *event,
 						[PERF_COUNT_HW_CACHE_RESULT_MAX],
 		     u32 raw_event_mask);
 
+void cpu_pmu_handle_fiq(int irq);
+
 struct pmu_probe_info {
 	unsigned int cpuid;
 	unsigned int mask;
diff --git a/arch/arm/kernel/perf_event.c b/arch/arm/kernel/perf_event.c
index f7c65adaa428..5ae9adf7f18e 100644
--- a/arch/arm/kernel/perf_event.c
+++ b/arch/arm/kernel/perf_event.c
@@ -296,7 +296,7 @@  validate_group(struct perf_event *event)
 	return 0;
 }
 
-static irqreturn_t armpmu_dispatch_irq(int irq, void *dev)
+irqreturn_t armpmu_dispatch_irq(int irq, void *dev)
 {
 	struct arm_pmu *armpmu;
 	struct platform_device *plat_device;
diff --git a/arch/arm/kernel/perf_event_cpu.c b/arch/arm/kernel/perf_event_cpu.c
index a80309087a7b..5c4e9ce23389 100644
--- a/arch/arm/kernel/perf_event_cpu.c
+++ b/arch/arm/kernel/perf_event_cpu.c
@@ -36,6 +36,9 @@ 
 /* Set at runtime when we know what CPU type we are. */
 static struct arm_pmu *cpu_pmu;
 
+/* Allows us to find out if an IRQ is for us (mostly used from NMI context) */
+static DEFINE_PER_CPU(int, cpu_pmu_irqs);
+
 /*
  * Despite the names, these two functions are CPU-specific and are used
  * by the OProfile/perf code.
@@ -127,6 +130,24 @@  static void cpu_pmu_free_irq(struct arm_pmu *cpu_pmu)
 	}
 }
 
+/*
+ * This handler is called *unconditionally* from the default NMI/FIQ
+ * handler. The irq may not be anything to do with us so the main
+ * job of this function is to figure out if the irq passed in is ours
+ * or not.
+ */
+void cpu_pmu_handle_fiq(int irq)
+{
+	int cpu = smp_processor_id();
+
+	if (irq != get_cpu_var(cpu_pmu_irqs))
+		return;
+
+	(void)armpmu_dispatch_irq(irq,
+				  get_cpu_ptr(&cpu_pmu->hw_events->percpu_pmu));
+}
+
+
 static int cpu_pmu_request_irq(struct arm_pmu *cpu_pmu, irq_handler_t handler)
 {
 	int i, err, irq, irqs;
@@ -170,9 +191,16 @@  static int cpu_pmu_request_irq(struct arm_pmu *cpu_pmu, irq_handler_t handler)
 				continue;
 			}
 
-			err = request_irq(irq, handler,
-					  IRQF_NOBALANCING | IRQF_NO_THREAD, "arm-pmu",
-					  per_cpu_ptr(&hw_events->percpu_pmu, i));
+			err = request_nmi_irq(
+			    irq, IRQF_NOBALANCING, "arm-pmu",
+			    per_cpu_ptr(&hw_events->percpu_pmu, i));
+			if (err) {
+				err = request_irq(
+				    irq, handler,
+				    IRQF_NOBALANCING | IRQF_NO_THREAD,
+				    "arm-pmu",
+				    per_cpu_ptr(&hw_events->percpu_pmu, i));
+			}
 			if (err) {
 				pr_err("unable to request IRQ%d for ARM PMU counters\n",
 					irq);
@@ -180,6 +208,7 @@  static int cpu_pmu_request_irq(struct arm_pmu *cpu_pmu, irq_handler_t handler)
 			}
 
 			cpumask_set_cpu(i, &cpu_pmu->active_irqs);
+			per_cpu(cpu_pmu_irqs, i) = irq;
 		}
 	}
 
diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
index 74c752b9db68..c581e07517ff 100644
--- a/arch/arm/kernel/traps.c
+++ b/arch/arm/kernel/traps.c
@@ -38,6 +38,7 @@ 
 #include <asm/tls.h>
 #include <asm/system_misc.h>
 #include <asm/opcodes.h>
+#include <asm/pmu.h>
 
 
 static const char *handler[]= {
@@ -485,7 +486,7 @@  asmlinkage void __exception_irq_entry handle_fiq_as_nmi(struct pt_regs *regs)
 	irq = gic_ack_fiq();
 
 	if (irq) {
-		/* empty - no SPI handlers (yet) */
+		cpu_pmu_handle_fiq(irq);
 	} else {
 #ifdef CONFIG_SMP
 		ipi_cpu_backtrace(regs);