ARM: oprofile: Always allow backtraces

PATCH In review Ari Kauppi 4 participants 10 messages Jan 19, 2011 – Jan 20, 2011 Arm (32-bit)

lore

Share & tools

lore

Ari Kauppi

author

2011-01-19 20:54 UTC #0 lore raw

PATCH 1/1 ARM: oprofile: Always allow backtraces

Always allow backtrace when using oprofile on ARM, even if a PMU isn't present.

Restores functionality originally introduced in 1b7b56982fdcd9d85effd76f3928cf5d6eb26155 by Richard Purdie.

Signed-off-by Ari Kauppi <[email protected]>

Cc Richard Purdie <[email protected]>

Cc Will Deacon <[email protected]>

Cc Matt Fleming <[email protected]>

---
 arch/arm/oprofile/common.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

arch/arm/oprofile/common.c +4 -0

index 8aa9744..a23e5ae 100644
@@ -46,6 +46,7 @@ char *op_name_from_perf_id(void)
 		return NULL;
 	}
 }
+#endif
 
 static int report_trace(struct stackframe *frame, void *d)
 {

@@ -109,6 +110,7 @@ static void arm_backtrace(struct pt_regs * const regs, unsigned int depth)
 		tail = user_backtrace(tail);
 }
 
+#ifdef CONFIG_HW_PERF_EVENTS
 int __init oprofile_arch_init(struct oprofile_operations *ops)
 {
 	ops->backtrace		= arm_backtrace;

@@ -124,6 +126,8 @@ void __exit oprofile_arch_exit(void)
 int __init oprofile_arch_init(struct oprofile_operations *ops)
 {
 	pr_info("oprofile: hardware counters not available\n");
+	ops->backtrace		= arm_backtrace;
+
 	return -ENODEV;
 }
 void __exit oprofile_arch_exit(void) {}

Signature · 1 line

1.7.4.rc2.3.g60a2e

Will Deacon

2011-01-20 09:42 UTC #1 lore raw

PATCH 1/1 ARM: oprofile: Always allow backtraces

Hi Ari,

Quoted 5 lines

Always allow backtrace when using oprofile on ARM, even if a PMU isn't present.
Restores functionality originally introduced in 1b7b56982fdcd9d85effd76f3928cf5d6eb26155 by Richard Purdie.

Well spotted, I'd forgotten we still want this for timer mode. Comments inline.

Quoted 13 lines

arch/arm/oprofile/common.c +1 -0

index 8aa9744..a23e5ae 100644
@@ -46,6 +46,7 @@ char *op_name_from_perf_id(void)
 		return NULL;
 	}
 }
+#endif
 
 static int report_trace(struct stackframe *frame, void *d)
 {

arch/arm/oprofile/common.c static void arm_backtrace(struct pt_regs * const regs, unsigned int depth) · line 113

Quoted 4 lines

arch/arm/oprofile/common.c +1 -0

@@ -109,6 +110,7 @@ static void arm_backtrace(struct pt_regs * const regs, unsigned int depth)
 		tail = user_backtrace(tail);
 }
 
+#ifdef CONFIG_HW_PERF_EVENTS

Can you reorder the backtrace stuff so that we don't need the extra #endif/#ifdef please?

arch/arm/oprofile/common.c void __exit oprofile_arch_exit(void) · line 133

Quoted 12 lines

