[ovs-dev] [PATCH ovn v13 3/7] ovn-controller: Handle runtime data changes in flow output engine

Numan Siddique numans at ovn.org
Fri Jun 19 12:36:07 UTC 2020


On Fri, Jun 19, 2020 at 5:22 PM Dumitru Ceara <dceara at redhat.com> wrote:

> On 6/19/20 1:10 PM, numans at ovn.org wrote:
> > From: Numan Siddique <numans at ovn.org>
> >
> > In order to handle runtime data changes incrementally, the flow outut
> > runtime data handle should know the changed runtime data.
> > Runtime data now tracks the changed data for any OVS interface
> > and SB port binding changes. The tracked data contains a hmap
> > of tracked datapaths (which changed during runtime data processing.
> >
> > The flow outout runtime_data handler in this patch doesn't do much
> > with the tracked data. It returns false if there is tracked data
> available
> > so that flow_output run is called. If no tracked data is available
> > then there is no need for flow computation and the handler returns true.
> >
> > Next patch in the series processes the tracked data incrementally.
> >
> > Acked-by: Han Zhou <hzhou at ovn.org>
> > Acked-by: Dumitru Ceara <dceara at redhat.com>
> > Co-Authored-by: Venkata Anil <anilvenkata at redhat.com>
> > Signed-off-by: Venkata Anil <anilvenkata at redhat.com>
> > Signed-off-by: Numan Siddique <numans at ovn.org>
> > ---
> >  controller/binding.c        | 195 ++++++++++++++++++++++++++++++------
> >  controller/binding.h        |  23 +++++
> >  controller/ovn-controller.c | 143 +++++++++++++++++++++++++-
> >  tests/ovn-performance.at    |  20 ++--
> >  4 files changed, 336 insertions(+), 45 deletions(-)
> >
> > diff --git a/controller/binding.c b/controller/binding.c
> > index 67fd2777f..58e0bf59f 100644
> > --- a/controller/binding.c
> > +++ b/controller/binding.c
> > @@ -69,13 +69,24 @@ binding_register_ovs_idl(struct ovsdb_idl *ovs_idl)
> >      ovsdb_idl_add_column(ovs_idl, &ovsrec_qos_col_type);
> >  }
> >
> > +static struct tracked_binding_datapath *tracked_binding_datapath_create(
> > +    const struct sbrec_datapath_binding *,
> > +    bool is_new, struct hmap *tracked_dps);
> > +static struct tracked_binding_datapath *tracked_binding_datapath_find(
> > +    struct hmap *, const struct sbrec_datapath_binding *);
> > +static void tracked_binding_datapath_lport_add(
> > +    const struct sbrec_port_binding *, struct hmap *tracked_datapaths);
> > +static void update_lport_tracking(const struct sbrec_port_binding *pb,
> > +                                  struct hmap *tracked_dp_bindings);
> > +
> >  static void
> >  add_local_datapath__(struct ovsdb_idl_index
> *sbrec_datapath_binding_by_key,
> >                       struct ovsdb_idl_index
> *sbrec_port_binding_by_datapath,
> >                       struct ovsdb_idl_index *sbrec_port_binding_by_name,
> >                       const struct sbrec_datapath_binding *datapath,
> >                       bool has_local_l3gateway, int depth,
> > -                     struct hmap *local_datapaths)
> > +                     struct hmap *local_datapaths,
> > +                     struct hmap *tracked_datapaths)
> >  {
> >      uint32_t dp_key = datapath->tunnel_key;
> >      struct local_datapath *ld = get_local_datapath(local_datapaths,
> dp_key);
> > @@ -92,6 +103,11 @@ add_local_datapath__(struct ovsdb_idl_index
> *sbrec_datapath_binding_by_key,
> >      ld->localnet_port = NULL;
> >      ld->has_local_l3gateway = has_local_l3gateway;
> >
> > +    if (tracked_datapaths &&
> > +        !tracked_binding_datapath_find(tracked_datapaths, datapath)) {
> > +        tracked_binding_datapath_create(datapath, true,
> tracked_datapaths);
> > +    }
> > +
> >      if (depth >= 100) {
> >          static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> >          VLOG_WARN_RL(&rl, "datapaths nested too deep");
> > @@ -124,7 +140,8 @@ add_local_datapath__(struct ovsdb_idl_index
> *sbrec_datapath_binding_by_key,
> >
>  sbrec_port_binding_by_datapath,
> >                                               sbrec_port_binding_by_name,
> >                                               peer->datapath, false,
> > -                                             depth + 1,
> local_datapaths);
> > +                                             depth + 1, local_datapaths,
> > +                                             tracked_datapaths);
> >                      }
> >                      ld->n_peer_ports++;
> >                      if (ld->n_peer_ports > ld->n_allocated_peer_ports) {
> > @@ -147,12 +164,14 @@ add_local_datapath(struct ovsdb_idl_index
> *sbrec_datapath_binding_by_key,
> >                     struct ovsdb_idl_index
> *sbrec_port_binding_by_datapath,
> >                     struct ovsdb_idl_index *sbrec_port_binding_by_name,
> >                     const struct sbrec_datapath_binding *datapath,
> > -                   bool has_local_l3gateway, struct hmap
> *local_datapaths)
> > +                   bool has_local_l3gateway, struct hmap
> *local_datapaths,
> > +                   struct hmap *tracked_datapaths)
> >  {
> >      add_local_datapath__(sbrec_datapath_binding_by_key,
> >                           sbrec_port_binding_by_datapath,
> >                           sbrec_port_binding_by_name,
> > -                         datapath, has_local_l3gateway, 0,
> local_datapaths);
> > +                         datapath, has_local_l3gateway, 0,
> local_datapaths,
> > +                         tracked_datapaths);
> >  }
> >
> >  static void
> > @@ -506,6 +525,11 @@ update_local_lport_ids(struct binding_ctx_out
> *b_ctx,
> >               pb->datapath->tunnel_key, pb->tunnel_key);
> >      if (sset_add(b_ctx->local_lport_ids, buf) != NULL) {
> >          b_ctx->local_lport_ids_changed = true;
> > +
> > +        if (b_ctx->tracked_dp_bindings) {
> > +            /* Add the 'pb' to the tracked_datapaths. */
> > +            tracked_binding_datapath_lport_add(pb,
> b_ctx->tracked_dp_bindings);
> > +        }
> >      }
> >  }
> >
> > @@ -521,6 +545,11 @@ remove_local_lport_ids(struct binding_ctx_out
> *b_ctx,
> >               pb->datapath->tunnel_key, pb->tunnel_key);
> >      if (sset_find_and_delete(b_ctx->local_lport_ids, buf)) {
> >          b_ctx->local_lport_ids_changed = true;
> > +
> > +        if (b_ctx->tracked_dp_bindings) {
> > +            /* Add the 'pb' to the tracked_datapaths. */
> > +            tracked_binding_datapath_lport_add(pb,
> b_ctx->tracked_dp_bindings);
> > +        }
> >      }
> >  }
> >
> > @@ -669,6 +698,74 @@ is_lport_container(const struct sbrec_port_binding
> *pb)
> >      return is_lport_vif(pb) && pb->parent_port && pb->parent_port[0];
> >  }
> >
> > +static struct tracked_binding_datapath *
> > +tracked_binding_datapath_create(const struct sbrec_datapath_binding *dp,
> > +                                bool is_new,
> > +                                struct hmap *tracked_datapaths)
> > +{
> > +    struct tracked_binding_datapath *t_dp = xzalloc(sizeof *t_dp);
> > +    t_dp->dp = dp;
> > +    t_dp->is_new = is_new;
> > +    shash_init(&t_dp->lports);
> > +    hmap_insert(tracked_datapaths, &t_dp->node,
> uuid_hash(&dp->header_.uuid));
> > +    return t_dp;
> > +}
> > +
> > +static struct tracked_binding_datapath *
> > +tracked_binding_datapath_find(struct hmap *tracked_datapaths,
> > +                              const struct sbrec_datapath_binding *dp)
> > +{
> > +    struct tracked_binding_datapath *t_dp;
> > +    size_t hash = uuid_hash(&dp->header_.uuid);
> > +    HMAP_FOR_EACH_WITH_HASH (t_dp, node, hash, tracked_datapaths) {
> > +        if (uuid_equals(&t_dp->dp->header_.uuid, &dp->header_.uuid)) {
> > +            return t_dp;
> > +        }
> > +    }
> > +
> > +    return NULL;
> > +}
> > +
> > +static void
> > +tracked_binding_datapath_lport_add(const struct sbrec_port_binding *pb,
> > +                                   struct hmap *tracked_datapaths)
> > +{
> > +    if (!tracked_datapaths) {
> > +        return;
> > +    }
> > +
> > +    struct tracked_binding_datapath *tracked_dp =
> > +        tracked_binding_datapath_find(tracked_datapaths, pb->datapath);
> > +    if (!tracked_dp) {
> > +        tracked_dp = tracked_binding_datapath_create(pb->datapath,
> false,
> > +                                                     tracked_datapaths);
> > +    }
> > +
> > +    /* Check if the lport is already present or not.
> > +     * If it is already present, then just update the 'pb' field. */
> > +    struct tracked_binding_lport *lport =
> > +        shash_find_data(&tracked_dp->lports, pb->logical_port);
> > +
> > +    if (!lport) {
> > +        lport = xmalloc(sizeof *lport);
> > +        shash_add(&tracked_dp->lports, pb->logical_port, lport);
> > +    }
> > +
> > +    lport->pb = pb;
> > +}
> > +
> > +void
> > +binding_tracked_dp_destroy(struct hmap *tracked_datapaths)
> > +{
> > +    struct tracked_binding_datapath *t_dp;
> > +    HMAP_FOR_EACH_POP (t_dp, node, tracked_datapaths) {
> > +        shash_destroy_free_data(&t_dp->lports);
> > +        free(t_dp);
> > +    }
> > +
> > +    hmap_destroy(tracked_datapaths);
> > +}
> > +
> >  /* Corresponds to each Port_Binding.type. */
> >  enum en_lport_type {
> >      LP_UNKNOWN,
> > @@ -722,7 +819,7 @@ static bool
> >  claim_lport(const struct sbrec_port_binding *pb,
> >              const struct sbrec_chassis *chassis_rec,
> >              const struct ovsrec_interface *iface_rec,
> > -            bool sb_readonly)
> > +            bool sb_readonly, struct hmap *tracked_datapaths)
> >  {
> >      if (pb->chassis != chassis_rec) {
> >          if (sb_readonly) {
> > @@ -741,6 +838,10 @@ claim_lport(const struct sbrec_port_binding *pb,
> >          }
> >
> >          sbrec_port_binding_set_chassis(pb, chassis_rec);
> > +
> > +        if (tracked_datapaths) {
> > +            update_lport_tracking(pb, tracked_datapaths);
> > +        }
> >      }
> >
> >      /* Check if the port encap binding, if any, has changed */
> > @@ -758,9 +859,14 @@ claim_lport(const struct sbrec_port_binding *pb,
> >
> >  /* Returns false if lport is not released due to 'sb_readonly'.
> >   * Returns true otherwise.
> > + *
> > + * This function assumes that the the 'pb' was claimed
> > + * earlier i.e port binding's chassis is set to this chassis.
> > + * Caller should make sure that this is the case.
> >   */
> >  static bool
> > -release_lport(const struct sbrec_port_binding *pb, bool sb_readonly)
> > +release_lport(const struct sbrec_port_binding *pb, bool sb_readonly,
> > +              struct hmap *tracked_datapaths)
> >  {
> >      if (pb->encap) {
> >          if (sb_readonly) {
> > @@ -783,6 +889,7 @@ release_lport(const struct sbrec_port_binding *pb,
> bool sb_readonly)
> >          sbrec_port_binding_set_virtual_parent(pb, NULL);
> >      }
> >
> > +    update_lport_tracking(pb, tracked_datapaths);
> >      VLOG_INFO("Releasing lport %s from this chassis.",
> pb->logical_port);
> >      return true;
> >  }
> > @@ -828,13 +935,14 @@ is_lbinding_container_parent(struct local_binding
> *lbinding)
> >  static bool
> >  release_local_binding_children(const struct sbrec_chassis *chassis_rec,
> >                                 struct local_binding *lbinding,
> > -                               bool sb_readonly)
> > +                               bool sb_readonly,
> > +                               struct hmap *tracked_dp_bindings)
> >  {
> >      struct shash_node *node;
> >      SHASH_FOR_EACH (node, &lbinding->children) {
> >          struct local_binding *l = node->data;
> >          if (is_lbinding_this_chassis(l, chassis_rec)) {
> > -            if (!release_lport(l->pb, sb_readonly)) {
> > +            if (!release_lport(l->pb, sb_readonly,
> tracked_dp_bindings)) {
> >                  return false;
> >              }
> >          }
> > @@ -849,16 +957,17 @@ release_local_binding_children(const struct
> sbrec_chassis *chassis_rec,
> >
> >  static bool
> >  release_local_binding(const struct sbrec_chassis *chassis_rec,
> > -                      struct local_binding *lbinding, bool sb_readonly)
> > +                      struct local_binding *lbinding, bool sb_readonly,
> > +                      struct hmap *tracked_dp_bindings)
> >  {
> >      if (!release_local_binding_children(chassis_rec, lbinding,
> > -                                        sb_readonly)) {
> > +                                        sb_readonly,
> tracked_dp_bindings)) {
> >          return false;
> >      }
> >
> >      bool retval = true;
> >      if (is_lbinding_this_chassis(lbinding, chassis_rec)) {
> > -        retval = release_lport(lbinding->pb, sb_readonly);
> > +        retval = release_lport(lbinding->pb, sb_readonly,
> tracked_dp_bindings);
> >      }
> >
> >      lbinding->pb = NULL;
> > @@ -879,7 +988,8 @@ consider_vif_lport_(const struct sbrec_port_binding
> *pb,
> >          if (can_bind) {
> >              /* We can claim the lport. */
> >              if (!claim_lport(pb, b_ctx_in->chassis_rec, lbinding->iface,
> > -                             !b_ctx_in->ovnsb_idl_txn)){
> > +                             !b_ctx_in->ovnsb_idl_txn,
> > +                             b_ctx_out->tracked_dp_bindings)){
> >                  return false;
> >              }
> >
> > @@ -887,7 +997,8 @@ consider_vif_lport_(const struct sbrec_port_binding
> *pb,
> >                                 b_ctx_in->sbrec_port_binding_by_datapath,
> >                                 b_ctx_in->sbrec_port_binding_by_name,
> >                                 pb->datapath, false,
> > -                               b_ctx_out->local_datapaths);
> > +                               b_ctx_out->local_datapaths,
> > +                               b_ctx_out->tracked_dp_bindings);
> >              update_local_lport_ids(b_ctx_out, pb);
> >              if (lbinding->iface && qos_map && b_ctx_in->ovs_idl_txn) {
> >                  get_qos_params(pb, qos_map);
> > @@ -907,7 +1018,8 @@ consider_vif_lport_(const struct sbrec_port_binding
> *pb,
> >      if (pb->chassis == b_ctx_in->chassis_rec) {
> >          /* Release the lport if there is no lbinding. */
> >          if (!lbinding_set || !can_bind) {
> > -            return release_lport(pb, !b_ctx_in->ovnsb_idl_txn);
> > +            return release_lport(pb, !b_ctx_in->ovnsb_idl_txn,
> > +                                 b_ctx_out->tracked_dp_bindings);
> >          }
> >      }
> >
> > @@ -994,7 +1106,8 @@ consider_container_lport(const struct
> sbrec_port_binding *pb,
> >               * release the container lport, if it was bound earlier. */
> >              if (is_lbinding_this_chassis(container_lbinding,
> >                                           b_ctx_in->chassis_rec)) {
> > -               return release_lport(pb, !b_ctx_in->ovnsb_idl_txn);
> > +               return release_lport(pb, !b_ctx_in->ovnsb_idl_txn,
> > +                                    b_ctx_out->tracked_dp_bindings);
> >              }
> >
> >              return true;
> > @@ -1074,13 +1187,16 @@ consider_nonvif_lport_(const struct
> sbrec_port_binding *pb,
> >                             b_ctx_in->sbrec_port_binding_by_datapath,
> >                             b_ctx_in->sbrec_port_binding_by_name,
> >                             pb->datapath, has_local_l3gateway,
> > -                           b_ctx_out->local_datapaths);
> > +                           b_ctx_out->local_datapaths,
> > +                           b_ctx_out->tracked_dp_bindings);
> >
> >          update_local_lport_ids(b_ctx_out, pb);
> >          return claim_lport(pb, b_ctx_in->chassis_rec, NULL,
> > -                           !b_ctx_in->ovnsb_idl_txn);
> > +                           !b_ctx_in->ovnsb_idl_txn,
> > +                           b_ctx_out->tracked_dp_bindings);
> >      } else if (pb->chassis == b_ctx_in->chassis_rec) {
> > -        return release_lport(pb, !b_ctx_in->ovnsb_idl_txn);
> > +        return release_lport(pb, !b_ctx_in->ovnsb_idl_txn,
> > +                             b_ctx_out->tracked_dp_bindings);
> >      }
> >
> >      return true;
> > @@ -1116,7 +1232,7 @@ consider_localnet_lport(const struct
> sbrec_port_binding *pb,
> >                          struct binding_ctx_out *b_ctx_out,
> >                          struct hmap *qos_map)
> >  {
> > -    /* Add all localnet ports to local_lports so that we allocate ct
> zones
> > +    /* Add all localnet ports to local_ifaces so that we allocate ct
> zones
> >       * for them. */
> >      update_local_lports(b_ctx_out, pb->logical_port);
> >
> > @@ -1152,7 +1268,9 @@ consider_ha_lport(const struct sbrec_port_binding
> *pb,
> >                             b_ctx_in->sbrec_port_binding_by_datapath,
> >                             b_ctx_in->sbrec_port_binding_by_name,
> >                             pb->datapath, false,
> > -                           b_ctx_out->local_datapaths);
> > +                           b_ctx_out->local_datapaths,
> > +                           b_ctx_out->tracked_dp_bindings);
> > +        update_local_lport_ids(b_ctx_out, pb);
> >      }
> >
> >      return consider_nonvif_lport_(pb, our_chassis, false, b_ctx_in,
> b_ctx_out);
> > @@ -1442,7 +1560,8 @@ add_local_datapath_peer_port(const struct
> sbrec_port_binding *pb,
> >                               b_ctx_in->sbrec_port_binding_by_datapath,
> >                               b_ctx_in->sbrec_port_binding_by_name,
> >                               peer->datapath, false,
> > -                             1, b_ctx_out->local_datapaths);
> > +                             1, b_ctx_out->local_datapaths,
> > +                             b_ctx_out->tracked_dp_bindings);
> >          return;
> >      }
> >
> > @@ -1522,6 +1641,17 @@ remove_pb_from_local_datapath(const struct
> sbrec_port_binding *pb,
> >      }
> >  }
> >
> > +static void
> > +update_lport_tracking(const struct sbrec_port_binding *pb,
> > +                      struct hmap *tracked_dp_bindings)
> > +{
> > +    if (!tracked_dp_bindings) {
> > +        return;
> > +    }
> > +
> > +    tracked_binding_datapath_lport_add(pb, tracked_dp_bindings);
> > +}
> > +
> >  /* Considers the ovs iface 'iface_rec' for claiming.
> >   * This function should be called if the external_ids:iface-id
> >   * and 'ofport' are set for the 'iface_rec'.
> > @@ -1618,7 +1748,8 @@ consider_iface_release(const struct
> ovsrec_interface *iface_rec,
> >           * lbinding->iface.
> >           * Cannot access these members of lbinding after this call. */
> >          if (!release_local_binding(b_ctx_in->chassis_rec, lbinding,
> > -                                   !b_ctx_in->ovnsb_idl_txn)) {
> > +                                   !b_ctx_in->ovnsb_idl_txn,
> > +                                   b_ctx_out->tracked_dp_bindings)) {
> >              return false;
> >          }
> >      }
> > @@ -1831,9 +1962,10 @@ handle_deleted_vif_lport(const struct
> sbrec_port_binding *pb,
> >           * clear the 'chassis' column of 'pb'. But we need to do
> >           * for the local_binding's children. */
> >          if (lbinding->type == BT_VIF &&
> > -                !release_local_binding_children(b_ctx_in->chassis_rec,
> > -                                                lbinding,
> > -
> !b_ctx_in->ovnsb_idl_txn)) {
> > +                !release_local_binding_children(
> > +                    b_ctx_in->chassis_rec, lbinding,
> > +                    !b_ctx_in->ovnsb_idl_txn,
> > +                    b_ctx_out->tracked_dp_bindings)) {
> >              return false;
> >          }
> >      }
> > @@ -1906,7 +2038,6 @@ binding_handle_port_binding_changes(struct
> binding_ctx_in *b_ctx_in,
> >                                      struct binding_ctx_out *b_ctx_out)
> >  {
> >      bool handled = true;
> > -
> >      const struct sbrec_port_binding *pb;
> >
> >      /* Run the tracked port binding loop twice. One to handle deleted
> > @@ -1963,7 +2094,6 @@ binding_handle_port_binding_changes(struct
> binding_ctx_in *b_ctx_in,
> >          case LP_PATCH:
> >          case LP_LOCALPORT:
> >          case LP_VTEP:
> > -            b_ctx_out->non_vif_ports_changed = true;
> >              update_local_lport_ids(b_ctx_out, pb);
> >              if (lport_type ==  LP_PATCH) {
> >                  /* Add the peer datapath to the local datapaths if it's
> > @@ -1973,30 +2103,31 @@ binding_handle_port_binding_changes(struct
> binding_ctx_in *b_ctx_in,
> >                      add_local_datapath_peer_port(pb, b_ctx_in,
> b_ctx_out, ld);
> >                  }
> >              }
> > +
> > +            if (lport_type == LP_VTEP) {
> > +                /* VTEP lports are claimed/released by
> ovn-controller-vteps.
> > +                 * We are not sure what changed. */
> > +                b_ctx_out->non_vif_ports_changed = true;
> > +            }
> >              break;
> >
> >          case LP_L2GATEWAY:
> > -            b_ctx_out->non_vif_ports_changed = true;
> >              handled = consider_l2gw_lport(pb, b_ctx_in, b_ctx_out);
> >              break;
> >
> >          case LP_L3GATEWAY:
> > -            b_ctx_out->non_vif_ports_changed = true;
> >              handled = consider_l3gw_lport(pb, b_ctx_in, b_ctx_out);
> >              break;
> >
> >          case LP_CHASSISREDIRECT:
> > -            b_ctx_out->non_vif_ports_changed = true;
> >              handled = consider_cr_lport(pb, b_ctx_in, b_ctx_out);
> >              break;
> >
> >          case LP_EXTERNAL:
> > -            b_ctx_out->non_vif_ports_changed = true;
> >              handled = consider_external_lport(pb, b_ctx_in, b_ctx_out);
> >              break;
> >
> >          case LP_LOCALNET: {
> > -            b_ctx_out->non_vif_ports_changed = true;
> >              consider_localnet_lport(pb, b_ctx_in, b_ctx_out,
> qos_map_ptr);
> >
> >              struct shash bridge_mappings =
> > diff --git a/controller/binding.h b/controller/binding.h
> > index 161766d2f..c9740560f 100644
> > --- a/controller/binding.h
> > +++ b/controller/binding.h
> > @@ -19,6 +19,9 @@
> >
> >  #include <stdbool.h>
> >  #include "openvswitch/shash.h"
> > +#include "openvswitch/hmap.h"
> > +#include "openvswitch/uuid.h"
> > +#include "openvswitch/list.h"
> >
> >  struct hmap;
> >  struct ovsdb_idl;
> > @@ -75,6 +78,12 @@ struct binding_ctx_out {
> >      /* smap of OVS interface name as key and
> >       * OVS interface external_ids:iface-id as value. */
> >      struct smap *local_iface_ids;
> > +
> > +    /* hmap of 'struct tracked_binding_datapath' which the
> > +     * callee (binding_handle_ovs_interface_changes and
> > +     * binding_handle_port_binding_changes) fills in for
> > +     * the changed datapaths and port bindings. */
> > +    struct hmap *tracked_dp_bindings;
> >  };
> >
> >  enum local_binding_type {
> > @@ -99,6 +108,19 @@ local_binding_find(struct shash *local_bindings,
> const char *name)
> >      return shash_find_data(local_bindings, name);
> >  }
> >
> > +/* Represents a tracked binding logical port. */
> > +struct tracked_binding_lport {
> > +    const struct sbrec_port_binding *pb;
> > +};
> > +
> > +/* Represent a tracked binding datapath. */
> > +struct tracked_binding_datapath {
> > +    struct hmap_node node;
> > +    const struct sbrec_datapath_binding *dp;
> > +    bool is_new;
> > +    struct shash lports; /* shash of struct tracked_binding_lport. */
> > +};
> > +
> >  void binding_register_ovs_idl(struct ovsdb_idl *);
> >  void binding_run(struct binding_ctx_in *, struct binding_ctx_out *);
> >  bool binding_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn,
> > @@ -111,4 +133,5 @@ bool binding_handle_ovs_interface_changes(struct
> binding_ctx_in *,
> >                                            struct binding_ctx_out *);
> >  bool binding_handle_port_binding_changes(struct binding_ctx_in *,
> >                                           struct binding_ctx_out *);
> > +void binding_tracked_dp_destroy(struct hmap *tracked_datapaths);
> >  #endif /* controller/binding.h */
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index dde531c6e..29712d2f6 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -980,10 +980,88 @@ struct ed_type_runtime_data {
> >      struct sset local_lport_ids;
> >      struct sset active_tunnels;
> >
> > +    /* runtime data engine private data. */
> >      struct sset egress_ifaces;
> >      struct smap local_iface_ids;
> > +
> > +    /* Tracked data. See below for more details and comments. */
> > +    bool tracked;
> > +    bool local_lports_changed;
> > +    struct hmap tracked_dp_bindings;
> >  };
> >
> > +/* struct ed_type_runtime_data has the below members for tracking the
> > + * changes done to the runtime_data engine by the runtime_data engine
> > + * handlers. Since this engine is an input to the flow_output engine,
> > + * the flow output runtime data handler will make use of this tracked
> data.
> > + *
> > + *
> ------------------------------------------------------------------------
> > + * |                      | This is a hmap of
>      |
> > + * |                      | 'struct tracked_binding_datapath' defined
> in    |
> > + * |                      | binding.h. Runtime data handlers for OVS
>     |
> > + * |                      | Interface and Port Binding changes store
> the    |
> > + * | @tracked_dp_bindings | changed datapaths (datapaths added/removed
> from |
> > + * |                      | local_datapaths) and changed port bindings
>     |
> > + * |                      | (added/updated/deleted in
> 'local_bindings').    |
> > + * |                      | So any changes to the runtime data -
>     |
> > + * |                      | local_datapaths and local_bindings is
> captured  |
> > + * |                      | here.
>      |
> > + *
> ------------------------------------------------------------------------
> > + * |                      | This is a bool which represents if the
> runtime  |
> > + * |                      | data 'local_lports' changed by the runtime
> data |
> > + * |                      | handlers for OVS Interface and Port
> Binding     |
> > + * |                      | changes. If 'local_lports' is updated and
> also  |
> > + * |                      | results in any port binding updates, it is
>     |
> > + * |@local_lports_changed | captured in the @tracked_dp_bindings. So
> there  |
> > + * |                      | is no need to capture the changes in the
>     |
> > + * |                      | local_lports. If @local_lports_changed is
> true  |
> > + * |                      | but without anydata in the
> @tracked_dp_bindings,|
> > + * |                      | it means we needto only update the SB
> monitor   |
> > + * |                      | clauses and there isno need for any flow
>     |
> > + * |                      | (re)computations.
>      |
> > + *
> ------------------------------------------------------------------------
> > + * |                      | This represents if the data was tracked or
> not  |
> > + * |                      | by the runtime data handlers during the
> engine  |
> > + * |   @tracked           | run. If the runtime data recompute is
>      |
> > + * |                      | triggered, it means there is no tracked
> data.   |
> > + *
> ------------------------------------------------------------------------
> > + *
> > + *
> > + * The changes to the following runtime_data variables are not tracked.
> > + *
> > + *
> ---------------------------------------------------------------------
> > + * | local_datapaths  | The changes to these runtime data is captured
> in |
> > + * | local_bindings   | the @tracked_dp_bindings indirectly and hence
> it |
> > + * | local_lport_ids  | is not tracked explicitly.
>  |
> > + *
> ---------------------------------------------------------------------
> > + * | local_iface_ids  | This is used internally within the runtime
> data  |
> > + * | egress_ifaces    | engine (used only in binding.c) and hence
> there  |
> > + * |                  | there is no need to track.
>  |
> > + *
> ---------------------------------------------------------------------
> > + * |                  | Active tunnels is built in the
>  |
> > + * |                  | bfd_calculate_active_tunnels() for the tunnel
>   |
> > + * |                  | OVS interfaces. Any changes to non VIF OVS
>  |
> > + * |                  | interfaces results in triggering the full
>   |
> > + * | active_tunnels   | recompute of runtime data engine and hence
> there |
> > + * |                  | the tracked data doesn't track it. When we
>  |
> > + * |                  | support handling changes to non VIF OVS
>   |
> > + * |                  | interfaces we need to track the changes to the
>  |
> > + * |                  | active tunnels.
>   |
> > + *
> ---------------------------------------------------------------------
> > + *
> > + */
> > +
> > +static void
> > +en_runtime_data_clear_tracked_data(void *data_)
> > +{
> > +    struct ed_type_runtime_data *data = data_;
> > +
> > +    binding_tracked_dp_destroy(&data->tracked_dp_bindings);
> > +    hmap_init(&data->tracked_dp_bindings);
> > +    data->local_lports_changed = false;
> > +    data->tracked = false;
> > +}
> > +
> >  static void *
> >  en_runtime_data_init(struct engine_node *node OVS_UNUSED,
> >                       struct engine_arg *arg OVS_UNUSED)
> > @@ -997,6 +1075,10 @@ en_runtime_data_init(struct engine_node *node
> OVS_UNUSED,
> >      sset_init(&data->egress_ifaces);
> >      smap_init(&data->local_iface_ids);
> >      local_bindings_init(&data->local_bindings);
> > +
> > +    /* Init the tracked data. */
> > +    hmap_init(&data->tracked_dp_bindings);
> > +
> >      return data;
> >  }
> >
> > @@ -1019,6 +1101,8 @@ en_runtime_data_cleanup(void *data)
> >      }
> >      hmap_destroy(&rt_data->local_datapaths);
> >      local_bindings_destroy(&rt_data->local_bindings);
> > +
> > +    en_runtime_data_clear_tracked_data(data);
>
> Sorry for not spotting this earlier but this is not needed anymore
> because we call the clear_tracked_data handler in the engine_cleanup
> routine.
>
> I guess this can be removed at commit time though.
>

