On Tuesday 29 January 2013, Keun-O Park wrote:
> On Mon, Jan 28, 2013 at 9:50 PM, Dave Martin <[email protected]> wrote:
> > On Mon, Jan 28, 2013 at 11:33:11AM +0900, Keun-O Park wrote:
> >> Hello guys,
> >>
> >> Could you please review the patch of fixing bug first of returning
> >> wrong address when using frame pointer?
> >> I am wondering if the first patch is not delivered to the mailing.
> >
> > I posted a similar patch to alkml a couple of months ago, but I got
> > no response and it looks like I forgot about it.
> >
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2012-November/129381.html
>
> Yes, same except initialization of data.addr. :)
> This means there might be no one interested in using
> ftrace-irqsoff/premptoff in ARM during a couple of months?
It's been almost a year since we last discussed the patches that were
posted by Dave and sahara, but nothing has changed in the mainline kernel.
Any chance that someone could be motivated to pick this work up again
and finally fix return_address().
Arnd
On Tue, Jan 07, 2014 at 03:33:34PM +0100, Arnd Bergmann wrote:
> On Tuesday 29 January 2013, Keun-O Park wrote:
> > On Mon, Jan 28, 2013 at 9:50 PM, Dave Martin <[email protected]> wrote:
> > > On Mon, Jan 28, 2013 at 11:33:11AM +0900, Keun-O Park wrote:
> > >> Hello guys,
> > >>
> > >> Could you please review the patch of fixing bug first of returning
> > >> wrong address when using frame pointer?
> > >> I am wondering if the first patch is not delivered to the mailing.
> > >
> > > I posted a similar patch to alkml a couple of months ago, but I got
> > > no response and it looks like I forgot about it.
> > >
> > > http://lists.infradead.org/pipermail/linux-arm-kernel/2012-November/129381.html
> >
> > Yes, same except initialization of data.addr. :)
> > This means there might be no one interested in using
> > ftrace-irqsoff/premptoff in ARM during a couple of months?
>
>
> It's been almost a year since we last discussed the patches that were
> posted by Dave and sahara, but nothing has changed in the mainline kernel.
>
> Any chance that someone could be motivated to pick this work up again
> and finally fix return_address().
I thought that we had _actively_ decided that we would not use the
unwinder for these paths - that it was too expensive for these paths,
and you had to use frame pointers instead.
--
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".
On Tuesday 07 January 2014 14:41:30 Russell King - ARM Linux wrote:
> On Tue, Jan 07, 2014 at 03:33:34PM +0100, Arnd Bergmann wrote:
> >
> >
> > It's been almost a year since we last discussed the patches that were
> > posted by Dave and sahara, but nothing has changed in the mainline kernel.
> >
> > Any chance that someone could be motivated to pick this work up again
> > and finally fix return_address().
>
> I thought that we had _actively_ decided that we would not use the
> unwinder for these paths - that it was too expensive for these paths,
> and you had to use frame pointers instead.
I don't remember that discussion, but it may well be. What does
that mean for the #warning in return_address.c then? Can we
just use the frame pointer version based on CONFIG_FRAME_POINTER
and ignore whether CONFIG_ARM_UNWIND is set as the patch below,
or did I misunderstand?
Arnd
diff --git a/arch/arm/include/asm/ftrace.h b/arch/arm/include/asm/ftrace.h
index f89515a..3247370 100644
--- a/arch/arm/include/asm/ftrace.h
+++ b/arch/arm/include/asm/ftrace.h
@@ -32,7 +32,7 @@ extern void ftrace_call_old(void);
#ifndef __ASSEMBLY__
-#if defined(CONFIG_FRAME_POINTER) && !defined(CONFIG_ARM_UNWIND)
+#if defined(CONFIG_FRAME_POINTER)
/*
* return_address uses walk_stackframe to do it's work. If both
* CONFIG_FRAME_POINTER=y and CONFIG_ARM_UNWIND=y walk_stackframe uses unwind
diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
index a30fc9b..c713f46 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -16,13 +16,14 @@ CFLAGS_REMOVE_return_address.o = -pg
# Object file lists.
obj-y := elf.o entry-common.o irq.o opcodes.o \
- process.o ptrace.o return_address.o \
+ process.o ptrace.o \
setup.o signal.o sigreturn_codes.o \
stacktrace.o sys_arm.o time.o traps.o
obj-$(CONFIG_ATAGS) += atags_parse.o
obj-$(CONFIG_ATAGS_PROC) += atags_proc.o
obj-$(CONFIG_DEPRECATED_PARAM_STRUCT) += atags_compat.o
+obj-$(CONFIG_FRAME_POINTER) += return_address.o
ifeq ($(CONFIG_CPU_V7M),y)
obj-y += entry-v7m.o v7m.o
diff --git a/arch/arm/kernel/return_address.c b/arch/arm/kernel/return_address.c
index fafedd8..d9f2c15 100644
--- a/arch/arm/kernel/return_address.c
+++ b/arch/arm/kernel/return_address.c
@@ -10,8 +10,6 @@
*/
#include <linux/export.h>
#include <linux/ftrace.h>
-
-#if defined(CONFIG_FRAME_POINTER) && !defined(CONFIG_ARM_UNWIND)
#include <linux/sched.h>
#include <asm/stacktrace.h>
@@ -56,18 +54,4 @@ void *return_address(unsigned int level)
else
return NULL;
}
-
-#else /* if defined(CONFIG_FRAME_POINTER) && !defined(CONFIG_ARM_UNWIND) */
-
-#if defined(CONFIG_ARM_UNWIND)
-#warning "TODO: return_address should use unwind tables"
-#endif
-
-void *return_address(unsigned int level)
-{
- return NULL;
-}
-
-#endif /* if defined(CONFIG_FRAME_POINTER) && !defined(CONFIG_ARM_UNWIND) / else */
-
EXPORT_SYMBOL_GPL(return_address);
On Tue, Jan 07, 2014 at 03:48:25PM +0000, Arnd Bergmann wrote:
> On Tuesday 07 January 2014 14:41:30 Russell King - ARM Linux wrote:
> > On Tue, Jan 07, 2014 at 03:33:34PM +0100, Arnd Bergmann wrote:
> > >
> > >
> > > It's been almost a year since we last discussed the patches that were
> > > posted by Dave and sahara, but nothing has changed in the mainline kernel.
> > >
> > > Any chance that someone could be motivated to pick this work up again
> > > and finally fix return_address().
> >
> > I thought that we had _actively_ decided that we would not use the
> > unwinder for these paths - that it was too expensive for these paths,
> > and you had to use frame pointers instead.
>
> I don't remember that discussion, but it may well be. What does
> that mean for the #warning in return_address.c then? Can we
> just use the frame pointer version based on CONFIG_FRAME_POINTER
> and ignore whether CONFIG_ARM_UNWIND is set as the patch below,
> or did I misunderstand?
For an ARM kernel this may work, but I thought that for THUMB2_KERNEL
there just isn't usable a framepointer at all.
If so, the only choices are to use the unwinder and accept the cost, or
to decide that return_address() will never work without
CONFIG_FRAMEPOINTER and remove the build-time warning.
My other concern was that we might end up in a recursive trace due to
the use of non-notrace core functions in the unwinder. But I seem to
remember Steve Rostedt saying the the tracer guards against recursive
invocation nowadays -- if so, that shouldn't be a problem.
Cheers
---Dave
On Tue, 7 Jan 2014 16:36:29 +0000
Dave Martin <[email protected]> wrote:
> My other concern was that we might end up in a recursive trace due to
> the use of non-notrace core functions in the unwinder. But I seem to
> remember Steve Rostedt saying the the tracer guards against recursive
> invocation nowadays -- if so, that shouldn't be a problem.
I guess it matters what type of tracing you are talking about. The
function tracer protects against all recursive contexts (normal,
softirq, irq and NMI) and so does the ring buffer (same levels).
Those may be the only ones that matter, as things like events shouldn't
recurse, unless you have an event in the unwinder itself. But that's
where you take the doctor's advice of "don't do that".
-- Steve