LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Steven Rostedt <rostedt@goodmis.org>
To: Cheng Jian <cj.chengjian@huawei.com>
Cc: <linux-kernel@vger.kernel.org>, <huawei.libin@huawei.com>,
	<xiexiuqi@huawei.com>, <mingo@redhat.com>
Subject: Re: [PATCH] ftrace: enable trampoline when rec count decrement to one
Date: Sun, 5 May 2019 15:34:47 -0400	[thread overview]
Message-ID: <20190505153447.594d4eab@oasis.local.home> (raw)
In-Reply-To: <1556969979-111047-1-git-send-email-cj.chengjian@huawei.com>

On Sat, 4 May 2019 19:39:39 +0800
Cheng Jian <cj.chengjian@huawei.com> wrote:

> Trampoline can only be enabled if there is only a single ops
> attached to it. If there's only a single callback registered
> to a function, and the ops has a trampoline registered for it,
> then we can call the trampoline directly. This is very useful
> for improving the performance of ftrace and livepatch.
> 
> But we always disable trampoline when unregister ftrace. So if
> you have registered multiple ftrace_ops at the same location,
> even if the other ones have been unregistered, you will no longer
> be able to use trampoline.
> 
> To fix it, set FTRACE_FL_TRAMP flag if rec count is decremented
> to one, and the ops that left has a trampoline.
> 
> Testing After this patch :
> 
> insmod livepatch_unshare_files.ko
> cat /sys/kernel/debug/tracing/enabled_functions
> 
> 	unshare_files (1) R I	tramp: 0xffffffffc0000000(klp_ftrace_handler+0x0/0xa0) ->ftrace_ops_assist_func+0x0/0xf0
> 
> echo unshare_files > /sys/kernel/debug/tracing/set_ftrace_filter
> echo function > /sys/kernel/debug/tracing/current_tracer
> cat /sys/kernel/debug/tracing/enabled_functions
> 
> 	unshare_files (2) R I ->ftrace_ops_list_func+0x0/0x150
> 
> echo nop > /sys/kernel/debug/tracing/current_tracer
> cat /sys/kernel/debug/tracing/enabled_functions
> 
> 	unshare_files (1) R I	tramp: 0xffffffffc0000000(klp_ftrace_handler+0x0/0xa0) ->ftrace_ops_assist_func+0x0/0xf0

Thanks for the patch. There was some race condition that prevented me
from doing this in the first place, but unfortunately, I don't remember
what that was :-/

I'll have to think about this before applying this patch.

Maybe there isn't a race condition, and I was just playing it safe, as
there was a race condition between switching from regs caller back to
non regs caller.

-- Steve


> 
> Signed-off-by: Cheng Jian <cj.chengjian@huawei.com>
> ---
>  kernel/trace/ftrace.c | 28 +++++++++++++++-------------
>  1 file changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index b920358..bdc29c2 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -1626,6 +1626,11 @@ static bool test_rec_ops_needs_regs(struct
> dyn_ftrace *rec) return  keep_regs;
>  }
>  
> +static struct ftrace_ops *
> +ftrace_find_tramp_ops_any(struct dyn_ftrace *rec);
> +static struct ftrace_ops *
> +ftrace_find_tramp_ops_next(struct dyn_ftrace *rec, struct ftrace_ops
> *ops); +
>  static bool __ftrace_hash_rec_update(struct ftrace_ops *ops,
>  				     int filter_hash,
>  				     bool inc)
> @@ -1754,15 +1759,17 @@ static bool __ftrace_hash_rec_update(struct
> ftrace_ops *ops, }
>  
>  			/*
> -			 * If the rec had TRAMP enabled, then it
> needs to
> -			 * be cleared. As TRAMP can only be enabled
> iff
> -			 * there is only a single ops attached to it.
> -			 * In otherwords, always disable it on
> decrementing.
> -			 * In the future, we may set it if rec count
> is
> -			 * decremented to one, and the ops that is
> left
> -			 * has a trampoline.
> +			 * The TRAMP needs to be set only if rec
> count
> +			 * is decremented to one, and the ops that is
> +			 * left has a trampoline. As TRAMP can only
> be
> +			 * enabled if there is only a single ops
> attached
> +			 * to it.
>  			 */
> -			rec->flags &= ~FTRACE_FL_TRAMP;
> +			if (ftrace_rec_count(rec) == 1 &&
> +			    ftrace_find_tramp_ops_any(rec))
> +				rec->flags |= FTRACE_FL_TRAMP;
> +			else
> +				rec->flags &= ~FTRACE_FL_TRAMP;
>  
>  			/*
>  			 * flags will be cleared in
> ftrace_check_record() @@ -1955,11 +1962,6 @@ static void
> print_ip_ins(const char *fmt, const unsigned char *p)
> printk(KERN_CONT "%s%02x", i ? ":" : "", p[i]); }
>  
> -static struct ftrace_ops *
> -ftrace_find_tramp_ops_any(struct dyn_ftrace *rec);
> -static struct ftrace_ops *
> -ftrace_find_tramp_ops_next(struct dyn_ftrace *rec, struct ftrace_ops
> *ops); -
>  enum ftrace_bug_type ftrace_bug_type;
>  const void *ftrace_expected;
>  


  reply	other threads:[~2019-05-05 19:34 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-04 11:39 Cheng Jian
2019-05-05 19:34 ` Steven Rostedt [this message]
2019-05-08 15:02   ` chengjian (D)
2019-05-08 16:53     ` Steven Rostedt

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190505153447.594d4eab@oasis.local.home \
    --to=rostedt@goodmis.org \
    --cc=cj.chengjian@huawei.com \
    --cc=huawei.libin@huawei.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=xiexiuqi@huawei.com \
    --subject='Re: [PATCH] ftrace: enable trampoline when rec count decrement to one' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).