Re: [RFC v2 6/7] tracepoint: use new hashtable implementation

[Mathieu Desnoyers]

* Steven Rostedt (rostedt@xxxxxxxxxxx) 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 <levinsasha928@xxxxxxxxx>
> > ---
> >  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 <linux/slab.h>
> >  #include <linux/sched.h>
> >  #include <linux/static_key.h>
> > +#include <linux/hashtable.h>
> >  
> >  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).

> >  
> >  /*
> >   * 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.

Other than that, it looks good!

Thanks!

Mathieu

> >  	return NULL;
> >  }
> >  
> > @@ -210,19 +208,13 @@ static struct tracepoint_entry *get_tracepoint(const char *name)
> >   */
> >  static struct tracepoint_entry *add_tracepoint(const char *name)
> >  {
> > -	struct hlist_head *head;
> > -	struct hlist_node *node;
> >  	struct tracepoint_entry *e;
> >  	size_t name_len = strlen(name) + 1;
> >  	u32 hash = jhash(name, name_len-1, 0);
> >  
> > -	head = &tracepoint_table[hash & (TRACEPOINT_TABLE_SIZE - 1)];
> > -	hlist_for_each_entry(e, node, head, hlist) {
> > -		if (!strcmp(name, e->name)) {
> > -			printk(KERN_NOTICE
> > -				"tracepoint %s busy\n", name);
> > -			return ERR_PTR(-EEXIST);	/* Already there */
> > -		}
> > +	if (get_tracepoint(name)) {
> > +		printk(KERN_NOTICE "tracepoint %s busy\n", name);
> > +		return ERR_PTR(-EEXIST);	/* Already there */
> >  	}
> >  	/*
> >  	 * Using kmalloc here to allocate a variable length element. Could
> > @@ -234,7 +226,7 @@ static struct tracepoint_entry *add_tracepoint(const char *name)
> >  	memcpy(&e->name[0], name, name_len);
> >  	e->funcs = NULL;
> >  	e->refcount = 0;
> > -	hlist_add_head(&e->hlist, head);
> > +	hash_add(&tracepoint_table, &e->hlist, hash);
> >  	return e;
> >  }
> >  
> > @@ -244,7 +236,7 @@ static struct tracepoint_entry *add_tracepoint(const char *name)
> >   */
> >  static inline void remove_tracepoint(struct tracepoint_entry *e)
> >  {
> > -	hlist_del(&e->hlist);
> > +	hash_del(&e->hlist);
> >  	kfree(e);
> >  }
> >  
> 
> 

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/