arch/arm/oprofile/common.c +2 -0

 int __init oprofile_arch_init(struct oprofile_operations *ops)
 {
 	ops->backtrace		= arm_backtrace;

@@ -124,6 +126,8 @@ void __exit oprofile_arch_exit(void)
 int __init oprofile_arch_init(struct oprofile_operations *ops)
 {
 	pr_info("oprofile: hardware counters not available\n");
+	ops->backtrace		= arm_backtrace;
+
 	return -ENODEV;
 }
 void __exit oprofile_arch_exit(void) {}

One thing worth mentioning is that the backtrace code relies on a sensible frame pointer, which you might not have in ARM code and you certainly won't have for Thumb-2 code.

Will

Robert Richter

2011-01-20 09:52 UTC #2 lore raw

PATCH 1/1 ARM: oprofile: Always allow backtraces

Quoted 27 lines · Will Deacon

Hi Ari,
Quoted 5 lines
Always allow backtrace when using oprofile on ARM, even if a PMU isn't present.
Restores functionality originally introduced in 1b7b56982fdcd9d85effd76f3928cf5d6eb26155 by Richard Purdie.
Well spotted, I'd forgotten we still want this for timer mode. Comments inline.
Quoted 16 lines
arch/arm/oprofile/common.c +1 -0
index 8aa9744..a23e5ae 100644
@@ -46,6 +46,7 @@ char *op_name_from_perf_id(void)
 		return NULL;
 	}
 }
+#endif
 
 static int report_trace(struct stackframe *frame, void *d)
 {
@@ -109,6 +110,7 @@ static void arm_backtrace(struct pt_regs * const regs, unsigned int depth)
 		tail = user_backtrace(tail);
 }
 

Quoted 4 lines

arch/arm/oprofile/common.c static void arm_backtrace(struct pt_regs * const regs, unsigned int depth) · line 113
Quoted 1 line
arch/arm/oprofile/common.c +1 -0
+#ifdef CONFIG_HW_PERF_EVENTS
Can you reorder the backtrace stuff so that we don't need the extra #endif/#ifdef please?

I would like to go even further, see the diff below.

-Robert

---
 arch/arm/oprofile/common.c |   45 ++++++++++++++++++++++++-------------------
 1 files changed, 25 insertions(+), 20 deletions(-)

arch/arm/oprofile/common.c +25 ~~-20~~

index 8aa9744..916aa2e 100644
@@ -47,18 +47,6 @@ char *op_name_from_perf_id(void)
 	}
 }
 
-static int report_trace(struct stackframe *frame, void *d)
-{
-	unsigned int *depth = d;
-
-	if (*depth) {
-		oprofile_add_trace(frame->pc);
-		(*depth)--;
-	}
-
-	return *depth == 0;
-}
-
 /*
  * The registers we're interested in are at the end of the variable
  * length saved register structure. The fp points at the end of this

@@ -91,6 +79,30 @@ static struct frame_tail* user_backtrace(struct frame_tail *tail)
 	return buftail[0].fp-1;
 }
 
+#else
+
+static int oprofile_arch_init(struct oprofile_operations *ops)
+{
+	pr_info("oprofile: hardware counters not available\n");
+	return -ENODEV;
+}
+
+static void oprofile_arch_exit(void) { }
+
+#endif /* CONFIG_HW_PERF_EVENTS */
+
+static int report_trace(struct stackframe *frame, void *d)
+{
+	unsigned int *depth = d;
+
+	if (*depth) {
+		oprofile_add_trace(frame->pc);
+		(*depth)--;
+	}
+
+	return *depth == 0;
+}
+
 static void arm_backtrace(struct pt_regs * const regs, unsigned int depth)
 {
 	struct frame_tail *tail = ((struct frame_tail *) regs->ARM_fp) - 1;

@@ -111,6 +123,7 @@ static void arm_backtrace(struct pt_regs * const regs, unsigned int depth)
 
 int __init oprofile_arch_init(struct oprofile_operations *ops)
 {
+	/* provide backtrace support also in timer mode: */
 	ops->backtrace		= arm_backtrace;
 
 	return oprofile_perf_init(ops);

@@ -120,11 +133,3 @@ void __exit oprofile_arch_exit(void)
 {
 	oprofile_perf_exit();
 }
-#else
-int __init oprofile_arch_init(struct oprofile_operations *ops)
-{
-	pr_info("oprofile: hardware counters not available\n");
-	return -ENODEV;
-}
-void __exit oprofile_arch_exit(void) {}
-#endif /* CONFIG_HW_PERF_EVENTS */

Signature · 7 lines

1.7.3.4



-- 
Advanced Micro Devices, Inc.
Operating System Research Center

Robert Richter

2011-01-20 09:57 UTC #3 lore raw

PATCH 1/1 ARM: oprofile: Always allow backtraces

Quoted 32 lines · Robert Richter

I would like to go even further, see the diff below.

(quoted diff) +0 -2

-Robert
 
---
 arch/arm/oprofile/common.c |   45 ++++++++++++++++++++++++-------------------
 1 files changed, 25 insertions(+), 20 deletions(-)

arch/arm/oprofile/common.c +0 ~~-12~~

index 8aa9744..916aa2e 100644
@@ -47,18 +47,6 @@ char *op_name_from_perf_id(void)
 	}
 }
 
