Messages in this thread | | | Date | Tue, 10 Sep 2019 07:39:58 +0800 | From | Changbin Du <> | Subject | Re: [PATCH v2] ftrace: simplify ftrace hash lookup code |
| |
On Mon, Sep 09, 2019 at 10:54:24AM -0400, Steven Rostedt wrote: > On Mon, 9 Sep 2019 08:31:59 +0800 > Changbin Du <changbin.du@gmail.com> wrote: > > > Function ftrace_lookup_ip() will check empty hash table. So we don't > > need extra check outside. > > > > Signed-off-by: Changbin Du <changbin.du@gmail.com> > > > > --- > > v2: fix incorrect code remove. > > --- > > kernel/trace/ftrace.c | 9 ++------- > > 1 file changed, 2 insertions(+), 7 deletions(-) > > > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > > index f9821a3374e9..92aab854d3b1 100644 > > --- a/kernel/trace/ftrace.c > > +++ b/kernel/trace/ftrace.c > > @@ -1463,8 +1463,7 @@ static bool hash_contains_ip(unsigned long ip, > > */ > > return (ftrace_hash_empty(hash->filter_hash) || > > __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); > > I don't care for this part. I've nacked this change in the past. Why? > let's compare the changes: > > return (ftrace_hash_empty(hash->filter_hash) || > __ftrace_lookup_ip(hash->filter_hash, ip)) && > (ftrace_hash_empty(hash->notrace_hash) || > !__ftrace_lookup_ip(hash->notrace_hash, ip)); > > vs: > > return (ftrace_hash_empty(hash->filter_hash) || > __ftrace_lookup_ip(hash->filter_hash, ip)) && > !ftrace_lookup_ip(hash->notrace_hash, ip); > > The issue I have with this is that it abstracts out the difference > between the filter_hash and the notrace_hash. Sometimes open coded > works better if it is compared to something that is similar. > > The current code I see: > > Return true if (filter_hash is empty or ip exists in filter_hash > and notrace_hash is empty or it does not exist in notrace_hash > > With your update I see: > > Return true if filter_hash is empty or ip exists in filter_hash > and ip does not exist in notrace_hash > > It makes it not easy to see if what happens if notrace_hash is empty > Yes, I agree with you entirly.
> Hmm, come to think of it, perhaps we should change ftrace_lookup_ip() > to include what to do on empty. > > Maybe: > > bool ftrace_lookup_ip(struct ftrace_hash *hash, unsigned long ip, bool empty_result) > { > if (ftrace_hash_empty(hash)) > return empty_result; > > return __ftrace_lookup_ip(hash, ip); > } > > Then we can change the above to: > > return ftrace_lookup_ip(hash->filter_hash, ip, true) && > !ftrace_lookup_ip(hash->notrace_hash, ip, false); > > That would probably work better. > > Want to send that update? > Yes, let me update it with your idea. Thanks!
> -- Steve > > > > } > > > > /* > > @@ -6036,11 +6035,7 @@ clear_func_from_hash(struct ftrace_init_func > > *func, struct ftrace_hash *hash) { > > struct ftrace_func_entry *entry; > > > > - if (ftrace_hash_empty(hash)) > > - return; > > - > > - entry = __ftrace_lookup_ip(hash, func->ip); > > - > > + entry = ftrace_lookup_ip(hash, func->ip); > > /* > > * Do not allow this rec to match again. > > * Yeah, it may waste some memory, but will be removed >
-- Cheers, Changbin Du
| |