ARM perf events support v4

This is the fourth revision of ARM performance counter support. This adds support for software performance events on all ARM platforms and hardware support for v6 processors. There is a generic PMU framework to allow for v7 and later to be added in the future.

Jamie Iles (5):
arm: provide a mechanism to reserve performance counters arm/oprofile: reserve the PMU when starting
arm: use the spinlocked, generic atomic64 support
arm: enable support for software perf events arm/perfevents: implement perf event support for ARMv6

 arch/arm/Kconfig                        |   16 +
 arch/arm/include/asm/atomic.h           |    4 +
 arch/arm/include/asm/perf_event.h       |   38 +
 arch/arm/include/asm/pmu.h              |   74 ++
 arch/arm/kernel/Makefile                |    2 +
 arch/arm/kernel/perf_event.c            | 1338 +++++++++++++++++++++++++++++++
 arch/arm/kernel/pmu.c                   |  107 +++
 arch/arm/mm/fault.c                     |    7 +
 arch/arm/oprofile/op_model_arm11_core.c |    4 +-
 arch/arm/oprofile/op_model_arm11_core.h |    4 +-
 arch/arm/oprofile/op_model_mpcore.c     |   42 +-
 arch/arm/oprofile/op_model_v6.c         |   30 +-
 arch/arm/oprofile/op_model_v7.c         |   30 +-
 arch/arm/oprofile/op_model_v7.h         |    4 +-
 arch/arm/oprofile/op_model_xscale.c     |   35 +-
 15 files changed, 1671 insertions(+), 64 deletions(-)

Jamie

To add support for perf events and to allow the hardware counters to be shared with oprofile, we need a way to reserve access to the pmu (performance monitor unit).

---
 arch/arm/Kconfig           |    5 ++
 arch/arm/include/asm/pmu.h |   74 ++++++++++++++++++++++++++++++
 arch/arm/kernel/Makefile   |    1 +
 arch/arm/kernel/pmu.c      |  107 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 187 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/include/asm/pmu.h
 create mode 100644 arch/arm/kernel/pmu.c

index 233a222..9e08891 100644
@@ -863,6 +863,11 @@ config XSCALE_PMU
 	depends on CPU_XSCALE && !XSCALE_PMU_TIMER
 	default y
 
+config CPU_HAS_PMU
+	depends on CPU_V6 || CPU_V7 || XSCALE_PMU
+	default y
+	bool
+
 if !MMU
 source "arch/arm/Kconfig-nommu"
 endif

index dd00f74..216890d 100644
@@ -46,6 +46,7 @@ obj-$(CONFIG_CPU_XSCALE)	+= xscale-cp0.o
 obj-$(CONFIG_CPU_XSC3)		+= xscale-cp0.o
 obj-$(CONFIG_CPU_MOHAWK)	+= xscale-cp0.o
 obj-$(CONFIG_IWMMXT)		+= iwmmxt.o
+obj-$(CONFIG_CPU_HAS_PMU)	+= pmu.o
 AFLAGS_iwmmxt.o			:= -Wa,-mcpu=iwmmxt
 
 ifneq ($(CONFIG_ARCH_EBSA110),y)

1.6.5.4

Make sure that we have access to the performance counters and that they aren't being used by perf events or anything else.

---
 arch/arm/oprofile/op_model_arm11_core.c |    4 +-
 arch/arm/oprofile/op_model_arm11_core.h |    4 +-
 arch/arm/oprofile/op_model_mpcore.c     |   42 ++++++++++++++++--------------
 arch/arm/oprofile/op_model_v6.c         |   30 ++++++++++++++--------
 arch/arm/oprofile/op_model_v7.c         |   30 ++++++++++++++--------
 arch/arm/oprofile/op_model_v7.h         |    4 +-
 arch/arm/oprofile/op_model_xscale.c     |   35 ++++++++++++++-----------
 7 files changed, 85 insertions(+), 64 deletions(-)

index ad80752..ef3e265 100644
@@ -132,7 +132,7 @@ static irqreturn_t arm11_pmu_interrupt(int irq, void *arg)
 	return IRQ_HANDLED;
 }
 
-int arm11_request_interrupts(int *irqs, int nr)
+int arm11_request_interrupts(const int *irqs, int nr)
 {
 	unsigned int i;
 	int ret = 0;

@@ -153,7 +153,7 @@ int arm11_request_interrupts(int *irqs, int nr)
 	return ret;
 }
 
