* [PATCH] ARM: oprofile: Always allow backtraces
@ 2011-01-19 20:54 Ari Kauppi
2011-01-20 9:42 ` Will Deacon
2011-01-20 12:31 ` Sergei Shtylyov
0 siblings, 2 replies; 8+ messages in thread
From: Ari Kauppi @ 2011-01-19 20:54 UTC (permalink / raw)
To: linux-arm-kernel
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 <kauppi@papupata.org>
Cc: Richard Purdie <rpurdie@rpsys.net>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Matt Fleming <matt@console-pimps.org>
---
arch/arm/oprofile/common.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
index 8aa9744..a23e5ae 100644
--- a/arch/arm/oprofile/common.c
+++ b/arch/arm/oprofile/common.c
@@ -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) {}
--
1.7.4.rc2.3.g60a2e
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH] ARM: oprofile: Always allow backtraces
2011-01-19 20:54 [PATCH] ARM: oprofile: Always allow backtraces Ari Kauppi
@ 2011-01-20 9:42 ` Will Deacon
2011-01-20 9:52 ` Robert Richter
2011-01-20 12:31 ` Sergei Shtylyov
1 sibling, 1 reply; 8+ messages in thread
From: Will Deacon @ 2011-01-20 9:42 UTC (permalink / raw)
To: linux-arm-kernel
Hi Ari,
> 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.
> diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
> index 8aa9744..a23e5ae 100644
> --- a/arch/arm/oprofile/common.c
> +++ b/arch/arm/oprofile/common.c
> @@ -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
Can you reorder the backtrace stuff so that we don't need the extra
#endif/#ifdef please?
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] ARM: oprofile: Always allow backtraces
2011-01-20 9:42 ` Will Deacon
@ 2011-01-20 9:52 ` Robert Richter
2011-01-20 9:57 ` Robert Richter
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Robert Richter @ 2011-01-20 9:52 UTC (permalink / raw)
To: linux-arm-kernel
On 20.01.11 04:42:24, Will Deacon wrote:
> Hi Ari,
>
> > 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.
>
> > diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
> > index 8aa9744..a23e5ae 100644
> > --- a/arch/arm/oprofile/common.c
> > +++ b/arch/arm/oprofile/common.c
> > @@ -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
>
> 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(-)
diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
index 8aa9744..916aa2e 100644
--- a/arch/arm/oprofile/common.c
+++ b/arch/arm/oprofile/common.c
@@ -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 */
--
1.7.3.4
--
Advanced Micro Devices, Inc.
Operating System Research Center
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH] ARM: oprofile: Always allow backtraces
2011-01-20 9:52 ` Robert Richter
@ 2011-01-20 9:57 ` Robert Richter
2011-01-20 10:02 ` Will Deacon
2011-01-20 10:17 ` Ari Kauppi
2 siblings, 0 replies; 8+ messages in thread
From: Robert Richter @ 2011-01-20 9:57 UTC (permalink / raw)
To: linux-arm-kernel
On 20.01.11 10:52:39, Robert Richter wrote:
> 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(-)
>
> diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
> index 8aa9744..916aa2e 100644
> --- a/arch/arm/oprofile/common.c
> +++ b/arch/arm/oprofile/common.c
> @@ -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)
Of course it should compile, but in this sense...
-Robert
> +{
> + 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 */
--
Advanced Micro Devices, Inc.
Operating System Research Center
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] ARM: oprofile: Always allow backtraces
2011-01-20 9:52 ` Robert Richter
2011-01-20 9:57 ` Robert Richter
@ 2011-01-20 10:02 ` Will Deacon
2011-01-20 10:17 ` Ari Kauppi
2 siblings, 0 replies; 8+ messages in thread
From: Will Deacon @ 2011-01-20 10:02 UTC (permalink / raw)
To: linux-arm-kernel
Hi Robert,
> I would like to go even further, see the diff below.
[...]
> +#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)?
> 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;
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] ARM: oprofile: Always allow backtraces
2011-01-20 9:52 ` Robert Richter
2011-01-20 9:57 ` Robert Richter
2011-01-20 10:02 ` Will Deacon
@ 2011-01-20 10:17 ` Ari Kauppi
2011-01-20 10:46 ` Robert Richter
2 siblings, 1 reply; 8+ messages in thread
From: Ari Kauppi @ 2011-01-20 10:17 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, 20 Jan 2011, Robert Richter wrote:
Hi,
> On 20.01.11 04:42:24, Will Deacon wrote:
>> Hi Ari,
>>
>>> 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.
>>
>>> diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
>>> index 8aa9744..a23e5ae 100644
>>> --- a/arch/arm/oprofile/common.c
>>> +++ b/arch/arm/oprofile/common.c
>>> @@ -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
>>
>> 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.
> -Robert
>
> ---
> arch/arm/oprofile/common.c | 45 ++++++++++++++++++++++++-------------------
> 1 files changed, 25 insertions(+), 20 deletions(-)
>
> diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
> index 8aa9744..916aa2e 100644
> --- a/arch/arm/oprofile/common.c
> +++ b/arch/arm/oprofile/common.c
> @@ -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) { }
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.
> +
> +#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 */
>
--
Ari
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] ARM: oprofile: Always allow backtraces
2011-01-20 10:17 ` Ari Kauppi
@ 2011-01-20 10:46 ` Robert Richter
0 siblings, 0 replies; 8+ messages in thread
From: Robert Richter @ 2011-01-20 10:46 UTC (permalink / raw)
To: linux-arm-kernel
On 20.01.11 05:17:16, Ari Kauppi wrote:
> > @@ -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) { }
>
> 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.
>
> Also the user_backtrace and frame_tail need to be moved out from the
> #ifdef.
Yes, thanks.
-Robert
--
Advanced Micro Devices, Inc.
Operating System Research Center
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] ARM: oprofile: Always allow backtraces
2011-01-19 20:54 [PATCH] ARM: oprofile: Always allow backtraces Ari Kauppi
2011-01-20 9:42 ` Will Deacon
@ 2011-01-20 12:31 ` Sergei Shtylyov
1 sibling, 0 replies; 8+ messages in thread
From: Sergei Shtylyov @ 2011-01-20 12:31 UTC (permalink / raw)
To: linux-arm-kernel
Hello.
On 19-01-2011 23:54, Ari Kauppi wrote:
> 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.
> Signed-off-by: Ari Kauppi<kauppi@papupata.org>
> Cc: Richard Purdie<rpurdie@rpsys.net>
> Cc: Will Deacon<will.deacon@arm.com>
> Cc: Matt Fleming<matt@console-pimps.org>
WBR, Sergei
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2011-01-20 12:31 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-01-19 20:54 [PATCH] ARM: oprofile: Always allow backtraces Ari Kauppi
2011-01-20 9:42 ` Will Deacon
2011-01-20 9:52 ` Robert Richter
2011-01-20 9:57 ` Robert Richter
2011-01-20 10:02 ` Will Deacon
2011-01-20 10:17 ` Ari Kauppi
2011-01-20 10:46 ` Robert Richter
2011-01-20 12:31 ` Sergei Shtylyov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).