diff mbox series

[ovs-dev,v2] ofctrl.c: Avoid repeatedly linking an installed flow and a desired flow.

Message ID 20201008193402.3786783-1-hzhou@ovn.org
State New
Headers show
Series [ovs-dev,v2] ofctrl.c: Avoid repeatedly linking an installed flow and a desired flow. | expand

Commit Message

Han Zhou Oct. 8, 2020, 7:34 p.m. UTC
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.

Signed-off-by: Han Zhou <hzhou@ovn.org>
---
v1 -> v2: Addresses Dumitru's comment.
    - Updated the check from checking if the desired flow is active to checking
      if the desired flow is linked to the installed flow.
    - Removed the first check that compares the desired flow with the active
      desired flow in link_installed_to_desired().
    - Updated commit message.

 controller/ofctrl.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

Comments

Dumitru Ceara Oct. 9, 2020, 7 a.m. UTC | #1
On 10/8/20 9:34 PM, Han Zhou wrote:
> 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.
> 
> Signed-off-by: Han Zhou <hzhou@ovn.org>

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

Thanks,
Dumitru
Han Zhou Oct. 11, 2020, 6:53 a.m. UTC | #2
On Fri, Oct 9, 2020 at 12:00 AM Dumitru Ceara <dceara@redhat.com> wrote:
>
> On 10/8/20 9:34 PM, Han Zhou wrote:
> > 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.
> >
> > Signed-off-by: Han Zhou <hzhou@ovn.org>
>
> Acked-by: Dumitru Ceara <dceara@redhat.com>
>
> Thanks,
> Dumitru
>

Thanks Dumitru, I applied this to master.

Han
diff mbox series

Patch

diff --git a/controller/ofctrl.c b/controller/ofctrl.c
index e725c003c..4425d9894 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);
     }
 }