diff mbox

[ovs-dev,1/4] ovn-controller: decouple localnet_port update from patch_run

Message ID 1495758407-20532-2-git-send-email-zhouhan@gmail.com
State Changes Requested
Headers show

Commit Message

Han Zhou May 26, 2017, 12:26 a.m. UTC 
We figure out local datapaths in binding_run() but update the field
localnet_port for each local datapath that has localnet port in
patch_run(). This patch updates the localnet_port field in binding_run
directly and removes the logic in patch_run(), since the logic is
more about port-binding processing, and patch_run() is focusing on
patch port creation only.

In a future patch binding_run() will be used in a new thread for
pinctrl, but patch_run() will not.

Signed-off-by: Han Zhou <zhouhan@gmail.com>
---
 ovn/controller/binding.c        | 47 +++++++++++++++++++++++++++++++++++++++--
 ovn/controller/ovn-controller.c |  2 +-
 ovn/controller/patch.c          | 30 ++------------------------
 ovn/controller/patch.h          |  2 +-
 4 files changed, 49 insertions(+), 32 deletions(-)

Comments

Ben Pfaff June 6, 2017, 7:31 p.m. UTC | #1 
On Thu, May 25, 2017 at 05:26:44PM -0700, Han Zhou wrote:
> We figure out local datapaths in binding_run() but update the field
> localnet_port for each local datapath that has localnet port in
> patch_run(). This patch updates the localnet_port field in binding_run
> directly and removes the logic in patch_run(), since the logic is
> more about port-binding processing, and patch_run() is focusing on
> patch port creation only.
> 
> In a future patch binding_run() will be used in a new thread for
> pinctrl, but patch_run() will not.
> 
> Signed-off-by: Han Zhou <zhouhan@gmail.com>

Thanks for working on this!  I'm looking forward to the improvements to
ovn-controller from this series.

I like this change even on its own.  I think that it will make the code
clearer.

I don't think that localnet_ports is being used as a hash table here.
It is really being used as a list or an array, because the only
operations on it are insertion and iteration.  Perhaps a dynamic array
of "struct ovsrec_binding *" would be a better way?  Another way, which
is even simpler, would be to simply add a second iteration over the
sbrec_port_binding that looks only at localnet ports.  This would be
slower, but it would not increase the big-O for binding_run() because it
already has a similar loop.

I think that there is a memory leak here because I don't see anything
that frees the "struct localnet_port"s (only the hash table that holds
them).
Han Zhou June 6, 2017, 11:59 p.m. UTC | #2 
On Tue, Jun 6, 2017 at 12:31 PM, Ben Pfaff <blp@ovn.org> wrote:
>
> On Thu, May 25, 2017 at 05:26:44PM -0700, Han Zhou wrote:
> > We figure out local datapaths in binding_run() but update the field
> > localnet_port for each local datapath that has localnet port in
> > patch_run(). This patch updates the localnet_port field in binding_run
> > directly and removes the logic in patch_run(), since the logic is
> > more about port-binding processing, and patch_run() is focusing on
> > patch port creation only.
> >
> > In a future patch binding_run() will be used in a new thread for
> > pinctrl, but patch_run() will not.
> >
> > Signed-off-by: Han Zhou <zhouhan@gmail.com>
>
> Thanks for working on this!  I'm looking forward to the improvements to
> ovn-controller from this series.
>
> I like this change even on its own.  I think that it will make the code
> clearer.
>
> I don't think that localnet_ports is being used as a hash table here.
> It is really being used as a list or an array, because the only
> operations on it are insertion and iteration.  Perhaps a dynamic array
> of "struct ovsrec_binding *" would be a better way?  Another way, which
> is even simpler, would be to simply add a second iteration over the
> sbrec_port_binding that looks only at localnet ports.  This would be
> slower, but it would not increase the big-O for binding_run() because it
> already has a similar loop.

Agree.

>
> I think that there is a memory leak here because I don't see anything
> that frees the "struct localnet_port"s (only the hash table that holds
> them).

Sorry, I will come up with next patch just iterate port-bindings again.
diff mbox

Patch

diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
index 95e9deb..8080f8f 100644
--- a/ovn/controller/binding.c
+++ b/ovn/controller/binding.c
@@ -40,6 +40,11 @@  struct qos_queue {
     uint32_t burst;
 };
 
