Eric Dumazet wrote, On 11/04/2007 12:31 PM:
> David Miller a écrit :
>> From: Andi Kleen <[EMAIL PROTECTED]>
>> 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.
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
