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

From: Namhyung Kim
Date: Fri Feb 03 2017 - 09:27:22 EST


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
>
>