Re: [for-next][PATCH 2/8] ftrace: Create a slight optimization on searching the ftrace_hash

[Namhyung Kim]

Hi Steve,

On Fri, Feb 3, 2017 at 10:40 PM, Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> From: "Steven Rostedt (VMware)" <rostedt@xxxxxxxxxxx>
>
> This is a micro-optimization, but as it has to deal with a fast path of the
> function tracer, these optimizations can be noticed.
>
> The ftrace_lookup_ip() returns true if the given ip is found in the hash. If
> it's not found or the hash is NULL, it returns false. But there's some cases
> that a NULL hash is a true, and the ftrace_hash_empty() is tested before
> calling ftrace_lookup_ip() in those cases. But as ftrace_lookup_ip() tests
> that first, that adds a few extra unneeded instructions in those cases.
>
> A new static "always_inlined" function is created that does not perform the
> hash empty test. This most only be used by callers that do the check first
> anyway, as an empty or NULL hash could cause a crash if a lookup is
> performed on it.
>
> Also add kernel doc for the ftrace_lookup_ip() main function.

It'd be nice if ftrace_graph_addr() was changed also.

Thanks,
Namhyung


>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@xxxxxxxxxxx>
> ---
>  kernel/trace/ftrace.c | 33 +++++++++++++++++++++++++--------
>  1 file changed, 25 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 89240f62061c..1595df0d7d79 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -1194,16 +1194,14 @@ ftrace_hash_key(struct ftrace_hash *hash, unsigned long ip)
>         return 0;
>  }
>
> -struct ftrace_func_entry *
> -ftrace_lookup_ip(struct ftrace_hash *hash, unsigned long ip)
> +/* Only use this function if ftrace_hash_empty() has already been tested */
> +static __always_inline struct ftrace_func_entry *
> +__ftrace_lookup_ip(struct ftrace_hash *hash, unsigned long ip)
>  {
>         unsigned long key;
>         struct ftrace_func_entry *entry;
>         struct hlist_head *hhd;
>
> -       if (ftrace_hash_empty(hash))
> -               return NULL;
> -
>         key = ftrace_hash_key(hash, ip);
>         hhd = &hash->buckets[key];
>
> @@ -1214,6 +1212,25 @@ ftrace_lookup_ip(struct ftrace_hash *hash, unsigned long ip)
>         return NULL;
>  }
>
> +/**
> + * ftrace_lookup_ip - Test to see if an ip exists in an ftrace_hash
> + * @hash: The hash to look at
> + * @ip: The instruction pointer to test
> + *
> + * Search a given @hash to see if a given instruction pointer (@ip)
> + * exists in it.
> + *
> + * Returns the entry that holds the @ip if found. NULL otherwise.
> + */
> +struct ftrace_func_entry *
> +ftrace_lookup_ip(struct ftrace_hash *hash, unsigned long ip)
> +{
> +       if (ftrace_hash_empty(hash))
> +               return NULL;
> +
> +       return __ftrace_lookup_ip(hash, ip);
> +}
> +
>  static void __add_hash_entry(struct ftrace_hash *hash,
>                              struct ftrace_func_entry *entry)
>  {
> @@ -1463,9 +1480,9 @@ static bool hash_contains_ip(unsigned long ip,
>          * notrace hash is considered not in the notrace hash.
>          */
>         return (ftrace_hash_empty(hash->filter_hash) ||
> -               ftrace_lookup_ip(hash->filter_hash, ip)) &&
> +               __ftrace_lookup_ip(hash->filter_hash, ip)) &&
>                 (ftrace_hash_empty(hash->notrace_hash) ||
> -                !ftrace_lookup_ip(hash->notrace_hash, ip));
> +                !__ftrace_lookup_ip(hash->notrace_hash, ip));
>  }
>
>  /*
> @@ -2877,7 +2894,7 @@ ops_references_rec(struct ftrace_ops *ops, struct dyn_ftrace *rec)
>
>         /* The function must be in the filter */
>         if (!ftrace_hash_empty(ops->func_hash->filter_hash) &&
> -           !ftrace_lookup_ip(ops->func_hash->filter_hash, rec->ip))
> +           !__ftrace_lookup_ip(ops->func_hash->filter_hash, rec->ip))
>                 return 0;
>
>         /* If in notrace hash, we ignore it too */
> --
> 2.10.2
>
>