-void arm11_release_interrupts(int *irqs, int nr)
+void arm11_release_interrupts(const int *irqs, int nr)
 {
 	unsigned int i;
 

index 6f8538e..1902b99 100644
@@ -39,7 +39,7 @@
 int arm11_setup_pmu(void);
 int arm11_start_pmu(void);
 int arm11_stop_pmu(void);
-int arm11_request_interrupts(int *, int);
-void arm11_release_interrupts(int *, int);
+int arm11_request_interrupts(const int *, int);
+void arm11_release_interrupts(const int *, int);
 
 #endif

index f7d2ec5..a22357a 100644
@@ -19,39 +19,47 @@
 /* #define DEBUG */
 #include <linux/types.h>
 #include <linux/errno.h>
+#include <linux/err.h>
 #include <linux/sched.h>
 #include <linux/oprofile.h>
 #include <linux/interrupt.h>
 #include <asm/irq.h>
 #include <asm/system.h>
+#include <asm/pmu.h>
 
 #include "op_counter.h"
 #include "op_arm_model.h"
 #include "op_model_arm11_core.h"
 
-static int irqs[] = {
-#ifdef CONFIG_ARCH_OMAP2
-	3,
-#endif
-#ifdef CONFIG_ARCH_BCMRING
-	IRQ_PMUIRQ, /* for BCMRING, ARM PMU interrupt is 43 */
-#endif
-};
+static const struct pmu_irqs *pmu_irqs;
 
 static void armv6_pmu_stop(void)
 {
 	arm11_stop_pmu();
-	arm11_release_interrupts(irqs, ARRAY_SIZE(irqs));
+	arm11_release_interrupts(pmu_irqs->irqs, pmu_irqs->num_irqs);
+	release_pmu(pmu_irqs);
+	pmu_irqs = NULL;
 }
 
 static int armv6_pmu_start(void)
 {
 	int ret;
 
-	ret = arm11_request_interrupts(irqs, ARRAY_SIZE(irqs));
-	if (ret >= 0)
+	pmu_irqs = reserve_pmu();
+	if (IS_ERR(pmu_irqs)) {
+		ret = PTR_ERR(pmu_irqs);
+		goto out;
+	}
+
+	ret = arm11_request_interrupts(pmu_irqs->irqs, pmu_irqs->num_irqs);
+	if (ret >= 0) {
 		ret = arm11_start_pmu();
+	} else {
+		release_pmu(pmu_irqs);
+		pmu_irqs = NULL;
+	}
 
+out:
 	return ret;
 }
 

index 2088a6c..8642d08 100644
@@ -11,11 +11,14 @@
  */
 #include <linux/types.h>
 #include <linux/errno.h>
+#include <linux/err.h>
 #include <linux/oprofile.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/smp.h>
 
+#include <asm/pmu.h>
+
 #include "op_counter.h"
 #include "op_arm_model.h"
 #include "op_model_v7.h"

@@ -295,7 +298,7 @@ static irqreturn_t armv7_pmnc_interrupt(int irq, void *arg)
 	return IRQ_HANDLED;
 }
 
-int armv7_request_interrupts(int *irqs, int nr)
+int armv7_request_interrupts(const int *irqs, int nr)
 {
 	unsigned int i;
 	int ret = 0;

@@ -318,7 +321,7 @@ int armv7_request_interrupts(int *irqs, int nr)
 	return ret;
 }
 
-void armv7_release_interrupts(int *irqs, int nr)
+void armv7_release_interrupts(const int *irqs, int nr)
 {
 	unsigned int i;
 

@@ -362,12 +365,7 @@ static void armv7_pmnc_dump_regs(void)
 }
 #endif
 
-
-static int irqs[] = {
-#ifdef CONFIG_ARCH_OMAP3
-	INT_34XX_BENCH_MPU_EMUL,
-#endif
-};
+static const struct pmu_irqs *pmu_irqs;
 
 static void armv7_pmnc_stop(void)
 {

@@ -375,19 +373,29 @@ static void armv7_pmnc_stop(void)
 	armv7_pmnc_dump_regs();
 #endif
 	armv7_stop_pmnc();
-	armv7_release_interrupts(irqs, ARRAY_SIZE(irqs));
+	armv7_release_interrupts(pmu_irqs->irqs, pmu_irqs->num_irqs);
+	release_pmu(pmu_irqs);
+	pmu_irqs = NULL;
 }
 
 static int armv7_pmnc_start(void)
 {
 	int ret;
 
+	pmu_irqs = reserve_pmu();
+	if (IS_ERR(pmu_irqs))
+		return PTR_ERR(pmu_irqs);
+
 #ifdef DEBUG
 	armv7_pmnc_dump_regs();
 #endif
-	ret = armv7_request_interrupts(irqs, ARRAY_SIZE(irqs));
-	if (ret >= 0)
+	ret = armv7_request_interrupts(pmu_irqs->irqs, pmu_irqs->num_irqs);
+	if (ret >= 0) {
 		armv7_start_pmnc();
+	} else {
+		release_pmu(pmu_irqs);
+		pmu_irqs = NULL;
+	}
 
 	return ret;
 }

index 0e19bcc..9ca334b 100644
@@ -97,7 +97,7 @@
 int armv7_setup_pmu(void);
 int armv7_start_pmu(void);
 int armv7_stop_pmu(void);
-int armv7_request_interrupts(int *, int);
-void armv7_release_interrupts(int *, int);
+int armv7_request_interrupts(const int *, int);
+void armv7_release_interrupts(const int *, int);
 
 #endif

index 724ab9c..1d34a02 100644
@@ -17,12 +17,14 @@
 /* #define DEBUG */
 #include <linux/types.h>
 #include <linux/errno.h>
