Skip to contentSkip to conversation

ftrace: to kill a daemon (small updates)

PATCH In review Steven Rostedt 6 participants 17 messages Aug 14, 2008 – Aug 21, 2008

Subsystems (5)
lore
Share & tools
lore

Steven Rostedt

author maintainer
#0 lore raw

v2 cover ftrace: to kill a daemon (small updates)

[
Changes since v1:

regex fix in x86_64 recordmcount.pl. Now it can handle all
mcount+0x... mcount-0x... and mcount, where as the original only handled mcount+0x...

Made mcount on start-up to simply return. The current mcount
is set up to be replaced with a call to ftrace_record_ip. This is no longer necessary.

Note: This patch series is focusing on how calls to mcount in
the kernel are converted to nops. It does not address what kind of nop is used. That is a different topic, and should be in a different patch series.

Note 2: I have found that the changes here are more stable than
the current daemon method, and these patches should be used. It also solves the resume from suspend to ram bug that was reported:

http://lkml.org/lkml/2008/8/12/234
and
http://lkml.org/lkml/2008/8/12/451

Note 3: I have already ported this to PowerPC64, but I am waiting
for this to be accepted first before submitting those changes.
]

One of the things that bothered me about the latest ftrace code was this annoying daemon that would wake up once a second to see if it had work to do. If it did not, it would go to sleep, otherwise it would do its work and then go to sleep.

You see, the reason for this is that for ftrace to maintain performance when configured in but disabled, it would need to change all the locations that called "mcount" (enabled with the gcc -pg option) into nops. The "-pg" option in gcc sets up a function profiler to call this function called "mcount". If you simply have "mcount" return, it will still add 15 to 18% overhead in performance. Changing all the calls to nops moved the overhead into noise.

To get rid of this, I had the mcount code record the location that called it. Later, the "ftraced" daemon would wake up and look to see if any new functions were recorded. If so, it would call kstop_machine and convert the calls to "nops". We needed kstop_machine because bad things happen on SMP if you modify code that happens to be in the instruction cache of another CPU.

This "ftraced" kernel thread would be a happy little worker, but it caused some pains. One, is that it woke up once a second, and Ted Tso got mad at me because it would show up on PowerTop. I could easily make the default 5 seconds, and even have it runtime configurable, with a trivial patch. I have not got around to doing that yet.

The other annoying thing, and this one bothers me the most, is that we can not have this enabled on a production -rt kernel. The latency caused by the kstop_machine when doing work is lost in the noise on a non-rt kernel, but it can be up to 800 microseconds, and that would kill the -rt kernel. The reason this bothered me the most, is that -rt is where it came from, and ftraced was not treating its motherland very well.

Along came Gregory Haskins, who was bickering about having ftrace enabled on a production -rt kernel. I told him the reasons that this would be bad and then he started thinking out loud, and suggesting wild ideas, like patching gcc!

Since I have recently seen "The Dark Knight", Gregory's comments put me into an "evil" mood. I then thought of the idea about using the relocation entries of the mcount call sites, in a prelinked object file, and create a separate section with a list of these sites. On boot up, record them and change them into nops.

That's it! No kstop_machine for turning them into nops. We would only need stop_machine to enable or disable tracing, but a user not tracing will not have to deal with this annoying "ftraced" kernel thread waking up every second or ever running kstop_machine.

What's more, this means we can enable it on a production -rt kernel!

Now, this was no easy task. We needed to add a section to every object file with a list of pointers to the call sites to mcount. The idea I came up with was to make a tmp.s file for every object just after it is compiled. This tmp.s would then be compiled and relinked into the original object. The tmp.s file would have something like:

.section __mcount_loc,"a",@progbits
.quad location_of_mcount1
.quad location_of_mcount2
(etc)

By running objdump on the object file we can find the offsets into the sections that the functions are called.

For example, looking at hrtimer.o:

Disassembly of section .text:

0000000000000000 <hrtimer_init_sleeper>:

       0:       55                      push   %rbp
       1:       48 89 e5                mov    %rsp,%rbp
       4:       e8 00 00 00 00          callq  9 <hrtimer_init_sleeper+0x9>
                        5: R_X86_64_PC32        mcount+0xfffffffffffffffc

[...]

the '5' in the '5: R_X86_64_PC32' is the offset that the mcount relocation is to be done for the call site. This offset is from the .text section, and not necessarily, from the function. If we look further we see:

