diff mbox

[ovs-dev,v5] ovn-controller: add quiet mode

Message ID 1492754632-57814-1-git-send-email-zhouhan@gmail.com
State Superseded
Headers show

Commit Message

Han Zhou April 21, 2017, 6:03 a.m. UTC
As discussed in [1], what the incremental processing code
actually accomplished was that the ovn-controller would
be "quiet" and not burn CPU when things weren't changing.
This patch set recreates this state by calculating whether
changes have occured that would require a full calculation
to be performed.  It does this by persisting a copy of
the localvif_to_ofport and tunnel information in the
controller module, rather than in the physical.c module
as was the case with previous commits.

Performance improved extremely in below test scenario:

- 1 lswitch with 10 lports bound locally
- Each lport has an ingress ACL, referencing the same address-set
- The address-set has 10,000 IPv4 addresses

For each IP address in the address-set, there will be 3
OpenFlow rules generated for each ACL. So the total number
of rules is 300k+.

Without the patch, it takes 50+ minutes to install all the
rules to ovs-vswitchd.

With the patch, it takes 20 seconds to install all the rules
to ovs-vswitchd.

The reason is that the large number of rules are sent to
ovs-vswitchd gradually in many iterations of ovn-controller
main loop. Without the patch, cpu cycles are wasted in
lflow_run to re-processing the large address set in every
main loop iteration. With the patch, lflow_run is not
executed in most iterations because there is no change of
input.

[1] https://mail.openvswitch.org/pipermail/ovs-dev/2016-August/078272.html

Signed-off-by: Ryan Moats <rmoats@us.ibm.com>
Signed-off-by: Han Zhou <zhouhan@gmail.com>
---

Notes:
    v4->v5:
    - Based on the old patch from Ryan Moats and rebased to current master.
    - Fixed a problem: when there are large number of flows being installed
      to OVS, changes made at the same time to SB DB (e.g. lflows) or
      physical mappings won't get processed until there are new changes to
      trigger processing, which may not happen at all. The root cause is
      ofctrl_put can abort temporarily if there are in-flight messages,
      which causes last round of flow updates got lost.

 ovn/controller/ofctrl.c         |  24 +++++++---
 ovn/controller/ofctrl.h         |   1 +
 ovn/controller/ovn-controller.c |  63 ++++++++++++++++++-------
 ovn/controller/ovn-controller.h |   1 +
 ovn/controller/patch.c          |   2 +
 ovn/controller/physical.c       | 102 +++++++++++++++++++++++++---------------
 ovn/controller/physical.h       |   5 ++
 7 files changed, 137 insertions(+), 61 deletions(-)

Comments

Han Zhou April 22, 2017, 2:03 a.m. UTC | #1
On Thu, Apr 20, 2017 at 11:03 PM, Han Zhou <zhouhan@gmail.com> wrote:
>
> As discussed in [1], what the incremental processing code
> actually accomplished was that the ovn-controller would
> be "quiet" and not burn CPU when things weren't changing.
> This patch set recreates this state by calculating whether
> changes have occured that would require a full calculation
> to be performed.  It does this by persisting a copy of
> the localvif_to_ofport and tunnel information in the
> controller module, rather than in the physical.c module
> as was the case with previous commits.
>
> Performance improved extremely in below test scenario:
>
> - 1 lswitch with 10 lports bound locally
> - Each lport has an ingress ACL, referencing the same address-set
> - The address-set has 10,000 IPv4 addresses
>
> For each IP address in the address-set, there will be 3
> OpenFlow rules generated for each ACL. So the total number
> of rules is 300k+.
>
> Without the patch, it takes 50+ minutes to install all the
> rules to ovs-vswitchd.
>
> With the patch, it takes 20 seconds to install all the rules
> to ovs-vswitchd.
>
> The reason is that the large number of rules are sent to
> ovs-vswitchd gradually in many iterations of ovn-controller
> main loop. Without the patch, cpu cycles are wasted in
> lflow_run to re-processing the large address set in every
> main loop iteration. With the patch, lflow_run is not
> executed in most iterations because there is no change of
> input.
>
> [1] https://mail.openvswitch.org/pipermail/ovs-dev/2016-August/078272.html
>
> Signed-off-by: Ryan Moats <rmoats@us.ibm.com>
> Signed-off-by: Han Zhou <zhouhan@gmail.com>
> ---
>
> Notes:
>     v4->v5:
>     - Based on the old patch from Ryan Moats and rebased to current
master.
>     - Fixed a problem: when there are large number of flows being
installed
>       to OVS, changes made at the same time to SB DB (e.g. lflows) or
>       physical mappings won't get processed until there are new changes to
>       trigger processing, which may not happen at all. The root cause is
>       ofctrl_put can abort temporarily if there are in-flight messages,
>       which causes last round of flow updates got lost.

I just realized that the ofctrl_can_put logic alone is able to achieve the
performance improvement, so I split it to 2 patches in a series for the
first (simpler) one to be reviewed/merged faster probably. The quiet mode
change depends on the first one to behave correctly.

1) https://mail.openvswitch.org/pipermail/ovs-dev/2017-April/331286.html
2) https://mail.openvswitch.org/pipermail/ovs-dev/2017-April/331287.html

Thanks,
Han
diff mbox

Patch

diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
index 10c8105..5c24771 100644
--- a/ovn/controller/ofctrl.c
+++ b/ovn/controller/ofctrl.c
@@ -383,6 +383,8 @@  run_S_CLEAR_FLOWS(void)
     queue_msg(encode_group_mod(&gm));
     ofputil_uninit_group_mod(&gm);
 
+    force_full_process();
+
     /* Clear existing groups, to match the state of the switch. */
     if (groups) {
         ovn_group_table_clear(groups, true);
@@ -813,6 +815,20 @@  add_ct_flush_zone(uint16_t zone_id, struct ovs_list *msgs)
     ovs_list_push_back(msgs, &msg->list_node);
 }
 
+/* The flow table can be updated if the connection to the switch is up and
+ * in the correct state and not backlogged with existing flow_mods.  (Our
+ * criteria for being backlogged appear very conservative, but the socket
+ * between ovn-controller and OVS provides some buffering.) */
+bool
+ofctrl_can_put(void)
+{
+    if (state != S_UPDATE_FLOWS
+        || rconn_packet_counter_n_packets(tx_counter)) {
+        return false;
+    }
+    return true;
+}
+
 /* Replaces the flow table on the switch, if possible, by the flows added
  * with ofctrl_add_flow().
  *
@@ -831,14 +847,10 @@  void
 ofctrl_put(struct hmap *flow_table, struct shash *pending_ct_zones,
            int64_t nb_cfg)
 {
-    /* The flow table can be updated if the connection to the switch is up and
-     * in the correct state and not backlogged with existing flow_mods.  (Our
-     * criteria for being backlogged appear very conservative, but the socket
-     * between ovn-controller and OVS provides some buffering.) */
-    if (state != S_UPDATE_FLOWS
-        || rconn_packet_counter_n_packets(tx_counter)) {
+    if (!ofctrl_can_put()) {
         ovn_flow_table_clear(flow_table);
         ovn_group_table_clear(groups, false);
+        force_full_process();
         return;
     }
 
diff --git a/ovn/controller/ofctrl.h b/ovn/controller/ofctrl.h
index bf5ba01..d83f6ae 100644
--- a/ovn/controller/ofctrl.h
+++ b/ovn/controller/ofctrl.h
@@ -34,6 +34,7 @@  struct shash;
 void ofctrl_init(struct group_table *group_table);
 enum mf_field_id ofctrl_run(const struct ovsrec_bridge *br_int,
                             struct shash *pending_ct_zones);
+bool ofctrl_can_put(void);
 void ofctrl_put(struct hmap *flow_table, struct shash *pending_ct_zones,
                 int64_t nb_cfg);
 void ofctrl_wait(void);
diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index e00f57a..0a75f65 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -484,6 +484,23 @@  get_nb_cfg(struct ovsdb_idl *idl)
     return sb ? sb->nb_cfg : 0;
 }
 
