[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 22:45:36 UTC 2020


On 6/9/20 3:53 PM, Numan Siddique wrote:
> 
> 
> On Tue, Jun 9, 2020 at 6:33 PM Dumitru Ceara <dceara at redhat.com
> <mailto:dceara at redhat.com>> wrote:
> 
>     On 6/9/20 1:00 PM, Numan Siddique wrote:
>     >
>     >
>     > On Tue, Jun 9, 2020 at 3:11 PM Dumitru Ceara <dceara at redhat.com
>     <mailto:dceara at redhat.com>
>     > <mailto:dceara at redhat.com <mailto:dceara at redhat.com>>> wrote:
>     >
>     >     On 6/8/20 3:50 PM, numans at ovn.org <mailto:numans at ovn.org>
>     <mailto:numans at ovn.org <mailto:numans at ovn.org>> wrote:
>     >     > From: Numan Siddique <numans at ovn.org <mailto:numans at ovn.org>
>     <mailto:numans at ovn.org <mailto: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
>     <mailto:anilvenkata at redhat.com>
>     >     <mailto:anilvenkata at redhat.com <mailto:anilvenkata at redhat.com>>>
>     >     > Signed-off-by: Venkata Anil <anilvenkata at redhat.com
>     <mailto:anilvenkata at redhat.com>
>     >     <mailto:anilvenkata at redhat.com <mailto:anilvenkata at redhat.com>>>
>     >     > Signed-off-by: Numan Siddique <numans at ovn.org
>     <mailto:numans at ovn.org> <mailto:numans at ovn.org <mailto:numans at ovn.org>>>
>     >     > ---
>     >     >  controller/binding.c        | 299
>     >     ++++++++++++++++++++++++++++--------
>     >     >  controller/binding.h        |  37 +++++
>     >     >  controller/ovn-controller.c | 144 +++++++++++++++--
>     >     >  tests/ovn-performance.at <http://ovn-performance.at>
>     <http://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,
>     >
>     >
>     > Hi Dumitru,
>     >
>     > Thanks for the review.
>     >
>     > Please see below the replies.
>     >
>     > Thanks
>     > Numan
>     >
>     >  
>     >
>     >     I guess this should be:
>     >
>     >     bool added = !!sset_add(local_lport_ids, buf);
>     >
>     >
>     > Ack.
>     >
>     >  
>     >
>     >     > +    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).
>     >
>     >
>     > Ack.
>     >  
>     >
>     >
>     >     >   */
>     >     >  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?
>     >
>     >
>     > Yeah. I'll remove the check.
>     >  
>     >
>     >
>     >     > +        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().
>     >
>     >
>     > I still feel it's better to set it to false here. This function
>     doesn't
>     > know anything about
>     > the tracked data.
>     >
>     >  
> 
>     Ok, then the reverse question, do we still need to clear
>     local_lports_changed in en_runtime_data_clear_tracked_data()?
> 
> 
> Why not ? Do you see any issue with it ?
> 

I was just confused why we need to reset it to false if it's already set
to false every time engine_init_run() is called. But I guess that it
might be that in the future binding_handle_ovs_interface_changes() could
be called in other parts of the code too (although it seems very
incremental engine specific) and then to be extra safe we can make sure
local_lports_changed is false.

