[PATCH 2/3] ftrace/x86: Add support for C version of recordmcount

[PATCH 2/3] ftrace/x86: Add support for C version of recordmcount

[Steven Rostedt]

[-- Attachment #1: 0002-ftrace-x86-Add-support-for-C-version-of-recordmcount.patch --]
[-- Type: text/plain, Size: 2978 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

This patch adds the support for the C version of recordmcount and
compile times show ~ 12% improvement.

After verifying this works, other archs can add:

 HAVE_C_MCOUNT_RECORD

in its Kconfig and it will use the C version of recordmcount
instead of the perl version.

Cc: <linux-arch@vger.kernel.org>
Cc: Michal Marek <mmarek@suse.cz>
Cc: linux-kbuild@vger.kernel.org
Cc: John Reiser <jreiser@bitwagon.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 Makefile               |    6 ++++++
 arch/x86/Kconfig       |    1 +
 kernel/trace/Kconfig   |    5 +++++
 scripts/Makefile       |    1 +
 scripts/Makefile.build |    4 ++++
 5 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/Makefile b/Makefile
index 534c09c..0dd3a8d 100644
--- a/Makefile
+++ b/Makefile
@@ -568,6 +568,12 @@ endif
 
 ifdef CONFIG_FUNCTION_TRACER
 KBUILD_CFLAGS	+= -pg
+ifdef CONFIG_DYNAMIC_FTRACE
+	ifdef CONFIG_HAVE_C_MCOUNT_RECORD
+		BUILD_C_RECORDMCOUNT := y
+		export BUILD_C_RECORDMCOUNT
+	endif
+endif
 endif
 
 # We trigger additional mismatches with less inlining
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index c14d8b4..788b50e 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -33,6 +33,7 @@ config X86
 	select HAVE_KRETPROBES
 	select HAVE_OPTPROBES
 	select HAVE_FTRACE_MCOUNT_RECORD
+	select HAVE_C_MCOUNT_RECORD
 	select HAVE_DYNAMIC_FTRACE
 	select HAVE_FUNCTION_TRACER
 	select HAVE_FUNCTION_GRAPH_TRACER
diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index 538501c..df00fbb 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -49,6 +49,11 @@ config HAVE_SYSCALL_TRACEPOINTS
 	help
 	  See Documentation/trace/ftrace-design.txt
 
+config HAVE_C_MCOUNT_RECORD
+	bool
+	help
+	  C version of recordmcount available?
+
 config TRACER_MAX_TRACE
 	bool
 
diff --git a/scripts/Makefile b/scripts/Makefile
index 842dbc2..2e08810 100644
--- a/scripts/Makefile
+++ b/scripts/Makefile
@@ -11,6 +11,7 @@ hostprogs-$(CONFIG_KALLSYMS)     += kallsyms
 hostprogs-$(CONFIG_LOGO)         += pnmtologo
 hostprogs-$(CONFIG_VT)           += conmakehash
 hostprogs-$(CONFIG_IKCONFIG)     += bin2c
+hostprogs-$(BUILD_C_RECORDMCOUNT) += recordmcount
 
 always		:= $(hostprogs-y) $(hostprogs-m)
 
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index a1a5cf9..4d03a7e 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -209,12 +209,16 @@ cmd_modversions =								\
 endif
 
 ifdef CONFIG_FTRACE_MCOUNT_RECORD
+ifdef BUILD_C_RECORDMCOUNT
+cmd_record_mcount = $(srctree)/scripts/recordmcount "$(@)";
+else
 cmd_record_mcount = set -e ; perl $(srctree)/scripts/recordmcount.pl "$(ARCH)" \
 	"$(if $(CONFIG_CPU_BIG_ENDIAN),big,little)" \
 	"$(if $(CONFIG_64BIT),64,32)" \
 	"$(OBJDUMP)" "$(OBJCOPY)" "$(CC)" "$(LD)" "$(NM)" "$(RM)" "$(MV)" \
 	"$(if $(part-of-module),1,0)" "$(@)";
 endif
+endif
 
 define rule_cc_o_c
 	$(call echo-cmd,checksrc) $(cmd_checksrc)			  \
-- 
1.7.1

Re: [PATCH 2/3] ftrace/x86: Add support for C version of recordmcount

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> From: Steven Rostedt <srostedt@redhat.com>
> 
> This patch adds the support for the C version of recordmcount and
> compile times show ~ 12% improvement.

I reported this recordmcount performance problem 2 years ago. Better 
later than never i guess.

> +ifdef CONFIG_DYNAMIC_FTRACE
> +	ifdef CONFIG_HAVE_C_MCOUNT_RECORD
> +		BUILD_C_RECORDMCOUNT := y
> +		export BUILD_C_RECORDMCOUNT

> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -33,6 +33,7 @@ config X86
>  	select HAVE_KRETPROBES
>  	select HAVE_OPTPROBES
>  	select HAVE_FTRACE_MCOUNT_RECORD
> +	select HAVE_C_MCOUNT_RECORD

The naming is inconsistent here - it should be HAVE_C_RECORDMCOUNT, like 
the build variable has, and like the utility is called. If we are going 
to add this flag to most architectures we should name it consistently.

Thanks,

	Ingo

Re: [PATCH 2/3] ftrace/x86: Add support for C version of recordmcount

[Steven Rostedt]

On Fri, 2010-10-15 at 04:50 +0200, Ingo Molnar wrote:
> * Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > From: Steven Rostedt <srostedt@redhat.com>
> > 
> > This patch adds the support for the C version of recordmcount and
> > compile times show ~ 12% improvement.
> 
> I reported this recordmcount performance problem 2 years ago. Better 
> later than never i guess.

And I also remember saying after I posted this code that it would have a
compile time performance hit. Heck, it's a perl script running on every
object file. It was obvious what was at issue here. But it's better to
slow down the kernel build than to brick network cards. Also, perl was
much easier to do.

That said, the embarrassing thing is not that I knew (or you reported
it) about this performance problem. I'm actually quite embarrassed that
I had this code sitting in my inbox for over a year. I just kept having
other things that were more important coming up than lowering the
compile time of the kernel. Although, I did work to get streamline
config to offset this performance hit.

Finally, while at the End Users Summit, I decided to take a look at
John's code, and I was quite impressed.

But as you said, better late than never.


> 
> > +ifdef CONFIG_DYNAMIC_FTRACE
> > +	ifdef CONFIG_HAVE_C_MCOUNT_RECORD
> > +		BUILD_C_RECORDMCOUNT := y
> > +		export BUILD_C_RECORDMCOUNT
> 
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -33,6 +33,7 @@ config X86
> >  	select HAVE_KRETPROBES
> >  	select HAVE_OPTPROBES
> >  	select HAVE_FTRACE_MCOUNT_RECORD
> > +	select HAVE_C_MCOUNT_RECORD
> 
> The naming is inconsistent here - it should be HAVE_C_RECORDMCOUNT, like 
> the build variable has, and like the utility is called. If we are going 
> to add this flag to most architectures we should name it consistently.

Sure, want me to rebase it or just write a patch on top of it?

-- Steve

Re: [PATCH 2/3] ftrace/x86: Add support for C version of recordmcount

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 2010-10-15 at 04:50 +0200, Ingo Molnar wrote:
> > * Steven Rostedt <rostedt@goodmis.org> wrote:
> > 
> > > From: Steven Rostedt <srostedt@redhat.com>
> > > 
> > > This patch adds the support for the C version of recordmcount and
> > > compile times show ~ 12% improvement.
> > 
> > I reported this recordmcount performance problem 2 years ago. Better 
> > later than never i guess.
> 
> And I also remember saying after I posted this code that it would have 
> a compile time performance hit. Heck, it's a perl script running on 
> every object file. It was obvious what was at issue here. But it's 
> better to slow down the kernel build than to brick network cards. 

Well, it's even better to do neither!

> Also, perl was much easier to do.

Lets write the whole kernel in perl and forget about performance ;-)

> That said, the embarrassing thing is not that I knew (or you reported 
> it) about this performance problem. I'm actually quite embarrassed 
> that I had this code sitting in my inbox for over a year. I just kept 
> having other things that were more important coming up than lowering 
> the compile time of the kernel. Although, I did work to get streamline 
> config to offset this performance hit.
> 
> Finally, while at the End Users Summit, I decided to take a look at 
> John's code, and I was quite impressed.
> 
> But as you said, better late than never.

Yeah. Note that as a maintainer i need to grumble when i see some 
not-so-good event - even if there's a happy resolution! Otherwise such 
cases would tend to creep up in frequency ;-)

