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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <501EA749.9060400@gmail.com>
Date:	Sun, 05 Aug 2012 19:03:05 +0200
From:	Sasha Levin <levinsasha928@...il.com>
To:	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
CC:	Steven Rostedt <rostedt@...dmis.org>,
	torvalds@...ux-foundation.org, tj@...nel.org,
	akpm@...ux-foundation.org, linux-kernel@...r.kernel.org,
	linux-mm@...ck.org, paul.gortmaker@...driver.com,
	davem@...emloft.net, mingo@...e.hu, ebiederm@...ssion.com,
	aarcange@...hat.com, ericvh@...il.com, netdev@...r.kernel.org
Subject: Re: [RFC v2 6/7] tracepoint: use new hashtable implementation

On 08/05/2012 06:31 PM, Mathieu Desnoyers wrote:
> * Steven Rostedt (rostedt@...dmis.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 <levinsasha928@...il.com>
>>> ---
>>>  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).

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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/