From: Andy Zhou <azhou@nicira.com>
Date: Sat, 3 Aug 2013 03:22:17 +0000 (-0700)
Subject: ofproto-dpif: avoid losing track of kernel flows upon reinstallation
X-Git-Tag: sliver-openvswitch-2.0.90-1~33^2~31
X-Git-Url: http://git.onelab.eu/?p=sliver-openvswitch.git;a=commitdiff_plain;h=24ce4b082f0b44a8b374b664c9fb382d0c2396c4

ofproto-dpif: avoid losing track of kernel flows upon reinstallation

This commit fixes a problem whereby userspace can lose track of a
flow installed in the kernel, instead believing that the flow is
not installed.  The most visible consequence of this bug was a
message in the ovs-vswitchd log warning about an unexpected flow
in the kernel.  Other possible consequences included loss of
statistics and failure to updates actions when the OpenFlow flow
table changed.

The problem arose in the following scenario.  Suppose userspace
sets up a kernel flow due to an arriving packet.  Before kernel
flow setup completes, another packet for that flow arrives.  The
kernel sends the new packet to userspace after userspace has
completed processing the batch of packets that set up the flow.
Userspace then attempts to reinstall the kernel flow.  This fails
with EEXIST, so userspace then marked the flow as not-installed,
even though it was successfully installed before and remains
installed.  The next time userspace dumped the kernel flow
table to gather statistics, it would complain about an unexpected
flow and delete it.

In practice, we have seen these messages with netperf TCP_CRR tests and
UDP stream tests.

This patch fixes the problem by changing userspace so that, once
it successfully installs a flow in the kernel, it will not reinstall
it when it sees another packet for the flow in userspace.  This
has the downside that, if something goes wrong and a flow
disappears from the kernel (e.g. ovs-dpctl del-flows), then userspace
won't reinstall it (until it tries to delete it).  (This is in fact
the reason why until now userspace reinstalled flows it knew it
already installed.)

Some more background may be warranted.  There are two EEXIST error
cases:

       1. A subfacet was installed successfully in a previous (recent)
          batch.  Now we've attempted to reinstall exactly the same
          subfacet in this batch.

       2. A subfacet was installed successfully in a previous (recent)
          batch or earlier in the current batch.  We've attempted to
          install a subfacet for an overlapping megaflow.

Before megaflows, installation errors were ignored completely.
Since megaflows were introduced, they have been handled by
considering on any installation error that the given subfacet is
not installed.  This works well for case #2 but causes case #1 to
yield unexpected flows, as described at the top of the commit
message.

This commit adds the wrinkle that we never try to reinstall
exactly the same subfacet that we know we installed successfully
earlier (and haven't deleted) unless its actions change.  This
ought to work just as well for case #2, and avoids the problem
with case #1.

Prepared with assistance from Ethan.

Signed-off-by: Andy Zhou <azhou@nicira.com>
[blp@nicira.com rewrote the commit message]
Signed-off-by: Ben Pfaff <blp@nicira.com>
---

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 3bf4fe39a..29a93e6cd 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -3460,7 +3460,7 @@ handle_flow_miss_with_facet(struct flow_miss *miss, struct facet *facet,
         subfacet_update_stats(subfacet, stats);
     }
 
-    if (miss->upcall_type == DPIF_UC_MISS || subfacet->path != want_path) {
+    if (subfacet->path != want_path) {
         struct flow_miss_op *op = &ops[(*n_ops)++];
         struct dpif_flow_put *put = &op->dpif_op.u.flow_put;