[ovs-dev] [PATCH ovn v3 1/5] controller: Move 'struct local_datapath' to a separate file.

Numan Siddique numans at ovn.org
Mon Aug 2 18:42:35 UTC 2021


On Mon, Aug 2, 2021 at 2:03 PM Han Zhou <zhouhan at gmail.com> wrote:
>
> On Mon, Aug 2, 2021 at 8:03 AM Numan Siddique <numans at ovn.org> wrote:
> >
> >
> >
> >
> > On Mon, Aug 2, 2021, 1:54 AM Han Zhou <zhouhan at gmail.com> wrote:
> >>
> >> On Tue, Jul 27, 2021 at 7:47 PM <numans at ovn.org> wrote:
> >> >
> >> > From: Numan Siddique <numans at ovn.org>
> >> >
> >> > This would uncomplicate the binding.c code a bit.  The tracking
> >> > data and the related functions are also moved to the file -
> local_data.c.
> >> > This would help in an upcoming patch.
> >> >
> >> > Signed-off-by: Numan Siddique <numans at ovn.org>
> >> > ---
> >> >  controller/automake.mk      |   4 +-
> >> >  controller/binding.c        | 365 +++++++----------------------------
> >> >  controller/binding.h        |  13 --
> >> >  controller/lflow.c          |   1 +
> >> >  controller/local_data.c     | 369 ++++++++++++++++++++++++++++++++++++
> >> >  controller/local_data.h     | 119 ++++++++++++
> >> >  controller/lport.c          |  40 ++++
> >> >  controller/lport.h          |   7 +-
> >> >  controller/ovn-controller.c |  56 ++----
> >> >  controller/ovn-controller.h |  34 ----
> >> >  controller/patch.c          |   1 +
> >> >  controller/physical.c       |   1 +
> >> >  controller/pinctrl.c        |   1 +
> >> >  13 files changed, 623 insertions(+), 388 deletions(-)
> >> >  create mode 100644 controller/local_data.c
> >> >  create mode 100644 controller/local_data.h
> >> >
> >> > diff --git a/controller/automake.mk b/controller/automake.mk
> >> > index 2f6c50890..41f907d6e 100644
> >> > --- a/controller/automake.mk
> >> > +++ b/controller/automake.mk
> >> > @@ -33,7 +33,9 @@ controller_ovn_controller_SOURCES = \
> >> >         controller/physical.c \
> >> >         controller/physical.h \
> >> >         controller/mac-learn.c \
> >> > -       controller/mac-learn.h
> >> > +       controller/mac-learn.h \
> >> > +       controller/local_data.c \
> >> > +       controller/local_data.h
> >> >
> >> >  controller_ovn_controller_LDADD = lib/libovn.la $(OVS_LIBDIR)/
> >> libopenvswitch.la
> >> >  man_MANS += controller/ovn-controller.8
> >> > diff --git a/controller/binding.c b/controller/binding.c
> >> > index d50f3affa..bc47b8c0e 100644
> >> > --- a/controller/binding.c
> >> > +++ b/controller/binding.c
> >> > @@ -14,13 +14,8 @@
> >> >   */
> >> >
> >> >  #include <config.h>
> >> > -#include "binding.h"
> >> > -#include "ha-chassis.h"
> >> > -#include "if-status.h"
> >> > -#include "lflow.h"
> >> > -#include "lport.h"
> >> > -#include "patch.h"
> >> >
> >> > +/* OVS includes. */
> >> >  #include "lib/bitmap.h"
> >> >  #include "openvswitch/poll-loop.h"
> >> >  #include "lib/sset.h"
> >> > @@ -29,9 +24,18 @@
> >> >  #include "lib/vswitch-idl.h"
> >> >  #include "openvswitch/hmap.h"
> >> >  #include "openvswitch/vlog.h"
> >> > +
> >> > +/* OVN includes. */
> >> > +#include "binding.h"
> >> > +#include "ha-chassis.h"
> >> > +#include "if-status.h"
> >> > +#include "lflow.h"
> >> >  #include "lib/chassis-index.h"
> >> >  #include "lib/ovn-sb-idl.h"
> >> > +#include "local_data.h"
> >> > +#include "lport.h"
> >> >  #include "ovn-controller.h"
> >> > +#include "patch.h"
> >> >
> >> >  VLOG_DEFINE_THIS_MODULE(binding);
> >> >
> >> > @@ -76,106 +80,27 @@ binding_register_ovs_idl(struct ovsdb_idl
> *ovs_idl)
> >> >      ovsdb_idl_add_column(ovs_idl, &ovsrec_qos_col_type);
> >> >  }
> >> >
> >> > -static struct tracked_binding_datapath
> *tracked_binding_datapath_create(
> >> > -    const struct sbrec_datapath_binding *,
> >> > -    bool is_new, struct hmap *tracked_dps);
> >> > -static struct tracked_binding_datapath *tracked_binding_datapath_find(
> >> > -    struct hmap *, const struct sbrec_datapath_binding *);
> >> > -static void tracked_binding_datapath_lport_add(
> >> > -    const struct sbrec_port_binding *, struct hmap
> *tracked_datapaths);
> >> >  static void update_lport_tracking(const struct sbrec_port_binding *pb,
> >> > -                                  struct hmap *tracked_dp_bindings);
> >> > -
> >> > -static void
> >> > -add_local_datapath__(struct ovsdb_idl_index
> >> *sbrec_datapath_binding_by_key,
> >> > -                     struct ovsdb_idl_index
> >> *sbrec_port_binding_by_datapath,
> >> > -                     struct ovsdb_idl_index
> *sbrec_port_binding_by_name,
> >> > -                     const struct sbrec_datapath_binding *datapath,
> >> > -                     bool has_local_l3gateway, int depth,
> >> > -                     struct hmap *local_datapaths,
> >> > -                     struct hmap *tracked_datapaths)
> >> > -{
> >> > -    uint32_t dp_key = datapath->tunnel_key;
> >> > -    struct local_datapath *ld = get_local_datapath(local_datapaths,
> >> dp_key);
> >> > -    if (ld) {
> >> > -        if (has_local_l3gateway) {
> >> > -            ld->has_local_l3gateway = true;
> >> > -        }
> >> > -        return;
> >> > -    }
> >> > +                                  struct hmap *tracked_dp_bindings,
> >> > +                                  bool claimed);
> >> >
> >> > -    ld = xzalloc(sizeof *ld);
> >> > -    hmap_insert(local_datapaths, &ld->hmap_node, dp_key);
> >> > -    ld->datapath = datapath;
> >> > -    ld->localnet_port = NULL;
> >> > -    shash_init(&ld->external_ports);
> >> > -    ld->has_local_l3gateway = has_local_l3gateway;
> >> > -
> >> > -    if (tracked_datapaths) {
> >> > -        struct tracked_binding_datapath *tdp =
> >> > -            tracked_binding_datapath_find(tracked_datapaths,
> datapath);
> >> > -        if (!tdp) {
> >> > -            tracked_binding_datapath_create(datapath, true,
> >> tracked_datapaths);
> >> > -        } else {
> >> > -            /* Its possible that there is already an entry in tracked
> >> datapaths
> >> > -             * for this 'datapath'.
> tracked_binding_datapath_lport_add()
> >> may
> >> > -             * have created it. Since the 'datapath' is added to the
> >> > -             * local datapaths, set 'tdp->is_new' to true so that the
> >> flows
> >> > -             * for this datapath are programmed properly.
> >> > -             * */
> >> > -            tdp->is_new = true;
> >> > -        }
> >> > -    }
> >> > +struct local_datpath_added_aux {
> >> > +    bool has_local_l3gateway;
> >> > +    struct hmap *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");
> >> > -        return;
> >> > +/* This function is called by local_datapath_add() if a new
> >> local_datapath
> >> > + * is created. */
> >> > +static void
> >> > +local_datapath_added(struct local_datapath *ld, void *aux)
> >> > +{
> >> > +    struct local_datpath_added_aux *aux_ = aux;
> >> > +    if (aux_->tracked_datapaths) {
> >> > +        tracked_datapath_add(ld->datapath, TRACKED_RESOURCE_NEW,
> >> > +                             aux_->tracked_datapaths);
> >> >      }
> >> >
> >> > -    struct sbrec_port_binding *target =
> >> > -
> >>  sbrec_port_binding_index_init_row(sbrec_port_binding_by_datapath);
> >> > -    sbrec_port_binding_index_set_datapath(target, datapath);
> >> > -
> >> > -    const struct sbrec_port_binding *pb;
> >> > -    SBREC_PORT_BINDING_FOR_EACH_EQUAL (pb, target,
> >> > -
> sbrec_port_binding_by_datapath) {
> >> > -        if (!strcmp(pb->type, "patch") || !strcmp(pb->type,
> >> "l3gateway")) {
> >> > -            const char *peer_name = smap_get(&pb->options, "peer");
> >> > -            if (peer_name) {
> >> > -                const struct sbrec_port_binding *peer;
> >> > -
> >> > -                peer =
> lport_lookup_by_name(sbrec_port_binding_by_name,
> >> > -                                            peer_name);
> >> > -
> >> > -                if (peer && peer->datapath) {
> >> > -                    if (!strcmp(pb->type, "patch")) {
> >> > -                        /* Add the datapath to local datapath only for
> >> patch
> >> > -                         * ports. For l3gateway ports, since gateway
> >> router
> >> > -                         * resides on one chassis, we don't need to
> add.
> >> > -                         * Otherwise, all other chassis might create
> >> patch
> >> > -                         * ports between br-int and the provider
> bridge.
> >> */
> >> > -
> >>  add_local_datapath__(sbrec_datapath_binding_by_key,
> >> > -
> >> sbrec_port_binding_by_datapath,
> >> > -
> sbrec_port_binding_by_name,
> >> > -                                             peer->datapath, false,
> >> > -                                             depth + 1,
> local_datapaths,
> >> > -                                             tracked_datapaths);
> >> > -                    }
> >> > -                    ld->n_peer_ports++;
> >> > -                    if (ld->n_peer_ports >
> ld->n_allocated_peer_ports) {
> >> > -                        ld->peer_ports =
> >> > -                            x2nrealloc(ld->peer_ports,
> >> > -                                       &ld->n_allocated_peer_ports,
> >> > -                                       sizeof *ld->peer_ports);
> >> > -                    }
> >> > -                    ld->peer_ports[ld->n_peer_ports - 1].local = pb;
> >> > -                    ld->peer_ports[ld->n_peer_ports - 1].remote =
> peer;
> >> > -                }
> >> > -            }
> >> > -        }
> >> > -    }
> >> > -    sbrec_port_binding_index_destroy_row(target);
> >> > +    ld->has_local_l3gateway = aux_->has_local_l3gateway;
> >> >  }
> >> >
> >> >  static void
> >> > @@ -186,11 +111,19 @@ add_local_datapath(struct ovsdb_idl_index
> >> *sbrec_datapath_binding_by_key,
> >> >                     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,
> >> > -                         tracked_datapaths);
> >> > +    struct local_datpath_added_aux aux = {
> >> > +        .has_local_l3gateway = has_local_l3gateway,
> >> > +        .tracked_datapaths = tracked_datapaths,
> >> > +    };
> >> > +
> >> > +    struct local_datapath *ld =
> >> > +        local_datapath_add(local_datapaths, datapath,
> >> > +                           sbrec_datapath_binding_by_key,
> >> > +                           sbrec_port_binding_by_datapath,
> >> > +                           sbrec_port_binding_by_name,
> >> > +                           local_datapath_added,
> >> > +                           &aux);
> >> > +    ld->has_local_l3gateway = has_local_l3gateway;
> >
> >
> > Hi Han,
> >
> > "has_local_l3gateway" is updated here after every call to
> "local_datapath_add()".  This ensures that
> > this patch doesn't change the functionality when compared to the present
> code.
> >
> >
> >> >  }
> >> >
> >> >  static void
> >> > @@ -559,7 +492,8 @@ update_related_lport(const struct
> sbrec_port_binding
> >> *pb,
> >> >
> >> >          if (b_ctx->tracked_dp_bindings) {
> >> >              /* Add the 'pb' to the tracked_datapaths. */
> >> > -            tracked_binding_datapath_lport_add(pb,
> >> b_ctx->tracked_dp_bindings);
> >> > +            tracked_datapath_lport_add(pb, TRACKED_RESOURCE_NEW,
> >> > +                                       b_ctx->tracked_dp_bindings);
> >> >          }
> >> >      }
> >> >      sset_add(&b_ctx->related_lports->lport_names, pb->logical_port);
> >> > @@ -582,7 +516,8 @@ remove_related_lport(const struct
> sbrec_port_binding
> >> *pb,
> >> >
> >> >          if (b_ctx->tracked_dp_bindings) {
> >> >              /* Add the 'pb' to the tracked_datapaths. */
> >> > -            tracked_binding_datapath_lport_add(pb,
> >> b_ctx->tracked_dp_bindings);
> >> > +            tracked_datapath_lport_add(pb, TRACKED_RESOURCE_REMOVED,
> >> > +                                       b_ctx->tracked_dp_bindings);
> >> >          }
> >> >      }
> >> >  }
> >> > @@ -927,74 +862,6 @@ is_lport_vif(const struct sbrec_port_binding *pb)
> >> >      return !pb->type[0];
> >> >  }
> >> >
> >> > -static struct tracked_binding_datapath *
> >> > -tracked_binding_datapath_create(const struct sbrec_datapath_binding
> *dp,
> >> > -                                bool is_new,
> >> > -                                struct hmap *tracked_datapaths)
> >> > -{
> >> > -    struct tracked_binding_datapath *t_dp = xzalloc(sizeof *t_dp);
> >> > -    t_dp->dp = dp;
> >> > -    t_dp->is_new = is_new;
> >> > -    shash_init(&t_dp->lports);
> >> > -    hmap_insert(tracked_datapaths, &t_dp->node,
> >> uuid_hash(&dp->header_.uuid));
> >> > -    return t_dp;
> >> > -}
> >> > -
> >> > -static struct tracked_binding_datapath *
> >> > -tracked_binding_datapath_find(struct hmap *tracked_datapaths,
> >> > -                              const struct sbrec_datapath_binding *dp)
> >> > -{
> >> > -    struct tracked_binding_datapath *t_dp;
> >> > -    size_t hash = uuid_hash(&dp->header_.uuid);
> >> > -    HMAP_FOR_EACH_WITH_HASH (t_dp, node, hash, tracked_datapaths) {
> >> > -        if (uuid_equals(&t_dp->dp->header_.uuid, &dp->header_.uuid)) {
> >> > -            return t_dp;
> >> > -        }
> >> > -    }
> >> > -
> >> > -    return NULL;
> >> > -}
> >> > -
> >> > -static void
> >> > -tracked_binding_datapath_lport_add(const struct sbrec_port_binding
> *pb,
> >> > -                                   struct hmap *tracked_datapaths)
> >> > -{
> >> > -    if (!tracked_datapaths) {
> >> > -        return;
> >> > -    }
> >> > -
> >> > -    struct tracked_binding_datapath *tracked_dp =
> >> > -        tracked_binding_datapath_find(tracked_datapaths,
> pb->datapath);
> >> > -    if (!tracked_dp) {
> >> > -        tracked_dp = tracked_binding_datapath_create(pb->datapath,
> false,
> >> > -
> tracked_datapaths);
> >> > -    }
> >> > -
> >> > -    /* Check if the lport is already present or not.
> >> > -     * If it is already present, then just update the 'pb' field. */
> >> > -    struct tracked_binding_lport *lport =
> >> > -        shash_find_data(&tracked_dp->lports, pb->logical_port);
> >> > -
> >> > -    if (!lport) {
> >> > -        lport = xmalloc(sizeof *lport);
> >> > -        shash_add(&tracked_dp->lports, pb->logical_port, lport);
> >> > -    }
> >> > -
> >> > -    lport->pb = pb;
> >> > -}
> >> > -
> >> > -void
> >> > -binding_tracked_dp_destroy(struct hmap *tracked_datapaths)
> >> > -{
> >> > -    struct tracked_binding_datapath *t_dp;
> >> > -    HMAP_FOR_EACH_POP (t_dp, node, tracked_datapaths) {
> >> > -        shash_destroy_free_data(&t_dp->lports);
> >> > -        free(t_dp);
> >> > -    }
> >> > -
> >> > -    hmap_destroy(tracked_datapaths);
> >> > -}
> >> > -
> >> >  static enum en_lport_type
> >> >  get_lport_type(const struct sbrec_port_binding *pb)
> >> >  {
> >> > @@ -1129,7 +996,7 @@ claim_lport(const struct sbrec_port_binding *pb,
> >> >          sbrec_port_binding_set_chassis(pb, chassis_rec);
> >> >
> >> >          if (tracked_datapaths) {
> >> > -            update_lport_tracking(pb, tracked_datapaths);
> >> > +            update_lport_tracking(pb, tracked_datapaths, true);
> >> >          }
> >> >      }
> >> >
> >> > @@ -1178,7 +1045,7 @@ release_lport(const struct sbrec_port_binding
> *pb,
> >> bool sb_readonly,
> >> >          sbrec_port_binding_set_virtual_parent(pb, NULL);
> >> >      }
> >> >
> >> > -    update_lport_tracking(pb, tracked_datapaths);
> >> > +    update_lport_tracking(pb, tracked_datapaths, false);
> >> >      if_status_mgr_release_iface(if_mgr, pb->logical_port);
> >> >      VLOG_INFO("Releasing lport %s from this chassis.",
> pb->logical_port);
> >> >      return true;
> >> > @@ -1828,42 +1695,6 @@ binding_cleanup(struct ovsdb_idl_txn
> >> *ovnsb_idl_txn,
> >> >      return !any_changes;
> >> >  }
> >> >
> >> > -static const struct sbrec_port_binding *
> >> > -get_peer_lport__(const struct sbrec_port_binding *pb,
> >> > -                 struct binding_ctx_in *b_ctx_in)
> >> > -{
> >> > -    const char *peer_name = smap_get(&pb->options, "peer");
> >> > -
> >> > -    if (!peer_name) {
> >> > -        return NULL;
> >> > -    }
> >> > -
> >> > -    const struct sbrec_port_binding *peer;
> >> > -    peer = lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name,
> >> > -                                peer_name);
> >> > -    return (peer && peer->datapath) ? peer : NULL;
> >> > -}
> >> > -
> >> > -static const struct sbrec_port_binding *
> >> > -get_l3gw_peer_lport(const struct sbrec_port_binding *pb,
> >> > -                    struct binding_ctx_in *b_ctx_in)
> >> > -{
> >> > -    if (strcmp(pb->type, "l3gateway")) {
> >> > -        return NULL;
> >> > -    }
> >> > -    return get_peer_lport__(pb, b_ctx_in);
> >> > -}
> >> > -
> >> > -static const struct sbrec_port_binding *
> >> > -get_peer_lport(const struct sbrec_port_binding *pb,
> >> > -               struct binding_ctx_in *b_ctx_in)
> >> > -{
> >> > -    if (strcmp(pb->type, "patch")) {
> >> > -        return NULL;
> >> > -    }
> >> > -    return get_peer_lport__(pb, b_ctx_in);
> >> > -}
> >> > -
> >> >  /* This function adds the local datapath of the 'peer' of
> >> >   * lport 'pb' to the local datapaths if it is not yet added.
> >> >   */
> >> > @@ -1873,61 +1704,16 @@ add_local_datapath_peer_port(const struct
> >> sbrec_port_binding *pb,
> >> >                               struct binding_ctx_out *b_ctx_out,
> >> >                               struct local_datapath *ld)
> >> >  {
> >> > -    const struct sbrec_port_binding *peer;
> >> > -    peer = get_peer_lport(pb, b_ctx_in);
> >> > -
> >> > -    if (!peer) {
> >> > -        return;
> >> > -    }
> >> > -
> >> > -    bool present = false;
> >> > -    for (size_t i = 0; i < ld->n_peer_ports; i++) {
> >> > -        if (ld->peer_ports[i].local == pb) {
> >> > -            present = true;
> >> > -            break;
> >> > -        }
> >> > -    }
> >> > -
> >> > -    if (!present) {
> >> > -        ld->n_peer_ports++;
> >> > -        if (ld->n_peer_ports > ld->n_allocated_peer_ports) {
> >> > -            ld->peer_ports =
> >> > -                x2nrealloc(ld->peer_ports,
> >> > -                           &ld->n_allocated_peer_ports,
> >> > -                           sizeof *ld->peer_ports);
> >> > -        }
> >> > -        ld->peer_ports[ld->n_peer_ports - 1].local = pb;
> >> > -        ld->peer_ports[ld->n_peer_ports - 1].remote = peer;
> >> > -    }
> >> > -
> >> > -    struct local_datapath *peer_ld =
> >> > -        get_local_datapath(b_ctx_out->local_datapaths,
> >> > -                           peer->datapath->tunnel_key);
> >> > -    if (!peer_ld) {
> >> > -        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,
> >> > -                             peer->datapath, false,
> >> > -                             1, b_ctx_out->local_datapaths,
> >> > -                             b_ctx_out->tracked_dp_bindings);
> >> > -        return;
> >> > -    }
> >> > -
> >> > -    for (size_t i = 0; i < peer_ld->n_peer_ports; i++) {
> >> > -        if (peer_ld->peer_ports[i].local == peer) {
> >> > -            return;
> >> > -        }
> >> > -    }
> >> > +    struct local_datpath_added_aux aux = {
> >> > +        .has_local_l3gateway = false,
> >> > +        .tracked_datapaths = b_ctx_out->tracked_dp_bindings,
> >> > +    };
> >> >
> >> > -    peer_ld->n_peer_ports++;
> >> > -    if (peer_ld->n_peer_ports > peer_ld->n_allocated_peer_ports) {
> >> > -        peer_ld->peer_ports =
> >> > -            x2nrealloc(peer_ld->peer_ports,
> >> > -                        &peer_ld->n_allocated_peer_ports,
> >> > -                        sizeof *peer_ld->peer_ports);
> >> > -    }
> >> > -    peer_ld->peer_ports[peer_ld->n_peer_ports - 1].local = peer;
> >> > -    peer_ld->peer_ports[peer_ld->n_peer_ports - 1].remote = pb;
> >> > +    local_datapath_add_peer_port(pb,
> >> b_ctx_in->sbrec_datapath_binding_by_key,
> >> > +
> >> b_ctx_in->sbrec_port_binding_by_datapath,
> >> > +                                 b_ctx_in->sbrec_port_binding_by_name,
> >> > +                                 ld, b_ctx_out->local_datapaths,
> >> > +                                 local_datapath_added, &aux);
> >> >  }
> >> >
> >> >  static void
> >> > @@ -1935,34 +1721,7 @@ remove_local_datapath_peer_port(const struct
> >> sbrec_port_binding *pb,
> >> >                                  struct local_datapath *ld,
> >> >                                  struct hmap *local_datapaths)
> >> >  {
> >> > -    size_t i = 0;
> >> > -    for (i = 0; i < ld->n_peer_ports; i++) {
> >> > -        if (ld->peer_ports[i].local == pb) {
> >> > -            break;
> >> > -        }
> >> > -    }
> >> > -
> >> > -    if (i == ld->n_peer_ports) {
> >> > -        return;
> >> > -    }
> >> > -
> >> > -    const struct sbrec_port_binding *peer = ld->peer_ports[i].remote;
> >> > -
> >> > -    /* Possible improvement: We can shrink the allocated peer ports
> >> > -     * if (ld->n_peer_ports < ld->n_allocated_peer_ports / 2).
> >> > -     */
> >> > -    ld->peer_ports[i].local = ld->peer_ports[ld->n_peer_ports -
> 1].local;
> >> > -    ld->peer_ports[i].remote = ld->peer_ports[ld->n_peer_ports -
> >> 1].remote;
> >> > -    ld->n_peer_ports--;
> >> > -
> >> > -    struct local_datapath *peer_ld =
> >> > -        get_local_datapath(local_datapaths,
> peer->datapath->tunnel_key);
> >> > -    if (peer_ld) {
> >> > -        /* Remove the peer port from the peer datapath. The peer
> >> > -         * datapath also tries to remove its peer lport, but that
> would
> >> > -         * be no-op. */
> >> > -        remove_local_datapath_peer_port(peer, peer_ld,
> local_datapaths);
> >> > -    }
> >> > +   local_datapath_remove_peer_port(pb, ld, local_datapaths);
> >> >  }
> >> >
> >> >  static void
> >> > @@ -1995,13 +1754,16 @@ remove_pb_from_local_datapath(const struct
> >> sbrec_port_binding *pb,
> >> >
> >> >  static void
> >> >  update_lport_tracking(const struct sbrec_port_binding *pb,
> >> > -                      struct hmap *tracked_dp_bindings)
> >> > +                      struct hmap *tracked_dp_bindings,
> >> > +                      bool claimed)
> >> >  {
> >> >      if (!tracked_dp_bindings) {
> >> >          return;
> >> >      }
> >> >
> >> > -    tracked_binding_datapath_lport_add(pb, tracked_dp_bindings);
> >> > +    tracked_datapath_lport_add(
> >> > +        pb, claimed ? TRACKED_RESOURCE_NEW : TRACKED_RESOURCE_REMOVED,
> >> > +        tracked_dp_bindings);
> >> >  }
> >> >
> >> >  /* Considers the ovs iface 'iface_rec' for claiming.
> >> > @@ -2315,7 +2077,7 @@ handle_deleted_lport(const struct
> >> sbrec_port_binding *pb,
> >> >      /* If the binding is not local, if 'pb' is a L3 gateway port, we
> >> should
> >> >       * remove its peer, if that one is local.
> >> >       */
> >> > -    pb = get_l3gw_peer_lport(pb, b_ctx_in);
> >> > +    pb = lport_get_l3gw_peer(pb,
> b_ctx_in->sbrec_port_binding_by_name);
> >> >      if (pb) {
> >> >          ld = get_local_datapath(b_ctx_out->local_datapaths,
> >> >                                  pb->datapath->tunnel_key);
> >> > @@ -2598,7 +2360,10 @@ delete_done:
> >> >                       * */
> >> >                      const struct sbrec_port_binding *peer;
> >> >                      struct local_datapath *peer_ld = NULL;
> >> > -                    peer = get_peer_lport(pb, b_ctx_in);
> >> > +                    peer =
> >> > +                        lport_get_peer(pb,
> >> > +
> >> b_ctx_in->sbrec_port_binding_by_name);
> >> > +
> >> >                      if (peer) {
> >> >                          peer_ld =
> >> >
> >>  get_local_datapath(b_ctx_out->local_datapaths,
> >> > diff --git a/controller/binding.h b/controller/binding.h
> >> > index 77197e742..b1717bd2b 100644
> >> > --- a/controller/binding.h
> >> > +++ b/controller/binding.h
> >> > @@ -121,19 +121,6 @@ void local_binding_set_up(struct shash
> >> *local_bindings, const char *pb_name,
> >> >  void local_binding_set_down(struct shash *local_bindings, const char
> >> *pb_name,
> >> >                              bool sb_readonly, bool ovs_readonly);
> >> >
> >> > -/* Represents a tracked binding logical port. */
> >> > -struct tracked_binding_lport {
> >> > -    const struct sbrec_port_binding *pb;
> >> > -};
> >> > -
> >> > -/* Represent a tracked binding datapath. */
> >> > -struct tracked_binding_datapath {
> >> > -    struct hmap_node node;
> >> > -    const struct sbrec_datapath_binding *dp;
> >> > -    bool is_new;
> >> > -    struct shash lports; /* shash of struct tracked_binding_lport. */
> >> > -};
> >> > -
> >> >  void binding_register_ovs_idl(struct ovsdb_idl *);
> >> >  void binding_run(struct binding_ctx_in *, struct binding_ctx_out *);
> >> >  bool binding_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn,
> >> > diff --git a/controller/lflow.c b/controller/lflow.c
> >> > index 67bc20203..2e4582009 100644
> >> > --- a/controller/lflow.c
> >> > +++ b/controller/lflow.c
> >> > @@ -18,6 +18,7 @@
> >> >  #include "coverage.h"
> >> >  #include "ha-chassis.h"
> >> >  #include "lflow-cache.h"
> >> > +#include "local_data.h"
> >> >  #include "lport.h"
> >> >  #include "ofctrl.h"
> >> >  #include "openvswitch/dynamic-string.h"
> >> > diff --git a/controller/local_data.c b/controller/local_data.c
> >> > new file mode 100644
> >> > index 000000000..1dc8c45a1
> >> > --- /dev/null
> >> > +++ b/controller/local_data.c
> >> > @@ -0,0 +1,369 @@
> >> > +/* Copyright (c) 2021, Red Hat, Inc.
> >> > + *
> >> > + * Licensed under the Apache License, Version 2.0 (the "License");
> >> > + * you may not use this file except in compliance with the License.
> >> > + * You may obtain a copy of the License at:
> >> > + *
> >> > + *     http://www.apache.org/licenses/LICENSE-2.0
> >> > + *
> >> > + * Unless required by applicable law or agreed to in writing, software
> >> > + * distributed under the License is distributed on an "AS IS" BASIS,
> >> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> >> implied.
> >> > + * See the License for the specific language governing permissions and
> >> > + * limitations under the License.
> >> > + */
> >> > +
> >> > +#include <config.h>
> >> > +
> >> > +/* OVS includes. */
> >> > +#include "include/openvswitch/json.h"
> >> > +#include "lib/hmapx.h"
> >> > +#include "lib/util.h"
> >> > +#include "openvswitch/vlog.h"
> >> > +
> >> > +/* OVN includes. */
> >> > +#include "lib/ovn-util.h"
> >> > +#include "lib/ovn-sb-idl.h"
> >> > +#include "local_data.h"
> >> > +#include "lport.h"
> >> > +
> >> > +VLOG_DEFINE_THIS_MODULE(ldata);
> >> > +
> >> > +static struct local_datapath *local_datapath_add__(
> >> > +    struct hmap *local_datapaths,
> >> > +    const struct sbrec_datapath_binding *,
> >> > +    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,
> >> > +    int depth,
> >> > +    void (*datapath_added)(struct local_datapath *,
> >> > +                           void *aux),
> >> > +    void *aux);
> >> > +
> >> > +static struct tracked_datapath *tracked_datapath_create(
> >> > +    const struct sbrec_datapath_binding *dp,
> >> > +    enum en_tracked_resource_type tracked_type,
> >> > +    struct hmap *tracked_datapaths);
> >> > +
> >> > +struct local_datapath *
> >> > +get_local_datapath(const struct hmap *local_datapaths, uint32_t
> >> tunnel_key)
> >> > +{
> >> > +    struct hmap_node *node = hmap_first_with_hash(local_datapaths,
> >> tunnel_key);
> >> > +    return (node
> >> > +            ? CONTAINER_OF(node, struct local_datapath, hmap_node)
> >> > +            : NULL);
> >> > +}
> >> > +
> >> > +struct local_datapath *
> >> > +local_datapath_alloc(const struct sbrec_datapath_binding *dp)
> >> > +{
> >> > +    struct local_datapath *ld = xzalloc(sizeof *ld);
> >> > +    ld->datapath = dp;
> >> > +    ld->is_switch = datapath_is_switch(dp);
> >> > +    shash_init(&ld->external_ports);
> >> > +    return ld;
> >> > +}
> >> > +
> >> > +void
> >> > +local_datapaths_destroy(struct hmap *local_datapaths)
> >> > +{
> >> > +    struct local_datapath *ld;
> >> > +    HMAP_FOR_EACH_POP (ld, hmap_node, local_datapaths) {
> >> > +        local_datapath_destroy(ld);
> >> > +    }
> >> > +
> >> > +    hmap_destroy(local_datapaths);
> >> > +}
> >> > +
> >> > +void
> >> > +local_datapath_destroy(struct local_datapath *ld)
> >> > +{
> >> > +    free(ld->peer_ports);
> >> > +    shash_destroy(&ld->external_ports);
> >> > +    free(ld);
> >> > +}
> >> > +
> >> > +struct local_datapath *
> >> > +local_datapath_add(struct hmap *local_datapaths,
> >> > +                   const struct sbrec_datapath_binding *dp,
> >> > +                   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,
> >> > +                   void (*datapath_added_cb)(
> >> > +                         struct local_datapath *ld,
> >> > +                         void *aux),
> >> > +                   void *aux)
> >> > +{
> >> > +    return local_datapath_add__(local_datapaths, dp,
> >> > +                                sbrec_datapath_binding_by_key,
> >> > +                                sbrec_port_binding_by_datapath,
> >> > +                                sbrec_port_binding_by_name, 0,
> >> > +                                datapath_added_cb, aux);
> >> > +}
> >> > +
> >> > +void
> >> > +local_datapath_add_peer_port(
> >> > +    const struct sbrec_port_binding *pb,
> >> > +    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,
> >> > +    struct local_datapath *ld,
> >> > +    struct hmap *local_datapaths,
> >> > +    void (*datapath_added_cb)(
> >> > +                         struct local_datapath *ld,
> >> > +                         void *aux),
> >> > +    void *aux)
> >> > +{
> >> > +    const struct sbrec_port_binding *peer;
> >> > +    peer = lport_get_peer(pb, sbrec_port_binding_by_name);
> >> > +
> >> > +    if (!peer) {
> >> > +        return;
> >> > +    }
> >> > +
> >> > +    bool present = false;
> >> > +    for (size_t i = 0; i < ld->n_peer_ports; i++) {
> >> > +        if (ld->peer_ports[i].local == pb) {
> >> > +            present = true;
> >> > +            break;
> >> > +        }
> >> > +    }
> >> > +
> >> > +    if (!present) {
> >> > +        ld->n_peer_ports++;
> >> > +        if (ld->n_peer_ports > ld->n_allocated_peer_ports) {
> >> > +            ld->peer_ports =
> >> > +                x2nrealloc(ld->peer_ports,
> >> > +                           &ld->n_allocated_peer_ports,
> >> > +                           sizeof *ld->peer_ports);
> >> > +        }
> >> > +        ld->peer_ports[ld->n_peer_ports - 1].local = pb;
> >> > +        ld->peer_ports[ld->n_peer_ports - 1].remote = peer;
> >> > +    }
> >> > +
> >> > +    struct local_datapath *peer_ld =
> >> > +        get_local_datapath(local_datapaths,
> >> > +                           peer->datapath->tunnel_key);
> >> > +    if (!peer_ld) {
> >> > +        local_datapath_add__(local_datapaths, peer->datapath,
> >> > +                             sbrec_datapath_binding_by_key,
> >> > +                             sbrec_port_binding_by_datapath,
> >> > +                             sbrec_port_binding_by_name, 1,
> >> > +                             datapath_added_cb, aux);
> >> > +        return;
> >> > +    }
> >> > +
> >> > +    for (size_t i = 0; i < peer_ld->n_peer_ports; i++) {
> >> > +        if (peer_ld->peer_ports[i].local == peer) {
> >> > +            return;
> >> > +        }
> >> > +    }
> >> > +
> >> > +    peer_ld->n_peer_ports++;
> >> > +    if (peer_ld->n_peer_ports > peer_ld->n_allocated_peer_ports) {
> >> > +        peer_ld->peer_ports =
> >> > +            x2nrealloc(peer_ld->peer_ports,
> >> > +                        &peer_ld->n_allocated_peer_ports,
> >> > +                        sizeof *peer_ld->peer_ports);
> >> > +    }
> >> > +    peer_ld->peer_ports[peer_ld->n_peer_ports - 1].local = peer;
> >> > +    peer_ld->peer_ports[peer_ld->n_peer_ports - 1].remote = pb;
> >> > +}
> >> > +
> >> > +void
> >> > +local_datapath_remove_peer_port(const struct sbrec_port_binding *pb,
> >> > +                                struct local_datapath *ld,
> >> > +                                struct hmap *local_datapaths)
> >> > +{
> >> > +    size_t i = 0;
> >> > +    for (i = 0; i < ld->n_peer_ports; i++) {
> >> > +        if (ld->peer_ports[i].local == pb) {
> >> > +            break;
> >> > +        }
> >> > +    }
> >> > +
> >> > +    if (i == ld->n_peer_ports) {
> >> > +        return;
> >> > +    }
> >> > +
> >> > +    const struct sbrec_port_binding *peer = ld->peer_ports[i].remote;
> >> > +
> >> > +    /* Possible improvement: We can shrink the allocated peer ports
> >> > +     * if (ld->n_peer_ports < ld->n_allocated_peer_ports / 2).
> >> > +     */
> >> > +    ld->peer_ports[i].local = ld->peer_ports[ld->n_peer_ports -
> 1].local;
> >> > +    ld->peer_ports[i].remote = ld->peer_ports[ld->n_peer_ports -
> >> 1].remote;
> >> > +    ld->n_peer_ports--;
> >> > +
> >> > +    struct local_datapath *peer_ld =
> >> > +        get_local_datapath(local_datapaths,
> peer->datapath->tunnel_key);
> >> > +    if (peer_ld) {
> >> > +        /* Remove the peer port from the peer datapath. The peer
> >> > +         * datapath also tries to remove its peer lport, but that
> would
> >> > +         * be no-op. */
> >> > +        local_datapath_remove_peer_port(peer, peer_ld,
> local_datapaths);
> >> > +    }
> >> > +}
> >> > +
> >> > +/* track datapath functions. */
> >> > +struct tracked_datapath *
> >> > +tracked_datapath_add(const struct sbrec_datapath_binding *dp,
> >> > +                     enum en_tracked_resource_type tracked_type,
> >> > +                     struct hmap *tracked_datapaths)
> >> > +{
> >> > +    struct tracked_datapath *t_dp =
> >> > +        tracked_datapath_find(tracked_datapaths, dp);
> >> > +    if (!t_dp) {
> >> > +        t_dp = tracked_datapath_create(dp, tracked_type,
> >> tracked_datapaths);
> >> > +    } else {
> >> > +        t_dp->tracked_type = tracked_type;
> >> > +    }
> >> > +
> >> > +    return t_dp;
> >> > +}
> >> > +
> >> > +struct tracked_datapath *
> >> > +tracked_datapath_find(struct hmap *tracked_datapaths,
> >> > +                      const struct sbrec_datapath_binding *dp)
> >> > +{
> >> > +    struct tracked_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;
> >> > +}
> >> > +
> >> > +void
> >> > +tracked_datapath_lport_add(const struct sbrec_port_binding *pb,
> >> > +                           enum en_tracked_resource_type tracked_type,
> >> > +                           struct hmap *tracked_datapaths)
> >> > +{
> >> > +    struct tracked_datapath *tracked_dp =
> >> > +        tracked_datapath_find(tracked_datapaths, pb->datapath);
> >> > +    if (!tracked_dp) {
> >> > +        tracked_dp = tracked_datapath_create(pb->datapath,
> >> > +                                             TRACKED_RESOURCE_UPDATED,
> >> > +                                             tracked_datapaths);
> >> > +    }
> >> > +
> >> > +    /* Check if the lport is already present or not.
> >> > +     * If it is already present, then just update the 'pb' field. */
> >> > +    struct tracked_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->tracked_type = tracked_type;
> >> > +}
> >> > +
> >> > +void
> >> > +tracked_datapaths_destroy(struct hmap *tracked_datapaths)
> >> > +{
> >> > +    struct tracked_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);
> >> > +}
> >> > +
> >> > +/* static functions. */
> >> > +static struct local_datapath *
> >> > +local_datapath_add__(struct hmap *local_datapaths,
> >> > +                     const struct sbrec_datapath_binding *dp,
> >> > +                     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,
> >> > +                     int depth,
> >> > +                     void (*datapath_added_cb)(
> >> > +                           struct local_datapath *ld,
> >> > +                           void *aux),
> >> > +                     void *aux)
> >> > +{
> >> > +    uint32_t dp_key = dp->tunnel_key;
> >> > +    struct local_datapath *ld = get_local_datapath(local_datapaths,
> >> dp_key);
> >> > +    if (ld) {
> >> > +        return ld;
> >>
> >> Hi Numan, I had a comment here for the previous version: "This changes
> the
> >> original behavior that when the ld is found it updates the
> >> has_local_l3gateway before returning. Would this cause a problem?"
> >> You confirmed it, but there seems to be no change in this version. Did I
> >> miss anything?
> >>
> >
> > Hi Han,
> >
> > Thanks for the reviews.
> >
> > Please see the comment above and let me know if that is not correct.
> > This patch makes sure that "has_local_l3gateway" is updated with the
> > latest value after every call to local_datapath_add() and the caller in
> binding.c does this
> > and not this module.
>
> Well, it is different from the current behavior, in two ways:
> 1) The current implementation would flip ld->has_local_l3gateway of an
> existing ld from false to true, but not from true to false. (For a newly
> added ld, it is set to whatever the value is from the parameter). But your
> change would flip from true to false.
> 2) The current implementation flips the flag in the recursive function,
> which means it can flip for "ld" connected by the input, while your change
> would only flip in the first level call of local_datapath_add().
>
> However, I looked further at the impact of this change but figured out that
> the has_local_l3gateway is not really useful anymore, maybe from long time
> ago. The only usage now is in pinctrl.c but it seems the check is
> redundant. So I went ahead and removed it in this patch:
> https://patchwork.ozlabs.org/project/ovn/patch/20210802175626.2917748-1-hzhou@ovn.org/.
> Please take a look if this simplifies things.

Thanks.  I'll take a  look.

>
> Moreover, I don't see the benefit of refactoring the function
> add_local_datapath with callbacks. Do you have potential use in the future?
> I'd avoid callbacks if not necessary.

The only use case of adding the call back is to add the newly added
local datapath
to the tracked hash map.  Since add_local_datapath__() recursively
calls the same
function, a datapath can be considered and added to the local datapath.

You'd prefer binding.c to pass the "hmap" of tracked_datapaths instead
to the function - local_datapath_add() of local_data.c ?

I've no strong preference.  Let me know.

Thanks
Numan

>
> Thanks,
> Han
>
> >
> > Thanks
> > Numan
> >
> >
> >>
> >> > +    }
> >> > +
> >> > +    ld = local_datapath_alloc(dp);
> >> > +    hmap_insert(local_datapaths, &ld->hmap_node, dp_key);
> >> > +    ld->datapath = dp;
> >> > +
> >> > +    if (datapath_added_cb) {
> >> > +        datapath_added_cb(ld, aux);
> >> > +    }
> >> > +
> >> > +    if (depth >= 100) {
> >> > +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> >> > +        VLOG_WARN_RL(&rl, "datapaths nested too deep");
> >> > +        return ld;
> >> > +    }
> >> > +
> >> > +    struct sbrec_port_binding *target =
> >> > +
> >>  sbrec_port_binding_index_init_row(sbrec_port_binding_by_datapath);
> >> > +    sbrec_port_binding_index_set_datapath(target, dp);
> >> > +
> >> > +    const struct sbrec_port_binding *pb;
> >> > +    SBREC_PORT_BINDING_FOR_EACH_EQUAL (pb, target,
> >> > +
> sbrec_port_binding_by_datapath) {
> >> > +        if (!strcmp(pb->type, "patch") || !strcmp(pb->type,
> >> "l3gateway")) {
> >> > +            const char *peer_name = smap_get(&pb->options, "peer");
> >> > +            if (peer_name) {
> >> > +                const struct sbrec_port_binding *peer;
> >> > +
> >> > +                peer =
> lport_lookup_by_name(sbrec_port_binding_by_name,
> >> > +                                            peer_name);
> >> > +
> >> > +                if (peer && peer->datapath) {
> >> > +                    if (!strcmp(pb->type, "patch")) {
> >> > +                        /* Add the datapath to local datapath only for
> >> patch
> >> > +                         * ports. For l3gateway ports, since gateway
> >> router
> >> > +                         * resides on one chassis, we don't need to
> add.
> >> > +                         * Otherwise, all other chassis might create
> >> patch
> >> > +                         * ports between br-int and the provider
> bridge.
> >> */
> >> > +                        local_datapath_add__(local_datapaths,
> >> peer->datapath,
> >> > +
> >> sbrec_datapath_binding_by_key,
> >> > +
> >> sbrec_port_binding_by_datapath,
> >> > +
> sbrec_port_binding_by_name,
> >> > +                                             depth + 1,
> >> datapath_added_cb,
> >> > +                                             aux);
> >> > +                    }
> >> > +                    ld->n_peer_ports++;
> >> > +                    if (ld->n_peer_ports >
> ld->n_allocated_peer_ports) {
> >> > +                        ld->peer_ports =
> >> > +                            x2nrealloc(ld->peer_ports,
> >> > +                                       &ld->n_allocated_peer_ports,
> >> > +                                       sizeof *ld->peer_ports);
> >> > +                    }
> >> > +                    ld->peer_ports[ld->n_peer_ports - 1].local = pb;
> >> > +                    ld->peer_ports[ld->n_peer_ports - 1].remote =
> peer;
> >> > +                }
> >> > +            }
> >> > +        }
> >> > +    }
> >> > +    sbrec_port_binding_index_destroy_row(target);
> >> > +    return ld;
> >> > +}
> >> > +
> >> > +static struct tracked_datapath *
> >> > +tracked_datapath_create(const struct sbrec_datapath_binding *dp,
> >> > +                        enum en_tracked_resource_type tracked_type,
> >> > +                        struct hmap *tracked_datapaths)
> >> > +{
> >> > +    struct tracked_datapath *t_dp = xzalloc(sizeof *t_dp);
> >> > +    t_dp->dp = dp;
> >> > +    t_dp->tracked_type = tracked_type;
> >> > +    shash_init(&t_dp->lports);
> >> > +    hmap_insert(tracked_datapaths, &t_dp->node,
> >> uuid_hash(&dp->header_.uuid));
> >> > +    return t_dp;
> >> > +}
> >> > diff --git a/controller/local_data.h b/controller/local_data.h
> >> > new file mode 100644
> >> > index 000000000..e29fe928c
> >> > --- /dev/null
> >> > +++ b/controller/local_data.h
> >> > @@ -0,0 +1,119 @@
> >> > +/* Copyright (c) 2021, Red Hat, Inc.
> >> > + *
> >> > + * Licensed under the Apache License, Version 2.0 (the "License");
> >> > + * you may not use this file except in compliance with the License.
> >> > + * You may obtain a copy of the License at:
> >> > + *
> >> > + *     http://www.apache.org/licenses/LICENSE-2.0
> >> > + *
> >> > + * Unless required by applicable law or agreed to in writing, software
> >> > + * distributed under the License is distributed on an "AS IS" BASIS,
> >> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> >> implied.
> >> > + * See the License for the specific language governing permissions and
> >> > + * limitations under the License.
> >> > + */
> >> > +
> >> > +#ifndef LOCAL_DATA_H
> >> > +#define LOCAL_DATA_H 1
> >> > +
> >> > +/* OVS includes. */
> >> > +#include "include/openvswitch/shash.h"
> >> > +#include "lib/smap.h"
> >> > +
> >> > +struct sbrec_datapath_binding;
> >> > +struct sbrec_port_binding;
> >> > +struct ovsdb_idl_index;
> >> > +
> >> > +/* A logical datapath that has some relevance to this hypervisor.  A
> >> logical
> >> > + * datapath D is relevant to hypervisor H if:
> >> > + *
> >> > + *     - Some VIF or l2gateway or l3gateway port in D is located on H.
> >> > + *
> >> > + *     - D is reachable over a series of hops across patch ports,
> >> starting from
> >> > + *       a datapath relevant to H.
> >> > + *
> >> > + * The 'hmap_node''s hash value is 'datapath->tunnel_key'. */
> >> > +struct local_datapath {
> >> > +    struct hmap_node hmap_node;
> >> > +    const struct sbrec_datapath_binding *datapath;
> >> > +    bool is_switch;
> >> > +
> >> > +    /* The localnet port in this datapath, if any (at most one is
> >> allowed). */
> >> > +    const struct sbrec_port_binding *localnet_port;
> >> > +
> >> > +    /* True if this datapath contains an l3gateway port located on
> this
> >> > +     * hypervisor. */
> >> > +    bool has_local_l3gateway;
> >> > +
> >> > +    struct {
> >> > +        const struct sbrec_port_binding *local;
> >> > +        const struct sbrec_port_binding *remote;
> >> > +    } *peer_ports;
> >> > +
> >> > +    size_t n_peer_ports;
> >> > +    size_t n_allocated_peer_ports;
> >> > +
> >> > +    struct shash external_ports;
> >> > +};
> >> > +
> >> > +struct local_datapath *local_datapath_alloc(
> >> > +    const struct sbrec_datapath_binding *);
> >> > +struct local_datapath *get_local_datapath(const struct hmap *,
> >> > +                                          uint32_t tunnel_key);
> >> > +struct local_datapath * local_datapath_add(
> >> > +    struct hmap *local_datapaths, const struct sbrec_datapath_binding
> *,
> >> > +    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,
> >> > +    void (*datapath_added)(struct local_datapath *, void *aux),
> >> > +    void *aux);
> >> > +
> >> > +void local_datapaths_destroy(struct hmap *local_datapaths);
> >> > +void local_datapath_destroy(struct local_datapath *ld);
> >> > +void local_datapath_add_peer_port(
> >> > +    const struct sbrec_port_binding *pb,
> >> > +    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,
> >> > +    struct local_datapath *ld,
> >> > +    struct hmap *local_datapaths,
> >> > +    void (*datapath_added_cb)(
> >> > +                         struct local_datapath *ld,
> >> > +                         void *aux),
> >> > +    void *aux);
> >> > +
> >> > +void local_datapath_remove_peer_port(const struct sbrec_port_binding
> *pb,
> >> > +                                     struct local_datapath *ld,
> >> > +                                     struct hmap *local_datapaths);
> >> > +
> >> > +enum en_tracked_resource_type {
> >> > +    TRACKED_RESOURCE_NEW,
> >> > +    TRACKED_RESOURCE_REMOVED,
> >> > +    TRACKED_RESOURCE_UPDATED
> >> > +};
> >> > +
> >> > +/* Represents a tracked logical port. */
> >> > +struct tracked_lport {
> >> > +    const struct sbrec_port_binding *pb;
> >> > +    enum en_tracked_resource_type tracked_type;
> >> > +};
> >> > +
> >> > +/* Represent a tracked datapath. */
> >> > +struct tracked_datapath {
> >> > +    struct hmap_node node;
> >> > +    const struct sbrec_datapath_binding *dp;
> >> > +    enum en_tracked_resource_type tracked_type;
> >> > +    struct shash lports; /* shash of struct tracked_binding_lport. */
> >> > +};
> >> > +
> >> > +struct tracked_datapath * tracked_datapath_add(
> >> > +    const struct sbrec_datapath_binding *, enum
> en_tracked_resource_type,
> >> > +    struct hmap *tracked_datapaths);
> >> > +struct tracked_datapath *tracked_datapath_find(
> >> > +    struct hmap *tracked_datapaths, const struct
> sbrec_datapath_binding
> >> *);
> >> > +void tracked_datapath_lport_add(const struct sbrec_port_binding *,
> >> > +                                enum en_tracked_resource_type,
> >> > +                                struct hmap *tracked_datapaths);
> >> > +void tracked_datapaths_destroy(struct hmap *tracked_datapaths);
> >> > +
> >> > +#endif /* controller/local_data.h */
> >> > diff --git a/controller/lport.c b/controller/lport.c
> >> > index 478fcfd82..25b4ef200 100644
> >> > --- a/controller/lport.c
> >> > +++ b/controller/lport.c
> >> > @@ -23,6 +23,10 @@
> >> >  #include "lib/ovn-sb-idl.h"
> >> >  VLOG_DEFINE_THIS_MODULE(lport);
> >> >
> >> > +static const struct sbrec_port_binding *get_peer_lport(
> >> > +    const struct sbrec_port_binding *pb,
> >> > +    struct ovsdb_idl_index *sbrec_port_binding_by_name);
> >> > +
> >> >  const struct sbrec_port_binding *
> >> >  lport_lookup_by_name(struct ovsdb_idl_index
> *sbrec_port_binding_by_name,
> >> >                       const char *name)
> >> > @@ -84,6 +88,26 @@ lport_is_chassis_resident(struct ovsdb_idl_index
> >> *sbrec_port_binding_by_name,
> >> >      }
> >> >  }
> >> >
> >> > +const struct sbrec_port_binding *
> >> > +lport_get_peer(const struct sbrec_port_binding *pb,
> >> > +               struct ovsdb_idl_index *sbrec_port_binding_by_name)
> >> > +{
> >> > +    if (strcmp(pb->type, "patch")) {
> >> > +        return NULL;
> >> > +    }
> >> > +    return get_peer_lport(pb, sbrec_port_binding_by_name);
> >> > +}
> >> > +
> >> > +const struct sbrec_port_binding *
> >> > +lport_get_l3gw_peer(const struct sbrec_port_binding *pb,
> >> > +                    struct ovsdb_idl_index
> *sbrec_port_binding_by_name)
> >> > +{
> >> > +    if (strcmp(pb->type, "l3gateway")) {
> >> > +        return NULL;
> >> > +    }
> >> > +    return get_peer_lport(pb, sbrec_port_binding_by_name);
> >> > +}
> >> > +
> >> >  const struct sbrec_datapath_binding *
> >> >  datapath_lookup_by_key(struct ovsdb_idl_index
> >> *sbrec_datapath_binding_by_key,
> >> >                         uint64_t dp_key)
> >> > @@ -120,3 +144,19 @@ mcgroup_lookup_by_dp_name(
> >> >
> >> >      return retval;
> >> >  }
> >> > +
> >> > +static const struct sbrec_port_binding *
> >> > +get_peer_lport(const struct sbrec_port_binding *pb,
> >> > +               struct ovsdb_idl_index *sbrec_port_binding_by_name)
> >> > +{
> >> > +    const char *peer_name = smap_get(&pb->options, "peer");
> >> > +
> >> > +    if (!peer_name) {
> >> > +        return NULL;
> >> > +    }
> >> > +
> >> > +    const struct sbrec_port_binding *peer;
> >> > +    peer = lport_lookup_by_name(sbrec_port_binding_by_name,
> >> > +                                peer_name);
> >> > +    return (peer && peer->datapath) ? peer : NULL;
> >> > +}
> >> > diff --git a/controller/lport.h b/controller/lport.h
> >> > index 345efc184..43b3d714d 100644
> >> > --- a/controller/lport.h
> >> > +++ b/controller/lport.h
> >> > @@ -54,5 +54,10 @@ lport_is_chassis_resident(struct ovsdb_idl_index
> >> *sbrec_port_binding_by_name,
> >> >                            const struct sbrec_chassis *chassis,
> >> >                            const struct sset *active_tunnels,
> >> >                            const char *port_name);
> >> > -
> >> > +const struct sbrec_port_binding *lport_get_peer(
> >> > +    const struct sbrec_port_binding *,
> >> > +    struct ovsdb_idl_index *sbrec_port_binding_by_name);
> >> > +const struct sbrec_port_binding *lport_get_l3gw_peer(
> >> > +    const struct sbrec_port_binding *,
> >> > +    struct ovsdb_idl_index *sbrec_port_binding_by_name);
> >> >  #endif /* controller/lport.h */
> >> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> >> > index 3a9bdbc97..317e9f967 100644
> >> > --- a/controller/ovn-controller.c
> >> > +++ b/controller/ovn-controller.c
> >> > @@ -39,6 +39,7 @@
> >> >  #include "lflow.h"
> >> >  #include "lflow-cache.h"
> >> >  #include "lib/vswitch-idl.h"
> >> > +#include "local_data.h"
> >> >  #include "lport.h"
> >> >  #include "memory.h"
> >> >  #include "ofctrl.h"
> >> > @@ -138,15 +139,6 @@ struct pending_pkt {
> >> >  /* Registered ofctrl seqno type for nb_cfg propagation. */
> >> >  static size_t ofctrl_seq_type_nb_cfg;
> >> >
> >> > -struct local_datapath *
> >> > -get_local_datapath(const struct hmap *local_datapaths, uint32_t
> >> tunnel_key)
> >> > -{
> >> > -    struct hmap_node *node = hmap_first_with_hash(local_datapaths,
> >> tunnel_key);
> >> > -    return (node
> >> > -            ? CONTAINER_OF(node, struct local_datapath, hmap_node)
> >> > -            : NULL);
> >> > -}
> >> > -
> >> >  uint32_t
> >> >  get_tunnel_type(const char *name)
> >> >  {
> >> > @@ -1075,14 +1067,14 @@ struct ed_type_runtime_data {
> >> >   *
> >> >   *
> >>  ------------------------------------------------------------------------
> >> >   * |                      | This is a hmap of
> >>     |
> >> > - * |                      | 'struct tracked_binding_datapath' defined
> in
> >>    |
> >> > - * |                      | binding.h. Runtime data handlers for OVS
> >>    |
> >> > + * |                      | 'struct tracked_datapath' defined in
> >>    |
> >> > + * |                      | ldata.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
> 'lbinding_data').
> >>    |
> >> > + * |                      | (added/updated/deleted in
> 'lbinding_data').
> >>     |
> >> >   * |                      | So any changes to the runtime data -
> >>    |
> >> > - * |                      | local_datapaths and lbinding_data is
> >> captured  |
> >> > + * |                      | local_datapaths and lbinding_data is
> >> captured   |
> >> >   * |                      | here.
> >>     |
> >> >   *
> >>  ------------------------------------------------------------------------
> >> >   * |                      | This is a bool which represents if the
> >> runtime  |
> >> > @@ -1109,7 +1101,7 @@ struct ed_type_runtime_data {
> >> >   *
> >> >   *
>  ---------------------------------------------------------------------
> >> >   * | local_datapaths  | The changes to these runtime data is captured
> in
> >> |
> >> > - * | lbinding_data   | the @tracked_dp_bindings indirectly and hence
> it |
> >> > + * | lbinding_data   | 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
> >>  |
> >> > @@ -1134,7 +1126,7 @@ en_runtime_data_clear_tracked_data(void *data_)
> >> >  {
> >> >      struct ed_type_runtime_data *data = data_;
> >> >
> >> > -    binding_tracked_dp_destroy(&data->tracked_dp_bindings);
> >> > +    tracked_datapaths_destroy(&data->tracked_dp_bindings);
> >> >      hmap_init(&data->tracked_dp_bindings);
> >> >      data->local_lports_changed = false;
> >> >      data->tracked = false;
> >> > @@ -1172,15 +1164,7 @@ en_runtime_data_cleanup(void *data)
> >> >      sset_destroy(&rt_data->active_tunnels);
> >> >      sset_destroy(&rt_data->egress_ifaces);
> >> >      smap_destroy(&rt_data->local_iface_ids);
> >> > -    struct local_datapath *cur_node, *next_node;
> >> > -    HMAP_FOR_EACH_SAFE (cur_node, next_node, hmap_node,
> >> > -                        &rt_data->local_datapaths) {
> >> > -        free(cur_node->peer_ports);
> >> > -        shash_destroy(&cur_node->external_ports);
> >> > -        hmap_remove(&rt_data->local_datapaths, &cur_node->hmap_node);
> >> > -        free(cur_node);
> >> > -    }
> >> > -    hmap_destroy(&rt_data->local_datapaths);
> >> > +    local_datapaths_destroy(&rt_data->local_datapaths);
> >> >      shash_destroy_free_data(&rt_data->local_active_ports_ipv6_pd);
> >> >      shash_destroy_free_data(&rt_data->local_active_ports_ras);
> >> >      local_binding_data_destroy(&rt_data->lbinding_data);
> >> > @@ -1292,14 +1276,7 @@ en_runtime_data_run(struct engine_node *node,
> void
> >> *data)
> >> >          /* don't cleanup since there is no data yet */
> >> >          first_run = false;
> >> >      } else {
> >> > -        struct local_datapath *cur_node, *next_node;
> >> > -        HMAP_FOR_EACH_SAFE (cur_node, next_node, hmap_node,
> >> local_datapaths) {
> >> > -            free(cur_node->peer_ports);
> >> > -            shash_destroy(&cur_node->external_ports);
> >> > -            hmap_remove(local_datapaths, &cur_node->hmap_node);
> >> > -            free(cur_node);
> >> > -        }
> >> > -        hmap_clear(local_datapaths);
> >> > +        local_datapaths_destroy(local_datapaths);
> >> >          shash_clear_free_data(local_active_ipv6_pd);
> >> >          shash_clear_free_data(local_active_ras);
> >> >          local_binding_data_destroy(&rt_data->lbinding_data);
> >> > @@ -1308,6 +1285,7 @@ en_runtime_data_run(struct engine_node *node,
> void
> >> *data)
> >> >          sset_destroy(active_tunnels);
> >> >          sset_destroy(&rt_data->egress_ifaces);
> >> >          smap_destroy(&rt_data->local_iface_ids);
> >> > +        hmap_init(local_datapaths);
> >> >          sset_init(local_lports);
> >> >          related_lports_init(&rt_data->related_lports);
> >> >          sset_init(active_tunnels);
> >> > @@ -1749,12 +1727,12 @@ port_groups_runtime_data_handler(struct
> >> engine_node *node, void *data)
> >> >                                                   pg_sb->name);
> >> >          ovs_assert(pg_lports);
> >> >
> >> > -        struct tracked_binding_datapath *tdp;
> >> > +        struct tracked_datapath *tdp;
> >> >          bool need_update = false;
> >> >          HMAP_FOR_EACH (tdp, node, &rt_data->tracked_dp_bindings) {
> >> >              struct shash_node *shash_node;
> >> >              SHASH_FOR_EACH (shash_node, &tdp->lports) {
> >> > -                struct tracked_binding_lport *lport =
> shash_node->data;
> >> > +                struct tracked_lport *lport = shash_node->data;
> >> >                  if (sset_contains(pg_lports,
> lport->pb->logical_port)) {
> >> >                      /* At least one local port-binding change is
> related
> >> to the
> >> >                       * port_group, so the port_group_cs_local needs
> >> update. */
> >> > @@ -1907,9 +1885,9 @@ ct_zones_runtime_data_handler(struct engine_node
> >> *node, void *data OVS_UNUSED)
> >> >      }
> >> >
> >> >      struct hmap *tracked_dp_bindings = &rt_data->tracked_dp_bindings;
> >> > -    struct tracked_binding_datapath *tdp;
> >> > +    struct tracked_datapath *tdp;
> >> >      HMAP_FOR_EACH (tdp, node, tracked_dp_bindings) {
> >> > -        if (tdp->is_new) {
> >> > +        if (tdp->tracked_type == TRACKED_RESOURCE_NEW) {
> >> >              /* A new datapath has been added. Fall back to full
> >> recompute. */
> >> >              return false;
> >> >          }
> >> > @@ -2431,9 +2409,9 @@ lflow_output_runtime_data_handler(struct
> >> engine_node *node,
> >> >      struct ed_type_lflow_output *fo = data;
> >> >      init_lflow_ctx(node, rt_data, fo, &l_ctx_in, &l_ctx_out);
> >> >
> >> > -    struct tracked_binding_datapath *tdp;
> >> > +    struct tracked_datapath *tdp;
> >> >      HMAP_FOR_EACH (tdp, node, tracked_dp_bindings) {
> >> > -        if (tdp->is_new) {
> >> > +        if (tdp->tracked_type == TRACKED_RESOURCE_NEW) {
> >> >              if (!lflow_add_flows_for_datapath(tdp->dp, &l_ctx_in,
> >> >                                                &l_ctx_out)) {
> >> >                  return false;
> >> > @@ -2441,7 +2419,7 @@ lflow_output_runtime_data_handler(struct
> >> engine_node *node,
> >> >          } else {
> >> >              struct shash_node *shash_node;
> >> >              SHASH_FOR_EACH (shash_node, &tdp->lports) {
> >> > -                struct tracked_binding_lport *lport =
> shash_node->data;
> >> > +                struct tracked_lport *lport = shash_node->data;
> >> >                  if (!lflow_handle_flows_for_lport(lport->pb,
> &l_ctx_in,
> >> >                                                    &l_ctx_out)) {
> >> >                      return false;
> >> > diff --git a/controller/ovn-controller.h b/controller/ovn-controller.h
> >> > index b864ed0fa..578588305 100644
> >> > --- a/controller/ovn-controller.h
> >> > +++ b/controller/ovn-controller.h
> >> > @@ -40,40 +40,6 @@ struct ct_zone_pending_entry {
> >> >      enum ct_zone_pending_state state;
> >> >  };
> >> >
> >> > -/* A logical datapath that has some relevance to this hypervisor.  A
> >> logical
> >> > - * datapath D is relevant to hypervisor H if:
> >> > - *
> >> > - *     - Some VIF or l2gateway or l3gateway port in D is located on H.
> >> > - *
> >> > - *     - D is reachable over a series of hops across patch ports,
> >> starting from
> >> > - *       a datapath relevant to H.
> >> > - *
> >> > - * The 'hmap_node''s hash value is 'datapath->tunnel_key'. */
> >> > -struct local_datapath {
> >> > -    struct hmap_node hmap_node;
> >> > -    const struct sbrec_datapath_binding *datapath;
> >> > -
> >> > -    /* The localnet port in this datapath, if any (at most one is
> >> allowed). */
> >> > -    const struct sbrec_port_binding *localnet_port;
> >> > -
> >> > -    /* True if this datapath contains an l3gateway port located on
> this
> >> > -     * hypervisor. */
> >> > -    bool has_local_l3gateway;
> >> > -
> >> > -    struct {
> >> > -        const struct sbrec_port_binding *local;
> >> > -        const struct sbrec_port_binding *remote;
> >> > -    } *peer_ports;
> >> > -
> >> > -    size_t n_peer_ports;
> >> > -    size_t n_allocated_peer_ports;
> >> > -
> >> > -    struct shash external_ports;
> >> > -};
> >> > -
> >> > -struct local_datapath *get_local_datapath(const struct hmap *,
> >> > -                                          uint32_t tunnel_key);
> >> > -
> >> >  const struct ovsrec_bridge *get_bridge(const struct
> ovsrec_bridge_table
> >> *,
> >> >                                         const char *br_name);
> >> >
> >> > diff --git a/controller/patch.c b/controller/patch.c
> >> > index e54b56354..a661025da 100644
> >> > --- a/controller/patch.c
> >> > +++ b/controller/patch.c
> >> > @@ -20,6 +20,7 @@
> >> >  #include "hash.h"
> >> >  #include "lflow.h"
> >> >  #include "lib/vswitch-idl.h"
> >> > +#include "local_data.h"
> >> >  #include "lport.h"
> >> >  #include "openvswitch/hmap.h"
> >> >  #include "openvswitch/vlog.h"
> >> > diff --git a/controller/physical.c b/controller/physical.c
> >> > index 00cf2ca35..188606927 100644
> >> > --- a/controller/physical.c
> >> > +++ b/controller/physical.c
> >> > @@ -21,6 +21,7 @@
> >> >  #include "flow.h"
> >> >  #include "ha-chassis.h"
> >> >  #include "lflow.h"
> >> > +#include "local_data.h"
> >> >  #include "lport.h"
> >> >  #include "chassis.h"
> >> >  #include "lib/bundle.h"
> >> > diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> >> > index 8e4c4d18c..9e448e3c7 100644
> >> > --- a/controller/pinctrl.c
> >> > +++ b/controller/pinctrl.c
> >> > @@ -25,6 +25,7 @@
> >> >  #include "encaps.h"
> >> >  #include "flow.h"
> >> >  #include "ha-chassis.h"
> >> > +#include "local_data.h"
> >> >  #include "lport.h"
> >> >  #include "mac-learn.h"
> >> >  #include "nx-match.h"
> >> > --
> >> > 2.31.1
> >> >
> >> > _______________________________________________
> >> > dev mailing list
> >> > dev at openvswitch.org
> >> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >> _______________________________________________
> >> dev mailing list
> >> dev at openvswitch.org
> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >>
> >>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list