Message ID | 20200619111021.2355895-1-numans@ovn.org |
---|---|
State | Accepted |
Headers | show |
Series | Incremental processing improvements. | expand |
On 6/19/20 1:10 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. > > Acked-by: Han Zhou <hzhou@ovn.org> > Acked-by: Dumitru Ceara <dceara@redhat.com> > 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 | 195 ++++++++++++++++++++++++++++++------ > controller/binding.h | 23 +++++ > controller/ovn-controller.c | 143 +++++++++++++++++++++++++- > tests/ovn-performance.at | 20 ++-- > 4 files changed, 336 insertions(+), 45 deletions(-) > > diff --git a/controller/binding.c b/controller/binding.c > index 67fd2777f..58e0bf59f 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -69,13 +69,24 @@ binding_register_ovs_idl(struct ovsdb_idl *ovs_idl) > ovsdb_idl_add_column(ovs_idl, &ovsrec_qos_col_type); > } > > +static struct tracked_binding_datapath *tracked_binding_datapath_create( > + const struct sbrec_datapath_binding *, > + bool is_new, struct hmap *tracked_dps); > +static struct tracked_binding_datapath *tracked_binding_datapath_find( > + struct hmap *, const struct sbrec_datapath_binding *); > +static void tracked_binding_datapath_lport_add( > + const struct sbrec_port_binding *, struct hmap *tracked_datapaths); > +static void update_lport_tracking(const struct sbrec_port_binding *pb, > + struct hmap *tracked_dp_bindings); > + > static void > add_local_datapath__(struct ovsdb_idl_index *sbrec_datapath_binding_by_key, > struct ovsdb_idl_index *sbrec_port_binding_by_datapath, > struct ovsdb_idl_index *sbrec_port_binding_by_name, > const struct sbrec_datapath_binding *datapath, > bool has_local_l3gateway, int depth, > - struct hmap *local_datapaths) > + struct hmap *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 +103,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 +140,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 +164,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 > @@ -506,6 +525,11 @@ update_local_lport_ids(struct binding_ctx_out *b_ctx, > pb->datapath->tunnel_key, pb->tunnel_key); > if (sset_add(b_ctx->local_lport_ids, buf) != NULL) { > b_ctx->local_lport_ids_changed = true; > + > + if (b_ctx->tracked_dp_bindings) { > + /* Add the 'pb' to the tracked_datapaths. */ > + tracked_binding_datapath_lport_add(pb, b_ctx->tracked_dp_bindings); > + } > } > } > > @@ -521,6 +545,11 @@ remove_local_lport_ids(struct binding_ctx_out *b_ctx, > pb->datapath->tunnel_key, pb->tunnel_key); > if (sset_find_and_delete(b_ctx->local_lport_ids, buf)) { > b_ctx->local_lport_ids_changed = true; > + > + if (b_ctx->tracked_dp_bindings) { > + /* Add the 'pb' to the tracked_datapaths. */ > + tracked_binding_datapath_lport_add(pb, b_ctx->tracked_dp_bindings); > + } > } > } > > @@ -669,6 +698,74 @@ 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, > + struct hmap *tracked_datapaths) > +{ > + if (!tracked_datapaths) { > + return; > + } > + > + struct tracked_binding_datapath *tracked_dp = > + tracked_binding_datapath_find(tracked_datapaths, pb->datapath); > + if (!tracked_dp) { > + tracked_dp = tracked_binding_datapath_create(pb->datapath, false, > + tracked_datapaths); > + } > + > + /* Check if the lport is already present or not. > + * If it is already present, then just update the 'pb' field. */ > + struct tracked_binding_lport *lport = > + shash_find_data(&tracked_dp->lports, pb->logical_port); > + > + if (!lport) { > + lport = xmalloc(sizeof *lport); > + shash_add(&tracked_dp->lports, pb->logical_port, lport); > + } > + > + lport->pb = pb; > +} > + > +void > +binding_tracked_dp_destroy(struct hmap *tracked_datapaths) > +{ > + struct tracked_binding_datapath *t_dp; > + HMAP_FOR_EACH_POP (t_dp, node, tracked_datapaths) { > + shash_destroy_free_data(&t_dp->lports); > + free(t_dp); > + } > + > + hmap_destroy(tracked_datapaths); > +} > + > /* Corresponds to each Port_Binding.type. */ > enum en_lport_type { > LP_UNKNOWN, > @@ -722,7 +819,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) { > @@ -741,6 +838,10 @@ claim_lport(const struct sbrec_port_binding *pb, > } > > sbrec_port_binding_set_chassis(pb, chassis_rec); > + > + if (tracked_datapaths) { > + update_lport_tracking(pb, tracked_datapaths); > + } > } > > /* Check if the port encap binding, if any, has changed */ > @@ -758,9 +859,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 the the 'pb' was claimed > + * earlier i.e port binding's chassis is set to this chassis. > + * Caller should make sure that this is 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) { > @@ -783,6 +889,7 @@ release_lport(const struct sbrec_port_binding *pb, bool sb_readonly) > sbrec_port_binding_set_virtual_parent(pb, NULL); > } > > + update_lport_tracking(pb, tracked_datapaths); > VLOG_INFO("Releasing lport %s from this chassis.", pb->logical_port); > return true; > } > @@ -828,13 +935,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; > } > } > @@ -849,16 +957,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; > @@ -879,7 +988,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; > } > > @@ -887,7 +997,8 @@ 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); > + b_ctx_out->local_datapaths, > + b_ctx_out->tracked_dp_bindings); > update_local_lport_ids(b_ctx_out, pb); > if (lbinding->iface && qos_map && b_ctx_in->ovs_idl_txn) { > get_qos_params(pb, qos_map); > @@ -907,7 +1018,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); > } > } > > @@ -994,7 +1106,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; > @@ -1074,13 +1187,16 @@ consider_nonvif_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, 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, pb); > 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; > @@ -1116,7 +1232,7 @@ 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. */ > update_local_lports(b_ctx_out, pb->logical_port); > > @@ -1152,7 +1268,9 @@ 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, pb); > } > > return consider_nonvif_lport_(pb, our_chassis, false, b_ctx_in, b_ctx_out); > @@ -1442,7 +1560,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; > } > > @@ -1522,6 +1641,17 @@ remove_pb_from_local_datapath(const struct sbrec_port_binding *pb, > } > } > > +static void > +update_lport_tracking(const struct sbrec_port_binding *pb, > + struct hmap *tracked_dp_bindings) > +{ > + if (!tracked_dp_bindings) { > + return; > + } > + > + tracked_binding_datapath_lport_add(pb, 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'. > @@ -1618,7 +1748,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; > } > } > @@ -1831,9 +1962,10 @@ 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; > } > } > @@ -1906,7 +2038,6 @@ binding_handle_port_binding_changes(struct binding_ctx_in *b_ctx_in, > struct binding_ctx_out *b_ctx_out) > { > bool handled = true; > - > const struct sbrec_port_binding *pb; > > /* Run the tracked port binding loop twice. One to handle deleted > @@ -1963,7 +2094,6 @@ binding_handle_port_binding_changes(struct binding_ctx_in *b_ctx_in, > case LP_PATCH: > case LP_LOCALPORT: > case LP_VTEP: > - b_ctx_out->non_vif_ports_changed = true; > update_local_lport_ids(b_ctx_out, pb); > if (lport_type == LP_PATCH) { > /* Add the peer datapath to the local datapaths if it's > @@ -1973,30 +2103,31 @@ 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. */ > + b_ctx_out->non_vif_ports_changed = true; > + } > break; > > case LP_L2GATEWAY: > - b_ctx_out->non_vif_ports_changed = true; > handled = consider_l2gw_lport(pb, b_ctx_in, b_ctx_out); > break; > > case LP_L3GATEWAY: > - b_ctx_out->non_vif_ports_changed = true; > handled = consider_l3gw_lport(pb, b_ctx_in, b_ctx_out); > break; > > case LP_CHASSISREDIRECT: > - b_ctx_out->non_vif_ports_changed = true; > handled = consider_cr_lport(pb, b_ctx_in, b_ctx_out); > break; > > case LP_EXTERNAL: > - b_ctx_out->non_vif_ports_changed = true; > handled = consider_external_lport(pb, b_ctx_in, b_ctx_out); > break; > > case LP_LOCALNET: { > - b_ctx_out->non_vif_ports_changed = true; > consider_localnet_lport(pb, b_ctx_in, b_ctx_out, qos_map_ptr); > > struct shash bridge_mappings = > diff --git a/controller/binding.h b/controller/binding.h > index 161766d2f..c9740560f 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; > @@ -75,6 +78,12 @@ struct binding_ctx_out { > /* 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 { > @@ -99,6 +108,19 @@ 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; > +}; > + > +/* 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, > @@ -111,4 +133,5 @@ bool binding_handle_ovs_interface_changes(struct binding_ctx_in *, > struct binding_ctx_out *); > bool binding_handle_port_binding_changes(struct binding_ctx_in *, > struct binding_ctx_out *); > +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 dde531c6e..29712d2f6 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -980,10 +980,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) > @@ -997,6 +1075,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; > } > > @@ -1019,6 +1101,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); Sorry for not spotting this earlier but this is not needed anymore because we call the clear_tracked_data handler in the engine_cleanup routine. I guess this can be removed at commit time though. > } > > static void > @@ -1102,6 +1186,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 > @@ -1113,6 +1199,23 @@ 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 (stale) tracked data if any. 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 in full recompute of runtime engine and rendering the tracked > + * data stale. > + * > + * It's possible that engine framework can be enhanced to indicate > + * the node handlers (in this case flow_output_runtime_data_handler) > + * that its input node had a full recompute. However we would still > + * need to clear the tracked data, because we don't want the > + * stale tracked data to be accessed outside of the engine, since the > + * tracked data is cleared in the engine_init_run() and not at the > + * end of the engine run. > + * */ > + en_runtime_data_clear_tracked_data(data); > + > static bool first_run = true; > if (first_run) { > /* don't cleanup since there is no data yet */ > @@ -1168,6 +1271,8 @@ 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; > > if (!binding_handle_ovs_interface_changes(&b_ctx_in, &b_ctx_out)) { > return false; > @@ -1175,6 +1280,7 @@ runtime_data_ovs_interface_handler(struct engine_node *node, void *data) > > if (b_ctx_out.local_lports_changed) { > engine_set_node_state(node, EN_UPDATED); > + rt_data->local_lports_changed = b_ctx_out.local_lports_changed; > } > > return true; > @@ -1191,12 +1297,16 @@ 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; > + > if (!binding_handle_port_binding_changes(&b_ctx_in, &b_ctx_out)) { > return false; > } > > if (b_ctx_out.local_lport_ids_changed || > - b_ctx_out.non_vif_ports_changed) { > + b_ctx_out.non_vif_ports_changed || > + !hmap_is_empty(b_ctx_out.tracked_dp_bindings)) { > engine_set_node_state(node, EN_UPDATED); > } > > @@ -1908,6 +2018,32 @@ flow_output_physical_flow_changes_handler(struct engine_node *node, void *data) > return true; > } > > +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; > +} > + > struct ovn_controller_exit_args { > bool *exiting; > bool *restart; > @@ -2029,7 +2165,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, > @@ -2066,7 +2202,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 Fri, Jun 19, 2020 at 5:22 PM Dumitru Ceara <dceara@redhat.com> wrote: > On 6/19/20 1:10 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. > > > > Acked-by: Han Zhou <hzhou@ovn.org> > > Acked-by: Dumitru Ceara <dceara@redhat.com> > > 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 | 195 ++++++++++++++++++++++++++++++------ > > controller/binding.h | 23 +++++ > > controller/ovn-controller.c | 143 +++++++++++++++++++++++++- > > tests/ovn-performance.at | 20 ++-- > > 4 files changed, 336 insertions(+), 45 deletions(-) > > > > diff --git a/controller/binding.c b/controller/binding.c > > index 67fd2777f..58e0bf59f 100644 > > --- a/controller/binding.c > > +++ b/controller/binding.c > > @@ -69,13 +69,24 @@ binding_register_ovs_idl(struct ovsdb_idl *ovs_idl) > > ovsdb_idl_add_column(ovs_idl, &ovsrec_qos_col_type); > > } > > > > +static struct tracked_binding_datapath *tracked_binding_datapath_create( > > + const struct sbrec_datapath_binding *, > > + bool is_new, struct hmap *tracked_dps); > > +static struct tracked_binding_datapath *tracked_binding_datapath_find( > > + struct hmap *, const struct sbrec_datapath_binding *); > > +static void tracked_binding_datapath_lport_add( > > + const struct sbrec_port_binding *, struct hmap *tracked_datapaths); > > +static void update_lport_tracking(const struct sbrec_port_binding *pb, > > + struct hmap *tracked_dp_bindings); > > + > > static void > > add_local_datapath__(struct ovsdb_idl_index > *sbrec_datapath_binding_by_key, > > struct ovsdb_idl_index > *sbrec_port_binding_by_datapath, > > struct ovsdb_idl_index *sbrec_port_binding_by_name, > > const struct sbrec_datapath_binding *datapath, > > bool has_local_l3gateway, int depth, > > - struct hmap *local_datapaths) > > + struct hmap *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 +103,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 +140,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 +164,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 > > @@ -506,6 +525,11 @@ update_local_lport_ids(struct binding_ctx_out > *b_ctx, > > pb->datapath->tunnel_key, pb->tunnel_key); > > if (sset_add(b_ctx->local_lport_ids, buf) != NULL) { > > b_ctx->local_lport_ids_changed = true; > > + > > + if (b_ctx->tracked_dp_bindings) { > > + /* Add the 'pb' to the tracked_datapaths. */ > > + tracked_binding_datapath_lport_add(pb, > b_ctx->tracked_dp_bindings); > > + } > > } > > } > > > > @@ -521,6 +545,11 @@ remove_local_lport_ids(struct binding_ctx_out > *b_ctx, > > pb->datapath->tunnel_key, pb->tunnel_key); > > if (sset_find_and_delete(b_ctx->local_lport_ids, buf)) { > > b_ctx->local_lport_ids_changed = true; > > + > > + if (b_ctx->tracked_dp_bindings) { > > + /* Add the 'pb' to the tracked_datapaths. */ > > + tracked_binding_datapath_lport_add(pb, > b_ctx->tracked_dp_bindings); > > + } > > } > > } > > > > @@ -669,6 +698,74 @@ 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, > > + struct hmap *tracked_datapaths) > > +{ > > + if (!tracked_datapaths) { > > + return; > > + } > > + > > + struct tracked_binding_datapath *tracked_dp = > > + tracked_binding_datapath_find(tracked_datapaths, pb->datapath); > > + if (!tracked_dp) { > > + tracked_dp = tracked_binding_datapath_create(pb->datapath, > false, > > + tracked_datapaths); > > + } > > + > > + /* Check if the lport is already present or not. > > + * If it is already present, then just update the 'pb' field. */ > > + struct tracked_binding_lport *lport = > > + shash_find_data(&tracked_dp->lports, pb->logical_port); > > + > > + if (!lport) { > > + lport = xmalloc(sizeof *lport); > > + shash_add(&tracked_dp->lports, pb->logical_port, lport); > > + } > > + > > + lport->pb = pb; > > +} > > + > > +void > > +binding_tracked_dp_destroy(struct hmap *tracked_datapaths) > > +{ > > + struct tracked_binding_datapath *t_dp; > > + HMAP_FOR_EACH_POP (t_dp, node, tracked_datapaths) { > > + shash_destroy_free_data(&t_dp->lports); > > + free(t_dp); > > + } > > + > > + hmap_destroy(tracked_datapaths); > > +} > > + > > /* Corresponds to each Port_Binding.type. */ > > enum en_lport_type { > > LP_UNKNOWN, > > @@ -722,7 +819,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) { > > @@ -741,6 +838,10 @@ claim_lport(const struct sbrec_port_binding *pb, > > } > > > > sbrec_port_binding_set_chassis(pb, chassis_rec); > > + > > + if (tracked_datapaths) { > > + update_lport_tracking(pb, tracked_datapaths); > > + } > > } > > > > /* Check if the port encap binding, if any, has changed */ > > @@ -758,9 +859,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 the the 'pb' was claimed > > + * earlier i.e port binding's chassis is set to this chassis. > > + * Caller should make sure that this is 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) { > > @@ -783,6 +889,7 @@ release_lport(const struct sbrec_port_binding *pb, > bool sb_readonly) > > sbrec_port_binding_set_virtual_parent(pb, NULL); > > } > > > > + update_lport_tracking(pb, tracked_datapaths); > > VLOG_INFO("Releasing lport %s from this chassis.", > pb->logical_port); > > return true; > > } > > @@ -828,13 +935,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; > > } > > } > > @@ -849,16 +957,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; > > @@ -879,7 +988,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; > > } > > > > @@ -887,7 +997,8 @@ 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); > > + b_ctx_out->local_datapaths, > > + b_ctx_out->tracked_dp_bindings); > > update_local_lport_ids(b_ctx_out, pb); > > if (lbinding->iface && qos_map && b_ctx_in->ovs_idl_txn) { > > get_qos_params(pb, qos_map); > > @@ -907,7 +1018,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); > > } > > } > > > > @@ -994,7 +1106,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; > > @@ -1074,13 +1187,16 @@ consider_nonvif_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, 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, pb); > > 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; > > @@ -1116,7 +1232,7 @@ 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. */ > > update_local_lports(b_ctx_out, pb->logical_port); > > > > @@ -1152,7 +1268,9 @@ 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, pb); > > } > > > > return consider_nonvif_lport_(pb, our_chassis, false, b_ctx_in, > b_ctx_out); > > @@ -1442,7 +1560,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; > > } > > > > @@ -1522,6 +1641,17 @@ remove_pb_from_local_datapath(const struct > sbrec_port_binding *pb, > > } > > } > > > > +static void > > +update_lport_tracking(const struct sbrec_port_binding *pb, > > + struct hmap *tracked_dp_bindings) > > +{ > > + if (!tracked_dp_bindings) { > > + return; > > + } > > + > > + tracked_binding_datapath_lport_add(pb, 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'. > > @@ -1618,7 +1748,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; > > } > > } > > @@ -1831,9 +1962,10 @@ 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; > > } > > } > > @@ -1906,7 +2038,6 @@ binding_handle_port_binding_changes(struct > binding_ctx_in *b_ctx_in, > > struct binding_ctx_out *b_ctx_out) > > { > > bool handled = true; > > - > > const struct sbrec_port_binding *pb; > > > > /* Run the tracked port binding loop twice. One to handle deleted > > @@ -1963,7 +2094,6 @@ binding_handle_port_binding_changes(struct > binding_ctx_in *b_ctx_in, > > case LP_PATCH: > > case LP_LOCALPORT: > > case LP_VTEP: > > - b_ctx_out->non_vif_ports_changed = true; > > update_local_lport_ids(b_ctx_out, pb); > > if (lport_type == LP_PATCH) { > > /* Add the peer datapath to the local datapaths if it's > > @@ -1973,30 +2103,31 @@ 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. */ > > + b_ctx_out->non_vif_ports_changed = true; > > + } > > break; > > > > case LP_L2GATEWAY: > > - b_ctx_out->non_vif_ports_changed = true; > > handled = consider_l2gw_lport(pb, b_ctx_in, b_ctx_out); > > break; > > > > case LP_L3GATEWAY: > > - b_ctx_out->non_vif_ports_changed = true; > > handled = consider_l3gw_lport(pb, b_ctx_in, b_ctx_out); > > break; > > > > case LP_CHASSISREDIRECT: > > - b_ctx_out->non_vif_ports_changed = true; > > handled = consider_cr_lport(pb, b_ctx_in, b_ctx_out); > > break; > > > > case LP_EXTERNAL: > > - b_ctx_out->non_vif_ports_changed = true; > > handled = consider_external_lport(pb, b_ctx_in, b_ctx_out); > > break; > > > > case LP_LOCALNET: { > > - b_ctx_out->non_vif_ports_changed = true; > > consider_localnet_lport(pb, b_ctx_in, b_ctx_out, > qos_map_ptr); > > > > struct shash bridge_mappings = > > diff --git a/controller/binding.h b/controller/binding.h > > index 161766d2f..c9740560f 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; > > @@ -75,6 +78,12 @@ struct binding_ctx_out { > > /* 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 { > > @@ -99,6 +108,19 @@ 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; > > +}; > > + > > +/* 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, > > @@ -111,4 +133,5 @@ bool binding_handle_ovs_interface_changes(struct > binding_ctx_in *, > > struct binding_ctx_out *); > > bool binding_handle_port_binding_changes(struct binding_ctx_in *, > > struct binding_ctx_out *); > > +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 dde531c6e..29712d2f6 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -980,10 +980,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) > > @@ -997,6 +1075,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; > > } > > > > @@ -1019,6 +1101,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); > > Sorry for not spotting this earlier but this is not needed anymore > because we call the clear_tracked_data handler in the engine_cleanup > routine. > > I guess this can be removed at commit time though. > Ack. I'll remove it. Thanks Numan > > > } > > > > static void > > @@ -1102,6 +1186,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 > > @@ -1113,6 +1199,23 @@ 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 (stale) tracked data if any. 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 in full recompute of runtime engine and rendering the > tracked > > + * data stale. > > + * > > + * It's possible that engine framework can be enhanced to indicate > > + * the node handlers (in this case flow_output_runtime_data_handler) > > + * that its input node had a full recompute. However we would still > > + * need to clear the tracked data, because we don't want the > > + * stale tracked data to be accessed outside of the engine, since > the > > + * tracked data is cleared in the engine_init_run() and not at the > > + * end of the engine run. > > + * */ > > + en_runtime_data_clear_tracked_data(data); > > + > > static bool first_run = true; > > if (first_run) { > > /* don't cleanup since there is no data yet */ > > @@ -1168,6 +1271,8 @@ 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; > > > > if (!binding_handle_ovs_interface_changes(&b_ctx_in, &b_ctx_out)) { > > return false; > > @@ -1175,6 +1280,7 @@ runtime_data_ovs_interface_handler(struct > engine_node *node, void *data) > > > > if (b_ctx_out.local_lports_changed) { > > engine_set_node_state(node, EN_UPDATED); > > + rt_data->local_lports_changed = b_ctx_out.local_lports_changed; > > } > > > > return true; > > @@ -1191,12 +1297,16 @@ 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; > > + > > if (!binding_handle_port_binding_changes(&b_ctx_in, &b_ctx_out)) { > > return false; > > } > > > > if (b_ctx_out.local_lport_ids_changed || > > - b_ctx_out.non_vif_ports_changed) { > > + b_ctx_out.non_vif_ports_changed || > > + !hmap_is_empty(b_ctx_out.tracked_dp_bindings)) { > > engine_set_node_state(node, EN_UPDATED); > > } > > > > @@ -1908,6 +2018,32 @@ flow_output_physical_flow_changes_handler(struct > engine_node *node, void *data) > > return true; > > } > > > > +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; > > +} > > + > > struct ovn_controller_exit_args { > > bool *exiting; > > bool *restart; > > @@ -2029,7 +2165,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, > > @@ -2066,7 +2202,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 > >
diff --git a/controller/binding.c b/controller/binding.c index 67fd2777f..58e0bf59f 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -69,13 +69,24 @@ binding_register_ovs_idl(struct ovsdb_idl *ovs_idl) ovsdb_idl_add_column(ovs_idl, &ovsrec_qos_col_type); } +static struct tracked_binding_datapath *tracked_binding_datapath_create( + const struct sbrec_datapath_binding *, + bool is_new, struct hmap *tracked_dps); +static struct tracked_binding_datapath *tracked_binding_datapath_find( + struct hmap *, const struct sbrec_datapath_binding *); +static void tracked_binding_datapath_lport_add( + const struct sbrec_port_binding *, struct hmap *tracked_datapaths); +static void update_lport_tracking(const struct sbrec_port_binding *pb, + struct hmap *tracked_dp_bindings); + static void add_local_datapath__(struct ovsdb_idl_index *sbrec_datapath_binding_by_key, struct ovsdb_idl_index *sbrec_port_binding_by_datapath, struct ovsdb_idl_index *sbrec_port_binding_by_name, const struct sbrec_datapath_binding *datapath, bool has_local_l3gateway, int depth, - struct hmap *local_datapaths) + struct hmap *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 +103,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 +140,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 +164,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 @@ -506,6 +525,11 @@ update_local_lport_ids(struct binding_ctx_out *b_ctx, pb->datapath->tunnel_key, pb->tunnel_key); if (sset_add(b_ctx->local_lport_ids, buf) != NULL) { b_ctx->local_lport_ids_changed = true; + + if (b_ctx->tracked_dp_bindings) { + /* Add the 'pb' to the tracked_datapaths. */ + tracked_binding_datapath_lport_add(pb, b_ctx->tracked_dp_bindings); + } } } @@ -521,6 +545,11 @@ remove_local_lport_ids(struct binding_ctx_out *b_ctx, pb->datapath->tunnel_key, pb->tunnel_key); if (sset_find_and_delete(b_ctx->local_lport_ids, buf)) { b_ctx->local_lport_ids_changed = true; + + if (b_ctx->tracked_dp_bindings) { + /* Add the 'pb' to the tracked_datapaths. */ + tracked_binding_datapath_lport_add(pb, b_ctx->tracked_dp_bindings); + } } } @@ -669,6 +698,74 @@ 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, + struct hmap *tracked_datapaths) +{ + if (!tracked_datapaths) { + return; + } + + struct tracked_binding_datapath *tracked_dp = + tracked_binding_datapath_find(tracked_datapaths, pb->datapath); + if (!tracked_dp) { + tracked_dp = tracked_binding_datapath_create(pb->datapath, false, + tracked_datapaths); + } + + /* Check if the lport is already present or not. + * If it is already present, then just update the 'pb' field. */ + struct tracked_binding_lport *lport = + shash_find_data(&tracked_dp->lports, pb->logical_port); + + if (!lport) { + lport = xmalloc(sizeof *lport); + shash_add(&tracked_dp->lports, pb->logical_port, lport); + } + + lport->pb = pb; +} + +void +binding_tracked_dp_destroy(struct hmap *tracked_datapaths) +{ + struct tracked_binding_datapath *t_dp; + HMAP_FOR_EACH_POP (t_dp, node, tracked_datapaths) { + shash_destroy_free_data(&t_dp->lports); + free(t_dp); + } + + hmap_destroy(tracked_datapaths); +} + /* Corresponds to each Port_Binding.type. */ enum en_lport_type { LP_UNKNOWN, @@ -722,7 +819,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) { @@ -741,6 +838,10 @@ claim_lport(const struct sbrec_port_binding *pb, } sbrec_port_binding_set_chassis(pb, chassis_rec); + + if (tracked_datapaths) { + update_lport_tracking(pb, tracked_datapaths); + } } /* Check if the port encap binding, if any, has changed */ @@ -758,9 +859,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 the the 'pb' was claimed + * earlier i.e port binding's chassis is set to this chassis. + * Caller should make sure that this is 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) { @@ -783,6 +889,7 @@ release_lport(const struct sbrec_port_binding *pb, bool sb_readonly) sbrec_port_binding_set_virtual_parent(pb, NULL); } + update_lport_tracking(pb, tracked_datapaths); VLOG_INFO("Releasing lport %s from this chassis.", pb->logical_port); return true; } @@ -828,13 +935,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; } } @@ -849,16 +957,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; @@ -879,7 +988,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; } @@ -887,7 +997,8 @@ 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); + b_ctx_out->local_datapaths, + b_ctx_out->tracked_dp_bindings); update_local_lport_ids(b_ctx_out, pb); if (lbinding->iface && qos_map && b_ctx_in->ovs_idl_txn) { get_qos_params(pb, qos_map); @@ -907,7 +1018,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); } } @@ -994,7 +1106,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; @@ -1074,13 +1187,16 @@ consider_nonvif_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, 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, pb); 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; @@ -1116,7 +1232,7 @@ 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. */ update_local_lports(b_ctx_out, pb->logical_port); @@ -1152,7 +1268,9 @@ 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, pb); } return consider_nonvif_lport_(pb, our_chassis, false, b_ctx_in, b_ctx_out); @@ -1442,7 +1560,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; } @@ -1522,6 +1641,17 @@ remove_pb_from_local_datapath(const struct sbrec_port_binding *pb, } } +static void +update_lport_tracking(const struct sbrec_port_binding *pb, + struct hmap *tracked_dp_bindings) +{ + if (!tracked_dp_bindings) { + return; + } + + tracked_binding_datapath_lport_add(pb, 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'. @@ -1618,7 +1748,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; } } @@ -1831,9 +1962,10 @@ 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; } } @@ -1906,7 +2038,6 @@ binding_handle_port_binding_changes(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) { bool handled = true; - const struct sbrec_port_binding *pb; /* Run the tracked port binding loop twice. One to handle deleted @@ -1963,7 +2094,6 @@ binding_handle_port_binding_changes(struct binding_ctx_in *b_ctx_in, case LP_PATCH: case LP_LOCALPORT: case LP_VTEP: - b_ctx_out->non_vif_ports_changed = true; update_local_lport_ids(b_ctx_out, pb); if (lport_type == LP_PATCH) { /* Add the peer datapath to the local datapaths if it's @@ -1973,30 +2103,31 @@ 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. */ + b_ctx_out->non_vif_ports_changed = true; + } break; case LP_L2GATEWAY: - b_ctx_out->non_vif_ports_changed = true; handled = consider_l2gw_lport(pb, b_ctx_in, b_ctx_out); break; case LP_L3GATEWAY: - b_ctx_out->non_vif_ports_changed = true; handled = consider_l3gw_lport(pb, b_ctx_in, b_ctx_out); break; case LP_CHASSISREDIRECT: - b_ctx_out->non_vif_ports_changed = true; handled = consider_cr_lport(pb, b_ctx_in, b_ctx_out); break; case LP_EXTERNAL: - b_ctx_out->non_vif_ports_changed = true; handled = consider_external_lport(pb, b_ctx_in, b_ctx_out); break; case LP_LOCALNET: { - b_ctx_out->non_vif_ports_changed = true; consider_localnet_lport(pb, b_ctx_in, b_ctx_out, qos_map_ptr); struct shash bridge_mappings = diff --git a/controller/binding.h b/controller/binding.h index 161766d2f..c9740560f 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; @@ -75,6 +78,12 @@ struct binding_ctx_out { /* 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 { @@ -99,6 +108,19 @@ 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; +}; + +/* 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, @@ -111,4 +133,5 @@ bool binding_handle_ovs_interface_changes(struct binding_ctx_in *, struct binding_ctx_out *); bool binding_handle_port_binding_changes(struct binding_ctx_in *, struct binding_ctx_out *); +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 dde531c6e..29712d2f6 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -980,10 +980,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) @@ -997,6 +1075,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; } @@ -1019,6 +1101,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 @@ -1102,6 +1186,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 @@ -1113,6 +1199,23 @@ 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 (stale) tracked data if any. 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 in full recompute of runtime engine and rendering the tracked + * data stale. + * + * It's possible that engine framework can be enhanced to indicate + * the node handlers (in this case flow_output_runtime_data_handler) + * that its input node had a full recompute. However we would still + * need to clear the tracked data, because we don't want the + * stale tracked data to be accessed outside of the engine, since the + * tracked data is cleared in the engine_init_run() and not at the + * end of the engine run. + * */ + en_runtime_data_clear_tracked_data(data); + static bool first_run = true; if (first_run) { /* don't cleanup since there is no data yet */ @@ -1168,6 +1271,8 @@ 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; if (!binding_handle_ovs_interface_changes(&b_ctx_in, &b_ctx_out)) { return false; @@ -1175,6 +1280,7 @@ runtime_data_ovs_interface_handler(struct engine_node *node, void *data) if (b_ctx_out.local_lports_changed) { engine_set_node_state(node, EN_UPDATED); + rt_data->local_lports_changed = b_ctx_out.local_lports_changed; } return true; @@ -1191,12 +1297,16 @@ 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; + if (!binding_handle_port_binding_changes(&b_ctx_in, &b_ctx_out)) { return false; } if (b_ctx_out.local_lport_ids_changed || - b_ctx_out.non_vif_ports_changed) { + b_ctx_out.non_vif_ports_changed || + !hmap_is_empty(b_ctx_out.tracked_dp_bindings)) { engine_set_node_state(node, EN_UPDATED); } @@ -1908,6 +2018,32 @@ flow_output_physical_flow_changes_handler(struct engine_node *node, void *data) return true; } +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; +} + struct ovn_controller_exit_args { bool *exiting; bool *restart; @@ -2029,7 +2165,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, @@ -2066,7 +2202,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(