Re: [PATCH] INET : removes per bucket rwlock in tcp/dccp ehash table

[Jarek Poplawski <jarkao2@o2.pl>]

Eric Dumazet wrote, On 11/04/2007 12:31 PM:

> David Miller a écrit :
>> From: Andi Kleen <ak@suse.de>
>> Date: Sun, 4 Nov 2007 00:18:14 +0100
>>
>>> On Thursday 01 November 2007 11:16:20 Eric Dumazet wrote:

...

>>> Also the EHASH_LOCK_SZ == 0 special case is a little strange. Why did
>>> you add that?
>> He explained this in another reply, because ifdefs are ugly.


But I hope he was only joking, didn't he?

Let's make it clear: ifdefs are in K&R, so they are very nice! Just like
all C! (K, &, and R as well.)

You know, I can even imagine, there are people, who have K&R around their
beds, instead of some other book, so they could be serious about such 
things. (But, don't worry, it's not me - happily I'm not serious!)

This patch looks OK now, but a bit of grumbling shouldn't harm?:

...

> [PATCH] INET : removes per bucket rwlock in tcp/dccp ehash table
> 
> As done two years ago on IP route cache table (commit 
> 22c047ccbc68fa8f3fa57f0e8f906479a062c426) , we can avoid using one lock per 
> hash bucket for the huge TCP/DCCP hash tables.
> 
> On a typical x86_64 platform, this saves about 2MB or 4MB of ram, for litle

- litle
+ little

... 

> +static inline int inet_ehash_locks_alloc(struct inet_hashinfo *hashinfo)
> +{
> +	unsigned int i, size = 256;
> +#if defined(CONFIG_PROVE_LOCKING)
> +	unsigned int nr_pcpus = 2;
> +#else
> +	unsigned int nr_pcpus = num_possible_cpus();
> +#endif
> +	if (nr_pcpus >= 4)
> +		size = 512;
> +	if (nr_pcpus >= 8)
> +		size = 1024;
> +	if (nr_pcpus >= 16)
> +		size = 2048;
> +	if (nr_pcpus >= 32)
> +		size = 4096;


It seems, maybe in the future this could look a bit nicer with some log
type shifting.

> +	if (sizeof(rwlock_t) != 0) {
> +#ifdef CONFIG_NUMA
> +		if (size * sizeof(rwlock_t) > PAGE_SIZE)
> +			hashinfo->ehash_locks = vmalloc(size * sizeof(rwlock_t));
> +		else
> +#endif
> +		hashinfo->ehash_locks =	kmalloc(size * sizeof(rwlock_t),
> +						GFP_KERNEL);
> +		if (!hashinfo->ehash_locks)
> +			return ENOMEM;


Probably doesn't matter now, but maybe more common?:
			return -ENOMEM;

> +		for (i = 0; i < size; i++)
> +			rwlock_init(&hashinfo->ehash_locks[i]);


This looks better now, but still is doubtful to me: even if it's safe with
current rwlock implementation, can't we imagine some new debugging or
statistical code added, which would be called from rwlock_init() without
using rwlock_t structure? IMHO, if read_lock() etc. are called in such a
case, rwlock_init() should be done as well.

Regards,
Jarek P.