[ovs-dev] [RFC 2/2] datapath: Cache netlink flow key, mask on flow_put.

[Thomas Graf]

On 07/11/14 at 11:29pm, Joe Stringer wrote:
> I'm skeptical of taking the ovs_lock(). The current patch performs this
> key/mask cache construction inside ovs_lock() as part of the critical
> section for flow install. If we perform this during flow_dump, but extend
> the ovs_lock to the entire flow dump operation, that has the potential to
> make things worse due to the added contention. If I follow correctly, the
> mspin_lock() bottleneck after the current patch is applied is already
> measuring contention on the ovs_mutex when revalidators attempt to delete
> flows. That's with flow_dump as RCU.

We would actually only hold the lock while constructing each
individual message but it is probably not worth it. A cmpxchg will
be expensive as well. It seems like your idea of fixing up the
original flow as provided by the user would be the most optimal
solution.

> Another sidenote:
> > Looking at the code, OVS does not handle NLM_F_DUMP_INTR in user space yet
> > and the kernel dump does not call genl_dump_check_consistent() yet to
> > provide
> > the flag. So what can currently happen even without your patch is that if
> > the dump requires more than one Netlink message, the RCU critical section
> > is left and thus a list deletion or insertion of a flow may occur in the
> > middle of the dump and thus provide a slightly incorrect set of flows.
> >
> 
> Yes, I believe this is done deliberately for performance reasons. OVS
> userspace doesn't assume that it's getting a consistent snapshot of the
> flow table, it handles cases where flows are duplicated or missing in the
> dump. An idea has been floated to keep track of flow adds/deletes in the
> kernel to counteract this, but I got distracted by this idea first :-)

Yes. Other code paths do the same thing. Which is why the
dump-interrupted flag has been introduced to ease detection on the
receiver side.