Messages in this thread | | | Date | Mon, 29 Oct 2012 09:07:36 -0400 | From | Mathieu Desnoyers <> | Subject | Re: [PATCH v7 10/16] dlm: use new hashtable implementation |
| |
* Mathieu Desnoyers (mathieu.desnoyers@efficios.com) wrote: > * Sasha Levin (levinsasha928@gmail.com) wrote: > [...] > > @@ -158,34 +159,21 @@ static int dlm_allow_conn; > > static struct workqueue_struct *recv_workqueue; > > static struct workqueue_struct *send_workqueue; > > > > -static struct hlist_head connection_hash[CONN_HASH_SIZE]; > > +static struct hlist_head connection_hash[CONN_HASH_BITS]; > > static DEFINE_MUTEX(connections_lock); > > static struct kmem_cache *con_cache; > > > > static void process_recv_sockets(struct work_struct *work); > > static void process_send_sockets(struct work_struct *work); > > > > - > > -/* This is deliberately very simple because most clusters have simple > > - sequential nodeids, so we should be able to go straight to a connection > > - struct in the array */ > > -static inline int nodeid_hash(int nodeid) > > -{ > > - return nodeid & (CONN_HASH_SIZE-1); > > -} > > There is one thing I dislike about this change: you remove a useful > comment. It's good to be informed of the reason why a direct mapping > "value -> hash" without any dispersion function is preferred here.
And now that I come to think of it: you're changing the behavior : you will now use a dispersion function on the key, which goes against the intent expressed in this comment.
It might be good to change hash_add(), hash_add_rcu(), hash_for_each_possible*() key parameter for a "hash" parameter, and let the caller provide the hash value computed by the function they like as parameter, rather than enforcing hash_32/hash_64.
Thoughts ?
Thanks,
Mathieu
-- Mathieu Desnoyers Operating System Efficiency R&D Consultant EfficiOS Inc. http://www.efficios.com
| |