[ovs-dev] [PATCH 08/16] datapath: Genericize hash table.

[Ben Pfaff]

On Tue, Apr 13, 2010 at 10:41:10AM -0400, Jesse Gross wrote:
> Currently the flow hash table assumes that it is storing flows.
> However, we will need additional types of hash tables in the
> future so remove assumptions about flows and convert the datapath
> to use the new table.

The flow hash table is designed to tolerate many kinds of changes,
including expanding and shrinking, while remaining consistent from an
RCU perspective the whole time.  Many kinds of kernel hash tables don't
need to be as flexible.  If the ones you have in mind don't, then the
flow table is probably kind of bloated for the purpose.

tbl_set_destructor() stuck out for me.  Could this be replaced by an
additional argument to tbl_destroy() and tbl_deferred_destroy()?  If so,
I would like that better.

I think that hash_seed is only used in flow.c, so that it should be
"static".  It's too bad that hash_seed can never change anymore; maybe
we should just use 0.

I don't think that table.c needs to include linux/jhash.h or
linux/vmalloc.h or linux/highmem.h.

I have always assumed (without profiling) that flow table lookup was an
important "fast path" in the kernel module, so I'm nervous about
introducing a call through a function pointer there unnecessarily.  I
would consider exposing struct tbl_bucket and find_bucket() and letting
the caller search the buckets.

If you do keep the tbl_lookup() function as-is then I would make
search_bucket() check "hash == bucket->hash" before it calls "cmp".
(Currently it doesn't use its "hash" argument at all so probably you
were intending to do that anyhow.)