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

[Rajahalme, Jarno (NSN - FI/Espoo)]

On Feb 19, 2013, at 21:38 , ext Jesse Gross wrote:

> On Mon, Feb 18, 2013 at 12:13 AM, Rajahalme, Jarno (NSN - FI/Espoo)
> <jarno.rajahalme at nsn.com> wrote:
>> 
>> 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)?
> 

Based on the sporadic coverage logs I concluded that most of the time my test will set up flows without facets. However, at least initially there should be a period when the governor has not yet been started, during which facets and kernel flows should be set up. So I think that the benefit seen is because of the reduced hash computations and maybe in part by reduced parameter passing(?).

So it should have a benefit by it's own, I just haven't been able to construct a test case for that. For some reason I can not get ovs-benchmark to get ovs-vswitchd anywhere above single digit CPU use in my dev setup...

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

This should be applicable by itself. IMO it is also a bit cleaner, conceptually, apart from any performance benefit :-)

  Jarno