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