[ovs-dev] [PATCH 03/11] datapath: Fold key_len and hash into sw_flow_key.

Jesse Gross jesse at nicira.com
Tue Feb 19 19:38:28 UTC 2013


On Mon, Feb 18, 2013 at 12:13 AM, Rajahalme, Jarno (NSN - FI/Espoo)
<jarno.rajahalme at nsn.com> wrote:
>
> On Feb 16, 2013, at 1:19 , ext Jesse Gross wrote:
>
>> On Mon, Feb 11, 2013 at 6:46 AM, Jarno Rajahalme
>> <jarno.rajahalme at nsn.com> wrote:
>>> Store key_len and hash fields at the end of struct sw_flow_key, past the
>>> area being hashed.
>>> Rename functions operating on keys from "_flow_" to "_flow_key_".
>>> Shift the responsibility for key hashing from lookup and insert to key
>>> construction side, which helps avaiding unnecessary hash computations.
>>>
>>> Signed-off-by: Jarno Rajahalme <jarno.rajahalme at nsn.com>
>>
>> This seems like three independent changes, so it really should be
>> broken up into different commits.  However, that being said, it also
>> needs more justification since I don't really see the benefit at the
>> moment.  In particular, how does moving the location of hash
>> computation reduce unnecessary work?
>
>
> I'd be happy to break it apart.
>
> In datapath.c ovs_flow_cmd_build_info(), there is first a call to ovs_flow_tbl_lookup(), which currently computes the hash. In the case of a new flow this is followed by ovs_flow_tbl_insert(), which currently recomputes the same hash. This can be avoided if hash is made a part of the key itself.

OK, I see now.  In the numbers that you provided before, the test did
not stress the kernel flow setup path (although the performance still
seemed to improve after this patch, which seems odd to me).  Do you
know if this helps independently (and also how such a situation might
arise)?

> In a later patch (which may be more controversial), I have proposed to expose the kernel computed key hash to the userspace (think "cookie"), which is given back to the kernel only when the key is passed back as-is. In this case additional key hash computations can be avoided in kernel flow set-up.

My hope was that if this had a benefit on it's own then it could be
applied independently.  Otherwise, we'll have to wait to see what
conclusion we come to on the rest of the patches.



More information about the dev mailing list