Re: [PATCH v6 7/8] x86: patch all traced function calls using the int3-based framework

[Petr Mladek <pmladek@suse.cz>]

On Wed, 2014-01-22 at 14:20 +0100, Petr Mladek wrote:
> On Wed, 2014-01-15 at 10:47 -0500, Steven Rostedt wrote:
> > On Tue, 10 Dec 2013 16:42:19 +0100
> > Petr Mladek <pmladek@suse.cz> wrote:
> > 
> > > 
> > > diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> > > index 03abf9abf681..e5c02af3a8cc 100644
> > > --- a/arch/x86/kernel/alternative.c
> > > +++ b/arch/x86/kernel/alternative.c
> > > @@ -613,7 +613,7 @@ void __kprobes text_poke_or_die(void *addr, const void *opcode, size_t len)
> > >   * responsible for setting the patched code read-write. This is more effective
> > >   * only when you patch many addresses at the same time.
> > >   */
> > > -static int text_poke_part(void *addr, const void *opcode, size_t len)
> > > +static int notrace text_poke_part(void *addr, const void *opcode, size_t len)
> > 
> > I can understand why you want to not trace any of these, but again, I
> > hate using notrace unless it is absolutely necessary. That's what the
> > set_ftrace_notrace is for. If you don't want to trace it, simply tell
> > the kernel not to.
> 
> I removed "notrace" from all locations, except for "poke_int3_handler".
> Then I tried to switch between 7 tracers, enable and disable tracing,
> and repeated this 100 times. It slowed down from:
> 
> real    3m25.837s    3m29.280s    3m27.408s
> user    0m0.004s     0m0.004s     0m0.000s
> sys     0m3.532s     0m3.460s     0m3.416s
> 
> to 
> 
> real    5m23.294s    5m24.944s    5m28.056s
> user    0m0.004s     0m0.004s	  0m0.004s
> sys     0m3.076s     0m3.180s	  0m3.396s
> 
> It means a change by 60%. As you know indeed, the reason is that the
> functions used during patching are called via the int3 handler once the
> breakpoint is added.
> 
> I thought about patching these functions separately, e.g. using separate
> list or extra filters with two round patching. But I think that it is
> not worth the extra complexity.
> 
> I agree that it might be worth to keep the functions traceable. So, if
> the slowness is acceptable for you, I am fine with it as well.

I updated numbers in commit messages for the patchset v7. I am slightly
in doubts now. I wonder if the complex iterator-based text_poke_bp_list
is still worth the effort.

I tried to switch between 7 tracers: blk, branch, function_graph,
wakeup_rt, irqsoff, function, and nop. Every tracer has also been
enabled and disabled. I tested it on idle system with Intel(R) Core(TM)2
Duo CPU E8400  @ 3.00GHz. With 500 cycles I got these times:

1. original code (without the patch set):

real    18m4.545s    18m11.568s    18m14.396s
user    0m0.004s     0m0.008s	   0m0.004s
sys     0m17.176s    0m16.940s	   0m16.664s


2. with patchset v7 ; notrace used only for poke_int3_handler; the
   slowness is caused by heavy using poke_int3_handler during patching

real    27m11.690s    27m18.176s    27m13.844s
user    0m0.008s      0m0.008s	    0m0.008s
sys     0m15.916s     0m15.956s	    0m15.860s


3. using text_poke_bp instead of text_poke_bp_list; every address
   is patched separately; the slowness is caused by heavy using of
   run_sync()

real    34m8.042s    34m15.584s    34m5.008s
user    0m0.004s     0m0.004s	   0m0.004s
sys     0m16.296s    0m16.268s	   0m16.048s


It means that the slowness caused by tracing "text_poke*" stuff is
comparable with the slowness caused by "run_sync()". It might mean that
"text_poke_bp_list" is not worth the added complexity.

Well, the iterator-based implementation is complex but still faster.
Also the many sync calls might be more painful for a busy system that
heavy use of the int3 handler. So, "text_poke_bp_list" still might be
useful.


Best Regards,
Petr