+#include <linux/err.h>
 #include <linux/sched.h>
 #include <linux/oprofile.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 
 #include <asm/cputype.h>
+#include <asm/pmu.h>
 
 #include "op_counter.h"
 #include "op_arm_model.h"

@@ -33,17 +35,6 @@
 #define	PMU_RESET	(CCNT_RESET | PMN_RESET)
 #define PMU_CNT64	0x008	/* Make CCNT count every 64th cycle */
 
-/* TODO do runtime detection */
-#ifdef CONFIG_ARCH_IOP32X
-#define XSCALE_PMU_IRQ  IRQ_IOP32X_CORE_PMU
-#endif
-#ifdef CONFIG_ARCH_IOP33X
-#define XSCALE_PMU_IRQ  IRQ_IOP33X_CORE_PMU
-#endif
-#ifdef CONFIG_ARCH_PXA
-#define XSCALE_PMU_IRQ  IRQ_PMU
-#endif
-
 /*
  * Different types of events that can be counted by the XScale PMU
  * as used by Oprofile userspace. Here primarily for documentation

@@ -367,6 +358,8 @@ static irqreturn_t xscale_pmu_interrupt(int irq, void *arg)
 	return IRQ_HANDLED;
 }
 
+static const struct pmu_irqs *pmu_irqs;
+
 static void xscale_pmu_stop(void)
 {
 	u32 pmnc = read_pmnc();

@@ -374,20 +367,30 @@ static void xscale_pmu_stop(void)
 	pmnc &= ~PMU_ENABLE;
 	write_pmnc(pmnc);
 
-	free_irq(XSCALE_PMU_IRQ, results);
+	free_irq(pmu_irqs->irqs[0], results);
+	release_pmu(pmu_irqs);
+	pmu_irqs = NULL;
 }
 
 static int xscale_pmu_start(void)
 {
 	int ret;
-	u32 pmnc = read_pmnc();
+	u32 pmnc;
+
+	pmu_irqs = reserve_pmu();
+	if (IS_ERR(pmu_irqs))
+		return PTR_ERR(pmu_irqs);
+
+	pmnc = read_pmnc();
 
-	ret = request_irq(XSCALE_PMU_IRQ, xscale_pmu_interrupt, IRQF_DISABLED,
-			"XScale PMU", (void *)results);
+	ret = request_irq(pmu_irqs->irqs[0], xscale_pmu_interrupt,
+			  IRQF_DISABLED, "XScale PMU", (void *)results);
 
 	if (ret < 0) {
 		printk(KERN_ERR "oprofile: unable to request IRQ%d for XScale PMU\n",
-			XSCALE_PMU_IRQ);
+		       pmu_irqs->irqs[0]);
+		release_pmu(pmu_irqs);
+		pmu_irqs = NULL;
 		return ret;
 	}
 

1.6.5.4

perf events require that we can support atomic64's. There is a generic, spinlocked version that we can use until we have proper hardware support.

---
 arch/arm/Kconfig              |    1 +
 arch/arm/include/asm/atomic.h |    4 ++++
 2 files changed, 5 insertions(+), 0 deletions(-)

index 9e08891..0dbfd9b 100644
@@ -18,6 +18,7 @@ config ARM
 	select HAVE_KRETPROBES if (HAVE_KPROBES)
 	select HAVE_FUNCTION_TRACER if (!XIP_KERNEL)
 	select HAVE_GENERIC_DMA_COHERENT
+	select GENERIC_ATOMIC64
 	help
 	  The ARM series is a line of low-power-consumption RISC chip designs
 	  licensed by ARM Ltd and targeted at embedded applications and

index d0daeab..ff286a8 100644
@@ -15,6 +15,10 @@
 #include <linux/types.h>
 #include <asm/system.h>
 
+#ifdef CONFIG_GENERIC_ATOMIC64
+#include <asm-generic/atomic64.h>
+#endif
+
 #define ATOMIC_INIT(i)	{ (i) }
 
 #ifdef __KERNEL__

1.6.5.4

The perf events subsystem allows counting of both hardware and software events. This patch implements the bare minimum for software performance events.

---
 arch/arm/Kconfig                  |    2 +
 arch/arm/include/asm/perf_event.h |   38 +++++++++++++++++++++++++++++++++++++
 arch/arm/mm/fault.c               |    7 ++++++
 3 files changed, 47 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/include/asm/perf_event.h

index 0dbfd9b..337eec4 100644
@@ -19,6 +19,8 @@ config ARM
 	select HAVE_FUNCTION_TRACER if (!XIP_KERNEL)
 	select HAVE_GENERIC_DMA_COHERENT
 	select GENERIC_ATOMIC64
+	select HAVE_PERF_EVENTS
+	select PERF_USE_VMALLOC
 	help
 	  The ARM series is a line of low-power-consumption RISC chip designs
 	  licensed by ARM Ltd and targeted at embedded applications and

new file mode 100644
index 0000000..32a66ac
@@ -0,0 +1,38 @@
+/*
+ *  linux/arch/arm/include/asm/perf_event.h
+ *
+ *  Copyright (C) 2009 picoChip Designs Ltd, Jamie Iles
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#ifndef __ARM_PERF_EVENT_H__
+#define __ARM_PERF_EVENT_H__
+
+/*
+ * NOP: on *most* (read: all supported) ARM platforms, the performance
+ * counter interrupts are regular interrupts and not an NMI. This
+ * means that when we receive the interrupt we can call
+ * perf_event_do_pending() that handles all of the work with
+ * interrupts enabled.
+ */
+static inline void
+set_perf_event_pending(void)
+{
+}
+
+/* Get the PC. Make sure that we have a 64bit value with the upper 32 cleared.
+ */
+#define perf_instruction_pointer(_regs) \
+	((u64)instruction_pointer(regs) & 0xFFFFFFFFLU)
+#define perf_misc_flags(regs)   (user_mode(regs) ? PERF_RECORD_MISC_USER : \
+                                 PERF_RECORD_MISC_KERNEL)
+
+/* ARM performance counters start from 1 (in the cp15 accesses) so use the
+ * same indexes here for consistency. */
+#define PERF_EVENT_INDEX_OFFSET 1
+
+#endif /* __ARM_PERF_EVENT_H__ */