000000000000001e <ktime_add_safe>:

      1e:       55                      push   %rbp
      1f:       48 89 e5                mov    %rsp,%rbp
      22:       e8 00 00 00 00          callq  27 <ktime_add_safe+0x9>
                        23: R_X86_64_PC32       mcount+0xfffffffffffffffc

This mcount call site is 0x23 from the .text section, and obviously not from the ktime_add_safe.

If we make a tmp.s that has the following:

.section __mcount_loc,"a",@progbits
.quad hrtimer_init_sleeper + 0x5
.quad hrtimer_init_sleeper + 0x23

We have a section with the locations of these two call sites. After the final linking, they will point to the actual address used.

All that would need to be done is:

gcc -c tmp.s -o tmp.o
ld -r tmp.o hrtimer.o -o tmp_hrtime.o
mv tmp_hrtimer.o hrtimer.o

Easy as that! Not quite. What happens if that first function in the section is a static function? That is, the symbol for the function is local to the object. If for some reason hrtimer_init_sleeper is static, the tmp_hrtimer.o would have two symbols for hrtimer_init_sleeper. One local and one global.

But we can be even more evil with this idea. We can do crazy things with objcopy to solve it for us.

  objcopy --globalize-symbol hrtimer_init_sleeper hrtimer.o tmp_hrtimer.o

Now the hrtimer_init_sleeper would be global for linking.

ld -r tmp_hrtimer.o tmp.o -o tmp2_hrtimer.o

Now the tmp.o could use the same global hrtimer_init_sleeper symbol. But we have tmp2_hritmer.o that has the tmp.o and tmp_hrtimer.o symbols, but we cant just blindly convert local symbols to globals.

The solution is simply put it back to local.

  objcopy --localize-symbol hrtimer_init_sleeper tmp2_hrtimer.o hrtimer.o

Now our hrtimer.o file has our __mcount_loc section and the reference to hrtimer_init_sleeper will be resolved.

This is a bit complex to do in shell scripting and Makefiles, so I wrote a well documented recordmcount.pl perl script, that will do the above all in one place.

With this new update, we can work to kill that kernel thread "ftraced"!

This patch set ports to x86_64 and i386, the other archs will still use the daemon until they are converted over.

I tested this on both x86_64 and i386 with and without CONFIG_RELOCATE set.

author maintainer
#1 lore raw

PATCH v2 1/6 ftrace: create __mcount_loc section

[
Note: after this patch is applied, you need to run:

chmod +x scripts/recordmcount.pl
]

This patch creates a section in the kernel called "__mcount_loc". This will hold a list of pointers to the mcount relocation for each call site of mcount.

For example:

objdump -dr init/main.o
[...]
Disassembly of section .text:

0000000000000000 <do_one_initcall>:
0: 55 push %rbp
[...]
000000000000017b <init_post>:

 17b:   55                      push   %rbp
 17c:   48 89 e5                mov    %rsp,%rbp
 17f:   53                      push   %rbx
 180:   48 83 ec 08             sub    $0x8,%rsp
 184:   e8 00 00 00 00          callq  189 <init_post+0xe>
                        185: R_X86_64_PC32      mcount+0xfffffffffffffffc

[...]

We will add a section to point to each function call.

.section __mcount_loc,"a",@progbits
[...]
.quad .text + 0x185
[...]

The offset to of the mcount call site in init_post is an offset from the start of the section, and not the start of the function init_post. The mcount relocation is at the call site 0x185 from the start of the .text section.

.text + 0x185 == init_post + 0xa

We need a way to add this __mcount_loc section in a way that we do not lose the relocations after final link. The .text section here will be attached to all other .text sections after final link and the offsets will be meaningless. We need to keep track of where these .text sections are.

To do this, we use the start of the first function in the section. do_one_initcall. We can make a tmp.s file with this function as a reference to the start of the .text section.

.section __mcount_loc,"a",@progbits
[...]
.quad do_one_initcall + 0x185
[...]

Then we can compile the tmp.s into a tmp.o

gcc -c tmp.s -o tmp.o

And link it into back into main.o.

ld -r main.o tmp.o -o tmp_main.o
mv tmp_main.o main.o

But we have a problem. What happens if the first function in a section is not exported, and is a static function. The linker will not let the tmp.o use it. This case exists in main.o as well.

Disassembly of section .init.text:

0000000000000000 <set_reset_devices>:

   0:   55                      push   %rbp
   1:   48 89 e5                mov    %rsp,%rbp
   4:   e8 00 00 00 00          callq  9 <set_reset_devices+0x9>
                        5: R_X86_64_PC32        mcount+0xfffffffffffffffc

The first function in .init.text is a static function.

# nm init/main.o | grep set_reset_devices
00000000000000a8 t __setup_set_reset_devices
000000000000105f t __setup_str_set_reset_devices
0000000000000000 t set_reset_devices

The lowercase 't' means that set_reset_devices is local and is not exported. If we simply try to link the tmp.o with the set_reset_devices we end up with two symbols: one local and one global.

# cat tmp.s
 .section __mcount_loc,"a",@progbits
 .quad set_reset_devices + 0x10
# gcc -c tmp.s -o tmp.o
# ld -r main.o tmp.o -o tmp_main.o
# nm tmp_main.o | grep set_reset_devices
00000000000000a8 t __setup_set_reset_devices
000000000000105f t __setup_str_set_reset_devices
0000000000000000 t set_reset_devices
                 U set_reset_devices

We still have an undefined reference to set_reset_devices, and if we try to compile the kernel, we will end up with an undefined reference to set_reset_devices, or even worst, it could be exported someplace else, and then we will have a reference to the wrong location.

To handle this case, we make an intermediate step using objcopy. We convert set_reset_devices into a global exported symbol before linking it with tmp.o and set it back afterwards.

# objcopy --globalize-symbol set_reset_devices main.o tmp_main.o
# nm tmp_main.o | grep set_reset_devices
00000000000000a8 t __setup_set_reset_devices
000000000000105f t __setup_str_set_reset_devices
0000000000000000 T set_reset_devices
# ld -r tmp_main.o tmp.o -o tmp2_main.o
# nm tmp2_main.o | grep set_reset_devices
00000000000000a8 t __setup_set_reset_devices
000000000000105f t __setup_str_set_reset_devices
0000000000000000 T set_reset_devices
# objcopy --localize-symbol set_reset_devices tmp2_main.o main.o
# nm main.o | grep set_reset_devices
00000000000000a8 t __setup_set_reset_devices
000000000000105f t __setup_str_set_reset_devices
0000000000000000 t set_reset_devices

Now we have a section in main.o called __mcount_loc that we can place somewhere in the kernel using vmlinux.ld.S and access it to convert all these locations that call mcount into nops before starting SMP and thus, eliminating the need to do this with kstop_machine.

Note, A well documented perl script (scripts/recordmcount.pl) is used to do all this in one location.

---
 include/asm-generic/vmlinux.lds.h |    8 +
 kernel/trace/Kconfig              |    8 +
 scripts/Makefile.build            |    6 
 scripts/recordmcount.pl           |  280 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 302 insertions(+)
include/asm-generic/vmlinux.lds.h +8 -0
Index: linux-tip.git/include/asm-generic/vmlinux.lds.h
===================================================================
@@ -37,6 +37,13 @@
 #define MEM_DISCARD(sec) *(.mem##sec)
 #endif
 
+#ifdef CONFIG_FTRACE_MCOUNT_RECORD
+#define MCOUNT_REC()	VMLINUX_SYMBOL(__start_mcount_loc) = .; \
+			*(__mcount_loc)				\
+			VMLINUX_SYMBOL(__stop_mcount_loc) = .;
+#else
+#define MCOUNT_REC()
+#endif
 
 /* .data section */
 #define DATA_DATA							\