+struct localnet_port {
+    struct hmap_node node;
+    const struct sbrec_port_binding *pb;
+};
+
 void
 binding_register_ovs_idl(struct ovsdb_idl *ovs_idl)
 {
@@ -362,7 +367,8 @@  consider_local_datapath(struct controller_ctx *ctx,
                         struct hmap *qos_map,
                         struct hmap *local_datapaths,
                         struct shash *lport_to_iface,
-                        struct sset *local_lports)
+                        struct sset *local_lports,
+                        struct hmap *localnet_ports)
 {
     const struct ovsrec_interface *iface_rec
         = shash_find_data(lport_to_iface, binding_rec->logical_port);
@@ -410,6 +416,15 @@  consider_local_datapath(struct controller_ctx *ctx,
         /* Add all localnet ports to local_lports so that we allocate ct zones
          * for them. */
         sset_add(local_lports, binding_rec->logical_port);
+
+        /* Add to localnet_ports so that we can update localnet_port field
+         * for each dp in local_datapaths later */
+        struct localnet_port *ln_port;
+        ln_port = xzalloc(sizeof *ln_port);
+        hmap_insert(localnet_ports, &ln_port->node,
+                    hash_string(binding_rec->logical_port, 0));
+        ln_port->pb = binding_rec;
+
         our_chassis = false;
     }
 
@@ -454,8 +469,10 @@  binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int,
     struct shash lport_to_iface = SHASH_INITIALIZER(&lport_to_iface);
     struct sset egress_ifaces = SSET_INITIALIZER(&egress_ifaces);
     struct hmap qos_map;
+    struct hmap localnet_ports;
 
     hmap_init(&qos_map);
+    hmap_init(&localnet_ports);
     if (br_int) {
         get_local_iface_ids(br_int, &lport_to_iface, local_lports,
                             &egress_ifaces);
@@ -469,8 +486,33 @@  binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int,
                                 chassis_rec, binding_rec,
                                 sset_is_empty(&egress_ifaces) ? NULL :
                                 &qos_map, local_datapaths, &lport_to_iface,
-                                local_lports);
+                                local_lports, &localnet_ports);
+
+    }
+
+    /* Run through each localnet port binding to see if it is on local
+     * datapaths discovered from above loop, and update the item
+     * accordingly. */
+    struct localnet_port *ln_port;
+    HMAP_FOR_EACH (ln_port, node, &localnet_ports) {
+        struct local_datapath *ld
+            = get_local_datapath(local_datapaths,
+                                 ln_port->pb->datapath->tunnel_key);
+        if (!ld) {
+            continue;
+        }
 
+        if (ld->localnet_port && strcmp(ld->localnet_port->logical_port,
+                                        ln_port->pb->logical_port)) {
+            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
+            VLOG_WARN_RL(&rl, "localnet port '%s' already set for datapath "
+                         "'%"PRId64"', skipping the new port '%s'.",
+                         ld->localnet_port->logical_port,
+                         ln_port->pb->datapath->tunnel_key,
+                         ln_port->pb->logical_port);
+            continue;
+        }
+        ld->localnet_port = ln_port->pb;
     }
 
     if (!sset_is_empty(&egress_ifaces)
@@ -484,6 +526,7 @@  binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int,
     shash_destroy(&lport_to_iface);
     sset_destroy(&egress_ifaces);
     hmap_destroy(&qos_map);
+    hmap_destroy(&localnet_ports);
 }
 
 /* Returns true if the database is all cleaned up, false if more work is
diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index f22551d..1155657 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -636,7 +636,7 @@  main(int argc, char *argv[])
             struct shash addr_sets = SHASH_INITIALIZER(&addr_sets);
             addr_sets_init(&ctx, &addr_sets);
 
-            patch_run(&ctx, br_int, chassis, &local_datapaths);
+            patch_run(&ctx, br_int, chassis);
 
             enum mf_field_id mff_ovn_geneve = ofctrl_run(br_int,
                                                          &pending_ct_zones);
diff --git a/ovn/controller/patch.c b/ovn/controller/patch.c
index 158413e..6305ec3 100644
--- a/ovn/controller/patch.c
+++ b/ovn/controller/patch.c
@@ -136,7 +136,6 @@  static void
 add_bridge_mappings(struct controller_ctx *ctx,
                     const struct ovsrec_bridge *br_int,
                     struct shash *existing_ports,
-                    struct hmap *local_datapaths,
                     const struct sbrec_chassis *chassis)
 {
     /* Get ovn-bridge-mappings. */
@@ -180,30 +179,6 @@  add_bridge_mappings(struct controller_ctx *ctx,
     SBREC_PORT_BINDING_FOR_EACH (binding, ctx->ovnsb_idl) {
         const char *patch_port_id;
         if (!strcmp(binding->type, "localnet")) {
-            struct local_datapath *ld
-                = get_local_datapath(local_datapaths,
-                                     binding->datapath->tunnel_key);
-            if (!ld) {
-                continue;
-            }
-
-            /* Under incremental processing, it is possible to re-enter the
-             * following block with a logical port that has already been
-             * recorded in binding->logical_port.  Rather than emit spurious
-             * warnings, add a check to see if the logical port name has
-             * actually changed. */
-
-            if (ld->localnet_port && strcmp(ld->localnet_port->logical_port,
-                                            binding->logical_port)) {
-                static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
-                VLOG_WARN_RL(&rl, "localnet port '%s' already set for datapath "
-                             "'%"PRId64"', skipping the new port '%s'.",
-                             ld->localnet_port->logical_port,
-                             binding->datapath->tunnel_key,
-                             binding->logical_port);
-                continue;
-            }
-            ld->localnet_port = binding;
             patch_port_id = "ovn-localnet-port";
         } else if (!strcmp(binding->type, "l2gateway")) {
             if (!binding->chassis
@@ -249,7 +224,7 @@  add_bridge_mappings(struct controller_ctx *ctx,
 
 void
 patch_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int,
-          const struct sbrec_chassis *chassis, struct hmap *local_datapaths)
+          const struct sbrec_chassis *chassis)
 {
     if (!ctx->ovs_idl_txn) {
         return;
@@ -275,8 +250,7 @@  patch_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int,
     /* Create in the database any patch ports that should exist.  Remove from
      * 'existing_ports' any patch ports that do exist in the database and
      * should be there. */
-    add_bridge_mappings(ctx, br_int, &existing_ports, local_datapaths,
-                        chassis);
+    add_bridge_mappings(ctx, br_int, &existing_ports, chassis);
 
     /* Now 'existing_ports' only still contains patch ports that exist in the
      * database but shouldn't.  Delete them from the database. */
diff --git a/ovn/controller/patch.h b/ovn/controller/patch.h
index 2feb44b..bcc89b6 100644
--- a/ovn/controller/patch.h
+++ b/ovn/controller/patch.h
@@ -28,6 +28,6 @@  struct ovsrec_bridge;
 struct sbrec_chassis;
 
 void patch_run(struct controller_ctx *, const struct ovsrec_bridge *br_int,
-               const struct sbrec_chassis *, struct hmap *local_datapaths);
+               const struct sbrec_chassis *);
 
 #endif /* ovn/patch.h */