index 10e0680..9d40c34 100644
@@ -18,6 +18,7 @@
 #include <linux/page-flags.h>
 #include <linux/sched.h>
 #include <linux/highmem.h>
+#include <linux/perf_event.h>
 
 #include <asm/system.h>
 #include <asm/pgtable.h>

@@ -302,6 +303,12 @@ do_page_fault(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 	fault = __do_page_fault(mm, addr, fsr, tsk);
 	up_read(&mm->mmap_sem);
 
+	perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS, 1, 0, regs, addr);
+	if (fault & VM_FAULT_MAJOR)
+		perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS_MAJ, 1, 0, regs, addr);
+	else if (fault & VM_FAULT_MINOR)
+		perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS_MIN, 1, 0, regs, addr);
+
 	/*
 	 * Handle the "normal" case first - VM_FAULT_MAJOR / VM_FAULT_MINOR
 	 */

1.6.5.4

This patch implements support for ARMv6 performance counters in the Linux performance events subsystem. ARMv6 architectures that have the performance counters should enable HW_PERF_EVENTS and define the interrupts for the counters in arch/arm/kernel/perf_event.c

This implementation also provides an ARM PMU abstraction layer to allow ARMv7 and others to be supported in the future by adding new a 'struct arm_pmu'.

---
 arch/arm/Kconfig             |    8 +
 arch/arm/kernel/Makefile     |    1 +
 arch/arm/kernel/perf_event.c | 1338 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 1347 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/kernel/perf_event.c

index 337eec4..53c3fde 100644
@@ -1175,6 +1175,14 @@ config HIGHPTE
 	depends on HIGHMEM
 	depends on !OUTER_CACHE
 
+config HW_PERF_EVENTS
+	bool "Enable hardware performance counter support for perf events"
+	depends on PERF_EVENTS && CPU_HAS_PMU && CPU_V6
+	default y
+	help
+	  Enable hardware performance counter support for perf events. If
+	  disabled, perf events will use software events only.
+
 source "mm/Kconfig"
 
 config LEDS

index 216890d..c76e6d2 100644
@@ -47,6 +47,7 @@ obj-$(CONFIG_CPU_XSC3)		+= xscale-cp0.o
 obj-$(CONFIG_CPU_MOHAWK)	+= xscale-cp0.o
 obj-$(CONFIG_IWMMXT)		+= iwmmxt.o
 obj-$(CONFIG_CPU_HAS_PMU)	+= pmu.o
+obj-$(CONFIG_HW_PERF_EVENTS)	+= perf_event.o
 AFLAGS_iwmmxt.o			:= -Wa,-mcpu=iwmmxt
 
 ifneq ($(CONFIG_ARCH_EBSA110),y)

1.6.5.4

Only comment is the number of BUG()s in this code - I count 6 of them. BUG() is not meant to be used as an "assert" for when expectations are not met - does it really make sense to crash the kernel when any of these are hit? Are you at risk of losing data if one of your conditions isn't satisfied?

You may want to consider using WARN_ON() instead - at least that doesn't take out the kernel.

Ok, good point. I've replaced these with WARN_ONCE()'s so we don't get too much noise with repeated accesses.

Jamie

Hi Jamie,

I've been trying to test your patches with a quad-core ARM 11MPCore on a Realview PB11MP board.

Unfortunately, I occasionally experience a complete system hang during some profiling runs. I don't think it's your fault however, as it can occur even when monitoring only software events. I've managed to reproduce this on the tip/master branch and got the following information [I enabled lock debugging]:

