2008-11-20 20:36:30

by Steven Rostedt

[permalink] [raw]
Subject: [PATCH 0/3] ftrace: updates to recordmcount.pl script

Ingo,

I took Matt's patch and stripped out all the sh changes, leaving
only the update to recordmcount.pl. Hopefully, this will disappear
when it is pulled into linux-next.

The second patch is the changes needed for PowerPC in the recordmcount.pl
script.

The thrird patch is the cleanup.

All of this is based off of tip/master.

The following patches are in:

git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git

branch: tip/devel


Matt Fleming (1):
sh: dynamic ftrace support.

Steven Rostedt (2):
ftrace: add support for powerpc to recordmcount.pl script
ftrace: create default variables for archs in recordmcount.pl

----
scripts/recordmcount.pl | 37 +++++++++++++++++++++++++++++--------
1 files changed, 29 insertions(+), 8 deletions(-)


2008-11-20 22:12:20

by Jim Radford

[permalink] [raw]
Subject: Re: [PATCH] ftrace: mcountrecord.pl for arm

Ingo and Steven,

Here's an updated version of the arch/arm changes for dynamic ftrace
based on top of your latest tip/master.

-Jim

---
From: Jim Radford <[email protected]>
Subject: ftrace: enable dynamic ftrace for arm

Update to the latest api, syncing functions with the x86 versions.

Index: linux-2.6/arch/arm/Kconfig
===================================================================
--- linux-2.6.orig/arch/arm/Kconfig
+++ linux-2.6/arch/arm/Kconfig
@@ -16,7 +16,9 @@ config ARM
select HAVE_ARCH_KGDB
select HAVE_KPROBES if (!XIP_KERNEL)
select HAVE_KRETPROBES if (HAVE_KPROBES)
- select HAVE_FUNCTION_TRACER if (!XIP_KERNEL)
+ select HAVE_FTRACE_MCOUNT_RECORD
+ select HAVE_DYNAMIC_FTRACE if (!XIP_KERNEL)
+ select HAVE_FUNCTION_TRACER
select HAVE_GENERIC_DMA_COHERENT
help
The ARM series is a line of low-power-consumption RISC chip designs
Index: linux-2.6/arch/arm/include/asm/ftrace.h
===================================================================
--- linux-2.6.orig/arch/arm/include/asm/ftrace.h
+++ linux-2.6/arch/arm/include/asm/ftrace.h
@@ -7,6 +7,19 @@

#ifndef __ASSEMBLY__
extern void mcount(void);
+
+static inline unsigned long ftrace_call_adjust(unsigned long addr)
+{
+ return addr;
+}
+
+#ifdef CONFIG_DYNAMIC_FTRACE
+
+struct dyn_arch_ftrace {
+ /* No extra data needed for x86 */
+};
+
+#endif /* CONFIG_DYNAMIC_FTRACE */
#endif

#endif
Index: linux-2.6/arch/arm/kernel/entry-common.S
===================================================================
--- linux-2.6.orig/arch/arm/kernel/entry-common.S
+++ linux-2.6/arch/arm/kernel/entry-common.S
@@ -104,14 +104,7 @@ ENDPROC(ret_from_fork)
#ifdef CONFIG_FUNCTION_TRACER
#ifdef CONFIG_DYNAMIC_FTRACE
ENTRY(mcount)
- stmdb sp!, {r0-r3, lr}
- mov r0, lr
- sub r0, r0, #MCOUNT_INSN_SIZE
-
- .globl mcount_call
-mcount_call:
- bl ftrace_stub
- ldmia sp!, {r0-r3, pc}
+ mov pc, lr

ENTRY(ftrace_caller)
stmdb sp!, {r0-r3, lr}
Index: linux-2.6/arch/arm/kernel/ftrace.c
===================================================================
--- linux-2.6.orig/arch/arm/kernel/ftrace.c
+++ linux-2.6/arch/arm/kernel/ftrace.c
@@ -23,13 +23,13 @@
static unsigned long bl_insn;
static const unsigned long NOP = 0xe1a00000; /* mov r0, r0 */

-unsigned char *ftrace_nop_replace(void)
+static unsigned char *ftrace_nop_replace(void)
{
return (char *)&NOP;
}

