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

[Jesse Gross]

On Wed, Apr 14, 2010 at 7:54 PM, Ben Pfaff <blp at nicira.com> wrote:

> 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.
>

The use case is to hold GRE ports, which is performance sensitive and the
number of them varies widely.


>
> 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.
>

There's now an extra argument to those functions which implicitly does what
tbl_set_destructor previously did.  It is a bit nicer.


>
> 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.
>

Yes, it should be static.  However, what do you mean that hash_seed can
never change?  It is randomized when the module is loaded.  Since I don't
expect that tables will be created and destroyed frequently it is
essentially equivalent.


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

Yes, they were left over.


>
> 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.
>

Since our goal is to only have one object per bucket it is unlikely that
there would be more than one call through the function pointer (even more so
now that we are directly comparing hashes).  I find it highly unlikely that
a single function pointer call would have any measurable performance impact.


>
> 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.)
>

Yes, thank you.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20100415/a79ffbda/attachment-0003.html>