=================================
[ INFO: inconsistent lock state ]
2.6.33-rc2-tip+ #5
---------------------------------
inconsistent {IN-HARDIRQ-W} -> {HARDIRQ-ON-W} usage. perf/1463 [HC0[0]:SC0[0]:HE1:SE1] takes:
(&ctx->lock){?.....}, at: [<c009e538>] __perf_event_sched_in+0x20/0x168
{IN-HARDIRQ-W} state was registered at:

  [<c00718b0>] __lock_acquire+0x5c8/0x17b4
  [<c0072b70>] lock_acquire+0xd4/0xec
  [<c0315700>] _raw_spin_lock+0x2c/0x3c
  [<c00a20f8>] perf_ctx_adjust_freq+0xc/0x1dc
  [<c00a230c>] perf_event_task_tick+0x44/0xb4
  [<c0041f4c>] scheduler_tick+0xec/0x144
  [<c0053eb8>] update_process_times+0x40/0x4c
  [<c006d91c>] tick_periodic+0xdc/0x108
  [<c006d96c>] tick_handle_periodic+0x24/0xf0
  [<c0030124>] ipi_timer+0x34/0x44
  [<c002a3cc>] do_local_timer+0x50/0x80
  [<c002aca4>] __irq_svc+0x44/0xe0
  [<c002c388>] default_idle+0x28/0x2c
  [<c002c8ac>] cpu_idle+0x8c/0xe4
  [<70008080>] 0x70008080
irq event stamp: 454
hardirqs last  enabled at (454): [<c0315d30>] _raw_spin_unlock_irq+0x24/0x2c
hardirqs last disabled at (453): [<c0315784>] _raw_spin_lock_irq+0x18/0x50
softirqs last  enabled at (0): [<c0045650>] copy_process+0x328/0xf70
softirqs last disabled at (0): [<(null)>] (null)

other info that might help us debug this:
no locks held by perf/1463.

stack backtrace:
[<c0031694>] (unwind_backtrace+0x0/0xd4) from [<c006ff88>] (print_usage_bug+0x16c/0x1ac)
[<c006ff88>] (print_usage_bug+0x16c/0x1ac) from [<c00702f0>] (mark_lock+0x328/0x5f0)
[<c00702f0>] (mark_lock+0x328/0x5f0) from [<c007193c>] (__lock_acquire+0x654/0x17b4)
[<c007193c>] (__lock_acquire+0x654/0x17b4) from [<c0072b70>] (lock_acquire+0xd4/0xec)
[<c0072b70>] (lock_acquire+0xd4/0xec) from [<c0315700>] (_raw_spin_lock+0x2c/0x3c)
[<c0315700>] (_raw_spin_lock+0x2c/0x3c) from [<c009e538>] (__perf_event_sched_in+0x20/0x168)
[<c009e538>] (__perf_event_sched_in+0x20/0x168) from [<c009e6c8>] (perf_event_task_sched_in+0x48/0x58)
[<c009e6c8>] (perf_event_task_sched_in+0x48/0x58) from [<c003f2dc>] (finish_task_switch+0x34/0xb4)
[<c003f2dc>] (finish_task_switch+0x34/0xb4) from [<c03133e0>] (schedule+0x728/0x834)
[<c03133e0>] (schedule+0x728/0x834) from [<c00d5190>] (pipe_wait+0x64/0x84)
[<c00d5190>] (pipe_wait+0x64/0x84) from [<c00d59d8>] (pipe_read+0x3ac/0x428)
[<c00d59d8>] (pipe_read+0x3ac/0x428) from [<c00cdd34>] (do_sync_read+0x94/0xe0)
[<c00cdd34>] (do_sync_read+0x94/0xe0) from [<c00ce868>] (vfs_read+0xa8/0x150)
[<c00ce868>] (vfs_read+0xa8/0x150) from [<c00ce9bc>] (sys_read+0x3c/0x68)
[<c00ce9bc>] (sys_read+0x3c/0x68) from [<c002b140>] (ret_fast_syscall+0x0/0x38)

This occurs more frequently when profiling code that spawns child processes [e.g. gcc] but I've see it happen with sleep too. It also happens if I pass maxcpus=1 as a bootarg.

Have you seen this happen on your board? I've failed to reproduce it on my x86 machine.

Will

<truncated>

Hi Will, Jamie,

This is the exact same problem as I have when stressing the system a little bit.

Jean

Hi Will,

Could it be to do with the fact that perf_event_task_sched_in() expects interrupts to be disabled but on ARM we have __ARCH_WANT_INTERRUPTS_ON_CTXSW defined and therefore run with interrupts enabled? If so, I'm not sure what the fix is!

At the moment, ARM is the only platform that context switches with interrupts enabled and has perf event support.

Jamie

If perf event support is only safe with interrupts disabled, it should disable them. Maybe a patch to do that conditional on __ARCH_WANT_INTERRUPTS_ON_CTXSW would be more acceptable than an unconditional one - don't know.

We could only define __ARCH_WANT_INTERRUPTS_ON_CTXSW for VIVT supporting kernels, which is the reason for it existing (the interrupt latency for VIVT would otherwise be unacceptable.) This approach would mean that perf events wouldn't be usable on VIVT CPUs (which includes Xscale CPUs.)

Ok, I've tried 2 things:
1. disabling interrupts around perf_event_task_sched_in()
2. undefining __ARCH_WANT_INTERRUPTS_ON_CTXSW

