From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from psmtp.com (na3sys010amx182.postini.com [74.125.245.182]) by kanga.kvack.org (Postfix) with SMTP id D086B6B0044 for ; Sun, 5 Aug 2012 13:02:31 -0400 (EDT) Received: by obhx4 with SMTP id x4so5503724obh.14 for ; Sun, 05 Aug 2012 10:02:31 -0700 (PDT) Message-ID: <501EA749.9060400@gmail.com> Date: Sun, 05 Aug 2012 19:03:05 +0200 From: Sasha Levin MIME-Version: 1.0 Subject: Re: [RFC v2 6/7] tracepoint: use new hashtable implementation References: <1344003788-1417-1-git-send-email-levinsasha928@gmail.com> <1344003788-1417-7-git-send-email-levinsasha928@gmail.com> <1344126994.27983.116.camel@gandalf.stny.rr.com> <20120805163114.GA21768@Krystal> In-Reply-To: <20120805163114.GA21768@Krystal> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: owner-linux-mm@kvack.org List-ID: To: Mathieu Desnoyers Cc: Steven Rostedt , torvalds@linux-foundation.org, tj@kernel.org, akpm@linux-foundation.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, paul.gortmaker@windriver.com, davem@davemloft.net, mingo@elte.hu, ebiederm@xmission.com, aarcange@redhat.com, ericvh@gmail.com, netdev@vger.kernel.org On 08/05/2012 06:31 PM, Mathieu Desnoyers wrote: > * Steven Rostedt (rostedt@goodmis.org) wrote: >> FYI, Mathieu is the author of this file. >> >> -- Steve >> >> >> On Fri, 2012-08-03 at 16:23 +0200, Sasha Levin wrote: >>> Switch tracepoints to use the new hashtable implementation. This reduces the amount of >>> generic unrelated code in the tracepoints. >>> >>> Signed-off-by: Sasha Levin >>> --- >>> kernel/tracepoint.c | 26 +++++++++----------------- >>> 1 files changed, 9 insertions(+), 17 deletions(-) >>> >>> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c >>> index d96ba22..b5a2650 100644 >>> --- a/kernel/tracepoint.c >>> +++ b/kernel/tracepoint.c >>> @@ -26,6 +26,7 @@ >>> #include >>> #include >>> #include >>> +#include >>> >>> extern struct tracepoint * const __start___tracepoints_ptrs[]; >>> extern struct tracepoint * const __stop___tracepoints_ptrs[]; >>> @@ -49,8 +50,7 @@ static LIST_HEAD(tracepoint_module_list); >>> * Protected by tracepoints_mutex. >>> */ >>> #define TRACEPOINT_HASH_BITS 6 >>> -#define TRACEPOINT_TABLE_SIZE (1 << TRACEPOINT_HASH_BITS) >>> -static struct hlist_head tracepoint_table[TRACEPOINT_TABLE_SIZE]; >>> +DEFINE_STATIC_HASHTABLE(tracepoint_table, TRACEPOINT_HASH_BITS); > > I wonder why the "static" has been embedded within > "DEFINE_STATIC_HASHTABLE" ? I'm used to see something similar to: > > static DEFINE_HASHTABLE(tracepoint_table, TRACEPOINT_HASH_BITS); > > elsewhere in the kernel (e.g. static DEFINE_PER_CPU(), static > DEFINE_MUTEX(), etc). We had to create two different definitions of hashtable, one to be used as static and one to be used in structs. I chose the uglier way of doing it, and Tejun already pointed it out :) It will be much nicer in the future. >>> >>> /* >>> * Note about RCU : >>> @@ -191,16 +191,14 @@ tracepoint_entry_remove_probe(struct tracepoint_entry *entry, >>> */ >>> static struct tracepoint_entry *get_tracepoint(const char *name) >>> { >>> - struct hlist_head *head; >>> struct hlist_node *node; >>> struct tracepoint_entry *e; >>> u32 hash = jhash(name, strlen(name), 0); >>> >>> - head = &tracepoint_table[hash & (TRACEPOINT_TABLE_SIZE - 1)]; >>> - hlist_for_each_entry(e, node, head, hlist) { >>> + hash_for_each_possible(&tracepoint_table, node, e, hlist, hash) >>> if (!strcmp(name, e->name)) >>> return e; >>> - } >>> + > > Typically, where there are 2 or more nesting levels, I try to keep the > outer brackets, even if the 1st level only contain a single statement > (this is what I did across tracepoint.c). This is especially useful when > nesting multiple if levels, and ensures the "else" clause match the > right if. We might want to keep it that way within the file, to ensure > style consistency. Roger that, will fix. > Other than that, it looks good! > > Thanks! > > Mathieu > Thanks for the review Mathieu! -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org