On 04/14/2014 05:59 PM, Pravin Shelar wrote:
On Mon, Apr 14, 2014 at 8:19 PM, Thomas Graf <tg...@redhat.com> wrote:On 04/14/2014 08:54 AM, Pravin Shelar wrote:On Wed, Apr 9, 2014 at 4:05 PM, Thomas Graf <tg...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