+/* Contains mapping of localvifs to OF ports for detecting if the physical
+ * configuration of the switch has changed. */
+static struct simap localvif_to_ofport =
+    SIMAP_INITIALIZER(&localvif_to_ofport);
+
+/* Contains the list of known tunnels. */
+static struct hmap tunnels = HMAP_INITIALIZER(&tunnels);
+
+/* The last sequence number seen from the southbound IDL. */
+static unsigned int seqno = 0;
+
+void
+force_full_process(void)
+{
+    seqno = 0;
+}
+
 int
 main(int argc, char *argv[])
 {
@@ -576,6 +593,7 @@  main(int argc, char *argv[])
 
     /* Main loop. */
     exiting = false;
+    bool physical_change = true;
     while (!exiting) {
         /* Check OVN SB database. */
         char *new_ovnsb_remote = get_ovnsb_remote(ovs_idl_loop.idl);
@@ -613,10 +631,8 @@  main(int argc, char *argv[])
 
         struct ldatapath_index ldatapaths;
         struct lport_index lports;
-        struct mcgroup_index mcgroups;
         ldatapath_index_init(&ldatapaths, ctx.ovnsb_idl);
         lport_index_init(&lports, ctx.ovnsb_idl);
-        mcgroup_index_init(&mcgroups, ctx.ovnsb_idl);
 
         const struct sbrec_chassis *chassis = NULL;
         if (chassis_id) {
@@ -627,35 +643,49 @@  main(int argc, char *argv[])
         }
 
         if (br_int && chassis) {
+            enum mf_field_id mff_ovn_geneve = ofctrl_run(br_int,
+                                                         &pending_ct_zones);
+            if (detect_and_save_physical_changes(
+                &localvif_to_ofport, &tunnels, mff_ovn_geneve, br_int,
+                chassis)) {
+                physical_change = true;
+            }
+            unsigned int cur_seqno = ovsdb_idl_get_seqno(ovnsb_idl_loop.idl);
+
             struct shash addr_sets = SHASH_INITIALIZER(&addr_sets);
             addr_sets_init(&ctx, &addr_sets);
 
             patch_run(&ctx, br_int, chassis, &local_datapaths);
 
-            enum mf_field_id mff_ovn_geneve = ofctrl_run(br_int,
-                                                         &pending_ct_zones);
-
             pinctrl_run(&ctx, &lports, br_int, chassis, &local_datapaths);
             update_ct_zones(&local_lports, &local_datapaths, &ct_zones,
                             ct_zone_bitmap, &pending_ct_zones);
             if (ctx.ovs_idl_txn) {
+                if ((physical_change || seqno < cur_seqno) &&
+                    ofctrl_can_put()) {
+                    seqno = cur_seqno;
+                    physical_change = false;
 
-                commit_ct_zones(br_int, &pending_ct_zones);
+                    commit_ct_zones(br_int, &pending_ct_zones);
 
-                struct hmap flow_table = HMAP_INITIALIZER(&flow_table);
-                lflow_run(&ctx, chassis, &lports, &mcgroups,
-                          &local_datapaths, &group_table, &ct_zones,
-                          &addr_sets, &flow_table);
+                    struct mcgroup_index mcgroups;
+                    mcgroup_index_init(&mcgroups, ctx.ovnsb_idl);
 
-                physical_run(&ctx, mff_ovn_geneve,
-                             br_int, chassis, &ct_zones, &lports,
-                             &flow_table, &local_datapaths);
+                    struct hmap flow_table = HMAP_INITIALIZER(&flow_table);
+                    lflow_run(&ctx, chassis, &lports, &mcgroups,
+                              &local_datapaths, &group_table, &ct_zones,
+                              &addr_sets, &flow_table);
 
-                ofctrl_put(&flow_table, &pending_ct_zones,
-                           get_nb_cfg(ctx.ovnsb_idl));
+                    physical_run(&ctx, mff_ovn_geneve,
+                                 br_int, chassis, &ct_zones, &lports,
+                                 &flow_table, &local_datapaths);
 
-                hmap_destroy(&flow_table);
+                    ofctrl_put(&flow_table, &pending_ct_zones,
+                               get_nb_cfg(ctx.ovnsb_idl));
 
+                    hmap_destroy(&flow_table);
+                    mcgroup_index_destroy(&mcgroups);
+                }
                 if (ctx.ovnsb_idl_txn) {
                     int64_t cur_cfg = ofctrl_get_cur_cfg();
                     if (cur_cfg && cur_cfg != chassis->nb_cfg) {
@@ -693,7 +723,6 @@  main(int argc, char *argv[])
             free(pending_pkt.flow_s);
         }
 
-        mcgroup_index_destroy(&mcgroups);
         lport_index_destroy(&lports);
         ldatapath_index_destroy(&ldatapaths);
 
diff --git a/ovn/controller/ovn-controller.h b/ovn/controller/ovn-controller.h
index 4bc0467..9a782d1 100644
--- a/ovn/controller/ovn-controller.h
+++ b/ovn/controller/ovn-controller.h
@@ -87,5 +87,6 @@  enum chassis_tunnel_type {
 
 uint32_t get_tunnel_type(const char *name);
 
+void force_full_process(void);
 
 #endif /* ovn/ovn-controller.h */
diff --git a/ovn/controller/patch.c b/ovn/controller/patch.c
index 158413e..5e383a5 100644
--- a/ovn/controller/patch.c
+++ b/ovn/controller/patch.c
@@ -96,6 +96,7 @@  create_patch_port(struct controller_ctx *ctx,
     ovsrec_bridge_set_ports(src, ports, src->n_ports + 1);
 
     free(ports);
+    force_full_process();
 }
 
 static void
@@ -124,6 +125,7 @@  remove_port(struct controller_ctx *ctx,
             ovsrec_bridge_set_ports(bridge, new_ports, bridge->n_ports - 1);
             free(new_ports);
             ovsrec_port_delete(port);
+            force_full_process();
             return;
         }
     }
diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
index 0f1aa63..084eea2 100644
--- a/ovn/controller/physical.c
+++ b/ovn/controller/physical.c
@@ -56,10 +56,6 @@  physical_register_ovs_idl(struct ovsdb_idl *ovs_idl)
     ovsdb_idl_add_column(ovs_idl, &ovsrec_interface_col_external_ids);
 }
 
-static struct simap localvif_to_ofport =
-    SIMAP_INITIALIZER(&localvif_to_ofport);
-static struct hmap tunnels = HMAP_INITIALIZER(&tunnels);
-
 /* Maps from a chassis to the OpenFlow port number of the tunnel that can be
  * used to reach that chassis. */
 struct chassis_tunnel {
@@ -70,11 +66,11 @@  struct chassis_tunnel {
 };
 
 static struct chassis_tunnel *
-chassis_tunnel_find(const char *chassis_id)
+chassis_tunnel_find(struct hmap *tunnels_p, const char *chassis_id)
 {
     struct chassis_tunnel *tun;
     HMAP_FOR_EACH_WITH_HASH (tun, hmap_node, hash_string(chassis_id, 0),
-                             &tunnels) {
+                             tunnels_p) {
         if (!strcmp(tun->chassis_id, chassis_id)) {
             return tun;
         }
@@ -293,7 +289,9 @@  consider_port_binding(enum mf_field_id mff_ovn_geneve,
                       const struct sbrec_port_binding *binding,
                       const struct sbrec_chassis *chassis,
                       struct ofpbuf *ofpacts_p,
-                      struct hmap *flow_table)
+                      struct hmap *flow_table,
+                      struct simap *localvif_to_ofport_p,
+                      struct hmap *tunnels_p)
 {
     uint32_t dp_key = binding->datapath->tunnel_key;
     uint32_t port_key = binding->tunnel_key;
@@ -444,14 +442,14 @@  consider_port_binding(enum mf_field_id mff_ovn_geneve,
         if (!binding->tag) {
             return;
         }
-        ofport = u16_to_ofp(simap_get(&localvif_to_ofport,
+        ofport = u16_to_ofp(simap_get(localvif_to_ofport_p,
                                       binding->parent_port));
         if (ofport) {
             tag = *binding->tag;
             nested_container = true;
         }
     } else {
-        ofport = u16_to_ofp(simap_get(&localvif_to_ofport,
+        ofport = u16_to_ofp(simap_get(localvif_to_ofport_p,
                                       binding->logical_port));
         if ((!strcmp(binding->type, "localnet")
             || !strcmp(binding->type, "l2gateway"))
@@ -470,13 +468,13 @@  consider_port_binding(enum mf_field_id mff_ovn_geneve,
             return;
         }
         if (localnet_port) {
-            ofport = u16_to_ofp(simap_get(&localvif_to_ofport,
+            ofport = u16_to_ofp(simap_get(localvif_to_ofport_p,
                                           localnet_port->logical_port));
             if (!ofport) {
                 return;
             }
         } else {
-            tun = chassis_tunnel_find(binding->chassis->name);
+            tun = chassis_tunnel_find(tunnels_p, binding->chassis->name);
             if (!tun) {
                 return;
             }
@@ -633,7 +631,9 @@  consider_mc_group(enum mf_field_id mff_ovn_geneve,
                   const struct sbrec_multicast_group *mc,
                   struct ofpbuf *ofpacts_p,
                   struct ofpbuf *remote_ofpacts_p,
-                  struct hmap *flow_table)
+                  struct hmap *flow_table,
+                  struct simap *localvif_to_ofport_p,
+                  struct hmap *tunnels_p)
 {
     uint32_t dp_key = mc->datapath->tunnel_key;
     if (!get_local_datapath(local_datapaths, dp_key)) {
@@ -681,7 +681,7 @@  consider_mc_group(enum mf_field_id mff_ovn_geneve,
             put_load(port->tunnel_key, MFF_LOG_OUTPORT, 0, 32,
                      remote_ofpacts_p);
             put_resubmit(OFTABLE_CHECK_LOOPBACK, remote_ofpacts_p);
-        } else if (simap_contains(&localvif_to_ofport,
+        } else if (simap_contains(localvif_to_ofport_p,
                            (port->parent_port && *port->parent_port)
                            ? port->parent_port : port->logical_port)
                    || (!strcmp(port->type, "l3gateway")
@@ -729,7 +729,7 @@  consider_mc_group(enum mf_field_id mff_ovn_geneve,
         const struct chassis_tunnel *prev = NULL;
         SSET_FOR_EACH (chassis, &remote_chassis) {
             const struct chassis_tunnel *tun
-                = chassis_tunnel_find(chassis);
+                = chassis_tunnel_find(tunnels_p, chassis);
             if (!tun) {
                 continue;
             }
@@ -753,12 +753,15 @@  consider_mc_group(enum mf_field_id mff_ovn_geneve,
     sset_destroy(&remote_chassis);
 }
 
-void
-physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
-             const struct ovsrec_bridge *br_int,
-             const struct sbrec_chassis *chassis,
-             const struct simap *ct_zones, struct lport_index *lports,
-             struct hmap *flow_table, struct hmap *local_datapaths)
+/* Scans the bridge 'br_int' and compares it to the supplied expected state.
+ * Returns true and updates the expected state if changes are detected.
+ * Returns false if no changes are found. */
+bool
+detect_and_save_physical_changes(struct simap *localvif_to_ofport_p,
+                                 struct hmap *tunnels_p,
+                                 enum mf_field_id mff_ovn_geneve,
+                                 const struct ovsrec_bridge *br_int,
+                                 const struct sbrec_chassis *chassis)
 {
 
     /* This bool tracks physical mapping changes. */
@@ -824,7 +827,8 @@  physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
                 }
 
                 simap_put(&new_tunnel_to_ofport, chassis_id, ofport);
-                struct chassis_tunnel *tun = chassis_tunnel_find(chassis_id);
+                struct chassis_tunnel *tun = chassis_tunnel_find(tunnels_p,
+                                                                 chassis_id);
                 if (tun) {
                     /* If the tunnel's ofport has changed, update. */
                     if (tun->ofport != u16_to_ofp(ofport) ||
@@ -835,7 +839,7 @@  physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
                     }
                 } else {
                     tun = xmalloc(sizeof *tun);
-                    hmap_insert(&tunnels, &tun->hmap_node,
+                    hmap_insert(tunnels_p, &tun->hmap_node,
                                 hash_string(chassis_id, 0));
                     tun->chassis_id = chassis_id;
                     tun->ofport = u16_to_ofp(ofport);
@@ -855,9 +859,9 @@  physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
 
     /* Remove tunnels that are no longer here. */
     struct chassis_tunnel *tun, *tun_next;
-    HMAP_FOR_EACH_SAFE (tun, tun_next, hmap_node, &tunnels) {
+    HMAP_FOR_EACH_SAFE (tun, tun_next, hmap_node, tunnels_p) {
         if (!simap_find(&new_tunnel_to_ofport, tun->chassis_id)) {
-            hmap_remove(&tunnels, &tun->hmap_node);
+            hmap_remove(tunnels_p, &tun->hmap_node);
             physical_map_changed = true;
             free(tun);
         }
@@ -865,29 +869,45 @@  physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
 
     /* Capture changed or removed openflow ports. */
     struct simap_node *vif_name, *vif_name_next;
-    SIMAP_FOR_EACH_SAFE (vif_name, vif_name_next, &localvif_to_ofport) {
+    SIMAP_FOR_EACH_SAFE (vif_name, vif_name_next, localvif_to_ofport_p) {
         int newport;
         if ((newport = simap_get(&new_localvif_to_ofport, vif_name->name))) {
-            if (newport != simap_get(&localvif_to_ofport, vif_name->name)) {
-                simap_put(&localvif_to_ofport, vif_name->name, newport);
+            if (newport != simap_get(localvif_to_ofport_p, vif_name->name)) {
+                simap_put(localvif_to_ofport_p, vif_name->name, newport);
                 physical_map_changed = true;
             }
         } else {
-            simap_find_and_delete(&localvif_to_ofport, vif_name->name);
+            simap_find_and_delete(localvif_to_ofport_p, vif_name->name);
             physical_map_changed = true;
         }
     }
     SIMAP_FOR_EACH (vif_name, &new_localvif_to_ofport) {
-        if (!simap_get(&localvif_to_ofport, vif_name->name)) {
-            simap_put(&localvif_to_ofport, vif_name->name,
+        if (!simap_get(localvif_to_ofport_p, vif_name->name)) {
+            simap_put(localvif_to_ofport_p, vif_name->name,
                       simap_get(&new_localvif_to_ofport, vif_name->name));
             physical_map_changed = true;
         }
     }
-    if (physical_map_changed) {
-        /* Reprocess logical flow table immediately. */
-        poll_immediate_wake();
-    }
+
+
+    simap_destroy(&new_localvif_to_ofport);
+    simap_destroy(&new_tunnel_to_ofport);
+
+    return physical_map_changed;
+}
+
+void
+physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
+             const struct ovsrec_bridge *br_int,
+             const struct sbrec_chassis *chassis,
+             const struct simap *ct_zones, struct lport_index *lports,
+             struct hmap *flow_table, struct hmap *local_datapaths)
+{
+    struct simap localvif_to_ofport = SIMAP_INITIALIZER(&localvif_to_ofport);
+    struct hmap tunnels = HMAP_INITIALIZER(&tunnels);
+
+    detect_and_save_physical_changes(&localvif_to_ofport, &tunnels,
+                                     mff_ovn_geneve, br_int, chassis);
 
     struct ofpbuf ofpacts;
     ofpbuf_init(&ofpacts, 0);
@@ -898,7 +918,8 @@  physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
     SBREC_PORT_BINDING_FOR_EACH (binding, ctx->ovnsb_idl) {
         consider_port_binding(mff_ovn_geneve, ct_zones, lports,
                               local_datapaths, binding, chassis,
-                              &ofpacts, flow_table);
+                              &ofpacts, flow_table,
+                              &localvif_to_ofport, &tunnels);
     }
 
     /* Handle output to multicast groups, in tables 32 and 33. */
@@ -907,7 +928,8 @@  physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
     ofpbuf_init(&remote_ofpacts, 0);
     SBREC_MULTICAST_GROUP_FOR_EACH (mc, ctx->ovnsb_idl) {
         consider_mc_group(mff_ovn_geneve, ct_zones, local_datapaths, chassis,
-                          mc, &ofpacts, &remote_ofpacts, flow_table);
+                          mc, &ofpacts, &remote_ofpacts, flow_table,
+                          &localvif_to_ofport, &tunnels);
     }
 
     ofpbuf_uninit(&remote_ofpacts);
@@ -923,6 +945,7 @@  physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
      * ports.  We set MFF_LOG_DATAPATH, MFF_LOG_INPORT, and
      * MFF_LOG_OUTPORT from the tunnel key data, then resubmit to table
      * 33 to handle packets to the local hypervisor. */
+    struct chassis_tunnel *tun;
     HMAP_FOR_EACH (tun, hmap_node, &tunnels) {
         struct match match = MATCH_CATCHALL_INITIALIZER;
         match_set_in_port(&match, tun->ofport);
@@ -1041,6 +1064,9 @@  physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
 
     ofpbuf_uninit(&ofpacts);
 
-    simap_destroy(&new_localvif_to_ofport);
-    simap_destroy(&new_tunnel_to_ofport);
+    simap_destroy(&localvif_to_ofport);
+    HMAP_FOR_EACH_POP (tun, hmap_node, &tunnels) {
+        free(tun);
+    }
+    hmap_destroy(&tunnels);
 }
diff --git a/ovn/controller/physical.h b/ovn/controller/physical.h
index e2debed..fc3d4c7 100644
--- a/ovn/controller/physical.h
+++ b/ovn/controller/physical.h
@@ -41,6 +41,11 @@  struct simap;
 #define OVN_GENEVE_LEN 4
 
 void physical_register_ovs_idl(struct ovsdb_idl *);
+bool detect_and_save_physical_changes(struct simap *localvif_to_ofport_p,
+                                      struct hmap *tunnels_p,
+                                      enum mf_field_id mff_ovn_geneve,
+                                      const struct ovsrec_bridge *br_int,
+                                      const struct sbrec_chassis *chassis);
 void physical_run(struct controller_ctx *, enum mf_field_id mff_ovn_geneve,
                   const struct ovsrec_bridge *br_int,
                   const struct sbrec_chassis *chassis,