> > > +ifdef CONFIG_DYNAMIC_FTRACE
> > > +	ifdef CONFIG_HAVE_C_MCOUNT_RECORD
> > > +		BUILD_C_RECORDMCOUNT := y
> > > +		export BUILD_C_RECORDMCOUNT
> > 
> > > --- a/arch/x86/Kconfig
> > > +++ b/arch/x86/Kconfig
> > > @@ -33,6 +33,7 @@ config X86
> > >  	select HAVE_KRETPROBES
> > >  	select HAVE_OPTPROBES
> > >  	select HAVE_FTRACE_MCOUNT_RECORD
> > > +	select HAVE_C_MCOUNT_RECORD
> > 
> > The naming is inconsistent here - it should be HAVE_C_RECORDMCOUNT, like 
> > the build variable has, and like the utility is called. If we are going 
> > to add this flag to most architectures we should name it consistently.
> 
> Sure, want me to rebase it or just write a patch on top of it?

Sure, patch on top would be fine.

Thanks,

	Ingo

Re: [PATCH 2/3] ftrace/x86: Add support for C version of recordmcount

[Steven Rostedt]

On Fri, 2010-10-15 at 05:18 +0200, Ingo Molnar wrote:
> * Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Fri, 2010-10-15 at 04:50 +0200, Ingo Molnar wrote:
> > > * Steven Rostedt <rostedt@goodmis.org> wrote:

> > Also, perl was much easier to do.
> 
> Lets write the whole kernel in perl and forget about performance ;-)

Shhh, you're letting people know about my evil secret agenda!


> > > 
> > > The naming is inconsistent here - it should be HAVE_C_RECORDMCOUNT, like 
> > > the build variable has, and like the utility is called. If we are going 
> > > to add this flag to most architectures we should name it consistently.
> > 
> > Sure, want me to rebase it or just write a patch on top of it?
> 
> Sure, patch on top would be fine.

OK, I'll add a patch on top.

Thanks,

-- Steve

Re: [PATCH 2/3] ftrace/x86: Add support for C version of recordmcount

[Paul Mundt]

On Thu, Oct 14, 2010 at 05:00:16PM -0400, Steven Rostedt wrote:
> From: Steven Rostedt <srostedt@redhat.com>
> 
> This patch adds the support for the C version of recordmcount and
> compile times show ~ 12% improvement.
> 
> After verifying this works, other archs can add:
> 
>  HAVE_C_MCOUNT_RECORD
> 
> in its Kconfig and it will use the C version of recordmcount
> instead of the perl version.
> 
While I haven't had the chance to debug this yet, turning it on for SH
blows up immediately:

ftrace: allocating 15200 entries in 30 pages
------------[ cut here ]------------
WARNING: at /home/pmundt/devel/git/sh-2.6/kernel/trace/ftrace.c:1007
Modules linked in:

