Re: [dm-devel] [PATCH v2 01/16] hashtable: introduce a small and naive hashtable

[Mathieu Desnoyers <mathieu desnoyers efficios com>]

* Sasha Levin (levinsasha928 gmail 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 gmail 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