[ovs-dev] [PATCH ovn v10 6/8] ovn-controller: Handle runtime data changes in flow output engine

Dumitru Ceara dceara at redhat.com
Tue Jun 9 09:40:42 UTC 2020


On 6/8/20 3:50 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.
> 
> 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        | 299 ++++++++++++++++++++++++++++--------
>  controller/binding.h        |  37 +++++
>  controller/ovn-controller.c | 144 +++++++++++++++--
>  tests/ovn-performance.at    |  20 +--
>  4 files changed, 413 insertions(+), 87 deletions(-)
> 
> diff --git a/controller/binding.c b/controller/binding.c
> index c2d9a4c6b..3c226cd7d 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -69,13 +69,26 @@ 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 *, bool deleted,
> +    struct hmap *tracked_datapaths);
> +static void update_lport_tracking(const struct sbrec_port_binding *pb,
> +                                  bool old_claim, bool new_claim,
> +                                  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 +105,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 +142,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 +166,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
> @@ -473,23 +494,45 @@ update_ld_localnet_port(const struct sbrec_port_binding *binding_rec,
>  
>  static void
>  update_local_lport_ids(struct sset *local_lport_ids,
> -                       const struct sbrec_port_binding *pb)
> +                       const struct sbrec_port_binding *pb,
> +                       struct hmap *tracked_datapaths)
>  {
>      char buf[16];
>      snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64,
>               pb->datapath->tunnel_key, pb->tunnel_key);
> -    sset_add(local_lport_ids, buf);
> +    bool added = sset_add(local_lport_ids, buf);

Hi Numan,

I guess this should be:

bool added = !!sset_add(local_lport_ids, buf);

> +    if (added && tracked_datapaths) {
> +        /* Add the 'pb' to the tracked_datapaths. */
> +        tracked_binding_datapath_lport_add(pb, false, tracked_datapaths);
> +    }
>  }
>  
>  static void
> -remove_local_lport_ids(const struct sbrec_port_binding *binding_rec,
> -                       struct sset *local_lport_ids)
> +remove_local_lport_ids(const struct sbrec_port_binding *pb,
> +                       struct sset *local_lport_ids,
> +                       struct hmap *tracked_datapaths)
>  {
>      char buf[16];
>      snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64,
> -                binding_rec->datapath->tunnel_key,
> -                binding_rec->tunnel_key);
> -    sset_find_and_delete(local_lport_ids, buf);
> +             pb->datapath->tunnel_key, pb->tunnel_key);
> +    bool deleted = sset_find_and_delete(local_lport_ids, buf);
> +    if (deleted && tracked_datapaths) {
> +        /* Add the 'pb' to the tracked_datapaths. */
> +        tracked_binding_datapath_lport_add(pb, true, tracked_datapaths);
> +    }
> +}
> +
> +static bool
> +add_lport_to_local_lports(struct sset *local_lports, const char *lport_name)
> +{
> +    return !!sset_add(local_lports, lport_name);
> +}
> +
> +static bool
> +remove_lport_from_local_lports(struct sset *local_lports,
> +                               const char *lport_name)
> +{
> +    return sset_find_and_delete(local_lports, lport_name);
>  }
>  
>  /* Local bindings. binding.c module binds the logical port (represented by
> @@ -637,6 +680,77 @@ 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,
> +                                   bool deleted,
> +                                   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' and 'deleted'
> +     * fields. */
> +    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;
> +    lport->deleted = deleted;
> +}
> +
> +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,
> @@ -690,7 +804,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) {
> @@ -709,6 +823,10 @@ claim_lport(const struct sbrec_port_binding *pb,
>          }
>  
>          sbrec_port_binding_set_chassis(pb, chassis_rec);
> +
> +        if (tracked_datapaths) {
> +            update_lport_tracking(pb, false, true, tracked_datapaths);
> +        }
>      }
>  
>      /* Check if the port encap binding, if any, has changed */
> @@ -726,9 +844,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 that the the 'pb' was claimed
> + * earlier i.e port binding's chassis is set to this chassis.
> + * Caller should make sure that, that's the case.

Can we skip "that, " above? (non-native speaker here).