Ack. I'll remove it.

Thanks
Numan


>
> >  }
> >
> >  static void
> > @@ -1102,6 +1186,8 @@ init_binding_ctx(struct engine_node *node,
> >      b_ctx_out->egress_ifaces = &rt_data->egress_ifaces;
> >      b_ctx_out->local_bindings = &rt_data->local_bindings;
> >      b_ctx_out->local_iface_ids = &rt_data->local_iface_ids;
> > +    b_ctx_out->tracked_dp_bindings = NULL;
> > +    b_ctx_out->local_lports_changed = NULL;
> >  }
> >
> >  static void
> > @@ -1113,6 +1199,23 @@ en_runtime_data_run(struct engine_node *node,
> void *data)
> >      struct sset *local_lport_ids = &rt_data->local_lport_ids;
> >      struct sset *active_tunnels = &rt_data->active_tunnels;
> >
> > +    /* Clear the (stale) tracked data if any. Even though the tracked
> data
> > +     * gets cleared in the beginning of engine_init_run(),
> > +     * any of the runtime data handler might have set some tracked
> > +     * data and later another runtime data handler might return false
> > +     * resulting in full recompute of runtime engine and rendering the
> tracked
> > +     * data stale.
> > +     *
> > +     * It's possible that engine framework can be enhanced to indicate
> > +     * the node handlers (in this case flow_output_runtime_data_handler)
> > +     * that its input node had a full recompute. However we would still
> > +     * need to clear the tracked data, because we don't want the
> > +     * stale tracked data to be accessed outside of the engine, since
> the
> > +     * tracked data is cleared in the engine_init_run() and not at the
> > +     * end of the engine run.
> > +     * */
> > +    en_runtime_data_clear_tracked_data(data);
> > +
> >      static bool first_run = true;
> >      if (first_run) {
> >          /* don't cleanup since there is no data yet */
> > @@ -1168,6 +1271,8 @@ runtime_data_ovs_interface_handler(struct
> engine_node *node, void *data)
> >      struct binding_ctx_in b_ctx_in;
> >      struct binding_ctx_out b_ctx_out;
> >      init_binding_ctx(node, rt_data, &b_ctx_in, &b_ctx_out);
> > +    rt_data->tracked = true;
> > +    b_ctx_out.tracked_dp_bindings = &rt_data->tracked_dp_bindings;
> >
> >      if (!binding_handle_ovs_interface_changes(&b_ctx_in, &b_ctx_out)) {
> >          return false;
> > @@ -1175,6 +1280,7 @@ runtime_data_ovs_interface_handler(struct
> engine_node *node, void *data)
> >
> >      if (b_ctx_out.local_lports_changed) {
> >          engine_set_node_state(node, EN_UPDATED);
> > +        rt_data->local_lports_changed = b_ctx_out.local_lports_changed;
> >      }
> >
> >      return true;
> > @@ -1191,12 +1297,16 @@ runtime_data_sb_port_binding_handler(struct
> engine_node *node, void *data)
> >          return false;
> >      }
> >
> > +    rt_data->tracked = true;
> > +    b_ctx_out.tracked_dp_bindings = &rt_data->tracked_dp_bindings;
> > +
> >      if (!binding_handle_port_binding_changes(&b_ctx_in, &b_ctx_out)) {
> >          return false;
> >      }
> >
> >      if (b_ctx_out.local_lport_ids_changed ||
> > -            b_ctx_out.non_vif_ports_changed) {
> > +            b_ctx_out.non_vif_ports_changed ||
> > +            !hmap_is_empty(b_ctx_out.tracked_dp_bindings)) {
> >          engine_set_node_state(node, EN_UPDATED);
> >      }
> >
> > @@ -1908,6 +2018,32 @@ flow_output_physical_flow_changes_handler(struct
> engine_node *node, void *data)
> >      return true;
> >  }
> >
> > +static bool
> > +flow_output_runtime_data_handler(struct engine_node *node,
> > +                                 void *data OVS_UNUSED)
> > +{
> > +    struct ed_type_runtime_data *rt_data =
> > +        engine_get_input_data("runtime_data", node);
> > +
> > +    /* There is no tracked data. Fall back to full recompute of
> > +     * flow_output. */
> > +    if (!rt_data->tracked) {
> > +        return false;
> > +    }
> > +
> > +    if (!hmap_is_empty(&rt_data->tracked_dp_bindings)) {
> > +        /* We are not yet handling the tracked datapath binding
> > +         * changes. Return false to trigger full recompute. */
> > +        return false;
> > +    }
> > +
> > +    if (rt_data->local_lports_changed) {
> > +        engine_set_node_state(node, EN_UPDATED);
> > +    }
> > +
> > +    return true;
> > +}
> > +
> >  struct ovn_controller_exit_args {
> >      bool *exiting;
> >      bool *restart;
> > @@ -2029,7 +2165,7 @@ main(int argc, char *argv[])
> >
> >      /* Define inc-proc-engine nodes. */
> >      ENGINE_NODE_CUSTOM_DATA(ct_zones, "ct_zones");
> > -    ENGINE_NODE(runtime_data, "runtime_data");
> > +    ENGINE_NODE_WITH_CLEAR_TRACK_DATA(runtime_data, "runtime_data");
> >      ENGINE_NODE(mff_ovn_geneve, "mff_ovn_geneve");
> >      ENGINE_NODE(ofctrl_is_connected, "ofctrl_is_connected");
> >      ENGINE_NODE_WITH_CLEAR_TRACK_DATA(physical_flow_changes,
> > @@ -2066,7 +2202,8 @@ main(int argc, char *argv[])
> >                       flow_output_addr_sets_handler);
> >      engine_add_input(&en_flow_output, &en_port_groups,
> >                       flow_output_port_groups_handler);
> > -    engine_add_input(&en_flow_output, &en_runtime_data, NULL);
> > +    engine_add_input(&en_flow_output, &en_runtime_data,
> > +                     flow_output_runtime_data_handler);
> >      engine_add_input(&en_flow_output, &en_mff_ovn_geneve, NULL);
> >      engine_add_input(&en_flow_output, &en_physical_flow_changes,
> >                       flow_output_physical_flow_changes_handler);
> > diff --git a/tests/ovn-performance.at b/tests/ovn-performance.at
> > index 5cc1960b6..08acd3bae 100644
> > --- a/tests/ovn-performance.at
> > +++ b/tests/ovn-performance.at
> > @@ -286,11 +286,11 @@ for i in 1 2; do
> >          [hv1 hv2], [lflow_run],
> >          [ovn-nbctl --wait=hv lsp-set-type $lsp router]
> >      )
> > -    OVN_CONTROLLER_EXPECT_HIT(
> > +    OVN_CONTROLLER_EXPECT_NO_HIT(
> >          [hv1 hv2], [lflow_run],
> >          [ovn-nbctl --wait=hv lsp-set-options $lsp router-port=$lrp]
> >      )
> > -    OVN_CONTROLLER_EXPECT_HIT(
> > +    OVN_CONTROLLER_EXPECT_NO_HIT(
> >          [hv1 hv2], [lflow_run],
> >          [ovn-nbctl --wait=hv lsp-set-addresses $lsp router]
> >      )
> > @@ -416,15 +416,15 @@ OVN_CONTROLLER_EXPECT_NO_HIT(
> >      [ovn-nbctl --wait=hv destroy Port_Group pg1]
> >  )
> >
> > -for i in 1 2; do
> > -    ls=ls$i
> > +OVN_CONTROLLER_EXPECT_HIT(
> > +    [hv1 hv2], [lflow_run],
> > +    [ovn-nbctl --wait=hv ls-del ls1]
> > +)
> >
> > -    # Delete switch $ls
> > -    OVN_CONTROLLER_EXPECT_HIT(
> > -        [hv1 hv2], [lflow_run],
> > -        [ovn-nbctl --wait=hv ls-del $ls]
> > -    )
> > -done
> > +OVN_CONTROLLER_EXPECT_NO_HIT(
> > +    [hv1 hv2], [lflow_run],
> > +    [ovn-nbctl --wait=hv ls-del ls2]
> > +)
> >
> >  # Delete router lr1
> >  OVN_CONTROLLER_EXPECT_NO_HIT(
> >
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>


More information about the dev mailing list