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