* [PATCH v2] ftrace: simplify ftrace hash lookup code @ 2019-09-09 0:31 Changbin Du 2019-09-09 14:54 ` Steven Rostedt 0 siblings, 1 reply; 6+ messages in thread From: Changbin Du @ 2019-09-09 0:31 UTC (permalink / raw) To: Steven Rostedt; +Cc: Ingo Molnar, linux-kernel, Changbin Du 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); } /* @@ -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 -- 2.20.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] ftrace: simplify ftrace hash lookup code 2019-09-09 0:31 [PATCH v2] ftrace: simplify ftrace hash lookup code Changbin Du @ 2019-09-09 14:54 ` Steven Rostedt 2019-09-09 23:39 ` Changbin Du 2019-09-10 0:33 ` Changbin Du 0 siblings, 2 replies; 6+ messages in thread From: Steven Rostedt @ 2019-09-09 14:54 UTC (permalink / raw) To: Changbin Du; +Cc: Ingo Molnar, linux-kernel 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 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? -- 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] ftrace: simplify ftrace hash lookup code 2019-09-09 14:54 ` Steven Rostedt @ 2019-09-09 23:39 ` Changbin Du 2019-09-10 0:33 ` Changbin Du 1 sibling, 0 replies; 6+ messages in thread From: Changbin Du @ 2019-09-09 23:39 UTC (permalink / raw) To: Steven Rostedt; +Cc: Changbin Du, Ingo Molnar, linux-kernel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] ftrace: simplify ftrace hash lookup code 2019-09-09 14:54 ` Steven Rostedt 2019-09-09 23:39 ` Changbin Du @ 2019-09-10 0:33 ` Changbin Du 2019-09-10 9:28 ` Steven Rostedt 1 sibling, 1 reply; 6+ messages in thread From: Changbin Du @ 2019-09-10 0:33 UTC (permalink / raw) To: Steven Rostedt; +Cc: Changbin Du, Ingo Molnar, linux-kernel 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 > > 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); > } > We must add another similar function since ftrace_lookup_ip() returns a pointer. bool ftrace_contains_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); } But after this, it's a little overkill I think. It is not much simpler than before. Do you still want this then? > 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? > > -- 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] ftrace: simplify ftrace hash lookup code 2019-09-10 0:33 ` Changbin Du @ 2019-09-10 9:28 ` Steven Rostedt 2019-09-10 14:29 ` Changbin Du 0 siblings, 1 reply; 6+ messages in thread From: Steven Rostedt @ 2019-09-10 9:28 UTC (permalink / raw) To: Changbin Du; +Cc: Ingo Molnar, linux-kernel On Tue, 10 Sep 2019 08:33:23 +0800 Changbin Du <changbin.du@gmail.com> wrote: > > > > 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); > > } > > > We must add another similar function since ftrace_lookup_ip() returns a pointer. > > bool ftrace_contains_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); > } > > But after this, it's a little overkill I think. It is not much simpler than before. > Do you still want this then? > > Or... static struct ftrace_func_entry empty_func_entry; #define EMPTY_FUNC_ENTRY = &empty_func_entry; [..] * @empty_result: return NULL if false or EMPTY_FUNC_ENTRY on true [..] * @empty_result should be false, unless this is used for testing if the ip * exists in the hash, and an empty hash should be considered true. * This is useful when the empty hash is considered to contain all addresses. [..] struct ftrace_func_entry * ftrace_lookup_ip(struct ftrace_hash *hash, unsigned long ip, bool empty_result) { if (ftrace_hash_empty(hash)) return empty_result ? EMPTY_FUNC_ENTRY : NULL; return __ftrace_lookup_ip(hash, ip); } But looking at this more, I'm going back to not touching the code in this location, because __ftrace_lookup_ip() is static, where as ftrace_lookup_ip() is not, and this is in a very fast path, and I rather keep it open coded. Lets just drop the first hunk of your patch. The second hunk is fine. -- Steve ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] ftrace: simplify ftrace hash lookup code 2019-09-10 9:28 ` Steven Rostedt @ 2019-09-10 14:29 ` Changbin Du 0 siblings, 0 replies; 6+ messages in thread From: Changbin Du @ 2019-09-10 14:29 UTC (permalink / raw) To: Steven Rostedt; +Cc: Changbin Du, Ingo Molnar, linux-kernel On Tue, Sep 10, 2019 at 05:28:04AM -0400, Steven Rostedt wrote: > On Tue, 10 Sep 2019 08:33:23 +0800 > Changbin Du <changbin.du@gmail.com> wrote: > > > > > > > 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); > > > } > > > > > We must add another similar function since ftrace_lookup_ip() returns a pointer. > > > > bool ftrace_contains_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); > > } > > > > But after this, it's a little overkill I think. It is not much simpler than before. > > Do you still want this then? > > > > > > Or... > > static struct ftrace_func_entry empty_func_entry; > #define EMPTY_FUNC_ENTRY = &empty_func_entry; > > [..] > * @empty_result: return NULL if false or EMPTY_FUNC_ENTRY on true > [..] > * @empty_result should be false, unless this is used for testing if the ip > * exists in the hash, and an empty hash should be considered true. > * This is useful when the empty hash is considered to contain all addresses. > [..] > struct ftrace_func_entry * > ftrace_lookup_ip(struct ftrace_hash *hash, unsigned long ip, bool empty_result) > { > if (ftrace_hash_empty(hash)) > return empty_result ? EMPTY_FUNC_ENTRY : NULL; > > return __ftrace_lookup_ip(hash, ip); > } > > But looking at this more, I'm going back to not touching the code in > this location, because __ftrace_lookup_ip() is static, where as > ftrace_lookup_ip() is not, and this is in a very fast path, and I > rather keep it open coded. > > Lets just drop the first hunk of your patch. The second hunk is fine. > Sure, I will send a update short later. Thanks for your suggestions. > > -- Steve -- Cheers, Changbin Du ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-09-10 14:29 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-09 0:31 [PATCH v2] ftrace: simplify ftrace hash lookup code Changbin Du 2019-09-09 14:54 ` Steven Rostedt 2019-09-09 23:39 ` Changbin Du 2019-09-10 0:33 ` Changbin Du 2019-09-10 9:28 ` Steven Rostedt 2019-09-10 14:29 ` Changbin Du
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).