>   */
>  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) {
> @@ -751,6 +874,7 @@ release_lport(const struct sbrec_port_binding *pb, bool sb_readonly)
>          sbrec_port_binding_set_virtual_parent(pb, NULL);
>      }
>  
> +    update_lport_tracking(pb, true, false, tracked_datapaths);
>      VLOG_INFO("Releasing lport %s from this chassis.", pb->logical_port);
>      return true;
>  }
> @@ -796,13 +920,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;
>              }
>          }
> @@ -817,16 +942,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;
> @@ -847,7 +973,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;
>              }
>  
> @@ -855,8 +982,10 @@ 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);
> -            update_local_lport_ids(b_ctx_out->local_lport_ids, pb);
> +                               b_ctx_out->local_datapaths,
> +                               b_ctx_out->tracked_dp_bindings);
> +            update_local_lport_ids(b_ctx_out->local_lport_ids, pb,
> +                                   b_ctx_out->tracked_dp_bindings);
>              if (lbinding->iface && qos_map && b_ctx_in->ovs_idl_txn) {
>                  get_qos_params(pb, qos_map);
>              }
> @@ -875,7 +1004,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);
>          }
>      }
>  
> @@ -962,7 +1092,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;
> @@ -1037,18 +1168,22 @@ consider_nonvif_lport_(const struct sbrec_port_binding *pb,
>                         struct binding_ctx_out *b_ctx_out)
>  {
>      if (our_chassis) {
> -        sset_add(b_ctx_out->local_lports, pb->logical_port);
> +        add_lport_to_local_lports(b_ctx_out->local_lports, pb->logical_port);
>          add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key,
>                             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->local_lport_ids, pb);
> +        update_local_lport_ids(b_ctx_out->local_lport_ids, pb,
> +                               b_ctx_out->tracked_dp_bindings);
>          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;
> @@ -1084,14 +1219,15 @@ 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. */
> -    sset_add(b_ctx_out->local_lports, pb->logical_port);
> +    add_lport_to_local_lports(b_ctx_out->local_lports, pb->logical_port);
>      if (qos_map && b_ctx_in->ovs_idl_txn) {
>          get_qos_params(pb, qos_map);
>      }
>  
> -    update_local_lport_ids(b_ctx_out->local_lport_ids, pb);
> +    update_local_lport_ids(b_ctx_out->local_lport_ids, pb,
> +                           b_ctx_out->tracked_dp_bindings);
>  }
>  
>  static bool
> @@ -1119,7 +1255,10 @@ 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->local_lport_ids, pb,
> +                               b_ctx_out->tracked_dp_bindings);
>      }
>  
>      return consider_nonvif_lport_(pb, our_chassis, false, b_ctx_in, b_ctx_out);
> @@ -1185,7 +1324,7 @@ build_local_bindings(struct binding_ctx_in *b_ctx_in,
>                      ovs_assert(lbinding->type == BT_VIF);
>                  }
>  
> -                sset_add(b_ctx_out->local_lports, iface_id);
> +                add_lport_to_local_lports(b_ctx_out->local_lports, iface_id);
>                  smap_replace(b_ctx_out->local_iface_ids, iface_rec->name,
>                               iface_id);
>              }
> @@ -1241,7 +1380,7 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out)
>          case LP_PATCH:
>          case LP_LOCALPORT:
>          case LP_VTEP:
> -            update_local_lport_ids(b_ctx_out->local_lport_ids, pb);
> +            update_local_lport_ids(b_ctx_out->local_lport_ids, pb, NULL);
>              break;
>  
>          case LP_VIF:
> @@ -1409,7 +1548,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;
>      }
>  
> @@ -1471,7 +1611,8 @@ remove_pb_from_local_datapath(const struct sbrec_port_binding *pb,
>                                struct binding_ctx_out *b_ctx_out,
>                                struct local_datapath *ld)
>  {
> -    remove_local_lport_ids(pb, b_ctx_out->local_lport_ids);
> +    remove_local_lport_ids(pb, b_ctx_out->local_lport_ids,
> +                           b_ctx_out->tracked_dp_bindings);
>      if (!strcmp(pb->type, "patch") ||
>          !strcmp(pb->type, "l3gateway")) {
>          remove_local_datapath_peer_port(pb, ld, b_ctx_out->local_datapaths);
> @@ -1489,6 +1630,18 @@ remove_pb_from_local_datapath(const struct sbrec_port_binding *pb,
>      }
>  }
>  
> +static void
> +update_lport_tracking(const struct sbrec_port_binding *pb,
> +                      bool old_claim, bool new_claim,
> +                      struct hmap *tracked_dp_bindings)
> +{
> +    if (!tracked_dp_bindings || !pb || (old_claim == new_claim)) {

I think pb can never be NULL here, right?

> +        return;
> +    }
> +
> +    tracked_binding_datapath_lport_add(pb, old_claim, 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'.
> @@ -1501,9 +1654,13 @@ consider_iface_claim(const struct ovsrec_interface *iface_rec,
>                       const char *iface_id,
>                       struct binding_ctx_in *b_ctx_in,
>                       struct binding_ctx_out *b_ctx_out,
> -                     struct hmap *qos_map)
> +                     struct hmap *qos_map,
> +                     bool *local_lports_changed)
>  {
> -    sset_add(b_ctx_out->local_lports, iface_id);
> +    if (add_lport_to_local_lports(b_ctx_out->local_lports, iface_id)) {
> +        *local_lports_changed = true;
> +    }
> +
>      smap_replace(b_ctx_out->local_iface_ids, iface_rec->name, iface_id);
>  
>      struct local_binding *lbinding =
> @@ -1566,7 +1723,8 @@ static bool
>  consider_iface_release(const struct ovsrec_interface *iface_rec,
>                         const char *iface_id,
>                         struct binding_ctx_in *b_ctx_in,
> -                       struct binding_ctx_out *b_ctx_out, bool *changed)
> +                       struct binding_ctx_out *b_ctx_out, bool *changed,
> +                       bool *local_lports_changed)
>  {
>      struct local_binding *lbinding;
>      lbinding = local_binding_find(b_ctx_out->local_bindings,
> @@ -1585,7 +1743,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;
>          }
>  
> @@ -1598,7 +1757,9 @@ consider_iface_release(const struct ovsrec_interface *iface_rec,
>          local_binding_delete(b_ctx_out->local_bindings, lbinding);
>      }
>  
> -    sset_find_and_delete(b_ctx_out->local_lports, iface_id);
> +    if (remove_lport_from_local_lports(b_ctx_out->local_lports, iface_id)) {
> +        *local_lports_changed = true;
> +    }
>      smap_remove(b_ctx_out->local_iface_ids, iface_rec->name);
>  
>      return true;
> @@ -1630,6 +1791,8 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in,
>      bool handled = true;
>      *changed = false;
>  
> +    *b_ctx_out->local_lports_changed = false;
> +

Is this needed? This is part of the data we clear at every run in
en_runtime_data_clear_tracked_data().

>      /* Run the tracked interfaces loop twice. One to handle deleted
>       * changes. And another to handle add/update changes.
>       * This will ensure correctness.
> @@ -1685,7 +1848,8 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in,
>  
>          if (cleared_iface_id) {
>              handled = consider_iface_release(iface_rec, cleared_iface_id,
> -                                             b_ctx_in, b_ctx_out, changed);
> +                                             b_ctx_in, b_ctx_out, changed,
> +                                             b_ctx_out->local_lports_changed);
>          }
>  
>          if (!handled) {
> @@ -1727,7 +1891,8 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in,
>          if (iface_id && ofport > 0) {
>              *changed = true;
>              handled = consider_iface_claim(iface_rec, iface_id, b_ctx_in,
> -                                           b_ctx_out, qos_map_ptr);
> +                                           b_ctx_out, qos_map_ptr,
> +                                           b_ctx_out->local_lports_changed);
>              if (!handled) {
>                  break;
>              }
> @@ -1792,8 +1957,7 @@ static bool
>  handle_deleted_vif_lport(const struct sbrec_port_binding *pb,
>                           enum en_lport_type lport_type,
>                           struct binding_ctx_in *b_ctx_in,
> -                         struct binding_ctx_out *b_ctx_out,
> -                         bool *changed)
> +                         struct binding_ctx_out *b_ctx_out)
>  {
>      struct local_binding *lbinding =
>          get_lbinding_for_lport(pb, lport_type, b_ctx_out);
> @@ -1804,13 +1968,12 @@ 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;
>          }
> -
> -        *changed = true;
>      }
>  
>      handle_deleted_lport(pb, b_ctx_in, b_ctx_out);
> @@ -1822,8 +1985,7 @@ handle_updated_vif_lport(const struct sbrec_port_binding *pb,
>                           enum en_lport_type lport_type,
>                           struct binding_ctx_in *b_ctx_in,
>                           struct binding_ctx_out *b_ctx_out,
> -                         struct hmap *qos_map,
> -                         bool *changed)
> +                         struct hmap *qos_map)
>  {
>      bool claimed = (pb->chassis == b_ctx_in->chassis_rec);
>      bool handled = true;
> @@ -1841,14 +2003,10 @@ handle_updated_vif_lport(const struct sbrec_port_binding *pb,
>      }
>  
>      bool now_claimed = (pb->chassis == b_ctx_in->chassis_rec);
> -    bool changed_ = (claimed != now_claimed);
> -
> -    if (changed_) {
> -        *changed = true;
> -    }
> +    bool changed = (claimed != now_claimed);
>  
>      if (lport_type == LP_VIRTUAL ||
> -        (lport_type == LP_VIF && is_lport_container(pb)) || !changed_) {
> +        (lport_type == LP_VIF && is_lport_container(pb)) || !changed) {
>          return true;
>      }
>  
> @@ -1898,7 +2056,7 @@ binding_handle_port_binding_changes(struct binding_ctx_in *b_ctx_in,
>          enum en_lport_type lport_type = get_lport_type(pb);
>          if (lport_type == LP_VIF || lport_type == LP_VIRTUAL) {
>              handled = handle_deleted_vif_lport(pb, lport_type, b_ctx_in,
> -                                                b_ctx_out, changed);
> +                                                b_ctx_out);
>          } else {
>              handle_deleted_lport(pb, b_ctx_in, b_ctx_out);
>          }
> @@ -1933,15 +2091,14 @@ binding_handle_port_binding_changes(struct binding_ctx_in *b_ctx_in,
>          case LP_VIF:
>          case LP_VIRTUAL:
>              handled = handle_updated_vif_lport(pb, lport_type, b_ctx_in,
> -                                               b_ctx_out, qos_map_ptr,
> -                                               changed);
> +                                               b_ctx_out, qos_map_ptr);
>              break;
>  
>          case LP_PATCH:
>          case LP_LOCALPORT:
>          case LP_VTEP:
> -            *changed = true;
> -            update_local_lport_ids(b_ctx_out->local_lport_ids, pb);
> +            update_local_lport_ids(b_ctx_out->local_lport_ids, pb,
> +                                   b_ctx_out->tracked_dp_bindings);
>              if (lport_type ==  LP_PATCH) {
>                  /* Add the peer datapath to the local datapaths if it's
>                   * not present yet.
> @@ -1950,30 +2107,32 @@ 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. So set *changed to true
> +                 * for any changes to vtep lports. */
> +                *changed = true;
> +            }
>              break;
>  
>          case LP_L2GATEWAY:
> -            *changed = true;
>              handled = consider_l2gw_lport(pb, b_ctx_in, b_ctx_out);
>              break;
>  
>          case LP_L3GATEWAY:
> -            *changed = true;
>              handled = consider_l3gw_lport(pb, b_ctx_in, b_ctx_out);
>              break;
>  
>          case LP_CHASSISREDIRECT:
> -            *changed = true;
>              handled = consider_cr_lport(pb, b_ctx_in, b_ctx_out);
>              break;
>  
>          case LP_EXTERNAL:
> -            *changed = true;
>              handled = consider_external_lport(pb, b_ctx_in, b_ctx_out);
>              break;
>  
>          case LP_LOCALNET: {
> -            *changed = true;
>              consider_localnet_lport(pb, b_ctx_in, b_ctx_out, qos_map_ptr);
>  
>              struct shash bridge_mappings =
> @@ -2008,6 +2167,10 @@ binding_handle_port_binding_changes(struct binding_ctx_in *b_ctx_in,
>          }
>      }
>  
> +    if (handled && !hmap_is_empty(b_ctx_out->tracked_dp_bindings)) {
> +        *changed = true;
> +    }
> +
>      destroy_qos_map(&qos_map);
>      return handled;
>  }
> diff --git a/controller/binding.h b/controller/binding.h
> index 21118ecd4..2974ebb30 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;
> @@ -54,10 +57,28 @@ struct binding_ctx_in {
>  struct binding_ctx_out {
>      struct hmap *local_datapaths;
>      struct shash *local_bindings;
> +
>      struct sset *local_lports;
> +
> +    /* If the sset local_lports is modified, this is set to true by
> +     * the callee. */
> +    bool *local_lports_changed;
> +
> +    /* sset of local lport ids in the format
> +     * <datapath-tunnel-key>_<port-tunnel-key>. */

Nit: shouldn't this comment be part of patch 1?

>      struct sset *local_lport_ids;
> +
>      struct sset *egress_ifaces;
> +
> +    /* smap of OVS interface name as key and
> +     * OVS interface external_ids:iface-id as value. */

Nit: shouldn't this comment be part of patch 2?

>      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 {
> @@ -82,6 +103,21 @@ 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;
> +    struct ovs_list list_node;

Nit: for consistency with tracked_binding_datapath and because list_node
is usually accessed before pb I'd define list_node before pb.

> +    bool deleted;
> +};
> +
> +/* Represent a tracked binding datapath. */
> +struct tracked_binding_datapath {
> +    struct hmap_node node;
> +    const struct sbrec_datapath_binding *dp;
> +    bool is_new;

I think it would be easier to read the code if both
tracked_binding_lport and tracked_binding_datapath would use the same
logic for tracking updates: either both should have "deleted" or both
should have "is_new".

Also, given that we never use tracked_binding_lport.deleted, maybe we
can skip it completely. What do you think?

> +    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,
> @@ -96,4 +132,5 @@ bool binding_handle_ovs_interface_changes(struct binding_ctx_in *,
>  bool binding_handle_port_binding_changes(struct binding_ctx_in *,
>                                           struct binding_ctx_out *,
>                                           bool *changed);
> +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 795a1c297..98652866c 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -973,10 +973,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;

I wonder if the "tracked" field should actually be part of the generic
engine_node_input. We use it for port groups and address sets too
("change_tracked" in ed_type_addr_sets and ed_type_port_groups) and
afaiu the semantics are always:
- "tracked" is set to true if a node's input handler was called
- "tracked" is reset to false if a node's run handler was called

So all this could be implemented generically in inc-proc-eng.c.

What do you think?

> +    bool local_lports_changed;

Do we really need local_lports_changed? Isn't it actually equivalent to
!hmap_is_empty(&

> +    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').    |

This is not really accurate, right? Afaiu we only track if a port
binding is added/deleted.

> + * |                      | 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);

Nit: just as we did for local_bindings_init()/local_bindings_destroy()
I'd move the "hmap_init(&data->tracked_dp_bindings)" inside a new
function called binding_tracked_dp_init().

> +    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)
> @@ -990,6 +1068,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;
>  }
>  
> @@ -1012,6 +1094,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);
>  }
>  
>  static void
> @@ -1092,6 +1176,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
> @@ -1103,6 +1189,13 @@ 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 tracked data. 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 full recompute of runtime engine. */
> +    en_runtime_data_clear_tracked_data(data);
> +

Could you please elaborate more on this? Why is it an issue if we keep
the "tracked" data until the end of the run?

Thanks,
Dumitru

>      static bool first_run = true;
>      if (first_run) {
>          /* don't cleanup since there is no data yet */
> @@ -1158,6 +1251,9 @@ 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;
> +    b_ctx_out.local_lports_changed = &rt_data->local_lports_changed;
>  
>      bool changed = false;
>      if (!binding_handle_ovs_interface_changes(&b_ctx_in, &b_ctx_out,
> @@ -1190,6 +1286,9 @@ 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;
> +
>      bool changed = false;
>      if (!binding_handle_port_binding_changes(&b_ctx_in, &b_ctx_out,
>                                               &changed)) {
> @@ -1797,6 +1896,39 @@ flow_output_port_groups_handler(struct engine_node *node, void *data)
>      return _flow_output_resource_ref_handler(node, data, REF_TYPE_PORTGROUP);
>  }
>  
> +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;
> +}
> +
> +static bool
> +flow_output_noop_handler(struct engine_node *node OVS_UNUSED,
> +                         void *data OVS_UNUSED)
> +{
> +    return true;
> +}
> +
>  /* Engine node en_physical_flow_changes indicates whether
>   * there is a need to
>   *   - recompute only physical flows or
> @@ -1908,13 +2040,6 @@ flow_output_physical_flow_changes_handler(struct engine_node *node, void *data)
>      return true;
>  }
>  
> -static bool
> -flow_output_noop_handler(struct engine_node *node OVS_UNUSED,
> -                         void *data OVS_UNUSED)
> -{
> -    return true;
> -}
> -
>  struct ovn_controller_exit_args {
>      bool *exiting;
>      bool *restart;
> @@ -2036,7 +2161,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,
> @@ -2073,7 +2198,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(
> 



More information about the dev mailing list