* [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).