As far as I can tell, both of these solutions work, although with 2, I had to define __ARCH_WANT_INTERRUPTS_ON_CTXSW.

Will, Jean - could you give the patch below a go and see if it works on your systems? I don't get any lockdep warnings on my platform with this and it still runs without the lock debugging.

It's not a nice patch but at least perf events could be used on all ARM platforms. Also, I guess that this could be a local_irq_{disable,enable} pair without the need of saving the flags when we know interrupts are enabled.

Thanks,

Jamie

index 918f343..f110994 100644
@@ -2767,6 +2767,9 @@ static void finish_task_switch(struct rq *rq, struct task_struct *prev)
 {
 	struct mm_struct *mm = rq->prev_mm;
 	long prev_state;
+#ifdef __ARCH_WANT_INTERRUPTS_ON_CTXSW
+	unsigned long flags;
+#endif /* __ARCH_WANT_INTERRUPTS_ON_CTXSW */
 
 	rq->prev_mm = NULL;
 

@@ -2783,7 +2786,14 @@ static void finish_task_switch(struct rq *rq, struct task_struct *prev)
 	 */
 	prev_state = prev->state;
 	finish_arch_switch(prev);
+#ifdef __ARCH_WANT_INTERRUPTS_ON_CTXSW
+	local_irq_save(flags);
 	perf_event_task_sched_in(current);
+	local_irq_restore(flags);
+#else /* __ARCH_WANT_INTERRUPTS_ON_CTXSW */
+	perf_event_task_sched_in(current);
+#endif /* __ARCH_WANT_INTERRUPTS_ON_CTXSW */
+
 	finish_lock_switch(rq, prev);
 
 	fire_sched_in_preempt_notifiers(current);

Hi Jamie,

I don't follow what you mean for point (2) when you say you have to define __ARCH_WANT_INTERRUPTS_ON_CTXSW. I tried defining __ARCH_WANT_INTERRUPTS_ON_CTXSW only when VIVT caches are present [as Russell mentioned], but I encountered further locking problems with __new_context [see below].

This patch solves the issue for me. Should this be integrated into your patchset as that is the first perf code for ARM?

Cheers,

Will

======================================================
[ INFO: HARDIRQ-safe -> HARDIRQ-unsafe lock order detected ] 2.6.33-rc2-tip+ #1
------------------------------------------------------
swapper/0 [HC0[0]:SC0[0]:HE0:SE1] is trying to acquire:
(cpu_asid_lock){+.+...}, at: [<c0035c14>] __new_context+0x14/0xc4

and this task is already holding:
(&rq->lock){-.-.-.}, at: [<c030c948>] schedule+0xa8/0x834
which would create a new lock dependency:
(&rq->lock){-.-.-.} -> (cpu_asid_lock){+.+...}

but this new dependency connects a HARDIRQ-irq-safe lock:
(&rq->lock){-.-.-.}
... which became HARDIRQ-irq-safe at:

  [<c0076074>] __lock_acquire+0x5c8/0x17b4
  [<c0077334>] lock_acquire+0xd4/0xec
  [<c030f2e8>] _raw_spin_lock+0x2c/0x3c
  [<c004665c>] scheduler_tick+0x34/0x144
  [<c0058670>] update_process_times+0x40/0x4c
  [<c00720e0>] tick_periodic+0xdc/0x108
  [<c0072130>] tick_handle_periodic+0x24/0xf0
  [<c0036e20>] realview_timer_interrupt+0x24/0x34
  [<c008aa30>] handle_IRQ_event+0x5c/0x144
  [<c008c848>] handle_level_irq+0xc0/0x134
  [<c002a084>] asm_do_IRQ+0x84/0xc0
  [<c002aca4>] __irq_svc+0x44/0xe0
  [<c0309c64>] calibrate_delay+0x84/0x1ac
  [<c0008be0>] start_kernel+0x224/0x2c8
  [<70008080>] 0x70008080

to a HARDIRQ-irq-unsafe lock:
(cpu_asid_lock){+.+...}
... which became HARDIRQ-irq-unsafe at:
... [<c0076100>] __lock_acquire+0x654/0x17b4

  [<c0077334>] lock_acquire+0xd4/0xec
  [<c030f2e8>] _raw_spin_lock+0x2c/0x3c
  [<c0035c14>] __new_context+0x14/0xc4
  [<c00d7d74>] flush_old_exec+0x3b8/0x75c
  [<c0109fa8>] load_elf_binary+0x340/0x1288
  [<c00d7500>] search_binary_handler+0x130/0x320
  [<c00d8bcc>] do_execve+0x1c0/0x2d4
  [<c002e558>] kernel_execve+0x34/0x84
  [<c002a7ac>] init_post+0xc0/0x110
  [<c0008730>] kernel_init+0x1b8/0x208
  [<c002c2ec>] kernel_thread_exit+0x0/0x8

other info that might help us debug this:

1 lock held by swapper/0:
#0: (&rq->lock){-.-.-.}, at: [<c030c948>] schedule+0xa8/0x834

the dependencies between HARDIRQ-irq-safe lock and the holding lock:
-> (&rq->lock){-.-.-.} ops: 0 {
IN-HARDIRQ-W at:

 [<c0076074>] __lock_acquire+0x5c8/0x17b4
                        [<c0077334>] lock_acquire+0xd4/0xec
                        [<c030f2e8>] _raw_spin_lock+0x2c/0x3c
                        [<c004665c>] scheduler_tick+0x34/0x144
                        [<c0058670>] update_process_times+0x40/0x4c
                        [<c00720e0>] tick_periodic+0xdc/0x108
                        [<c0072130>] tick_handle_periodic+0x24/0xf0
                        [<c0036e20>] realview_timer_interrupt+0x24/0x34
                        [<c008aa30>] handle_IRQ_event+0x5c/0x144
                        [<c008c848>] handle_level_irq+0xc0/0x134
                        [<c002a084>] asm_do_IRQ+0x84/0xc0
                        [<c002aca4>] __irq_svc+0x44/0xe0
                        [<c0309c64>] calibrate_delay+0x84/0x1ac
                        [<c0008be0>] start_kernel+0x224/0x2c8
                        [<70008080>] 0x70008080
   IN-SOFTIRQ-W at:  
                        [<c0076098>] __lock_acquire+0x5ec/0x17b4
                        [<c0077334>] lock_acquire+0xd4/0xec
                        [<c030f2e8>] _raw_spin_lock+0x2c/0x3c
                        [<c004492c>] double_rq_lock+0x40/0x84
                        [<c0045c90>] run_rebalance_domains+0x208/0x510
                        [<c0051510>] __do_softirq+0xe8/0x1e4
                        [<c002a3cc>] do_local_timer+0x50/0x80
                        [<c002aca4>] __irq_svc+0x44/0xe0
                        [<c002c388>] default_idle+0x28/0x2c
                        [<c002c8ac>] cpu_idle+0x8c/0xe4
                        [<c0008c28>] start_kernel+0x26c/0x2c8
                        [<70008080>] 0x70008080
   IN-RECLAIM_FS-W at:
                           [<c0076170>] __lock_acquire+0x6c4/0x17b4
                           [<c0077334>] lock_acquire+0xd4/0xec
                           [<c030f2e8>] _raw_spin_lock+0x2c/0x3c
                           [<c003f35c>] task_rq_lock+0x40/0x78
                           [<c0045fc8>] set_cpus_allowed_ptr+0x30/0x1bc
                           [<c00b426c>] kswapd+0x78/0x620
                           [<c0066a5c>] kthread+0x7c/0x84
                           [<c002c2ec>] kernel_thread_exit+0x0/0x8
   INITIAL USE at:   
                       [<c0076188>] __lock_acquire+0x6dc/0x17b4
                       [<c0077334>] lock_acquire+0xd4/0xec
                       [<c030f3e4>] _raw_spin_lock_irqsave+0x40/0x54
                       [<c004302c>] rq_attach_root+0x14/0x10c
                       [<c000c780>] sched_init+0x234/0x35c
                       [<c0008b28>] start_kernel+0x16c/0x2c8
                       [<70008080>] 0x70008080
 }
 ... key      at: [<c044ef3c>] __key.45524+0x0/0x8
 ... acquired at:
   [<c0075a4c>] check_irq_usage+0x58/0xb8
   [<c0076b54>] __lock_acquire+0x10a8/0x17b4
   [<c0077334>] lock_acquire+0xd4/0xec
   [<c030f2e8>] _raw_spin_lock+0x2c/0x3c
   [<c0035c14>] __new_context+0x14/0xc4
   [<c030cf50>] schedule+0x6b0/0x834
   [<c002c8ec>] cpu_idle+0xcc/0xe4
   [<70008080>] 0x70008080

the dependencies between the lock to be acquired and HARDIRQ-irq-unsafe lock:
-> (cpu_asid_lock){+.+...} ops: 0 {
HARDIRQ-ON-W at:

                        [<c0076100>] __lock_acquire+0x654/0x17b4
                        [<c0077334>] lock_acquire+0xd4/0xec
                        [<c030f2e8>] _raw_spin_lock+0x2c/0x3c
                        [<c0035c14>] __new_context+0x14/0xc4
                        [<c00d7d74>] flush_old_exec+0x3b8/0x75c
                        [<c0109fa8>] load_elf_binary+0x340/0x1288
                        [<c00d7500>] search_binary_handler+0x130/0x320
                        [<c00d8bcc>] do_execve+0x1c0/0x2d4
                        [<c002e558>] kernel_execve+0x34/0x84
                        [<c002a7ac>] init_post+0xc0/0x110
                        [<c0008730>] kernel_init+0x1b8/0x208
                        [<c002c2ec>] kernel_thread_exit+0x0/0x8
   SOFTIRQ-ON-W at:  
                        [<c0076124>] __lock_acquire+0x678/0x17b4
                        [<c0077334>] lock_acquire+0xd4/0xec
                        [<c030f2e8>] _raw_spin_lock+0x2c/0x3c
                        [<c0035c14>] __new_context+0x14/0xc4
                        [<c00d7d74>] flush_old_exec+0x3b8/0x75c
                        [<c0109fa8>] load_elf_binary+0x340/0x1288
                        [<c00d7500>] search_binary_handler+0x130/0x320
                        [<c00d8bcc>] do_execve+0x1c0/0x2d4
                        [<c002e558>] kernel_execve+0x34/0x84
                        [<c002a7ac>] init_post+0xc0/0x110
                        [<c0008730>] kernel_init+0x1b8/0x208
                        [<c002c2ec>] kernel_thread_exit+0x0/0x8
   INITIAL USE at:   
                       [<c0076188>] __lock_acquire+0x6dc/0x17b4
                       [<c0077334>] lock_acquire+0xd4/0xec
                       [<c030f2e8>] _raw_spin_lock+0x2c/0x3c
                       [<c0035c14>] __new_context+0x14/0xc4
                       [<c00d7d74>] flush_old_exec+0x3b8/0x75c
                       [<c0109fa8>] load_elf_binary+0x340/0x1288
                       [<c00d7500>] search_binary_handler+0x130/0x320
                       [<c00d8bcc>] do_execve+0x1c0/0x2d4
                       [<c002e558>] kernel_execve+0x34/0x84
                       [<c002a7ac>] init_post+0xc0/0x110
                       [<c0008730>] kernel_init+0x1b8/0x208
                       [<c002c2ec>] kernel_thread_exit+0x0/0x8
 }
 ... key      at: [<c042c57c>] cpu_asid_lock+0x10/0x1c
 ... acquired at:
   [<c0075a4c>] check_irq_usage+0x58/0xb8
   [<c0076b54>] __lock_acquire+0x10a8/0x17b4
   [<c0077334>] lock_acquire+0xd4/0xec
   [<c030f2e8>] _raw_spin_lock+0x2c/0x3c
   [<c0035c14>] __new_context+0x14/0xc4
   [<c030cf50>] schedule+0x6b0/0x834
   [<c002c8ec>] cpu_idle+0xcc/0xe4
 [<70008080>] 0x70008080


stack backtrace:
[<c0031760>] (unwind_backtrace+0x0/0xd4) from [<c0075984>] (check_usage+0x3f0/0x460)
[<c0075984>] (check_usage+0x3f0/0x460) from [<c0075a4c>] (check_irq_usage+0x58/0xb8)
[<c0075a4c>] (check_irq_usage+0x58/0xb8) from [<c0076b54>] (__lock_acquire+0x10a8/0x17b4)
[<c0076b54>] (__lock_acquire+0x10a8/0x17b4) from [<c0077334>] (lock_acquire+0xd4/0xec)
[<c0077334>] (lock_acquire+0xd4/0xec) from [<c030f2e8>] (_raw_spin_lock+0x2c/0x3c)
[<c030f2e8>] (_raw_spin_lock+0x2c/0x3c) from [<c0035c14>] (__new_context+0x14/0xc4)
[<c0035c14>] (__new_context+0x14/0xc4) from [<c030cf50>] (schedule+0x6b0/0x834)
[<c030cf50>] (schedule+0x6b0/0x834) from [<c002c8ec>] (cpu_idle+0xcc/0xe4)
[<c002c8ec>] (cpu_idle+0xcc/0xe4) from [<70008080>] (0x70008080)

I got my define's mixed up. If you undef __ARCH_WANT_INTERRUPTS_ON_CTXSW then you also need to define __ARCH_WANT_UNLOCKED_CTXSW to avoid the locking problems you've described.

As long as no-one else has any objection.

Cheers,

Jamie

Ok, how about this?

Jamie

[PATCH] arm ptrace: use unsigned types for kernel pt_regs

Make registers unsigned for kernel space. This is important for example in the perf events where the PC is stored into a u64. We don't want it sign extended so make the regs unsigned to prevent casts throughout the kernel.

---
 arch/arm/include/asm/ptrace.h |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

index bbecccd..eec6e89 100644
@@ -97,9 +97,15 @@
  * stack during a system call.  Note that sizeof(struct pt_regs)
  * has to be a multiple of 8.
  */
+#ifndef __KERNEL__
 struct pt_regs {
 	long uregs[18];
 };
+#else /* __KERNEL__ */
+struct pt_regs {
+	unsigned long uregs[18];
+};
+#endif /* __KERNEL__ */
 
 #define ARM_cpsr	uregs[16]
 #define ARM_pc		uregs[15]

1.6.5.4

Perfect.

Is that simply waiting for LDREXD/STREXD-based atomic64's, or is it depending on something subtler?

-- Jamie

Yes, Will Deacon submitted a patch last month that does this but afaik it hasn't been merged in yet.

Jamie

[...]

[...]

[...]

[...]

[...]

Best regards,                                           _     _
  .o. | Liege of Serenely Enlightened Majesty of       o' \,=./ `o
  ..o | Computer Science,  Micha? "mina86" Nazarewicz     (o o)
  ooo +---<[email protected]>---<[email protected]>---ooO--(_)--Ooo--

Thanks, well spotted!