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