lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sun, 19 Aug 2012 09:16:37 -0400 From: Mathieu Desnoyers <mathieu.desnoyers@...icios.com> To: Sasha Levin <levinsasha928@...il.com> Cc: torvalds@...ux-foundation.org, tj@...nel.org, akpm@...ux-foundation.org, linux-kernel@...r.kernel.org, linux-mm@...ck.org, paul.gortmaker@...driver.com, davem@...emloft.net, rostedt@...dmis.org, mingo@...e.hu, ebiederm@...ssion.com, aarcange@...hat.com, ericvh@...il.com, netdev@...r.kernel.org, josh@...htriplett.org, eric.dumazet@...il.com, axboe@...nel.dk, agk@...hat.com, dm-devel@...hat.com, neilb@...e.de, ccaulfie@...hat.com, teigland@...hat.com, Trond.Myklebust@...app.com, bfields@...ldses.org, fweisbec@...il.com, jesse@...ira.com, venkat.x.venkatsubra@...cle.com, ejt@...hat.com, snitzer@...hat.com, edumazet@...gle.com, linux-nfs@...r.kernel.org, dev@...nvswitch.org, rds-devel@....oracle.com, lw@...fujitsu.com Subject: Re: [PATCH v2 01/16] hashtable: introduce a small and naive hashtable * Sasha Levin (levinsasha928@...il.com) wrote: > This hashtable implementation is using hlist buckets to provide a simple > hashtable to prevent it from getting reimplemented all over the kernel. > > Signed-off-by: Sasha Levin <levinsasha928@...il.com> > --- > include/linux/hashtable.h | 284 +++++++++++++++++++++++++++++++++++++++++++++ [...] Hi Sasha, There are still a few API naming nits that I'd like to discuss: > + > +/** > + * hash_for_each_size - iterate over a hashtable > + * @name: hashtable to iterate > + * @bits: bit count of hashing function of the hashtable > + * @bkt: integer to use as bucket loop cursor > + * @node: the &struct list_head to use as a loop cursor for each bucket > + * @obj: the type * to use as a loop cursor for each bucket > + * @member: the name of the hlist_node within the struct > + */ > +#define hash_for_each_size(name, bits, bkt, node, obj, member) \ What is the meaning of "for each size" ? By looking at the implementation, I see that it takes an extra "bits" argument to specify the key width. But in the other patches of this patchset, I cannot find a single user of the "*_size" API. If you do not typically expect users to specify this parameter by hand (thanks to use of HASH_BITS(name) in for_each functions that do not take the bits parameter), I would recommend to only expose hash_for_each() and similar defines, but not the *_size variants. So I recommend merging hash_for_each_size into hash_for_each (and doing similarly for other *_size variants). On the plus side, it will cut down the number of for_each macros from 12 down to 6, which is more reasonable. > + for (bkt = 0; bkt < HASH_SIZE(bits); bkt++) \ > + hlist_for_each_entry(obj, node, &name[bkt], member) > + > +/** > + * hash_for_each - iterate over a hashtable > + * @name: hashtable to iterate > + * @bkt: integer to use as bucket loop cursor > + * @node: the &struct list_head to use as a loop cursor for each bucket > + * @obj: the type * to use as a loop cursor for each bucket > + * @member: the name of the hlist_node within the struct > + */ > +#define hash_for_each(name, bkt, node, obj, member) \ > + hash_for_each_size(name, HASH_BITS(name), bkt, node, obj, member) > + [...] > +/** > + * hash_for_each_possible - iterate over all possible objects for a given key > + * @name: hashtable to iterate > + * @obj: the type * to use as a loop cursor for each bucket > + * @bits: bit count of hashing function of the hashtable > + * @node: the &struct list_head to use as a loop cursor for each bucket > + * @member: the name of the hlist_node within the struct > + * @key: the key of the objects to iterate over > + */ > +#define hash_for_each_possible_size(name, obj, bits, node, member, key) \ > + hlist_for_each_entry(obj, node, &name[hash_min(key, bits)], member) Second point: "for_each_possible" does not express the iteration scope. Citing WordNet: "possible adj 1: capable of happening or existing;" -- which has nothing to do with iteration on duplicate keys within a hash table. I would recommend to rename "possible" to "duplicate", e.g.: hash_for_each_duplicate() which clearly says what is the scope of this iteration: duplicate keys. Thanks, Mathieu -- Mathieu Desnoyers Operating System Efficiency R&D Consultant EfficiOS Inc. http://www.efficios.com -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists