From 93f257e672f814b87ff30512aadc4afc4387c3eb Mon Sep 17 00:00:00 2001

From: Han Zhou <hzhou@ovn.org>

Date: Fri, 2 Oct 2020 12:47:52 -0700

Subject: [PATCH 2/7] ofctrl.c: Avoid repeatedly linking an installed flow and

 a desired flow.

In update_installed_flows_by_compare() there are two loops. The first

loop iterates the installed flows and find its peer in desired flows to:

1. uninstall flows that are not needed anymore

2. update flows if needed

At the same time, it links the desired flow found for the installed flow

which also set the desired flow as the current active installed flow.

The second loop iterates the desired flows and find its peer in installed

flows to install missing flows. At the same time it will detect if there

are conflict desired flows matching same installed flow then just link

them.

However, currently in the second loop, it blindly link the desired flows to the

installed flows, without checking if it is already linked in the first loop.

Lucky enough, this won't cause any real problem so far, because when there are

conflict flows, the one found in the first loop will be set as active in the

installed_flow, and in the function link_installed_to_desired() checks if it is

already the active desired flow it just does nothing but return.  However, the

check in the link_installed_to_desired() is confusing because a desired_flow

may be linked to the installed_flow already but not the active flow, and the

check is insufficient. It should be rather an assertion and let the caller

ensure that a pair of desired_flow and installed_flow is never linked twice.

For the above reason, this patch does the following changes:

1. Removes the check in link_installed_to_desired() and convert it to an assert.

2. Before calling link_installed_to_desired() in the above mentioned loop,

   check if the desired flow is already installed.

Acked-by: Dumitru Ceara <dceara@redhat.com>

Signed-off-by: Han Zhou <hzhou@ovn.org>

(cherry picked from upstream commit 7cab7bd1268ba67429954da4f73de91090acf779)

Change-Id: I84f8d7c771c30785dc8c30ce2d4c8c0b4735faae

---

 controller/ofctrl.c | 19 ++++++++++++++-----

 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/controller/ofctrl.c b/controller/ofctrl.c

index e725c00..4425d98 100644

--- a/controller/ofctrl.c

+++ b/controller/ofctrl.c

@@ -806,13 +806,18 @@ desired_flow_set_active(struct desired_flow *d)

     d->installed_flow->desired_flow = d;

 }

+/* Adds the desired flow to the list of desired flows that have same match

+ * conditions as the installed flow.

+ *

+ * If the newly added desired flow is the first one in the list, it is also set

+ * as the active one.

+ *

+ * It is caller's responsibility to make sure the link between the pair didn't

+ * exist before. */

 static void

 link_installed_to_desired(struct installed_flow *i, struct desired_flow *d)

 {

-    if (i->desired_flow == d) {

-        return;

-    }

-

+    ovs_assert(i->desired_flow != d);

     if (ovs_list_is_empty(&i->desired_refs)) {

         ovs_assert(!i->desired_flow);

         i->desired_flow = d;

@@ -1705,8 +1710,12 @@ update_installed_flows_by_compare(struct ovn_desired_flow_table *flow_table,

             /* Copy 'd' from 'flow_table' to installed_flows. */

             i = installed_flow_dup(d);

             hmap_insert(&installed_flows, &i->match_hmap_node, i->flow.hash);

+            link_installed_to_desired(i, d);

+        } else if (!d->installed_flow) {

+            /* This is a desired_flow that conflicts with one installed

+             * previously but not linked yet. */

+            link_installed_to_desired(i, d);

         }

-        link_installed_to_desired(i, d);

     }

 }

-- 

1.8.3.1