-static int report_trace(struct stackframe *frame, void *d)
-{
-	unsigned int *depth = d;
-
-	if (*depth) {
-		oprofile_add_trace(frame->pc);
-		(*depth)--;
-	}
-
-	return *depth == 0;
-}
-
 /*
  * The registers we're interested in are at the end of the variable
  * length saved register structure. The fp points at the end of this

arch/arm/oprofile/common.c static struct frame_tail* user_backtrace(struct frame_tail *tail) · line 84

Quoted 6 lines

arch/arm/oprofile/common.c +3 -0

@@ -91,6 +79,30 @@ static struct frame_tail* user_backtrace(struct frame_tail *tail)
 	return buftail[0].fp-1;
 }
 
+#else
+
+static int oprofile_arch_init(struct oprofile_operations *ops)

Of course it should compile, but in this sense...

-Robert

Quoted 44 lines

arch/arm/oprofile/common.c +22 -8

+{
+	pr_info("oprofile: hardware counters not available\n");
+	return -ENODEV;
+}
+
+static void oprofile_arch_exit(void) { }
+
+#endif /* CONFIG_HW_PERF_EVENTS */
+
+static int report_trace(struct stackframe *frame, void *d)
+{
+	unsigned int *depth = d;
+
+	if (*depth) {
+		oprofile_add_trace(frame->pc);
+		(*depth)--;
+	}
+
+	return *depth == 0;
+}
+
 static void arm_backtrace(struct pt_regs * const regs, unsigned int depth)
 {
 	struct frame_tail *tail = ((struct frame_tail *) regs->ARM_fp) - 1;

@@ -111,6 +123,7 @@ static void arm_backtrace(struct pt_regs * const regs, unsigned int depth)
 
 int __init oprofile_arch_init(struct oprofile_operations *ops)
 {
+	/* provide backtrace support also in timer mode: */
 	ops->backtrace		= arm_backtrace;
 
 	return oprofile_perf_init(ops);

@@ -120,11 +133,3 @@ void __exit oprofile_arch_exit(void)
 {
 	oprofile_perf_exit();
 }
-#else
-int __init oprofile_arch_init(struct oprofile_operations *ops)
-{
-	pr_info("oprofile: hardware counters not available\n");
-	return -ENODEV;
-}
-void __exit oprofile_arch_exit(void) {}
-#endif /* CONFIG_HW_PERF_EVENTS */

Signature · 2 lines

Advanced Micro Devices, Inc.
Operating System Research Center

Will Deacon

2011-01-20 10:02 UTC #4 lore raw

PATCH 1/1 ARM: oprofile: Always allow backtraces

In reply to Robert Richter

Hi Robert,

Quoted 1 line

I would like to go even further, see the diff below.

[...]

arch/arm/oprofile/common.c static struct frame_tail* user_backtrace(struct frame_tail *tail) · line 92

Quoted 11 lines

arch/arm/oprofile/common.c +11 -0

+#else
+
+static int oprofile_arch_init(struct oprofile_operations *ops)
+{
+	pr_info("oprofile: hardware counters not available\n");
+	return -ENODEV;
+}
+
+static void oprofile_arch_exit(void) { }
+
+#endif /* CONFIG_HW_PERF_EVENTS */

Don't we now have oprofile_arch_init defined twice in the non-PMU case? (and one of these definitions is __init)?

arch/arm/oprofile/common.c static void arm_backtrace(struct pt_regs * const regs, unsigned int depth) · line 127

Quoted 9 lines

static void arm_backtrace(struct pt_regs * const regs, unsigned int depth) {

arch/arm/oprofile/common.c +1 -0

 	struct frame_tail *tail = ((struct frame_tail *) regs->ARM_fp) - 1;

@@ -111,6 +123,7 @@ static void arm_backtrace(struct pt_regs * const regs, unsigned int depth)
 
 int __init oprofile_arch_init(struct oprofile_operations *ops)
 {
+	/* provide backtrace support also in timer mode: */
 	ops->backtrace		= arm_backtrace;

Right, so this becomes common code but we need to ensure that we don't call oprofile_perf_init. I suppose an alternative would be to provide an empty static inline stub for oprofile_perf_init instead.

Will

Will Deacon

2011-01-20 10:02 UTC #5 lore raw

In reply to Robert Richter

Hi Robert,

Quoted 1 line

I would like to go even further, see the diff below.

[...]

arch/arm/oprofile/common.c static struct frame_tail* user_backtrace(struct frame_tail *tail) · line 92

Quoted 11 lines

arch/arm/oprofile/common.c +11 -0

+#else
+
+static int oprofile_arch_init(struct oprofile_operations *ops)
+{
+	pr_info("oprofile: hardware counters not available\n");
+	return -ENODEV;
+}
+
+static void oprofile_arch_exit(void) { }
+
+#endif /* CONFIG_HW_PERF_EVENTS */

Don't we now have oprofile_arch_init defined twice in the non-PMU case? (and one of these definitions is __init)?

arch/arm/oprofile/common.c static void arm_backtrace(struct pt_regs * const regs, unsigned int depth) · line 127

Quoted 9 lines

static void arm_backtrace(struct pt_regs * const regs, unsigned int depth) {

arch/arm/oprofile/common.c +1 -0

 	struct frame_tail *tail = ((struct frame_tail *) regs->ARM_fp) - 1;

@@ -111,6 +123,7 @@ static void arm_backtrace(struct pt_regs * const regs, unsigned int depth)
 
 int __init oprofile_arch_init(struct oprofile_operations *ops)
 {
+	/* provide backtrace support also in timer mode: */
 	ops->backtrace		= arm_backtrace;

Will

Ari Kauppi

author

2011-01-20 10:17 UTC #6 lore raw

PATCH 1/1 ARM: oprofile: Always allow backtraces

In reply to Robert Richter

On Thu, 20 Jan 2011, Robert Richter wrote:

Hi,

Quoted 28 lines · Will Deacon

Quoted 27 lines
Hi Ari,
Quoted 5 lines
Always allow backtrace when using oprofile on ARM, even if a PMU isn't present.
Restores functionality originally introduced in 1b7b56982fdcd9d85effd76f3928cf5d6eb26155 by Richard Purdie.
Well spotted, I'd forgotten we still want this for timer mode. Comments inline.
Quoted 16 lines
arch/arm/oprofile/common.c +1 -0
index 8aa9744..a23e5ae 100644
@@ -46,6 +46,7 @@ char *op_name_from_perf_id(void)
 		return NULL;
 	}
 }
+#endif
 
 static int report_trace(struct stackframe *frame, void *d)
 {
@@ -109,6 +110,7 @@ static void arm_backtrace(struct pt_regs * const regs, unsigned int depth)
 		tail = user_backtrace(tail);
 }
 

Quoted 6 lines

Quoted 4 lines
arch/arm/oprofile/common.c static void arm_backtrace(struct pt_regs * const regs, unsigned int depth) · line 113
Quoted 1 line
arch/arm/oprofile/common.c +1 -0
+#ifdef CONFIG_HW_PERF_EVENTS
Can you reorder the backtrace stuff so that we don't need the extra #endif/#ifdef please?
I would like to go even further, see the diff below.

Thanks for comments, I can prepare a new patch. I think your patch below has two typos but the idea was clear, please see comments below.

Quoted 32 lines

-Robert

---
arch/arm/oprofile/common.c | 45 ++++++++++++++++++++++++-------------------

1 files changed, 25 insertions(+), 20 deletions(-)

arch/arm/oprofile/common.c +0 ~~-12~~

index 8aa9744..916aa2e 100644
@@ -47,18 +47,6 @@ char *op_name_from_perf_id(void)
 	}
 }
 
-static int report_trace(struct stackframe *frame, void *d)
-{
-	unsigned int *depth = d;
-
-	if (*depth) {
-		oprofile_add_trace(frame->pc);
-		(*depth)--;
-	}
-
-	return *depth == 0;
-}
-
 /*
 * The registers we're interested in are at the end of the variable
 * length saved register structure. The fp points at the end of this

@@ -91,6 +79,30 @@ static struct frame_tail* user_backtrace(struct frame_tail *tail)
 	return buftail[0].fp-1;
 }

arch/arm/oprofile/common.c static struct frame_tail* user_backtrace(struct frame_tail *tail) · line 90

Quoted 10 lines

arch/arm/oprofile/common.c +9 -0

 
+#else
+
+static int oprofile_arch_init(struct oprofile_operations *ops)
+{
+	pr_info("oprofile: hardware counters not available\n");
+	return -ENODEV;
+}
+
+static void oprofile_arch_exit(void) { }

I guess you mean oprofile_perf_init and oprofile_perf_exit?

Also the user_backtrace and frame_tail need to be moved out from the #ifdef.

Quoted 39 lines

arch/arm/oprofile/common.c +16 -8

+
+#endif /* CONFIG_HW_PERF_EVENTS */
+
+static int report_trace(struct stackframe *frame, void *d)
+{
+	unsigned int *depth = d;
+
+	if (*depth) {
+		oprofile_add_trace(frame->pc);
+		(*depth)--;
+	}
+
+	return *depth == 0;
+}
+
 static void arm_backtrace(struct pt_regs * const regs, unsigned int depth)
 {
 	struct frame_tail *tail = ((struct frame_tail *) regs->ARM_fp) - 1;

@@ -111,6 +123,7 @@ static void arm_backtrace(struct pt_regs * const regs, unsigned int depth)
 
 int __init oprofile_arch_init(struct oprofile_operations *ops)
 {
+	/* provide backtrace support also in timer mode: */
 	ops->backtrace		= arm_backtrace;
 
 	return oprofile_perf_init(ops);

@@ -120,11 +133,3 @@ void __exit oprofile_arch_exit(void)
 {
 	oprofile_perf_exit();
 }
-#else
-int __init oprofile_arch_init(struct oprofile_operations *ops)
-{
-	pr_info("oprofile: hardware counters not available\n");
-	return -ENODEV;
-}
-void __exit oprofile_arch_exit(void) {}
-#endif /* CONFIG_HW_PERF_EVENTS */

Signature · 1 line

Ari

Robert Richter

2011-01-20 10:46 UTC #7 lore raw

PATCH 1/1 ARM: oprofile: Always allow backtraces

Quoted 4 lines · Ari Kauppi

Quoted 4 lines

arch/arm/oprofile/common.c +0 -0

@@ -91,6 +79,30 @@ static struct frame_tail* user_backtrace(struct frame_tail *tail)
 	return buftail[0].fp-1;
 }

Quoted 11 lines

arch/arm/oprofile/common.c static struct frame_tail* user_backtrace(struct frame_tail *tail) · line 90
Quoted 9 lines
arch/arm/oprofile/common.c +9 -0
+#else
+
+static int oprofile_arch_init(struct oprofile_operations *ops)
+{
+	pr_info("oprofile: hardware counters not available\n");
+	return -ENODEV;
+}
+
+static void oprofile_arch_exit(void) { }
I guess you mean oprofile_perf_init and oprofile_perf_exit?

Yes, did not compile test it as my arm cross compile chain was broken.

Maybe we move it to include/linux/oprofile.h.

I also observed some mismatches while using CONFIG_HW_PERF_EVENTS and CONFIG_PERF_EVENTS, esp. with include/linux/oprofile.h. /arch/sh is also affected. We need to review this too.

Quoted 3 lines

Also the user_backtrace and frame_tail need to be moved out from the #ifdef.

Yes, thanks.

-Robert

Signature · 2 lines

Advanced Micro Devices, Inc.
Operating System Research Center

Will Deacon

2011-01-20 09:42 UTC #8 lore raw

In reply to Ari Kauppi

Hi Ari,

Quoted 5 lines

Always allow backtrace when using oprofile on ARM, even if a PMU isn't present.
Restores functionality originally introduced in 1b7b56982fdcd9d85effd76f3928cf5d6eb26155 by Richard Purdie.

Well spotted, I'd forgotten we still want this for timer mode. Comments inline.

Quoted 13 lines

arch/arm/oprofile/common.c +1 -0

index 8aa9744..a23e5ae 100644
@@ -46,6 +46,7 @@ char *op_name_from_perf_id(void)
 		return NULL;
 	}
 }
+#endif
 
 static int report_trace(struct stackframe *frame, void *d)
 {

arch/arm/oprofile/common.c static void arm_backtrace(struct pt_regs * const regs, unsigned int depth) · line 113

Quoted 4 lines

arch/arm/oprofile/common.c +1 -0

@@ -109,6 +110,7 @@ static void arm_backtrace(struct pt_regs * const regs, unsigned int depth)
 		tail = user_backtrace(tail);
 }
 
+#ifdef CONFIG_HW_PERF_EVENTS

Can you reorder the backtrace stuff so that we don't need the extra #endif/#ifdef please?

arch/arm/oprofile/common.c void __exit oprofile_arch_exit(void) · line 133

Quoted 12 lines

arch/arm/oprofile/common.c +2 -0

 int __init oprofile_arch_init(struct oprofile_operations *ops)
 {
 	ops->backtrace		= arm_backtrace;

@@ -124,6 +126,8 @@ void __exit oprofile_arch_exit(void)
 int __init oprofile_arch_init(struct oprofile_operations *ops)
 {
 	pr_info("oprofile: hardware counters not available\n");
+	ops->backtrace		= arm_backtrace;
+
 	return -ENODEV;
 }
 void __exit oprofile_arch_exit(void) {}

One thing worth mentioning is that the backtrace code relies on a sensible frame pointer, which you might not have in ARM code and you certainly won't have for Thumb-2 code.

Will

Sergei Shtylyov

2011-01-20 12:31 UTC #9 lore raw

PATCH 1/1 ARM: oprofile: Always allow backtraces

In reply to Ari Kauppi

Hello.

Quoted 5 lines · Ari Kauppi

Always allow backtrace when using oprofile on ARM, even if a PMU isn't present.
Restores functionality originally introduced in 1b7b56982fdcd9d85effd76f3928cf5d6eb26155 by Richard Purdie.

You're lacking the commit summary specified in parens which Linus asked to
add.

Quoted 4 lines

Signed-off-by Ari Kauppi<[email protected]>
Cc Richard Purdie<[email protected]>
Cc Will Deacon<[email protected]>
Cc Matt Fleming<[email protected]>

WBR, Sergei