On 6/11/20 2:44 PM, num...@ovn.org wrote:
> From: Numan Siddique <num...@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
Hi Numan,
Typo: s/outout/output.
I have 3 more minor comments below. With all addressed:
Acked-by: Dumitru Ceara <dce...@redhat.com>
Thanks,
Dumitru
> 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 <anilvenk...@redhat.com>
> Signed-off-by: Venkata Anil <anilvenk...@redhat.com>
> Signed-off-by: Numan Siddique <num...@ovn.org>
> ---
> controller/binding.c | 198 ++++++++++++++++++++++++++++++------
> controller/binding.h | 23 +++++
> controller/ovn-controller.c | 143 +++++++++++++++++++++++++-
> tests/ovn-performance.at | 20 ++--
> 4 files changed, 339 insertions(+), 45 deletions(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index 67fd2777f..77ed7441a 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -69,13 +69,25 @@ 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,
> + 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 +104,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 +141,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 +165,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 +526,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 +546,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 +699,75 @@ 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' and 'deleted'
Nit: there's no 'deleted' field in tracked_binding_lport.
> + * 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;
> +}
> +
> +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 +821,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 +840,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 */
> @@ -758,9 +861,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 +891,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;
> }
> @@ -828,13 +937,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 +959,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 +990,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 +999,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 +1020,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 +1108,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 +1189,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 +1234,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 +1270,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 +1562,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 +1643,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 || (old_claim == new_claim)) {
Unless I'm missing something, we never call update_lport_tracking() with
old_claim == new_claim. Wouldn't it be better to simplify the code and
just remove these two arguments?
> + 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 +1751,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 +1965,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 +2041,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 +2097,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 +2106,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 6e66705da..351fa32db 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. */
Nit: s/init/Init
> + 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);
> }
>
> @@ -1796,6 +1906,32 @@ 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;
> +}
> +
> /* Engine node en_physical_flow_changes indicates whether
> * there is a need to
> * - recompute only physical flows or
> @@ -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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
