Re: [PATCH 01/16] hashtable: introduce a small and naive hashtable

[Sasha Levin]

On 08/15/2012 01:25 AM, NeilBrown wrote:
> On Tue, 14 Aug 2012 18:24:35 +0200 Sasha Levin <levinsasha928@xxxxxxxxx>
> wrote:
> 
> 
>> +static inline void hash_init_size(struct hlist_head *hashtable, int bits)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < HASH_SIZE(bits); i++)
>> +		INIT_HLIST_HEAD(hashtable + i);
>> +}
> 
> This seems like an inefficient way to do "memset(hashtable, 0, ...);".
> And in many cases it isn't needed as the hash table is static and initialised
> to zero.
> I note that in the SUNRPC/cache patch you call hash_init(), but in the lockd
> patch you don't.  You don't actually need to in either case.

Agreed that the code will run just fine if we wouldn't use hash_init().

> I realise that any optimisation here is for code that is only executed once
> per boot, so no big deal, and even the presence of extra code making the
> kernel bigger is unlikely to be an issue.  But I'd at least like to see
> consistency: Either use hash_init everywhere, even when not needed, or only
> use it where absolutely needed which might be no-where because static tables
> are already initialised, and dynamic tables can use GFP_ZERO.

This is a consistency problem. I didn't want to add a module_init() to modules that didn't have it just to get hash_init() in there.

I'll get it fixed.

> And if you keep hash_init_size I would rather see a memset(0)....

My concern with using a memset(0) is that I'm going to break layering.

The hashtable uses hlist. hlist provides us with an entire family of init functions which I'm supposed to use to initialize hlist heads.

So while a memset(0) will work perfectly here, I consider that cheating - it results in an uglier code that assumes to know about hlist internals, and will probably break as soon as someone tries to do something to hlist.

I can think of several alternatives here, and all of them involve changes to hlist instead of the hashtable:

 - Remove INIT_HLIST_HEAD()/HLIST_HEAD()/HLIST_HEAD_INIT() and introduce a CLEAR_HLIST instead, documenting that it's enough to memset(0) the hlist to initialize it properly.
 - Add a block initializer INIT_HLIST_HEADS() or something similar that would initialize an array of heads.
--
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/