>  
> 
> 
>     >
>     >
>     >     >      /* 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?
>     >
>     >
>     > Could be.
>     >  
>     >
>     >
>     >     >      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?
>     >
>     >
>     > Could be. 
>     >
>     >
>     >     >      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.
>     >
>     >
>     > Actually we don't need list_node now. It was a leftover from the
>     > previous versions.
>     > I'll delete it. Thanks.
>     >  
>     >
>     >
>     >     > +    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".
>     >
>     >
>     > Ack.
>     >  
>     >
>     >
>     >     Also, given that we never use tracked_binding_lport.deleted,
>     maybe we
>     >     can skip it completely. What do you think?
>     >
>     >
>     > I'll delete the "deleted". We can add it later if it is really
>     required.
>     >  
>     >
>     >
>     >     > +    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?
>     >
>     >
>     > Could be. But I'd prefer to be a separate patch (not part of this
>     series).
>     >  
> 
>     Ok, but please see my comment regarding
>     en_runtime_data_clear_tracked_data() in en_runtime_data_run().
> 
>     >
>     >
>     >     > +    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;
>     >     >  };
>     >
>     >
>     > Not really.
>     >
>     > Let's say you run the command - "ovs-vsctl add-port br-int p0 -- set
>     > interface p0 externa_ids:iface-id=lport1"
>     >
>     > And if logical lport lport1 doesn't exist (or not seen by
>     ovn-controller
>     > due to conditional monitoring), we
>     > need to update the runtime_data engine node so that
>     "update_sb_monitor"
>     > is called.
>     >
>     >  
> 
>     Ah, right, thanks for the explanation.
> 
>     >
>     >     > 
>     >     > +/* 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.
>     >
>     >
>     > Not all the time. We can bind an "external" port when the port
>     binding gets
>     > updated due to the command - "ovn-nbctl lsp-set-type <lport> external"
>     > or for the command - "ovn-nbctl lsp-set-options router-port=<peer
>     port>
>     >
>     >
> 
>     OK.
> 
>     >  
>     >
>     >
>     >     > + * |                      | 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().
>     >
>     >
>     > Not sure if we want to have the same functions here just to init
>     the hmap.
>     >
>     >  
> 
>     Well, local_bindings_init(), just does shash_init() so I don't really
>     see why not :)
> 
> 
> I don't see any advantage, but rather an unnecessary function overhead. 
> 

It was just to group create/delete implementations together but it's not
a big deal I guess.

> So, I'd live it as is.
> 

Ok.

Thanks,
Dumitru

> 
> 
>     >
>     >     > +    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?
>     >
>     >
>     >
>     > When a runtime data node gets updated (engine_set_node_state()) either
>     > due to
>     > full recompute of runtime or due to handler setting it,
>     > flow_output_runtime_data_handler()
>     > will not trigger flow_output recompute unless it see
>     runtime->tracked =
>     > false.
>     >
>     > Hence it clears the tracked data. Instead of calling
>     > en_runtime_data_clear_tracked_data(),
>     > en_runtime_data_run() can set just 'tracked' to false, but I think
>     it's
>     > better to clear
>     > the whole tracked data.
>     >
> 
>     So this brings us back to my original comment regarding "tracked". If
>     tracked would be part of the I-P engine internal input node fields, it
>     would get set before running en_runtime_data_run() and we wouldn't have
>     to explain why we need to clear the "tracked" field here.
> 
> 
> We may still have to clear the tracked data if some of the handlers
> added some
> data to it before one of the handlers gives up and we fall back to fully
> recompute
> given that we don't clear the tracked data at the end of the engine.
> 
> Also I've not thought about it and not sure if we really want to move to
> engine.
> 
> It could be a good idea. But I'll leave it to others or you, if you want
> to work on this
> as a follow up patch.
> 
> 
> 
>     If we decide to address the "tracked" fields as a follow up patch I
>     think it'd be good to add some TODO/FIXME/XXX comments so we don't
>     forget about addressing this later. E.g., there's already "
>     /* XXX: The change_tracked check may be added to inc-proc framework. */"
>     for address sets.
> 
>  
> 
> I'll add a comment that it could be moved to the engine.
> 
> Thanks
> Numan
> 
> 
>     Thanks,
>     Dumitru
> 
>     >
>     >     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
>     <http://ovn-performance.at> <http://ovn-performance.at>
>     >     b/tests/ovn-performance.at <http://ovn-performance.at>
>     <http://ovn-performance.at>
>     >     > index 5cc1960b6..08acd3bae 100644
>     >     > --- a/tests/ovn-performance.at <http://ovn-performance.at>
>     <http://ovn-performance.at>
>     >     > +++ b/tests/ovn-performance.at <http://ovn-performance.at>
>     <http://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 <mailto:dev at openvswitch.org>
>     <mailto:dev at openvswitch.org <mailto:dev at openvswitch.org>>
>     >     https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>     >
> 
>     _______________________________________________
>     dev mailing list
>     dev at openvswitch.org <mailto:dev at openvswitch.org>
>     https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 



More information about the dev mailing list