Pid : 0, Comm:           swapper
CPU : 0                  Not tainted  (2.6.36-05622-g38ab134-dirty #508)

PC is at ftrace_bug+0x78/0x23c
PR is at ftrace_bug+0x74/0x23c
PC  : 80064df4 SP  : 8056ff70 SR  : 400080f0 TEA : c0000004
R0  : 00000001 R1  : 00000001 R2  : 8064d862 R3  : 8056ff64
R4  : 805b47b4 R5  : 00000001 R6  : 00000000 R7  : 00000001
R8  : 803b15d8 R9  : 00000001 R10 : 9fc38be8 R11 : 00000000
R12 : 8064e88c R13 : 8064e880 R14 : 8056ff70
MACH: 00000000 MACL: 003d0900 GBR : 296e1678 PR  : 80064df0

Call trace:
 [<80066a86>] ftrace_process_locs+0x15a/0x284
 [<803b15d8>] dns_query+0x0/0x26c
 [<805f6a1a>] ftrace_init+0x112/0x1a8
 [<801deec0>] strlen+0x0/0x58
 [<8008f098>] get_zeroed_page+0x0/0x34
 [<805f0918>] start_kernel+0x3e0/0x480
 [<801deec0>] strlen+0x0/0x58
 [<801eb388>] debug_smp_processor_id+0x0/0xe4
 [<80002132>] _stext+0x132/0x140

Code:
  80064dee:  mov       r9, r5
  80064df0:  tst       r9, r9
  80064df2:  bt        80064df6
->80064df4:  trapa     #62
  80064df6:  bra       80064ef2
  80064df8:  mov       r9, r5
  80064dfa:  mov.l     80064f68 <ftrace_bug+0x1ec/0x23c>, r1  ! 8064d862 <__warned.27604+0x0/0x1>
  80064dfc:  mov.b     r2, @r1
  80064dfe:  mov.l     80064f48 <ftrace_bug+0x1cc/0x23c>, r1  ! 8057021c <ftrace_disabled+0x0/0x4>

---[ end trace 4eaa2a86a8e2da22 ]---
ftrace failed to modify [<803b15d8>] dns_query+0x0/0x26c
 actual: 02:d1:22:4f
Testing tracer nop: PASSED

Suggestions?

Re: [PATCH 2/3] ftrace/x86: Add support for C version of recordmcount

[John Reiser]

[Note that I have trimmed the addressees for this reply.]
On 10/26/2010, Paul Mundt wrote:

>> This patch adds the support for the C version of recordmcount and
>> compile times show ~ 12% improvement.

> While I haven't had the chance to debug this yet, turning it on for SH
> blows up immediately:
> 
> ftrace: allocating 15200 entries in 30 pages
> ------------[ cut here ]------------
> WARNING: at /home/pmundt/devel/git/sh-2.6/kernel/trace/ftrace.c:1007
> Modules linked in:
> 
> Pid : 0, Comm:           swapper
> CPU : 0                  Not tainted  (2.6.36-05622-g38ab134-dirty #508)
> 
> PC is at ftrace_bug+0x78/0x23c
> PR is at ftrace_bug+0x74/0x23c

> Call trace:
>  [<80066a86>] ftrace_process_locs+0x15a/0x284
>  [<803b15d8>] dns_query+0x0/0x26c

> Code:
>   80064dee:  mov       r9, r5
>   80064df0:  tst       r9, r9
>   80064df2:  bt        80064df6
> ->80064df4:  trapa     #62
>   80064df6:  bra       80064ef2

> ---[ end trace 4eaa2a86a8e2da22 ]---
> ftrace failed to modify [<803b15d8>] dns_query+0x0/0x26c
>  actual: 02:d1:22:4f
> Testing tracer nop: PASSED
> 
> Suggestions?


This looks like a disagreement over the interpretation of relocation type
R_SH_DIR32 in Elf32_Rela on a 32-bit EM_SH (SuperH) machine.

arch/sh/kernel/module.c says:
-----
                sym = (Elf32_Sym *)sechdrs[symindex].sh_addr
                        + ELF32_R_SYM(rel[i].r_info);
                relocation = sym->st_value + rel[i].r_addend;
   ...
                case R_SH_DIR32:
                        value = get_unaligned(location);
                        value += relocation;
                        put_unaligned(value, location);
-----

Paul Mundt sent to me an actual kernel object file main.o compiled -pg for EM_SH.
According to 'readelf', the Perl script recordmcount.pl generates:
-----
Relocation section '.rela__mcount_loc' at offset 0x17ec8 contains 3 entries:
 Offset     Info    Type            Sym.Value  Sym. Name + Addend
00000000  00006a01 R_SH_DIR32        00000000   do_one_initcall + 0
00000004  00006a01 R_SH_DIR32        00000000   do_one_initcall + 0
00000008  00006a01 R_SH_DIR32        00000000   do_one_initcall + 0

Symbol table '.symtab' contains 198 entries:
   Num:    Value  Size Type    Bind   Vis      Ndx Name
   106: 00000000   364 FUNC    GLOBAL DEFAULT    1 do_one_initcall
###  (1==Ndx) ==> .text, so do_one_initcall is first in .text
###  106==0x6a

Section Headers:
  [Nr] Name              Type            Addr     Off    Size   ES Flg Lk Inf Al
  [ 1] .text             PROGBITS        00000000 000034 00026c 00  AX  0   0  4
  [17] __mcount_loc      PROGBITS        00000000 001140 00000c 00   A  0   0  4
  [18] .rela__mcount_loc RELA            00000000 017ec8 000024 0c     46  17  4
  [46] .symtab           SYMTAB          00000000 022ac4 000c60 10     47  71  4
-----
where section 17, __mcount_loc, has three 32-bit words with values 0xc, 0x178, 0x1b0.

Also according to 'readelf', the C program recordmcount generates:
-----
Relocation section '.rela__mcount_loc' at offset 0x249f4 contains 3 entries:
 Offset     Info    Type            Sym.Value  Sym. Name + Addend
00000000  00000201 R_SH_DIR32        00000000   .text + c
00000004  00000201 R_SH_DIR32        00000000   .text + 178
00000008  00000201 R_SH_DIR32        00000000   .text + 1b0

Symbol table '.symtab' contains 198 entries:
   Num:    Value  Size Type    Bind   Vis      Ndx Name
     2: 00000000     0 SECTION LOCAL  DEFAULT    1
### (1==Ndx) ==> .text

Section Headers:
  [Nr] Name              Type            Addr     Off    Size   ES Flg Lk Inf Al
  [ 1] .text             PROGBITS        00000000 000034 00026c 00  AX  0   0  4
  [44] .symtab           SYMTAB          00000000 017440 000c50 10     45  70  4
  [46] __mcount_loc      PROGBITS        00000000 0249e8 00000c 04   A  0   0  4
  [47] .rela__mcount_loc RELA            00000000 0249f4 000024 0c     44  46  4
-----
where section 46, __mcount_loc, has three 32-bit words with values 0, 0, 0.

Assuming the code in arch/sh/kernel/module.c, then I believe that those two
descriptions are equivalent.  The output of the Perl script calls for

   *location = *location + do_one_initcall.st_value + .r_addend

with  0==.r_addend  and initially  *location==offset .
The output of the C version calls for

   *location = *location + .text.st_value           + .r_addend

with  offset==.r_addend  and initially  *location==0.

I did find in binutils-2.18.50.0.9/bfd/elf32-sh.c this ominous comment:
-----
      switch ((int) r_type)
        {
        final_link_relocate:
          /* COFF relocs don't use the addend. The addend is used for
             R_SH_DIR32 to be compatible with other compilers.  */
          r = _bfd_final_link_relocate (howto, input_bfd, input_section,
                                        contents, rel->r_offset,
                                        relocation, addend);
-----

The natural question is, "Does /bin/ld for EM_SH ignore the .r_addend,
or does /bin/ld process .r_addend just like arch/sh/kernel/module.c ?"
If /bin/ld ignores .r_addend for R_SH_DIR32, as hinted in the comment,
then that would explain the crash.

Another question is, "Does the SECTION LOCAL symbol for .text have
.st_value equal to the virtual address of the start of the section?"
If a SECTION LOCAL symbol was not relocated like this, and thus had
the value 0, then that also would explain the crash.

That's my analysis.  If anyone can shed more light, then please do!

-- 
John Reiser, jreiser@BitWagon.com