[ovs-dev] [PATCH 2/4] datapath: Add flow mask cache.

Thomas Graf tgraf at redhat.com
Mon Apr 14 17:19:57 UTC 2014


On 04/14/2014 05:59 PM, Pravin Shelar wrote:
> On Mon, Apr 14, 2014 at 8:19 PM, Thomas Graf <tgraf at redhat.com> wrote:
>> On 04/14/2014 08:54 AM, Pravin Shelar wrote:
>>>
>>> On Wed, Apr 9, 2014 at 4:05 PM, Thomas Graf <tgraf at redhat.com> wrote:
>>>>
>>>> I believe skb_get_hash() can be 0 and that would result in mistaking any
>>>> empty slot to be a cached entry and we would only look at the first flow
>>>> mask.
>>>
>>>
>>> software skb_get_hash() returns zero for error case which should be
>>> rare. Thats why I avoided special check for zero hash for every cache
>>> lookup.
>>
>>
>> I agree but in those rare cases the lookup will be incorrect. With
>> an appropriate unlikely() hint to the compiler the additional cost is
>> minimal.
>
> I am not sure why lookup would be incorrect. If cached mask is not
> correct then lookup fails and it does fallback to complete mask list
> search anyways.

You are right. masked_flow_lookup() does verify the mask again and
will ignore flows with mismatching mask, I didn't see this previously.
An incorrect lookup is therefore not possible.

It still means that if entries[0] is unused, we will traverse though
all flow masks MC_HASH_SEGS times and traverse through a flow table
hash chain for each segment.

If the hashing is broken due to some reason then this cost is added for
every packet. Traversing through lists unnecessarily is really painful
as it typically involves a lot of cache misses.

I definitely don't want to block inclusion of this but I still think
that if (likely(hash)) { cache lookup; } would make sense.




More information about the dev mailing list