On Tue, Jun 9, 2020 at 3:11 PM Dumitru Ceara <[email protected]> wrote:
> On 6/8/20 3:50 PM, [email protected] wrote: > > From: Numan Siddique <[email protected]> > > > > 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 <[email protected]> > > Signed-off-by: Venkata Anil <[email protected]> > > Signed-off-by: Numan Siddique <[email protected]> > > --- > > 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 > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
