On Mon, Jan 22, 2024 at 2:31 AM Han Zhou <hz...@ovn.org> wrote:
>
> On Thu, Jan 11, 2024 at 7:28 AM <num...@ovn.org> wrote:
> >
> > From: Numan Siddique <num...@ovn.org>
> >
> > northd engine tracking data now has the following tracking data
> > - changed ovn_ports (right now only changed logical switch ports are
> > tracked.)
> > - changed load balancers.
> >
> > This separation becomes easier to add lflow handling for these
> > changes in lflow northd engine handler. This patch doesn't
> > handle the load balancer changes in lflow handler. It will
> > be handled in upcoming commits.
> >
> > Before this patch, any changes to load balancers or lb groups
> > resulted in full recompute of 'sync_to_sb_lb' and 'lflow' engine
> > nodes. Now this scenario is optimized and it results in
> > recomputes of these nodes only if 'northd' engine node adds
> > any changed load balancers in its tracked data. One example
> > is created or updating an empty load balancer group.
> >
> > Signed-off-by: Numan Siddique <num...@ovn.org>
> > ---
> > northd/en-lflow.c | 12 +-
> > northd/en-northd.c | 18 +-
> > northd/en-sync-from-sb.c | 2 +-
> > northd/en-sync-sb.c | 9 +-
> > northd/northd.c | 436 ++++++++++++++++++++-------------------
> > northd/northd.h | 86 +++++---
> > tests/ovn-northd.at | 10 +-
> > 7 files changed, 313 insertions(+), 260 deletions(-)
> >
> > diff --git a/northd/en-lflow.c b/northd/en-lflow.c
> > index 2b84fef0ef..6ba26006e0 100644
> > --- a/northd/en-lflow.c
> > +++ b/northd/en-lflow.c
> > @@ -104,12 +104,12 @@ lflow_northd_handler(struct engine_node *node,
> > void *data)
> > {
> > struct northd_data *northd_data = engine_get_input_data("northd",
> node);
> > - if (!northd_data->change_tracked) {
> > + if (!northd_has_tracked_data(&northd_data->trk_data)) {
> > return false;
> > }
> >
> > - /* Fall back to recompute if lb related data has changed. */
> > - if (northd_data->lb_changed) {
> > + /* Fall back to recompute if load balancers have changed. */
> > + if (northd_has_lbs_in_tracked_data(&northd_data->trk_data)) {
> > return false;
> > }
> >
> > @@ -119,9 +119,9 @@ lflow_northd_handler(struct engine_node *node,
> > struct lflow_input lflow_input;
> > lflow_get_input_data(node, &lflow_input);
> >
> > - if (!lflow_handle_northd_ls_changes(eng_ctx->ovnsb_idl_txn,
> > - &northd_data->tracked_ls_changes,
> > - &lflow_input,
> &lflow_data->lflows)) {
> > + if (!lflow_handle_northd_port_changes(eng_ctx->ovnsb_idl_txn,
> > + &northd_data->trk_data.trk_lsps,
> > + &lflow_input, &lflow_data->lflows)) {
> > return false;
> > }
> >
> > diff --git a/northd/en-northd.c b/northd/en-northd.c
> > index aa0f20f0c2..28559ed211 100644
> > --- a/northd/en-northd.c
> > +++ b/northd/en-northd.c
> > @@ -171,7 +171,7 @@ northd_nb_logical_switch_handler(struct engine_node
> *node,
> > return false;
> > }
> >
> > - if (nd->change_tracked) {
> > + if (northd_has_lsps_in_tracked_data(&nd->trk_data)) {
> > engine_set_node_state(node, EN_UPDATED);
> > }
> >
> > @@ -209,10 +209,6 @@ northd_nb_logical_router_handler(struct engine_node
> *node,
> > return false;
> > }
> >
> > - if (nd->change_tracked) {
> > - engine_set_node_state(node, EN_UPDATED);
> > - }
> > -
> > return true;
> > }
> >
> > @@ -230,15 +226,15 @@ northd_lb_data_handler(struct engine_node *node,
> void *data)
> > &nd->ls_datapaths,
> > &nd->lr_datapaths,
> > &nd->lb_datapaths_map,
> > - &nd->lb_group_datapaths_map)) {
> > + &nd->lb_group_datapaths_map,
> > + &nd->trk_data)) {
> > return false;
> > }
> >
> > - /* Indicate the depedendant engine nodes that load balancer/group
> > - * related data has changed (including association to logical
> > - * switch/router). */
> > - nd->lb_changed = true;
> > - engine_set_node_state(node, EN_UPDATED);
> > + if (northd_has_lbs_in_tracked_data(&nd->trk_data)) {
> > + engine_set_node_state(node, EN_UPDATED);
> > + }
> > +
> > return true;
> > }
> >
> > diff --git a/northd/en-sync-from-sb.c b/northd/en-sync-from-sb.c
> > index 48b1576173..8c05239b49 100644
> > --- a/northd/en-sync-from-sb.c
> > +++ b/northd/en-sync-from-sb.c
> > @@ -65,7 +65,7 @@ sync_from_sb_northd_handler(struct engine_node *node,
> > void *data OVS_UNUSED)
> > {
> > struct northd_data *nd = engine_get_input_data("northd", node);
> > - if (nd->change_tracked) {
> > + if (northd_has_tracked_data(&nd->trk_data)) {
> > /* So far the changes tracked in northd don't impact this node.
> > *
> > * In particular, for the LS related changes, the only field
> this node
> > diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c
> > index 2ec3bf54f8..3aaab8d005 100644
> > --- a/northd/en-sync-sb.c
> > +++ b/northd/en-sync-sb.c
> > @@ -236,7 +236,8 @@ sync_to_sb_lb_northd_handler(struct engine_node
> *node, void *data OVS_UNUSED)
> > {
> > struct northd_data *nd = engine_get_input_data("northd", node);
> >
> > - if (!nd->change_tracked || nd->lb_changed) {
> > + if (!northd_has_tracked_data(&nd->trk_data) ||
> > + northd_has_lbs_in_tracked_data(&nd->trk_data)) {
> > /* Return false if no tracking data or if lbs changed. */
> > return false;
> > }
> > @@ -306,11 +307,13 @@ sync_to_sb_pb_northd_handler(struct engine_node
> *node, void *data OVS_UNUSED)
> > }
> >
> > struct northd_data *nd = engine_get_input_data("northd", node);
> > - if (!nd->change_tracked) {
> > + if (!northd_has_tracked_data(&nd->trk_data) ||
> > + northd_has_lbs_in_tracked_data(&nd->trk_data)) {
> > + /* Return false if no tracking data or if lbs changed. */
> > return false;
> > }
> >
> > - if (!sync_pbs_for_northd_ls_changes(&nd->tracked_ls_changes)) {
> > + if (!sync_pbs_for_northd_changed_ovn_ports(&nd->trk_data.trk_lsps)) {
> > return false;
> > }
> >
> > diff --git a/northd/northd.c b/northd/northd.c
> > index 952f8200d4..f32e3bf21a 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -4910,21 +4910,20 @@ sync_pbs(struct ovsdb_idl_txn *ovnsb_idl_txn,
> struct hmap *ls_ports)
> > }
> >
> > /* Sync the SB Port bindings for the added and updated logical switch
> ports
> > - * of the tracked logical switches (from the northd engine node). */
> > + * of the tracked northd engine data. */
> > bool
> > -sync_pbs_for_northd_ls_changes(struct tracked_ls_changes *ls_changes)
> > +sync_pbs_for_northd_changed_ovn_ports( struct tracked_ovn_ports
> *trk_ovn_ports)
> > {
> > - struct ls_change *ls_change;
> > - LIST_FOR_EACH (ls_change, list_node, &ls_changes->updated) {
> > - struct ovn_port *op;
> > -
> > - LIST_FOR_EACH (op, list, &ls_change->added_ports) {
> > - sync_pb_for_op(op);
> > - }
> > + struct hmapx_node *hmapx_node;
> > + struct ovn_port *op;
> > + HMAPX_FOR_EACH (hmapx_node, &trk_ovn_ports->created) {
> > + op = hmapx_node->data;
> > + sync_pb_for_op(op);
> > + }
> >
> > - LIST_FOR_EACH (op, list, &ls_change->updated_ports) {
> > - sync_pb_for_op(op);
> > - }
> > + HMAPX_FOR_EACH (hmapx_node, &trk_ovn_ports->updated) {
> > + op = hmapx_node->data;
> > + sync_pb_for_op(op);
> > }
> >
> > return true;
> > @@ -5126,34 +5125,68 @@ build_ports(struct ovsdb_idl_txn *ovnsb_txn,
> > }
> >
> > static void
> > -destroy_tracked_ls_change(struct ls_change *ls_change)
> > +destroy_tracked_ovn_ports(struct tracked_ovn_ports *trk_ovn_ports)
> > {
> > - 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);
> > + struct hmapx_node *hmapx_node;
> > + HMAPX_FOR_EACH_SAFE (hmapx_node, &trk_ovn_ports->deleted) {
> > + ovn_port_destroy_orphan(hmapx_node->data);
> > + hmapx_delete(&trk_ovn_ports->deleted, hmapx_node);
> > }
> > - LIST_FOR_EACH_SAFE (op, list, &ls_change->deleted_ports) {
> > - ovs_list_remove(&op->list);
> > - ovn_port_destroy_orphan(op);
> > +
> > + hmapx_clear(&trk_ovn_ports->created);
> > + hmapx_clear(&trk_ovn_ports->updated);
> > +}
> > +
> > +static void
> > +destroy_tracked_lbs(struct tracked_lbs *trk_lbs)
> > +{
> > + struct hmapx_node *hmapx_node;
> > + HMAPX_FOR_EACH_SAFE (hmapx_node, &trk_lbs->deleted) {
> > + ovn_lb_datapaths_destroy(hmapx_node->data);
> > + hmapx_delete(&trk_lbs->deleted, hmapx_node);
> > }
> > +
> > + hmapx_clear(&trk_lbs->crupdated);
> > +}
> > +
> > +static void
> > +add_op_to_northd_tracked_ports(struct hmapx *tracked_ovn_ports,
> > + struct ovn_port *op)
> > +{
> > + hmapx_add(tracked_ovn_ports, op);
> > }
> >
> > 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) {
> > - destroy_tracked_ls_change(ls_change);
> > - ovs_list_remove(&ls_change->list_node);
> > - free(ls_change);
> > - }
> > + struct northd_tracked_data *trk_changes = &nd->trk_data;
> > + destroy_tracked_ovn_ports(&trk_changes->trk_lsps);
> > + destroy_tracked_lbs(&trk_changes->trk_lbs);
> > + nd->trk_data.type = NORTHD_TRACKED_NONE;
> > +}
> > +
> > +static void
> > +init_northd_tracked_data(struct northd_data *nd)
> > +{
> > + struct northd_tracked_data *trk_data = &nd->trk_data;
> > + trk_data->type = NORTHD_TRACKED_NONE;
> > + hmapx_init(&trk_data->trk_lsps.created);
> > + hmapx_init(&trk_data->trk_lsps.updated);
> > + hmapx_init(&trk_data->trk_lsps.deleted);
> > + hmapx_init(&trk_data->trk_lbs.crupdated);
> > + hmapx_init(&trk_data->trk_lbs.deleted);
> > +}
> >
> > - nd->change_tracked = false;
> > - nd->lb_changed = false;
> > +static void
> > +destroy_northd_tracked_data(struct northd_data *nd)
> > +{
> > + struct northd_tracked_data *trk_data = &nd->trk_data;
> > + trk_data->type = NORTHD_TRACKED_NONE;
> > + hmapx_destroy(&trk_data->trk_lsps.created);
> > + hmapx_destroy(&trk_data->trk_lsps.updated);
> > + hmapx_destroy(&trk_data->trk_lsps.deleted);
> > + hmapx_destroy(&trk_data->trk_lbs.crupdated);
> > + hmapx_destroy(&trk_data->trk_lbs.deleted);
> > }
> >
> > /* Check if a changed LSP can be handled incrementally within the I-P
> engine
> > @@ -5358,12 +5391,11 @@ check_lsp_changes_other_than_up(const struct
> nbrec_logical_switch_port *nbsp)
> > */
> > static bool
> > ls_handle_lsp_changes(struct ovsdb_idl_txn *ovnsb_idl_txn,
> > - const struct nbrec_logical_switch
> *changed_ls,
> > - const struct northd_input *ni,
> > - struct northd_data *nd,
> > - struct ovn_datapath *od,
> > - struct ls_change *ls_change,
> > - bool *updated)
> > + const struct nbrec_logical_switch *changed_ls,
> > + const struct northd_input *ni,
> > + struct northd_data *nd,
> > + struct ovn_datapath *od,
> > + struct tracked_ovn_ports *trk_lsps)
> > {
> > bool ls_ports_changed = false;
> > if (!nbrec_logical_switch_is_updated(changed_ls,
> > @@ -5384,7 +5416,7 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
> > return true;
> > }
> >
> > - ls_change->had_only_router_ports = (od->n_router_ports
> > + bool ls_had_only_router_ports = (od->n_router_ports
> > && (od->n_router_ports == hmap_count(&od->ports)));
> >
> > struct ovn_port *op;
> > @@ -5410,8 +5442,7 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
> > if (!op) {
> > goto fail;
> > }
> > - ovs_list_push_back(&ls_change->added_ports,
> > - &op->list);
> > + add_op_to_northd_tracked_ports(&trk_lsps->created, op);
> > } else if (ls_port_has_changed(new_nbsp)) {
> > /* Existing port updated */
> > bool temp = false;
> > @@ -5447,7 +5478,7 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
> > if (!op) {
> > goto fail;
> > }
> > - ovs_list_push_back(&ls_change->updated_ports, &op->list);
> > + add_op_to_northd_tracked_ports(&trk_lsps->updated, op);
> > }
> > op->visited = true;
> > }
> > @@ -5456,15 +5487,14 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
> > HMAP_FOR_EACH_SAFE (op, dp_node, &od->ports) {
> > if (!op->visited) {
> > if (!op->lsp_can_be_inc_processed) {
> > - goto fail_clean_deleted;
> > + 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_clean_deleted;
> > + goto fail;
> > }
> > - ovs_list_push_back(&ls_change->deleted_ports,
> > - &op->list);
> > + add_op_to_northd_tracked_ports(&trk_lsps->deleted, op);
> > hmap_remove(&nd->ls_ports, &op->key_node);
> > hmap_remove(&od->ports, &op->dp_node);
> > sbrec_port_binding_delete(op->sb);
> > @@ -5473,27 +5503,32 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
> > }
> > }
> >
> > - 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)) {
> > - *updated = true;
> > + bool ls_has_only_router_ports = (od->n_router_ports
> > + && (od->n_router_ports == hmap_count(&od->ports)));
> > +
> > + /* There are lflows related to router ports that depends on whether
> > + * there are switch ports on the logical switch (see
> > + * build_lswitch_rport_arp_req_flow() for more details). Check if
> this
> > + * dependency has changed and if it has, then add the router ports
> > + * to the tracked 'updated' ovn ports so that lflow engine can
> > + * regenerate lflows for these router ports. */
> > + if (ls_had_only_router_ports != ls_has_only_router_ports) {
> > + for (size_t i = 0; i < od->n_router_ports; i++) {
> > + op = od->router_ports[i];
> > + add_op_to_northd_tracked_ports(&trk_lsps->updated, op);
> > + }
> > }
> >
> > return true;
> >
> > -fail_clean_deleted:
> > - LIST_FOR_EACH_POP (op, list, &ls_change->deleted_ports) {
> > - ovn_port_destroy_orphan(op);
> > - }
> > -
> > fail:
> > + destroy_tracked_ovn_ports(trk_lsps);
> > 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.
> > + * northd_data->trk_data accordingly.
> > *
> > * Note: Changes to load balancer and load balancer groups associated
> with
> > * the logical switches are handled separately in the lb_data change
> handlers.
> > @@ -5504,7 +5539,7 @@ northd_handle_ls_changes(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
> > struct northd_data *nd)
> > {
> > const struct nbrec_logical_switch *changed_ls;
> > - struct ls_change *ls_change = NULL;
> > + struct northd_tracked_data *trk_data = &nd->trk_data;
> >
> > NBREC_LOGICAL_SWITCH_TABLE_FOR_EACH_TRACKED (changed_ls,
> >
> ni->nbrec_logical_switch_table) {
> > @@ -5528,31 +5563,16 @@ northd_handle_ls_changes(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
> > 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);
> > -
> > - bool updated = false;
> > if (!ls_handle_lsp_changes(ovnsb_idl_txn, changed_ls,
> > - ni, nd, od, ls_change,
> > - &updated)) {
> > - destroy_tracked_ls_change(ls_change);
> > - free(ls_change);
> > + ni, nd, od, &trk_data->trk_lsps)) {
> > goto fail;
> > }
> > -
> > - if (updated) {
> > - 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;
> > + if (!hmapx_is_empty(&trk_data->trk_lsps.created)
> > + || !hmapx_is_empty(&trk_data->trk_lsps.updated)
> > + || !hmapx_is_empty(&trk_data->trk_lsps.deleted)) {
> > + trk_data->type |= NORTHD_TRACKED_PORTS;
> > }
> >
> > return true;
> > @@ -5617,13 +5637,10 @@ lr_changes_can_be_handled(
> > }
> >
> > /* 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.
> > + *
> > * Note: Changes to load balancer and load balancer groups associated
> with
> > * the logical routers are handled separately in the lb_data change
> > - * handlers (northd_handle_lb_data_changes_pre_od and
> > - * northd_handle_lb_data_changes_post_od).
> > + * handler - northd_handle_lb_data_changes().
> > * */
> > bool
> > northd_handle_lr_changes(const struct northd_input *ni,
> > @@ -5729,7 +5746,8 @@ northd_handle_lb_data_changes(struct
> tracked_lb_data *trk_lb_data,
> > struct ovn_datapaths *ls_datapaths,
> > struct ovn_datapaths *lr_datapaths,
> > struct hmap *lb_datapaths_map,
> > - struct hmap *lbgrp_datapaths_map)
> > + struct hmap *lbgrp_datapaths_map,
> > + struct northd_tracked_data *nd_changes)
> > {
> > if (trk_lb_data->has_health_checks) {
> > /* Fall back to recompute since a tracked load balancer
> > @@ -5792,7 +5810,9 @@ northd_handle_lb_data_changes(struct
> tracked_lb_data *trk_lb_data,
> > }
> >
> > hmap_remove(lb_datapaths_map, &lb_dps->hmap_node);
> > - ovn_lb_datapaths_destroy(lb_dps);
> > +
> > + /* Add the deleted lb to the northd tracked data. */
> > + hmapx_add(&nd_changes->trk_lbs.deleted, lb_dps);
> > }
> >
> > /* Create the 'lb_dps' if not already created for each
> > @@ -5809,6 +5829,9 @@ northd_handle_lb_data_changes(struct
> tracked_lb_data *trk_lb_data,
> > hmap_insert(lb_datapaths_map, &lb_dps->hmap_node,
> > uuid_hash(lb_uuid));
> > }
> > +
> > + /* Add the updated lb to the northd tracked data. */
> > + hmapx_add(&nd_changes->trk_lbs.crupdated, lb_dps);
> > }
> >
> > struct ovn_lb_group_datapaths *lbgrp_dps;
> > @@ -5839,6 +5862,9 @@ northd_handle_lb_data_changes(struct
> tracked_lb_data *trk_lb_data,
> > lb_dps = ovn_lb_datapaths_find(lb_datapaths_map,
> &uuidnode->uuid);
> > ovs_assert(lb_dps);
> > ovn_lb_datapaths_add_ls(lb_dps, 1, &od);
> > +
> > + /* Add the lb to the northd tracked data. */
> > + hmapx_add(&nd_changes->trk_lbs.crupdated, lb_dps);
> > }
> >
> > UUIDSET_FOR_EACH (uuidnode, &codlb->assoc_lbgrps) {
> > @@ -5854,6 +5880,9 @@ northd_handle_lb_data_changes(struct
> tracked_lb_data *trk_lb_data,
> > lb_dps = ovn_lb_datapaths_find(lb_datapaths_map,
> lb_uuid);
> > ovs_assert(lb_dps);
> > ovn_lb_datapaths_add_ls(lb_dps, 1, &od);
> > +
> > + /* Add the lb to the northd tracked data. */
> > + hmapx_add(&nd_changes->trk_lbs.crupdated, lb_dps);
> > }
> > }
> >
> > @@ -5874,6 +5903,9 @@ northd_handle_lb_data_changes(struct
> tracked_lb_data *trk_lb_data,
> > /* Add the lb_ips of lb_dps to the od. */
> > build_lrouter_lb_ips(od->lb_ips, lb_dps->lb);
> > build_lrouter_lb_reachable_ips(od, lb_dps->lb);
> > +
> > + /* Add the lb to the northd tracked data. */
> > + hmapx_add(&nd_changes->trk_lbs.crupdated, lb_dps);
> > }
> >
> > UUIDSET_FOR_EACH (uuidnode, &codlb->assoc_lbgrps) {
> > @@ -5893,6 +5925,9 @@ northd_handle_lb_data_changes(struct
> tracked_lb_data *trk_lb_data,
> > /* Add the lb_ips of lb_dps to the od. */
> > build_lrouter_lb_ips(od->lb_ips, lb_dps->lb);
> > build_lrouter_lb_reachable_ips(od, lb_dps->lb);
> > +
> > + /* Add the lb to the northd tracked data. */
> > + hmapx_add(&nd_changes->trk_lbs.crupdated, lb_dps);
> > }
> > }
> >
> > @@ -5971,9 +6006,17 @@ northd_handle_lb_data_changes(struct
> tracked_lb_data *trk_lb_data,
> > /* Re-evaluate 'od->has_lb_vip' */
> > init_lb_for_datapath(od);
> > }
> > +
> > + /* Add the lb to the northd tracked data. */
> > + hmapx_add(&nd_changes->trk_lbs.crupdated, lb_dps);
> > }
> > }
> >
> > + if (!hmapx_is_empty(&nd_changes->trk_lbs.crupdated)
> > + || !hmapx_is_empty(&nd_changes->trk_lbs.deleted)) {
> > + nd_changes->type |= NORTHD_TRACKED_LBS;
> > + }
> > +
> > return true;
> > }
> >
> > @@ -17029,149 +17072,122 @@ delete_lflow_for_lsp(struct ovn_port *op,
> bool is_update,
> > return true;
> > }
> >
> > -bool lflow_handle_northd_ls_changes(struct ovsdb_idl_txn *ovnsb_txn,
> > - struct tracked_ls_changes
> *ls_changes,
> > - struct lflow_input *lflow_input,
> > - struct hmap *lflows)
> > +bool
> > +lflow_handle_northd_port_changes(struct ovsdb_idl_txn *ovnsb_txn,
> > + struct tracked_ovn_ports *trk_lsps,
> > + struct lflow_input *lflow_input,
> > + struct hmap *lflows)
> > {
> > - struct ls_change *ls_change;
> > + struct hmapx_node *hmapx_node;
> > + struct ovn_port *op;
> >
> > - LIST_FOR_EACH (ls_change, list_node, &ls_changes->updated) {
> > - const struct sbrec_multicast_group *sbmc_flood =
> > - mcast_group_lookup(lflow_input->sbrec_mcast_group_by_name_dp,
> > - MC_FLOOD, ls_change->od->sb);
> > - const struct sbrec_multicast_group *sbmc_flood_l2 =
> > - mcast_group_lookup(lflow_input->sbrec_mcast_group_by_name_dp,
> > - MC_FLOOD_L2, ls_change->od->sb);
> > - const struct sbrec_multicast_group *sbmc_unknown =
> > - mcast_group_lookup(lflow_input->sbrec_mcast_group_by_name_dp,
> > - MC_UNKNOWN, ls_change->od->sb);
> > + HMAPX_FOR_EACH (hmapx_node, &trk_lsps->deleted) {
> > + op = hmapx_node->data;
> > + /* Make sure 'op' is an lsp and not lrp. */
> > + ovs_assert(op->nbsp);
> >
> > - struct ovn_port *op;
> > - LIST_FOR_EACH (op, list, &ls_change->deleted_ports) {
> > - if (!delete_lflow_for_lsp(op, false,
> > -
> lflow_input->sbrec_logical_flow_table,
> > - lflows)) {
> > + if (!delete_lflow_for_lsp(op, false,
> > + lflow_input->sbrec_logical_flow_table,
> > + lflows)) {
> > return false;
> > }
> >
> > - /* No need to update SB multicast groups, thanks to weak
> > - * references. */
> > - }
> > + /* No need to update SB multicast groups, thanks to weak
> > + * references. */
> > + }
> >
> > - LIST_FOR_EACH (op, list, &ls_change->updated_ports) {
> > - /* Delete old lflows. */
> > - if (!delete_lflow_for_lsp(op, true,
> > -
> lflow_input->sbrec_logical_flow_table,
> > - lflows)) {
> > - return false;
> > - }
> > + HMAPX_FOR_EACH (hmapx_node, &trk_lsps->updated) {
> > + op = hmapx_node->data;
> > + /* Make sure 'op' is an lsp and not lrp. */
> > + ovs_assert(op->nbsp);
> >
> > - /* Generate new lflows. */
> > - struct ds match = DS_EMPTY_INITIALIZER;
> > - struct ds actions = DS_EMPTY_INITIALIZER;
> > - build_lswitch_and_lrouter_iterate_by_lsp(op,
> lflow_input->ls_ports,
> > -
> lflow_input->lr_ports,
> > -
> lflow_input->meter_groups,
> > - &match, &actions,
> > - lflows);
> > - ds_destroy(&match);
> > - ds_destroy(&actions);
> > -
> > - /* SB port_binding is not deleted, so don't update SB
> multicast
> > - * groups. */
> > -
> > - /* Sync the new flows to SB. */
> > - struct lflow_ref_node *lfrn;
> > - LIST_FOR_EACH (lfrn, lflow_list_node, &op->lflows) {
> > - sync_lsp_lflows_to_sb(ovnsb_txn, lflow_input, lflows,
> > - lfrn->lflow);
> > - }
> > + /* Delete old lflows. */
> > + if (!delete_lflow_for_lsp(op, true,
> > + lflow_input->sbrec_logical_flow_table,
> > + lflows)) {
> > + return false;
> > }
> >
> > - LIST_FOR_EACH (op, list, &ls_change->added_ports) {
> > - struct ds match = DS_EMPTY_INITIALIZER;
> > - struct ds actions = DS_EMPTY_INITIALIZER;
> > - build_lswitch_and_lrouter_iterate_by_lsp(op,
> lflow_input->ls_ports,
> > -
> lflow_input->lr_ports,
> > -
> lflow_input->meter_groups,
> > - &match, &actions,
> > - lflows);
> > - ds_destroy(&match);
> > - ds_destroy(&actions);
> > -
> > - /* Update SB multicast groups for the new port. */
> > - if (!sbmc_flood) {
> > - sbmc_flood = create_sb_multicast_group(ovnsb_txn,
> > - ls_change->od->sb, MC_FLOOD,
> OVN_MCAST_FLOOD_TUNNEL_KEY);
> > - }
> > - sbrec_multicast_group_update_ports_addvalue(sbmc_flood,
> op->sb);
> > + /* Generate new lflows. */
> > + struct ds match = DS_EMPTY_INITIALIZER;
> > + struct ds actions = DS_EMPTY_INITIALIZER;
> > + build_lswitch_and_lrouter_iterate_by_lsp(op,
> lflow_input->ls_ports,
> > + lflow_input->lr_ports,
> > +
> lflow_input->meter_groups,
> > + &match, &actions,
> > + lflows);
> > + ds_destroy(&match);
> > + ds_destroy(&actions);
> >
> > - if (!sbmc_flood_l2) {
> > - sbmc_flood_l2 = create_sb_multicast_group(ovnsb_txn,
> > - ls_change->od->sb, MC_FLOOD_L2,
> > - OVN_MCAST_FLOOD_L2_TUNNEL_KEY);
> > - }
> > - sbrec_multicast_group_update_ports_addvalue(sbmc_flood_l2,
> op->sb);
> > + /* SB port_binding is not deleted, so don't update SB multicast
> > + * groups. */
> >
> > - if (op->has_unknown) {
> > - if (!sbmc_unknown) {
> > - sbmc_unknown = create_sb_multicast_group(ovnsb_txn,
> > - ls_change->od->sb, MC_UNKNOWN,
> > - OVN_MCAST_UNKNOWN_TUNNEL_KEY);
> > - }
> > - sbrec_multicast_group_update_ports_addvalue(sbmc_unknown,
> > - op->sb);
> > - }
> > -
> > - /* Sync the newly added flows to SB. */
> > - struct lflow_ref_node *lfrn;
> > - LIST_FOR_EACH (lfrn, lflow_list_node, &op->lflows) {
> > - sync_lsp_lflows_to_sb(ovnsb_txn, lflow_input, lflows,
> > - lfrn->lflow);
> > - }
> > + /* Sync the new flows to SB. */
> > + struct lflow_ref_node *lfrn;
> > + LIST_FOR_EACH (lfrn, lflow_list_node, &op->lflows) {
> > + sync_lsp_lflows_to_sb(ovnsb_txn, lflow_input, lflows,
> > + lfrn->lflow);
> > }
> > + }
> >
> > - bool ls_has_only_router_ports = (ls_change->od->n_router_ports &&
> > - (ls_change->od->n_router_ports
> ==
> > -
> hmap_count(&ls_change->od->ports)));
> > -
> > - if (ls_change->had_only_router_ports !=
> ls_has_only_router_ports) {
> > - /* There are lflows related to router ports that depends on
> whether
> > - * there are switch ports on the logical switch (see
> > - * build_lswitch_rport_arp_req_flow() for more details).
> Since this
> > - * dependency changed, we need to regenerate lflows for each
> router
> > - * port on this logical switch. */
> > - for (size_t i = 0; i < ls_change->od->n_router_ports; i++) {
> > - op = ls_change->od->router_ports[i];
> > -
> > - /* Delete old lflows. */
> > - if (!delete_lflow_for_lsp(op, "affected router",
> > -
> lflow_input->sbrec_logical_flow_table,
> > - lflows)) {
> > - return false;
> > - }
> > + HMAPX_FOR_EACH (hmapx_node, &trk_lsps->created) {
> > + op = hmapx_node->data;
> > + /* Make sure 'op' is an lsp and not lrp. */
> > + ovs_assert(op->nbsp);
> >
> > - /* Generate new lflows. */
> > - struct ds match = DS_EMPTY_INITIALIZER;
> > - struct ds actions = DS_EMPTY_INITIALIZER;
> > - build_lswitch_and_lrouter_iterate_by_lsp(op,
> > - lflow_input->ls_ports, lflow_input->lr_ports,
> > - lflow_input->meter_groups, &match, &actions, lflows);
> > - ds_destroy(&match);
> > - ds_destroy(&actions);
> > -
> > - /* Sync the new flows to SB. */
> > - struct lflow_ref_node *lfrn;
> > - LIST_FOR_EACH (lfrn, lflow_list_node, &op->lflows) {
> > - sync_lsp_lflows_to_sb(ovnsb_txn, lflow_input, lflows,
> > - lfrn->lflow);
> > - }
> > + const struct sbrec_multicast_group *sbmc_flood =
> > + mcast_group_lookup(lflow_input->sbrec_mcast_group_by_name_dp,
> > + MC_FLOOD, op->od->sb);
> > + const struct sbrec_multicast_group *sbmc_flood_l2 =
> > + mcast_group_lookup(lflow_input->sbrec_mcast_group_by_name_dp,
> > + MC_FLOOD_L2, op->od->sb);
> > + const struct sbrec_multicast_group *sbmc_unknown =
> > + mcast_group_lookup(lflow_input->sbrec_mcast_group_by_name_dp,
> > + MC_UNKNOWN, op->od->sb);
> > +
> > + struct ds match = DS_EMPTY_INITIALIZER;
> > + struct ds actions = DS_EMPTY_INITIALIZER;
> > + build_lswitch_and_lrouter_iterate_by_lsp(op,
> lflow_input->ls_ports,
> > +
> lflow_input->lr_ports,
> > +
> lflow_input->meter_groups,
> > + &match, &actions,
> > + lflows);
> > + ds_destroy(&match);
> > + ds_destroy(&actions);
> > +
> > + /* Update SB multicast groups for the new port. */
> > + if (!sbmc_flood) {
> > + sbmc_flood = create_sb_multicast_group(ovnsb_txn,
> > + op->od->sb, MC_FLOOD, OVN_MCAST_FLOOD_TUNNEL_KEY);
> > + }
> > + sbrec_multicast_group_update_ports_addvalue(sbmc_flood, op->sb);
> > +
> > + if (!sbmc_flood_l2) {
> > + sbmc_flood_l2 = create_sb_multicast_group(ovnsb_txn,
> > + op->od->sb, MC_FLOOD_L2,
> > + OVN_MCAST_FLOOD_L2_TUNNEL_KEY);
> > + }
> > + sbrec_multicast_group_update_ports_addvalue(sbmc_flood_l2,
> op->sb);
> > +
> > + if (op->has_unknown) {
> > + if (!sbmc_unknown) {
> > + sbmc_unknown = create_sb_multicast_group(ovnsb_txn,
> > + op->od->sb, MC_UNKNOWN,
> > + OVN_MCAST_UNKNOWN_TUNNEL_KEY);
> > }
> > + sbrec_multicast_group_update_ports_addvalue(sbmc_unknown,
> > + op->sb);
> > + }
> > +
> > + /* Sync the newly added flows to SB. */
> > + struct lflow_ref_node *lfrn;
> > + LIST_FOR_EACH (lfrn, lflow_list_node, &op->lflows) {
> > + sync_lsp_lflows_to_sb(ovnsb_txn, lflow_input, lflows,
> > + lfrn->lflow);
> > }
> > }
> > - return true;
> >
> > + return true;
> > }
> >
> > static bool
> > @@ -17816,8 +17832,7 @@ northd_init(struct northd_data *data)
> > data->ovn_internal_version_changed = false;
> > sset_init(&data->svc_monitor_lsps);
> > hmap_init(&data->svc_monitor_map);
> > - data->change_tracked = false;
> > - ovs_list_init(&data->tracked_ls_changes.updated);
> > + init_northd_tracked_data(data);
> > }
> >
> > void
> > @@ -17857,6 +17872,7 @@ northd_destroy(struct northd_data *data)
> > destroy_debug_config();
> >
> > sset_destroy(&data->svc_monitor_lsps);
> > + destroy_northd_tracked_data(data);
> > }
> >
> > void
> > diff --git a/northd/northd.h b/northd/northd.h
> > index 5be7b5384d..23521065e8 100644
> > --- a/northd/northd.h
> > +++ b/northd/northd.h
> > @@ -83,22 +83,44 @@ 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;
> > - bool had_only_router_ports;
> > +struct tracked_ovn_ports {
> > + /* tracked created ports.
> > + * hmapx node data is 'struct ovn_port *' */
> > + struct hmapx created;
> > +
> > + /* tracked updated ports.
> > + * hmapx node data is 'struct ovn_port *' */
> > + struct hmapx updated;
> > +
> > + /* tracked deleted ports.
> > + * hmapx node data is 'struct ovn_port *' */
> > + struct hmapx deleted;
> > };
>
> In general I would use lists instead of hmap if there is no requirement for
> searching. In the discussion of v2 you mentioned using hmapx to avoid
> adding list_node member in the linked objects. However, with lists it can
> be done without touching the target objects by creating separate list_node
> objects with a pointer pointing to the target object, similar to what hmapx
> does. Does this make sense?
>
> That being said, I don't have a very strong opinion, if it takes too much
> effort to change.
>
> Other than this, this patch looks good to me:
>
> Acked-by: Han Zhou <hz...@ovn.org>
Thanks for the reviews.
For now I went ahead and applied this patch addressing the review
comments from Dumitru.
Regarding the hmapx, I kept it the same for 2 reasons:
1. It is a bit of an effort to update the whole patch series.
2. The code here
https://github.com/ovn-org/ovn/blob/main/northd/northd.c#L5516 adds
the 'op' to the tracked
data after looping through the changed ports. If we use
list, then we need to first search the list before adding the 'op'
to make sure we don't have duplicate entries for the same
'op' in the tracked list. With hmapx it comes for free.
I applied this patch 1 and patch 2 to make my job easier for the next
version :).
Thanks
Numan
>
> >
> > -/* 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 tracked_lbs {
> > + /* Tracked created or updated load balancers.
> > + * hmapx node data is 'struct ovn_lb_datapaths' */
> > + struct hmapx crupdated;
> > +
> > + /* Tracked deleted lbs.
> > + * hmapx node data is 'struct ovn_lb_datapaths' */
> > + struct hmapx deleted;
> > +};
> > +
> > +enum northd_tracked_data_type {
> > + NORTHD_TRACKED_NONE,
> > + NORTHD_TRACKED_PORTS = (1 << 0),
> > + NORTHD_TRACKED_LBS = (1 << 1),
> > +};
> > +
> > +/* Track what's changed in the northd engine node.
> > + * Now only tracks ovn_ports (of vif type) - created, updated
> > + * and deleted. */
> > +struct northd_tracked_data {
> > + /* Indicates the type of data tracked. One or all of
> NORTHD_TRACKED_*. */
> > + enum northd_tracked_data_type type;
> > + struct tracked_ovn_ports trk_lsps;
> > + struct tracked_lbs trk_lbs;
> > };
> >
> > struct northd_data {
> > @@ -114,10 +136,9 @@ struct northd_data {
> > struct chassis_features features;
> > struct sset svc_monitor_lsps;
> > struct hmap svc_monitor_map;
> > - bool change_tracked;
> > - struct tracked_ls_changes tracked_ls_changes;
> > - bool lb_changed; /* Indicates if load balancers changed or
> association of
> > - * load balancer to logical switch/router changed.
> */
> > +
> > + /* Change tracking data. */
> > + struct northd_tracked_data trk_data;
> > };
> >
> > struct lflow_data {
> > @@ -338,9 +359,10 @@ void northd_indices_create(struct northd_data *data,
> > void build_lflows(struct ovsdb_idl_txn *ovnsb_txn,
> > struct lflow_input *input_data,
> > struct hmap *lflows);
> > -bool lflow_handle_northd_ls_changes(struct ovsdb_idl_txn *ovnsb_txn,
> > - struct tracked_ls_changes *,
> > - struct lflow_input *, struct hmap
> *lflows);
> > +bool lflow_handle_northd_port_changes(struct ovsdb_idl_txn *ovnsb_txn,
> > + struct tracked_ovn_ports *,
> > + struct lflow_input *,
> > + struct hmap *lflows);
> > bool northd_handle_sb_port_binding_changes(
> > const struct sbrec_port_binding_table *, struct hmap *ls_ports);
> >
> > @@ -349,7 +371,8 @@ bool northd_handle_lb_data_changes(struct
> tracked_lb_data *,
> > struct ovn_datapaths *ls_datapaths,
> > struct ovn_datapaths *lr_datapaths,
> > struct hmap *lb_datapaths_map,
> > - struct hmap *lbgrp_datapaths_map);
> > + struct hmap *lbgrp_datapaths_map,
> > + struct northd_tracked_data *);
> >
> > void build_bfd_table(struct ovsdb_idl_txn *ovnsb_txn,
> > const struct nbrec_bfd_table *,
> > @@ -372,6 +395,23 @@ void sync_lbs(struct ovsdb_idl_txn *, const struct
> sbrec_load_balancer_table *,
> > bool check_sb_lb_duplicates(const struct sbrec_load_balancer_table *);
> >
> > void sync_pbs(struct ovsdb_idl_txn *, struct hmap *ls_ports);
> > -bool sync_pbs_for_northd_ls_changes(struct tracked_ls_changes *);
> > +bool sync_pbs_for_northd_changed_ovn_ports( struct tracked_ovn_ports *);
> > +
> > +static inline bool
> > +northd_has_tracked_data(struct northd_tracked_data *trk_nd_changes) {
> > + return trk_nd_changes->type != NORTHD_TRACKED_NONE;
> > +}
> > +
> > +static inline bool
> > +northd_has_lbs_in_tracked_data(struct northd_tracked_data
> *trk_nd_changes)
> > +{
> > + return (trk_nd_changes->type & NORTHD_TRACKED_LBS);
> > +}
> > +
> > +static inline bool
> > +northd_has_lsps_in_tracked_data(struct northd_tracked_data
> *trk_nd_changes)
> > +{
> > + return (trk_nd_changes->type & NORTHD_TRACKED_PORTS);
> > +}
> >
> > #endif /* NORTHD_H */
> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > index 9a0d418e46..7bcd113d8f 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -10644,9 +10644,8 @@ check as northd ovn-appctl -t ovn-northd
> inc-engine/clear-stats
> > lbg1_uuid=$(ovn-nbctl --wait=sb create load_balancer_group name=lbg1)
> > check_engine_stats lb_data norecompute compute
> > check_engine_stats northd norecompute compute
> > -check_engine_stats lflow recompute nocompute
> > -check_engine_stats sync_to_sb_lb recompute nocompute
> > -
> > +check_engine_stats lflow norecompute nocompute
> > +check_engine_stats sync_to_sb_lb norecompute nocompute
> > CHECK_NO_CHANGE_AFTER_RECOMPUTE
> > check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >
> > @@ -10674,7 +10673,6 @@ check_engine_stats lb_data norecompute compute
> > check_engine_stats northd norecompute compute
> > check_engine_stats lflow recompute nocompute
> > check_engine_stats sync_to_sb_lb recompute nocompute
> > -
> > CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
> > check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> > @@ -10822,8 +10820,8 @@ check as northd ovn-appctl -t ovn-northd
> inc-engine/clear-stats
> > lbg1_uuid=$(ovn-nbctl --wait=sb create load_balancer_group name=lbg1)
> > check_engine_stats lb_data norecompute compute
> > check_engine_stats northd norecompute compute
> > -check_engine_stats lflow recompute nocompute
> > -check_engine_stats sync_to_sb_lb recompute nocompute
> > +check_engine_stats lflow norecompute nocompute
> > +check_engine_stats sync_to_sb_lb norecompute nocompute
> >
> > check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> > check ovn-nbctl --wait=sb set load_balancer_group .
> load_balancer="$lb2_uuid,$lb3_uuid,$lb4_uuid"
> > --
> > 2.43.0
> >
> >
> > _______________________________________________
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