/* construct a branch (BL) instruction to addr */
-unsigned char *ftrace_call_replace(unsigned long pc, unsigned long addr)
+static unsigned char *ftrace_call_replace(unsigned long pc, unsigned long addr)
{
long offset;

@@ -46,7 +46,7 @@ unsigned char *ftrace_call_replace(unsig
return (unsigned char *)&bl_insn;
}

-int ftrace_modify_code(unsigned long pc, unsigned char *old_code,
+static int ftrace_modify_code(unsigned long pc, unsigned char *old_code,
unsigned char *new_code)
{
unsigned long err = 0, replaced = 0, old, new;
@@ -82,22 +82,46 @@ int ftrace_modify_code(unsigned long pc,
return err;
}

+int ftrace_make_nop(struct module *mod,
+ struct dyn_ftrace *rec, unsigned long addr)
+{
+ unsigned char *new, *old;
+ unsigned long ip = rec->ip;
+
+ old = ftrace_call_replace(ip, addr);
+ new = ftrace_nop_replace();
+
+ return ftrace_modify_code(rec->ip, old, new);
+}
+
+int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
+{
+ unsigned char *new, *old;
+ unsigned long ip = rec->ip;
+
+ old = ftrace_nop_replace();
+ new = ftrace_call_replace(ip, addr);
+
+ return ftrace_modify_code(rec->ip, old, new);
+}
+
int ftrace_update_ftrace_func(ftrace_func_t func)
{
+ unsigned long ip = (unsigned long)(&ftrace_call);
+ unsigned char old[MCOUNT_INSN_SIZE], *new;
int ret;
- unsigned long pc, old;
- unsigned char *new;

- pc = (unsigned long)&ftrace_call;
- memcpy(&old, &ftrace_call, MCOUNT_INSN_SIZE);
- new = ftrace_call_replace(pc, (unsigned long)func);
- ret = ftrace_modify_code(pc, (unsigned char *)&old, new);
+ memcpy(old, &ftrace_call, sizeof(old));
+ new = ftrace_call_replace(ip, (unsigned long)func);
+ ret = ftrace_modify_code(ip, (unsigned char *)&old, new);
+
return ret;
}

/* run from kstop_machine */
int __init ftrace_dyn_arch_init(void *data)
{
- ftrace_mcount_set(data);
+ /* The return code is retured via data */
+ *(unsigned long *)data = 0;
return 0;
}

2008-11-20 22:13:37

by Jim Radford

[permalink] [raw]
Subject: [PATCH] ftrace: mcountrecord.pl for arm

Ingo and Steven,

Here is a patch on top of Steven's for recordmcount.pl on arm.

-Jim

---
From: Jim Radford <[email protected]>
Subject: ftrace: mcountrecord.pl for arm

Signed-Off-By: Jim Radford <[email protected]>

diff --git a/scripts/recordmcount.pl b/scripts/recordmcount.pl
index c5c58ac..3d16795 100755
--- a/scripts/recordmcount.pl
+++ b/scripts/recordmcount.pl
@@ -135,6 +135,7 @@ my $section_regex; # Find the start of a section
my $function_regex; # Find the name of a function
# (return offset and func name)
my $mcount_regex; # Find the call site to mcount (return offset)
+my $section_type; # Section header plus possible alignment command
my $alignment; # The .align value to use for $mcount_section

if ($arch eq "x86") {
@@ -153,6 +154,7 @@ $nm_regex = "^[0-9a-fA-F]+\\s+t\\s+(\\S+)";
$section_regex = "Disassembly of section\\s+(\\S+):";
$function_regex = "^([0-9a-fA-F]+)\\s+<(.*?)>:";
$mcount_regex = "^\\s*([0-9a-fA-F]+):.*\\smcount\$";
+$section_type = '@progbits';
$type = ".long";

if ($arch eq "x86_64") {
@@ -191,6 +193,10 @@ if ($arch eq "x86_64") {
$type = ".quad";
}

+} elsif ($arch eq "arm") {
+ $alignment = 2;
+ $section_type = '%progbits';
+
} else {
die "Arch $arch is not supported with CONFIG_FTRACE_MCOUNT_RECORD";
}
@@ -311,7 +317,7 @@ sub update_funcs
if (!$opened) {
open(FILE, ">$mcount_s") || die "can't create $mcount_s\n";
$opened = 1;
- print FILE "\t.section $mcount_section,\"a\",\@progbits\n";
+ print FILE "\t.section $mcount_section,\"a\",$section_type\n";
print FILE "\t.align $alignment\n" if (defined($alignment));
}
printf FILE "\t%s %s + %d\n", $type, $ref_func, $offsets[$i] - $offset;

2008-11-21 13:11:55

by Russell King - ARM Linux

[permalink] [raw]
Subject: Re: [PATCH] ftrace: mcountrecord.pl for arm

On Thu, Nov 20, 2008 at 02:11:49PM -0800, Jim Radford wrote:
> Ingo and Steven,
>
> Here's an updated version of the arch/arm changes for dynamic ftrace
> based on top of your latest tip/master.

Excuse me if I'm rather confused, but...

When ftrace for ARM was originally merged, neither linux-arm-kernel
nor myself were copied with the patches. Now, I'm being sent updates
to code that I've no understanding of and haven't seen before.

I mean, yes, it's nice to be copied with patches which are relevent.
It would've been even nicer to have been copied with the patches adding
ftrace in the first place, so people knew something about it and were
aware of the changes.

It seems to me like there's been a total breakdown of communication
when ftrace was initially merged...

So, questions: has ftrace actually been tested on ARM at all? Has it
been reviewed? Which ARM platforms has it been tried on? How stable
is it? How has it been implemented on ARM? Does it rely on any CPU
specific behaviour?

Looking at the git history, ftrace was merged via Ingo, so I assume
that Ingo has some understanding of this code. So, for the time being
if these are urgent updates, I suggest that updates go through Ingo's
tree rather than mine.

And looking at arch/arm/kernel/ftrace.c, it's incompatible with Thumb2
which we've been working towards supporting. What about SMP? ARM is
a SMP capable architecture now, and I see no locking in there - what
I do see is static data with pointers to it being returned to other
code... Yuck.

2008-11-21 13:31:26

by Steven Rostedt

[permalink] [raw]
Subject: Re: [PATCH] ftrace: mcountrecord.pl for arm


On Fri, 21 Nov 2008, Russell King - ARM Linux wrote:

> On Thu, Nov 20, 2008 at 02:11:49PM -0800, Jim Radford wrote:
> > Ingo and Steven,
> >
> > Here's an updated version of the arch/arm changes for dynamic ftrace
> > based on top of your latest tip/master.
>
> Excuse me if I'm rather confused, but...
>
> When ftrace for ARM was originally merged, neither linux-arm-kernel
> nor myself were copied with the patches. Now, I'm being sent updates
> to code that I've no understanding of and haven't seen before.
>
> I mean, yes, it's nice to be copied with patches which are relevent.
> It would've been even nicer to have been copied with the patches adding
> ftrace in the first place, so people knew something about it and were
> aware of the changes.
>
> It seems to me like there's been a total breakdown of communication
> when ftrace was initially merged...

Yes I totally agree that in the beginning there was a breakdown of
communication. I myself just learned of the ARM port.

>
> So, questions: has ftrace actually been tested on ARM at all? Has it
> been reviewed? Which ARM platforms has it been tried on? How stable
> is it? How has it been implemented on ARM? Does it rely on any CPU
> specific behaviour?
>
> Looking at the git history, ftrace was merged via Ingo, so I assume
> that Ingo has some understanding of this code. So, for the time being
> if these are urgent updates, I suggest that updates go through Ingo's
> tree rather than mine.

I would suggest that they at least get an ACK from you. The original
code should have too.

>
> And looking at arch/arm/kernel/ftrace.c, it's incompatible with Thumb2
> which we've been working towards supporting. What about SMP? ARM is
> a SMP capable architecture now, and I see no locking in there - what
> I do see is static data with pointers to it being returned to other
> code... Yuck.

Some of this code will be redesigned in 29. But as for the locking, this
code is run under kstop_machine. Which means that even on SMP
architectures, this acts like a UP box. Some of the code can be run
outside of kstop_machine, but it is protected by locks in the module code.
I'll take a look at the ftrace.c arm code and see if there's any problems
with it. I wrote the x86 version as well as the coming PowerPC port.

Thanks,

-- Steve