On Fri, Jun 2, 2023 at 12:13 AM Han Zhou <[email protected]> wrote:
>
> This patch introduces a change handler for NB logical_switch within the
> 'northd' node. It specifically handles cases where logical switch ports
> in the tracked logical switches are changed (added/updated/deleted).
> Only regular logical switch ports - which are common for VMs/Pods - are
> addressed. For other scenarios, it reverts to recompute.
>
> This update avoids recompute in the northd node (especially the
> resource-intensive build_ports()) for NB changes of VIF
> add/update/delete.  However, it does not eliminate the need for lflow
> recompute.
>
> Below are the performance test results simulating an ovn-k8s topology of
> 500 nodes x 50 lsp per node:
>
> Before:
> ovn-nbctl --wait=hv --print-wait-time lsp-del lsp_1_0001_01
>     ovn-northd completion:          955ms
> ovn-nbctl --wait=hv --print-wait-time lsp-add ls_1_0001 lsp_1_0001_01 -- 
> lsp-set-addresses lsp_1_0001_01 "ff:f1:bb:00:01:01 1.0.1.101" -- 
> lsp-set-port-security lsp_1_0001_01 "ff:f1:bb:00:01:01 1.0.1.101"
>     ovn-northd completion:          919ms
>
> After:
> ovn-nbctl --wait=hv --print-wait-time lsp-del lsp_1_0001_01
>     ovn-northd completion:          776ms
> ovn-nbctl --wait=hv --print-wait-time lsp-add ls_1_0001 lsp_1_0001_01 -- 
> lsp-set-addresses lsp_1_0001_01 "ff:f1:bb:00:01:01 1.0.1.101" -- 
> lsp-set-port-security lsp_1_0001_01 "ff:f1:bb:00:01:01 1.0.1.101"
>     ovn-northd completion:          773ms
>
> Both addition and deletion show ~20% reduction of ovn-northd completion
> time.  Note: the test uses only 1 thread of ovn-northd for flow
> recompute. Using multithread should show a larger percentage of
> improvement.
>
> Signed-off-by: Han Zhou <[email protected]>
> Reviewed-by: Ales Musil <[email protected]>

Few nits below.

Acked-by: Numan Siddique <[email protected]>

Numan