@@ -192,6 +199,7 @@
 	/* __*init sections */						\
 	__init_rodata : AT(ADDR(__init_rodata) - LOAD_OFFSET) {		\
 		*(.ref.rodata)						\
+		MCOUNT_REC()						\
 		DEV_KEEP(init.rodata)					\
 		DEV_KEEP(exit.rodata)					\
 		CPU_KEEP(init.rodata)					\
scripts/Makefile.build +6 -0
Index: linux-tip.git/scripts/Makefile.build
===================================================================
@@ -198,10 +198,16 @@ cmd_modversions =							\
 	fi;
 endif
 
+ifdef CONFIG_FTRACE_MCOUNT_RECORD
+cmd_record_mcount = scripts/recordmcount.pl "$(ARCH)" \
+	"$(OBJDUMP)" "$(OBJCOPY)" "$(CC)" "$(LD)" "$(NM)" "$(RM)" "$(MV)" "$(@)";
+endif
+
 define rule_cc_o_c
 	$(call echo-cmd,checksrc) $(cmd_checksrc)			  \
 	$(call echo-cmd,cc_o_c) $(cmd_cc_o_c);				  \
 	$(cmd_modversions)						  \
+	$(cmd_record_mcount)						  \
 	scripts/basic/fixdep $(depfile) $@ '$(call make-cmd,cc_o_c)' >    \
 	                                              $(dot-target).tmp;  \
 	rm -f $(depfile);						  \
scripts/recordmcount.pl new file +280 -0

280 lines not on this page · raw message

kernel/trace/Kconfig +8 -0
Index: linux-tip.git/kernel/trace/Kconfig
===================================================================
@@ -7,6 +7,9 @@ config HAVE_FTRACE
 config HAVE_DYNAMIC_FTRACE
 	bool
 
+config HAVE_FTRACE_MCOUNT_RECORD
+	bool
+
 config TRACER_MAX_TRACE
 	bool
 
@@ -122,6 +125,11 @@ config DYNAMIC_FTRACE
 	 were made. If so, it runs stop_machine (stops all CPUS)
 	 and modifies the code to jump over the call to ftrace.
 
+config FTRACE_MCOUNT_RECORD
+	def_bool y
+	depends on DYNAMIC_FTRACE
+	depends on HAVE_FTRACE_MCOUNT_RECORD
+
 config FTRACE_SELFTEST
 	bool
 
author maintainer
#2 lore raw

PATCH v2 2/6 ftrace: mcount call site on boot nops core

In reply to Steven Rostedt

This is the infrastructure to the converting the mcount call sites recorded by the __mcount_loc section into nops on boot. It also allows for using these sites to enable tracing as normal. When the __mcount_loc section is used, the "ftraced" kernel thread is disabled.

This uses the current infrastructure to record the mcount call sites as well as convert them to nops. The mcount function is kept as a stub on boot up and not converted to the ftrace_record_ip function. We use the ftrace_record_ip to only record from the table.

This patch does not handle modules. That comes with a later patch.

---
 include/asm-x86/ftrace.h |   10 +++
 include/linux/ftrace.h   |    6 +
 init/main.c              |    3 
 kernel/trace/ftrace.c    |  148 +++++++++++++++++++++++++++++++++--------------
 4 files changed, 124 insertions(+), 43 deletions(-)
include/asm-x86/ftrace.h +10 -0
Index: linux-tip.git/include/asm-x86/ftrace.h
===================================================================
@@ -7,6 +7,16 @@
 
 #ifndef __ASSEMBLY__
 extern void mcount(void);
+
+static inline unsigned long ftrace_call_adjust(unsigned long addr)
+{
+	/*
+	 * call mcount is "e8 <4 byte offset>"
+	 * The addr points to the 4 byte offset and the caller of this
+	 * function wants the pointer to e8. Simply subtract one.
+	 */
+	return addr - 1;
+}
 #endif
 
 #endif /* CONFIG_FTRACE */
include/linux/ftrace.h +6 -0
Index: linux-tip.git/include/linux/ftrace.h
===================================================================
@@ -141,4 +141,10 @@ static inline void
 ftrace_special(unsigned long arg1, unsigned long arg2, unsigned long arg3) { }
 #endif
 
+#ifdef CONFIG_FTRACE_MCOUNT_RECORD
+extern void ftrace_init(void);
+#else
+static inline void ftrace_init(void) { }
+#endif
+
 #endif /* _LINUX_FTRACE_H */
init/main.c +3 -0
Index: linux-tip.git/init/main.c
===================================================================
@@ -62,6 +62,7 @@
 #include <linux/signal.h>
 #include <linux/idr.h>
 #include <linux/kmemcheck.h>
+#include <linux/ftrace.h>
 
 #include <asm/io.h>
 #include <asm/bugs.h>
@@ -820,6 +821,8 @@ asmlinkage void __init start_kernel(void
 
 	acpi_early_init(); /* before LAPIC and SMP init */
 
+	ftrace_init();
+
 	/* Do the rest non-__init'ed, we're now alive */
 	rest_init();
 }
kernel/trace/ftrace.c +105 -43

178 lines not on this page · raw message

author maintainer
#3 lore raw

PATCH v2 3/6 ftrace: enable mcount recording for modules

In reply to Steven Rostedt

This patch enables the loading of the __mcount_section of modules and changing all the callers of mcount into nops.

The modification is done before the init_module function is called, so again, we do not need to use kstop_machine to make these changes.

---
 include/linux/ftrace.h |    3 +++
 kernel/module.c        |   11 +++++++++++
 kernel/trace/ftrace.c  |    5 +++++
 3 files changed, 19 insertions(+)
include/linux/ftrace.h +3 -0
Index: linux-tip.git/include/linux/ftrace.h
===================================================================
@@ -143,8 +143,11 @@ ftrace_special(unsigned long arg1, unsig
 
 #ifdef CONFIG_FTRACE_MCOUNT_RECORD
 extern void ftrace_init(void);
+extern void ftrace_init_module(unsigned long *start, unsigned long *end);
 #else
 static inline void ftrace_init(void) { }
+static inline void
+ftrace_init_module(unsigned long *start, unsigned long *end) { }
 #endif
 
 #endif /* _LINUX_FTRACE_H */
kernel/module.c +11 -0
Index: linux-tip.git/kernel/module.c
===================================================================
@@ -48,6 +48,7 @@
 #include <linux/license.h>
 #include <asm/sections.h>
 #include <linux/tracepoint.h>
+#include <linux/ftrace.h>
 
 #if 0
 #define DEBUGP printk
@@ -1835,6 +1836,7 @@ static struct module *load_module(void _
 	unsigned int markersstringsindex;
 	unsigned int tracepointsindex;
 	unsigned int tracepointsstringsindex;
+	unsigned int mcountindex;
 	struct module *mod;
 	long err = 0;
 	void *percpu = NULL, *ptr = NULL; /* Stops spurious gcc warning */
@@ -2125,6 +2127,9 @@ static struct module *load_module(void _
 	tracepointsstringsindex = find_sec(hdr, sechdrs, secstrings,
 					"__tracepoints_strings");
 
+	mcountindex = find_sec(hdr, sechdrs, secstrings,
+			       "__mcount_loc");
+
 	/* Now do relocations. */
 	for (i = 1; i < hdr->e_shnum; i++) {
 		const char *strtab = (char *)sechdrs[strindex].sh_addr;
@@ -2185,6 +2190,12 @@ static struct module *load_module(void _
 			mod->tracepoints + mod->num_tracepoints);
 #endif
 	}
+
+	if (mcountindex) {
+		void *mseg = (void *)sechdrs[mcountindex].sh_addr;
+		ftrace_init_module(mseg, mseg + sechdrs[mcountindex].sh_size);
+	}
+
 	err = module_finalize(hdr, sechdrs, mod);
 	if (err < 0)
 		goto cleanup;
kernel/trace/ftrace.c +5 -0
Index: linux-tip.git/kernel/trace/ftrace.c
===================================================================
@@ -1548,6 +1548,11 @@ static int ftrace_convert_nops(unsigned 
 	return 0;
 }
 
+void ftrace_init_module(unsigned long *start, unsigned long *end)
+{
+	ftrace_convert_nops(start, end);
+}
+
 extern unsigned long __start_mcount_loc[];
 extern unsigned long __stop_mcount_loc[];
 
#4 lore raw

Re: [PATCH v2 3/6] ftrace: enable mcount recording for modules

Quoted 2 lines · On Friday 15 August 2008 05:45:09 Steven Rostedt wrote:

This patch enables the loading of the __mcount_section of modules and changing all the callers of mcount into nops.

...

kernel/module.c static struct module *load_module(void _ · line 2196

Quoted 3 lines
kernel/module.c +3 -0
+	if (mcountindex) {
+		void *mseg = (void *)sechdrs[mcountindex].sh_addr;
+		ftrace_init_module(mseg, mseg + sechdrs[mcountindex].sh_size);

You don't actually need the if. The size of section 0 is 0.

Rusty.

#5 lore raw

Re: [PATCH v2 3/6] ftrace: enable mcount recording for modules

Quoted 5 lines · Rusty Russell
Quoted 3 lines · On Friday 15 August 2008 05:45:09 Steven Rostedt wrote:

This patch enables the loading of the __mcount_section of modules and changing all the callers of mcount into nops.

Quoted 9 lines

...

kernel/module.c static struct module *load_module(void _ · line 2196

Quoted 4 lines
kernel/module.c +3 -0
+	if (mcountindex) {
+		void *mseg = (void *)sechdrs[mcountindex].sh_addr;
+		ftrace_init_module(mseg, mseg + sechdrs[mcountindex].sh_size);

You don't actually need the if. The size of section 0 is 0.

Thanks,

But I find the if is a bit more clear. It may confuse code reviewers if we expect them to know that sechdrs[0].sh_size is zero. This isn't a hot path.

-- Steve

author maintainer
#6 lore raw

PATCH v2 4/6 ftrace: rebuild everything on change to FTRACE_MCOUNT_RECORD

In reply to Steven Rostedt

When enabling or disabling CONFIG_FTRACE_MCOUNT_RECORD, we want a full kernel compile to handle the adding of the __mcount_loc sections.

---
 include/linux/kernel.h |    5 +++++
 1 file changed, 5 insertions(+)
include/linux/kernel.h +5 -0
Index: linux-tip.git/include/linux/kernel.h
===================================================================
@@ -489,4 +489,9 @@ struct sysinfo {
 #define NUMA_BUILD 0
 #endif
 
+/* Rebuild everything on CONFIG_FTRACE_MCOUNT_RECORD */
+#ifdef CONFIG_FTRACE_MCOUNT_RECORD
+# define REBUILD_DUE_TO_FTRACE_MCOUNT_RECORD
+#endif
+
 #endif
author maintainer
#7 lore raw

PATCH v2 5/6 ftrace: enable using mcount recording on x86

In reply to Steven Rostedt

Enable the use of the __mcount_loc infrastructure on x86_64 and i386.

---
 arch/x86/Kconfig |    1 +
 1 file changed, 1 insertion(+)
arch/x86/Kconfig +1 -0
Index: linux-tip.git/arch/x86/Kconfig
===================================================================
@@ -25,6 +25,7 @@ config X86
 	select HAVE_KPROBES
 	select ARCH_WANT_OPTIONAL_GPIOLIB
 	select HAVE_KRETPROBES
+	select HAVE_FTRACE_MCOUNT_RECORD
 	select HAVE_DYNAMIC_FTRACE
 	select HAVE_FTRACE
 	select HAVE_KVM if ((X86_32 && !X86_VOYAGER && !X86_VISWS && !X86_NUMAQ) || X86_64)
author maintainer
#8 lore raw

PATCH v2 6/6 ftrace: x86 mcount stub

In reply to Steven Rostedt

x86 now sets up the mcount locations through the build and no longer needs to record the ip when the function is executed. This patch changes the initial mcount to simply return. There's no need to do any other work. If the ftrace start up test fails, the original mcount will be what everything will use, so having this as fast as possible is a good thing.

---
 arch/x86/kernel/entry_32.S |   14 --------------
 arch/x86/kernel/entry_64.S |   26 --------------------------
 arch/x86/kernel/ftrace.c   |   14 ++------------
 3 files changed, 2 insertions(+), 52 deletions(-)
arch/x86/kernel/entry_32.S +0 -14
Index: linux-tip.git/arch/x86/kernel/entry_32.S
===================================================================
@@ -1189,20 +1189,6 @@ ENDPROC(xen_failsafe_callback)
 #ifdef CONFIG_DYNAMIC_FTRACE
 
 ENTRY(mcount)
-	pushl %eax
-	pushl %ecx
-	pushl %edx
-	movl 0xc(%esp), %eax
-	subl $MCOUNT_INSN_SIZE, %eax
-
-.globl mcount_call
-mcount_call:
-	call ftrace_stub
-
-	popl %edx
-	popl %ecx
-	popl %eax
-
 	ret
 END(mcount)
 
arch/x86/kernel/entry_64.S +0 -26
Index: linux-tip.git/arch/x86/kernel/entry_64.S
===================================================================
@@ -64,32 +64,6 @@
 #ifdef CONFIG_FTRACE
 #ifdef CONFIG_DYNAMIC_FTRACE
 ENTRY(mcount)
-
-	subq $0x38, %rsp
-	movq %rax, (%rsp)
-	movq %rcx, 8(%rsp)
-	movq %rdx, 16(%rsp)
-	movq %rsi, 24(%rsp)
-	movq %rdi, 32(%rsp)
-	movq %r8, 40(%rsp)
-	movq %r9, 48(%rsp)
-
-	movq 0x38(%rsp), %rdi
-	subq $MCOUNT_INSN_SIZE, %rdi
-
-.globl mcount_call
-mcount_call:
-	call ftrace_stub
-
-	movq 48(%rsp), %r9
-	movq 40(%rsp), %r8
-	movq 32(%rsp), %rdi
-	movq 24(%rsp), %rsi
-	movq 16(%rsp), %rdx
-	movq 8(%rsp), %rcx
-	movq (%rsp), %rax
-	addq $0x38, %rsp
-
 	retq
 END(mcount)
 
arch/x86/kernel/ftrace.c +2 -12
Index: linux-tip.git/arch/x86/kernel/ftrace.c
===================================================================
@@ -112,18 +112,8 @@ notrace int ftrace_update_ftrace_func(ft
 
 notrace int ftrace_mcount_set(unsigned long *data)
 {
-	unsigned long ip = (long)(&mcount_call);
-	unsigned long *addr = data;
-	unsigned char old[MCOUNT_INSN_SIZE], *new;
-
-	/*
-	 * Replace the mcount stub with a pointer to the
-	 * ip recorder function.
-	 */
-	memcpy(old, &mcount_call, MCOUNT_INSN_SIZE);
-	new = ftrace_call_replace(ip, *addr);
-	*addr = ftrace_modify_code(ip, old, new);
-
+	/* mcount is initialized as a nop */
+	*data = 0;
 	return 0;
 }
 
#9 lore raw
In reply to Steven Rostedt
Quoted 25 lines · Steven Rostedt

[
Changes since v1:

regex fix in x86_64 recordmcount.pl. Now it can handle all
mcount+0x... mcount-0x... and mcount, where as the original only handled mcount+0x...

Made mcount on start-up to simply return. The current mcount
is set up to be replaced with a call to ftrace_record_ip. This is no longer necessary.

Note: This patch series is focusing on how calls to mcount in
the kernel are converted to nops. It does not address what kind of nop is used. That is a different topic, and should be in a different patch series.

Note 2: I have found that the changes here are more stable than
the current daemon method, and these patches should be used. It also solves the resume from suspend to ram bug that was reported:

http://lkml.org/lkml/2008/8/12/234
and
http://lkml.org/lkml/2008/8/12/451

Quoted 8 lines

Note 3: I have already ported this to PowerPC64, but I am waiting
for this to be accepted first before submitting those changes.
]

One of the things that bothered me about the latest ftrace code was this annoying daemon that would wake up once a second to see if it had work to do. If it did not, it would go to sleep, otherwise it would do its work and then go to sleep.

i like this concept alot - i've applied the whole lot to tip/tracing/ftrace.

Eventually gcc should be extended to provide a separate section for instrumentation patch sites, instead of us having to disassemble the object code. That would also make the ftrace build faster. Any gcc folks interested in that?

Ingo

#10 lore raw

(From the original mail)

Quoted 6 lines · Steven Rostedt

Along came Gregory Haskins, who was bickering about having ftrace enabled on a production -rt kernel. I told him the reasons that this would be bad and then he started thinking out loud, and suggesting wild ideas, like patching gcc!

^^^^^^^^^^

Quoted 1 line · Ingo Molnar

Eventually gcc should be extended to provide a separate section for instrumentation patch sites, instead of us having to disassemble the object code.

:)

I obviously agree with this, so +1

Though, tbh, at the time I suggested it I didn't think of Steve's idea to post-process which was quite clever. But I do agree that having gcc do it will probably save some build time since it will probably be trivial for it to do this when already processing -pg. It would have the added benefit of letting the arch specific toolchain do the arch specific work (though I think Steve's solution capitalizes on the toolchain extensively as it is).

The biggest downside is that we would have an external dependency on gcc for the feature, but I guess the kernel already has some of those anyway (e.g. the stack overflow guard feature, etc). We could always fall back on Steve's post-processing if the toolchain lacks the feature.

-Greg

#11 lore raw
In reply to Steven Rostedt
Quoted 8 lines · Steven Rostedt

Note 2: I have found that the changes here are more stable than
the current daemon method, and these patches should be used. It also solves the resume from suspend to ram bug that was reported:

http://lkml.org/lkml/2008/8/12/234
and
http://lkml.org/lkml/2008/8/12/451

I don't remember testing any patch for my bug... ;)

For which tree this patchset is?
I tried to apply it on rc3, rc3+tip/tip and rc3+tip/master and all failed.

Marcin

author maintainer
#12 lore raw
Quoted 6 lines · Marcin Slusarz
Quoted 5 lines · Steven Rostedt

Note 2: I have found that the changes here are more stable than
the current daemon method, and these patches should be used. It also solves the resume from suspend to ram bug that was reported:

Quoted 9 lines
Quoted 3 lines
      http://lkml.org/lkml/2008/8/12/234
       and
      http://lkml.org/lkml/2008/8/12/451

I don't remember testing any patch for my bug... ;)

For which tree this patchset is?
I tried to apply it on rc3, rc3+tip/tip and rc3+tip/master and all failed.

Ah, Bruce reported the same error, and said this patch series fixed it. I assumed that this would fix your bug too. But if it does not, then I need to look deeper into what you are reporting.

Could you recheck-out the latest tip/master and see if it works for you.

Thanks,

-- Steve

#13 lore raw
Quoted 19 lines · Steven Rostedt
Quoted 6 lines · Marcin Slusarz
Quoted 5 lines · Steven Rostedt

Note 2: I have found that the changes here are more stable than
the current daemon method, and these patches should be used. It also solves the resume from suspend to ram bug that was reported:

Quoted 9 lines
Quoted 3 lines
      http://lkml.org/lkml/2008/8/12/234
       and
      http://lkml.org/lkml/2008/8/12/451

I don't remember testing any patch for my bug... ;)

For which tree this patchset is?
I tried to apply it on rc3, rc3+tip/tip and rc3+tip/master and all failed.

Quoted 5 lines

Ah, Bruce reported the same error, and said this patch series fixed it. I assumed that this would fix your bug too. But if it does not, then I need to look deeper into what you are reporting.

Could you recheck-out the latest tip/master and see if it works for you.

Ok, I tried tip/master and resume from suspend to ram works again. Thanks!

Marcin

#14 lore raw
Quoted 20 lines · Marcin Slusarz
Quoted 19 lines · Steven Rostedt
Quoted 15 lines · Marcin Slusarz
Quoted 8 lines · Steven Rostedt

Note 2: I have found that the changes here are more stable than
the current daemon method, and these patches should be used. It also solves the resume from suspend to ram bug that was reported:

http://lkml.org/lkml/2008/8/12/234
and
http://lkml.org/lkml/2008/8/12/451

I don't remember testing any patch for my bug... ;)

For which tree this patchset is?
I tried to apply it on rc3, rc3+tip/tip and rc3+tip/master and all failed.

Quoted 8 lines
Quoted 5 lines

Ah, Bruce reported the same error, and said this patch series fixed it. I assumed that this would fix your bug too. But if it does not, then I need to look deeper into what you are reporting.

Could you recheck-out the latest tip/master and see if it works for you.

Ok, I tried tip/master and resume from suspend to ram works again. Thanks!

I assume this patchset won't go into .27. So is there any plan to fix it in .27?

Marcin

author maintainer
#15 lore raw
Quoted 25 lines · Marcin Slusarz
Quoted 24 lines · Marcin Slusarz
Quoted 23 lines · Steven Rostedt
Quoted 6 lines · Marcin Slusarz
Quoted 5 lines · Steven Rostedt

Note 2: I have found that the changes here are more stable than
the current daemon method, and these patches should be used. It also solves the resume from suspend to ram bug that was reported:

Quoted 9 lines
Quoted 3 lines
      http://lkml.org/lkml/2008/8/12/234
       and
      http://lkml.org/lkml/2008/8/12/451

I don't remember testing any patch for my bug... ;)

For which tree this patchset is?
I tried to apply it on rc3, rc3+tip/tip and rc3+tip/master and all failed.

Ah, Bruce reported the same error, and said this patch series fixed it. I assumed that this would fix your bug too. But if it does not, then I need to look deeper into what you are reporting.

Quoted 7 lines
Quoted 4 lines
Quoted 1 line

Could you recheck-out the latest tip/master and see if it works for you.

Ok, I tried tip/master and resume from suspend to ram works again. Thanks!

I assume this patchset won't go into .27. So is there any plan to fix it in .27?

I'm hoping that this does go into 27, since it is more reliable than the current daemon approach.

-- Steve

#16 lore raw
Quoted 5 lines · Steven Rostedt
Quoted 2 lines

I assume this patchset won't go into .27. So is there any plan to fix it in .27?

I'm hoping that this does go into 27, since it is more reliable than the current daemon approach.

it cannot really go into .27 - there's a lot of ftrace stuff piled up in tip/tracing/ftrace. (the script is also pretty hacky and unproven. All this stuff was written after the merge window.)

Ingo