Message ID | 20200608135030.1379174-1-numans@ovn.org |
---|---|
State | Superseded |
Headers | show |
Series | Incremental processing improvements. | expand |
On 6/8/20 3:50 PM, numans@ovn.org wrote: > From: Numan Siddique <numans@ovn.org> > > In order to handle runtime data changes incrementally, the flow outut > runtime data handle should know the changed runtime data. > Runtime data now tracks the changed data for any OVS interface > and SB port binding changes. The tracked data contains a hmap > of tracked datapaths (which changed during runtime data processing. > > The flow outout runtime_data handler in this patch doesn't do much > with the tracked data. It returns false if there is tracked data available > so that flow_output run is called. If no tracked data is available > then there is no need for flow computation and the handler returns true. > > Next patch in the series processes the tracked data incrementally. > > Co-Authored-by: Venkata Anil <anilvenkata@redhat.com> > Signed-off-by: Venkata Anil <anilvenkata@redhat.com> > Signed-off-by: Numan Siddique <numans@ovn.org> > --- > controller/binding.c | 299 ++++++++++++++++++++++++++++-------- > controller/binding.h | 37 +++++ > controller/ovn-controller.c | 144 +++++++++++++++-- > tests/ovn-performance.at | 20 +-- > 4 files changed, 413 insertions(+), 87 deletions(-) > > diff --git a/controller/binding.c b/controller/binding.c > index c2d9a4c6b..3c226cd7d 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -69,13 +69,26 @@ binding_register_ovs_idl(struct ovsdb_idl *ovs_idl) > ovsdb_idl_add_column(ovs_idl, &ovsrec_qos_col_type); > } > > +static struct tracked_binding_datapath *tracked_binding_datapath_create( > + const struct sbrec_datapath_binding *, > + bool is_new, struct hmap *tracked_dps); > +static struct tracked_binding_datapath *tracked_binding_datapath_find( > + struct hmap *, const struct sbrec_datapath_binding *); > +static void tracked_binding_datapath_lport_add( > + const struct sbrec_port_binding *, bool deleted, > + struct hmap *tracked_datapaths); > +static void update_lport_tracking(const struct sbrec_port_binding *pb, > + bool old_claim, bool new_claim, > + struct hmap *tracked_dp_bindings); > + > static void > add_local_datapath__(struct ovsdb_idl_index *sbrec_datapath_binding_by_key, > struct ovsdb_idl_index *sbrec_port_binding_by_datapath, > struct ovsdb_idl_index *sbrec_port_binding_by_name, > const struct sbrec_datapath_binding *datapath, > bool has_local_l3gateway, int depth, > - struct hmap *local_datapaths) > + struct hmap *local_datapaths, > + struct hmap *tracked_datapaths) > { > uint32_t dp_key = datapath->tunnel_key; > struct local_datapath *ld = get_local_datapath(local_datapaths, dp_key); > @@ -92,6 +105,11 @@ add_local_datapath__(struct ovsdb_idl_index *sbrec_datapath_binding_by_key, > ld->localnet_port = NULL; > ld->has_local_l3gateway = has_local_l3gateway; > > + if (tracked_datapaths && > + !tracked_binding_datapath_find(tracked_datapaths, datapath)) { > + tracked_binding_datapath_create(datapath, true, tracked_datapaths); > + } > + > if (depth >= 100) { > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > VLOG_WARN_RL(&rl, "datapaths nested too deep"); > @@ -124,7 +142,8 @@ add_local_datapath__(struct ovsdb_idl_index *sbrec_datapath_binding_by_key, > sbrec_port_binding_by_datapath, > sbrec_port_binding_by_name, > peer->datapath, false, > - depth + 1, local_datapaths); > + depth + 1, local_datapaths, > + tracked_datapaths); > } > ld->n_peer_ports++; > if (ld->n_peer_ports > ld->n_allocated_peer_ports) { > @@ -147,12 +166,14 @@ add_local_datapath(struct ovsdb_idl_index *sbrec_datapath_binding_by_key, > struct ovsdb_idl_index *sbrec_port_binding_by_datapath, > struct ovsdb_idl_index *sbrec_port_binding_by_name, > const struct sbrec_datapath_binding *datapath, > - bool has_local_l3gateway, struct hmap *local_datapaths) > + bool has_local_l3gateway, struct hmap *local_datapaths, > + struct hmap *tracked_datapaths) > { > add_local_datapath__(sbrec_datapath_binding_by_key, > sbrec_port_binding_by_datapath, > sbrec_port_binding_by_name, > - datapath, has_local_l3gateway, 0, local_datapaths); > + datapath, has_local_l3gateway, 0, local_datapaths, > + tracked_datapaths); > } > > static void > @@ -473,23 +494,45 @@ update_ld_localnet_port(const struct sbrec_port_binding *binding_rec, > > static void > update_local_lport_ids(struct sset *local_lport_ids, > - const struct sbrec_port_binding *pb) > + const struct sbrec_port_binding *pb, > + struct hmap *tracked_datapaths) > { > char buf[16]; > snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64, > pb->datapath->tunnel_key, pb->tunnel_key); > - sset_add(local_lport_ids, buf); > + bool added = sset_add(local_lport_ids, buf); Hi Numan, I guess this should be: bool added = !!sset_add(local_lport_ids, buf); > + if (added && tracked_datapaths) { > + /* Add the 'pb' to the tracked_datapaths. */ > + tracked_binding_datapath_lport_add(pb, false, tracked_datapaths); > + } > } > > static void > -remove_local_lport_ids(const struct sbrec_port_binding *binding_rec, > - struct sset *local_lport_ids) > +remove_local_lport_ids(const struct sbrec_port_binding *pb, > + struct sset *local_lport_ids, > + struct hmap *tracked_datapaths) > { > char buf[16]; > snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64, > - binding_rec->datapath->tunnel_key, > - binding_rec->tunnel_key); > - sset_find_and_delete(local_lport_ids, buf); > + pb->datapath->tunnel_key, pb->tunnel_key); > + bool deleted = sset_find_and_delete(local_lport_ids, buf); > + if (deleted && tracked_datapaths) { > + /* Add the 'pb' to the tracked_datapaths. */ > + tracked_binding_datapath_lport_add(pb, true, tracked_datapaths); > + } > +} > + > +static bool > +add_lport_to_local_lports(struct sset *local_lports, const char *lport_name) > +{ > + return !!sset_add(local_lports, lport_name); > +} > + > +static bool > +remove_lport_from_local_lports(struct sset *local_lports, > + const char *lport_name) > +{ > + return sset_find_and_delete(local_lports, lport_name); > } > > /* Local bindings. binding.c module binds the logical port (represented by > @@ -637,6 +680,77 @@ is_lport_container(const struct sbrec_port_binding *pb) > return is_lport_vif(pb) && pb->parent_port && pb->parent_port[0]; > } > > +static struct tracked_binding_datapath * > +tracked_binding_datapath_create(const struct sbrec_datapath_binding *dp, > + bool is_new, > + struct hmap *tracked_datapaths) > +{ > + struct tracked_binding_datapath *t_dp = xzalloc(sizeof *t_dp); > + t_dp->dp = dp; > + t_dp->is_new = is_new; > + shash_init(&t_dp->lports); > + hmap_insert(tracked_datapaths, &t_dp->node, uuid_hash(&dp->header_.uuid)); > + return t_dp; > +} > + > +static struct tracked_binding_datapath * > +tracked_binding_datapath_find(struct hmap *tracked_datapaths, > + const struct sbrec_datapath_binding *dp) > +{ > + struct tracked_binding_datapath *t_dp; > + size_t hash = uuid_hash(&dp->header_.uuid); > + HMAP_FOR_EACH_WITH_HASH (t_dp, node, hash, tracked_datapaths) { > + if (uuid_equals(&t_dp->dp->header_.uuid, &dp->header_.uuid)) { > + return t_dp; > + } > + } > + > + return NULL; > +} > + > +static void > +tracked_binding_datapath_lport_add(const struct sbrec_port_binding *pb, > + bool deleted, > + struct hmap *tracked_datapaths) > +{ > + if (!tracked_datapaths) { > + return; > + } > + > + struct tracked_binding_datapath *tracked_dp = > + tracked_binding_datapath_find(tracked_datapaths, pb->datapath); > + if (!tracked_dp) { > + tracked_dp = tracked_binding_datapath_create(pb->datapath, false, > + tracked_datapaths); > + } > + > + /* Check if the lport is already present or not. > + * If it is already present, then just update the 'pb' and 'deleted' > + * fields. */ > + struct tracked_binding_lport *lport = > + shash_find_data(&tracked_dp->lports, pb->logical_port); > + > + if (!lport) { > + lport = xmalloc(sizeof *lport); > + shash_add(&tracked_dp->lports, pb->logical_port, lport); > + } > + > + lport->pb = pb; > + lport->deleted = deleted; > +} > + > +void > +binding_tracked_dp_destroy(struct hmap *tracked_datapaths) > +{ > + struct tracked_binding_datapath *t_dp; > + HMAP_FOR_EACH_POP (t_dp, node, tracked_datapaths) { > + shash_destroy_free_data(&t_dp->lports); > + free(t_dp); > + } > + > + hmap_destroy(tracked_datapaths); > +} > + > /* Corresponds to each Port_Binding.type. */ > enum en_lport_type { > LP_UNKNOWN, > @@ -690,7 +804,7 @@ static bool > claim_lport(const struct sbrec_port_binding *pb, > const struct sbrec_chassis *chassis_rec, > const struct ovsrec_interface *iface_rec, > - bool sb_readonly) > + bool sb_readonly, struct hmap *tracked_datapaths) > { > if (pb->chassis != chassis_rec) { > if (sb_readonly) { > @@ -709,6 +823,10 @@ claim_lport(const struct sbrec_port_binding *pb, > } > > sbrec_port_binding_set_chassis(pb, chassis_rec); > + > + if (tracked_datapaths) { > + update_lport_tracking(pb, false, true, tracked_datapaths); > + } > } > > /* Check if the port encap binding, if any, has changed */ > @@ -726,9 +844,14 @@ claim_lport(const struct sbrec_port_binding *pb, > > /* Returns false if lport is not released due to 'sb_readonly'. > * Returns true otherwise. > + * > + * This function assumes that that the the 'pb' was claimed > + * earlier i.e port binding's chassis is set to this chassis. > + * Caller should make sure that, that's the case. Can we skip "that, " above? (non-native speaker here). > */ > static bool > -release_lport(const struct sbrec_port_binding *pb, bool sb_readonly) > +release_lport(const struct sbrec_port_binding *pb, bool sb_readonly, > + struct hmap *tracked_datapaths) > { > if (pb->encap) { > if (sb_readonly) { > @@ -751,6 +874,7 @@ release_lport(const struct sbrec_port_binding *pb, bool sb_readonly) > sbrec_port_binding_set_virtual_parent(pb, NULL); > } > > + update_lport_tracking(pb, true, false, tracked_datapaths); > VLOG_INFO("Releasing lport %s from this chassis.", pb->logical_port); > return true; > } > @@ -796,13 +920,14 @@ is_lbinding_container_parent(struct local_binding *lbinding) > static bool > release_local_binding_children(const struct sbrec_chassis *chassis_rec, > struct local_binding *lbinding, > - bool sb_readonly) > + bool sb_readonly, > + struct hmap *tracked_dp_bindings) > { > struct shash_node *node; > SHASH_FOR_EACH (node, &lbinding->children) { > struct local_binding *l = node->data; > if (is_lbinding_this_chassis(l, chassis_rec)) { > - if (!release_lport(l->pb, sb_readonly)) { > + if (!release_lport(l->pb, sb_readonly, tracked_dp_bindings)) { > return false; > } > } > @@ -817,16 +942,17 @@ release_local_binding_children(const struct sbrec_chassis *chassis_rec, > > static bool > release_local_binding(const struct sbrec_chassis *chassis_rec, > - struct local_binding *lbinding, bool sb_readonly) > + struct local_binding *lbinding, bool sb_readonly, > + struct hmap *tracked_dp_bindings) > { > if (!release_local_binding_children(chassis_rec, lbinding, > - sb_readonly)) { > + sb_readonly, tracked_dp_bindings)) { > return false; > } > > bool retval = true; > if (is_lbinding_this_chassis(lbinding, chassis_rec)) { > - retval = release_lport(lbinding->pb, sb_readonly); > + retval = release_lport(lbinding->pb, sb_readonly, tracked_dp_bindings); > } > > lbinding->pb = NULL; > @@ -847,7 +973,8 @@ consider_vif_lport_(const struct sbrec_port_binding *pb, > if (can_bind) { > /* We can claim the lport. */ > if (!claim_lport(pb, b_ctx_in->chassis_rec, lbinding->iface, > - !b_ctx_in->ovnsb_idl_txn)){ > + !b_ctx_in->ovnsb_idl_txn, > + b_ctx_out->tracked_dp_bindings)){ > return false; > } > > @@ -855,8 +982,10 @@ consider_vif_lport_(const struct sbrec_port_binding *pb, > b_ctx_in->sbrec_port_binding_by_datapath, > b_ctx_in->sbrec_port_binding_by_name, > pb->datapath, false, > - b_ctx_out->local_datapaths); > - update_local_lport_ids(b_ctx_out->local_lport_ids, pb); > + b_ctx_out->local_datapaths, > + b_ctx_out->tracked_dp_bindings); > + update_local_lport_ids(b_ctx_out->local_lport_ids, pb, > + b_ctx_out->tracked_dp_bindings); > if (lbinding->iface && qos_map && b_ctx_in->ovs_idl_txn) { > get_qos_params(pb, qos_map); > } > @@ -875,7 +1004,8 @@ consider_vif_lport_(const struct sbrec_port_binding *pb, > if (pb->chassis == b_ctx_in->chassis_rec) { > /* Release the lport if there is no lbinding. */ > if (!lbinding_set || !can_bind) { > - return release_lport(pb, !b_ctx_in->ovnsb_idl_txn); > + return release_lport(pb, !b_ctx_in->ovnsb_idl_txn, > + b_ctx_out->tracked_dp_bindings); > } > } > > @@ -962,7 +1092,8 @@ consider_container_lport(const struct sbrec_port_binding *pb, > * release the container lport, if it was bound earlier. */ > if (is_lbinding_this_chassis(container_lbinding, > b_ctx_in->chassis_rec)) { > - return release_lport(pb, !b_ctx_in->ovnsb_idl_txn); > + return release_lport(pb, !b_ctx_in->ovnsb_idl_txn, > + b_ctx_out->tracked_dp_bindings); > } > > return true; > @@ -1037,18 +1168,22 @@ consider_nonvif_lport_(const struct sbrec_port_binding *pb, > struct binding_ctx_out *b_ctx_out) > { > if (our_chassis) { > - sset_add(b_ctx_out->local_lports, pb->logical_port); > + add_lport_to_local_lports(b_ctx_out->local_lports, pb->logical_port); > add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key, > b_ctx_in->sbrec_port_binding_by_datapath, > b_ctx_in->sbrec_port_binding_by_name, > pb->datapath, has_local_l3gateway, > - b_ctx_out->local_datapaths); > + b_ctx_out->local_datapaths, > + b_ctx_out->tracked_dp_bindings); > > - update_local_lport_ids(b_ctx_out->local_lport_ids, pb); > + update_local_lport_ids(b_ctx_out->local_lport_ids, pb, > + b_ctx_out->tracked_dp_bindings); > return claim_lport(pb, b_ctx_in->chassis_rec, NULL, > - !b_ctx_in->ovnsb_idl_txn); > + !b_ctx_in->ovnsb_idl_txn, > + b_ctx_out->tracked_dp_bindings); > } else if (pb->chassis == b_ctx_in->chassis_rec) { > - return release_lport(pb, !b_ctx_in->ovnsb_idl_txn); > + return release_lport(pb, !b_ctx_in->ovnsb_idl_txn, > + b_ctx_out->tracked_dp_bindings); > } > > return true; > @@ -1084,14 +1219,15 @@ consider_localnet_lport(const struct sbrec_port_binding *pb, > struct binding_ctx_out *b_ctx_out, > struct hmap *qos_map) > { > - /* Add all localnet ports to local_lports so that we allocate ct zones > + /* Add all localnet ports to local_ifaces so that we allocate ct zones > * for them. */ > - sset_add(b_ctx_out->local_lports, pb->logical_port); > + add_lport_to_local_lports(b_ctx_out->local_lports, pb->logical_port); > if (qos_map && b_ctx_in->ovs_idl_txn) { > get_qos_params(pb, qos_map); > } > > - update_local_lport_ids(b_ctx_out->local_lport_ids, pb); > + update_local_lport_ids(b_ctx_out->local_lport_ids, pb, > + b_ctx_out->tracked_dp_bindings); > } > > static bool > @@ -1119,7 +1255,10 @@ consider_ha_lport(const struct sbrec_port_binding *pb, > b_ctx_in->sbrec_port_binding_by_datapath, > b_ctx_in->sbrec_port_binding_by_name, > pb->datapath, false, > - b_ctx_out->local_datapaths); > + b_ctx_out->local_datapaths, > + b_ctx_out->tracked_dp_bindings); > + update_local_lport_ids(b_ctx_out->local_lport_ids, pb, > + b_ctx_out->tracked_dp_bindings); > } > > return consider_nonvif_lport_(pb, our_chassis, false, b_ctx_in, b_ctx_out); > @@ -1185,7 +1324,7 @@ build_local_bindings(struct binding_ctx_in *b_ctx_in, > ovs_assert(lbinding->type == BT_VIF); > } > > - sset_add(b_ctx_out->local_lports, iface_id); > + add_lport_to_local_lports(b_ctx_out->local_lports, iface_id); > smap_replace(b_ctx_out->local_iface_ids, iface_rec->name, > iface_id); > } > @@ -1241,7 +1380,7 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) > case LP_PATCH: > case LP_LOCALPORT: > case LP_VTEP: > - update_local_lport_ids(b_ctx_out->local_lport_ids, pb); > + update_local_lport_ids(b_ctx_out->local_lport_ids, pb, NULL); > break; > > case LP_VIF: > @@ -1409,7 +1548,8 @@ add_local_datapath_peer_port(const struct sbrec_port_binding *pb, > b_ctx_in->sbrec_port_binding_by_datapath, > b_ctx_in->sbrec_port_binding_by_name, > peer->datapath, false, > - 1, b_ctx_out->local_datapaths); > + 1, b_ctx_out->local_datapaths, > + b_ctx_out->tracked_dp_bindings); > return; > } > > @@ -1471,7 +1611,8 @@ remove_pb_from_local_datapath(const struct sbrec_port_binding *pb, > struct binding_ctx_out *b_ctx_out, > struct local_datapath *ld) > { > - remove_local_lport_ids(pb, b_ctx_out->local_lport_ids); > + remove_local_lport_ids(pb, b_ctx_out->local_lport_ids, > + b_ctx_out->tracked_dp_bindings); > if (!strcmp(pb->type, "patch") || > !strcmp(pb->type, "l3gateway")) { > remove_local_datapath_peer_port(pb, ld, b_ctx_out->local_datapaths); > @@ -1489,6 +1630,18 @@ remove_pb_from_local_datapath(const struct sbrec_port_binding *pb, > } > } > > +static void > +update_lport_tracking(const struct sbrec_port_binding *pb, > + bool old_claim, bool new_claim, > + struct hmap *tracked_dp_bindings) > +{ > + if (!tracked_dp_bindings || !pb || (old_claim == new_claim)) { I think pb can never be NULL here, right? > + return; > + } > + > + tracked_binding_datapath_lport_add(pb, old_claim, tracked_dp_bindings); > +} > + > /* Considers the ovs iface 'iface_rec' for claiming. > * This function should be called if the external_ids:iface-id > * and 'ofport' are set for the 'iface_rec'. > @@ -1501,9 +1654,13 @@ consider_iface_claim(const struct ovsrec_interface *iface_rec, > const char *iface_id, > struct binding_ctx_in *b_ctx_in, > struct binding_ctx_out *b_ctx_out, > - struct hmap *qos_map) > + struct hmap *qos_map, > + bool *local_lports_changed) > { > - sset_add(b_ctx_out->local_lports, iface_id); > + if (add_lport_to_local_lports(b_ctx_out->local_lports, iface_id)) { > + *local_lports_changed = true; > + } > + > smap_replace(b_ctx_out->local_iface_ids, iface_rec->name, iface_id); > > struct local_binding *lbinding = > @@ -1566,7 +1723,8 @@ static bool > consider_iface_release(const struct ovsrec_interface *iface_rec, > const char *iface_id, > struct binding_ctx_in *b_ctx_in, > - struct binding_ctx_out *b_ctx_out, bool *changed) > + struct binding_ctx_out *b_ctx_out, bool *changed, > + bool *local_lports_changed) > { > struct local_binding *lbinding; > lbinding = local_binding_find(b_ctx_out->local_bindings, > @@ -1585,7 +1743,8 @@ consider_iface_release(const struct ovsrec_interface *iface_rec, > * lbinding->iface. > * Cannot access these members of lbinding after this call. */ > if (!release_local_binding(b_ctx_in->chassis_rec, lbinding, > - !b_ctx_in->ovnsb_idl_txn)) { > + !b_ctx_in->ovnsb_idl_txn, > + b_ctx_out->tracked_dp_bindings)) { > return false; > } > > @@ -1598,7 +1757,9 @@ consider_iface_release(const struct ovsrec_interface *iface_rec, > local_binding_delete(b_ctx_out->local_bindings, lbinding); > } > > - sset_find_and_delete(b_ctx_out->local_lports, iface_id); > + if (remove_lport_from_local_lports(b_ctx_out->local_lports, iface_id)) { > + *local_lports_changed = true; > + } > smap_remove(b_ctx_out->local_iface_ids, iface_rec->name); > > return true; > @@ -1630,6 +1791,8 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, > bool handled = true; > *changed = false; > > + *b_ctx_out->local_lports_changed = false; > + Is this needed? This is part of the data we clear at every run in en_runtime_data_clear_tracked_data(). > /* Run the tracked interfaces loop twice. One to handle deleted > * changes. And another to handle add/update changes. > * This will ensure correctness. > @@ -1685,7 +1848,8 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, > > if (cleared_iface_id) { > handled = consider_iface_release(iface_rec, cleared_iface_id, > - b_ctx_in, b_ctx_out, changed); > + b_ctx_in, b_ctx_out, changed, > + b_ctx_out->local_lports_changed); > } > > if (!handled) { > @@ -1727,7 +1891,8 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, > if (iface_id && ofport > 0) { > *changed = true; > handled = consider_iface_claim(iface_rec, iface_id, b_ctx_in, > - b_ctx_out, qos_map_ptr); > + b_ctx_out, qos_map_ptr, > + b_ctx_out->local_lports_changed); > if (!handled) { > break; > } > @@ -1792,8 +1957,7 @@ static bool > handle_deleted_vif_lport(const struct sbrec_port_binding *pb, > enum en_lport_type lport_type, > struct binding_ctx_in *b_ctx_in, > - struct binding_ctx_out *b_ctx_out, > - bool *changed) > + struct binding_ctx_out *b_ctx_out) > { > struct local_binding *lbinding = > get_lbinding_for_lport(pb, lport_type, b_ctx_out); > @@ -1804,13 +1968,12 @@ handle_deleted_vif_lport(const struct sbrec_port_binding *pb, > * clear the 'chassis' column of 'pb'. But we need to do > * for the local_binding's children. */ > if (lbinding->type == BT_VIF && > - !release_local_binding_children(b_ctx_in->chassis_rec, > - lbinding, > - !b_ctx_in->ovnsb_idl_txn)) { > + !release_local_binding_children( > + b_ctx_in->chassis_rec, lbinding, > + !b_ctx_in->ovnsb_idl_txn, > + b_ctx_out->tracked_dp_bindings)) { > return false; > } > - > - *changed = true; > } > > handle_deleted_lport(pb, b_ctx_in, b_ctx_out); > @@ -1822,8 +1985,7 @@ handle_updated_vif_lport(const struct sbrec_port_binding *pb, > enum en_lport_type lport_type, > struct binding_ctx_in *b_ctx_in, > struct binding_ctx_out *b_ctx_out, > - struct hmap *qos_map, > - bool *changed) > + struct hmap *qos_map) > { > bool claimed = (pb->chassis == b_ctx_in->chassis_rec); > bool handled = true; > @@ -1841,14 +2003,10 @@ handle_updated_vif_lport(const struct sbrec_port_binding *pb, > } > > bool now_claimed = (pb->chassis == b_ctx_in->chassis_rec); > - bool changed_ = (claimed != now_claimed); > - > - if (changed_) { > - *changed = true; > - } > + bool changed = (claimed != now_claimed); > > if (lport_type == LP_VIRTUAL || > - (lport_type == LP_VIF && is_lport_container(pb)) || !changed_) { > + (lport_type == LP_VIF && is_lport_container(pb)) || !changed) { > return true; > } > > @@ -1898,7 +2056,7 @@ binding_handle_port_binding_changes(struct binding_ctx_in *b_ctx_in, > enum en_lport_type lport_type = get_lport_type(pb); > if (lport_type == LP_VIF || lport_type == LP_VIRTUAL) { > handled = handle_deleted_vif_lport(pb, lport_type, b_ctx_in, > - b_ctx_out, changed); > + b_ctx_out); > } else { > handle_deleted_lport(pb, b_ctx_in, b_ctx_out); > } > @@ -1933,15 +2091,14 @@ binding_handle_port_binding_changes(struct binding_ctx_in *b_ctx_in, > case LP_VIF: > case LP_VIRTUAL: > handled = handle_updated_vif_lport(pb, lport_type, b_ctx_in, > - b_ctx_out, qos_map_ptr, > - changed); > + b_ctx_out, qos_map_ptr); > break; > > case LP_PATCH: > case LP_LOCALPORT: > case LP_VTEP: > - *changed = true; > - update_local_lport_ids(b_ctx_out->local_lport_ids, pb); > + update_local_lport_ids(b_ctx_out->local_lport_ids, pb, > + b_ctx_out->tracked_dp_bindings); > if (lport_type == LP_PATCH) { > /* Add the peer datapath to the local datapaths if it's > * not present yet. > @@ -1950,30 +2107,32 @@ binding_handle_port_binding_changes(struct binding_ctx_in *b_ctx_in, > add_local_datapath_peer_port(pb, b_ctx_in, b_ctx_out, ld); > } > } > + > + if (lport_type == LP_VTEP) { > + /* VTEP lports are claimed/released by ovn-controller-vteps. > + * We are not sure what changed. So set *changed to true > + * for any changes to vtep lports. */ > + *changed = true; > + } > break; > > case LP_L2GATEWAY: > - *changed = true; > handled = consider_l2gw_lport(pb, b_ctx_in, b_ctx_out); > break; > > case LP_L3GATEWAY: > - *changed = true; > handled = consider_l3gw_lport(pb, b_ctx_in, b_ctx_out); > break; > > case LP_CHASSISREDIRECT: > - *changed = true; > handled = consider_cr_lport(pb, b_ctx_in, b_ctx_out); > break; > > case LP_EXTERNAL: > - *changed = true; > handled = consider_external_lport(pb, b_ctx_in, b_ctx_out); > break; > > case LP_LOCALNET: { > - *changed = true; > consider_localnet_lport(pb, b_ctx_in, b_ctx_out, qos_map_ptr); > > struct shash bridge_mappings = > @@ -2008,6 +2167,10 @@ binding_handle_port_binding_changes(struct binding_ctx_in *b_ctx_in, > } > } > > + if (handled && !hmap_is_empty(b_ctx_out->tracked_dp_bindings)) { > + *changed = true; > + } > + > destroy_qos_map(&qos_map); > return handled; > } > diff --git a/controller/binding.h b/controller/binding.h > index 21118ecd4..2974ebb30 100644 > --- a/controller/binding.h > +++ b/controller/binding.h > @@ -19,6 +19,9 @@ > > #include <stdbool.h> > #include "openvswitch/shash.h" > +#include "openvswitch/hmap.h" > +#include "openvswitch/uuid.h" > +#include "openvswitch/list.h" > > struct hmap; > struct ovsdb_idl; > @@ -54,10 +57,28 @@ struct binding_ctx_in { > struct binding_ctx_out { > struct hmap *local_datapaths; > struct shash *local_bindings; > + > struct sset *local_lports; > + > + /* If the sset local_lports is modified, this is set to true by > + * the callee. */ > + bool *local_lports_changed; > + > + /* sset of local lport ids in the format > + * <datapath-tunnel-key>_<port-tunnel-key>. */ Nit: shouldn't this comment be part of patch 1? > struct sset *local_lport_ids; > + > struct sset *egress_ifaces; > + > + /* smap of OVS interface name as key and > + * OVS interface external_ids:iface-id as value. */ Nit: shouldn't this comment be part of patch 2? > struct smap *local_iface_ids; > + > + /* hmap of 'struct tracked_binding_datapath' which the > + * callee (binding_handle_ovs_interface_changes and > + * binding_handle_port_binding_changes) fills in for > + * the changed datapaths and port bindings. */ > + struct hmap *tracked_dp_bindings; > }; > > enum local_binding_type { > @@ -82,6 +103,21 @@ local_binding_find(struct shash *local_bindings, const char *name) > return shash_find_data(local_bindings, name); > } > > +/* Represents a tracked binding logical port. */ > +struct tracked_binding_lport { > + const struct sbrec_port_binding *pb; > + struct ovs_list list_node; Nit: for consistency with tracked_binding_datapath and because list_node is usually accessed before pb I'd define list_node before pb. > + bool deleted; > +}; > + > +/* Represent a tracked binding datapath. */ > +struct tracked_binding_datapath { > + struct hmap_node node; > + const struct sbrec_datapath_binding *dp; > + bool is_new; I think it would be easier to read the code if both tracked_binding_lport and tracked_binding_datapath would use the same logic for tracking updates: either both should have "deleted" or both should have "is_new". Also, given that we never use tracked_binding_lport.deleted, maybe we can skip it completely. What do you think? > + struct shash lports; /* shash of struct tracked_binding_lport. */ > +}; > + > void binding_register_ovs_idl(struct ovsdb_idl *); > void binding_run(struct binding_ctx_in *, struct binding_ctx_out *); > bool binding_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn, > @@ -96,4 +132,5 @@ bool binding_handle_ovs_interface_changes(struct binding_ctx_in *, > bool binding_handle_port_binding_changes(struct binding_ctx_in *, > struct binding_ctx_out *, > bool *changed); > +void binding_tracked_dp_destroy(struct hmap *tracked_datapaths); > #endif /* controller/binding.h */ > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 795a1c297..98652866c 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -973,10 +973,88 @@ struct ed_type_runtime_data { > struct sset local_lport_ids; > struct sset active_tunnels; > > + /* runtime data engine private data. */ > struct sset egress_ifaces; > struct smap local_iface_ids; > + > + /* Tracked data. See below for more details and comments. */ > + bool tracked; I wonder if the "tracked" field should actually be part of the generic engine_node_input. We use it for port groups and address sets too ("change_tracked" in ed_type_addr_sets and ed_type_port_groups) and afaiu the semantics are always: - "tracked" is set to true if a node's input handler was called - "tracked" is reset to false if a node's run handler was called So all this could be implemented generically in inc-proc-eng.c. What do you think? > + bool local_lports_changed; Do we really need local_lports_changed? Isn't it actually equivalent to !hmap_is_empty(& > + struct hmap tracked_dp_bindings; > }; > > +/* struct ed_type_runtime_data has the below members for tracking the > + * changes done to the runtime_data engine by the runtime_data engine > + * handlers. Since this engine is an input to the flow_output engine, > + * the flow output runtime data handler will make use of this tracked data. > + * > + * ------------------------------------------------------------------------ > + * | | This is a hmap of | > + * | | 'struct tracked_binding_datapath' defined in | > + * | | binding.h. Runtime data handlers for OVS | > + * | | Interface and Port Binding changes store the | > + * | @tracked_dp_bindings | changed datapaths (datapaths added/removed from | > + * | | local_datapaths) and changed port bindings | > + * | | (added/updated/deleted in 'local_bindings'). | This is not really accurate, right? Afaiu we only track if a port binding is added/deleted. > + * | | So any changes to the runtime data - | > + * | | local_datapaths and local_bindings is captured | > + * | | here. | > + * ------------------------------------------------------------------------ > + * | | This is a bool which represents if the runtime | > + * | | data 'local_lports' changed by the runtime data | > + * | | handlers for OVS Interface and Port Binding | > + * | | changes. If 'local_lports' is updated and also | > + * | | results in any port binding updates, it is | > + * |@local_lports_changed | captured in the @tracked_dp_bindings. So there | > + * | | is no need to capture the changes in the | > + * | | local_lports. If @local_lports_changed is true | > + * | | but without anydata in the @tracked_dp_bindings,| > + * | | it means we needto only update the SB monitor | > + * | | clauses and there isno need for any flow | > + * | | (re)computations. | > + * ------------------------------------------------------------------------ > + * | | This represents if the data was tracked or not | > + * | | by the runtime data handlers during the engine | > + * | @tracked | run. If the runtime data recompute is | > + * | | triggered, it means there is no tracked data. | > + * ------------------------------------------------------------------------ > + * > + * > + * The changes to the following runtime_data variables are not tracked. > + * > + * --------------------------------------------------------------------- > + * | local_datapaths | The changes to these runtime data is captured in | > + * | local_bindings | the @tracked_dp_bindings indirectly and hence it | > + * | local_lport_ids | is not tracked explicitly. | > + * --------------------------------------------------------------------- > + * | local_iface_ids | This is used internally within the runtime data | > + * | egress_ifaces | engine (used only in binding.c) and hence there | > + * | | there is no need to track. | > + * --------------------------------------------------------------------- > + * | | Active tunnels is built in the | > + * | | bfd_calculate_active_tunnels() for the tunnel | > + * | | OVS interfaces. Any changes to non VIF OVS | > + * | | interfaces results in triggering the full | > + * | active_tunnels | recompute of runtime data engine and hence there | > + * | | the tracked data doesn't track it. When we | > + * | | support handling changes to non VIF OVS | > + * | | interfaces we need to track the changes to the | > + * | | active tunnels. | > + * --------------------------------------------------------------------- > + * > + */ > + > +static void > +en_runtime_data_clear_tracked_data(void *data_) > +{ > + struct ed_type_runtime_data *data = data_; > + > + binding_tracked_dp_destroy(&data->tracked_dp_bindings); > + hmap_init(&data->tracked_dp_bindings); Nit: just as we did for local_bindings_init()/local_bindings_destroy() I'd move the "hmap_init(&data->tracked_dp_bindings)" inside a new function called binding_tracked_dp_init(). > + data->local_lports_changed = false; > + data->tracked = false; > +} > + > static void * > en_runtime_data_init(struct engine_node *node OVS_UNUSED, > struct engine_arg *arg OVS_UNUSED) > @@ -990,6 +1068,10 @@ en_runtime_data_init(struct engine_node *node OVS_UNUSED, > sset_init(&data->egress_ifaces); > smap_init(&data->local_iface_ids); > local_bindings_init(&data->local_bindings); > + > + /* init the tracked data. */ > + hmap_init(&data->tracked_dp_bindings); > + > return data; > } > > @@ -1012,6 +1094,8 @@ en_runtime_data_cleanup(void *data) > } > hmap_destroy(&rt_data->local_datapaths); > local_bindings_destroy(&rt_data->local_bindings); > + > + en_runtime_data_clear_tracked_data(data); > } > > static void > @@ -1092,6 +1176,8 @@ init_binding_ctx(struct engine_node *node, > b_ctx_out->egress_ifaces = &rt_data->egress_ifaces; > b_ctx_out->local_bindings = &rt_data->local_bindings; > b_ctx_out->local_iface_ids = &rt_data->local_iface_ids; > + b_ctx_out->tracked_dp_bindings = NULL; > + b_ctx_out->local_lports_changed = NULL; > } > > static void > @@ -1103,6 +1189,13 @@ en_runtime_data_run(struct engine_node *node, void *data) > struct sset *local_lport_ids = &rt_data->local_lport_ids; > struct sset *active_tunnels = &rt_data->active_tunnels; > > + /* Clear the tracked data. Even though the tracked data > + * gets cleared in the beginning of engine_init_run(), > + * any of the runtime data handler might have set some tracked > + * data and later another runtime data handler might return false > + * resulting full recompute of runtime engine. */ > + en_runtime_data_clear_tracked_data(data); > + Could you please elaborate more on this? Why is it an issue if we keep the "tracked" data until the end of the run? Thanks, Dumitru > static bool first_run = true; > if (first_run) { > /* don't cleanup since there is no data yet */ > @@ -1158,6 +1251,9 @@ runtime_data_ovs_interface_handler(struct engine_node *node, void *data) > struct binding_ctx_in b_ctx_in; > struct binding_ctx_out b_ctx_out; > init_binding_ctx(node, rt_data, &b_ctx_in, &b_ctx_out); > + rt_data->tracked = true; > + b_ctx_out.tracked_dp_bindings = &rt_data->tracked_dp_bindings; > + b_ctx_out.local_lports_changed = &rt_data->local_lports_changed; > > bool changed = false; > if (!binding_handle_ovs_interface_changes(&b_ctx_in, &b_ctx_out, > @@ -1190,6 +1286,9 @@ runtime_data_sb_port_binding_handler(struct engine_node *node, void *data) > return false; > } > > + rt_data->tracked = true; > + b_ctx_out.tracked_dp_bindings = &rt_data->tracked_dp_bindings; > + > bool changed = false; > if (!binding_handle_port_binding_changes(&b_ctx_in, &b_ctx_out, > &changed)) { > @@ -1797,6 +1896,39 @@ flow_output_port_groups_handler(struct engine_node *node, void *data) > return _flow_output_resource_ref_handler(node, data, REF_TYPE_PORTGROUP); > } > > +static bool > +flow_output_runtime_data_handler(struct engine_node *node, > + void *data OVS_UNUSED) > +{ > + struct ed_type_runtime_data *rt_data = > + engine_get_input_data("runtime_data", node); > + > + /* There is no tracked data. Fall back to full recompute of > + * flow_output. */ > + if (!rt_data->tracked) { > + return false; > + } > + > + if (!hmap_is_empty(&rt_data->tracked_dp_bindings)) { > + /* We are not yet handling the tracked datapath binding > + * changes. Return false to trigger full recompute. */ > + return false; > + } > + > + if (rt_data->local_lports_changed) { > + engine_set_node_state(node, EN_UPDATED); > + } > + > + return true; > +} > + > +static bool > +flow_output_noop_handler(struct engine_node *node OVS_UNUSED, > + void *data OVS_UNUSED) > +{ > + return true; > +} > + > /* Engine node en_physical_flow_changes indicates whether > * there is a need to > * - recompute only physical flows or > @@ -1908,13 +2040,6 @@ flow_output_physical_flow_changes_handler(struct engine_node *node, void *data) > return true; > } > > -static bool > -flow_output_noop_handler(struct engine_node *node OVS_UNUSED, > - void *data OVS_UNUSED) > -{ > - return true; > -} > - > struct ovn_controller_exit_args { > bool *exiting; > bool *restart; > @@ -2036,7 +2161,7 @@ main(int argc, char *argv[]) > > /* Define inc-proc-engine nodes. */ > ENGINE_NODE_CUSTOM_DATA(ct_zones, "ct_zones"); > - ENGINE_NODE(runtime_data, "runtime_data"); > + ENGINE_NODE_WITH_CLEAR_TRACK_DATA(runtime_data, "runtime_data"); > ENGINE_NODE(mff_ovn_geneve, "mff_ovn_geneve"); > ENGINE_NODE(ofctrl_is_connected, "ofctrl_is_connected"); > ENGINE_NODE_WITH_CLEAR_TRACK_DATA(physical_flow_changes, > @@ -2073,7 +2198,8 @@ main(int argc, char *argv[]) > flow_output_addr_sets_handler); > engine_add_input(&en_flow_output, &en_port_groups, > flow_output_port_groups_handler); > - engine_add_input(&en_flow_output, &en_runtime_data, NULL); > + engine_add_input(&en_flow_output, &en_runtime_data, > + flow_output_runtime_data_handler); > engine_add_input(&en_flow_output, &en_mff_ovn_geneve, NULL); > engine_add_input(&en_flow_output, &en_physical_flow_changes, > flow_output_physical_flow_changes_handler); > diff --git a/tests/ovn-performance.at b/tests/ovn-performance.at > index 5cc1960b6..08acd3bae 100644 > --- a/tests/ovn-performance.at > +++ b/tests/ovn-performance.at > @@ -286,11 +286,11 @@ for i in 1 2; do > [hv1 hv2], [lflow_run], > [ovn-nbctl --wait=hv lsp-set-type $lsp router] > ) > - OVN_CONTROLLER_EXPECT_HIT( > + OVN_CONTROLLER_EXPECT_NO_HIT( > [hv1 hv2], [lflow_run], > [ovn-nbctl --wait=hv lsp-set-options $lsp router-port=$lrp] > ) > - OVN_CONTROLLER_EXPECT_HIT( > + OVN_CONTROLLER_EXPECT_NO_HIT( > [hv1 hv2], [lflow_run], > [ovn-nbctl --wait=hv lsp-set-addresses $lsp router] > ) > @@ -416,15 +416,15 @@ OVN_CONTROLLER_EXPECT_NO_HIT( > [ovn-nbctl --wait=hv destroy Port_Group pg1] > ) > > -for i in 1 2; do > - ls=ls$i > +OVN_CONTROLLER_EXPECT_HIT( > + [hv1 hv2], [lflow_run], > + [ovn-nbctl --wait=hv ls-del ls1] > +) > > - # Delete switch $ls > - OVN_CONTROLLER_EXPECT_HIT( > - [hv1 hv2], [lflow_run], > - [ovn-nbctl --wait=hv ls-del $ls] > - ) > -done > +OVN_CONTROLLER_EXPECT_NO_HIT( > + [hv1 hv2], [lflow_run], > + [ovn-nbctl --wait=hv ls-del ls2] > +) > > # Delete router lr1 > OVN_CONTROLLER_EXPECT_NO_HIT( >
On Tue, Jun 9, 2020 at 3:11 PM Dumitru Ceara <dceara@redhat.com> wrote: > On 6/8/20 3:50 PM, numans@ovn.org wrote: > > From: Numan Siddique <numans@ovn.org> > > > > In order to handle runtime data changes incrementally, the flow outut > > runtime data handle should know the changed runtime data. > > Runtime data now tracks the changed data for any OVS interface > > and SB port binding changes. The tracked data contains a hmap > > of tracked datapaths (which changed during runtime data processing. > > > > The flow outout runtime_data handler in this patch doesn't do much > > with the tracked data. It returns false if there is tracked data > available > > so that flow_output run is called. If no tracked data is available > > then there is no need for flow computation and the handler returns true. > > > > Next patch in the series processes the tracked data incrementally. > > > > Co-Authored-by: Venkata Anil <anilvenkata@redhat.com> > > Signed-off-by: Venkata Anil <anilvenkata@redhat.com> > > Signed-off-by: Numan Siddique <numans@ovn.org> > > --- > > controller/binding.c | 299 ++++++++++++++++++++++++++++-------- > > controller/binding.h | 37 +++++ > > controller/ovn-controller.c | 144 +++++++++++++++-- > > tests/ovn-performance.at | 20 +-- > > 4 files changed, 413 insertions(+), 87 deletions(-) > > > > diff --git a/controller/binding.c b/controller/binding.c > > index c2d9a4c6b..3c226cd7d 100644 > > --- a/controller/binding.c > > +++ b/controller/binding.c > > @@ -69,13 +69,26 @@ binding_register_ovs_idl(struct ovsdb_idl *ovs_idl) > > ovsdb_idl_add_column(ovs_idl, &ovsrec_qos_col_type); > > } > > > > +static struct tracked_binding_datapath *tracked_binding_datapath_create( > > + const struct sbrec_datapath_binding *, > > + bool is_new, struct hmap *tracked_dps); > > +static struct tracked_binding_datapath *tracked_binding_datapath_find( > > + struct hmap *, const struct sbrec_datapath_binding *); > > +static void tracked_binding_datapath_lport_add( > > + const struct sbrec_port_binding *, bool deleted, > > + struct hmap *tracked_datapaths); > > +static void update_lport_tracking(const struct sbrec_port_binding *pb, > > + bool old_claim, bool new_claim, > > + struct hmap *tracked_dp_bindings); > > + > > static void > > add_local_datapath__(struct ovsdb_idl_index > *sbrec_datapath_binding_by_key, > > struct ovsdb_idl_index > *sbrec_port_binding_by_datapath, > > struct ovsdb_idl_index *sbrec_port_binding_by_name, > > const struct sbrec_datapath_binding *datapath, > > bool has_local_l3gateway, int depth, > > - struct hmap *local_datapaths) > > + struct hmap *local_datapaths, > > + struct hmap *tracked_datapaths) > > { > > uint32_t dp_key = datapath->tunnel_key; > > struct local_datapath *ld = get_local_datapath(local_datapaths, > dp_key); > > @@ -92,6 +105,11 @@ add_local_datapath__(struct ovsdb_idl_index > *sbrec_datapath_binding_by_key, > > ld->localnet_port = NULL; > > ld->has_local_l3gateway = has_local_l3gateway; > > > > + if (tracked_datapaths && > > + !tracked_binding_datapath_find(tracked_datapaths, datapath)) { > > + tracked_binding_datapath_create(datapath, true, > tracked_datapaths); > > + } > > + > > if (depth >= 100) { > > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > > VLOG_WARN_RL(&rl, "datapaths nested too deep"); > > @@ -124,7 +142,8 @@ add_local_datapath__(struct ovsdb_idl_index > *sbrec_datapath_binding_by_key, > > > sbrec_port_binding_by_datapath, > > sbrec_port_binding_by_name, > > peer->datapath, false, > > - depth + 1, > local_datapaths); > > + depth + 1, local_datapaths, > > + tracked_datapaths); > > } > > ld->n_peer_ports++; > > if (ld->n_peer_ports > ld->n_allocated_peer_ports) { > > @@ -147,12 +166,14 @@ add_local_datapath(struct ovsdb_idl_index > *sbrec_datapath_binding_by_key, > > struct ovsdb_idl_index > *sbrec_port_binding_by_datapath, > > struct ovsdb_idl_index *sbrec_port_binding_by_name, > > const struct sbrec_datapath_binding *datapath, > > - bool has_local_l3gateway, struct hmap > *local_datapaths) > > + bool has_local_l3gateway, struct hmap > *local_datapaths, > > + struct hmap *tracked_datapaths) > > { > > add_local_datapath__(sbrec_datapath_binding_by_key, > > sbrec_port_binding_by_datapath, > > sbrec_port_binding_by_name, > > - datapath, has_local_l3gateway, 0, > local_datapaths); > > + datapath, has_local_l3gateway, 0, > local_datapaths, > > + tracked_datapaths); > > } > > > > static void > > @@ -473,23 +494,45 @@ update_ld_localnet_port(const struct > sbrec_port_binding *binding_rec, > > > > static void > > update_local_lport_ids(struct sset *local_lport_ids, > > - const struct sbrec_port_binding *pb) > > + const struct sbrec_port_binding *pb, > > + struct hmap *tracked_datapaths) > > { > > char buf[16]; > > snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64, > > pb->datapath->tunnel_key, pb->tunnel_key); > > - sset_add(local_lport_ids, buf); > > + bool added = sset_add(local_lport_ids, buf); > > Hi Numan, > > Hi Dumitru, Thanks for the review. Please see below the replies. Thanks Numan > I guess this should be: > > bool added = !!sset_add(local_lport_ids, buf); > > Ack. > > + if (added && tracked_datapaths) { > > + /* Add the 'pb' to the tracked_datapaths. */ > > + tracked_binding_datapath_lport_add(pb, false, > tracked_datapaths); > > + } > > } > > > > static void > > -remove_local_lport_ids(const struct sbrec_port_binding *binding_rec, > > - struct sset *local_lport_ids) > > +remove_local_lport_ids(const struct sbrec_port_binding *pb, > > + struct sset *local_lport_ids, > > + struct hmap *tracked_datapaths) > > { > > char buf[16]; > > snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64, > > - binding_rec->datapath->tunnel_key, > > - binding_rec->tunnel_key); > > - sset_find_and_delete(local_lport_ids, buf); > > + pb->datapath->tunnel_key, pb->tunnel_key); > > + bool deleted = sset_find_and_delete(local_lport_ids, buf); > > + if (deleted && tracked_datapaths) { > > + /* Add the 'pb' to the tracked_datapaths. */ > > + tracked_binding_datapath_lport_add(pb, true, tracked_datapaths); > > + } > > +} > > + > > +static bool > > +add_lport_to_local_lports(struct sset *local_lports, const char > *lport_name) > > +{ > > + return !!sset_add(local_lports, lport_name); > > +} > > + > > +static bool > > +remove_lport_from_local_lports(struct sset *local_lports, > > + const char *lport_name) > > +{ > > + return sset_find_and_delete(local_lports, lport_name); > > } > > > > /* Local bindings. binding.c module binds the logical port (represented > by > > @@ -637,6 +680,77 @@ is_lport_container(const struct sbrec_port_binding > *pb) > > return is_lport_vif(pb) && pb->parent_port && pb->parent_port[0]; > > } > > > > +static struct tracked_binding_datapath * > > +tracked_binding_datapath_create(const struct sbrec_datapath_binding *dp, > > + bool is_new, > > + struct hmap *tracked_datapaths) > > +{ > > + struct tracked_binding_datapath *t_dp = xzalloc(sizeof *t_dp); > > + t_dp->dp = dp; > > + t_dp->is_new = is_new; > > + shash_init(&t_dp->lports); > > + hmap_insert(tracked_datapaths, &t_dp->node, > uuid_hash(&dp->header_.uuid)); > > + return t_dp; > > +} > > + > > +static struct tracked_binding_datapath * > > +tracked_binding_datapath_find(struct hmap *tracked_datapaths, > > + const struct sbrec_datapath_binding *dp) > > +{ > > + struct tracked_binding_datapath *t_dp; > > + size_t hash = uuid_hash(&dp->header_.uuid); > > + HMAP_FOR_EACH_WITH_HASH (t_dp, node, hash, tracked_datapaths) { > > + if (uuid_equals(&t_dp->dp->header_.uuid, &dp->header_.uuid)) { > > + return t_dp; > > + } > > + } > > + > > + return NULL; > > +} > > + > > +static void > > +tracked_binding_datapath_lport_add(const struct sbrec_port_binding *pb, > > + bool deleted, > > + struct hmap *tracked_datapaths) > > +{ > > + if (!tracked_datapaths) { > > + return; > > + } > > + > > + struct tracked_binding_datapath *tracked_dp = > > + tracked_binding_datapath_find(tracked_datapaths, pb->datapath); > > + if (!tracked_dp) { > > + tracked_dp = tracked_binding_datapath_create(pb->datapath, > false, > > + tracked_datapaths); > > + } > > + > > + /* Check if the lport is already present or not. > > + * If it is already present, then just update the 'pb' and 'deleted' > > + * fields. */ > > + struct tracked_binding_lport *lport = > > + shash_find_data(&tracked_dp->lports, pb->logical_port); > > + > > + if (!lport) { > > + lport = xmalloc(sizeof *lport); > > + shash_add(&tracked_dp->lports, pb->logical_port, lport); > > + } > > + > > + lport->pb = pb; > > + lport->deleted = deleted; > > +} > > + > > +void > > +binding_tracked_dp_destroy(struct hmap *tracked_datapaths) > > +{ > > + struct tracked_binding_datapath *t_dp; > > + HMAP_FOR_EACH_POP (t_dp, node, tracked_datapaths) { > > + shash_destroy_free_data(&t_dp->lports); > > + free(t_dp); > > + } > > + > > + hmap_destroy(tracked_datapaths); > > +} > > + > > /* Corresponds to each Port_Binding.type. */ > > enum en_lport_type { > > LP_UNKNOWN, > > @@ -690,7 +804,7 @@ static bool > > claim_lport(const struct sbrec_port_binding *pb, > > const struct sbrec_chassis *chassis_rec, > > const struct ovsrec_interface *iface_rec, > > - bool sb_readonly) > > + bool sb_readonly, struct hmap *tracked_datapaths) > > { > > if (pb->chassis != chassis_rec) { > > if (sb_readonly) { > > @@ -709,6 +823,10 @@ claim_lport(const struct sbrec_port_binding *pb, > > } > > > > sbrec_port_binding_set_chassis(pb, chassis_rec); > > + > > + if (tracked_datapaths) { > > + update_lport_tracking(pb, false, true, tracked_datapaths); > > + } > > } > > > > /* Check if the port encap binding, if any, has changed */ > > @@ -726,9 +844,14 @@ claim_lport(const struct sbrec_port_binding *pb, > > > > /* Returns false if lport is not released due to 'sb_readonly'. > > * Returns true otherwise. > > + * > > + * This function assumes that that the the 'pb' was claimed > > + * earlier i.e port binding's chassis is set to this chassis. > > + * Caller should make sure that, that's the case. > > Can we skip "that, " above? (non-native speaker here). > Ack. > > > */ > > static bool > > -release_lport(const struct sbrec_port_binding *pb, bool sb_readonly) > > +release_lport(const struct sbrec_port_binding *pb, bool sb_readonly, > > + struct hmap *tracked_datapaths) > > { > > if (pb->encap) { > > if (sb_readonly) { > > @@ -751,6 +874,7 @@ release_lport(const struct sbrec_port_binding *pb, > bool sb_readonly) > > sbrec_port_binding_set_virtual_parent(pb, NULL); > > } > > > > + update_lport_tracking(pb, true, false, tracked_datapaths); > > VLOG_INFO("Releasing lport %s from this chassis.", > pb->logical_port); > > return true; > > } > > @@ -796,13 +920,14 @@ is_lbinding_container_parent(struct local_binding > *lbinding) > > static bool > > release_local_binding_children(const struct sbrec_chassis *chassis_rec, > > struct local_binding *lbinding, > > - bool sb_readonly) > > + bool sb_readonly, > > + struct hmap *tracked_dp_bindings) > > { > > struct shash_node *node; > > SHASH_FOR_EACH (node, &lbinding->children) { > > struct local_binding *l = node->data; > > if (is_lbinding_this_chassis(l, chassis_rec)) { > > - if (!release_lport(l->pb, sb_readonly)) { > > + if (!release_lport(l->pb, sb_readonly, > tracked_dp_bindings)) { > > return false; > > } > > } > > @@ -817,16 +942,17 @@ release_local_binding_children(const struct > sbrec_chassis *chassis_rec, > > > > static bool > > release_local_binding(const struct sbrec_chassis *chassis_rec, > > - struct local_binding *lbinding, bool sb_readonly) > > + struct local_binding *lbinding, bool sb_readonly, > > + struct hmap *tracked_dp_bindings) > > { > > if (!release_local_binding_children(chassis_rec, lbinding, > > - sb_readonly)) { > > + sb_readonly, > tracked_dp_bindings)) { > > return false; > > } > > > > bool retval = true; > > if (is_lbinding_this_chassis(lbinding, chassis_rec)) { > > - retval = release_lport(lbinding->pb, sb_readonly); > > + retval = release_lport(lbinding->pb, sb_readonly, > tracked_dp_bindings); > > } > > > > lbinding->pb = NULL; > > @@ -847,7 +973,8 @@ consider_vif_lport_(const struct sbrec_port_binding > *pb, > > if (can_bind) { > > /* We can claim the lport. */ > > if (!claim_lport(pb, b_ctx_in->chassis_rec, lbinding->iface, > > - !b_ctx_in->ovnsb_idl_txn)){ > > + !b_ctx_in->ovnsb_idl_txn, > > + b_ctx_out->tracked_dp_bindings)){ > > return false; > > } > > > > @@ -855,8 +982,10 @@ consider_vif_lport_(const struct sbrec_port_binding > *pb, > > b_ctx_in->sbrec_port_binding_by_datapath, > > b_ctx_in->sbrec_port_binding_by_name, > > pb->datapath, false, > > - b_ctx_out->local_datapaths); > > - update_local_lport_ids(b_ctx_out->local_lport_ids, pb); > > + b_ctx_out->local_datapaths, > > + b_ctx_out->tracked_dp_bindings); > > + update_local_lport_ids(b_ctx_out->local_lport_ids, pb, > > + b_ctx_out->tracked_dp_bindings); > > if (lbinding->iface && qos_map && b_ctx_in->ovs_idl_txn) { > > get_qos_params(pb, qos_map); > > } > > @@ -875,7 +1004,8 @@ consider_vif_lport_(const struct sbrec_port_binding > *pb, > > if (pb->chassis == b_ctx_in->chassis_rec) { > > /* Release the lport if there is no lbinding. */ > > if (!lbinding_set || !can_bind) { > > - return release_lport(pb, !b_ctx_in->ovnsb_idl_txn); > > + return release_lport(pb, !b_ctx_in->ovnsb_idl_txn, > > + b_ctx_out->tracked_dp_bindings); > > } > > } > > > > @@ -962,7 +1092,8 @@ consider_container_lport(const struct > sbrec_port_binding *pb, > > * release the container lport, if it was bound earlier. */ > > if (is_lbinding_this_chassis(container_lbinding, > > b_ctx_in->chassis_rec)) { > > - return release_lport(pb, !b_ctx_in->ovnsb_idl_txn); > > + return release_lport(pb, !b_ctx_in->ovnsb_idl_txn, > > + b_ctx_out->tracked_dp_bindings); > > } > > > > return true; > > @@ -1037,18 +1168,22 @@ consider_nonvif_lport_(const struct > sbrec_port_binding *pb, > > struct binding_ctx_out *b_ctx_out) > > { > > if (our_chassis) { > > - sset_add(b_ctx_out->local_lports, pb->logical_port); > > + add_lport_to_local_lports(b_ctx_out->local_lports, > pb->logical_port); > > add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key, > > b_ctx_in->sbrec_port_binding_by_datapath, > > b_ctx_in->sbrec_port_binding_by_name, > > pb->datapath, has_local_l3gateway, > > - b_ctx_out->local_datapaths); > > + b_ctx_out->local_datapaths, > > + b_ctx_out->tracked_dp_bindings); > > > > - update_local_lport_ids(b_ctx_out->local_lport_ids, pb); > > + update_local_lport_ids(b_ctx_out->local_lport_ids, pb, > > + b_ctx_out->tracked_dp_bindings); > > return claim_lport(pb, b_ctx_in->chassis_rec, NULL, > > - !b_ctx_in->ovnsb_idl_txn); > > + !b_ctx_in->ovnsb_idl_txn, > > + b_ctx_out->tracked_dp_bindings); > > } else if (pb->chassis == b_ctx_in->chassis_rec) { > > - return release_lport(pb, !b_ctx_in->ovnsb_idl_txn); > > + return release_lport(pb, !b_ctx_in->ovnsb_idl_txn, > > + b_ctx_out->tracked_dp_bindings); > > } > > > > return true; > > @@ -1084,14 +1219,15 @@ consider_localnet_lport(const struct > sbrec_port_binding *pb, > > struct binding_ctx_out *b_ctx_out, > > struct hmap *qos_map) > > { > > - /* Add all localnet ports to local_lports so that we allocate ct > zones > > + /* Add all localnet ports to local_ifaces so that we allocate ct > zones > > * for them. */ > > - sset_add(b_ctx_out->local_lports, pb->logical_port); > > + add_lport_to_local_lports(b_ctx_out->local_lports, > pb->logical_port); > > if (qos_map && b_ctx_in->ovs_idl_txn) { > > get_qos_params(pb, qos_map); > > } > > > > - update_local_lport_ids(b_ctx_out->local_lport_ids, pb); > > + update_local_lport_ids(b_ctx_out->local_lport_ids, pb, > > + b_ctx_out->tracked_dp_bindings); > > } > > > > static bool > > @@ -1119,7 +1255,10 @@ consider_ha_lport(const struct sbrec_port_binding > *pb, > > b_ctx_in->sbrec_port_binding_by_datapath, > > b_ctx_in->sbrec_port_binding_by_name, > > pb->datapath, false, > > - b_ctx_out->local_datapaths); > > + b_ctx_out->local_datapaths, > > + b_ctx_out->tracked_dp_bindings); > > + update_local_lport_ids(b_ctx_out->local_lport_ids, pb, > > + b_ctx_out->tracked_dp_bindings); > > } > > > > return consider_nonvif_lport_(pb, our_chassis, false, b_ctx_in, > b_ctx_out); > > @@ -1185,7 +1324,7 @@ build_local_bindings(struct binding_ctx_in > *b_ctx_in, > > ovs_assert(lbinding->type == BT_VIF); > > } > > > > - sset_add(b_ctx_out->local_lports, iface_id); > > + add_lport_to_local_lports(b_ctx_out->local_lports, > iface_id); > > smap_replace(b_ctx_out->local_iface_ids, > iface_rec->name, > > iface_id); > > } > > @@ -1241,7 +1380,7 @@ binding_run(struct binding_ctx_in *b_ctx_in, > struct binding_ctx_out *b_ctx_out) > > case LP_PATCH: > > case LP_LOCALPORT: > > case LP_VTEP: > > - update_local_lport_ids(b_ctx_out->local_lport_ids, pb); > > + update_local_lport_ids(b_ctx_out->local_lport_ids, pb, > NULL); > > break; > > > > case LP_VIF: > > @@ -1409,7 +1548,8 @@ add_local_datapath_peer_port(const struct > sbrec_port_binding *pb, > > b_ctx_in->sbrec_port_binding_by_datapath, > > b_ctx_in->sbrec_port_binding_by_name, > > peer->datapath, false, > > - 1, b_ctx_out->local_datapaths); > > + 1, b_ctx_out->local_datapaths, > > + b_ctx_out->tracked_dp_bindings); > > return; > > } > > > > @@ -1471,7 +1611,8 @@ remove_pb_from_local_datapath(const struct > sbrec_port_binding *pb, > > struct binding_ctx_out *b_ctx_out, > > struct local_datapath *ld) > > { > > - remove_local_lport_ids(pb, b_ctx_out->local_lport_ids); > > + remove_local_lport_ids(pb, b_ctx_out->local_lport_ids, > > + b_ctx_out->tracked_dp_bindings); > > if (!strcmp(pb->type, "patch") || > > !strcmp(pb->type, "l3gateway")) { > > remove_local_datapath_peer_port(pb, ld, > b_ctx_out->local_datapaths); > > @@ -1489,6 +1630,18 @@ remove_pb_from_local_datapath(const struct > sbrec_port_binding *pb, > > } > > } > > > > +static void > > +update_lport_tracking(const struct sbrec_port_binding *pb, > > + bool old_claim, bool new_claim, > > + struct hmap *tracked_dp_bindings) > > +{ > > + if (!tracked_dp_bindings || !pb || (old_claim == new_claim)) { > > I think pb can never be NULL here, right? > Yeah. I'll remove the check. > > > + return; > > + } > > + > > + tracked_binding_datapath_lport_add(pb, old_claim, > tracked_dp_bindings); > > +} > > + > > /* Considers the ovs iface 'iface_rec' for claiming. > > * This function should be called if the external_ids:iface-id > > * and 'ofport' are set for the 'iface_rec'. > > @@ -1501,9 +1654,13 @@ consider_iface_claim(const struct > ovsrec_interface *iface_rec, > > const char *iface_id, > > struct binding_ctx_in *b_ctx_in, > > struct binding_ctx_out *b_ctx_out, > > - struct hmap *qos_map) > > + struct hmap *qos_map, > > + bool *local_lports_changed) > > { > > - sset_add(b_ctx_out->local_lports, iface_id); > > + if (add_lport_to_local_lports(b_ctx_out->local_lports, iface_id)) { > > + *local_lports_changed = true; > > + } > > + > > smap_replace(b_ctx_out->local_iface_ids, iface_rec->name, iface_id); > > > > struct local_binding *lbinding = > > @@ -1566,7 +1723,8 @@ static bool > > consider_iface_release(const struct ovsrec_interface *iface_rec, > > const char *iface_id, > > struct binding_ctx_in *b_ctx_in, > > - struct binding_ctx_out *b_ctx_out, bool *changed) > > + struct binding_ctx_out *b_ctx_out, bool *changed, > > + bool *local_lports_changed) > > { > > struct local_binding *lbinding; > > lbinding = local_binding_find(b_ctx_out->local_bindings, > > @@ -1585,7 +1743,8 @@ consider_iface_release(const struct > ovsrec_interface *iface_rec, > > * lbinding->iface. > > * Cannot access these members of lbinding after this call. */ > > if (!release_local_binding(b_ctx_in->chassis_rec, lbinding, > > - !b_ctx_in->ovnsb_idl_txn)) { > > + !b_ctx_in->ovnsb_idl_txn, > > + b_ctx_out->tracked_dp_bindings)) { > > return false; > > } > > > > @@ -1598,7 +1757,9 @@ consider_iface_release(const struct > ovsrec_interface *iface_rec, > > local_binding_delete(b_ctx_out->local_bindings, lbinding); > > } > > > > - sset_find_and_delete(b_ctx_out->local_lports, iface_id); > > + if (remove_lport_from_local_lports(b_ctx_out->local_lports, > iface_id)) { > > + *local_lports_changed = true; > > + } > > smap_remove(b_ctx_out->local_iface_ids, iface_rec->name); > > > > return true; > > @@ -1630,6 +1791,8 @@ binding_handle_ovs_interface_changes(struct > binding_ctx_in *b_ctx_in, > > bool handled = true; > > *changed = false; > > > > + *b_ctx_out->local_lports_changed = false; > > + > > Is this needed? This is part of the data we clear at every run in > en_runtime_data_clear_tracked_data(). > I still feel it's better to set it to false here. This function doesn't know anything about the tracked data. > > > /* Run the tracked interfaces loop twice. One to handle deleted > > * changes. And another to handle add/update changes. > > * This will ensure correctness. > > @@ -1685,7 +1848,8 @@ binding_handle_ovs_interface_changes(struct > binding_ctx_in *b_ctx_in, > > > > if (cleared_iface_id) { > > handled = consider_iface_release(iface_rec, > cleared_iface_id, > > - b_ctx_in, b_ctx_out, > changed); > > + b_ctx_in, b_ctx_out, > changed, > > + > b_ctx_out->local_lports_changed); > > } > > > > if (!handled) { > > @@ -1727,7 +1891,8 @@ binding_handle_ovs_interface_changes(struct > binding_ctx_in *b_ctx_in, > > if (iface_id && ofport > 0) { > > *changed = true; > > handled = consider_iface_claim(iface_rec, iface_id, > b_ctx_in, > > - b_ctx_out, qos_map_ptr); > > + b_ctx_out, qos_map_ptr, > > + > b_ctx_out->local_lports_changed); > > if (!handled) { > > break; > > } > > @@ -1792,8 +1957,7 @@ static bool > > handle_deleted_vif_lport(const struct sbrec_port_binding *pb, > > enum en_lport_type lport_type, > > struct binding_ctx_in *b_ctx_in, > > - struct binding_ctx_out *b_ctx_out, > > - bool *changed) > > + struct binding_ctx_out *b_ctx_out) > > { > > struct local_binding *lbinding = > > get_lbinding_for_lport(pb, lport_type, b_ctx_out); > > @@ -1804,13 +1968,12 @@ handle_deleted_vif_lport(const struct > sbrec_port_binding *pb, > > * clear the 'chassis' column of 'pb'. But we need to do > > * for the local_binding's children. */ > > if (lbinding->type == BT_VIF && > > - !release_local_binding_children(b_ctx_in->chassis_rec, > > - lbinding, > > - > !b_ctx_in->ovnsb_idl_txn)) { > > + !release_local_binding_children( > > + b_ctx_in->chassis_rec, lbinding, > > + !b_ctx_in->ovnsb_idl_txn, > > + b_ctx_out->tracked_dp_bindings)) { > > return false; > > } > > - > > - *changed = true; > > } > > > > handle_deleted_lport(pb, b_ctx_in, b_ctx_out); > > @@ -1822,8 +1985,7 @@ handle_updated_vif_lport(const struct > sbrec_port_binding *pb, > > enum en_lport_type lport_type, > > struct binding_ctx_in *b_ctx_in, > > struct binding_ctx_out *b_ctx_out, > > - struct hmap *qos_map, > > - bool *changed) > > + struct hmap *qos_map) > > { > > bool claimed = (pb->chassis == b_ctx_in->chassis_rec); > > bool handled = true; > > @@ -1841,14 +2003,10 @@ handle_updated_vif_lport(const struct > sbrec_port_binding *pb, > > } > > > > bool now_claimed = (pb->chassis == b_ctx_in->chassis_rec); > > - bool changed_ = (claimed != now_claimed); > > - > > - if (changed_) { > > - *changed = true; > > - } > > + bool changed = (claimed != now_claimed); > > > > if (lport_type == LP_VIRTUAL || > > - (lport_type == LP_VIF && is_lport_container(pb)) || !changed_) { > > + (lport_type == LP_VIF && is_lport_container(pb)) || !changed) { > > return true; > > } > > > > @@ -1898,7 +2056,7 @@ binding_handle_port_binding_changes(struct > binding_ctx_in *b_ctx_in, > > enum en_lport_type lport_type = get_lport_type(pb); > > if (lport_type == LP_VIF || lport_type == LP_VIRTUAL) { > > handled = handle_deleted_vif_lport(pb, lport_type, b_ctx_in, > > - b_ctx_out, changed); > > + b_ctx_out); > > } else { > > handle_deleted_lport(pb, b_ctx_in, b_ctx_out); > > } > > @@ -1933,15 +2091,14 @@ binding_handle_port_binding_changes(struct > binding_ctx_in *b_ctx_in, > > case LP_VIF: > > case LP_VIRTUAL: > > handled = handle_updated_vif_lport(pb, lport_type, b_ctx_in, > > - b_ctx_out, qos_map_ptr, > > - changed); > > + b_ctx_out, qos_map_ptr); > > break; > > > > case LP_PATCH: > > case LP_LOCALPORT: > > case LP_VTEP: > > - *changed = true; > > - update_local_lport_ids(b_ctx_out->local_lport_ids, pb); > > + update_local_lport_ids(b_ctx_out->local_lport_ids, pb, > > + b_ctx_out->tracked_dp_bindings); > > if (lport_type == LP_PATCH) { > > /* Add the peer datapath to the local datapaths if it's > > * not present yet. > > @@ -1950,30 +2107,32 @@ binding_handle_port_binding_changes(struct > binding_ctx_in *b_ctx_in, > > add_local_datapath_peer_port(pb, b_ctx_in, > b_ctx_out, ld); > > } > > } > > + > > + if (lport_type == LP_VTEP) { > > + /* VTEP lports are claimed/released by > ovn-controller-vteps. > > + * We are not sure what changed. So set *changed to true > > + * for any changes to vtep lports. */ > > + *changed = true; > > + } > > break; > > > > case LP_L2GATEWAY: > > - *changed = true; > > handled = consider_l2gw_lport(pb, b_ctx_in, b_ctx_out); > > break; > > > > case LP_L3GATEWAY: > > - *changed = true; > > handled = consider_l3gw_lport(pb, b_ctx_in, b_ctx_out); > > break; > > > > case LP_CHASSISREDIRECT: > > - *changed = true; > > handled = consider_cr_lport(pb, b_ctx_in, b_ctx_out); > > break; > > > > case LP_EXTERNAL: > > - *changed = true; > > handled = consider_external_lport(pb, b_ctx_in, b_ctx_out); > > break; > > > > case LP_LOCALNET: { > > - *changed = true; > > consider_localnet_lport(pb, b_ctx_in, b_ctx_out, > qos_map_ptr); > > > > struct shash bridge_mappings = > > @@ -2008,6 +2167,10 @@ binding_handle_port_binding_changes(struct > binding_ctx_in *b_ctx_in, > > } > > } > > > > + if (handled && !hmap_is_empty(b_ctx_out->tracked_dp_bindings)) { > > + *changed = true; > > + } > > + > > destroy_qos_map(&qos_map); > > return handled; > > } > > diff --git a/controller/binding.h b/controller/binding.h > > index 21118ecd4..2974ebb30 100644 > > --- a/controller/binding.h > > +++ b/controller/binding.h > > @@ -19,6 +19,9 @@ > > > > #include <stdbool.h> > > #include "openvswitch/shash.h" > > +#include "openvswitch/hmap.h" > > +#include "openvswitch/uuid.h" > > +#include "openvswitch/list.h" > > > > struct hmap; > > struct ovsdb_idl; > > @@ -54,10 +57,28 @@ struct binding_ctx_in { > > struct binding_ctx_out { > > struct hmap *local_datapaths; > > struct shash *local_bindings; > > + > > struct sset *local_lports; > > + > > + /* If the sset local_lports is modified, this is set to true by > > + * the callee. */ > > + bool *local_lports_changed; > > + > > + /* sset of local lport ids in the format > > + * <datapath-tunnel-key>_<port-tunnel-key>. */ > > Nit: shouldn't this comment be part of patch 1? > Could be. > > > struct sset *local_lport_ids; > > + > > struct sset *egress_ifaces; > > + > > + /* smap of OVS interface name as key and > > + * OVS interface external_ids:iface-id as value. */ > > Nit: shouldn't this comment be part of patch 2? > Could be. > > > struct smap *local_iface_ids; > > + > > + /* hmap of 'struct tracked_binding_datapath' which the > > + * callee (binding_handle_ovs_interface_changes and > > + * binding_handle_port_binding_changes) fills in for > > + * the changed datapaths and port bindings. */ > > + struct hmap *tracked_dp_bindings; > > }; > > > > enum local_binding_type { > > @@ -82,6 +103,21 @@ local_binding_find(struct shash *local_bindings, > const char *name) > > return shash_find_data(local_bindings, name); > > } > > > > +/* Represents a tracked binding logical port. */ > > +struct tracked_binding_lport { > > + const struct sbrec_port_binding *pb; > > + struct ovs_list list_node; > > Nit: for consistency with tracked_binding_datapath and because list_node > is usually accessed before pb I'd define list_node before pb. > Actually we don't need list_node now. It was a leftover from the previous versions. I'll delete it. Thanks. > > > + bool deleted; > > +}; > > + > > +/* Represent a tracked binding datapath. */ > > +struct tracked_binding_datapath { > > + struct hmap_node node; > > + const struct sbrec_datapath_binding *dp; > > + bool is_new; > > I think it would be easier to read the code if both > tracked_binding_lport and tracked_binding_datapath would use the same > logic for tracking updates: either both should have "deleted" or both > should have "is_new". > Ack. > > Also, given that we never use tracked_binding_lport.deleted, maybe we > can skip it completely. What do you think? > I'll delete the "deleted". We can add it later if it is really required. > > > + struct shash lports; /* shash of struct tracked_binding_lport. */ > > +}; > > + > > void binding_register_ovs_idl(struct ovsdb_idl *); > > void binding_run(struct binding_ctx_in *, struct binding_ctx_out *); > > bool binding_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn, > > @@ -96,4 +132,5 @@ bool binding_handle_ovs_interface_changes(struct > binding_ctx_in *, > > bool binding_handle_port_binding_changes(struct binding_ctx_in *, > > struct binding_ctx_out *, > > bool *changed); > > +void binding_tracked_dp_destroy(struct hmap *tracked_datapaths); > > #endif /* controller/binding.h */ > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index 795a1c297..98652866c 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -973,10 +973,88 @@ struct ed_type_runtime_data { > > struct sset local_lport_ids; > > struct sset active_tunnels; > > > > + /* runtime data engine private data. */ > > struct sset egress_ifaces; > > struct smap local_iface_ids; > > + > > + /* Tracked data. See below for more details and comments. */ > > + bool tracked; > > I wonder if the "tracked" field should actually be part of the generic > engine_node_input. We use it for port groups and address sets too > ("change_tracked" in ed_type_addr_sets and ed_type_port_groups) and > afaiu the semantics are always: > - "tracked" is set to true if a node's input handler was called > - "tracked" is reset to false if a node's run handler was called > > So all this could be implemented generically in inc-proc-eng.c. > > What do you think? > Could be. But I'd prefer to be a separate patch (not part of this series). > > > + bool local_lports_changed; > > Do we really need local_lports_changed? Isn't it actually equivalent to > !hmap_is_empty(& > > > + struct hmap tracked_dp_bindings; > > }; > Not really. Let's say you run the command - "ovs-vsctl add-port br-int p0 -- set interface p0 externa_ids:iface-id=lport1" And if logical lport lport1 doesn't exist (or not seen by ovn-controller due to conditional monitoring), we need to update the runtime_data engine node so that "update_sb_monitor" is called. > > > > +/* struct ed_type_runtime_data has the below members for tracking the > > + * changes done to the runtime_data engine by the runtime_data engine > > + * handlers. Since this engine is an input to the flow_output engine, > > + * the flow output runtime data handler will make use of this tracked > data. > > + * > > + * > ------------------------------------------------------------------------ > > + * | | This is a hmap of > | > > + * | | 'struct tracked_binding_datapath' defined > in | > > + * | | binding.h. Runtime data handlers for OVS > | > > + * | | Interface and Port Binding changes store > the | > > + * | @tracked_dp_bindings | changed datapaths (datapaths added/removed > from | > > + * | | local_datapaths) and changed port bindings > | > > + * | | (added/updated/deleted in > 'local_bindings'). | > > This is not really accurate, right? Afaiu we only track if a port > binding is added/deleted. > Not all the time. We can bind an "external" port when the port binding gets updated due to the command - "ovn-nbctl lsp-set-type <lport> external" or for the command - "ovn-nbctl lsp-set-options router-port=<peer port> > > > + * | | So any changes to the runtime data - > | > > + * | | local_datapaths and local_bindings is > captured | > > + * | | here. > | > > + * > ------------------------------------------------------------------------ > > + * | | This is a bool which represents if the > runtime | > > + * | | data 'local_lports' changed by the runtime > data | > > + * | | handlers for OVS Interface and Port > Binding | > > + * | | changes. If 'local_lports' is updated and > also | > > + * | | results in any port binding updates, it is > | > > + * |@local_lports_changed | captured in the @tracked_dp_bindings. So > there | > > + * | | is no need to capture the changes in the > | > > + * | | local_lports. If @local_lports_changed is > true | > > + * | | but without anydata in the > @tracked_dp_bindings,| > > + * | | it means we needto only update the SB > monitor | > > + * | | clauses and there isno need for any flow > | > > + * | | (re)computations. > | > > + * > ------------------------------------------------------------------------ > > + * | | This represents if the data was tracked or > not | > > + * | | by the runtime data handlers during the > engine | > > + * | @tracked | run. If the runtime data recompute is > | > > + * | | triggered, it means there is no tracked > data. | > > + * > ------------------------------------------------------------------------ > > + * > > + * > > + * The changes to the following runtime_data variables are not tracked. > > + * > > + * > --------------------------------------------------------------------- > > + * | local_datapaths | The changes to these runtime data is captured > in | > > + * | local_bindings | the @tracked_dp_bindings indirectly and hence > it | > > + * | local_lport_ids | is not tracked explicitly. > | > > + * > --------------------------------------------------------------------- > > + * | local_iface_ids | This is used internally within the runtime > data | > > + * | egress_ifaces | engine (used only in binding.c) and hence > there | > > + * | | there is no need to track. > | > > + * > --------------------------------------------------------------------- > > + * | | Active tunnels is built in the > | > > + * | | bfd_calculate_active_tunnels() for the tunnel > | > > + * | | OVS interfaces. Any changes to non VIF OVS > | > > + * | | interfaces results in triggering the full > | > > + * | active_tunnels | recompute of runtime data engine and hence > there | > > + * | | the tracked data doesn't track it. When we > | > > + * | | support handling changes to non VIF OVS > | > > + * | | interfaces we need to track the changes to the > | > > + * | | active tunnels. > | > > + * > --------------------------------------------------------------------- > > + * > > + */ > > + > > +static void > > +en_runtime_data_clear_tracked_data(void *data_) > > +{ > > + struct ed_type_runtime_data *data = data_; > > + > > + binding_tracked_dp_destroy(&data->tracked_dp_bindings); > > + hmap_init(&data->tracked_dp_bindings); > > Nit: just as we did for local_bindings_init()/local_bindings_destroy() > I'd move the "hmap_init(&data->tracked_dp_bindings)" inside a new > function called binding_tracked_dp_init(). > > Not sure if we want to have the same functions here just to init the hmap. > > + data->local_lports_changed = false; > > + data->tracked = false; > > +} > > + > > static void * > > en_runtime_data_init(struct engine_node *node OVS_UNUSED, > > struct engine_arg *arg OVS_UNUSED) > > @@ -990,6 +1068,10 @@ en_runtime_data_init(struct engine_node *node > OVS_UNUSED, > > sset_init(&data->egress_ifaces); > > smap_init(&data->local_iface_ids); > > local_bindings_init(&data->local_bindings); > > + > > + /* init the tracked data. */ > > + hmap_init(&data->tracked_dp_bindings); > > + > > return data; > > } > > > > @@ -1012,6 +1094,8 @@ en_runtime_data_cleanup(void *data) > > } > > hmap_destroy(&rt_data->local_datapaths); > > local_bindings_destroy(&rt_data->local_bindings); > > + > > + en_runtime_data_clear_tracked_data(data); > > } > > > > static void > > @@ -1092,6 +1176,8 @@ init_binding_ctx(struct engine_node *node, > > b_ctx_out->egress_ifaces = &rt_data->egress_ifaces; > > b_ctx_out->local_bindings = &rt_data->local_bindings; > > b_ctx_out->local_iface_ids = &rt_data->local_iface_ids; > > + b_ctx_out->tracked_dp_bindings = NULL; > > + b_ctx_out->local_lports_changed = NULL; > > } > > > > static void > > @@ -1103,6 +1189,13 @@ en_runtime_data_run(struct engine_node *node, > void *data) > > struct sset *local_lport_ids = &rt_data->local_lport_ids; > > struct sset *active_tunnels = &rt_data->active_tunnels; > > > > + /* Clear the tracked data. Even though the tracked data > > + * gets cleared in the beginning of engine_init_run(), > > + * any of the runtime data handler might have set some tracked > > + * data and later another runtime data handler might return false > > + * resulting full recompute of runtime engine. */ > > + en_runtime_data_clear_tracked_data(data); > > + > > Could you please elaborate more on this? Why is it an issue if we keep > the "tracked" data until the end of the run? > When a runtime data node gets updated (engine_set_node_state()) either due to full recompute of runtime or due to handler setting it, flow_output_runtime_data_handler() will not trigger flow_output recompute unless it see runtime->tracked = false. Hence it clears the tracked data. Instead of calling en_runtime_data_clear_tracked_data(), en_runtime_data_run() can set just 'tracked' to false, but I think it's better to clear the whole tracked data. > > Thanks, > Dumitru > > > static bool first_run = true; > > if (first_run) { > > /* don't cleanup since there is no data yet */ > > @@ -1158,6 +1251,9 @@ runtime_data_ovs_interface_handler(struct > engine_node *node, void *data) > > struct binding_ctx_in b_ctx_in; > > struct binding_ctx_out b_ctx_out; > > init_binding_ctx(node, rt_data, &b_ctx_in, &b_ctx_out); > > + rt_data->tracked = true; > > + b_ctx_out.tracked_dp_bindings = &rt_data->tracked_dp_bindings; > > + b_ctx_out.local_lports_changed = &rt_data->local_lports_changed; > > > > bool changed = false; > > if (!binding_handle_ovs_interface_changes(&b_ctx_in, &b_ctx_out, > > @@ -1190,6 +1286,9 @@ runtime_data_sb_port_binding_handler(struct > engine_node *node, void *data) > > return false; > > } > > > > + rt_data->tracked = true; > > + b_ctx_out.tracked_dp_bindings = &rt_data->tracked_dp_bindings; > > + > > bool changed = false; > > if (!binding_handle_port_binding_changes(&b_ctx_in, &b_ctx_out, > > &changed)) { > > @@ -1797,6 +1896,39 @@ flow_output_port_groups_handler(struct > engine_node *node, void *data) > > return _flow_output_resource_ref_handler(node, data, > REF_TYPE_PORTGROUP); > > } > > > > +static bool > > +flow_output_runtime_data_handler(struct engine_node *node, > > + void *data OVS_UNUSED) > > +{ > > + struct ed_type_runtime_data *rt_data = > > + engine_get_input_data("runtime_data", node); > > + > > + /* There is no tracked data. Fall back to full recompute of > > + * flow_output. */ > > + if (!rt_data->tracked) { > > + return false; > > + } > > + > > + if (!hmap_is_empty(&rt_data->tracked_dp_bindings)) { > > + /* We are not yet handling the tracked datapath binding > > + * changes. Return false to trigger full recompute. */ > > + return false; > > + } > > + > > + if (rt_data->local_lports_changed) { > > + engine_set_node_state(node, EN_UPDATED); > > + } > > + > > + return true; > > +} > > + > > +static bool > > +flow_output_noop_handler(struct engine_node *node OVS_UNUSED, > > + void *data OVS_UNUSED) > > +{ > > + return true; > > +} > > + > > /* Engine node en_physical_flow_changes indicates whether > > * there is a need to > > * - recompute only physical flows or > > @@ -1908,13 +2040,6 @@ flow_output_physical_flow_changes_handler(struct > engine_node *node, void *data) > > return true; > > } > > > > -static bool > > -flow_output_noop_handler(struct engine_node *node OVS_UNUSED, > > - void *data OVS_UNUSED) > > -{ > > - return true; > > -} > > - > > struct ovn_controller_exit_args { > > bool *exiting; > > bool *restart; > > @@ -2036,7 +2161,7 @@ main(int argc, char *argv[]) > > > > /* Define inc-proc-engine nodes. */ > > ENGINE_NODE_CUSTOM_DATA(ct_zones, "ct_zones"); > > - ENGINE_NODE(runtime_data, "runtime_data"); > > + ENGINE_NODE_WITH_CLEAR_TRACK_DATA(runtime_data, "runtime_data"); > > ENGINE_NODE(mff_ovn_geneve, "mff_ovn_geneve"); > > ENGINE_NODE(ofctrl_is_connected, "ofctrl_is_connected"); > > ENGINE_NODE_WITH_CLEAR_TRACK_DATA(physical_flow_changes, > > @@ -2073,7 +2198,8 @@ main(int argc, char *argv[]) > > flow_output_addr_sets_handler); > > engine_add_input(&en_flow_output, &en_port_groups, > > flow_output_port_groups_handler); > > - engine_add_input(&en_flow_output, &en_runtime_data, NULL); > > + engine_add_input(&en_flow_output, &en_runtime_data, > > + flow_output_runtime_data_handler); > > engine_add_input(&en_flow_output, &en_mff_ovn_geneve, NULL); > > engine_add_input(&en_flow_output, &en_physical_flow_changes, > > flow_output_physical_flow_changes_handler); > > diff --git a/tests/ovn-performance.at b/tests/ovn-performance.at > > index 5cc1960b6..08acd3bae 100644 > > --- a/tests/ovn-performance.at > > +++ b/tests/ovn-performance.at > > @@ -286,11 +286,11 @@ for i in 1 2; do > > [hv1 hv2], [lflow_run], > > [ovn-nbctl --wait=hv lsp-set-type $lsp router] > > ) > > - OVN_CONTROLLER_EXPECT_HIT( > > + OVN_CONTROLLER_EXPECT_NO_HIT( > > [hv1 hv2], [lflow_run], > > [ovn-nbctl --wait=hv lsp-set-options $lsp router-port=$lrp] > > ) > > - OVN_CONTROLLER_EXPECT_HIT( > > + OVN_CONTROLLER_EXPECT_NO_HIT( > > [hv1 hv2], [lflow_run], > > [ovn-nbctl --wait=hv lsp-set-addresses $lsp router] > > ) > > @@ -416,15 +416,15 @@ OVN_CONTROLLER_EXPECT_NO_HIT( > > [ovn-nbctl --wait=hv destroy Port_Group pg1] > > ) > > > > -for i in 1 2; do > > - ls=ls$i > > +OVN_CONTROLLER_EXPECT_HIT( > > + [hv1 hv2], [lflow_run], > > + [ovn-nbctl --wait=hv ls-del ls1] > > +) > > > > - # Delete switch $ls > > - OVN_CONTROLLER_EXPECT_HIT( > > - [hv1 hv2], [lflow_run], > > - [ovn-nbctl --wait=hv ls-del $ls] > > - ) > > -done > > +OVN_CONTROLLER_EXPECT_NO_HIT( > > + [hv1 hv2], [lflow_run], > > + [ovn-nbctl --wait=hv ls-del ls2] > > +) > > > > # Delete router lr1 > > OVN_CONTROLLER_EXPECT_NO_HIT( > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On 6/9/20 1:00 PM, Numan Siddique wrote: > > > On Tue, Jun 9, 2020 at 3:11 PM Dumitru Ceara <dceara@redhat.com > <mailto:dceara@redhat.com>> wrote: > > On 6/8/20 3:50 PM, numans@ovn.org <mailto:numans@ovn.org> wrote: > > From: Numan Siddique <numans@ovn.org <mailto:numans@ovn.org>> > > > > In order to handle runtime data changes incrementally, the flow outut > > runtime data handle should know the changed runtime data. > > Runtime data now tracks the changed data for any OVS interface > > and SB port binding changes. The tracked data contains a hmap > > of tracked datapaths (which changed during runtime data processing. > > > > The flow outout runtime_data handler in this patch doesn't do much > > with the tracked data. It returns false if there is tracked data > available > > so that flow_output run is called. If no tracked data is available > > then there is no need for flow computation and the handler returns > true. > > > > Next patch in the series processes the tracked data incrementally. > > > > Co-Authored-by: Venkata Anil <anilvenkata@redhat.com > <mailto:anilvenkata@redhat.com>> > > Signed-off-by: Venkata Anil <anilvenkata@redhat.com > <mailto:anilvenkata@redhat.com>> > > Signed-off-by: Numan Siddique <numans@ovn.org <mailto:numans@ovn.org>> > > --- > > controller/binding.c | 299 > ++++++++++++++++++++++++++++-------- > > controller/binding.h | 37 +++++ > > controller/ovn-controller.c | 144 +++++++++++++++-- > > tests/ovn-performance.at <http://ovn-performance.at> | 20 +-- > > 4 files changed, 413 insertions(+), 87 deletions(-) > > > > diff --git a/controller/binding.c b/controller/binding.c > > index c2d9a4c6b..3c226cd7d 100644 > > --- a/controller/binding.c > > +++ b/controller/binding.c > > @@ -69,13 +69,26 @@ binding_register_ovs_idl(struct ovsdb_idl > *ovs_idl) > > ovsdb_idl_add_column(ovs_idl, &ovsrec_qos_col_type); > > } > > > > +static struct tracked_binding_datapath > *tracked_binding_datapath_create( > > + const struct sbrec_datapath_binding *, > > + bool is_new, struct hmap *tracked_dps); > > +static struct tracked_binding_datapath > *tracked_binding_datapath_find( > > + struct hmap *, const struct sbrec_datapath_binding *); > > +static void tracked_binding_datapath_lport_add( > > + const struct sbrec_port_binding *, bool deleted, > > + struct hmap *tracked_datapaths); > > +static void update_lport_tracking(const struct sbrec_port_binding > *pb, > > + bool old_claim, bool new_claim, > > + struct hmap *tracked_dp_bindings); > > + > > static void > > add_local_datapath__(struct ovsdb_idl_index > *sbrec_datapath_binding_by_key, > > struct ovsdb_idl_index > *sbrec_port_binding_by_datapath, > > struct ovsdb_idl_index > *sbrec_port_binding_by_name, > > const struct sbrec_datapath_binding *datapath, > > bool has_local_l3gateway, int depth, > > - struct hmap *local_datapaths) > > + struct hmap *local_datapaths, > > + struct hmap *tracked_datapaths) > > { > > uint32_t dp_key = datapath->tunnel_key; > > struct local_datapath *ld = > get_local_datapath(local_datapaths, dp_key); > > @@ -92,6 +105,11 @@ add_local_datapath__(struct ovsdb_idl_index > *sbrec_datapath_binding_by_key, > > ld->localnet_port = NULL; > > ld->has_local_l3gateway = has_local_l3gateway; > > > > + if (tracked_datapaths && > > + !tracked_binding_datapath_find(tracked_datapaths, > datapath)) { > > + tracked_binding_datapath_create(datapath, true, > tracked_datapaths); > > + } > > + > > if (depth >= 100) { > > static struct vlog_rate_limit rl = > VLOG_RATE_LIMIT_INIT(1, 1); > > VLOG_WARN_RL(&rl, "datapaths nested too deep"); > > @@ -124,7 +142,8 @@ add_local_datapath__(struct ovsdb_idl_index > *sbrec_datapath_binding_by_key, > > > sbrec_port_binding_by_datapath, > > > sbrec_port_binding_by_name, > > peer->datapath, false, > > - depth + 1, > local_datapaths); > > + depth + 1, > local_datapaths, > > + tracked_datapaths); > > } > > ld->n_peer_ports++; > > if (ld->n_peer_ports > > ld->n_allocated_peer_ports) { > > @@ -147,12 +166,14 @@ add_local_datapath(struct ovsdb_idl_index > *sbrec_datapath_binding_by_key, > > struct ovsdb_idl_index > *sbrec_port_binding_by_datapath, > > struct ovsdb_idl_index > *sbrec_port_binding_by_name, > > const struct sbrec_datapath_binding *datapath, > > - bool has_local_l3gateway, struct hmap > *local_datapaths) > > + bool has_local_l3gateway, struct hmap > *local_datapaths, > > + struct hmap *tracked_datapaths) > > { > > add_local_datapath__(sbrec_datapath_binding_by_key, > > sbrec_port_binding_by_datapath, > > sbrec_port_binding_by_name, > > - datapath, has_local_l3gateway, 0, > local_datapaths); > > + datapath, has_local_l3gateway, 0, > local_datapaths, > > + tracked_datapaths); > > } > > > > static void > > @@ -473,23 +494,45 @@ update_ld_localnet_port(const struct > sbrec_port_binding *binding_rec, > > > > static void > > update_local_lport_ids(struct sset *local_lport_ids, > > - const struct sbrec_port_binding *pb) > > + const struct sbrec_port_binding *pb, > > + struct hmap *tracked_datapaths) > > { > > char buf[16]; > > snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64, > > pb->datapath->tunnel_key, pb->tunnel_key); > > - sset_add(local_lport_ids, buf); > > + bool added = sset_add(local_lport_ids, buf); > > Hi Numan, > > > Hi Dumitru, > > Thanks for the review. > > Please see below the replies. > > Thanks > Numan > > > > I guess this should be: > > bool added = !!sset_add(local_lport_ids, buf); > > > Ack. > > > > > + if (added && tracked_datapaths) { > > + /* Add the 'pb' to the tracked_datapaths. */ > > + tracked_binding_datapath_lport_add(pb, false, > tracked_datapaths); > > + } > > } > > > > static void > > -remove_local_lport_ids(const struct sbrec_port_binding *binding_rec, > > - struct sset *local_lport_ids) > > +remove_local_lport_ids(const struct sbrec_port_binding *pb, > > + struct sset *local_lport_ids, > > + struct hmap *tracked_datapaths) > > { > > char buf[16]; > > snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64, > > - binding_rec->datapath->tunnel_key, > > - binding_rec->tunnel_key); > > - sset_find_and_delete(local_lport_ids, buf); > > + pb->datapath->tunnel_key, pb->tunnel_key); > > + bool deleted = sset_find_and_delete(local_lport_ids, buf); > > + if (deleted && tracked_datapaths) { > > + /* Add the 'pb' to the tracked_datapaths. */ > > + tracked_binding_datapath_lport_add(pb, true, > tracked_datapaths); > > + } > > +} > > + > > +static bool > > +add_lport_to_local_lports(struct sset *local_lports, const char > *lport_name) > > +{ > > + return !!sset_add(local_lports, lport_name); > > +} > > + > > +static bool > > +remove_lport_from_local_lports(struct sset *local_lports, > > + const char *lport_name) > > +{ > > + return sset_find_and_delete(local_lports, lport_name); > > } > > > > /* Local bindings. binding.c module binds the logical port > (represented by > > @@ -637,6 +680,77 @@ is_lport_container(const struct > sbrec_port_binding *pb) > > return is_lport_vif(pb) && pb->parent_port && pb->parent_port[0]; > > } > > > > +static struct tracked_binding_datapath * > > +tracked_binding_datapath_create(const struct > sbrec_datapath_binding *dp, > > + bool is_new, > > + struct hmap *tracked_datapaths) > > +{ > > + struct tracked_binding_datapath *t_dp = xzalloc(sizeof *t_dp); > > + t_dp->dp = dp; > > + t_dp->is_new = is_new; > > + shash_init(&t_dp->lports); > > + hmap_insert(tracked_datapaths, &t_dp->node, > uuid_hash(&dp->header_.uuid)); > > + return t_dp; > > +} > > + > > +static struct tracked_binding_datapath * > > +tracked_binding_datapath_find(struct hmap *tracked_datapaths, > > + const struct sbrec_datapath_binding > *dp) > > +{ > > + struct tracked_binding_datapath *t_dp; > > + size_t hash = uuid_hash(&dp->header_.uuid); > > + HMAP_FOR_EACH_WITH_HASH (t_dp, node, hash, tracked_datapaths) { > > + if (uuid_equals(&t_dp->dp->header_.uuid, > &dp->header_.uuid)) { > > + return t_dp; > > + } > > + } > > + > > + return NULL; > > +} > > + > > +static void > > +tracked_binding_datapath_lport_add(const struct > sbrec_port_binding *pb, > > + bool deleted, > > + struct hmap *tracked_datapaths) > > +{ > > + if (!tracked_datapaths) { > > + return; > > + } > > + > > + struct tracked_binding_datapath *tracked_dp = > > + tracked_binding_datapath_find(tracked_datapaths, > pb->datapath); > > + if (!tracked_dp) { > > + tracked_dp = > tracked_binding_datapath_create(pb->datapath, false, > > + > tracked_datapaths); > > + } > > + > > + /* Check if the lport is already present or not. > > + * If it is already present, then just update the 'pb' and > 'deleted' > > + * fields. */ > > + struct tracked_binding_lport *lport = > > + shash_find_data(&tracked_dp->lports, pb->logical_port); > > + > > + if (!lport) { > > + lport = xmalloc(sizeof *lport); > > + shash_add(&tracked_dp->lports, pb->logical_port, lport); > > + } > > + > > + lport->pb = pb; > > + lport->deleted = deleted; > > +} > > + > > +void > > +binding_tracked_dp_destroy(struct hmap *tracked_datapaths) > > +{ > > + struct tracked_binding_datapath *t_dp; > > + HMAP_FOR_EACH_POP (t_dp, node, tracked_datapaths) { > > + shash_destroy_free_data(&t_dp->lports); > > + free(t_dp); > > + } > > + > > + hmap_destroy(tracked_datapaths); > > +} > > + > > /* Corresponds to each Port_Binding.type. */ > > enum en_lport_type { > > LP_UNKNOWN, > > @@ -690,7 +804,7 @@ static bool > > claim_lport(const struct sbrec_port_binding *pb, > > const struct sbrec_chassis *chassis_rec, > > const struct ovsrec_interface *iface_rec, > > - bool sb_readonly) > > + bool sb_readonly, struct hmap *tracked_datapaths) > > { > > if (pb->chassis != chassis_rec) { > > if (sb_readonly) { > > @@ -709,6 +823,10 @@ claim_lport(const struct sbrec_port_binding *pb, > > } > > > > sbrec_port_binding_set_chassis(pb, chassis_rec); > > + > > + if (tracked_datapaths) { > > + update_lport_tracking(pb, false, true, > tracked_datapaths); > > + } > > } > > > > /* Check if the port encap binding, if any, has changed */ > > @@ -726,9 +844,14 @@ claim_lport(const struct sbrec_port_binding *pb, > > > > /* Returns false if lport is not released due to 'sb_readonly'. > > * Returns true otherwise. > > + * > > + * This function assumes that that the the 'pb' was claimed > > + * earlier i.e port binding's chassis is set to this chassis. > > + * Caller should make sure that, that's the case. > > Can we skip "that, " above? (non-native speaker here). > > > Ack. > > > > > */ > > static bool > > -release_lport(const struct sbrec_port_binding *pb, bool sb_readonly) > > +release_lport(const struct sbrec_port_binding *pb, bool sb_readonly, > > + struct hmap *tracked_datapaths) > > { > > if (pb->encap) { > > if (sb_readonly) { > > @@ -751,6 +874,7 @@ release_lport(const struct sbrec_port_binding > *pb, bool sb_readonly) > > sbrec_port_binding_set_virtual_parent(pb, NULL); > > } > > > > + update_lport_tracking(pb, true, false, tracked_datapaths); > > VLOG_INFO("Releasing lport %s from this chassis.", > pb->logical_port); > > return true; > > } > > @@ -796,13 +920,14 @@ is_lbinding_container_parent(struct > local_binding *lbinding) > > static bool > > release_local_binding_children(const struct sbrec_chassis > *chassis_rec, > > struct local_binding *lbinding, > > - bool sb_readonly) > > + bool sb_readonly, > > + struct hmap *tracked_dp_bindings) > > { > > struct shash_node *node; > > SHASH_FOR_EACH (node, &lbinding->children) { > > struct local_binding *l = node->data; > > if (is_lbinding_this_chassis(l, chassis_rec)) { > > - if (!release_lport(l->pb, sb_readonly)) { > > + if (!release_lport(l->pb, sb_readonly, > tracked_dp_bindings)) { > > return false; > > } > > } > > @@ -817,16 +942,17 @@ release_local_binding_children(const struct > sbrec_chassis *chassis_rec, > > > > static bool > > release_local_binding(const struct sbrec_chassis *chassis_rec, > > - struct local_binding *lbinding, bool > sb_readonly) > > + struct local_binding *lbinding, bool > sb_readonly, > > + struct hmap *tracked_dp_bindings) > > { > > if (!release_local_binding_children(chassis_rec, lbinding, > > - sb_readonly)) { > > + sb_readonly, > tracked_dp_bindings)) { > > return false; > > } > > > > bool retval = true; > > if (is_lbinding_this_chassis(lbinding, chassis_rec)) { > > - retval = release_lport(lbinding->pb, sb_readonly); > > + retval = release_lport(lbinding->pb, sb_readonly, > tracked_dp_bindings); > > } > > > > lbinding->pb = NULL; > > @@ -847,7 +973,8 @@ consider_vif_lport_(const struct > sbrec_port_binding *pb, > > if (can_bind) { > > /* We can claim the lport. */ > > if (!claim_lport(pb, b_ctx_in->chassis_rec, > lbinding->iface, > > - !b_ctx_in->ovnsb_idl_txn)){ > > + !b_ctx_in->ovnsb_idl_txn, > > + b_ctx_out->tracked_dp_bindings)){ > > return false; > > } > > > > @@ -855,8 +982,10 @@ consider_vif_lport_(const struct > sbrec_port_binding *pb, > > > b_ctx_in->sbrec_port_binding_by_datapath, > > b_ctx_in->sbrec_port_binding_by_name, > > pb->datapath, false, > > - b_ctx_out->local_datapaths); > > - update_local_lport_ids(b_ctx_out->local_lport_ids, pb); > > + b_ctx_out->local_datapaths, > > + b_ctx_out->tracked_dp_bindings); > > + update_local_lport_ids(b_ctx_out->local_lport_ids, pb, > > + b_ctx_out->tracked_dp_bindings); > > if (lbinding->iface && qos_map && > b_ctx_in->ovs_idl_txn) { > > get_qos_params(pb, qos_map); > > } > > @@ -875,7 +1004,8 @@ consider_vif_lport_(const struct > sbrec_port_binding *pb, > > if (pb->chassis == b_ctx_in->chassis_rec) { > > /* Release the lport if there is no lbinding. */ > > if (!lbinding_set || !can_bind) { > > - return release_lport(pb, !b_ctx_in->ovnsb_idl_txn); > > + return release_lport(pb, !b_ctx_in->ovnsb_idl_txn, > > + b_ctx_out->tracked_dp_bindings); > > } > > } > > > > @@ -962,7 +1092,8 @@ consider_container_lport(const struct > sbrec_port_binding *pb, > > * release the container lport, if it was bound > earlier. */ > > if (is_lbinding_this_chassis(container_lbinding, > > b_ctx_in->chassis_rec)) { > > - return release_lport(pb, !b_ctx_in->ovnsb_idl_txn); > > + return release_lport(pb, !b_ctx_in->ovnsb_idl_txn, > > + b_ctx_out->tracked_dp_bindings); > > } > > > > return true; > > @@ -1037,18 +1168,22 @@ consider_nonvif_lport_(const struct > sbrec_port_binding *pb, > > struct binding_ctx_out *b_ctx_out) > > { > > if (our_chassis) { > > - sset_add(b_ctx_out->local_lports, pb->logical_port); > > + add_lport_to_local_lports(b_ctx_out->local_lports, > pb->logical_port); > > add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key, > > b_ctx_in->sbrec_port_binding_by_datapath, > > b_ctx_in->sbrec_port_binding_by_name, > > pb->datapath, has_local_l3gateway, > > - b_ctx_out->local_datapaths); > > + b_ctx_out->local_datapaths, > > + b_ctx_out->tracked_dp_bindings); > > > > - update_local_lport_ids(b_ctx_out->local_lport_ids, pb); > > + update_local_lport_ids(b_ctx_out->local_lport_ids, pb, > > + b_ctx_out->tracked_dp_bindings); > > return claim_lport(pb, b_ctx_in->chassis_rec, NULL, > > - !b_ctx_in->ovnsb_idl_txn); > > + !b_ctx_in->ovnsb_idl_txn, > > + b_ctx_out->tracked_dp_bindings); > > } else if (pb->chassis == b_ctx_in->chassis_rec) { > > - return release_lport(pb, !b_ctx_in->ovnsb_idl_txn); > > + return release_lport(pb, !b_ctx_in->ovnsb_idl_txn, > > + b_ctx_out->tracked_dp_bindings); > > } > > > > return true; > > @@ -1084,14 +1219,15 @@ consider_localnet_lport(const struct > sbrec_port_binding *pb, > > struct binding_ctx_out *b_ctx_out, > > struct hmap *qos_map) > > { > > - /* Add all localnet ports to local_lports so that we allocate > ct zones > > + /* Add all localnet ports to local_ifaces so that we allocate > ct zones > > * for them. */ > > - sset_add(b_ctx_out->local_lports, pb->logical_port); > > + add_lport_to_local_lports(b_ctx_out->local_lports, > pb->logical_port); > > if (qos_map && b_ctx_in->ovs_idl_txn) { > > get_qos_params(pb, qos_map); > > } > > > > - update_local_lport_ids(b_ctx_out->local_lport_ids, pb); > > + update_local_lport_ids(b_ctx_out->local_lport_ids, pb, > > + b_ctx_out->tracked_dp_bindings); > > } > > > > static bool > > @@ -1119,7 +1255,10 @@ consider_ha_lport(const struct > sbrec_port_binding *pb, > > b_ctx_in->sbrec_port_binding_by_datapath, > > b_ctx_in->sbrec_port_binding_by_name, > > pb->datapath, false, > > - b_ctx_out->local_datapaths); > > + b_ctx_out->local_datapaths, > > + b_ctx_out->tracked_dp_bindings); > > + update_local_lport_ids(b_ctx_out->local_lport_ids, pb, > > + b_ctx_out->tracked_dp_bindings); > > } > > > > return consider_nonvif_lport_(pb, our_chassis, false, > b_ctx_in, b_ctx_out); > > @@ -1185,7 +1324,7 @@ build_local_bindings(struct binding_ctx_in > *b_ctx_in, > > ovs_assert(lbinding->type == BT_VIF); > > } > > > > - sset_add(b_ctx_out->local_lports, iface_id); > > + > add_lport_to_local_lports(b_ctx_out->local_lports, iface_id); > > smap_replace(b_ctx_out->local_iface_ids, > iface_rec->name, > > iface_id); > > } > > @@ -1241,7 +1380,7 @@ binding_run(struct binding_ctx_in *b_ctx_in, > struct binding_ctx_out *b_ctx_out) > > case LP_PATCH: > > case LP_LOCALPORT: > > case LP_VTEP: > > - update_local_lport_ids(b_ctx_out->local_lport_ids, pb); > > + update_local_lport_ids(b_ctx_out->local_lport_ids, > pb, NULL); > > break; > > > > case LP_VIF: > > @@ -1409,7 +1548,8 @@ add_local_datapath_peer_port(const struct > sbrec_port_binding *pb, > > > b_ctx_in->sbrec_port_binding_by_datapath, > > b_ctx_in->sbrec_port_binding_by_name, > > peer->datapath, false, > > - 1, b_ctx_out->local_datapaths); > > + 1, b_ctx_out->local_datapaths, > > + b_ctx_out->tracked_dp_bindings); > > return; > > } > > > > @@ -1471,7 +1611,8 @@ remove_pb_from_local_datapath(const struct > sbrec_port_binding *pb, > > struct binding_ctx_out *b_ctx_out, > > struct local_datapath *ld) > > { > > - remove_local_lport_ids(pb, b_ctx_out->local_lport_ids); > > + remove_local_lport_ids(pb, b_ctx_out->local_lport_ids, > > + b_ctx_out->tracked_dp_bindings); > > if (!strcmp(pb->type, "patch") || > > !strcmp(pb->type, "l3gateway")) { > > remove_local_datapath_peer_port(pb, ld, > b_ctx_out->local_datapaths); > > @@ -1489,6 +1630,18 @@ remove_pb_from_local_datapath(const struct > sbrec_port_binding *pb, > > } > > } > > > > +static void > > +update_lport_tracking(const struct sbrec_port_binding *pb, > > + bool old_claim, bool new_claim, > > + struct hmap *tracked_dp_bindings) > > +{ > > + if (!tracked_dp_bindings || !pb || (old_claim == new_claim)) { > > I think pb can never be NULL here, right? > > > Yeah. I'll remove the check. > > > > > + return; > > + } > > + > > + tracked_binding_datapath_lport_add(pb, old_claim, > tracked_dp_bindings); > > +} > > + > > /* Considers the ovs iface 'iface_rec' for claiming. > > * This function should be called if the external_ids:iface-id > > * and 'ofport' are set for the 'iface_rec'. > > @@ -1501,9 +1654,13 @@ consider_iface_claim(const struct > ovsrec_interface *iface_rec, > > const char *iface_id, > > struct binding_ctx_in *b_ctx_in, > > struct binding_ctx_out *b_ctx_out, > > - struct hmap *qos_map) > > + struct hmap *qos_map, > > + bool *local_lports_changed) > > { > > - sset_add(b_ctx_out->local_lports, iface_id); > > + if (add_lport_to_local_lports(b_ctx_out->local_lports, > iface_id)) { > > + *local_lports_changed = true; > > + } > > + > > smap_replace(b_ctx_out->local_iface_ids, iface_rec->name, > iface_id); > > > > struct local_binding *lbinding = > > @@ -1566,7 +1723,8 @@ static bool > > consider_iface_release(const struct ovsrec_interface *iface_rec, > > const char *iface_id, > > struct binding_ctx_in *b_ctx_in, > > - struct binding_ctx_out *b_ctx_out, bool > *changed) > > + struct binding_ctx_out *b_ctx_out, bool > *changed, > > + bool *local_lports_changed) > > { > > struct local_binding *lbinding; > > lbinding = local_binding_find(b_ctx_out->local_bindings, > > @@ -1585,7 +1743,8 @@ consider_iface_release(const struct > ovsrec_interface *iface_rec, > > * lbinding->iface. > > * Cannot access these members of lbinding after this > call. */ > > if (!release_local_binding(b_ctx_in->chassis_rec, lbinding, > > - !b_ctx_in->ovnsb_idl_txn)) { > > + !b_ctx_in->ovnsb_idl_txn, > > + b_ctx_out->tracked_dp_bindings)) { > > return false; > > } > > > > @@ -1598,7 +1757,9 @@ consider_iface_release(const struct > ovsrec_interface *iface_rec, > > local_binding_delete(b_ctx_out->local_bindings, lbinding); > > } > > > > - sset_find_and_delete(b_ctx_out->local_lports, iface_id); > > + if (remove_lport_from_local_lports(b_ctx_out->local_lports, > iface_id)) { > > + *local_lports_changed = true; > > + } > > smap_remove(b_ctx_out->local_iface_ids, iface_rec->name); > > > > return true; > > @@ -1630,6 +1791,8 @@ binding_handle_ovs_interface_changes(struct > binding_ctx_in *b_ctx_in, > > bool handled = true; > > *changed = false; > > > > + *b_ctx_out->local_lports_changed = false; > > + > > Is this needed? This is part of the data we clear at every run in > en_runtime_data_clear_tracked_data(). > > > I still feel it's better to set it to false here. This function doesn't > know anything about > the tracked data. > > Ok, then the reverse question, do we still need to clear local_lports_changed in en_runtime_data_clear_tracked_data()? > > > > /* Run the tracked interfaces loop twice. One to handle deleted > > * changes. And another to handle add/update changes. > > * This will ensure correctness. > > @@ -1685,7 +1848,8 @@ binding_handle_ovs_interface_changes(struct > binding_ctx_in *b_ctx_in, > > > > if (cleared_iface_id) { > > handled = consider_iface_release(iface_rec, > cleared_iface_id, > > - b_ctx_in, b_ctx_out, > changed); > > + b_ctx_in, b_ctx_out, > changed, > > + > b_ctx_out->local_lports_changed); > > } > > > > if (!handled) { > > @@ -1727,7 +1891,8 @@ binding_handle_ovs_interface_changes(struct > binding_ctx_in *b_ctx_in, > > if (iface_id && ofport > 0) { > > *changed = true; > > handled = consider_iface_claim(iface_rec, iface_id, > b_ctx_in, > > - b_ctx_out, qos_map_ptr); > > + b_ctx_out, qos_map_ptr, > > + > b_ctx_out->local_lports_changed); > > if (!handled) { > > break; > > } > > @@ -1792,8 +1957,7 @@ static bool > > handle_deleted_vif_lport(const struct sbrec_port_binding *pb, > > enum en_lport_type lport_type, > > struct binding_ctx_in *b_ctx_in, > > - struct binding_ctx_out *b_ctx_out, > > - bool *changed) > > + struct binding_ctx_out *b_ctx_out) > > { > > struct local_binding *lbinding = > > get_lbinding_for_lport(pb, lport_type, b_ctx_out); > > @@ -1804,13 +1968,12 @@ handle_deleted_vif_lport(const struct > sbrec_port_binding *pb, > > * clear the 'chassis' column of 'pb'. But we need to do > > * for the local_binding's children. */ > > if (lbinding->type == BT_VIF && > > - > !release_local_binding_children(b_ctx_in->chassis_rec, > > - lbinding, > > - > !b_ctx_in->ovnsb_idl_txn)) { > > + !release_local_binding_children( > > + b_ctx_in->chassis_rec, lbinding, > > + !b_ctx_in->ovnsb_idl_txn, > > + b_ctx_out->tracked_dp_bindings)) { > > return false; > > } > > - > > - *changed = true; > > } > > > > handle_deleted_lport(pb, b_ctx_in, b_ctx_out); > > @@ -1822,8 +1985,7 @@ handle_updated_vif_lport(const struct > sbrec_port_binding *pb, > > enum en_lport_type lport_type, > > struct binding_ctx_in *b_ctx_in, > > struct binding_ctx_out *b_ctx_out, > > - struct hmap *qos_map, > > - bool *changed) > > + struct hmap *qos_map) > > { > > bool claimed = (pb->chassis == b_ctx_in->chassis_rec); > > bool handled = true; > > @@ -1841,14 +2003,10 @@ handle_updated_vif_lport(const struct > sbrec_port_binding *pb, > > } > > > > bool now_claimed = (pb->chassis == b_ctx_in->chassis_rec); > > - bool changed_ = (claimed != now_claimed); > > - > > - if (changed_) { > > - *changed = true; > > - } > > + bool changed = (claimed != now_claimed); > > > > if (lport_type == LP_VIRTUAL || > > - (lport_type == LP_VIF && is_lport_container(pb)) || > !changed_) { > > + (lport_type == LP_VIF && is_lport_container(pb)) || > !changed) { > > return true; > > } > > > > @@ -1898,7 +2056,7 @@ binding_handle_port_binding_changes(struct > binding_ctx_in *b_ctx_in, > > enum en_lport_type lport_type = get_lport_type(pb); > > if (lport_type == LP_VIF || lport_type == LP_VIRTUAL) { > > handled = handle_deleted_vif_lport(pb, lport_type, > b_ctx_in, > > - b_ctx_out, changed); > > + b_ctx_out); > > } else { > > handle_deleted_lport(pb, b_ctx_in, b_ctx_out); > > } > > @@ -1933,15 +2091,14 @@ binding_handle_port_binding_changes(struct > binding_ctx_in *b_ctx_in, > > case LP_VIF: > > case LP_VIRTUAL: > > handled = handle_updated_vif_lport(pb, lport_type, > b_ctx_in, > > - b_ctx_out, > qos_map_ptr, > > - changed); > > + b_ctx_out, > qos_map_ptr); > > break; > > > > case LP_PATCH: > > case LP_LOCALPORT: > > case LP_VTEP: > > - *changed = true; > > - update_local_lport_ids(b_ctx_out->local_lport_ids, pb); > > + update_local_lport_ids(b_ctx_out->local_lport_ids, pb, > > + b_ctx_out->tracked_dp_bindings); > > if (lport_type == LP_PATCH) { > > /* Add the peer datapath to the local datapaths > if it's > > * not present yet. > > @@ -1950,30 +2107,32 @@ binding_handle_port_binding_changes(struct > binding_ctx_in *b_ctx_in, > > add_local_datapath_peer_port(pb, b_ctx_in, > b_ctx_out, ld); > > } > > } > > + > > + if (lport_type == LP_VTEP) { > > + /* VTEP lports are claimed/released by > ovn-controller-vteps. > > + * We are not sure what changed. So set *changed > to true > > + * for any changes to vtep lports. */ > > + *changed = true; > > + } > > break; > > > > case LP_L2GATEWAY: > > - *changed = true; > > handled = consider_l2gw_lport(pb, b_ctx_in, b_ctx_out); > > break; > > > > case LP_L3GATEWAY: > > - *changed = true; > > handled = consider_l3gw_lport(pb, b_ctx_in, b_ctx_out); > > break; > > > > case LP_CHASSISREDIRECT: > > - *changed = true; > > handled = consider_cr_lport(pb, b_ctx_in, b_ctx_out); > > break; > > > > case LP_EXTERNAL: > > - *changed = true; > > handled = consider_external_lport(pb, b_ctx_in, > b_ctx_out); > > break; > > > > case LP_LOCALNET: { > > - *changed = true; > > consider_localnet_lport(pb, b_ctx_in, b_ctx_out, > qos_map_ptr); > > > > struct shash bridge_mappings = > > @@ -2008,6 +2167,10 @@ binding_handle_port_binding_changes(struct > binding_ctx_in *b_ctx_in, > > } > > } > > > > + if (handled && !hmap_is_empty(b_ctx_out->tracked_dp_bindings)) { > > + *changed = true; > > + } > > + > > destroy_qos_map(&qos_map); > > return handled; > > } > > diff --git a/controller/binding.h b/controller/binding.h > > index 21118ecd4..2974ebb30 100644 > > --- a/controller/binding.h > > +++ b/controller/binding.h > > @@ -19,6 +19,9 @@ > > > > #include <stdbool.h> > > #include "openvswitch/shash.h" > > +#include "openvswitch/hmap.h" > > +#include "openvswitch/uuid.h" > > +#include "openvswitch/list.h" > > > > struct hmap; > > struct ovsdb_idl; > > @@ -54,10 +57,28 @@ struct binding_ctx_in { > > struct binding_ctx_out { > > struct hmap *local_datapaths; > > struct shash *local_bindings; > > + > > struct sset *local_lports; > > + > > + /* If the sset local_lports is modified, this is set to true by > > + * the callee. */ > > + bool *local_lports_changed; > > + > > + /* sset of local lport ids in the format > > + * <datapath-tunnel-key>_<port-tunnel-key>. */ > > Nit: shouldn't this comment be part of patch 1? > > > Could be. > > > > > struct sset *local_lport_ids; > > + > > struct sset *egress_ifaces; > > + > > + /* smap of OVS interface name as key and > > + * OVS interface external_ids:iface-id as value. */ > > Nit: shouldn't this comment be part of patch 2? > > > Could be. > > > > struct smap *local_iface_ids; > > + > > + /* hmap of 'struct tracked_binding_datapath' which the > > + * callee (binding_handle_ovs_interface_changes and > > + * binding_handle_port_binding_changes) fills in for > > + * the changed datapaths and port bindings. */ > > + struct hmap *tracked_dp_bindings; > > }; > > > > enum local_binding_type { > > @@ -82,6 +103,21 @@ local_binding_find(struct shash > *local_bindings, const char *name) > > return shash_find_data(local_bindings, name); > > } > > > > +/* Represents a tracked binding logical port. */ > > +struct tracked_binding_lport { > > + const struct sbrec_port_binding *pb; > > + struct ovs_list list_node; > > Nit: for consistency with tracked_binding_datapath and because list_node > is usually accessed before pb I'd define list_node before pb. > > > Actually we don't need list_node now. It was a leftover from the > previous versions. > I'll delete it. Thanks. > > > > > + bool deleted; > > +}; > > + > > +/* Represent a tracked binding datapath. */ > > +struct tracked_binding_datapath { > > + struct hmap_node node; > > + const struct sbrec_datapath_binding *dp; > > + bool is_new; > > I think it would be easier to read the code if both > tracked_binding_lport and tracked_binding_datapath would use the same > logic for tracking updates: either both should have "deleted" or both > should have "is_new". > > > Ack. > > > > Also, given that we never use tracked_binding_lport.deleted, maybe we > can skip it completely. What do you think? > > > I'll delete the "deleted". We can add it later if it is really required. > > > > > + struct shash lports; /* shash of struct tracked_binding_lport. */ > > +}; > > + > > void binding_register_ovs_idl(struct ovsdb_idl *); > > void binding_run(struct binding_ctx_in *, struct binding_ctx_out *); > > bool binding_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn, > > @@ -96,4 +132,5 @@ bool > binding_handle_ovs_interface_changes(struct binding_ctx_in *, > > bool binding_handle_port_binding_changes(struct binding_ctx_in *, > > struct binding_ctx_out *, > > bool *changed); > > +void binding_tracked_dp_destroy(struct hmap *tracked_datapaths); > > #endif /* controller/binding.h */ > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index 795a1c297..98652866c 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -973,10 +973,88 @@ struct ed_type_runtime_data { > > struct sset local_lport_ids; > > struct sset active_tunnels; > > > > + /* runtime data engine private data. */ > > struct sset egress_ifaces; > > struct smap local_iface_ids; > > + > > + /* Tracked data. See below for more details and comments. */ > > + bool tracked; > > I wonder if the "tracked" field should actually be part of the generic > engine_node_input. We use it for port groups and address sets too > ("change_tracked" in ed_type_addr_sets and ed_type_port_groups) and > afaiu the semantics are always: > - "tracked" is set to true if a node's input handler was called > - "tracked" is reset to false if a node's run handler was called > > So all this could be implemented generically in inc-proc-eng.c. > > What do you think? > > > Could be. But I'd prefer to be a separate patch (not part of this series). > Ok, but please see my comment regarding en_runtime_data_clear_tracked_data() in en_runtime_data_run(). > > > > + bool local_lports_changed; > > Do we really need local_lports_changed? Isn't it actually equivalent to > !hmap_is_empty(& > > > + struct hmap tracked_dp_bindings; > > }; > > > Not really. > > Let's say you run the command - "ovs-vsctl add-port br-int p0 -- set > interface p0 externa_ids:iface-id=lport1" > > And if logical lport lport1 doesn't exist (or not seen by ovn-controller > due to conditional monitoring), we > need to update the runtime_data engine node so that "update_sb_monitor" > is called. > > Ah, right, thanks for the explanation. > > > > > +/* struct ed_type_runtime_data has the below members for tracking the > > + * changes done to the runtime_data engine by the runtime_data engine > > + * handlers. Since this engine is an input to the flow_output engine, > > + * the flow output runtime data handler will make use of this > tracked data. > > + * > > + * > ------------------------------------------------------------------------ > > + * | | This is a hmap of > | > > + * | | 'struct tracked_binding_datapath' > defined in | > > + * | | binding.h. Runtime data handlers for > OVS | > > + * | | Interface and Port Binding changes > store the | > > + * | @tracked_dp_bindings | changed datapaths (datapaths > added/removed from | > > + * | | local_datapaths) and changed port > bindings | > > + * | | (added/updated/deleted in > 'local_bindings'). | > > This is not really accurate, right? Afaiu we only track if a port > binding is added/deleted. > > > Not all the time. We can bind an "external" port when the port binding gets > updated due to the command - "ovn-nbctl lsp-set-type <lport> external" > or for the command - "ovn-nbctl lsp-set-options router-port=<peer port> > > OK. > > > > > + * | | So any changes to the runtime data - > | > > + * | | local_datapaths and local_bindings is > captured | > > + * | | here. > | > > + * > ------------------------------------------------------------------------ > > + * | | This is a bool which represents if > the runtime | > > + * | | data 'local_lports' changed by the > runtime data | > > + * | | handlers for OVS Interface and Port > Binding | > > + * | | changes. If 'local_lports' is updated > and also | > > + * | | results in any port binding updates, > it is | > > + * |@local_lports_changed | captured in the @tracked_dp_bindings. > So there | > > + * | | is no need to capture the changes in > the | > > + * | | local_lports. If > @local_lports_changed is true | > > + * | | but without anydata in the > @tracked_dp_bindings,| > > + * | | it means we needto only update the SB > monitor | > > + * | | clauses and there isno need for any > flow | > > + * | | (re)computations. > | > > + * > ------------------------------------------------------------------------ > > + * | | This represents if the data was > tracked or not | > > + * | | by the runtime data handlers during > the engine | > > + * | @tracked | run. If the runtime data recompute > is | > > + * | | triggered, it means there is no > tracked data. | > > + * > ------------------------------------------------------------------------ > > + * > > + * > > + * The changes to the following runtime_data variables are not > tracked. > > + * > > + * > --------------------------------------------------------------------- > > + * | local_datapaths | The changes to these runtime data is > captured in | > > + * | local_bindings | the @tracked_dp_bindings indirectly and > hence it | > > + * | local_lport_ids | is not tracked explicitly. > | > > + * > --------------------------------------------------------------------- > > + * | local_iface_ids | This is used internally within the > runtime data | > > + * | egress_ifaces | engine (used only in binding.c) and hence > there | > > + * | | there is no need to track. > | > > + * > --------------------------------------------------------------------- > > + * | | Active tunnels is built in the > | > > + * | | bfd_calculate_active_tunnels() for the > tunnel | > > + * | | OVS interfaces. Any changes to non VIF > OVS | > > + * | | interfaces results in triggering the > full | > > + * | active_tunnels | recompute of runtime data engine and > hence there | > > + * | | the tracked data doesn't track it. When > we | > > + * | | support handling changes to non VIF OVS > | > > + * | | interfaces we need to track the changes > to the | > > + * | | active tunnels. > | > > + * > --------------------------------------------------------------------- > > + * > > + */ > > + > > +static void > > +en_runtime_data_clear_tracked_data(void *data_) > > +{ > > + struct ed_type_runtime_data *data = data_; > > + > > + binding_tracked_dp_destroy(&data->tracked_dp_bindings); > > + hmap_init(&data->tracked_dp_bindings); > > Nit: just as we did for local_bindings_init()/local_bindings_destroy() > I'd move the "hmap_init(&data->tracked_dp_bindings)" inside a new > function called binding_tracked_dp_init(). > > > Not sure if we want to have the same functions here just to init the hmap. > > Well, local_bindings_init(), just does shash_init() so I don't really see why not :) > > > + data->local_lports_changed = false; > > + data->tracked = false; > > +} > > + > > static void * > > en_runtime_data_init(struct engine_node *node OVS_UNUSED, > > struct engine_arg *arg OVS_UNUSED) > > @@ -990,6 +1068,10 @@ en_runtime_data_init(struct engine_node > *node OVS_UNUSED, > > sset_init(&data->egress_ifaces); > > smap_init(&data->local_iface_ids); > > local_bindings_init(&data->local_bindings); > > + > > + /* init the tracked data. */ > > + hmap_init(&data->tracked_dp_bindings); > > + > > return data; > > } > > > > @@ -1012,6 +1094,8 @@ en_runtime_data_cleanup(void *data) > > } > > hmap_destroy(&rt_data->local_datapaths); > > local_bindings_destroy(&rt_data->local_bindings); > > + > > + en_runtime_data_clear_tracked_data(data); > > } > > > > static void > > @@ -1092,6 +1176,8 @@ init_binding_ctx(struct engine_node *node, > > b_ctx_out->egress_ifaces = &rt_data->egress_ifaces; > > b_ctx_out->local_bindings = &rt_data->local_bindings; > > b_ctx_out->local_iface_ids = &rt_data->local_iface_ids; > > + b_ctx_out->tracked_dp_bindings = NULL; > > + b_ctx_out->local_lports_changed = NULL; > > } > > > > static void > > @@ -1103,6 +1189,13 @@ en_runtime_data_run(struct engine_node > *node, void *data) > > struct sset *local_lport_ids = &rt_data->local_lport_ids; > > struct sset *active_tunnels = &rt_data->active_tunnels; > > > > + /* Clear the tracked data. Even though the tracked data > > + * gets cleared in the beginning of engine_init_run(), > > + * any of the runtime data handler might have set some tracked > > + * data and later another runtime data handler might return false > > + * resulting full recompute of runtime engine. */ > > + en_runtime_data_clear_tracked_data(data); > > + > > Could you please elaborate more on this? Why is it an issue if we keep > the "tracked" data until the end of the run? > > > > When a runtime data node gets updated (engine_set_node_state()) either > due to > full recompute of runtime or due to handler setting it, > flow_output_runtime_data_handler() > will not trigger flow_output recompute unless it see runtime->tracked = > false. > > Hence it clears the tracked data. Instead of calling > en_runtime_data_clear_tracked_data(), > en_runtime_data_run() can set just 'tracked' to false, but I think it's > better to clear > the whole tracked data. > So this brings us back to my original comment regarding "tracked". If tracked would be part of the I-P engine internal input node fields, it would get set before running en_runtime_data_run() and we wouldn't have to explain why we need to clear the "tracked" field here. If we decide to address the "tracked" fields as a follow up patch I think it'd be good to add some TODO/FIXME/XXX comments so we don't forget about addressing this later. E.g., there's already " /* XXX: The change_tracked check may be added to inc-proc framework. */" for address sets. Thanks, Dumitru > > Thanks, > Dumitru > > > static bool first_run = true; > > if (first_run) { > > /* don't cleanup since there is no data yet */ > > @@ -1158,6 +1251,9 @@ runtime_data_ovs_interface_handler(struct > engine_node *node, void *data) > > struct binding_ctx_in b_ctx_in; > > struct binding_ctx_out b_ctx_out; > > init_binding_ctx(node, rt_data, &b_ctx_in, &b_ctx_out); > > + rt_data->tracked = true; > > + b_ctx_out.tracked_dp_bindings = &rt_data->tracked_dp_bindings; > > + b_ctx_out.local_lports_changed = &rt_data->local_lports_changed; > > > > bool changed = false; > > if (!binding_handle_ovs_interface_changes(&b_ctx_in, &b_ctx_out, > > @@ -1190,6 +1286,9 @@ runtime_data_sb_port_binding_handler(struct > engine_node *node, void *data) > > return false; > > } > > > > + rt_data->tracked = true; > > + b_ctx_out.tracked_dp_bindings = &rt_data->tracked_dp_bindings; > > + > > bool changed = false; > > if (!binding_handle_port_binding_changes(&b_ctx_in, &b_ctx_out, > > &changed)) { > > @@ -1797,6 +1896,39 @@ flow_output_port_groups_handler(struct > engine_node *node, void *data) > > return _flow_output_resource_ref_handler(node, data, > REF_TYPE_PORTGROUP); > > } > > > > +static bool > > +flow_output_runtime_data_handler(struct engine_node *node, > > + void *data OVS_UNUSED) > > +{ > > + struct ed_type_runtime_data *rt_data = > > + engine_get_input_data("runtime_data", node); > > + > > + /* There is no tracked data. Fall back to full recompute of > > + * flow_output. */ > > + if (!rt_data->tracked) { > > + return false; > > + } > > + > > + if (!hmap_is_empty(&rt_data->tracked_dp_bindings)) { > > + /* We are not yet handling the tracked datapath binding > > + * changes. Return false to trigger full recompute. */ > > + return false; > > + } > > + > > + if (rt_data->local_lports_changed) { > > + engine_set_node_state(node, EN_UPDATED); > > + } > > + > > + return true; > > +} > > + > > +static bool > > +flow_output_noop_handler(struct engine_node *node OVS_UNUSED, > > + void *data OVS_UNUSED) > > +{ > > + return true; > > +} > > + > > /* Engine node en_physical_flow_changes indicates whether > > * there is a need to > > * - recompute only physical flows or > > @@ -1908,13 +2040,6 @@ > flow_output_physical_flow_changes_handler(struct engine_node *node, > void *data) > > return true; > > } > > > > -static bool > > -flow_output_noop_handler(struct engine_node *node OVS_UNUSED, > > - void *data OVS_UNUSED) > > -{ > > - return true; > > -} > > - > > struct ovn_controller_exit_args { > > bool *exiting; > > bool *restart; > > @@ -2036,7 +2161,7 @@ main(int argc, char *argv[]) > > > > /* Define inc-proc-engine nodes. */ > > ENGINE_NODE_CUSTOM_DATA(ct_zones, "ct_zones"); > > - ENGINE_NODE(runtime_data, "runtime_data"); > > + ENGINE_NODE_WITH_CLEAR_TRACK_DATA(runtime_data, "runtime_data"); > > ENGINE_NODE(mff_ovn_geneve, "mff_ovn_geneve"); > > ENGINE_NODE(ofctrl_is_connected, "ofctrl_is_connected"); > > ENGINE_NODE_WITH_CLEAR_TRACK_DATA(physical_flow_changes, > > @@ -2073,7 +2198,8 @@ main(int argc, char *argv[]) > > flow_output_addr_sets_handler); > > engine_add_input(&en_flow_output, &en_port_groups, > > flow_output_port_groups_handler); > > - engine_add_input(&en_flow_output, &en_runtime_data, NULL); > > + engine_add_input(&en_flow_output, &en_runtime_data, > > + flow_output_runtime_data_handler); > > engine_add_input(&en_flow_output, &en_mff_ovn_geneve, NULL); > > engine_add_input(&en_flow_output, &en_physical_flow_changes, > > flow_output_physical_flow_changes_handler); > > diff --git a/tests/ovn-performance.at <http://ovn-performance.at> > b/tests/ovn-performance.at <http://ovn-performance.at> > > index 5cc1960b6..08acd3bae 100644 > > --- a/tests/ovn-performance.at <http://ovn-performance.at> > > +++ b/tests/ovn-performance.at <http://ovn-performance.at> > > @@ -286,11 +286,11 @@ for i in 1 2; do > > [hv1 hv2], [lflow_run], > > [ovn-nbctl --wait=hv lsp-set-type $lsp router] > > ) > > - OVN_CONTROLLER_EXPECT_HIT( > > + OVN_CONTROLLER_EXPECT_NO_HIT( > > [hv1 hv2], [lflow_run], > > [ovn-nbctl --wait=hv lsp-set-options $lsp router-port=$lrp] > > ) > > - OVN_CONTROLLER_EXPECT_HIT( > > + OVN_CONTROLLER_EXPECT_NO_HIT( > > [hv1 hv2], [lflow_run], > > [ovn-nbctl --wait=hv lsp-set-addresses $lsp router] > > ) > > @@ -416,15 +416,15 @@ OVN_CONTROLLER_EXPECT_NO_HIT( > > [ovn-nbctl --wait=hv destroy Port_Group pg1] > > ) > > > > -for i in 1 2; do > > - ls=ls$i > > +OVN_CONTROLLER_EXPECT_HIT( > > + [hv1 hv2], [lflow_run], > > + [ovn-nbctl --wait=hv ls-del ls1] > > +) > > > > - # Delete switch $ls > > - OVN_CONTROLLER_EXPECT_HIT( > > - [hv1 hv2], [lflow_run], > > - [ovn-nbctl --wait=hv ls-del $ls] > > - ) > > -done > > +OVN_CONTROLLER_EXPECT_NO_HIT( > > + [hv1 hv2], [lflow_run], > > + [ovn-nbctl --wait=hv ls-del ls2] > > +) > > > > # Delete router lr1 > > OVN_CONTROLLER_EXPECT_NO_HIT( > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org <mailto:dev@openvswitch.org> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Tue, Jun 9, 2020 at 6:33 PM Dumitru Ceara <dceara@redhat.com> wrote: > On 6/9/20 1:00 PM, Numan Siddique wrote: > > > > > > On Tue, Jun 9, 2020 at 3:11 PM Dumitru Ceara <dceara@redhat.com > > <mailto:dceara@redhat.com>> wrote: > > > > On 6/8/20 3:50 PM, numans@ovn.org <mailto:numans@ovn.org> wrote: > > > From: Numan Siddique <numans@ovn.org <mailto:numans@ovn.org>> > > > > > > In order to handle runtime data changes incrementally, the flow > outut > > > runtime data handle should know the changed runtime data. > > > Runtime data now tracks the changed data for any OVS interface > > > and SB port binding changes. The tracked data contains a hmap > > > of tracked datapaths (which changed during runtime data processing. > > > > > > The flow outout runtime_data handler in this patch doesn't do much > > > with the tracked data. It returns false if there is tracked data > > available > > > so that flow_output run is called. If no tracked data is available > > > then there is no need for flow computation and the handler returns > > true. > > > > > > Next patch in the series processes the tracked data incrementally. > > > > > > Co-Authored-by: Venkata Anil <anilvenkata@redhat.com > > <mailto:anilvenkata@redhat.com>> > > > Signed-off-by: Venkata Anil <anilvenkata@redhat.com > > <mailto:anilvenkata@redhat.com>> > > > Signed-off-by: Numan Siddique <numans@ovn.org <mailto: > numans@ovn.org>> > > > --- > > > controller/binding.c | 299 > > ++++++++++++++++++++++++++++-------- > > > controller/binding.h | 37 +++++ > > > controller/ovn-controller.c | 144 +++++++++++++++-- > > > tests/ovn-performance.at <http://ovn-performance.at> | 20 +-- > > > 4 files changed, 413 insertions(+), 87 deletions(-) > > > > > > diff --git a/controller/binding.c b/controller/binding.c > > > index c2d9a4c6b..3c226cd7d 100644 > > > --- a/controller/binding.c > > > +++ b/controller/binding.c > > > @@ -69,13 +69,26 @@ binding_register_ovs_idl(struct ovsdb_idl > > *ovs_idl) > > > ovsdb_idl_add_column(ovs_idl, &ovsrec_qos_col_type); > > > } > > > > > > +static struct tracked_binding_datapath > > *tracked_binding_datapath_create( > > > + const struct sbrec_datapath_binding *, > > > + bool is_new, struct hmap *tracked_dps); > > > +static struct tracked_binding_datapath > > *tracked_binding_datapath_find( > > > + struct hmap *, const struct sbrec_datapath_binding *); > > > +static void tracked_binding_datapath_lport_add( > > > + const struct sbrec_port_binding *, bool deleted, > > > + struct hmap *tracked_datapaths); > > > +static void update_lport_tracking(const struct sbrec_port_binding > > *pb, > > > + bool old_claim, bool new_claim, > > > + struct hmap > *tracked_dp_bindings); > > > + > > > static void > > > add_local_datapath__(struct ovsdb_idl_index > > *sbrec_datapath_binding_by_key, > > > struct ovsdb_idl_index > > *sbrec_port_binding_by_datapath, > > > struct ovsdb_idl_index > > *sbrec_port_binding_by_name, > > > const struct sbrec_datapath_binding > *datapath, > > > bool has_local_l3gateway, int depth, > > > - struct hmap *local_datapaths) > > > + struct hmap *local_datapaths, > > > + struct hmap *tracked_datapaths) > > > { > > > uint32_t dp_key = datapath->tunnel_key; > > > struct local_datapath *ld = > > get_local_datapath(local_datapaths, dp_key); > > > @@ -92,6 +105,11 @@ add_local_datapath__(struct ovsdb_idl_index > > *sbrec_datapath_binding_by_key, > > > ld->localnet_port = NULL; > > > ld->has_local_l3gateway = has_local_l3gateway; > > > > > > + if (tracked_datapaths && > > > + !tracked_binding_datapath_find(tracked_datapaths, > > datapath)) { > > > + tracked_binding_datapath_create(datapath, true, > > tracked_datapaths); > > > + } > > > + > > > if (depth >= 100) { > > > static struct vlog_rate_limit rl = > > VLOG_RATE_LIMIT_INIT(1, 1); > > > VLOG_WARN_RL(&rl, "datapaths nested too deep"); > > > @@ -124,7 +142,8 @@ add_local_datapath__(struct ovsdb_idl_index > > *sbrec_datapath_binding_by_key, > > > > > sbrec_port_binding_by_datapath, > > > > > sbrec_port_binding_by_name, > > > peer->datapath, > false, > > > - depth + 1, > > local_datapaths); > > > + depth + 1, > > local_datapaths, > > > + tracked_datapaths); > > > } > > > ld->n_peer_ports++; > > > if (ld->n_peer_ports > > > ld->n_allocated_peer_ports) { > > > @@ -147,12 +166,14 @@ add_local_datapath(struct ovsdb_idl_index > > *sbrec_datapath_binding_by_key, > > > struct ovsdb_idl_index > > *sbrec_port_binding_by_datapath, > > > struct ovsdb_idl_index > > *sbrec_port_binding_by_name, > > > const struct sbrec_datapath_binding *datapath, > > > - bool has_local_l3gateway, struct hmap > > *local_datapaths) > > > + bool has_local_l3gateway, struct hmap > > *local_datapaths, > > > + struct hmap *tracked_datapaths) > > > { > > > add_local_datapath__(sbrec_datapath_binding_by_key, > > > sbrec_port_binding_by_datapath, > > > sbrec_port_binding_by_name, > > > - datapath, has_local_l3gateway, 0, > > local_datapaths); > > > + datapath, has_local_l3gateway, 0, > > local_datapaths, > > > + tracked_datapaths); > > > } > > > > > > static void > > > @@ -473,23 +494,45 @@ update_ld_localnet_port(const struct > > sbrec_port_binding *binding_rec, > > > > > > static void > > > update_local_lport_ids(struct sset *local_lport_ids, > > > - const struct sbrec_port_binding *pb) > > > + const struct sbrec_port_binding *pb, > > > + struct hmap *tracked_datapaths) > > > { > > > char buf[16]; > > > snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64, > > > pb->datapath->tunnel_key, pb->tunnel_key); > > > - sset_add(local_lport_ids, buf); > > > + bool added = sset_add(local_lport_ids, buf); > > > > Hi Numan, > > > > > > Hi Dumitru, > > > > Thanks for the review. > > > > Please see below the replies. > > > > Thanks > > Numan > > > > > > > > I guess this should be: > > > > bool added = !!sset_add(local_lport_ids, buf); > > > > > > Ack. > > > > > > > > > + if (added && tracked_datapaths) { > > > + /* Add the 'pb' to the tracked_datapaths. */ > > > + tracked_binding_datapath_lport_add(pb, false, > > tracked_datapaths); > > > + } > > > } > > > > > > static void > > > -remove_local_lport_ids(const struct sbrec_port_binding > *binding_rec, > > > - struct sset *local_lport_ids) > > > +remove_local_lport_ids(const struct sbrec_port_binding *pb, > > > + struct sset *local_lport_ids, > > > + struct hmap *tracked_datapaths) > > > { > > > char buf[16]; > > > snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64, > > > - binding_rec->datapath->tunnel_key, > > > - binding_rec->tunnel_key); > > > - sset_find_and_delete(local_lport_ids, buf); > > > + pb->datapath->tunnel_key, pb->tunnel_key); > > > + bool deleted = sset_find_and_delete(local_lport_ids, buf); > > > + if (deleted && tracked_datapaths) { > > > + /* Add the 'pb' to the tracked_datapaths. */ > > > + tracked_binding_datapath_lport_add(pb, true, > > tracked_datapaths); > > > + } > > > +} > > > + > > > +static bool > > > +add_lport_to_local_lports(struct sset *local_lports, const char > > *lport_name) > > > +{ > > > + return !!sset_add(local_lports, lport_name); > > > +} > > > + > > > +static bool > > > +remove_lport_from_local_lports(struct sset *local_lports, > > > + const char *lport_name) > > > +{ > > > + return sset_find_and_delete(local_lports, lport_name); > > > } > > > > > > /* Local bindings. binding.c module binds the logical port > > (represented by > > > @@ -637,6 +680,77 @@ is_lport_container(const struct > > sbrec_port_binding *pb) > > > return is_lport_vif(pb) && pb->parent_port && > pb->parent_port[0]; > > > } > > > > > > +static struct tracked_binding_datapath * > > > +tracked_binding_datapath_create(const struct > > sbrec_datapath_binding *dp, > > > + bool is_new, > > > + struct hmap *tracked_datapaths) > > > +{ > > > + struct tracked_binding_datapath *t_dp = xzalloc(sizeof *t_dp); > > > + t_dp->dp = dp; > > > + t_dp->is_new = is_new; > > > + shash_init(&t_dp->lports); > > > + hmap_insert(tracked_datapaths, &t_dp->node, > > uuid_hash(&dp->header_.uuid)); > > > + return t_dp; > > > +} > > > + > > > +static struct tracked_binding_datapath * > > > +tracked_binding_datapath_find(struct hmap *tracked_datapaths, > > > + const struct sbrec_datapath_binding > > *dp) > > > +{ > > > + struct tracked_binding_datapath *t_dp; > > > + size_t hash = uuid_hash(&dp->header_.uuid); > > > + HMAP_FOR_EACH_WITH_HASH (t_dp, node, hash, tracked_datapaths) > { > > > + if (uuid_equals(&t_dp->dp->header_.uuid, > > &dp->header_.uuid)) { > > > + return t_dp; > > > + } > > > + } > > > + > > > + return NULL; > > > +} > > > + > > > +static void > > > +tracked_binding_datapath_lport_add(const struct > > sbrec_port_binding *pb, > > > + bool deleted, > > > + struct hmap *tracked_datapaths) > > > +{ > > > + if (!tracked_datapaths) { > > > + return; > > > + } > > > + > > > + struct tracked_binding_datapath *tracked_dp = > > > + tracked_binding_datapath_find(tracked_datapaths, > > pb->datapath); > > > + if (!tracked_dp) { > > > + tracked_dp = > > tracked_binding_datapath_create(pb->datapath, false, > > > + > > tracked_datapaths); > > > + } > > > + > > > + /* Check if the lport is already present or not. > > > + * If it is already present, then just update the 'pb' and > > 'deleted' > > > + * fields. */ > > > + struct tracked_binding_lport *lport = > > > + shash_find_data(&tracked_dp->lports, pb->logical_port); > > > + > > > + if (!lport) { > > > + lport = xmalloc(sizeof *lport); > > > + shash_add(&tracked_dp->lports, pb->logical_port, lport); > > > + } > > > + > > > + lport->pb = pb; > > > + lport->deleted = deleted; > > > +} > > > + > > > +void > > > +binding_tracked_dp_destroy(struct hmap *tracked_datapaths) > > > +{ > > > + struct tracked_binding_datapath *t_dp; > > > + HMAP_FOR_EACH_POP (t_dp, node, tracked_datapaths) { > > > + shash_destroy_free_data(&t_dp->lports); > > > + free(t_dp); > > > + } > > > + > > > + hmap_destroy(tracked_datapaths); > > > +} > > > + > > > /* Corresponds to each Port_Binding.type. */ > > > enum en_lport_type { > > > LP_UNKNOWN, > > > @@ -690,7 +804,7 @@ static bool > > > claim_lport(const struct sbrec_port_binding *pb, > > > const struct sbrec_chassis *chassis_rec, > > > const struct ovsrec_interface *iface_rec, > > > - bool sb_readonly) > > > + bool sb_readonly, struct hmap *tracked_datapaths) > > > { > > > if (pb->chassis != chassis_rec) { > > > if (sb_readonly) { > > > @@ -709,6 +823,10 @@ claim_lport(const struct sbrec_port_binding > *pb, > > > } > > > > > > sbrec_port_binding_set_chassis(pb, chassis_rec); > > > + > > > + if (tracked_datapaths) { > > > + update_lport_tracking(pb, false, true, > > tracked_datapaths); > > > + } > > > } > > > > > > /* Check if the port encap binding, if any, has changed */ > > > @@ -726,9 +844,14 @@ claim_lport(const struct sbrec_port_binding > *pb, > > > > > > /* Returns false if lport is not released due to 'sb_readonly'. > > > * Returns true otherwise. > > > + * > > > + * This function assumes that that the the 'pb' was claimed > > > + * earlier i.e port binding's chassis is set to this chassis. > > > + * Caller should make sure that, that's the case. > > > > Can we skip "that, " above? (non-native speaker here). > > > > > > Ack. > > > > > > > > > */ > > > static bool > > > -release_lport(const struct sbrec_port_binding *pb, bool > sb_readonly) > > > +release_lport(const struct sbrec_port_binding *pb, bool > sb_readonly, > > > + struct hmap *tracked_datapaths) > > > { > > > if (pb->encap) { > > > if (sb_readonly) { > > > @@ -751,6 +874,7 @@ release_lport(const struct sbrec_port_binding > > *pb, bool sb_readonly) > > > sbrec_port_binding_set_virtual_parent(pb, NULL); > > > } > > > > > > + update_lport_tracking(pb, true, false, tracked_datapaths); > > > VLOG_INFO("Releasing lport %s from this chassis.", > > pb->logical_port); > > > return true; > > > } > > > @@ -796,13 +920,14 @@ is_lbinding_container_parent(struct > > local_binding *lbinding) > > > static bool > > > release_local_binding_children(const struct sbrec_chassis > > *chassis_rec, > > > struct local_binding *lbinding, > > > - bool sb_readonly) > > > + bool sb_readonly, > > > + struct hmap *tracked_dp_bindings) > > > { > > > struct shash_node *node; > > > SHASH_FOR_EACH (node, &lbinding->children) { > > > struct local_binding *l = node->data; > > > if (is_lbinding_this_chassis(l, chassis_rec)) { > > > - if (!release_lport(l->pb, sb_readonly)) { > > > + if (!release_lport(l->pb, sb_readonly, > > tracked_dp_bindings)) { > > > return false; > > > } > > > } > > > @@ -817,16 +942,17 @@ release_local_binding_children(const struct > > sbrec_chassis *chassis_rec, > > > > > > static bool > > > release_local_binding(const struct sbrec_chassis *chassis_rec, > > > - struct local_binding *lbinding, bool > > sb_readonly) > > > + struct local_binding *lbinding, bool > > sb_readonly, > > > + struct hmap *tracked_dp_bindings) > > > { > > > if (!release_local_binding_children(chassis_rec, lbinding, > > > - sb_readonly)) { > > > + sb_readonly, > > tracked_dp_bindings)) { > > > return false; > > > } > > > > > > bool retval = true; > > > if (is_lbinding_this_chassis(lbinding, chassis_rec)) { > > > - retval = release_lport(lbinding->pb, sb_readonly); > > > + retval = release_lport(lbinding->pb, sb_readonly, > > tracked_dp_bindings); > > > } > > > > > > lbinding->pb = NULL; > > > @@ -847,7 +973,8 @@ consider_vif_lport_(const struct > > sbrec_port_binding *pb, > > > if (can_bind) { > > > /* We can claim the lport. */ > > > if (!claim_lport(pb, b_ctx_in->chassis_rec, > > lbinding->iface, > > > - !b_ctx_in->ovnsb_idl_txn)){ > > > + !b_ctx_in->ovnsb_idl_txn, > > > + b_ctx_out->tracked_dp_bindings)){ > > > return false; > > > } > > > > > > @@ -855,8 +982,10 @@ consider_vif_lport_(const struct > > sbrec_port_binding *pb, > > > > > b_ctx_in->sbrec_port_binding_by_datapath, > > > > b_ctx_in->sbrec_port_binding_by_name, > > > pb->datapath, false, > > > - b_ctx_out->local_datapaths); > > > - update_local_lport_ids(b_ctx_out->local_lport_ids, > pb); > > > + b_ctx_out->local_datapaths, > > > + b_ctx_out->tracked_dp_bindings); > > > + update_local_lport_ids(b_ctx_out->local_lport_ids, pb, > > > + > b_ctx_out->tracked_dp_bindings); > > > if (lbinding->iface && qos_map && > > b_ctx_in->ovs_idl_txn) { > > > get_qos_params(pb, qos_map); > > > } > > > @@ -875,7 +1004,8 @@ consider_vif_lport_(const struct > > sbrec_port_binding *pb, > > > if (pb->chassis == b_ctx_in->chassis_rec) { > > > /* Release the lport if there is no lbinding. */ > > > if (!lbinding_set || !can_bind) { > > > - return release_lport(pb, !b_ctx_in->ovnsb_idl_txn); > > > + return release_lport(pb, !b_ctx_in->ovnsb_idl_txn, > > > + b_ctx_out->tracked_dp_bindings); > > > } > > > } > > > > > > @@ -962,7 +1092,8 @@ consider_container_lport(const struct > > sbrec_port_binding *pb, > > > * release the container lport, if it was bound > > earlier. */ > > > if (is_lbinding_this_chassis(container_lbinding, > > > b_ctx_in->chassis_rec)) { > > > - return release_lport(pb, !b_ctx_in->ovnsb_idl_txn); > > > + return release_lport(pb, !b_ctx_in->ovnsb_idl_txn, > > > + > b_ctx_out->tracked_dp_bindings); > > > } > > > > > > return true; > > > @@ -1037,18 +1168,22 @@ consider_nonvif_lport_(const struct > > sbrec_port_binding *pb, > > > struct binding_ctx_out *b_ctx_out) > > > { > > > if (our_chassis) { > > > - sset_add(b_ctx_out->local_lports, pb->logical_port); > > > + add_lport_to_local_lports(b_ctx_out->local_lports, > > pb->logical_port); > > > > add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key, > > > > b_ctx_in->sbrec_port_binding_by_datapath, > > > b_ctx_in->sbrec_port_binding_by_name, > > > pb->datapath, has_local_l3gateway, > > > - b_ctx_out->local_datapaths); > > > + b_ctx_out->local_datapaths, > > > + b_ctx_out->tracked_dp_bindings); > > > > > > - update_local_lport_ids(b_ctx_out->local_lport_ids, pb); > > > + update_local_lport_ids(b_ctx_out->local_lport_ids, pb, > > > + b_ctx_out->tracked_dp_bindings); > > > return claim_lport(pb, b_ctx_in->chassis_rec, NULL, > > > - !b_ctx_in->ovnsb_idl_txn); > > > + !b_ctx_in->ovnsb_idl_txn, > > > + b_ctx_out->tracked_dp_bindings); > > > } else if (pb->chassis == b_ctx_in->chassis_rec) { > > > - return release_lport(pb, !b_ctx_in->ovnsb_idl_txn); > > > + return release_lport(pb, !b_ctx_in->ovnsb_idl_txn, > > > + b_ctx_out->tracked_dp_bindings); > > > } > > > > > > return true; > > > @@ -1084,14 +1219,15 @@ consider_localnet_lport(const struct > > sbrec_port_binding *pb, > > > struct binding_ctx_out *b_ctx_out, > > > struct hmap *qos_map) > > > { > > > - /* Add all localnet ports to local_lports so that we allocate > > ct zones > > > + /* Add all localnet ports to local_ifaces so that we allocate > > ct zones > > > * for them. */ > > > - sset_add(b_ctx_out->local_lports, pb->logical_port); > > > + add_lport_to_local_lports(b_ctx_out->local_lports, > > pb->logical_port); > > > if (qos_map && b_ctx_in->ovs_idl_txn) { > > > get_qos_params(pb, qos_map); > > > } > > > > > > - update_local_lport_ids(b_ctx_out->local_lport_ids, pb); > > > + update_local_lport_ids(b_ctx_out->local_lport_ids, pb, > > > + b_ctx_out->tracked_dp_bindings); > > > } > > > > > > static bool > > > @@ -1119,7 +1255,10 @@ consider_ha_lport(const struct > > sbrec_port_binding *pb, > > > > b_ctx_in->sbrec_port_binding_by_datapath, > > > b_ctx_in->sbrec_port_binding_by_name, > > > pb->datapath, false, > > > - b_ctx_out->local_datapaths); > > > + b_ctx_out->local_datapaths, > > > + b_ctx_out->tracked_dp_bindings); > > > + update_local_lport_ids(b_ctx_out->local_lport_ids, pb, > > > + b_ctx_out->tracked_dp_bindings); > > > } > > > > > > return consider_nonvif_lport_(pb, our_chassis, false, > > b_ctx_in, b_ctx_out); > > > @@ -1185,7 +1324,7 @@ build_local_bindings(struct binding_ctx_in > > *b_ctx_in, > > > ovs_assert(lbinding->type == BT_VIF); > > > } > > > > > > - sset_add(b_ctx_out->local_lports, iface_id); > > > + > > add_lport_to_local_lports(b_ctx_out->local_lports, iface_id); > > > smap_replace(b_ctx_out->local_iface_ids, > > iface_rec->name, > > > iface_id); > > > } > > > @@ -1241,7 +1380,7 @@ binding_run(struct binding_ctx_in *b_ctx_in, > > struct binding_ctx_out *b_ctx_out) > > > case LP_PATCH: > > > case LP_LOCALPORT: > > > case LP_VTEP: > > > - update_local_lport_ids(b_ctx_out->local_lport_ids, > pb); > > > + update_local_lport_ids(b_ctx_out->local_lport_ids, > > pb, NULL); > > > break; > > > > > > case LP_VIF: > > > @@ -1409,7 +1548,8 @@ add_local_datapath_peer_port(const struct > > sbrec_port_binding *pb, > > > > > b_ctx_in->sbrec_port_binding_by_datapath, > > > b_ctx_in->sbrec_port_binding_by_name, > > > peer->datapath, false, > > > - 1, b_ctx_out->local_datapaths); > > > + 1, b_ctx_out->local_datapaths, > > > + b_ctx_out->tracked_dp_bindings); > > > return; > > > } > > > > > > @@ -1471,7 +1611,8 @@ remove_pb_from_local_datapath(const struct > > sbrec_port_binding *pb, > > > struct binding_ctx_out *b_ctx_out, > > > struct local_datapath *ld) > > > { > > > - remove_local_lport_ids(pb, b_ctx_out->local_lport_ids); > > > + remove_local_lport_ids(pb, b_ctx_out->local_lport_ids, > > > + b_ctx_out->tracked_dp_bindings); > > > if (!strcmp(pb->type, "patch") || > > > !strcmp(pb->type, "l3gateway")) { > > > remove_local_datapath_peer_port(pb, ld, > > b_ctx_out->local_datapaths); > > > @@ -1489,6 +1630,18 @@ remove_pb_from_local_datapath(const struct > > sbrec_port_binding *pb, > > > } > > > } > > > > > > +static void > > > +update_lport_tracking(const struct sbrec_port_binding *pb, > > > + bool old_claim, bool new_claim, > > > + struct hmap *tracked_dp_bindings) > > > +{ > > > + if (!tracked_dp_bindings || !pb || (old_claim == new_claim)) { > > > > I think pb can never be NULL here, right? > > > > > > Yeah. I'll remove the check. > > > > > > > > > + return; > > > + } > > > + > > > + tracked_binding_datapath_lport_add(pb, old_claim, > > tracked_dp_bindings); > > > +} > > > + > > > /* Considers the ovs iface 'iface_rec' for claiming. > > > * This function should be called if the external_ids:iface-id > > > * and 'ofport' are set for the 'iface_rec'. > > > @@ -1501,9 +1654,13 @@ consider_iface_claim(const struct > > ovsrec_interface *iface_rec, > > > const char *iface_id, > > > struct binding_ctx_in *b_ctx_in, > > > struct binding_ctx_out *b_ctx_out, > > > - struct hmap *qos_map) > > > + struct hmap *qos_map, > > > + bool *local_lports_changed) > > > { > > > - sset_add(b_ctx_out->local_lports, iface_id); > > > + if (add_lport_to_local_lports(b_ctx_out->local_lports, > > iface_id)) { > > > + *local_lports_changed = true; > > > + } > > > + > > > smap_replace(b_ctx_out->local_iface_ids, iface_rec->name, > > iface_id); > > > > > > struct local_binding *lbinding = > > > @@ -1566,7 +1723,8 @@ static bool > > > consider_iface_release(const struct ovsrec_interface *iface_rec, > > > const char *iface_id, > > > struct binding_ctx_in *b_ctx_in, > > > - struct binding_ctx_out *b_ctx_out, bool > > *changed) > > > + struct binding_ctx_out *b_ctx_out, bool > > *changed, > > > + bool *local_lports_changed) > > > { > > > struct local_binding *lbinding; > > > lbinding = local_binding_find(b_ctx_out->local_bindings, > > > @@ -1585,7 +1743,8 @@ consider_iface_release(const struct > > ovsrec_interface *iface_rec, > > > * lbinding->iface. > > > * Cannot access these members of lbinding after this > > call. */ > > > if (!release_local_binding(b_ctx_in->chassis_rec, > lbinding, > > > - !b_ctx_in->ovnsb_idl_txn)) { > > > + !b_ctx_in->ovnsb_idl_txn, > > > + > b_ctx_out->tracked_dp_bindings)) { > > > return false; > > > } > > > > > > @@ -1598,7 +1757,9 @@ consider_iface_release(const struct > > ovsrec_interface *iface_rec, > > > local_binding_delete(b_ctx_out->local_bindings, lbinding); > > > } > > > > > > - sset_find_and_delete(b_ctx_out->local_lports, iface_id); > > > + if (remove_lport_from_local_lports(b_ctx_out->local_lports, > > iface_id)) { > > > + *local_lports_changed = true; > > > + } > > > smap_remove(b_ctx_out->local_iface_ids, iface_rec->name); > > > > > > return true; > > > @@ -1630,6 +1791,8 @@ binding_handle_ovs_interface_changes(struct > > binding_ctx_in *b_ctx_in, > > > bool handled = true; > > > *changed = false; > > > > > > + *b_ctx_out->local_lports_changed = false; > > > + > > > > Is this needed? This is part of the data we clear at every run in > > en_runtime_data_clear_tracked_data(). > > > > > > I still feel it's better to set it to false here. This function doesn't > > know anything about > > the tracked data. > > > > > > Ok, then the reverse question, do we still need to clear > local_lports_changed in en_runtime_data_clear_tracked_data()? > Why not ? Do you see any issue with it ? > > > > > > > > /* Run the tracked interfaces loop twice. One to handle > deleted > > > * changes. And another to handle add/update changes. > > > * This will ensure correctness. > > > @@ -1685,7 +1848,8 @@ binding_handle_ovs_interface_changes(struct > > binding_ctx_in *b_ctx_in, > > > > > > if (cleared_iface_id) { > > > handled = consider_iface_release(iface_rec, > > cleared_iface_id, > > > - b_ctx_in, b_ctx_out, > > changed); > > > + b_ctx_in, b_ctx_out, > > changed, > > > + > > b_ctx_out->local_lports_changed); > > > } > > > > > > if (!handled) { > > > @@ -1727,7 +1891,8 @@ binding_handle_ovs_interface_changes(struct > > binding_ctx_in *b_ctx_in, > > > if (iface_id && ofport > 0) { > > > *changed = true; > > > handled = consider_iface_claim(iface_rec, iface_id, > > b_ctx_in, > > > - b_ctx_out, > qos_map_ptr); > > > + b_ctx_out, qos_map_ptr, > > > + > > b_ctx_out->local_lports_changed); > > > if (!handled) { > > > break; > > > } > > > @@ -1792,8 +1957,7 @@ static bool > > > handle_deleted_vif_lport(const struct sbrec_port_binding *pb, > > > enum en_lport_type lport_type, > > > struct binding_ctx_in *b_ctx_in, > > > - struct binding_ctx_out *b_ctx_out, > > > - bool *changed) > > > + struct binding_ctx_out *b_ctx_out) > > > { > > > struct local_binding *lbinding = > > > get_lbinding_for_lport(pb, lport_type, b_ctx_out); > > > @@ -1804,13 +1968,12 @@ handle_deleted_vif_lport(const struct > > sbrec_port_binding *pb, > > > * clear the 'chassis' column of 'pb'. But we need to do > > > * for the local_binding's children. */ > > > if (lbinding->type == BT_VIF && > > > - > > !release_local_binding_children(b_ctx_in->chassis_rec, > > > - lbinding, > > > - > > !b_ctx_in->ovnsb_idl_txn)) { > > > + !release_local_binding_children( > > > + b_ctx_in->chassis_rec, lbinding, > > > + !b_ctx_in->ovnsb_idl_txn, > > > + b_ctx_out->tracked_dp_bindings)) { > > > return false; > > > } > > > - > > > - *changed = true; > > > } > > > > > > handle_deleted_lport(pb, b_ctx_in, b_ctx_out); > > > @@ -1822,8 +1985,7 @@ handle_updated_vif_lport(const struct > > sbrec_port_binding *pb, > > > enum en_lport_type lport_type, > > > struct binding_ctx_in *b_ctx_in, > > > struct binding_ctx_out *b_ctx_out, > > > - struct hmap *qos_map, > > > - bool *changed) > > > + struct hmap *qos_map) > > > { > > > bool claimed = (pb->chassis == b_ctx_in->chassis_rec); > > > bool handled = true; > > > @@ -1841,14 +2003,10 @@ handle_updated_vif_lport(const struct > > sbrec_port_binding *pb, > > > } > > > > > > bool now_claimed = (pb->chassis == b_ctx_in->chassis_rec); > > > - bool changed_ = (claimed != now_claimed); > > > - > > > - if (changed_) { > > > - *changed = true; > > > - } > > > + bool changed = (claimed != now_claimed); > > > > > > if (lport_type == LP_VIRTUAL || > > > - (lport_type == LP_VIF && is_lport_container(pb)) || > > !changed_) { > > > + (lport_type == LP_VIF && is_lport_container(pb)) || > > !changed) { > > > return true; > > > } > > > > > > @@ -1898,7 +2056,7 @@ binding_handle_port_binding_changes(struct > > binding_ctx_in *b_ctx_in, > > > enum en_lport_type lport_type = get_lport_type(pb); > > > if (lport_type == LP_VIF || lport_type == LP_VIRTUAL) { > > > handled = handle_deleted_vif_lport(pb, lport_type, > > b_ctx_in, > > > - b_ctx_out, > changed); > > > + b_ctx_out); > > > } else { > > > handle_deleted_lport(pb, b_ctx_in, b_ctx_out); > > > } > > > @@ -1933,15 +2091,14 @@ binding_handle_port_binding_changes(struct > > binding_ctx_in *b_ctx_in, > > > case LP_VIF: > > > case LP_VIRTUAL: > > > handled = handle_updated_vif_lport(pb, lport_type, > > b_ctx_in, > > > - b_ctx_out, > > qos_map_ptr, > > > - changed); > > > + b_ctx_out, > > qos_map_ptr); > > > break; > > > > > > case LP_PATCH: > > > case LP_LOCALPORT: > > > case LP_VTEP: > > > - *changed = true; > > > - update_local_lport_ids(b_ctx_out->local_lport_ids, > pb); > > > + update_local_lport_ids(b_ctx_out->local_lport_ids, pb, > > > + > b_ctx_out->tracked_dp_bindings); > > > if (lport_type == LP_PATCH) { > > > /* Add the peer datapath to the local datapaths > > if it's > > > * not present yet. > > > @@ -1950,30 +2107,32 @@ binding_handle_port_binding_changes(struct > > binding_ctx_in *b_ctx_in, > > > add_local_datapath_peer_port(pb, b_ctx_in, > > b_ctx_out, ld); > > > } > > > } > > > + > > > + if (lport_type == LP_VTEP) { > > > + /* VTEP lports are claimed/released by > > ovn-controller-vteps. > > > + * We are not sure what changed. So set *changed > > to true > > > + * for any changes to vtep lports. */ > > > + *changed = true; > > > + } > > > break; > > > > > > case LP_L2GATEWAY: > > > - *changed = true; > > > handled = consider_l2gw_lport(pb, b_ctx_in, > b_ctx_out); > > > break; > > > > > > case LP_L3GATEWAY: > > > - *changed = true; > > > handled = consider_l3gw_lport(pb, b_ctx_in, > b_ctx_out); > > > break; > > > > > > case LP_CHASSISREDIRECT: > > > - *changed = true; > > > handled = consider_cr_lport(pb, b_ctx_in, b_ctx_out); > > > break; > > > > > > case LP_EXTERNAL: > > > - *changed = true; > > > handled = consider_external_lport(pb, b_ctx_in, > > b_ctx_out); > > > break; > > > > > > case LP_LOCALNET: { > > > - *changed = true; > > > consider_localnet_lport(pb, b_ctx_in, b_ctx_out, > > qos_map_ptr); > > > > > > struct shash bridge_mappings = > > > @@ -2008,6 +2167,10 @@ binding_handle_port_binding_changes(struct > > binding_ctx_in *b_ctx_in, > > > } > > > } > > > > > > + if (handled && > !hmap_is_empty(b_ctx_out->tracked_dp_bindings)) { > > > + *changed = true; > > > + } > > > + > > > destroy_qos_map(&qos_map); > > > return handled; > > > } > > > diff --git a/controller/binding.h b/controller/binding.h > > > index 21118ecd4..2974ebb30 100644 > > > --- a/controller/binding.h > > > +++ b/controller/binding.h > > > @@ -19,6 +19,9 @@ > > > > > > #include <stdbool.h> > > > #include "openvswitch/shash.h" > > > +#include "openvswitch/hmap.h" > > > +#include "openvswitch/uuid.h" > > > +#include "openvswitch/list.h" > > > > > > struct hmap; > > > struct ovsdb_idl; > > > @@ -54,10 +57,28 @@ struct binding_ctx_in { > > > struct binding_ctx_out { > > > struct hmap *local_datapaths; > > > struct shash *local_bindings; > > > + > > > struct sset *local_lports; > > > + > > > + /* If the sset local_lports is modified, this is set to true > by > > > + * the callee. */ > > > + bool *local_lports_changed; > > > + > > > + /* sset of local lport ids in the format > > > + * <datapath-tunnel-key>_<port-tunnel-key>. */ > > > > Nit: shouldn't this comment be part of patch 1? > > > > > > Could be. > > > > > > > > > struct sset *local_lport_ids; > > > + > > > struct sset *egress_ifaces; > > > + > > > + /* smap of OVS interface name as key and > > > + * OVS interface external_ids:iface-id as value. */ > > > > Nit: shouldn't this comment be part of patch 2? > > > > > > Could be. > > > > > > > struct smap *local_iface_ids; > > > + > > > + /* hmap of 'struct tracked_binding_datapath' which the > > > + * callee (binding_handle_ovs_interface_changes and > > > + * binding_handle_port_binding_changes) fills in for > > > + * the changed datapaths and port bindings. */ > > > + struct hmap *tracked_dp_bindings; > > > }; > > > > > > enum local_binding_type { > > > @@ -82,6 +103,21 @@ local_binding_find(struct shash > > *local_bindings, const char *name) > > > return shash_find_data(local_bindings, name); > > > } > > > > > > +/* Represents a tracked binding logical port. */ > > > +struct tracked_binding_lport { > > > + const struct sbrec_port_binding *pb; > > > + struct ovs_list list_node; > > > > Nit: for consistency with tracked_binding_datapath and because > list_node > > is usually accessed before pb I'd define list_node before pb. > > > > > > Actually we don't need list_node now. It was a leftover from the > > previous versions. > > I'll delete it. Thanks. > > > > > > > > > + bool deleted; > > > +}; > > > + > > > +/* Represent a tracked binding datapath. */ > > > +struct tracked_binding_datapath { > > > + struct hmap_node node; > > > + const struct sbrec_datapath_binding *dp; > > > + bool is_new; > > > > I think it would be easier to read the code if both > > tracked_binding_lport and tracked_binding_datapath would use the same > > logic for tracking updates: either both should have "deleted" or both > > should have "is_new". > > > > > > Ack. > > > > > > > > Also, given that we never use tracked_binding_lport.deleted, maybe we > > can skip it completely. What do you think? > > > > > > I'll delete the "deleted". We can add it later if it is really required. > > > > > > > > > + struct shash lports; /* shash of struct > tracked_binding_lport. */ > > > +}; > > > + > > > void binding_register_ovs_idl(struct ovsdb_idl *); > > > void binding_run(struct binding_ctx_in *, struct binding_ctx_out > *); > > > bool binding_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn, > > > @@ -96,4 +132,5 @@ bool > > binding_handle_ovs_interface_changes(struct binding_ctx_in *, > > > bool binding_handle_port_binding_changes(struct binding_ctx_in *, > > > struct binding_ctx_out *, > > > bool *changed); > > > +void binding_tracked_dp_destroy(struct hmap *tracked_datapaths); > > > #endif /* controller/binding.h */ > > > diff --git a/controller/ovn-controller.c > b/controller/ovn-controller.c > > > index 795a1c297..98652866c 100644 > > > --- a/controller/ovn-controller.c > > > +++ b/controller/ovn-controller.c > > > @@ -973,10 +973,88 @@ struct ed_type_runtime_data { > > > struct sset local_lport_ids; > > > struct sset active_tunnels; > > > > > > + /* runtime data engine private data. */ > > > struct sset egress_ifaces; > > > struct smap local_iface_ids; > > > + > > > + /* Tracked data. See below for more details and comments. */ > > > + bool tracked; > > > > I wonder if the "tracked" field should actually be part of the > generic > > engine_node_input. We use it for port groups and address sets too > > ("change_tracked" in ed_type_addr_sets and ed_type_port_groups) and > > afaiu the semantics are always: > > - "tracked" is set to true if a node's input handler was called > > - "tracked" is reset to false if a node's run handler was called > > > > So all this could be implemented generically in inc-proc-eng.c. > > > > What do you think? > > > > > > Could be. But I'd prefer to be a separate patch (not part of this > series). > > > > Ok, but please see my comment regarding > en_runtime_data_clear_tracked_data() in en_runtime_data_run(). > > > > > > > > + bool local_lports_changed; > > > > Do we really need local_lports_changed? Isn't it actually equivalent > to > > !hmap_is_empty(& > > > > > + struct hmap tracked_dp_bindings; > > > }; > > > > > > Not really. > > > > Let's say you run the command - "ovs-vsctl add-port br-int p0 -- set > > interface p0 externa_ids:iface-id=lport1" > > > > And if logical lport lport1 doesn't exist (or not seen by ovn-controller > > due to conditional monitoring), we > > need to update the runtime_data engine node so that "update_sb_monitor" > > is called. > > > > > > Ah, right, thanks for the explanation. > > > > > > > > > +/* struct ed_type_runtime_data has the below members for tracking > the > > > + * changes done to the runtime_data engine by the runtime_data > engine > > > + * handlers. Since this engine is an input to the flow_output > engine, > > > + * the flow output runtime data handler will make use of this > > tracked data. > > > + * > > > + * > > > ------------------------------------------------------------------------ > > > + * | | This is a hmap of > > | > > > + * | | 'struct tracked_binding_datapath' > > defined in | > > > + * | | binding.h. Runtime data handlers for > > OVS | > > > + * | | Interface and Port Binding changes > > store the | > > > + * | @tracked_dp_bindings | changed datapaths (datapaths > > added/removed from | > > > + * | | local_datapaths) and changed port > > bindings | > > > + * | | (added/updated/deleted in > > 'local_bindings'). | > > > > This is not really accurate, right? Afaiu we only track if a port > > binding is added/deleted. > > > > > > Not all the time. We can bind an "external" port when the port binding > gets > > updated due to the command - "ovn-nbctl lsp-set-type <lport> external" > > or for the command - "ovn-nbctl lsp-set-options router-port=<peer port> > > > > > > OK. > > > > > > > > > > + * | | So any changes to the runtime data - > > | > > > + * | | local_datapaths and local_bindings is > > captured | > > > + * | | here. > > | > > > + * > > > ------------------------------------------------------------------------ > > > + * | | This is a bool which represents if > > the runtime | > > > + * | | data 'local_lports' changed by the > > runtime data | > > > + * | | handlers for OVS Interface and Port > > Binding | > > > + * | | changes. If 'local_lports' is updated > > and also | > > > + * | | results in any port binding updates, > > it is | > > > + * |@local_lports_changed | captured in the @tracked_dp_bindings. > > So there | > > > + * | | is no need to capture the changes in > > the | > > > + * | | local_lports. If > > @local_lports_changed is true | > > > + * | | but without anydata in the > > @tracked_dp_bindings,| > > > + * | | it means we needto only update the SB > > monitor | > > > + * | | clauses and there isno need for any > > flow | > > > + * | | (re)computations. > > | > > > + * > > > ------------------------------------------------------------------------ > > > + * | | This represents if the data was > > tracked or not | > > > + * | | by the runtime data handlers during > > the engine | > > > + * | @tracked | run. If the runtime data recompute > > is | > > > + * | | triggered, it means there is no > > tracked data. | > > > + * > > > ------------------------------------------------------------------------ > > > + * > > > + * > > > + * The changes to the following runtime_data variables are not > > tracked. > > > + * > > > + * > > --------------------------------------------------------------------- > > > + * | local_datapaths | The changes to these runtime data is > > captured in | > > > + * | local_bindings | the @tracked_dp_bindings indirectly and > > hence it | > > > + * | local_lport_ids | is not tracked explicitly. > > | > > > + * > > --------------------------------------------------------------------- > > > + * | local_iface_ids | This is used internally within the > > runtime data | > > > + * | egress_ifaces | engine (used only in binding.c) and hence > > there | > > > + * | | there is no need to track. > > | > > > + * > > --------------------------------------------------------------------- > > > + * | | Active tunnels is built in the > > | > > > + * | | bfd_calculate_active_tunnels() for the > > tunnel | > > > + * | | OVS interfaces. Any changes to non VIF > > OVS | > > > + * | | interfaces results in triggering the > > full | > > > + * | active_tunnels | recompute of runtime data engine and > > hence there | > > > + * | | the tracked data doesn't track it. When > > we | > > > + * | | support handling changes to non VIF OVS > > | > > > + * | | interfaces we need to track the changes > > to the | > > > + * | | active tunnels. > > | > > > + * > > --------------------------------------------------------------------- > > > + * > > > + */ > > > + > > > +static void > > > +en_runtime_data_clear_tracked_data(void *data_) > > > +{ > > > + struct ed_type_runtime_data *data = data_; > > > + > > > + binding_tracked_dp_destroy(&data->tracked_dp_bindings); > > > + hmap_init(&data->tracked_dp_bindings); > > > > Nit: just as we did for > local_bindings_init()/local_bindings_destroy() > > I'd move the "hmap_init(&data->tracked_dp_bindings)" inside a new > > function called binding_tracked_dp_init(). > > > > > > Not sure if we want to have the same functions here just to init the > hmap. > > > > > > Well, local_bindings_init(), just does shash_init() so I don't really > see why not :) > I don't see any advantage, but rather an unnecessary function overhead. So, I'd live it as is. > > > > > + data->local_lports_changed = false; > > > + data->tracked = false; > > > +} > > > + > > > static void * > > > en_runtime_data_init(struct engine_node *node OVS_UNUSED, > > > struct engine_arg *arg OVS_UNUSED) > > > @@ -990,6 +1068,10 @@ en_runtime_data_init(struct engine_node > > *node OVS_UNUSED, > > > sset_init(&data->egress_ifaces); > > > smap_init(&data->local_iface_ids); > > > local_bindings_init(&data->local_bindings); > > > + > > > + /* init the tracked data. */ > > > + hmap_init(&data->tracked_dp_bindings); > > > + > > > return data; > > > } > > > > > > @@ -1012,6 +1094,8 @@ en_runtime_data_cleanup(void *data) > > > } > > > hmap_destroy(&rt_data->local_datapaths); > > > local_bindings_destroy(&rt_data->local_bindings); > > > + > > > + en_runtime_data_clear_tracked_data(data); > > > } > > > > > > static void > > > @@ -1092,6 +1176,8 @@ init_binding_ctx(struct engine_node *node, > > > b_ctx_out->egress_ifaces = &rt_data->egress_ifaces; > > > b_ctx_out->local_bindings = &rt_data->local_bindings; > > > b_ctx_out->local_iface_ids = &rt_data->local_iface_ids; > > > + b_ctx_out->tracked_dp_bindings = NULL; > > > + b_ctx_out->local_lports_changed = NULL; > > > } > > > > > > static void > > > @@ -1103,6 +1189,13 @@ en_runtime_data_run(struct engine_node > > *node, void *data) > > > struct sset *local_lport_ids = &rt_data->local_lport_ids; > > > struct sset *active_tunnels = &rt_data->active_tunnels; > > > > > > + /* Clear the tracked data. Even though the tracked data > > > + * gets cleared in the beginning of engine_init_run(), > > > + * any of the runtime data handler might have set some tracked > > > + * data and later another runtime data handler might return > false > > > + * resulting full recompute of runtime engine. */ > > > + en_runtime_data_clear_tracked_data(data); > > > + > > > > Could you please elaborate more on this? Why is it an issue if we > keep > > the "tracked" data until the end of the run? > > > > > > > > When a runtime data node gets updated (engine_set_node_state()) either > > due to > > full recompute of runtime or due to handler setting it, > > flow_output_runtime_data_handler() > > will not trigger flow_output recompute unless it see runtime->tracked = > > false. > > > > Hence it clears the tracked data. Instead of calling > > en_runtime_data_clear_tracked_data(), > > en_runtime_data_run() can set just 'tracked' to false, but I think it's > > better to clear > > the whole tracked data. > > > > So this brings us back to my original comment regarding "tracked". If > tracked would be part of the I-P engine internal input node fields, it > would get set before running en_runtime_data_run() and we wouldn't have > to explain why we need to clear the "tracked" field here. > We may still have to clear the tracked data if some of the handlers added some data to it before one of the handlers gives up and we fall back to fully recompute given that we don't clear the tracked data at the end of the engine. Also I've not thought about it and not sure if we really want to move to engine. It could be a good idea. But I'll leave it to others or you, if you want to work on this as a follow up patch. > If we decide to address the "tracked" fields as a follow up patch I > think it'd be good to add some TODO/FIXME/XXX comments so we don't > forget about addressing this later. E.g., there's already " > /* XXX: The change_tracked check may be added to inc-proc framework. */" > for address sets. > I'll add a comment that it could be moved to the engine. Thanks Numan > Thanks, > Dumitru > > > > > Thanks, > > Dumitru > > > > > static bool first_run = true; > > > if (first_run) { > > > /* don't cleanup since there is no data yet */ > > > @@ -1158,6 +1251,9 @@ runtime_data_ovs_interface_handler(struct > > engine_node *node, void *data) > > > struct binding_ctx_in b_ctx_in; > > > struct binding_ctx_out b_ctx_out; > > > init_binding_ctx(node, rt_data, &b_ctx_in, &b_ctx_out); > > > + rt_data->tracked = true; > > > + b_ctx_out.tracked_dp_bindings = &rt_data->tracked_dp_bindings; > > > + b_ctx_out.local_lports_changed = > &rt_data->local_lports_changed; > > > > > > bool changed = false; > > > if (!binding_handle_ovs_interface_changes(&b_ctx_in, > &b_ctx_out, > > > @@ -1190,6 +1286,9 @@ runtime_data_sb_port_binding_handler(struct > > engine_node *node, void *data) > > > return false; > > > } > > > > > > + rt_data->tracked = true; > > > + b_ctx_out.tracked_dp_bindings = &rt_data->tracked_dp_bindings; > > > + > > > bool changed = false; > > > if (!binding_handle_port_binding_changes(&b_ctx_in, > &b_ctx_out, > > > &changed)) { > > > @@ -1797,6 +1896,39 @@ flow_output_port_groups_handler(struct > > engine_node *node, void *data) > > > return _flow_output_resource_ref_handler(node, data, > > REF_TYPE_PORTGROUP); > > > } > > > > > > +static bool > > > +flow_output_runtime_data_handler(struct engine_node *node, > > > + void *data OVS_UNUSED) > > > +{ > > > + struct ed_type_runtime_data *rt_data = > > > + engine_get_input_data("runtime_data", node); > > > + > > > + /* There is no tracked data. Fall back to full recompute of > > > + * flow_output. */ > > > + if (!rt_data->tracked) { > > > + return false; > > > + } > > > + > > > + if (!hmap_is_empty(&rt_data->tracked_dp_bindings)) { > > > + /* We are not yet handling the tracked datapath binding > > > + * changes. Return false to trigger full recompute. */ > > > + return false; > > > + } > > > + > > > + if (rt_data->local_lports_changed) { > > > + engine_set_node_state(node, EN_UPDATED); > > > + } > > > + > > > + return true; > > > +} > > > + > > > +static bool > > > +flow_output_noop_handler(struct engine_node *node OVS_UNUSED, > > > + void *data OVS_UNUSED) > > > +{ > > > + return true; > > > +} > > > + > > > /* Engine node en_physical_flow_changes indicates whether > > > * there is a need to > > > * - recompute only physical flows or > > > @@ -1908,13 +2040,6 @@ > > flow_output_physical_flow_changes_handler(struct engine_node *node, > > void *data) > > > return true; > > > } > > > > > > -static bool > > > -flow_output_noop_handler(struct engine_node *node OVS_UNUSED, > > > - void *data OVS_UNUSED) > > > -{ > > > - return true; > > > -} > > > - > > > struct ovn_controller_exit_args { > > > bool *exiting; > > > bool *restart; > > > @@ -2036,7 +2161,7 @@ main(int argc, char *argv[]) > > > > > > /* Define inc-proc-engine nodes. */ > > > ENGINE_NODE_CUSTOM_DATA(ct_zones, "ct_zones"); > > > - ENGINE_NODE(runtime_data, "runtime_data"); > > > + ENGINE_NODE_WITH_CLEAR_TRACK_DATA(runtime_data, > "runtime_data"); > > > ENGINE_NODE(mff_ovn_geneve, "mff_ovn_geneve"); > > > ENGINE_NODE(ofctrl_is_connected, "ofctrl_is_connected"); > > > ENGINE_NODE_WITH_CLEAR_TRACK_DATA(physical_flow_changes, > > > @@ -2073,7 +2198,8 @@ main(int argc, char *argv[]) > > > flow_output_addr_sets_handler); > > > engine_add_input(&en_flow_output, &en_port_groups, > > > flow_output_port_groups_handler); > > > - engine_add_input(&en_flow_output, &en_runtime_data, NULL); > > > + engine_add_input(&en_flow_output, &en_runtime_data, > > > + flow_output_runtime_data_handler); > > > engine_add_input(&en_flow_output, &en_mff_ovn_geneve, NULL); > > > engine_add_input(&en_flow_output, &en_physical_flow_changes, > > > flow_output_physical_flow_changes_handler); > > > diff --git a/tests/ovn-performance.at <http://ovn-performance.at> > > b/tests/ovn-performance.at <http://ovn-performance.at> > > > index 5cc1960b6..08acd3bae 100644 > > > --- a/tests/ovn-performance.at <http://ovn-performance.at> > > > +++ b/tests/ovn-performance.at <http://ovn-performance.at> > > > @@ -286,11 +286,11 @@ for i in 1 2; do > > > [hv1 hv2], [lflow_run], > > > [ovn-nbctl --wait=hv lsp-set-type $lsp router] > > > ) > > > - OVN_CONTROLLER_EXPECT_HIT( > > > + OVN_CONTROLLER_EXPECT_NO_HIT( > > > [hv1 hv2], [lflow_run], > > > [ovn-nbctl --wait=hv lsp-set-options $lsp > router-port=$lrp] > > > ) > > > - OVN_CONTROLLER_EXPECT_HIT( > > > + OVN_CONTROLLER_EXPECT_NO_HIT( > > > [hv1 hv2], [lflow_run], > > > [ovn-nbctl --wait=hv lsp-set-addresses $lsp router] > > > ) > > > @@ -416,15 +416,15 @@ OVN_CONTROLLER_EXPECT_NO_HIT( > > > [ovn-nbctl --wait=hv destroy Port_Group pg1] > > > ) > > > > > > -for i in 1 2; do > > > - ls=ls$i > > > +OVN_CONTROLLER_EXPECT_HIT( > > > + [hv1 hv2], [lflow_run], > > > + [ovn-nbctl --wait=hv ls-del ls1] > > > +) > > > > > > - # Delete switch $ls > > > - OVN_CONTROLLER_EXPECT_HIT( > > > - [hv1 hv2], [lflow_run], > > > - [ovn-nbctl --wait=hv ls-del $ls] > > > - ) > > > -done > > > +OVN_CONTROLLER_EXPECT_NO_HIT( > > > + [hv1 hv2], [lflow_run], > > > + [ovn-nbctl --wait=hv ls-del ls2] > > > +) > > > > > > # Delete router lr1 > > > OVN_CONTROLLER_EXPECT_NO_HIT( > > > > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org <mailto:dev@openvswitch.org> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 6/9/20 3:53 PM, Numan Siddique wrote: > > > On Tue, Jun 9, 2020 at 6:33 PM Dumitru Ceara <dceara@redhat.com > <mailto:dceara@redhat.com>> wrote: > > On 6/9/20 1:00 PM, Numan Siddique wrote: > > > > > > On Tue, Jun 9, 2020 at 3:11 PM Dumitru Ceara <dceara@redhat.com > <mailto:dceara@redhat.com> > > <mailto:dceara@redhat.com <mailto:dceara@redhat.com>>> wrote: > > > > On 6/8/20 3:50 PM, numans@ovn.org <mailto:numans@ovn.org> > <mailto:numans@ovn.org <mailto:numans@ovn.org>> wrote: > > > From: Numan Siddique <numans@ovn.org <mailto:numans@ovn.org> > <mailto:numans@ovn.org <mailto:numans@ovn.org>>> > > > > > > In order to handle runtime data changes incrementally, the > flow outut > > > runtime data handle should know the changed runtime data. > > > Runtime data now tracks the changed data for any OVS interface > > > and SB port binding changes. The tracked data contains a hmap > > > of tracked datapaths (which changed during runtime data > processing. > > > > > > The flow outout runtime_data handler in this patch doesn't > do much > > > with the tracked data. It returns false if there is tracked data > > available > > > so that flow_output run is called. If no tracked data is > available > > > then there is no need for flow computation and the handler > returns > > true. > > > > > > Next patch in the series processes the tracked data > incrementally. > > > > > > Co-Authored-by: Venkata Anil <anilvenkata@redhat.com > <mailto:anilvenkata@redhat.com> > > <mailto:anilvenkata@redhat.com <mailto:anilvenkata@redhat.com>>> > > > Signed-off-by: Venkata Anil <anilvenkata@redhat.com > <mailto:anilvenkata@redhat.com> > > <mailto:anilvenkata@redhat.com <mailto:anilvenkata@redhat.com>>> > > > Signed-off-by: Numan Siddique <numans@ovn.org > <mailto:numans@ovn.org> <mailto:numans@ovn.org <mailto:numans@ovn.org>>> > > > --- > > > controller/binding.c | 299 > > ++++++++++++++++++++++++++++-------- > > > controller/binding.h | 37 +++++ > > > controller/ovn-controller.c | 144 +++++++++++++++-- > > > tests/ovn-performance.at <http://ovn-performance.at> > <http://ovn-performance.at> | 20 +-- > > > 4 files changed, 413 insertions(+), 87 deletions(-) > > > > > > diff --git a/controller/binding.c b/controller/binding.c > > > index c2d9a4c6b..3c226cd7d 100644 > > > --- a/controller/binding.c > > > +++ b/controller/binding.c > > > @@ -69,13 +69,26 @@ binding_register_ovs_idl(struct ovsdb_idl > > *ovs_idl) > > > ovsdb_idl_add_column(ovs_idl, &ovsrec_qos_col_type); > > > } > > > > > > +static struct tracked_binding_datapath > > *tracked_binding_datapath_create( > > > + const struct sbrec_datapath_binding *, > > > + bool is_new, struct hmap *tracked_dps); > > > +static struct tracked_binding_datapath > > *tracked_binding_datapath_find( > > > + struct hmap *, const struct sbrec_datapath_binding *); > > > +static void tracked_binding_datapath_lport_add( > > > + const struct sbrec_port_binding *, bool deleted, > > > + struct hmap *tracked_datapaths); > > > +static void update_lport_tracking(const struct > sbrec_port_binding > > *pb, > > > + bool old_claim, bool > new_claim, > > > + struct hmap > *tracked_dp_bindings); > > > + > > > static void > > > add_local_datapath__(struct ovsdb_idl_index > > *sbrec_datapath_binding_by_key, > > > struct ovsdb_idl_index > > *sbrec_port_binding_by_datapath, > > > struct ovsdb_idl_index > > *sbrec_port_binding_by_name, > > > const struct sbrec_datapath_binding > *datapath, > > > bool has_local_l3gateway, int depth, > > > - struct hmap *local_datapaths) > > > + struct hmap *local_datapaths, > > > + struct hmap *tracked_datapaths) > > > { > > > uint32_t dp_key = datapath->tunnel_key; > > > struct local_datapath *ld = > > get_local_datapath(local_datapaths, dp_key); > > > @@ -92,6 +105,11 @@ add_local_datapath__(struct ovsdb_idl_index > > *sbrec_datapath_binding_by_key, > > > ld->localnet_port = NULL; > > > ld->has_local_l3gateway = has_local_l3gateway; > > > > > > + if (tracked_datapaths && > > > + !tracked_binding_datapath_find(tracked_datapaths, > > datapath)) { > > > + tracked_binding_datapath_create(datapath, true, > > tracked_datapaths); > > > + } > > > + > > > if (depth >= 100) { > > > static struct vlog_rate_limit rl = > > VLOG_RATE_LIMIT_INIT(1, 1); > > > VLOG_WARN_RL(&rl, "datapaths nested too deep"); > > > @@ -124,7 +142,8 @@ add_local_datapath__(struct ovsdb_idl_index > > *sbrec_datapath_binding_by_key, > > > > > sbrec_port_binding_by_datapath, > > > > > sbrec_port_binding_by_name, > > > > peer->datapath, false, > > > - depth + 1, > > local_datapaths); > > > + depth + 1, > > local_datapaths, > > > + > tracked_datapaths); > > > } > > > ld->n_peer_ports++; > > > if (ld->n_peer_ports > > > ld->n_allocated_peer_ports) { > > > @@ -147,12 +166,14 @@ add_local_datapath(struct ovsdb_idl_index > > *sbrec_datapath_binding_by_key, > > > struct ovsdb_idl_index > > *sbrec_port_binding_by_datapath, > > > struct ovsdb_idl_index > > *sbrec_port_binding_by_name, > > > const struct sbrec_datapath_binding > *datapath, > > > - bool has_local_l3gateway, struct hmap > > *local_datapaths) > > > + bool has_local_l3gateway, struct hmap > > *local_datapaths, > > > + struct hmap *tracked_datapaths) > > > { > > > add_local_datapath__(sbrec_datapath_binding_by_key, > > > sbrec_port_binding_by_datapath, > > > sbrec_port_binding_by_name, > > > - datapath, has_local_l3gateway, 0, > > local_datapaths); > > > + datapath, has_local_l3gateway, 0, > > local_datapaths, > > > + tracked_datapaths); > > > } > > > > > > static void > > > @@ -473,23 +494,45 @@ update_ld_localnet_port(const struct > > sbrec_port_binding *binding_rec, > > > > > > static void > > > update_local_lport_ids(struct sset *local_lport_ids, > > > - const struct sbrec_port_binding *pb) > > > + const struct sbrec_port_binding *pb, > > > + struct hmap *tracked_datapaths) > > > { > > > char buf[16]; > > > snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64, > > > pb->datapath->tunnel_key, pb->tunnel_key); > > > - sset_add(local_lport_ids, buf); > > > + bool added = sset_add(local_lport_ids, buf); > > > > Hi Numan, > > > > > > Hi Dumitru, > > > > Thanks for the review. > > > > Please see below the replies. > > > > Thanks > > Numan > > > > > > > > I guess this should be: > > > > bool added = !!sset_add(local_lport_ids, buf); > > > > > > Ack. > > > > > > > > > + if (added && tracked_datapaths) { > > > + /* Add the 'pb' to the tracked_datapaths. */ > > > + tracked_binding_datapath_lport_add(pb, false, > > tracked_datapaths); > > > + } > > > } > > > > > > static void > > > -remove_local_lport_ids(const struct sbrec_port_binding > *binding_rec, > > > - struct sset *local_lport_ids) > > > +remove_local_lport_ids(const struct sbrec_port_binding *pb, > > > + struct sset *local_lport_ids, > > > + struct hmap *tracked_datapaths) > > > { > > > char buf[16]; > > > snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64, > > > - binding_rec->datapath->tunnel_key, > > > - binding_rec->tunnel_key); > > > - sset_find_and_delete(local_lport_ids, buf); > > > + pb->datapath->tunnel_key, pb->tunnel_key); > > > + bool deleted = sset_find_and_delete(local_lport_ids, buf); > > > + if (deleted && tracked_datapaths) { > > > + /* Add the 'pb' to the tracked_datapaths. */ > > > + tracked_binding_datapath_lport_add(pb, true, > > tracked_datapaths); > > > + } > > > +} > > > + > > > +static bool > > > +add_lport_to_local_lports(struct sset *local_lports, const char > > *lport_name) > > > +{ > > > + return !!sset_add(local_lports, lport_name); > > > +} > > > + > > > +static bool > > > +remove_lport_from_local_lports(struct sset *local_lports, > > > + const char *lport_name) > > > +{ > > > + return sset_find_and_delete(local_lports, lport_name); > > > } > > > > > > /* Local bindings. binding.c module binds the logical port > > (represented by > > > @@ -637,6 +680,77 @@ is_lport_container(const struct > > sbrec_port_binding *pb) > > > return is_lport_vif(pb) && pb->parent_port && > pb->parent_port[0]; > > > } > > > > > > +static struct tracked_binding_datapath * > > > +tracked_binding_datapath_create(const struct > > sbrec_datapath_binding *dp, > > > + bool is_new, > > > + struct hmap *tracked_datapaths) > > > +{ > > > + struct tracked_binding_datapath *t_dp = xzalloc(sizeof > *t_dp); > > > + t_dp->dp = dp; > > > + t_dp->is_new = is_new; > > > + shash_init(&t_dp->lports); > > > + hmap_insert(tracked_datapaths, &t_dp->node, > > uuid_hash(&dp->header_.uuid)); > > > + return t_dp; > > > +} > > > + > > > +static struct tracked_binding_datapath * > > > +tracked_binding_datapath_find(struct hmap *tracked_datapaths, > > > + const struct > sbrec_datapath_binding > > *dp) > > > +{ > > > + struct tracked_binding_datapath *t_dp; > > > + size_t hash = uuid_hash(&dp->header_.uuid); > > > + HMAP_FOR_EACH_WITH_HASH (t_dp, node, hash, > tracked_datapaths) { > > > + if (uuid_equals(&t_dp->dp->header_.uuid, > > &dp->header_.uuid)) { > > > + return t_dp; > > > + } > > > + } > > > + > > > + return NULL; > > > +} > > > + > > > +static void > > > +tracked_binding_datapath_lport_add(const struct > > sbrec_port_binding *pb, > > > + bool deleted, > > > + struct hmap > *tracked_datapaths) > > > +{ > > > + if (!tracked_datapaths) { > > > + return; > > > + } > > > + > > > + struct tracked_binding_datapath *tracked_dp = > > > + tracked_binding_datapath_find(tracked_datapaths, > > pb->datapath); > > > + if (!tracked_dp) { > > > + tracked_dp = > > tracked_binding_datapath_create(pb->datapath, false, > > > + > > tracked_datapaths); > > > + } > > > + > > > + /* Check if the lport is already present or not. > > > + * If it is already present, then just update the 'pb' and > > 'deleted' > > > + * fields. */ > > > + struct tracked_binding_lport *lport = > > > + shash_find_data(&tracked_dp->lports, pb->logical_port); > > > + > > > + if (!lport) { > > > + lport = xmalloc(sizeof *lport); > > > + shash_add(&tracked_dp->lports, pb->logical_port, > lport); > > > + } > > > + > > > + lport->pb = pb; > > > + lport->deleted = deleted; > > > +} > > > + > > > +void > > > +binding_tracked_dp_destroy(struct hmap *tracked_datapaths) > > > +{ > > > + struct tracked_binding_datapath *t_dp; > > > + HMAP_FOR_EACH_POP (t_dp, node, tracked_datapaths) { > > > + shash_destroy_free_data(&t_dp->lports); > > > + free(t_dp); > > > + } > > > + > > > + hmap_destroy(tracked_datapaths); > > > +} > > > + > > > /* Corresponds to each Port_Binding.type. */ > > > enum en_lport_type { > > > LP_UNKNOWN, > > > @@ -690,7 +804,7 @@ static bool > > > claim_lport(const struct sbrec_port_binding *pb, > > > const struct sbrec_chassis *chassis_rec, > > > const struct ovsrec_interface *iface_rec, > > > - bool sb_readonly) > > > + bool sb_readonly, struct hmap *tracked_datapaths) > > > { > > > if (pb->chassis != chassis_rec) { > > > if (sb_readonly) { > > > @@ -709,6 +823,10 @@ claim_lport(const struct > sbrec_port_binding *pb, > > > } > > > > > > sbrec_port_binding_set_chassis(pb, chassis_rec); > > > + > > > + if (tracked_datapaths) { > > > + update_lport_tracking(pb, false, true, > > tracked_datapaths); > > > + } > > > } > > > > > > /* Check if the port encap binding, if any, has changed */ > > > @@ -726,9 +844,14 @@ claim_lport(const struct > sbrec_port_binding *pb, > > > > > > /* Returns false if lport is not released due to 'sb_readonly'. > > > * Returns true otherwise. > > > + * > > > + * This function assumes that that the the 'pb' was claimed > > > + * earlier i.e port binding's chassis is set to this chassis. > > > + * Caller should make sure that, that's the case. > > > > Can we skip "that, " above? (non-native speaker here). > > > > > > Ack. > > > > > > > > > */ > > > static bool > > > -release_lport(const struct sbrec_port_binding *pb, bool > sb_readonly) > > > +release_lport(const struct sbrec_port_binding *pb, bool > sb_readonly, > > > + struct hmap *tracked_datapaths) > > > { > > > if (pb->encap) { > > > if (sb_readonly) { > > > @@ -751,6 +874,7 @@ release_lport(const struct > sbrec_port_binding > > *pb, bool sb_readonly) > > > sbrec_port_binding_set_virtual_parent(pb, NULL); > > > } > > > > > > + update_lport_tracking(pb, true, false, tracked_datapaths); > > > VLOG_INFO("Releasing lport %s from this chassis.", > > pb->logical_port); > > > return true; > > > } > > > @@ -796,13 +920,14 @@ is_lbinding_container_parent(struct > > local_binding *lbinding) > > > static bool > > > release_local_binding_children(const struct sbrec_chassis > > *chassis_rec, > > > struct local_binding *lbinding, > > > - bool sb_readonly) > > > + bool sb_readonly, > > > + struct hmap > *tracked_dp_bindings) > > > { > > > struct shash_node *node; > > > SHASH_FOR_EACH (node, &lbinding->children) { > > > struct local_binding *l = node->data; > > > if (is_lbinding_this_chassis(l, chassis_rec)) { > > > - if (!release_lport(l->pb, sb_readonly)) { > > > + if (!release_lport(l->pb, sb_readonly, > > tracked_dp_bindings)) { > > > return false; > > > } > > > } > > > @@ -817,16 +942,17 @@ release_local_binding_children(const > struct > > sbrec_chassis *chassis_rec, > > > > > > static bool > > > release_local_binding(const struct sbrec_chassis *chassis_rec, > > > - struct local_binding *lbinding, bool > > sb_readonly) > > > + struct local_binding *lbinding, bool > > sb_readonly, > > > + struct hmap *tracked_dp_bindings) > > > { > > > if (!release_local_binding_children(chassis_rec, lbinding, > > > - sb_readonly)) { > > > + sb_readonly, > > tracked_dp_bindings)) { > > > return false; > > > } > > > > > > bool retval = true; > > > if (is_lbinding_this_chassis(lbinding, chassis_rec)) { > > > - retval = release_lport(lbinding->pb, sb_readonly); > > > + retval = release_lport(lbinding->pb, sb_readonly, > > tracked_dp_bindings); > > > } > > > > > > lbinding->pb = NULL; > > > @@ -847,7 +973,8 @@ consider_vif_lport_(const struct > > sbrec_port_binding *pb, > > > if (can_bind) { > > > /* We can claim the lport. */ > > > if (!claim_lport(pb, b_ctx_in->chassis_rec, > > lbinding->iface, > > > - !b_ctx_in->ovnsb_idl_txn)){ > > > + !b_ctx_in->ovnsb_idl_txn, > > > + b_ctx_out->tracked_dp_bindings)){ > > > return false; > > > } > > > > > > @@ -855,8 +982,10 @@ consider_vif_lport_(const struct > > sbrec_port_binding *pb, > > > > > b_ctx_in->sbrec_port_binding_by_datapath, > > > > b_ctx_in->sbrec_port_binding_by_name, > > > pb->datapath, false, > > > - b_ctx_out->local_datapaths); > > > - > update_local_lport_ids(b_ctx_out->local_lport_ids, pb); > > > + b_ctx_out->local_datapaths, > > > + b_ctx_out->tracked_dp_bindings); > > > + > update_local_lport_ids(b_ctx_out->local_lport_ids, pb, > > > + > b_ctx_out->tracked_dp_bindings); > > > if (lbinding->iface && qos_map && > > b_ctx_in->ovs_idl_txn) { > > > get_qos_params(pb, qos_map); > > > } > > > @@ -875,7 +1004,8 @@ consider_vif_lport_(const struct > > sbrec_port_binding *pb, > > > if (pb->chassis == b_ctx_in->chassis_rec) { > > > /* Release the lport if there is no lbinding. */ > > > if (!lbinding_set || !can_bind) { > > > - return release_lport(pb, !b_ctx_in->ovnsb_idl_txn); > > > + return release_lport(pb, !b_ctx_in->ovnsb_idl_txn, > > > + > b_ctx_out->tracked_dp_bindings); > > > } > > > } > > > > > > @@ -962,7 +1092,8 @@ consider_container_lport(const struct > > sbrec_port_binding *pb, > > > * release the container lport, if it was bound > > earlier. */ > > > if (is_lbinding_this_chassis(container_lbinding, > > > > b_ctx_in->chassis_rec)) { > > > - return release_lport(pb, > !b_ctx_in->ovnsb_idl_txn); > > > + return release_lport(pb, > !b_ctx_in->ovnsb_idl_txn, > > > + > b_ctx_out->tracked_dp_bindings); > > > } > > > > > > return true; > > > @@ -1037,18 +1168,22 @@ consider_nonvif_lport_(const struct > > sbrec_port_binding *pb, > > > struct binding_ctx_out *b_ctx_out) > > > { > > > if (our_chassis) { > > > - sset_add(b_ctx_out->local_lports, pb->logical_port); > > > + add_lport_to_local_lports(b_ctx_out->local_lports, > > pb->logical_port); > > > > add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key, > > > > b_ctx_in->sbrec_port_binding_by_datapath, > > > > b_ctx_in->sbrec_port_binding_by_name, > > > pb->datapath, has_local_l3gateway, > > > - b_ctx_out->local_datapaths); > > > + b_ctx_out->local_datapaths, > > > + b_ctx_out->tracked_dp_bindings); > > > > > > - update_local_lport_ids(b_ctx_out->local_lport_ids, pb); > > > + update_local_lport_ids(b_ctx_out->local_lport_ids, pb, > > > + b_ctx_out->tracked_dp_bindings); > > > return claim_lport(pb, b_ctx_in->chassis_rec, NULL, > > > - !b_ctx_in->ovnsb_idl_txn); > > > + !b_ctx_in->ovnsb_idl_txn, > > > + b_ctx_out->tracked_dp_bindings); > > > } else if (pb->chassis == b_ctx_in->chassis_rec) { > > > - return release_lport(pb, !b_ctx_in->ovnsb_idl_txn); > > > + return release_lport(pb, !b_ctx_in->ovnsb_idl_txn, > > > + b_ctx_out->tracked_dp_bindings); > > > } > > > > > > return true; > > > @@ -1084,14 +1219,15 @@ consider_localnet_lport(const struct > > sbrec_port_binding *pb, > > > struct binding_ctx_out *b_ctx_out, > > > struct hmap *qos_map) > > > { > > > - /* Add all localnet ports to local_lports so that we > allocate > > ct zones > > > + /* Add all localnet ports to local_ifaces so that we > allocate > > ct zones > > > * for them. */ > > > - sset_add(b_ctx_out->local_lports, pb->logical_port); > > > + add_lport_to_local_lports(b_ctx_out->local_lports, > > pb->logical_port); > > > if (qos_map && b_ctx_in->ovs_idl_txn) { > > > get_qos_params(pb, qos_map); > > > } > > > > > > - update_local_lport_ids(b_ctx_out->local_lport_ids, pb); > > > + update_local_lport_ids(b_ctx_out->local_lport_ids, pb, > > > + b_ctx_out->tracked_dp_bindings); > > > } > > > > > > static bool > > > @@ -1119,7 +1255,10 @@ consider_ha_lport(const struct > > sbrec_port_binding *pb, > > > > b_ctx_in->sbrec_port_binding_by_datapath, > > > > b_ctx_in->sbrec_port_binding_by_name, > > > pb->datapath, false, > > > - b_ctx_out->local_datapaths); > > > + b_ctx_out->local_datapaths, > > > + b_ctx_out->tracked_dp_bindings); > > > + update_local_lport_ids(b_ctx_out->local_lport_ids, pb, > > > + b_ctx_out->tracked_dp_bindings); > > > } > > > > > > return consider_nonvif_lport_(pb, our_chassis, false, > > b_ctx_in, b_ctx_out); > > > @@ -1185,7 +1324,7 @@ build_local_bindings(struct binding_ctx_in > > *b_ctx_in, > > > ovs_assert(lbinding->type == BT_VIF); > > > } > > > > > > - sset_add(b_ctx_out->local_lports, iface_id); > > > + > > add_lport_to_local_lports(b_ctx_out->local_lports, iface_id); > > > smap_replace(b_ctx_out->local_iface_ids, > > iface_rec->name, > > > iface_id); > > > } > > > @@ -1241,7 +1380,7 @@ binding_run(struct binding_ctx_in > *b_ctx_in, > > struct binding_ctx_out *b_ctx_out) > > > case LP_PATCH: > > > case LP_LOCALPORT: > > > case LP_VTEP: > > > - > update_local_lport_ids(b_ctx_out->local_lport_ids, pb); > > > + update_local_lport_ids(b_ctx_out->local_lport_ids, > > pb, NULL); > > > break; > > > > > > case LP_VIF: > > > @@ -1409,7 +1548,8 @@ add_local_datapath_peer_port(const struct > > sbrec_port_binding *pb, > > > > > b_ctx_in->sbrec_port_binding_by_datapath, > > > > b_ctx_in->sbrec_port_binding_by_name, > > > peer->datapath, false, > > > - 1, b_ctx_out->local_datapaths); > > > + 1, b_ctx_out->local_datapaths, > > > + b_ctx_out->tracked_dp_bindings); > > > return; > > > } > > > > > > @@ -1471,7 +1611,8 @@ remove_pb_from_local_datapath(const struct > > sbrec_port_binding *pb, > > > struct binding_ctx_out > *b_ctx_out, > > > struct local_datapath *ld) > > > { > > > - remove_local_lport_ids(pb, b_ctx_out->local_lport_ids); > > > + remove_local_lport_ids(pb, b_ctx_out->local_lport_ids, > > > + b_ctx_out->tracked_dp_bindings); > > > if (!strcmp(pb->type, "patch") || > > > !strcmp(pb->type, "l3gateway")) { > > > remove_local_datapath_peer_port(pb, ld, > > b_ctx_out->local_datapaths); > > > @@ -1489,6 +1630,18 @@ remove_pb_from_local_datapath(const > struct > > sbrec_port_binding *pb, > > > } > > > } > > > > > > +static void > > > +update_lport_tracking(const struct sbrec_port_binding *pb, > > > + bool old_claim, bool new_claim, > > > + struct hmap *tracked_dp_bindings) > > > +{ > > > + if (!tracked_dp_bindings || !pb || (old_claim == > new_claim)) { > > > > I think pb can never be NULL here, right? > > > > > > Yeah. I'll remove the check. > > > > > > > > > + return; > > > + } > > > + > > > + tracked_binding_datapath_lport_add(pb, old_claim, > > tracked_dp_bindings); > > > +} > > > + > > > /* Considers the ovs iface 'iface_rec' for claiming. > > > * This function should be called if the external_ids:iface-id > > > * and 'ofport' are set for the 'iface_rec'. > > > @@ -1501,9 +1654,13 @@ consider_iface_claim(const struct > > ovsrec_interface *iface_rec, > > > const char *iface_id, > > > struct binding_ctx_in *b_ctx_in, > > > struct binding_ctx_out *b_ctx_out, > > > - struct hmap *qos_map) > > > + struct hmap *qos_map, > > > + bool *local_lports_changed) > > > { > > > - sset_add(b_ctx_out->local_lports, iface_id); > > > + if (add_lport_to_local_lports(b_ctx_out->local_lports, > > iface_id)) { > > > + *local_lports_changed = true; > > > + } > > > + > > > smap_replace(b_ctx_out->local_iface_ids, iface_rec->name, > > iface_id); > > > > > > struct local_binding *lbinding = > > > @@ -1566,7 +1723,8 @@ static bool > > > consider_iface_release(const struct ovsrec_interface > *iface_rec, > > > const char *iface_id, > > > struct binding_ctx_in *b_ctx_in, > > > - struct binding_ctx_out *b_ctx_out, bool > > *changed) > > > + struct binding_ctx_out *b_ctx_out, bool > > *changed, > > > + bool *local_lports_changed) > > > { > > > struct local_binding *lbinding; > > > lbinding = local_binding_find(b_ctx_out->local_bindings, > > > @@ -1585,7 +1743,8 @@ consider_iface_release(const struct > > ovsrec_interface *iface_rec, > > > * lbinding->iface. > > > * Cannot access these members of lbinding after this > > call. */ > > > if (!release_local_binding(b_ctx_in->chassis_rec, > lbinding, > > > - !b_ctx_in->ovnsb_idl_txn)) { > > > + !b_ctx_in->ovnsb_idl_txn, > > > + > b_ctx_out->tracked_dp_bindings)) { > > > return false; > > > } > > > > > > @@ -1598,7 +1757,9 @@ consider_iface_release(const struct > > ovsrec_interface *iface_rec, > > > local_binding_delete(b_ctx_out->local_bindings, > lbinding); > > > } > > > > > > - sset_find_and_delete(b_ctx_out->local_lports, iface_id); > > > + if (remove_lport_from_local_lports(b_ctx_out->local_lports, > > iface_id)) { > > > + *local_lports_changed = true; > > > + } > > > smap_remove(b_ctx_out->local_iface_ids, iface_rec->name); > > > > > > return true; > > > @@ -1630,6 +1791,8 @@ > binding_handle_ovs_interface_changes(struct > > binding_ctx_in *b_ctx_in, > > > bool handled = true; > > > *changed = false; > > > > > > + *b_ctx_out->local_lports_changed = false; > > > + > > > > Is this needed? This is part of the data we clear at every run in > > en_runtime_data_clear_tracked_data(). > > > > > > I still feel it's better to set it to false here. This function > doesn't > > know anything about > > the tracked data. > > > > > > Ok, then the reverse question, do we still need to clear > local_lports_changed in en_runtime_data_clear_tracked_data()? > > > Why not ? Do you see any issue with it ? > I was just confused why we need to reset it to false if it's already set to false every time engine_init_run() is called. But I guess that it might be that in the future binding_handle_ovs_interface_changes() could be called in other parts of the code too (although it seems very incremental engine specific) and then to be extra safe we can make sure local_lports_changed is false. > > > > > > > > > > /* Run the tracked interfaces loop twice. One to handle > deleted > > > * changes. And another to handle add/update changes. > > > * This will ensure correctness. > > > @@ -1685,7 +1848,8 @@ > binding_handle_ovs_interface_changes(struct > > binding_ctx_in *b_ctx_in, > > > > > > if (cleared_iface_id) { > > > handled = consider_iface_release(iface_rec, > > cleared_iface_id, > > > - b_ctx_in, > b_ctx_out, > > changed); > > > + b_ctx_in, > b_ctx_out, > > changed, > > > + > > b_ctx_out->local_lports_changed); > > > } > > > > > > if (!handled) { > > > @@ -1727,7 +1891,8 @@ > binding_handle_ovs_interface_changes(struct > > binding_ctx_in *b_ctx_in, > > > if (iface_id && ofport > 0) { > > > *changed = true; > > > handled = consider_iface_claim(iface_rec, iface_id, > > b_ctx_in, > > > - b_ctx_out, > qos_map_ptr); > > > + b_ctx_out, > qos_map_ptr, > > > + > > b_ctx_out->local_lports_changed); > > > if (!handled) { > > > break; > > > } > > > @@ -1792,8 +1957,7 @@ static bool > > > handle_deleted_vif_lport(const struct sbrec_port_binding *pb, > > > enum en_lport_type lport_type, > > > struct binding_ctx_in *b_ctx_in, > > > - struct binding_ctx_out *b_ctx_out, > > > - bool *changed) > > > + struct binding_ctx_out *b_ctx_out) > > > { > > > struct local_binding *lbinding = > > > get_lbinding_for_lport(pb, lport_type, b_ctx_out); > > > @@ -1804,13 +1968,12 @@ handle_deleted_vif_lport(const struct > > sbrec_port_binding *pb, > > > * clear the 'chassis' column of 'pb'. But we need > to do > > > * for the local_binding's children. */ > > > if (lbinding->type == BT_VIF && > > > - > > !release_local_binding_children(b_ctx_in->chassis_rec, > > > - lbinding, > > > - > > !b_ctx_in->ovnsb_idl_txn)) { > > > + !release_local_binding_children( > > > + b_ctx_in->chassis_rec, lbinding, > > > + !b_ctx_in->ovnsb_idl_txn, > > > + b_ctx_out->tracked_dp_bindings)) { > > > return false; > > > } > > > - > > > - *changed = true; > > > } > > > > > > handle_deleted_lport(pb, b_ctx_in, b_ctx_out); > > > @@ -1822,8 +1985,7 @@ handle_updated_vif_lport(const struct > > sbrec_port_binding *pb, > > > enum en_lport_type lport_type, > > > struct binding_ctx_in *b_ctx_in, > > > struct binding_ctx_out *b_ctx_out, > > > - struct hmap *qos_map, > > > - bool *changed) > > > + struct hmap *qos_map) > > > { > > > bool claimed = (pb->chassis == b_ctx_in->chassis_rec); > > > bool handled = true; > > > @@ -1841,14 +2003,10 @@ handle_updated_vif_lport(const struct > > sbrec_port_binding *pb, > > > } > > > > > > bool now_claimed = (pb->chassis == b_ctx_in->chassis_rec); > > > - bool changed_ = (claimed != now_claimed); > > > - > > > - if (changed_) { > > > - *changed = true; > > > - } > > > + bool changed = (claimed != now_claimed); > > > > > > if (lport_type == LP_VIRTUAL || > > > - (lport_type == LP_VIF && is_lport_container(pb)) || > > !changed_) { > > > + (lport_type == LP_VIF && is_lport_container(pb)) || > > !changed) { > > > return true; > > > } > > > > > > @@ -1898,7 +2056,7 @@ binding_handle_port_binding_changes(struct > > binding_ctx_in *b_ctx_in, > > > enum en_lport_type lport_type = get_lport_type(pb); > > > if (lport_type == LP_VIF || lport_type == LP_VIRTUAL) { > > > handled = handle_deleted_vif_lport(pb, lport_type, > > b_ctx_in, > > > - b_ctx_out, > changed); > > > + b_ctx_out); > > > } else { > > > handle_deleted_lport(pb, b_ctx_in, b_ctx_out); > > > } > > > @@ -1933,15 +2091,14 @@ > binding_handle_port_binding_changes(struct > > binding_ctx_in *b_ctx_in, > > > case LP_VIF: > > > case LP_VIRTUAL: > > > handled = handle_updated_vif_lport(pb, lport_type, > > b_ctx_in, > > > - b_ctx_out, > > qos_map_ptr, > > > - changed); > > > + b_ctx_out, > > qos_map_ptr); > > > break; > > > > > > case LP_PATCH: > > > case LP_LOCALPORT: > > > case LP_VTEP: > > > - *changed = true; > > > - > update_local_lport_ids(b_ctx_out->local_lport_ids, pb); > > > + > update_local_lport_ids(b_ctx_out->local_lport_ids, pb, > > > + > b_ctx_out->tracked_dp_bindings); > > > if (lport_type == LP_PATCH) { > > > /* Add the peer datapath to the local datapaths > > if it's > > > * not present yet. > > > @@ -1950,30 +2107,32 @@ > binding_handle_port_binding_changes(struct > > binding_ctx_in *b_ctx_in, > > > add_local_datapath_peer_port(pb, b_ctx_in, > > b_ctx_out, ld); > > > } > > > } > > > + > > > + if (lport_type == LP_VTEP) { > > > + /* VTEP lports are claimed/released by > > ovn-controller-vteps. > > > + * We are not sure what changed. So set > *changed > > to true > > > + * for any changes to vtep lports. */ > > > + *changed = true; > > > + } > > > break; > > > > > > case LP_L2GATEWAY: > > > - *changed = true; > > > handled = consider_l2gw_lport(pb, b_ctx_in, > b_ctx_out); > > > break; > > > > > > case LP_L3GATEWAY: > > > - *changed = true; > > > handled = consider_l3gw_lport(pb, b_ctx_in, > b_ctx_out); > > > break; > > > > > > case LP_CHASSISREDIRECT: > > > - *changed = true; > > > handled = consider_cr_lport(pb, b_ctx_in, > b_ctx_out); > > > break; > > > > > > case LP_EXTERNAL: > > > - *changed = true; > > > handled = consider_external_lport(pb, b_ctx_in, > > b_ctx_out); > > > break; > > > > > > case LP_LOCALNET: { > > > - *changed = true; > > > consider_localnet_lport(pb, b_ctx_in, b_ctx_out, > > qos_map_ptr); > > > > > > struct shash bridge_mappings = > > > @@ -2008,6 +2167,10 @@ > binding_handle_port_binding_changes(struct > > binding_ctx_in *b_ctx_in, > > > } > > > } > > > > > > + if (handled && > !hmap_is_empty(b_ctx_out->tracked_dp_bindings)) { > > > + *changed = true; > > > + } > > > + > > > destroy_qos_map(&qos_map); > > > return handled; > > > } > > > diff --git a/controller/binding.h b/controller/binding.h > > > index 21118ecd4..2974ebb30 100644 > > > --- a/controller/binding.h > > > +++ b/controller/binding.h > > > @@ -19,6 +19,9 @@ > > > > > > #include <stdbool.h> > > > #include "openvswitch/shash.h" > > > +#include "openvswitch/hmap.h" > > > +#include "openvswitch/uuid.h" > > > +#include "openvswitch/list.h" > > > > > > struct hmap; > > > struct ovsdb_idl; > > > @@ -54,10 +57,28 @@ struct binding_ctx_in { > > > struct binding_ctx_out { > > > struct hmap *local_datapaths; > > > struct shash *local_bindings; > > > + > > > struct sset *local_lports; > > > + > > > + /* If the sset local_lports is modified, this is set to > true by > > > + * the callee. */ > > > + bool *local_lports_changed; > > > + > > > + /* sset of local lport ids in the format > > > + * <datapath-tunnel-key>_<port-tunnel-key>. */ > > > > Nit: shouldn't this comment be part of patch 1? > > > > > > Could be. > > > > > > > > > struct sset *local_lport_ids; > > > + > > > struct sset *egress_ifaces; > > > + > > > + /* smap of OVS interface name as key and > > > + * OVS interface external_ids:iface-id as value. */ > > > > Nit: shouldn't this comment be part of patch 2? > > > > > > Could be. > > > > > > > struct smap *local_iface_ids; > > > + > > > + /* hmap of 'struct tracked_binding_datapath' which the > > > + * callee (binding_handle_ovs_interface_changes and > > > + * binding_handle_port_binding_changes) fills in for > > > + * the changed datapaths and port bindings. */ > > > + struct hmap *tracked_dp_bindings; > > > }; > > > > > > enum local_binding_type { > > > @@ -82,6 +103,21 @@ local_binding_find(struct shash > > *local_bindings, const char *name) > > > return shash_find_data(local_bindings, name); > > > } > > > > > > +/* Represents a tracked binding logical port. */ > > > +struct tracked_binding_lport { > > > + const struct sbrec_port_binding *pb; > > > + struct ovs_list list_node; > > > > Nit: for consistency with tracked_binding_datapath and because > list_node > > is usually accessed before pb I'd define list_node before pb. > > > > > > Actually we don't need list_node now. It was a leftover from the > > previous versions. > > I'll delete it. Thanks. > > > > > > > > > + bool deleted; > > > +}; > > > + > > > +/* Represent a tracked binding datapath. */ > > > +struct tracked_binding_datapath { > > > + struct hmap_node node; > > > + const struct sbrec_datapath_binding *dp; > > > + bool is_new; > > > > I think it would be easier to read the code if both > > tracked_binding_lport and tracked_binding_datapath would use > the same > > logic for tracking updates: either both should have "deleted" > or both > > should have "is_new". > > > > > > Ack. > > > > > > > > Also, given that we never use tracked_binding_lport.deleted, > maybe we > > can skip it completely. What do you think? > > > > > > I'll delete the "deleted". We can add it later if it is really > required. > > > > > > > > > + struct shash lports; /* shash of struct > tracked_binding_lport. */ > > > +}; > > > + > > > void binding_register_ovs_idl(struct ovsdb_idl *); > > > void binding_run(struct binding_ctx_in *, struct > binding_ctx_out *); > > > bool binding_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn, > > > @@ -96,4 +132,5 @@ bool > > binding_handle_ovs_interface_changes(struct binding_ctx_in *, > > > bool binding_handle_port_binding_changes(struct > binding_ctx_in *, > > > struct > binding_ctx_out *, > > > bool *changed); > > > +void binding_tracked_dp_destroy(struct hmap > *tracked_datapaths); > > > #endif /* controller/binding.h */ > > > diff --git a/controller/ovn-controller.c > b/controller/ovn-controller.c > > > index 795a1c297..98652866c 100644 > > > --- a/controller/ovn-controller.c > > > +++ b/controller/ovn-controller.c > > > @@ -973,10 +973,88 @@ struct ed_type_runtime_data { > > > struct sset local_lport_ids; > > > struct sset active_tunnels; > > > > > > + /* runtime data engine private data. */ > > > struct sset egress_ifaces; > > > struct smap local_iface_ids; > > > + > > > + /* Tracked data. See below for more details and > comments. */ > > > + bool tracked; > > > > I wonder if the "tracked" field should actually be part of the > generic > > engine_node_input. We use it for port groups and address sets too > > ("change_tracked" in ed_type_addr_sets and > ed_type_port_groups) and > > afaiu the semantics are always: > > - "tracked" is set to true if a node's input handler was called > > - "tracked" is reset to false if a node's run handler was called > > > > So all this could be implemented generically in inc-proc-eng.c. > > > > What do you think? > > > > > > Could be. But I'd prefer to be a separate patch (not part of this > series). > > > > Ok, but please see my comment regarding > en_runtime_data_clear_tracked_data() in en_runtime_data_run(). > > > > > > > > + bool local_lports_changed; > > > > Do we really need local_lports_changed? Isn't it actually > equivalent to > > !hmap_is_empty(& > > > > > + struct hmap tracked_dp_bindings; > > > }; > > > > > > Not really. > > > > Let's say you run the command - "ovs-vsctl add-port br-int p0 -- set > > interface p0 externa_ids:iface-id=lport1" > > > > And if logical lport lport1 doesn't exist (or not seen by > ovn-controller > > due to conditional monitoring), we > > need to update the runtime_data engine node so that > "update_sb_monitor" > > is called. > > > > > > Ah, right, thanks for the explanation. > > > > > > > > > +/* struct ed_type_runtime_data has the below members for > tracking the > > > + * changes done to the runtime_data engine by the > runtime_data engine > > > + * handlers. Since this engine is an input to the > flow_output engine, > > > + * the flow output runtime data handler will make use of this > > tracked data. > > > + * > > > + * > > > ------------------------------------------------------------------------ > > > + * | | This is a hmap of > > > | > > > + * | | 'struct tracked_binding_datapath' > > defined in | > > > + * | | binding.h. Runtime data > handlers for > > OVS | > > > + * | | Interface and Port Binding changes > > store the | > > > + * | @tracked_dp_bindings | changed datapaths (datapaths > > added/removed from | > > > + * | | local_datapaths) and changed port > > bindings | > > > + * | | (added/updated/deleted in > > 'local_bindings'). | > > > > This is not really accurate, right? Afaiu we only track if a port > > binding is added/deleted. > > > > > > Not all the time. We can bind an "external" port when the port > binding gets > > updated due to the command - "ovn-nbctl lsp-set-type <lport> external" > > or for the command - "ovn-nbctl lsp-set-options router-port=<peer > port> > > > > > > OK. > > > > > > > > > > + * | | So any changes to the runtime > data - > > | > > > + * | | local_datapaths and > local_bindings is > > captured | > > > + * | | here. > > > | > > > + * > > > ------------------------------------------------------------------------ > > > + * | | This is a bool which represents if > > the runtime | > > > + * | | data 'local_lports' changed by the > > runtime data | > > > + * | | handlers for OVS Interface and Port > > Binding | > > > + * | | changes. If 'local_lports' is > updated > > and also | > > > + * | | results in any port binding > updates, > > it is | > > > + * |@local_lports_changed | captured in the > @tracked_dp_bindings. > > So there | > > > + * | | is no need to capture the > changes in > > the | > > > + * | | local_lports. If > > @local_lports_changed is true | > > > + * | | but without anydata in the > > @tracked_dp_bindings,| > > > + * | | it means we needto only update > the SB > > monitor | > > > + * | | clauses and there isno need for any > > flow | > > > + * | | (re)computations. > > > | > > > + * > > > ------------------------------------------------------------------------ > > > + * | | This represents if the data was > > tracked or not | > > > + * | | by the runtime data handlers during > > the engine | > > > + * | @tracked | run. If the runtime data recompute > > is | > > > + * | | triggered, it means there is no > > tracked data. | > > > + * > > > ------------------------------------------------------------------------ > > > + * > > > + * > > > + * The changes to the following runtime_data variables are not > > tracked. > > > + * > > > + * > > > --------------------------------------------------------------------- > > > + * | local_datapaths | The changes to these runtime data is > > captured in | > > > + * | local_bindings | the @tracked_dp_bindings indirectly and > > hence it | > > > + * | local_lport_ids | is not tracked explicitly. > > > | > > > + * > > > --------------------------------------------------------------------- > > > + * | local_iface_ids | This is used internally within the > > runtime data | > > > + * | egress_ifaces | engine (used only in binding.c) and > hence > > there | > > > + * | | there is no need to track. > > > | > > > + * > > > --------------------------------------------------------------------- > > > + * | | Active tunnels is built in the > > > | > > > + * | | bfd_calculate_active_tunnels() for the > > tunnel | > > > + * | | OVS interfaces. Any changes to non VIF > > OVS | > > > + * | | interfaces results in triggering the > > full | > > > + * | active_tunnels | recompute of runtime data engine and > > hence there | > > > + * | | the tracked data doesn't track it. When > > we | > > > + * | | support handling changes to non VIF > OVS > > | > > > + * | | interfaces we need to track the changes > > to the | > > > + * | | active tunnels. > > > | > > > + * > > > --------------------------------------------------------------------- > > > + * > > > + */ > > > + > > > +static void > > > +en_runtime_data_clear_tracked_data(void *data_) > > > +{ > > > + struct ed_type_runtime_data *data = data_; > > > + > > > + binding_tracked_dp_destroy(&data->tracked_dp_bindings); > > > + hmap_init(&data->tracked_dp_bindings); > > > > Nit: just as we did for > local_bindings_init()/local_bindings_destroy() > > I'd move the "hmap_init(&data->tracked_dp_bindings)" inside a new > > function called binding_tracked_dp_init(). > > > > > > Not sure if we want to have the same functions here just to init > the hmap. > > > > > > Well, local_bindings_init(), just does shash_init() so I don't really > see why not :) > > > I don't see any advantage, but rather an unnecessary function overhead. > It was just to group create/delete implementations together but it's not a big deal I guess. > So, I'd live it as is. > Ok. Thanks, Dumitru > > > > > > > + data->local_lports_changed = false; > > > + data->tracked = false; > > > +} > > > + > > > static void * > > > en_runtime_data_init(struct engine_node *node OVS_UNUSED, > > > struct engine_arg *arg OVS_UNUSED) > > > @@ -990,6 +1068,10 @@ en_runtime_data_init(struct engine_node > > *node OVS_UNUSED, > > > sset_init(&data->egress_ifaces); > > > smap_init(&data->local_iface_ids); > > > local_bindings_init(&data->local_bindings); > > > + > > > + /* init the tracked data. */ > > > + hmap_init(&data->tracked_dp_bindings); > > > + > > > return data; > > > } > > > > > > @@ -1012,6 +1094,8 @@ en_runtime_data_cleanup(void *data) > > > } > > > hmap_destroy(&rt_data->local_datapaths); > > > local_bindings_destroy(&rt_data->local_bindings); > > > + > > > + en_runtime_data_clear_tracked_data(data); > > > } > > > > > > static void > > > @@ -1092,6 +1176,8 @@ init_binding_ctx(struct engine_node *node, > > > b_ctx_out->egress_ifaces = &rt_data->egress_ifaces; > > > b_ctx_out->local_bindings = &rt_data->local_bindings; > > > b_ctx_out->local_iface_ids = &rt_data->local_iface_ids; > > > + b_ctx_out->tracked_dp_bindings = NULL; > > > + b_ctx_out->local_lports_changed = NULL; > > > } > > > > > > static void > > > @@ -1103,6 +1189,13 @@ en_runtime_data_run(struct engine_node > > *node, void *data) > > > struct sset *local_lport_ids = &rt_data->local_lport_ids; > > > struct sset *active_tunnels = &rt_data->active_tunnels; > > > > > > + /* Clear the tracked data. Even though the tracked data > > > + * gets cleared in the beginning of engine_init_run(), > > > + * any of the runtime data handler might have set some > tracked > > > + * data and later another runtime data handler might > return false > > > + * resulting full recompute of runtime engine. */ > > > + en_runtime_data_clear_tracked_data(data); > > > + > > > > Could you please elaborate more on this? Why is it an issue if > we keep > > the "tracked" data until the end of the run? > > > > > > > > When a runtime data node gets updated (engine_set_node_state()) either > > due to > > full recompute of runtime or due to handler setting it, > > flow_output_runtime_data_handler() > > will not trigger flow_output recompute unless it see > runtime->tracked = > > false. > > > > Hence it clears the tracked data. Instead of calling > > en_runtime_data_clear_tracked_data(), > > en_runtime_data_run() can set just 'tracked' to false, but I think > it's > > better to clear > > the whole tracked data. > > > > So this brings us back to my original comment regarding "tracked". If > tracked would be part of the I-P engine internal input node fields, it > would get set before running en_runtime_data_run() and we wouldn't have > to explain why we need to clear the "tracked" field here. > > > We may still have to clear the tracked data if some of the handlers > added some > data to it before one of the handlers gives up and we fall back to fully > recompute > given that we don't clear the tracked data at the end of the engine. > > Also I've not thought about it and not sure if we really want to move to > engine. > > It could be a good idea. But I'll leave it to others or you, if you want > to work on this > as a follow up patch. > > > > If we decide to address the "tracked" fields as a follow up patch I > think it'd be good to add some TODO/FIXME/XXX comments so we don't > forget about addressing this later. E.g., there's already " > /* XXX: The change_tracked check may be added to inc-proc framework. */" > for address sets. > > > > I'll add a comment that it could be moved to the engine. > > Thanks > Numan > > > Thanks, > Dumitru > > > > > Thanks, > > Dumitru > > > > > static bool first_run = true; > > > if (first_run) { > > > /* don't cleanup since there is no data yet */ > > > @@ -1158,6 +1251,9 @@ runtime_data_ovs_interface_handler(struct > > engine_node *node, void *data) > > > struct binding_ctx_in b_ctx_in; > > > struct binding_ctx_out b_ctx_out; > > > init_binding_ctx(node, rt_data, &b_ctx_in, &b_ctx_out); > > > + rt_data->tracked = true; > > > + b_ctx_out.tracked_dp_bindings = > &rt_data->tracked_dp_bindings; > > > + b_ctx_out.local_lports_changed = > &rt_data->local_lports_changed; > > > > > > bool changed = false; > > > if (!binding_handle_ovs_interface_changes(&b_ctx_in, > &b_ctx_out, > > > @@ -1190,6 +1286,9 @@ > runtime_data_sb_port_binding_handler(struct > > engine_node *node, void *data) > > > return false; > > > } > > > > > > + rt_data->tracked = true; > > > + b_ctx_out.tracked_dp_bindings = > &rt_data->tracked_dp_bindings; > > > + > > > bool changed = false; > > > if (!binding_handle_port_binding_changes(&b_ctx_in, > &b_ctx_out, > > > &changed)) { > > > @@ -1797,6 +1896,39 @@ flow_output_port_groups_handler(struct > > engine_node *node, void *data) > > > return _flow_output_resource_ref_handler(node, data, > > REF_TYPE_PORTGROUP); > > > } > > > > > > +static bool > > > +flow_output_runtime_data_handler(struct engine_node *node, > > > + void *data OVS_UNUSED) > > > +{ > > > + struct ed_type_runtime_data *rt_data = > > > + engine_get_input_data("runtime_data", node); > > > + > > > + /* There is no tracked data. Fall back to full recompute of > > > + * flow_output. */ > > > + if (!rt_data->tracked) { > > > + return false; > > > + } > > > + > > > + if (!hmap_is_empty(&rt_data->tracked_dp_bindings)) { > > > + /* We are not yet handling the tracked datapath binding > > > + * changes. Return false to trigger full recompute. */ > > > + return false; > > > + } > > > + > > > + if (rt_data->local_lports_changed) { > > > + engine_set_node_state(node, EN_UPDATED); > > > + } > > > + > > > + return true; > > > +} > > > + > > > +static bool > > > +flow_output_noop_handler(struct engine_node *node OVS_UNUSED, > > > + void *data OVS_UNUSED) > > > +{ > > > + return true; > > > +} > > > + > > > /* Engine node en_physical_flow_changes indicates whether > > > * there is a need to > > > * - recompute only physical flows or > > > @@ -1908,13 +2040,6 @@ > > flow_output_physical_flow_changes_handler(struct engine_node > *node, > > void *data) > > > return true; > > > } > > > > > > -static bool > > > -flow_output_noop_handler(struct engine_node *node OVS_UNUSED, > > > - void *data OVS_UNUSED) > > > -{ > > > - return true; > > > -} > > > - > > > struct ovn_controller_exit_args { > > > bool *exiting; > > > bool *restart; > > > @@ -2036,7 +2161,7 @@ main(int argc, char *argv[]) > > > > > > /* Define inc-proc-engine nodes. */ > > > ENGINE_NODE_CUSTOM_DATA(ct_zones, "ct_zones"); > > > - ENGINE_NODE(runtime_data, "runtime_data"); > > > + ENGINE_NODE_WITH_CLEAR_TRACK_DATA(runtime_data, > "runtime_data"); > > > ENGINE_NODE(mff_ovn_geneve, "mff_ovn_geneve"); > > > ENGINE_NODE(ofctrl_is_connected, "ofctrl_is_connected"); > > > ENGINE_NODE_WITH_CLEAR_TRACK_DATA(physical_flow_changes, > > > @@ -2073,7 +2198,8 @@ main(int argc, char *argv[]) > > > flow_output_addr_sets_handler); > > > engine_add_input(&en_flow_output, &en_port_groups, > > > flow_output_port_groups_handler); > > > - engine_add_input(&en_flow_output, &en_runtime_data, NULL); > > > + engine_add_input(&en_flow_output, &en_runtime_data, > > > + flow_output_runtime_data_handler); > > > engine_add_input(&en_flow_output, &en_mff_ovn_geneve, > NULL); > > > engine_add_input(&en_flow_output, > &en_physical_flow_changes, > > > > flow_output_physical_flow_changes_handler); > > > diff --git a/tests/ovn-performance.at > <http://ovn-performance.at> <http://ovn-performance.at> > > b/tests/ovn-performance.at <http://ovn-performance.at> > <http://ovn-performance.at> > > > index 5cc1960b6..08acd3bae 100644 > > > --- a/tests/ovn-performance.at <http://ovn-performance.at> > <http://ovn-performance.at> > > > +++ b/tests/ovn-performance.at <http://ovn-performance.at> > <http://ovn-performance.at> > > > @@ -286,11 +286,11 @@ for i in 1 2; do > > > [hv1 hv2], [lflow_run], > > > [ovn-nbctl --wait=hv lsp-set-type $lsp router] > > > ) > > > - OVN_CONTROLLER_EXPECT_HIT( > > > + OVN_CONTROLLER_EXPECT_NO_HIT( > > > [hv1 hv2], [lflow_run], > > > [ovn-nbctl --wait=hv lsp-set-options $lsp > router-port=$lrp] > > > ) > > > - OVN_CONTROLLER_EXPECT_HIT( > > > + OVN_CONTROLLER_EXPECT_NO_HIT( > > > [hv1 hv2], [lflow_run], > > > [ovn-nbctl --wait=hv lsp-set-addresses $lsp router] > > > ) > > > @@ -416,15 +416,15 @@ OVN_CONTROLLER_EXPECT_NO_HIT( > > > [ovn-nbctl --wait=hv destroy Port_Group pg1] > > > ) > > > > > > -for i in 1 2; do > > > - ls=ls$i > > > +OVN_CONTROLLER_EXPECT_HIT( > > > + [hv1 hv2], [lflow_run], > > > + [ovn-nbctl --wait=hv ls-del ls1] > > > +) > > > > > > - # Delete switch $ls > > > - OVN_CONTROLLER_EXPECT_HIT( > > > - [hv1 hv2], [lflow_run], > > > - [ovn-nbctl --wait=hv ls-del $ls] > > > - ) > > > -done > > > +OVN_CONTROLLER_EXPECT_NO_HIT( > > > + [hv1 hv2], [lflow_run], > > > + [ovn-nbctl --wait=hv ls-del ls2] > > > +) > > > > > > # Delete router lr1 > > > OVN_CONTROLLER_EXPECT_NO_HIT( > > > > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org <mailto:dev@openvswitch.org> > <mailto:dev@openvswitch.org <mailto:dev@openvswitch.org>> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org <mailto:dev@openvswitch.org> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/controller/binding.c b/controller/binding.c index c2d9a4c6b..3c226cd7d 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -69,13 +69,26 @@ binding_register_ovs_idl(struct ovsdb_idl *ovs_idl) ovsdb_idl_add_column(ovs_idl, &ovsrec_qos_col_type); } +static struct tracked_binding_datapath *tracked_binding_datapath_create( + const struct sbrec_datapath_binding *, + bool is_new, struct hmap *tracked_dps); +static struct tracked_binding_datapath *tracked_binding_datapath_find( + struct hmap *, const struct sbrec_datapath_binding *); +static void tracked_binding_datapath_lport_add( + const struct sbrec_port_binding *, bool deleted, + struct hmap *tracked_datapaths); +static void update_lport_tracking(const struct sbrec_port_binding *pb, + bool old_claim, bool new_claim, + struct hmap *tracked_dp_bindings); + static void add_local_datapath__(struct ovsdb_idl_index *sbrec_datapath_binding_by_key, struct ovsdb_idl_index *sbrec_port_binding_by_datapath, struct ovsdb_idl_index *sbrec_port_binding_by_name, const struct sbrec_datapath_binding *datapath, bool has_local_l3gateway, int depth, - struct hmap *local_datapaths) + struct hmap *local_datapaths, + struct hmap *tracked_datapaths) { uint32_t dp_key = datapath->tunnel_key; struct local_datapath *ld = get_local_datapath(local_datapaths, dp_key); @@ -92,6 +105,11 @@ add_local_datapath__(struct ovsdb_idl_index *sbrec_datapath_binding_by_key, ld->localnet_port = NULL; ld->has_local_l3gateway = has_local_l3gateway; + if (tracked_datapaths && + !tracked_binding_datapath_find(tracked_datapaths, datapath)) { + tracked_binding_datapath_create(datapath, true, tracked_datapaths); + } + if (depth >= 100) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); VLOG_WARN_RL(&rl, "datapaths nested too deep"); @@ -124,7 +142,8 @@ add_local_datapath__(struct ovsdb_idl_index *sbrec_datapath_binding_by_key, sbrec_port_binding_by_datapath, sbrec_port_binding_by_name, peer->datapath, false, - depth + 1, local_datapaths); + depth + 1, local_datapaths, + tracked_datapaths); } ld->n_peer_ports++; if (ld->n_peer_ports > ld->n_allocated_peer_ports) { @@ -147,12 +166,14 @@ add_local_datapath(struct ovsdb_idl_index *sbrec_datapath_binding_by_key, struct ovsdb_idl_index *sbrec_port_binding_by_datapath, struct ovsdb_idl_index *sbrec_port_binding_by_name, const struct sbrec_datapath_binding *datapath, - bool has_local_l3gateway, struct hmap *local_datapaths) + bool has_local_l3gateway, struct hmap *local_datapaths, + struct hmap *tracked_datapaths) { add_local_datapath__(sbrec_datapath_binding_by_key, sbrec_port_binding_by_datapath, sbrec_port_binding_by_name, - datapath, has_local_l3gateway, 0, local_datapaths); + datapath, has_local_l3gateway, 0, local_datapaths, + tracked_datapaths); } static void @@ -473,23 +494,45 @@ update_ld_localnet_port(const struct sbrec_port_binding *binding_rec, static void update_local_lport_ids(struct sset *local_lport_ids, - const struct sbrec_port_binding *pb) + const struct sbrec_port_binding *pb, + struct hmap *tracked_datapaths) { char buf[16]; snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64, pb->datapath->tunnel_key, pb->tunnel_key); - sset_add(local_lport_ids, buf); + bool added = sset_add(local_lport_ids, buf); + if (added && tracked_datapaths) { + /* Add the 'pb' to the tracked_datapaths. */ + tracked_binding_datapath_lport_add(pb, false, tracked_datapaths); + } } static void -remove_local_lport_ids(const struct sbrec_port_binding *binding_rec, - struct sset *local_lport_ids) +remove_local_lport_ids(const struct sbrec_port_binding *pb, + struct sset *local_lport_ids, + struct hmap *tracked_datapaths) { char buf[16]; snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64, - binding_rec->datapath->tunnel_key, - binding_rec->tunnel_key); - sset_find_and_delete(local_lport_ids, buf); + pb->datapath->tunnel_key, pb->tunnel_key); + bool deleted = sset_find_and_delete(local_lport_ids, buf); + if (deleted && tracked_datapaths) { + /* Add the 'pb' to the tracked_datapaths. */ + tracked_binding_datapath_lport_add(pb, true, tracked_datapaths); + } +} + +static bool +add_lport_to_local_lports(struct sset *local_lports, const char *lport_name) +{ + return !!sset_add(local_lports, lport_name); +} + +static bool +remove_lport_from_local_lports(struct sset *local_lports, + const char *lport_name) +{ + return sset_find_and_delete(local_lports, lport_name); } /* Local bindings. binding.c module binds the logical port (represented by @@ -637,6 +680,77 @@ is_lport_container(const struct sbrec_port_binding *pb) return is_lport_vif(pb) && pb->parent_port && pb->parent_port[0]; } +static struct tracked_binding_datapath * +tracked_binding_datapath_create(const struct sbrec_datapath_binding *dp, + bool is_new, + struct hmap *tracked_datapaths) +{ + struct tracked_binding_datapath *t_dp = xzalloc(sizeof *t_dp); + t_dp->dp = dp; + t_dp->is_new = is_new; + shash_init(&t_dp->lports); + hmap_insert(tracked_datapaths, &t_dp->node, uuid_hash(&dp->header_.uuid)); + return t_dp; +} + +static struct tracked_binding_datapath * +tracked_binding_datapath_find(struct hmap *tracked_datapaths, + const struct sbrec_datapath_binding *dp) +{ + struct tracked_binding_datapath *t_dp; + size_t hash = uuid_hash(&dp->header_.uuid); + HMAP_FOR_EACH_WITH_HASH (t_dp, node, hash, tracked_datapaths) { + if (uuid_equals(&t_dp->dp->header_.uuid, &dp->header_.uuid)) { + return t_dp; + } + } + + return NULL; +} + +static void +tracked_binding_datapath_lport_add(const struct sbrec_port_binding *pb, + bool deleted, + struct hmap *tracked_datapaths) +{ + if (!tracked_datapaths) { + return; + } + + struct tracked_binding_datapath *tracked_dp = + tracked_binding_datapath_find(tracked_datapaths, pb->datapath); + if (!tracked_dp) { + tracked_dp = tracked_binding_datapath_create(pb->datapath, false, + tracked_datapaths); + } + + /* Check if the lport is already present or not. + * If it is already present, then just update the 'pb' and 'deleted' + * fields. */ + struct tracked_binding_lport *lport = + shash_find_data(&tracked_dp->lports, pb->logical_port); + + if (!lport) { + lport = xmalloc(sizeof *lport); + shash_add(&tracked_dp->lports, pb->logical_port, lport); + } + + lport->pb = pb; + lport->deleted = deleted; +} + +void +binding_tracked_dp_destroy(struct hmap *tracked_datapaths) +{ + struct tracked_binding_datapath *t_dp; + HMAP_FOR_EACH_POP (t_dp, node, tracked_datapaths) { + shash_destroy_free_data(&t_dp->lports); + free(t_dp); + } + + hmap_destroy(tracked_datapaths); +} + /* Corresponds to each Port_Binding.type. */ enum en_lport_type { LP_UNKNOWN, @@ -690,7 +804,7 @@ static bool claim_lport(const struct sbrec_port_binding *pb, const struct sbrec_chassis *chassis_rec, const struct ovsrec_interface *iface_rec, - bool sb_readonly) + bool sb_readonly, struct hmap *tracked_datapaths) { if (pb->chassis != chassis_rec) { if (sb_readonly) { @@ -709,6 +823,10 @@ claim_lport(const struct sbrec_port_binding *pb, } sbrec_port_binding_set_chassis(pb, chassis_rec); + + if (tracked_datapaths) { + update_lport_tracking(pb, false, true, tracked_datapaths); + } } /* Check if the port encap binding, if any, has changed */ @@ -726,9 +844,14 @@ claim_lport(const struct sbrec_port_binding *pb, /* Returns false if lport is not released due to 'sb_readonly'. * Returns true otherwise. + * + * This function assumes that that the the 'pb' was claimed + * earlier i.e port binding's chassis is set to this chassis. + * Caller should make sure that, that's the case. */ static bool -release_lport(const struct sbrec_port_binding *pb, bool sb_readonly) +release_lport(const struct sbrec_port_binding *pb, bool sb_readonly, + struct hmap *tracked_datapaths) { if (pb->encap) { if (sb_readonly) { @@ -751,6 +874,7 @@ release_lport(const struct sbrec_port_binding *pb, bool sb_readonly) sbrec_port_binding_set_virtual_parent(pb, NULL); } + update_lport_tracking(pb, true, false, tracked_datapaths); VLOG_INFO("Releasing lport %s from this chassis.", pb->logical_port); return true; } @@ -796,13 +920,14 @@ is_lbinding_container_parent(struct local_binding *lbinding) static bool release_local_binding_children(const struct sbrec_chassis *chassis_rec, struct local_binding *lbinding, - bool sb_readonly) + bool sb_readonly, + struct hmap *tracked_dp_bindings) { struct shash_node *node; SHASH_FOR_EACH (node, &lbinding->children) { struct local_binding *l = node->data; if (is_lbinding_this_chassis(l, chassis_rec)) { - if (!release_lport(l->pb, sb_readonly)) { + if (!release_lport(l->pb, sb_readonly, tracked_dp_bindings)) { return false; } } @@ -817,16 +942,17 @@ release_local_binding_children(const struct sbrec_chassis *chassis_rec, static bool release_local_binding(const struct sbrec_chassis *chassis_rec, - struct local_binding *lbinding, bool sb_readonly) + struct local_binding *lbinding, bool sb_readonly, + struct hmap *tracked_dp_bindings) { if (!release_local_binding_children(chassis_rec, lbinding, - sb_readonly)) { + sb_readonly, tracked_dp_bindings)) { return false; } bool retval = true; if (is_lbinding_this_chassis(lbinding, chassis_rec)) { - retval = release_lport(lbinding->pb, sb_readonly); + retval = release_lport(lbinding->pb, sb_readonly, tracked_dp_bindings); } lbinding->pb = NULL; @@ -847,7 +973,8 @@ consider_vif_lport_(const struct sbrec_port_binding *pb, if (can_bind) { /* We can claim the lport. */ if (!claim_lport(pb, b_ctx_in->chassis_rec, lbinding->iface, - !b_ctx_in->ovnsb_idl_txn)){ + !b_ctx_in->ovnsb_idl_txn, + b_ctx_out->tracked_dp_bindings)){ return false; } @@ -855,8 +982,10 @@ consider_vif_lport_(const struct sbrec_port_binding *pb, b_ctx_in->sbrec_port_binding_by_datapath, b_ctx_in->sbrec_port_binding_by_name, pb->datapath, false, - b_ctx_out->local_datapaths); - update_local_lport_ids(b_ctx_out->local_lport_ids, pb); + b_ctx_out->local_datapaths, + b_ctx_out->tracked_dp_bindings); + update_local_lport_ids(b_ctx_out->local_lport_ids, pb, + b_ctx_out->tracked_dp_bindings); if (lbinding->iface && qos_map && b_ctx_in->ovs_idl_txn) { get_qos_params(pb, qos_map); } @@ -875,7 +1004,8 @@ consider_vif_lport_(const struct sbrec_port_binding *pb, if (pb->chassis == b_ctx_in->chassis_rec) { /* Release the lport if there is no lbinding. */ if (!lbinding_set || !can_bind) { - return release_lport(pb, !b_ctx_in->ovnsb_idl_txn); + return release_lport(pb, !b_ctx_in->ovnsb_idl_txn, + b_ctx_out->tracked_dp_bindings); } } @@ -962,7 +1092,8 @@ consider_container_lport(const struct sbrec_port_binding *pb, * release the container lport, if it was bound earlier. */ if (is_lbinding_this_chassis(container_lbinding, b_ctx_in->chassis_rec)) { - return release_lport(pb, !b_ctx_in->ovnsb_idl_txn); + return release_lport(pb, !b_ctx_in->ovnsb_idl_txn, + b_ctx_out->tracked_dp_bindings); } return true; @@ -1037,18 +1168,22 @@ consider_nonvif_lport_(const struct sbrec_port_binding *pb, struct binding_ctx_out *b_ctx_out) { if (our_chassis) { - sset_add(b_ctx_out->local_lports, pb->logical_port); + add_lport_to_local_lports(b_ctx_out->local_lports, pb->logical_port); add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key, b_ctx_in->sbrec_port_binding_by_datapath, b_ctx_in->sbrec_port_binding_by_name, pb->datapath, has_local_l3gateway, - b_ctx_out->local_datapaths); + b_ctx_out->local_datapaths, + b_ctx_out->tracked_dp_bindings); - update_local_lport_ids(b_ctx_out->local_lport_ids, pb); + update_local_lport_ids(b_ctx_out->local_lport_ids, pb, + b_ctx_out->tracked_dp_bindings); return claim_lport(pb, b_ctx_in->chassis_rec, NULL, - !b_ctx_in->ovnsb_idl_txn); + !b_ctx_in->ovnsb_idl_txn, + b_ctx_out->tracked_dp_bindings); } else if (pb->chassis == b_ctx_in->chassis_rec) { - return release_lport(pb, !b_ctx_in->ovnsb_idl_txn); + return release_lport(pb, !b_ctx_in->ovnsb_idl_txn, + b_ctx_out->tracked_dp_bindings); } return true; @@ -1084,14 +1219,15 @@ consider_localnet_lport(const struct sbrec_port_binding *pb, struct binding_ctx_out *b_ctx_out, struct hmap *qos_map) { - /* Add all localnet ports to local_lports so that we allocate ct zones + /* Add all localnet ports to local_ifaces so that we allocate ct zones * for them. */ - sset_add(b_ctx_out->local_lports, pb->logical_port); + add_lport_to_local_lports(b_ctx_out->local_lports, pb->logical_port); if (qos_map && b_ctx_in->ovs_idl_txn) { get_qos_params(pb, qos_map); } - update_local_lport_ids(b_ctx_out->local_lport_ids, pb); + update_local_lport_ids(b_ctx_out->local_lport_ids, pb, + b_ctx_out->tracked_dp_bindings); } static bool @@ -1119,7 +1255,10 @@ consider_ha_lport(const struct sbrec_port_binding *pb, b_ctx_in->sbrec_port_binding_by_datapath, b_ctx_in->sbrec_port_binding_by_name, pb->datapath, false, - b_ctx_out->local_datapaths); + b_ctx_out->local_datapaths, + b_ctx_out->tracked_dp_bindings); + update_local_lport_ids(b_ctx_out->local_lport_ids, pb, + b_ctx_out->tracked_dp_bindings); } return consider_nonvif_lport_(pb, our_chassis, false, b_ctx_in, b_ctx_out); @@ -1185,7 +1324,7 @@ build_local_bindings(struct binding_ctx_in *b_ctx_in, ovs_assert(lbinding->type == BT_VIF); } - sset_add(b_ctx_out->local_lports, iface_id); + add_lport_to_local_lports(b_ctx_out->local_lports, iface_id); smap_replace(b_ctx_out->local_iface_ids, iface_rec->name, iface_id); } @@ -1241,7 +1380,7 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) case LP_PATCH: case LP_LOCALPORT: case LP_VTEP: - update_local_lport_ids(b_ctx_out->local_lport_ids, pb); + update_local_lport_ids(b_ctx_out->local_lport_ids, pb, NULL); break; case LP_VIF: @@ -1409,7 +1548,8 @@ add_local_datapath_peer_port(const struct sbrec_port_binding *pb, b_ctx_in->sbrec_port_binding_by_datapath, b_ctx_in->sbrec_port_binding_by_name, peer->datapath, false, - 1, b_ctx_out->local_datapaths); + 1, b_ctx_out->local_datapaths, + b_ctx_out->tracked_dp_bindings); return; } @@ -1471,7 +1611,8 @@ remove_pb_from_local_datapath(const struct sbrec_port_binding *pb, struct binding_ctx_out *b_ctx_out, struct local_datapath *ld) { - remove_local_lport_ids(pb, b_ctx_out->local_lport_ids); + remove_local_lport_ids(pb, b_ctx_out->local_lport_ids, + b_ctx_out->tracked_dp_bindings); if (!strcmp(pb->type, "patch") || !strcmp(pb->type, "l3gateway")) { remove_local_datapath_peer_port(pb, ld, b_ctx_out->local_datapaths); @@ -1489,6 +1630,18 @@ remove_pb_from_local_datapath(const struct sbrec_port_binding *pb, } } +static void +update_lport_tracking(const struct sbrec_port_binding *pb, + bool old_claim, bool new_claim, + struct hmap *tracked_dp_bindings) +{ + if (!tracked_dp_bindings || !pb || (old_claim == new_claim)) { + return; + } + + tracked_binding_datapath_lport_add(pb, old_claim, tracked_dp_bindings); +} + /* Considers the ovs iface 'iface_rec' for claiming. * This function should be called if the external_ids:iface-id * and 'ofport' are set for the 'iface_rec'. @@ -1501,9 +1654,13 @@ consider_iface_claim(const struct ovsrec_interface *iface_rec, const char *iface_id, struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out, - struct hmap *qos_map) + struct hmap *qos_map, + bool *local_lports_changed) { - sset_add(b_ctx_out->local_lports, iface_id); + if (add_lport_to_local_lports(b_ctx_out->local_lports, iface_id)) { + *local_lports_changed = true; + } + smap_replace(b_ctx_out->local_iface_ids, iface_rec->name, iface_id); struct local_binding *lbinding = @@ -1566,7 +1723,8 @@ static bool consider_iface_release(const struct ovsrec_interface *iface_rec, const char *iface_id, struct binding_ctx_in *b_ctx_in, - struct binding_ctx_out *b_ctx_out, bool *changed) + struct binding_ctx_out *b_ctx_out, bool *changed, + bool *local_lports_changed) { struct local_binding *lbinding; lbinding = local_binding_find(b_ctx_out->local_bindings, @@ -1585,7 +1743,8 @@ consider_iface_release(const struct ovsrec_interface *iface_rec, * lbinding->iface. * Cannot access these members of lbinding after this call. */ if (!release_local_binding(b_ctx_in->chassis_rec, lbinding, - !b_ctx_in->ovnsb_idl_txn)) { + !b_ctx_in->ovnsb_idl_txn, + b_ctx_out->tracked_dp_bindings)) { return false; } @@ -1598,7 +1757,9 @@ consider_iface_release(const struct ovsrec_interface *iface_rec, local_binding_delete(b_ctx_out->local_bindings, lbinding); } - sset_find_and_delete(b_ctx_out->local_lports, iface_id); + if (remove_lport_from_local_lports(b_ctx_out->local_lports, iface_id)) { + *local_lports_changed = true; + } smap_remove(b_ctx_out->local_iface_ids, iface_rec->name); return true; @@ -1630,6 +1791,8 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, bool handled = true; *changed = false; + *b_ctx_out->local_lports_changed = false; + /* Run the tracked interfaces loop twice. One to handle deleted * changes. And another to handle add/update changes. * This will ensure correctness. @@ -1685,7 +1848,8 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, if (cleared_iface_id) { handled = consider_iface_release(iface_rec, cleared_iface_id, - b_ctx_in, b_ctx_out, changed); + b_ctx_in, b_ctx_out, changed, + b_ctx_out->local_lports_changed); } if (!handled) { @@ -1727,7 +1891,8 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, if (iface_id && ofport > 0) { *changed = true; handled = consider_iface_claim(iface_rec, iface_id, b_ctx_in, - b_ctx_out, qos_map_ptr); + b_ctx_out, qos_map_ptr, + b_ctx_out->local_lports_changed); if (!handled) { break; } @@ -1792,8 +1957,7 @@ static bool handle_deleted_vif_lport(const struct sbrec_port_binding *pb, enum en_lport_type lport_type, struct binding_ctx_in *b_ctx_in, - struct binding_ctx_out *b_ctx_out, - bool *changed) + struct binding_ctx_out *b_ctx_out) { struct local_binding *lbinding = get_lbinding_for_lport(pb, lport_type, b_ctx_out); @@ -1804,13 +1968,12 @@ handle_deleted_vif_lport(const struct sbrec_port_binding *pb, * clear the 'chassis' column of 'pb'. But we need to do * for the local_binding's children. */ if (lbinding->type == BT_VIF && - !release_local_binding_children(b_ctx_in->chassis_rec, - lbinding, - !b_ctx_in->ovnsb_idl_txn)) { + !release_local_binding_children( + b_ctx_in->chassis_rec, lbinding, + !b_ctx_in->ovnsb_idl_txn, + b_ctx_out->tracked_dp_bindings)) { return false; } - - *changed = true; } handle_deleted_lport(pb, b_ctx_in, b_ctx_out); @@ -1822,8 +1985,7 @@ handle_updated_vif_lport(const struct sbrec_port_binding *pb, enum en_lport_type lport_type, struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out, - struct hmap *qos_map, - bool *changed) + struct hmap *qos_map) { bool claimed = (pb->chassis == b_ctx_in->chassis_rec); bool handled = true; @@ -1841,14 +2003,10 @@ handle_updated_vif_lport(const struct sbrec_port_binding *pb, } bool now_claimed = (pb->chassis == b_ctx_in->chassis_rec); - bool changed_ = (claimed != now_claimed); - - if (changed_) { - *changed = true; - } + bool changed = (claimed != now_claimed); if (lport_type == LP_VIRTUAL || - (lport_type == LP_VIF && is_lport_container(pb)) || !changed_) { + (lport_type == LP_VIF && is_lport_container(pb)) || !changed) { return true; } @@ -1898,7 +2056,7 @@ binding_handle_port_binding_changes(struct binding_ctx_in *b_ctx_in, enum en_lport_type lport_type = get_lport_type(pb); if (lport_type == LP_VIF || lport_type == LP_VIRTUAL) { handled = handle_deleted_vif_lport(pb, lport_type, b_ctx_in, - b_ctx_out, changed); + b_ctx_out); } else { handle_deleted_lport(pb, b_ctx_in, b_ctx_out); } @@ -1933,15 +2091,14 @@ binding_handle_port_binding_changes(struct binding_ctx_in *b_ctx_in, case LP_VIF: case LP_VIRTUAL: handled = handle_updated_vif_lport(pb, lport_type, b_ctx_in, - b_ctx_out, qos_map_ptr, - changed); + b_ctx_out, qos_map_ptr); break; case LP_PATCH: case LP_LOCALPORT: case LP_VTEP: - *changed = true; - update_local_lport_ids(b_ctx_out->local_lport_ids, pb); + update_local_lport_ids(b_ctx_out->local_lport_ids, pb, + b_ctx_out->tracked_dp_bindings); if (lport_type == LP_PATCH) { /* Add the peer datapath to the local datapaths if it's * not present yet. @@ -1950,30 +2107,32 @@ binding_handle_port_binding_changes(struct binding_ctx_in *b_ctx_in, add_local_datapath_peer_port(pb, b_ctx_in, b_ctx_out, ld); } } + + if (lport_type == LP_VTEP) { + /* VTEP lports are claimed/released by ovn-controller-vteps. + * We are not sure what changed. So set *changed to true + * for any changes to vtep lports. */ + *changed = true; + } break; case LP_L2GATEWAY: - *changed = true; handled = consider_l2gw_lport(pb, b_ctx_in, b_ctx_out); break; case LP_L3GATEWAY: - *changed = true; handled = consider_l3gw_lport(pb, b_ctx_in, b_ctx_out); break; case LP_CHASSISREDIRECT: - *changed = true; handled = consider_cr_lport(pb, b_ctx_in, b_ctx_out); break; case LP_EXTERNAL: - *changed = true; handled = consider_external_lport(pb, b_ctx_in, b_ctx_out); break; case LP_LOCALNET: { - *changed = true; consider_localnet_lport(pb, b_ctx_in, b_ctx_out, qos_map_ptr); struct shash bridge_mappings = @@ -2008,6 +2167,10 @@ binding_handle_port_binding_changes(struct binding_ctx_in *b_ctx_in, } } + if (handled && !hmap_is_empty(b_ctx_out->tracked_dp_bindings)) { + *changed = true; + } + destroy_qos_map(&qos_map); return handled; } diff --git a/controller/binding.h b/controller/binding.h index 21118ecd4..2974ebb30 100644 --- a/controller/binding.h +++ b/controller/binding.h @@ -19,6 +19,9 @@ #include <stdbool.h> #include "openvswitch/shash.h" +#include "openvswitch/hmap.h" +#include "openvswitch/uuid.h" +#include "openvswitch/list.h" struct hmap; struct ovsdb_idl; @@ -54,10 +57,28 @@ struct binding_ctx_in { struct binding_ctx_out { struct hmap *local_datapaths; struct shash *local_bindings; + struct sset *local_lports; + + /* If the sset local_lports is modified, this is set to true by + * the callee. */ + bool *local_lports_changed; + + /* sset of local lport ids in the format + * <datapath-tunnel-key>_<port-tunnel-key>. */ struct sset *local_lport_ids; + struct sset *egress_ifaces; + + /* smap of OVS interface name as key and + * OVS interface external_ids:iface-id as value. */ struct smap *local_iface_ids; + + /* hmap of 'struct tracked_binding_datapath' which the + * callee (binding_handle_ovs_interface_changes and + * binding_handle_port_binding_changes) fills in for + * the changed datapaths and port bindings. */ + struct hmap *tracked_dp_bindings; }; enum local_binding_type { @@ -82,6 +103,21 @@ local_binding_find(struct shash *local_bindings, const char *name) return shash_find_data(local_bindings, name); } +/* Represents a tracked binding logical port. */ +struct tracked_binding_lport { + const struct sbrec_port_binding *pb; + struct ovs_list list_node; + bool deleted; +}; + +/* 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, @@ -96,4 +132,5 @@ bool binding_handle_ovs_interface_changes(struct binding_ctx_in *, bool binding_handle_port_binding_changes(struct binding_ctx_in *, struct binding_ctx_out *, bool *changed); +void binding_tracked_dp_destroy(struct hmap *tracked_datapaths); #endif /* controller/binding.h */ diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 795a1c297..98652866c 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -973,10 +973,88 @@ struct ed_type_runtime_data { struct sset local_lport_ids; struct sset active_tunnels; + /* runtime data engine private data. */ struct sset egress_ifaces; struct smap local_iface_ids; + + /* Tracked data. See below for more details and comments. */ + bool tracked; + bool local_lports_changed; + struct hmap tracked_dp_bindings; }; +/* struct ed_type_runtime_data has the below members for tracking the + * changes done to the runtime_data engine by the runtime_data engine + * handlers. Since this engine is an input to the flow_output engine, + * the flow output runtime data handler will make use of this tracked data. + * + * ------------------------------------------------------------------------ + * | | This is a hmap of | + * | | 'struct tracked_binding_datapath' defined in | + * | | binding.h. Runtime data handlers for OVS | + * | | Interface and Port Binding changes store the | + * | @tracked_dp_bindings | changed datapaths (datapaths added/removed from | + * | | local_datapaths) and changed port bindings | + * | | (added/updated/deleted in 'local_bindings'). | + * | | So any changes to the runtime data - | + * | | local_datapaths and local_bindings is captured | + * | | here. | + * ------------------------------------------------------------------------ + * | | This is a bool which represents if the runtime | + * | | data 'local_lports' changed by the runtime data | + * | | handlers for OVS Interface and Port Binding | + * | | changes. If 'local_lports' is updated and also | + * | | results in any port binding updates, it is | + * |@local_lports_changed | captured in the @tracked_dp_bindings. So there | + * | | is no need to capture the changes in the | + * | | local_lports. If @local_lports_changed is true | + * | | but without anydata in the @tracked_dp_bindings,| + * | | it means we needto only update the SB monitor | + * | | clauses and there isno need for any flow | + * | | (re)computations. | + * ------------------------------------------------------------------------ + * | | This represents if the data was tracked or not | + * | | by the runtime data handlers during the engine | + * | @tracked | run. If the runtime data recompute is | + * | | triggered, it means there is no tracked data. | + * ------------------------------------------------------------------------ + * + * + * The changes to the following runtime_data variables are not tracked. + * + * --------------------------------------------------------------------- + * | local_datapaths | The changes to these runtime data is captured in | + * | local_bindings | the @tracked_dp_bindings indirectly and hence it | + * | local_lport_ids | is not tracked explicitly. | + * --------------------------------------------------------------------- + * | local_iface_ids | This is used internally within the runtime data | + * | egress_ifaces | engine (used only in binding.c) and hence there | + * | | there is no need to track. | + * --------------------------------------------------------------------- + * | | Active tunnels is built in the | + * | | bfd_calculate_active_tunnels() for the tunnel | + * | | OVS interfaces. Any changes to non VIF OVS | + * | | interfaces results in triggering the full | + * | active_tunnels | recompute of runtime data engine and hence there | + * | | the tracked data doesn't track it. When we | + * | | support handling changes to non VIF OVS | + * | | interfaces we need to track the changes to the | + * | | active tunnels. | + * --------------------------------------------------------------------- + * + */ + +static void +en_runtime_data_clear_tracked_data(void *data_) +{ + struct ed_type_runtime_data *data = data_; + + binding_tracked_dp_destroy(&data->tracked_dp_bindings); + hmap_init(&data->tracked_dp_bindings); + data->local_lports_changed = false; + data->tracked = false; +} + static void * en_runtime_data_init(struct engine_node *node OVS_UNUSED, struct engine_arg *arg OVS_UNUSED) @@ -990,6 +1068,10 @@ en_runtime_data_init(struct engine_node *node OVS_UNUSED, sset_init(&data->egress_ifaces); smap_init(&data->local_iface_ids); local_bindings_init(&data->local_bindings); + + /* init the tracked data. */ + hmap_init(&data->tracked_dp_bindings); + return data; } @@ -1012,6 +1094,8 @@ en_runtime_data_cleanup(void *data) } hmap_destroy(&rt_data->local_datapaths); local_bindings_destroy(&rt_data->local_bindings); + + en_runtime_data_clear_tracked_data(data); } static void @@ -1092,6 +1176,8 @@ init_binding_ctx(struct engine_node *node, b_ctx_out->egress_ifaces = &rt_data->egress_ifaces; b_ctx_out->local_bindings = &rt_data->local_bindings; b_ctx_out->local_iface_ids = &rt_data->local_iface_ids; + b_ctx_out->tracked_dp_bindings = NULL; + b_ctx_out->local_lports_changed = NULL; } static void @@ -1103,6 +1189,13 @@ en_runtime_data_run(struct engine_node *node, void *data) struct sset *local_lport_ids = &rt_data->local_lport_ids; struct sset *active_tunnels = &rt_data->active_tunnels; + /* Clear the tracked data. Even though the tracked data + * gets cleared in the beginning of engine_init_run(), + * any of the runtime data handler might have set some tracked + * data and later another runtime data handler might return false + * resulting full recompute of runtime engine. */ + en_runtime_data_clear_tracked_data(data); + static bool first_run = true; if (first_run) { /* don't cleanup since there is no data yet */ @@ -1158,6 +1251,9 @@ runtime_data_ovs_interface_handler(struct engine_node *node, void *data) struct binding_ctx_in b_ctx_in; struct binding_ctx_out b_ctx_out; init_binding_ctx(node, rt_data, &b_ctx_in, &b_ctx_out); + rt_data->tracked = true; + b_ctx_out.tracked_dp_bindings = &rt_data->tracked_dp_bindings; + b_ctx_out.local_lports_changed = &rt_data->local_lports_changed; bool changed = false; if (!binding_handle_ovs_interface_changes(&b_ctx_in, &b_ctx_out, @@ -1190,6 +1286,9 @@ runtime_data_sb_port_binding_handler(struct engine_node *node, void *data) return false; } + rt_data->tracked = true; + b_ctx_out.tracked_dp_bindings = &rt_data->tracked_dp_bindings; + bool changed = false; if (!binding_handle_port_binding_changes(&b_ctx_in, &b_ctx_out, &changed)) { @@ -1797,6 +1896,39 @@ flow_output_port_groups_handler(struct engine_node *node, void *data) return _flow_output_resource_ref_handler(node, data, REF_TYPE_PORTGROUP); } +static bool +flow_output_runtime_data_handler(struct engine_node *node, + void *data OVS_UNUSED) +{ + struct ed_type_runtime_data *rt_data = + engine_get_input_data("runtime_data", node); + + /* There is no tracked data. Fall back to full recompute of + * flow_output. */ + if (!rt_data->tracked) { + return false; + } + + if (!hmap_is_empty(&rt_data->tracked_dp_bindings)) { + /* We are not yet handling the tracked datapath binding + * changes. Return false to trigger full recompute. */ + return false; + } + + if (rt_data->local_lports_changed) { + engine_set_node_state(node, EN_UPDATED); + } + + return true; +} + +static bool +flow_output_noop_handler(struct engine_node *node OVS_UNUSED, + void *data OVS_UNUSED) +{ + return true; +} + /* Engine node en_physical_flow_changes indicates whether * there is a need to * - recompute only physical flows or @@ -1908,13 +2040,6 @@ flow_output_physical_flow_changes_handler(struct engine_node *node, void *data) return true; } -static bool -flow_output_noop_handler(struct engine_node *node OVS_UNUSED, - void *data OVS_UNUSED) -{ - return true; -} - struct ovn_controller_exit_args { bool *exiting; bool *restart; @@ -2036,7 +2161,7 @@ main(int argc, char *argv[]) /* Define inc-proc-engine nodes. */ ENGINE_NODE_CUSTOM_DATA(ct_zones, "ct_zones"); - ENGINE_NODE(runtime_data, "runtime_data"); + ENGINE_NODE_WITH_CLEAR_TRACK_DATA(runtime_data, "runtime_data"); ENGINE_NODE(mff_ovn_geneve, "mff_ovn_geneve"); ENGINE_NODE(ofctrl_is_connected, "ofctrl_is_connected"); ENGINE_NODE_WITH_CLEAR_TRACK_DATA(physical_flow_changes, @@ -2073,7 +2198,8 @@ main(int argc, char *argv[]) flow_output_addr_sets_handler); engine_add_input(&en_flow_output, &en_port_groups, flow_output_port_groups_handler); - engine_add_input(&en_flow_output, &en_runtime_data, NULL); + engine_add_input(&en_flow_output, &en_runtime_data, + flow_output_runtime_data_handler); engine_add_input(&en_flow_output, &en_mff_ovn_geneve, NULL); engine_add_input(&en_flow_output, &en_physical_flow_changes, flow_output_physical_flow_changes_handler); diff --git a/tests/ovn-performance.at b/tests/ovn-performance.at index 5cc1960b6..08acd3bae 100644 --- a/tests/ovn-performance.at +++ b/tests/ovn-performance.at @@ -286,11 +286,11 @@ for i in 1 2; do [hv1 hv2], [lflow_run], [ovn-nbctl --wait=hv lsp-set-type $lsp router] ) - OVN_CONTROLLER_EXPECT_HIT( + OVN_CONTROLLER_EXPECT_NO_HIT( [hv1 hv2], [lflow_run], [ovn-nbctl --wait=hv lsp-set-options $lsp router-port=$lrp] ) - OVN_CONTROLLER_EXPECT_HIT( + OVN_CONTROLLER_EXPECT_NO_HIT( [hv1 hv2], [lflow_run], [ovn-nbctl --wait=hv lsp-set-addresses $lsp router] ) @@ -416,15 +416,15 @@ OVN_CONTROLLER_EXPECT_NO_HIT( [ovn-nbctl --wait=hv destroy Port_Group pg1] ) -for i in 1 2; do - ls=ls$i +OVN_CONTROLLER_EXPECT_HIT( + [hv1 hv2], [lflow_run], + [ovn-nbctl --wait=hv ls-del ls1] +) - # Delete switch $ls - OVN_CONTROLLER_EXPECT_HIT( - [hv1 hv2], [lflow_run], - [ovn-nbctl --wait=hv ls-del $ls] - ) -done +OVN_CONTROLLER_EXPECT_NO_HIT( + [hv1 hv2], [lflow_run], + [ovn-nbctl --wait=hv ls-del ls2] +) # Delete router lr1 OVN_CONTROLLER_EXPECT_NO_HIT(