> ---
>  lib/ovn-util.c           |  15 ++
>  lib/ovn-util.h           |   1 +
>  northd/en-northd.c       | 130 ++++++---
>  northd/en-northd.h       |   2 +
>  northd/inc-proc-northd.c |  12 +-
>  northd/northd.c          | 567 +++++++++++++++++++++++++++++++++------
>  northd/northd.h          |  27 +-
>  tests/ovn-northd.at      |  58 ++++
>  8 files changed, 685 insertions(+), 127 deletions(-)
>
> diff --git a/lib/ovn-util.c b/lib/ovn-util.c
> index bffb521cfd9d..da2a9d45ac64 100644
> --- a/lib/ovn-util.c
> +++ b/lib/ovn-util.c
> @@ -720,6 +720,21 @@ ovn_allocate_tnlid(struct hmap *set, const char *name, 
> uint32_t min,
>      return 0;
>  }
>
> +bool
> +ovn_free_tnlid(struct hmap *tnlids, uint32_t tnlid)
> +{
> +    uint32_t hash = hash_int(tnlid, 0);
> +    struct tnlid_node *node;
> +    HMAP_FOR_EACH_IN_BUCKET (node, hmap_node, hash, tnlids) {
> +        if (node->tnlid == tnlid) {
> +            hmap_remove(tnlids, &node->hmap_node);
> +            free(node);
> +            return true;
> +        }
> +    }
> +    return false;
> +}
> +
>  char *
>  ovn_chassis_redirect_name(const char *port_name)
>  {
> diff --git a/lib/ovn-util.h b/lib/ovn-util.h
> index b17b0e2364c5..9681a12219a9 100644
> --- a/lib/ovn-util.h
> +++ b/lib/ovn-util.h
> @@ -167,6 +167,7 @@ bool ovn_add_tnlid(struct hmap *set, uint32_t tnlid);
>  bool ovn_tnlid_present(struct hmap *tnlids, uint32_t tnlid);
>  uint32_t ovn_allocate_tnlid(struct hmap *set, const char *name, uint32_t min,
>                              uint32_t max, uint32_t *hint);
> +bool ovn_free_tnlid(struct hmap *tnlids, uint32_t tnlid);
>
>  static inline void
>  get_unique_lport_key(uint64_t dp_tunnel_key, uint64_t lport_tunnel_key,
> diff --git a/northd/en-northd.c b/northd/en-northd.c
> index a3dc37e198e3..f2bf98f774b1 100644
> --- a/northd/en-northd.c
> +++ b/northd/en-northd.c
> @@ -31,92 +31,103 @@
>
>  VLOG_DEFINE_THIS_MODULE(en_northd);
>
> -void en_northd_run(struct engine_node *node, void *data)
> +static void
> +northd_get_input_data(struct engine_node *node,
> +                      struct northd_input *input_data)
>  {
> -    const struct engine_context *eng_ctx = engine_get_context();
> -
> -    struct northd_input input_data;
> -
> -    northd_destroy(data);
> -    northd_init(data);
> -
> -    input_data.sbrec_chassis_by_name =
> +    input_data->sbrec_chassis_by_name =
>          engine_ovsdb_node_get_index(
>              engine_get_input("SB_chassis", node),
>              "sbrec_chassis_by_name");
> -    input_data.sbrec_chassis_by_hostname =
> +    input_data->sbrec_chassis_by_hostname =
>          engine_ovsdb_node_get_index(
>              engine_get_input("SB_chassis", node),
>              "sbrec_chassis_by_hostname");
> -    input_data.sbrec_ha_chassis_grp_by_name =
> +    input_data->sbrec_ha_chassis_grp_by_name =
>          engine_ovsdb_node_get_index(
>              engine_get_input("SB_ha_chassis_group", node),
>              "sbrec_ha_chassis_grp_by_name");
> -    input_data.sbrec_ip_mcast_by_dp =
> +    input_data->sbrec_ip_mcast_by_dp =
>          engine_ovsdb_node_get_index(
>              engine_get_input("SB_ip_multicast", node),
>              "sbrec_ip_mcast_by_dp");
> -    input_data.sbrec_static_mac_binding_by_lport_ip =
> +    input_data->sbrec_static_mac_binding_by_lport_ip =
>          engine_ovsdb_node_get_index(
>              engine_get_input("SB_static_mac_binding", node),
>              "sbrec_static_mac_binding_by_lport_ip");
> +    input_data->sbrec_fdb_by_dp_and_port =
> +        engine_ovsdb_node_get_index(
> +            engine_get_input("SB_fdb", node),
> +            "sbrec_fdb_by_dp_and_port");
>
> -    input_data.nbrec_nb_global_table =
> +    input_data->nbrec_nb_global_table =
>          EN_OVSDB_GET(engine_get_input("NB_nb_global", node));
> -    input_data.nbrec_logical_switch_table =
> +    input_data->nbrec_logical_switch_table =
>          EN_OVSDB_GET(engine_get_input("NB_logical_switch", node));
> -    input_data.nbrec_logical_router_table =
> +    input_data->nbrec_logical_router_table =
>          EN_OVSDB_GET(engine_get_input("NB_logical_router", node));
> -    input_data.nbrec_load_balancer_table =
> +    input_data->nbrec_load_balancer_table =
>          EN_OVSDB_GET(engine_get_input("NB_load_balancer", node));
> -    input_data.nbrec_load_balancer_group_table =
> +    input_data->nbrec_load_balancer_group_table =
>          EN_OVSDB_GET(engine_get_input("NB_load_balancer_group", node));
> -    input_data.nbrec_port_group_table =
> +    input_data->nbrec_port_group_table =
>          EN_OVSDB_GET(engine_get_input("NB_port_group", node));
> -    input_data.nbrec_meter_table =
> +    input_data->nbrec_meter_table =
>          EN_OVSDB_GET(engine_get_input("NB_meter", node));
> -    input_data.nbrec_acl_table =
> +    input_data->nbrec_acl_table =
>          EN_OVSDB_GET(engine_get_input("NB_acl", node));
> -    input_data.nbrec_static_mac_binding_table =
> +    input_data->nbrec_static_mac_binding_table =
>          EN_OVSDB_GET(engine_get_input("NB_static_mac_binding", node));
> -    input_data.nbrec_chassis_template_var_table =
> +    input_data->nbrec_chassis_template_var_table =
>          EN_OVSDB_GET(engine_get_input("NB_chassis_template_var", node));
> -    input_data.nbrec_mirror_table =
> +    input_data->nbrec_mirror_table =
>          EN_OVSDB_GET(engine_get_input("NB_mirror", node));
>
> -    input_data.sbrec_sb_global_table =
> +    input_data->sbrec_sb_global_table =
>          EN_OVSDB_GET(engine_get_input("SB_sb_global", node));
> -    input_data.sbrec_datapath_binding_table =
> +    input_data->sbrec_datapath_binding_table =
>          EN_OVSDB_GET(engine_get_input("SB_datapath_binding", node));
> -    input_data.sbrec_port_binding_table =
> +    input_data->sbrec_port_binding_table =
>          EN_OVSDB_GET(engine_get_input("SB_port_binding", node));
> -    input_data.sbrec_mac_binding_table =
> +    input_data->sbrec_mac_binding_table =
>          EN_OVSDB_GET(engine_get_input("SB_mac_binding", node));
> -    input_data.sbrec_ha_chassis_group_table =
> +    input_data->sbrec_ha_chassis_group_table =
>          EN_OVSDB_GET(engine_get_input("SB_ha_chassis_group", node));
> -    input_data.sbrec_chassis_table =
> +    input_data->sbrec_chassis_table =
>          EN_OVSDB_GET(engine_get_input("SB_chassis", node));
> -    input_data.sbrec_fdb_table =
> +    input_data->sbrec_fdb_table =
>          EN_OVSDB_GET(engine_get_input("SB_fdb", node));
> -    input_data.sbrec_load_balancer_table =
> +    input_data->sbrec_load_balancer_table =
>          EN_OVSDB_GET(engine_get_input("SB_load_balancer", node));
> -    input_data.sbrec_service_monitor_table =
> +    input_data->sbrec_service_monitor_table =
>          EN_OVSDB_GET(engine_get_input("SB_service_monitor", node));
> -    input_data.sbrec_port_group_table =
> +    input_data->sbrec_port_group_table =
>          EN_OVSDB_GET(engine_get_input("SB_port_group", node));
> -    input_data.sbrec_meter_table =
> +    input_data->sbrec_meter_table =
>          EN_OVSDB_GET(engine_get_input("SB_meter", node));
> -    input_data.sbrec_dns_table =
> +    input_data->sbrec_dns_table =
>          EN_OVSDB_GET(engine_get_input("SB_dns", node));
> -    input_data.sbrec_ip_multicast_table =
> +    input_data->sbrec_ip_multicast_table =
>          EN_OVSDB_GET(engine_get_input("SB_ip_multicast", node));
> -    input_data.sbrec_static_mac_binding_table =
> +    input_data->sbrec_static_mac_binding_table =
>          EN_OVSDB_GET(engine_get_input("SB_static_mac_binding", node));
> -    input_data.sbrec_chassis_template_var_table =
> +    input_data->sbrec_chassis_template_var_table =
>          EN_OVSDB_GET(engine_get_input("SB_chassis_template_var", node));
> -    input_data.sbrec_mirror_table =
> +    input_data->sbrec_mirror_table =
>          EN_OVSDB_GET(engine_get_input("SB_mirror", node));
> +}
>
> +void
> +en_northd_run(struct engine_node *node, void *data)
> +{
> +    const struct engine_context *eng_ctx = engine_get_context();
> +
> +    struct northd_input input_data;
> +
> +    northd_destroy(data);
> +    northd_init(data);
> +
> +    northd_get_input_data(node, &input_data);
>      northd_run(&input_data, data,
>                 eng_ctx->ovnnb_idl_txn,
>                 eng_ctx->ovnsb_idl_txn);
> @@ -148,17 +159,48 @@ northd_nb_nb_global_handler(struct engine_node *node,
>      return true;
>  }
>
> -void *en_northd_init(struct engine_node *node OVS_UNUSED,
> -                     struct engine_arg *arg OVS_UNUSED)
> +bool
> +northd_nb_logical_switch_handler(struct engine_node *node,
> +                                 void *data)
>  {
> -    struct northd_data *data = xmalloc(sizeof *data);
> +    const struct engine_context *eng_ctx = engine_get_context();
> +    struct northd_data *nd = data;
> +
> +    struct northd_input input_data;
> +
> +    northd_get_input_data(node, &input_data);
> +
> +    if (!northd_handle_ls_changes(eng_ctx->ovnsb_idl_txn, &input_data, nd)) {
> +        return false;
> +    }
> +
> +    if (nd->change_tracked) {
> +        engine_set_node_state(node, EN_UPDATED);
> +    }
> +
> +    return true;
> +}
> +
> +void
> +*en_northd_init(struct engine_node *node OVS_UNUSED,
> +                struct engine_arg *arg OVS_UNUSED)
> +{
> +    struct northd_data *data = xzalloc(sizeof *data);
>
>      northd_init(data);
>
>      return data;
>  }
>
> -void en_northd_cleanup(void *data)
> +void
> +en_northd_cleanup(void *data)
>  {
>      northd_destroy(data);
>  }
> +
> +void
> +en_northd_clear_tracked_data(void *data_)
> +{
> +    struct northd_data *data = data_;
> +    destroy_northd_data_tracked_changes(data);
> +}
> diff --git a/northd/en-northd.h b/northd/en-northd.h
> index 8d8343b459a6..a53a162bda48 100644
> --- a/northd/en-northd.h
> +++ b/northd/en-northd.h
> @@ -13,6 +13,8 @@ void en_northd_run(struct engine_node *node OVS_UNUSED, 
> void *data OVS_UNUSED);
>  void *en_northd_init(struct engine_node *node OVS_UNUSED,
>                       struct engine_arg *arg);
>  void en_northd_cleanup(void *data);
> +void en_northd_clear_tracked_data(void *data);
>  bool northd_nb_nb_global_handler(struct engine_node *, void *data 
> OVS_UNUSED);
> +bool northd_nb_logical_switch_handler(struct engine_node *, void *data);
>
>  #endif /* EN_NORTHD_H */
> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> index 863c9323c444..f992a9ec8420 100644
> --- a/northd/inc-proc-northd.c
> +++ b/northd/inc-proc-northd.c
> @@ -128,7 +128,7 @@ enum sb_engine_node {
>
>  /* Define engine nodes for other nodes. They should be defined as static to
>   * avoid sparse errors. */
> -static ENGINE_NODE(northd, "northd");
> +static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(northd, "northd");
>  static ENGINE_NODE(lflow, "lflow");
>  static ENGINE_NODE(mac_binding_aging, "mac_binding_aging");
>  static ENGINE_NODE(mac_binding_aging_waker, "mac_binding_aging_waker");
> @@ -143,7 +143,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
>       * on the second argument */
>      engine_add_input(&en_northd, &en_nb_nb_global,
>                       northd_nb_nb_global_handler);
> -    engine_add_input(&en_northd, &en_nb_logical_switch, NULL);
> +    engine_add_input(&en_northd, &en_nb_logical_switch,
> +                     northd_nb_logical_switch_handler);
>      engine_add_input(&en_northd, &en_nb_port_group, NULL);
>      engine_add_input(&en_northd, &en_nb_load_balancer, NULL);
>      engine_add_input(&en_northd, &en_nb_load_balancer_group, NULL);
> @@ -252,6 +253,13 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
>                                  "sbrec_address_set_by_name",
>                                  sbrec_address_set_by_name);
>
> +    struct ovsdb_idl_index *sbrec_fdb_by_dp_and_port
> +        = ovsdb_idl_index_create2(sb->idl, &sbrec_fdb_col_dp_key,
> +                                  &sbrec_fdb_col_port_key);
> +    engine_ovsdb_node_add_index(&en_sb_fdb,
> +                                "sbrec_fdb_by_dp_and_port",
> +                                sbrec_fdb_by_dp_and_port);
> +
>      struct northd_data *northd_data =
>          engine_get_internal_data(&en_northd);
>      unixctl_command_register("debug/chassis-features-list", "", 0, 0,
> diff --git a/northd/northd.c b/northd/northd.c
> index d79f075681d9..1a4e24978642 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -19,6 +19,7 @@
>
>  #include "debug.h"
>  #include "bitmap.h"
> +#include "coverage.h"
>  #include "dirs.h"
>  #include "ipam.h"
>  #include "openvswitch/dynamic-string.h"
> @@ -59,6 +60,8 @@
>
>  VLOG_DEFINE_THIS_MODULE(northd);
>
> +COVERAGE_DEFINE(northd_run);
> +
>  static bool controller_event_en;
>  static bool lflow_hash_lock_initialized = false;
>
> @@ -807,13 +810,20 @@ ovn_datapath_create(struct hmap *datapaths, const 
> struct uuid *key,
>      od->port_key_hint = 0;
>      hmap_insert(datapaths, &od->key_node, uuid_hash(&od->key));
>      od->lr_group = NULL;
> -    ovs_list_init(&od->port_list);
> +    hmap_init(&od->ports);
>      return od;
>  }
>
>  static void ovn_ls_port_group_destroy(struct hmap *nb_pgs);
>  static void destroy_mcast_info_for_datapath(struct ovn_datapath *od);
>
> +static void
> +destroy_ports_for_datapath(struct ovn_datapath *od)
> +{
> +    ovs_assert(hmap_is_empty(&od->ports));
> +    hmap_destroy(&od->ports);
> +}
> +
>  static void
>  ovn_datapath_destroy(struct hmap *datapaths, struct ovn_datapath *od)
>  {
> @@ -834,6 +844,7 @@ ovn_datapath_destroy(struct hmap *datapaths, struct 
> ovn_datapath *od)
>          free(od->l3dgw_ports);
>          ovn_ls_port_group_destroy(&od->nb_pgs);
>          destroy_mcast_info_for_datapath(od);
> +        destroy_ports_for_datapath(od);
>
>          free(od);
>      }
> @@ -1480,6 +1491,9 @@ struct ovn_port {
>      struct lport_addresses *ps_addrs;   /* Port security addresses. */
>      unsigned int n_ps_addrs;
>
> +    bool lsp_can_be_inc_processed; /* If it can be incrementally processed 
> when
> +                                      the port changes. */
> +
>      /* Logical router port data. */
>      const struct nbrec_logical_router_port *nbrp; /* May be NULL. */
>
> @@ -1521,11 +1535,16 @@ struct ovn_port {
>
>      struct ovs_list list;       /* In list of similar records. */
>
> -    struct ovs_list dp_node;
> +    struct hmap_node dp_node;   /* Node in od->ports. */
>
>      struct lport_addresses proxy_arp_addrs;
> +
> +    /* Temporarily used for traversing a list (or hmap) of ports. */
> +    bool visited;
>  };
>
> +static bool lsp_can_be_inc_processed(const struct nbrec_logical_switch_port 
> *);
> +
>  static bool
>  is_l3dgw_port(const struct ovn_port *op)
>  {
> @@ -1585,6 +1604,9 @@ ovn_port_set_nb(struct ovn_port *op,
>                  const struct nbrec_logical_router_port *nbrp)
>  {
>      op->nbsp = nbsp;
> +    if (nbsp) {
> +        op->lsp_can_be_inc_processed = lsp_can_be_inc_processed(nbsp);
> +    }
>      op->nbrp = nbrp;
>      init_mcast_port_info(&op->mcast_info, op->nbsp, op->nbrp);
>  }
> @@ -1610,35 +1632,46 @@ ovn_port_create(struct hmap *ports, const char *key,
>  }
>
>  static void
> -ovn_port_destroy(struct hmap *ports, struct ovn_port *port)
> +ovn_port_destroy_orphan(struct ovn_port *port)
>  {
> -    if (port) {
> -        /* Don't remove port->list.  It is used within build_ports() as a
> -         * private list and once we've exited that function it is not safe to
> -         * use it. */
> -        hmap_remove(ports, &port->key_node);
> +    if (port->tunnel_key) {
> +        ovs_assert(port->od);
> +        ovn_free_tnlid(&port->od->port_tnlids, port->tunnel_key);
> +    }
> +    for (int i = 0; i < port->n_lsp_addrs; i++) {
> +        destroy_lport_addresses(&port->lsp_addrs[i]);
> +    }
> +    free(port->lsp_addrs);
>
> -        if (port->peer) {
> -            port->peer->peer = NULL;
> -        }
> +    if (port->peer) {
> +        port->peer->peer = NULL;
> +    }
>
> -        for (int i = 0; i < port->n_lsp_addrs; i++) {
> -            destroy_lport_addresses(&port->lsp_addrs[i]);
> -        }
> -        free(port->lsp_addrs);
> +    for (int i = 0; i < port->n_ps_addrs; i++) {
> +        destroy_lport_addresses(&port->ps_addrs[i]);
> +    }
> +    free(port->ps_addrs);
>
> -        for (int i = 0; i < port->n_ps_addrs; i++) {
> -            destroy_lport_addresses(&port->ps_addrs[i]);
> -        }
> -        free(port->ps_addrs);
> +    destroy_routable_addresses(&port->routables);
>
> -        destroy_routable_addresses(&port->routables);
> +    destroy_lport_addresses(&port->lrp_networks);
> +    destroy_lport_addresses(&port->proxy_arp_addrs);
> +    free(port->json_key);
> +    free(port->key);
> +    free(port);
> +}
>
> -        destroy_lport_addresses(&port->lrp_networks);
> -        destroy_lport_addresses(&port->proxy_arp_addrs);
> -        free(port->json_key);
> -        free(port->key);
> -        free(port);
> +static void
> +ovn_port_destroy(struct hmap *ports, struct ovn_port *port)
> +{
> +    if (port) {
> +        /* Don't remove port->list. The node should be removed from such 
> lists
> +         * before calling this function. */

nit:  Please update the comment accordingly (from list to hmap)



> +        hmap_remove(ports, &port->key_node);
> +        if (port->od && !port->l3dgw_port) {
> +            hmap_remove(&port->od->ports, &port->dp_node);
> +        }
> +        ovn_port_destroy_orphan(port);
>      }
>  }
>
> @@ -2408,6 +2441,53 @@ tag_alloc_create_new_tag(struct hmap *tag_alloc_table,
>  }
>
>
> +static void
> +parse_lsp_addrs(struct ovn_port *op)
> +{
> +    const struct nbrec_logical_switch_port *nbsp = op->nbsp;
> +    ovs_assert(nbsp);
> +    op->lsp_addrs
> +        = xmalloc(sizeof *op->lsp_addrs * nbsp->n_addresses);
> +    for (size_t j = 0; j < nbsp->n_addresses; j++) {
> +        if (!strcmp(nbsp->addresses[j], "unknown")) {
> +            op->has_unknown = true;
> +            continue;
> +        }
> +        if (!strcmp(nbsp->addresses[j], "router")) {
> +            continue;
> +        }
> +        if (is_dynamic_lsp_address(nbsp->addresses[j])) {
> +            continue;
> +        } else if (!extract_lsp_addresses(nbsp->addresses[j],
> +                               &op->lsp_addrs[op->n_lsp_addrs])) {
> +            static struct vlog_rate_limit rl
> +                = VLOG_RATE_LIMIT_INIT(1, 1);
> +            VLOG_INFO_RL(&rl, "invalid syntax '%s' in logical "
> +                              "switch port addresses. No MAC "
> +                              "address found",
> +                              op->nbsp->addresses[j]);
> +            continue;
> +        }
> +        op->n_lsp_addrs++;
> +    }
> +    op->n_lsp_non_router_addrs = op->n_lsp_addrs;
> +
> +    op->ps_addrs
> +        = xmalloc(sizeof *op->ps_addrs * nbsp->n_port_security);
> +    for (size_t j = 0; j < nbsp->n_port_security; j++) {
> +        if (!extract_lsp_addresses(nbsp->port_security[j],
> +                                   &op->ps_addrs[op->n_ps_addrs])) {
> +            static struct vlog_rate_limit rl
> +                = VLOG_RATE_LIMIT_INIT(1, 1);
> +            VLOG_INFO_RL(&rl, "invalid syntax '%s' in port "
> +                              "security. No MAC address found",
> +                              op->nbsp->port_security[j]);
> +            continue;
> +        }
> +        op->n_ps_addrs++;
> +    }
> +}
> +
>  static void
>  join_logical_ports(const struct sbrec_port_binding_table *sbrec_pb_table,
>                     struct hmap *ls_datapaths, struct hmap *lr_datapaths,
> @@ -2504,49 +2584,11 @@ join_logical_ports(const struct 
> sbrec_port_binding_table *sbrec_pb_table,
>                  od->has_vtep_lports = true;
>              }
>
> -            op->lsp_addrs
> -                = xmalloc(sizeof *op->lsp_addrs * nbsp->n_addresses);
> -            for (size_t j = 0; j < nbsp->n_addresses; j++) {
> -                if (!strcmp(nbsp->addresses[j], "unknown")) {
> -                    op->has_unknown = true;
> -                    continue;
> -                }
> -                if (!strcmp(nbsp->addresses[j], "router")) {
> -                    continue;
> -                }
> -                if (is_dynamic_lsp_address(nbsp->addresses[j])) {
> -                    continue;
> -                } else if (!extract_lsp_addresses(nbsp->addresses[j],
> -                                       &op->lsp_addrs[op->n_lsp_addrs])) {
> -                    static struct vlog_rate_limit rl
> -                        = VLOG_RATE_LIMIT_INIT(1, 1);
> -                    VLOG_INFO_RL(&rl, "invalid syntax '%s' in logical "
> -                                      "switch port addresses. No MAC "
> -                                      "address found",
> -                                      op->nbsp->addresses[j]);
> -                    continue;
> -                }
> -                op->n_lsp_addrs++;
> -            }
> -            op->n_lsp_non_router_addrs = op->n_lsp_addrs;
> -
> -            op->ps_addrs
> -                = xmalloc(sizeof *op->ps_addrs * nbsp->n_port_security);
> -            for (size_t j = 0; j < nbsp->n_port_security; j++) {
> -                if (!extract_lsp_addresses(nbsp->port_security[j],
> -                                           &op->ps_addrs[op->n_ps_addrs])) {
> -                    static struct vlog_rate_limit rl
> -                        = VLOG_RATE_LIMIT_INIT(1, 1);
> -                    VLOG_INFO_RL(&rl, "invalid syntax '%s' in port "
> -                                      "security. No MAC address found",
> -                                      op->nbsp->port_security[j]);
> -                    continue;
> -                }
> -                op->n_ps_addrs++;
> -            }
> +            parse_lsp_addrs(op);
>
>              op->od = od;
> -            ovs_list_push_back(&od->port_list, &op->dp_node);
> +            hmap_insert(&od->ports, &op->dp_node,
> +                        hmap_node_hash(&op->key_node));
>              tag_alloc_add_existing_tags(tag_alloc_table, nbsp);
>          }
>      }
> @@ -2593,7 +2635,8 @@ join_logical_ports(const struct 
> sbrec_port_binding_table *sbrec_pb_table,
>
>              op->lrp_networks = lrp_networks;
>              op->od = od;
> -            ovs_list_push_back(&od->port_list, &op->dp_node);
> +            hmap_insert(&od->ports, &op->dp_node,
> +                        hmap_node_hash(&op->key_node));
>
>              if (!od->redirect_bridged) {
>                  const char *redirect_type =
> @@ -3314,6 +3357,10 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn,
>          struct smap new;
>          smap_init(&new);
>          if (is_cr_port(op)) {
> +            ovs_assert(sbrec_chassis_by_name);
> +            ovs_assert(sbrec_chassis_by_hostname);
> +            ovs_assert(sbrec_ha_chassis_grp_by_name);
> +            ovs_assert(active_ha_chassis_grps);
>              const char *redirect_type = smap_get(&op->nbrp->options,
>                                                   "redirect-type");
>
> @@ -3423,8 +3470,10 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn,
>              struct smap options;
>
>              if (has_qos && !queue_id) {
> +                ovs_assert(queue_id_bitmap);
>                  queue_id = allocate_queueid(queue_id_bitmap);
>              } else if (!has_qos && queue_id) {
> +                ovs_assert(queue_id_bitmap);
>                  bitmap_set0(queue_id_bitmap, queue_id);
>                  queue_id = 0;
>              }
> @@ -3473,6 +3522,10 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn,
>              sbrec_port_binding_set_nat_addresses(op->sb, NULL, 0);
>
>              if (!strcmp(op->nbsp->type, "external")) {
> +                ovs_assert(sbrec_chassis_by_name);
> +                ovs_assert(sbrec_chassis_by_hostname);
> +                ovs_assert(sbrec_ha_chassis_grp_by_name);
> +                ovs_assert(active_ha_chassis_grps);
>                  if (op->nbsp->ha_chassis_group) {
>                      sync_ha_chassis_group_for_sbpb(
>                          ovnsb_txn, sbrec_chassis_by_name,
> @@ -3729,6 +3782,24 @@ cleanup_stale_fdb_entries(const struct sbrec_fdb_table 
> *sbrec_fdb_table,
>      }
>  }
>
> +static void
> +delete_fdb_entry(struct ovsdb_idl_index *sbrec_fdb_by_dp_and_port,
> +                 uint32_t dp_key, uint32_t port_key)
> +{
> +    struct sbrec_fdb *target =
> +        sbrec_fdb_index_init_row(sbrec_fdb_by_dp_and_port);
> +    sbrec_fdb_index_set_dp_key(target, dp_key);
> +    sbrec_fdb_index_set_port_key(target, port_key);
> +
> +    struct sbrec_fdb *fdb_e = sbrec_fdb_index_find(sbrec_fdb_by_dp_and_port,
> +                                                   target);
> +    sbrec_fdb_index_destroy_row(target);
> +
> +    if (fdb_e) {
> +        sbrec_fdb_delete(fdb_e);
> +    }
> +}
> +
>  struct service_monitor_info {
>      struct hmap_node hmap_node;
>      const struct sbrec_service_monitor *sbrec_mon;
> @@ -3820,14 +3891,15 @@ ovn_lb_svc_create(struct ovsdb_idl_txn *ovnsb_txn, 
> struct ovn_northd_lb *lb,
>              }
>              ds_destroy(&key);
>
> -            backend_nb->op = op;
> -            backend_nb->svc_mon_src_ip = svc_mon_src_ip;
> -
>              if (!lb_vip_nb->lb_health_check || !op || !svc_mon_src_ip ||
>                  !lsp_is_enabled(op->nbsp)) {
> +                free(svc_mon_src_ip);
>                  continue;
>              }
>
> +            backend_nb->op = op;
> +            backend_nb->svc_mon_src_ip = svc_mon_src_ip;
> +
>              const char *protocol = lb->nlb->protocol;
>              if (!protocol || !protocol[0]) {
>                  protocol = "tcp";
> @@ -4155,7 +4227,7 @@ build_lrouter_lb_reachable_ips(struct ovn_datapath *od,
>              ovs_be32 vip_ip4 = in6_addr_get_mapped_ipv4(&lb->vips[i].vip);
>              struct ovn_port *op;
>
> -            LIST_FOR_EACH (op, dp_node, &od->port_list) {
> +            HMAP_FOR_EACH (op, dp_node, &od->ports) {
>                  if (lrouter_port_ipv4_reachable(op, vip_ip4)) {
>                      sset_add(&od->lb_ips->ips_v4_reachable,
>                               lb->vips[i].vip_str);
> @@ -4165,7 +4237,7 @@ build_lrouter_lb_reachable_ips(struct ovn_datapath *od,
>          } else {
>              struct ovn_port *op;
>
> -            LIST_FOR_EACH (op, dp_node, &od->port_list) {
> +            HMAP_FOR_EACH (op, dp_node, &od->ports) {
>                  if (lrouter_port_ipv6_reachable(op, &lb->vips[i].vip)) {
>                      sset_add(&od->lb_ips->ips_v6_reachable,
>                               lb->vips[i].vip_str);
> @@ -4538,7 +4610,10 @@ ovn_port_add_tnlid(struct ovn_port *op, uint32_t 
> tunnel_key)
>      return added;
>  }
>
> -static void
> +/* Returns false if the requested key is confict with another allocated key, 
> so
> + * that the I-P engine can fallback to recompute if needed; otherwise return
> + * true (even if the key is not allocated). */
> +static bool
>  ovn_port_assign_requested_tnl_id(
>      const struct sbrec_chassis_table *sbrec_chassis_table, struct ovn_port 
> *op)
>  {
> @@ -4553,7 +4628,7 @@ ovn_port_assign_requested_tnl_id(
>              VLOG_WARN_RL(&rl, "Tunnel key %"PRIu32" for port %s "
>                           "is incompatible with VXLAN",
>                           tunnel_key, op_get_name(op));
> -            return;
> +            return true;
>          }
>          if (!ovn_port_add_tnlid(op, tunnel_key)) {
>              static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> @@ -4561,11 +4636,13 @@ ovn_port_assign_requested_tnl_id(
>                           "%"PRIu32" as another LSP or LRP",
>                           op->nbsp ? "switch" : "router",
>                           op_get_name(op), tunnel_key);
> +            return false;
>          }
>      }
> +    return true;
>  }
>
> -static void
> +static bool
>  ovn_port_allocate_key(const struct sbrec_chassis_table *sbrec_chassis_table,
>                        struct hmap *ports,
>                        struct ovn_port *op)
> @@ -4581,8 +4658,10 @@ ovn_port_allocate_key(const struct sbrec_chassis_table 
> *sbrec_chassis_table,
>              }
>              ovs_list_remove(&op->list);
>              ovn_port_destroy(ports, op);
> +            return false;
>          }
>      }
> +    return true;
>  }
>
>  /* Updates the southbound Port_Binding table so that it contains the logical
> @@ -4605,6 +4684,8 @@ build_ports(struct ovsdb_idl_txn *ovnsb_txn,
>      struct hmap *ls_ports, struct hmap *lr_ports)
>  {
>      struct ovs_list sb_only, nb_only, both;
> +    /* XXX: Add tag_alloc_table and queue_id_bitmap as part of northd_data
> +     * to improve I-P. */
>      struct hmap tag_alloc_table = HMAP_INITIALIZER(&tag_alloc_table);
>      unsigned long *queue_id_bitmap = bitmap_allocate(QDISC_MAX_QUEUE_ID + 1);
>      bitmap_set1(queue_id_bitmap, 0);
> @@ -4711,6 +4792,329 @@ build_ports(struct ovsdb_idl_txn *ovnsb_txn,
>      sset_destroy(&active_ha_chassis_grps);
>  }
>
> +void
> +destroy_northd_data_tracked_changes(struct northd_data *nd)
> +{
> +    struct ls_change *ls_change;
> +    LIST_FOR_EACH_SAFE (ls_change, list_node,
> +                        &nd->tracked_ls_changes.updated) {
> +        struct ovn_port *op;
> +        LIST_FOR_EACH (op, list, &ls_change->added_ports) {
> +            ovs_list_remove(&op->list);
> +        }
> +        LIST_FOR_EACH (op, list, &ls_change->updated_ports) {
> +            ovs_list_remove(&op->list);
> +        }
> +        LIST_FOR_EACH_SAFE (op, list, &ls_change->deleted_ports) {
> +            ovs_list_remove(&op->list);
> +            ovn_port_destroy_orphan(op);
> +        }
> +        ovs_list_remove(&ls_change->list_node);
> +        free(ls_change);
> +    }
> +
> +    nd->change_tracked = false;
> +}
> +
> +/* Check if a changed LSP can be handled incrementally within the I-P engine
> + * node en_northd.
> + */
> +static bool
> +lsp_can_be_inc_processed(const struct nbrec_logical_switch_port *nbsp)
> +{
> +    /* Currently only support normal VIF. */
nit: s/Currently only support normal VIF/ Support only normal VIF for now


> +    if (nbsp->type[0]) {
> +        return false;
> +    }
> +
> +    /* Currently not support tag allocation. */
nit: s/Currently not support tag allocation/ Do not support tag
allocation for now

Same small nits for below comments.

I'm fine if you want to ignore these.


> +    if ((nbsp->parent_name && nbsp->parent_name[0]) || nbsp->tag ||
> +        nbsp->tag_request) {
> +        return false;
> +    }
> +
> +    /* Currently not support port with qos settings (need special handling 
> for
> +     * qdisc_queue_id sync. */
> +    if (port_has_qos_params(&nbsp->options)) {
> +        return false;
> +    }
> +
> +    for (size_t j = 0; j < nbsp->n_addresses; j++) {
> +        /* Currently not support dynamic address handling. */
> +        if (is_dynamic_lsp_address(nbsp->addresses[j])) {
> +            return false;
> +        }
> +        /* Currently not support "unknown" address handling.  XXX: Need to
> +         * handle od->has_unknown change and track it when the first LSP with
> +         * 'unknown' is added or when the last one is removed. */
> +        if (!strcmp(nbsp->addresses[j], "unknown")) {
> +            return false;
> +        }
> +    }
> +
> +    return true;
> +}
> +
> +static bool
> +ls_port_has_changed(const struct nbrec_logical_switch_port *old,
> +                    const struct nbrec_logical_switch_port *new)
> +{
> +    if (old != new) {
> +        return true;
> +    }
> +    /* XXX: Need a better OVSDB IDL interface for this check. */
> +    return (nbrec_logical_switch_port_row_get_seqno(new,
> +                                OVSDB_IDL_CHANGE_MODIFY) > 0);
> +}
> +
> +static struct ovn_port *
> +ovn_port_find_in_datapath(struct ovn_datapath *od, const char *name)
> +{
> +    struct ovn_port *op;
> +    HMAP_FOR_EACH_WITH_HASH (op, dp_node, hash_string(name, 0), &od->ports) {
> +        if (!strcmp(op->key, name)) {
> +            return op;
> +        }
> +    }
> +    return NULL;
> +}
> +
> +static struct ovn_port *
> +ls_port_create(struct ovsdb_idl_txn *ovnsb_txn, struct hmap *ls_ports,
> +               const char *key, const struct nbrec_logical_switch_port *nbsp,
> +               struct ovn_datapath *od, const struct sbrec_port_binding *sb,
> +               const struct sbrec_mirror_table *sbrec_mirror_table,
> +               const struct sbrec_chassis_table *sbrec_chassis_table,
> +               struct ovsdb_idl_index *sbrec_chassis_by_name,
> +               struct ovsdb_idl_index *sbrec_chassis_by_hostname)
> +{
> +    struct ovn_port *op = ovn_port_create(ls_ports, key, nbsp, NULL,
> +                                          NULL);
> +    parse_lsp_addrs(op);
> +    op->od = od;
> +    hmap_insert(&od->ports, &op->dp_node, hmap_node_hash(&op->key_node));
> +
> +    /* Assign explicitly requested tunnel ids first. */
> +    if (!ovn_port_assign_requested_tnl_id(sbrec_chassis_table, op)) {
> +        return NULL;
> +    }
> +    if (sb) {
> +        op->sb = sb;
> +        /* Keep nonconflicting tunnel IDs that are already assigned. */
> +        if (!op->tunnel_key) {
> +            ovn_port_add_tnlid(op, op->sb->tunnel_key);
> +        }
> +    } else {
> +        /* XXX: the new SB port_binding will change in IDL, so need to handle
> +         * SB port_binding updates incrementally to achieve end-to-end
> +         * incremental processing. */
> +        op->sb = sbrec_port_binding_insert(ovnsb_txn);
> +        sbrec_port_binding_set_logical_port(op->sb, op->key);
> +    }
> +    /* Assign new tunnel ids where needed. */
> +    if (!ovn_port_allocate_key(sbrec_chassis_table, ls_ports, op)) {
> +        return NULL;
> +    }
> +    ovn_port_update_sbrec(ovnsb_txn, sbrec_chassis_by_name,
> +                          sbrec_chassis_by_hostname, NULL, 
> sbrec_mirror_table,
> +                          op, NULL, NULL);
> +    return op;
> +}
> +
> +static bool
> +check_ls_changes_other_than_lsp(const struct nbrec_logical_switch *ls)
> +{
> +    /* Check if the columns are changed in this row. */
> +    enum nbrec_logical_switch_column_id col;
> +    for (col = 0; col < NBREC_LOGICAL_SWITCH_N_COLUMNS; col++) {
> +        if (nbrec_logical_switch_is_updated(ls, col) &&
> +            col != NBREC_LOGICAL_SWITCH_COL_PORTS) {
> +            return true;
> +        }
> +    }
> +
> +    /* Check if the referenced rows are changed.
> +       XXX: Need a better OVSDB IDL interface for this check. */
> +    for (size_t i = 0; i < ls->n_acls; i++) {
> +        if (nbrec_acl_row_get_seqno(ls->acls[i],
> +                                OVSDB_IDL_CHANGE_MODIFY) > 0) {
> +            return true;
> +        }
> +    }
> +    if (ls->copp && nbrec_copp_row_get_seqno(ls->copp,
> +                                OVSDB_IDL_CHANGE_MODIFY) > 0) {
> +        return true;
> +    }
> +    for (size_t i = 0; i < ls->n_dns_records; i++) {
> +        if (nbrec_dns_row_get_seqno(ls->dns_records[i],
> +                                OVSDB_IDL_CHANGE_MODIFY) > 0) {
> +            return true;
> +        }
> +    }
> +    for (size_t i = 0; i < ls->n_forwarding_groups; i++) {
> +        if (nbrec_forwarding_group_row_get_seqno(ls->forwarding_groups[i],
> +                                OVSDB_IDL_CHANGE_MODIFY) > 0) {
> +            return true;
> +        }
> +    }
> +    for (size_t i = 0; i < ls->n_load_balancer; i++) {
> +        if (nbrec_load_balancer_row_get_seqno(ls->load_balancer[i],
> +                                OVSDB_IDL_CHANGE_MODIFY) > 0) {
> +            return true;
> +        }
> +    }
> +    for (size_t i = 0; i < ls->n_load_balancer_group; i++) {
> +        if 
> (nbrec_load_balancer_group_row_get_seqno(ls->load_balancer_group[i],
> +                                OVSDB_IDL_CHANGE_MODIFY) > 0) {
> +            return true;
> +        }
> +    }
> +    for (size_t i = 0; i < ls->n_qos_rules; i++) {
> +        if (nbrec_qos_row_get_seqno(ls->qos_rules[i],
> +                                OVSDB_IDL_CHANGE_MODIFY) > 0) {
> +            return true;
> +        }
> +    }
> +    return false;
> +}
> +
> +/* Return true if changes are handled incrementally, false otherwise.
> + * When there are any changes, try to track what's exactly changed and set
> + * northd_data->change_tracked accordingly: change tracked - true, otherwise,
> + * false. */
> +bool
> +northd_handle_ls_changes(struct ovsdb_idl_txn *ovnsb_idl_txn,
> +                         const struct northd_input *ni,
> +                         struct northd_data *nd)
> +{
> +    const struct nbrec_logical_switch *changed_ls;
> +    struct ls_change *ls_change = NULL;
> +
> +    NBREC_LOGICAL_SWITCH_TABLE_FOR_EACH_TRACKED (changed_ls,
> +                                             ni->nbrec_logical_switch_table) 
> {
> +        ls_change = NULL;
> +        if (nbrec_logical_switch_is_new(changed_ls) ||
> +            nbrec_logical_switch_is_deleted(changed_ls)) {
> +            goto fail;
> +        }
> +        struct ovn_datapath *od = ovn_datapath_find(
> +                                    &nd->ls_datapaths.datapaths,
> +                                    &changed_ls->header_.uuid);
> +        if (!od) {
> +            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> +            VLOG_WARN_RL(&rl, "Internal error: a tracked updated LS doesn't "
> +                         "exist in ls_datapaths: "UUID_FMT,
> +                         UUID_ARGS(&changed_ls->header_.uuid));
> +            goto fail;
> +        }
> +
> +        /* Now only able to handle lsp changes. */
> +        if (check_ls_changes_other_than_lsp(changed_ls)) {
> +            goto fail;
> +        }
> +
> +        ls_change = xzalloc(sizeof *ls_change);
> +        ls_change->od = od;
> +        ovs_list_init(&ls_change->added_ports);
> +        ovs_list_init(&ls_change->deleted_ports);
> +        ovs_list_init(&ls_change->updated_ports);
> +
> +        struct ovn_port *op;
> +        HMAP_FOR_EACH (op, dp_node, &od->ports) {
> +            op->visited = false;
> +        }
> +
> +        /* Compare the individual ports in the old and new Logical Switches 
> */
> +        for (size_t j = 0; j < changed_ls->n_ports; ++j) {
> +            struct nbrec_logical_switch_port *new_nbsp = 
> changed_ls->ports[j];
> +            op = ovn_port_find_in_datapath(od, new_nbsp->name);
> +
> +            if (!op) {
> +                if (!lsp_can_be_inc_processed(new_nbsp)) {
> +                    goto fail;
> +                }
> +                op = ls_port_create(ovnsb_idl_txn, &nd->ls_ports,
> +                                    new_nbsp->name, new_nbsp, od, NULL,
> +                                    ni->sbrec_mirror_table,
> +                                    ni->sbrec_chassis_table,
> +                                    ni->sbrec_chassis_by_name,
> +                                    ni->sbrec_chassis_by_hostname);
> +                if (!op) {
> +                    goto fail;
> +                }
> +                ovs_list_push_back(&ls_change->added_ports,
> +                                   &op->list);
> +            } else if (ls_port_has_changed(op->nbsp, new_nbsp)) {
> +                /* Existing port updated */
> +                bool temp = false;
> +                if (lsp_is_type_changed(op->sb, new_nbsp, &temp) ||
> +                    !op->lsp_can_be_inc_processed ||
> +                    !lsp_can_be_inc_processed(new_nbsp)) {
> +                    goto fail;
> +                }
> +                const struct sbrec_port_binding *sb = op->sb;
> +                if (sset_contains(&nd->svc_monitor_lsps, new_nbsp->name)) {
> +                    /* This port is used for svc monitor, which may be 
> impacted
> +                     * by this change. Fallback to recompute. */
> +                    goto fail;
> +                }
> +                ovn_port_destroy(&nd->ls_ports, op);
> +                op = ls_port_create(ovnsb_idl_txn, &nd->ls_ports,
> +                                    new_nbsp->name, new_nbsp, od, sb,
> +                                    ni->sbrec_mirror_table,
> +                                    ni->sbrec_chassis_table,
> +                                    ni->sbrec_chassis_by_name,
> +                                    ni->sbrec_chassis_by_hostname);
> +                if (!op) {
> +                    goto fail;
> +                }
> +                ovs_list_push_back(&ls_change->updated_ports, &op->list);
> +            }
> +            op->visited = true;
> +        }
> +
> +        /* Check for deleted ports */
> +        HMAP_FOR_EACH_SAFE (op, dp_node, &od->ports) {
> +            if (!op->visited) {
> +                if (!op->lsp_can_be_inc_processed) {
> +                    goto fail;
> +                }
> +                if (sset_contains(&nd->svc_monitor_lsps, op->key)) {
> +                    /* This port was used for svc monitor, which may be
> +                     * impacted by this deletion. Fallback to recompute. */
> +                    goto fail;
> +                }
> +                ovs_list_push_back(&ls_change->deleted_ports,
> +                                   &op->list);
> +                hmap_remove(&nd->ls_ports, &op->key_node);
> +                hmap_remove(&od->ports, &op->dp_node);
> +                sbrec_port_binding_delete(op->sb);
> +                delete_fdb_entry(ni->sbrec_fdb_by_dp_and_port, 
> od->tunnel_key,
> +                                 op->tunnel_key);
> +            }
> +        }
> +
> +        if (!ovs_list_is_empty(&ls_change->added_ports) ||
> +            !ovs_list_is_empty(&ls_change->updated_ports) ||
> +            !ovs_list_is_empty(&ls_change->deleted_ports)) {
> +            ovs_list_push_back(&nd->tracked_ls_changes.updated,
> +                               &ls_change->list_node);
> +        } else {
> +            free(ls_change);
> +        }
> +    }
> +
> +    if (!ovs_list_is_empty(&nd->tracked_ls_changes.updated)) {
> +        nd->change_tracked = true;
> +    }
> +    return true;
> +
> +fail:
> +    free(ls_change);
> +    destroy_northd_data_tracked_changes(nd);
> +    return false;
> +}
> +
>  struct multicast_group {
>      const char *name;
>      uint16_t key;               /* OVN_MIN_MULTICAST...OVN_MAX_MULTICAST. */
> @@ -6072,7 +6476,7 @@ build_interconn_mcast_snoop_flows(struct ovn_datapath 
> *od,
>
>      struct ovn_port *op;
>
> -    LIST_FOR_EACH (op, dp_node, &od->port_list) {
> +    HMAP_FOR_EACH (op, dp_node, &od->ports) {
>          if (!lsp_is_remote(op->nbsp)) {
>              continue;
>          }
> @@ -12394,7 +12798,7 @@ build_mcast_lookup_flows_for_lrouter(
>           * own mac addresses.
>           */
>          struct ovn_port *op;
> -        LIST_FOR_EACH (op, dp_node, &od->port_list) {
> +        HMAP_FOR_EACH (op, dp_node, &od->ports) {
>              ds_clear(match);
>              ds_put_format(match, "eth.src == %s && igmp",
>                            op->lrp_networks.ea_s);
> @@ -16476,9 +16880,6 @@ destroy_datapaths_and_ports(struct ovn_datapaths 
> *ls_datapaths,
>          }
>      }
>
> -    ovn_datapaths_destroy(ls_datapaths);
> -    ovn_datapaths_destroy(lr_datapaths);
> -
>      struct ovn_port *port;
>      HMAP_FOR_EACH_SAFE (port, key_node, ls_ports) {
>          ovn_port_destroy(ls_ports, port);
> @@ -16489,6 +16890,9 @@ destroy_datapaths_and_ports(struct ovn_datapaths 
> *ls_datapaths,
>          ovn_port_destroy(lr_ports, port);
>      }
>      hmap_destroy(lr_ports);
> +
> +    ovn_datapaths_destroy(ls_datapaths);
> +    ovn_datapaths_destroy(lr_datapaths);
>  }
>
>  void
> @@ -16510,6 +16914,8 @@ northd_init(struct northd_data *data)
>      };
>      data->ovn_internal_version_changed = false;
>      sset_init(&data->svc_monitor_lsps);
> +    data->change_tracked = false;
> +    ovs_list_init(&data->tracked_ls_changes.updated);
>  }
>
>  void
> @@ -16976,6 +17382,7 @@ void northd_run(struct northd_input *input_data,
>                  struct ovsdb_idl_txn *ovnnb_txn,
>                  struct ovsdb_idl_txn *ovnsb_txn)
>  {
> +    COVERAGE_INC(northd_run);
>      stopwatch_start(OVNNB_DB_RUN_STOPWATCH_NAME, time_msec());
>      ovnnb_db_run(input_data, data, ovnnb_txn, ovnsb_txn);
>      stopwatch_stop(OVNNB_DB_RUN_STOPWATCH_NAME, time_msec());
> diff --git a/northd/northd.h b/northd/northd.h
> index b26828bb7111..1fd9bed477e7 100644
> --- a/northd/northd.h
> +++ b/northd/northd.h
> @@ -65,6 +65,7 @@ struct northd_input {
>      struct ovsdb_idl_index *sbrec_ha_chassis_grp_by_name;
>      struct ovsdb_idl_index *sbrec_ip_mcast_by_dp;
>      struct ovsdb_idl_index *sbrec_static_mac_binding_by_lport_ip;
> +    struct ovsdb_idl_index *sbrec_fdb_by_dp_and_port;
>  };
>
>  struct chassis_features {
> @@ -83,6 +84,22 @@ struct ovn_datapaths {
>      struct ovn_datapath **array;
>  };
>
> +/* Track what's changed for a single LS.
> + * Now only track port changes. */
> +struct ls_change {
> +    struct ovs_list list_node;
> +    struct ovn_datapath *od;
> +    struct ovs_list added_ports;
> +    struct ovs_list deleted_ports;
> +    struct ovs_list updated_ports;
> +};
> +
> +/* Track what's changed for logical switches.
> + * Now only track updated ones (added or deleted may be supported in the
> + * future). */
> +struct tracked_ls_changes {
> +    struct ovs_list updated; /* Contains struct ls_change */
> +};
>
>  struct northd_data {
>      /* Global state for 'en-northd'. */
> @@ -98,6 +115,8 @@ struct northd_data {
>      bool ovn_internal_version_changed;
>      struct chassis_features features;
>      struct sset svc_monitor_lsps;
> +    bool change_tracked;
> +    struct tracked_ls_changes tracked_ls_changes;
>  };
>
>  struct lflow_data {
> @@ -292,13 +311,19 @@ struct ovn_datapath {
>      /* Port groups related to the datapath, used only when nbs is NOT NULL. 
> */
>      struct hmap nb_pgs;
>
> -    struct ovs_list port_list;
> +    /* Map of ovn_port objects belonging to this datapath.
> +     * This map doesn't include derived ports. */
> +    struct hmap ports;
>  };
>
>  void northd_run(struct northd_input *input_data,
>                  struct northd_data *data,
>                  struct ovsdb_idl_txn *ovnnb_txn,
>                  struct ovsdb_idl_txn *ovnsb_txn);
> +bool northd_handle_ls_changes(struct ovsdb_idl_txn *,
> +                              const struct northd_input *,
> +                              struct northd_data *);
> +void destroy_northd_data_tracked_changes(struct northd_data *);
>  void northd_destroy(struct northd_data *data);
>  void northd_init(struct northd_data *data);
>  void northd_indices_create(struct northd_data *data,
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 6736429ae201..74ae84530112 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -9481,3 +9481,61 @@ AT_CHECK([ovn-sbctl lflow-list sw | grep ls_out_pre_lb 
> | grep priority=110 | gre
>
>  AT_CLEANUP
>  ])
> +
> +OVN_FOR_EACH_NORTHD_NO_HV([
> +AT_SETUP([LSP incremental processing])
> +ovn_start
> +
> +net_add n1
> +sim_add hv1
> +as hv1
> +ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.11
> +ovs-vsctl add-port br-int lsp0-0 -- set interface lsp0-0 
> external_ids:iface-id=lsp0-0
> +ovs-vsctl add-port br-int lsp0-1 -- set interface lsp0-1 
> external_ids:iface-id=lsp0-1
> +ovs-vsctl add-port br-int lsp0-2 -- set interface lsp0-2 
> external_ids:iface-id=lsp0-2
> +
> +check ovn-nbctl --wait=hv ls-add ls0
> +
> +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> +check ovn-nbctl --wait=hv lsp-add ls0 lsp0-0 -- lsp-set-addresses lsp0-0 
> "unknown"
> +OVS_WAIT_UNTIL([test `as northd ovn-appctl -t NORTHD_TYPE 
> inc-engine/show-stats northd recompute` = 5])
> +OVS_WAIT_UNTIL([test `as northd ovn-appctl -t NORTHD_TYPE 
> inc-engine/show-stats lflow recompute` = 5])
> +
> +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> +check ovn-nbctl --wait=hv lsp-add ls0 lsp0-1 -- lsp-set-addresses lsp0-1 
> "aa:aa:aa:00:00:01 192.168.0.11"
> +AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats northd 
> recompute], [0], [3
> +])
> +AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats lflow 
> recompute], [0], [6
> +])
> +
> +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> +check ovn-nbctl --wait=hv lsp-add ls0 lsp0-2 -- lsp-set-addresses lsp0-2 
> "aa:aa:aa:00:00:02 192.168.0.12"
> +AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats northd 
> recompute], [0], [3
> +])
> +AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats lflow 
> recompute], [0], [6
> +])
> +
> +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> +check ovn-nbctl --wait=hv lsp-del lsp0-1
> +AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats northd 
> recompute], [0], [1
> +])
> +AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats lflow 
> recompute], [0], [2
> +])
> +
> +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> +check ovn-nbctl --wait=hv lsp-set-addresses lsp0-2 "aa:aa:aa:00:00:88 
> 192.168.0.88"
> +AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats northd 
> recompute], [0], [1
> +])
> +AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats lflow 
> recompute], [0], [2
> +])
> +
> +# No change, no recompute
> +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> +check ovn-nbctl --wait=sb sync
> +AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats northd 
> recompute], [0], [0
> +])
> +
> +OVN_CLEANUP([hv1])
> +AT_CLEANUP
> +])
> --
> 2.30.2
>
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to