On Fri, Jun 25, 2021 at 2:53 PM Han Zhou <[email protected]> wrote: > > On Fri, Jun 25, 2021 at 4:50 AM Dumitru Ceara <[email protected]> wrote: > > > > It's valid that port_groups contain non-vif ports, they can actually > > contain any type of logical_switch_port. > > > > Also, there's no need to allocate a new sset containing the local ports' > > names every time the I-P engine processes a change, we can maintain a > > sset and incrementally update it when port bindings are added/removed. > > > Thanks Dumitru for the fix and thanks Numan for the review. > > I battled with myself when deciding to allocate a new sset for the local > ports' names at the I-P main iteration level. I did this because the > current data structures maintaining the local bindings were already very > complex, and the sset was not to maintain any extra information but just > redundant information (for efficiency). So I decided to abstract this part > as a helper function so that I don't add more complexity in the binding > data structure, and other I-P engine nodes doesn't need to understand the > internals of how the bindings are maintained in the bindings module. > Regarding the cost, the local binding data should be small, and the sset > allocation is at the main loop level, so really nothing to worry about the > cost. > > However, I didn't think about the non-VIF use case of port-group, and the > local_binding doesn't maintain non-VIF bindings, so came the bug. This > patch fixes it by maintaining a sset that includes all types of lport > names. It looks correct to me, but I have some comments: > > 1) The structures in bindings module is really complex and I spent a lot of > time to understand it before, but when I am reviewing this patch I had to > spent some time again to understand it. There are fields in binding_ctx > look quite similar, and the comments don't tell exactly the difference: > > - struct local_binding_data *lbinding_data; > - struct sset *local_lports; > - struct sset *local_lport_ids; >
I agree with the complexity and the naming confusion. I think local_lports and local_lport_ids have been maintained in the binding code since a long time and lbinding_data was added recently. I think there is a lot of redundant data which can be unified. > According to the code (and also the bug the patch is trying to fix), > lbinding_data is supposed to maintain VIFs only. I agree. "lbinding_data" is supposed to maintain local vif binding information. > local_lports maintains all types, but it maintains *potentially* local VIFs > as well (meaning the VIF may not be bound locally yet). I was thinking if I > could use local_lports directly. I think it would work, but just not > accurate enough (maybe it doesn't really matter). > The local_lport_ids may look straightforward, which maintains generated id > keys for local_lports, but the functions update_local_lport_ids() and > remove_local_lport_ids() are not only updating the local_lport_ids, but > also tracking information of lbinding_data, which is really confusing. > > 2) Now for this patch, the intention is to include non-VIF bindings, but it > adds a sset to maintain all types of lports in "lbinding_data", which was > supposed to maintain VIF bindings only. I think it is not the right place > to maintain this sset. And the > update_local_lport_ids()/remove_local_lport_ids() are not the right place > to update them either. > > So here are my suggestions: > > 1) Clarify a little more about the role of each of the above fields in > binding_ctx with some comments. These comments would be super helpful. But I think it is outside the scope of this bug fix patch. It's better if it's a separate patch. > 2) Can we use local_lports directly, instead of maintaining a new sset? If > we can't (I am not sure yet), can we generate it on-the-fly, just updating > the "binding_collect_local_binding_lports" by adding non-VIF lports from > "local_lports"? I really don't think the cost makes any difference overall. > If none of the above work, can we maintain it as a separate field at > binding_ctx level instead of in lbinding_data (with proper comment telling > the difference from local_lports)? I think local_lports can be used. The side effect would be that we will be allocating the zone id even for patch ports. Thanks Numan > > (+1 for Numan's comment regarding test case) > > I hope this makes sense. Thanks again for the fix! > Han > > > Reported-at: https://github.com/ovn-org/ovn/pull/61#issuecomment-865094163 > > Reported-by: Antonio Ojea <[email protected]> > > Fixes: 0cfeba6b55e3 ("ovn-controller: Fix port group conjunction flow > explosion problem.") > > Signed-off-by: Dumitru Ceara <[email protected]> > > --- > > controller/binding.c | 25 +++++-------------------- > > controller/binding.h | 11 ++--------- > > controller/ovn-controller.c | 24 +++++++----------------- > > 3 files changed, 14 insertions(+), 46 deletions(-) > > > > diff --git a/controller/binding.c b/controller/binding.c > > index 7fde0fdbb..1f6188e0d 100644 > > --- a/controller/binding.c > > +++ b/controller/binding.c > > @@ -549,6 +549,7 @@ update_local_lport_ids(const struct > sbrec_port_binding *pb, > > tracked_binding_datapath_lport_add(pb, > b_ctx->tracked_dp_bindings); > > } > > } > > + sset_add(&b_ctx->lbinding_data->port_bindings, pb->logical_port); > > } > > > > /* Remove a port binding id from the set of local lport IDs. Also track > if > > @@ -569,6 +570,8 @@ remove_local_lport_ids(const struct > sbrec_port_binding *pb, > > tracked_binding_datapath_lport_add(pb, > b_ctx->tracked_dp_bindings); > > } > > } > > + sset_find_and_delete(&b_ctx->lbinding_data->port_bindings, > > + pb->logical_port); > > } > > > > /* Corresponds to each Port_Binding.type. */ > > @@ -683,6 +686,7 @@ local_binding_data_init(struct local_binding_data > *lbinding_data) > > { > > shash_init(&lbinding_data->bindings); > > shash_init(&lbinding_data->lports); > > + sset_init(&lbinding_data->port_bindings); > > } > > > > void > > @@ -702,6 +706,7 @@ local_binding_data_destroy(struct local_binding_data > *lbinding_data) > > shash_delete(&lbinding_data->bindings, node); > > } > > > > + sset_destroy(&lbinding_data->port_bindings); > > shash_destroy(&lbinding_data->lports); > > shash_destroy(&lbinding_data->bindings); > > } > > @@ -2926,23 +2931,3 @@ cleanup: > > > > return b_lport; > > } > > - > > -struct sset * > > -binding_collect_local_binding_lports(struct local_binding_data > *lbinding_data) > > -{ > > - struct sset *lports = xzalloc(sizeof *lports); > > - sset_init(lports); > > - struct shash_node *shash_node; > > - SHASH_FOR_EACH (shash_node, &lbinding_data->lports) { > > - struct binding_lport *b_lport = shash_node->data; > > - sset_add(lports, b_lport->name); > > - } > > - return lports; > > -} > > - > > -void > > -binding_destroy_local_binding_lports(struct sset *lports) > > -{ > > - sset_destroy(lports); > > - free(lports); > > -} > > diff --git a/controller/binding.h b/controller/binding.h > > index 8f3289476..7a35faa0d 100644 > > --- a/controller/binding.h > > +++ b/controller/binding.h > > @@ -22,6 +22,7 @@ > > #include "openvswitch/hmap.h" > > #include "openvswitch/uuid.h" > > #include "openvswitch/list.h" > > +#include "sset.h" > > > > struct hmap; > > struct ovsdb_idl; > > @@ -93,6 +94,7 @@ struct binding_ctx_out { > > struct local_binding_data { > > struct shash bindings; > > struct shash lports; > > + struct sset port_bindings; > > }; > > > > void local_binding_data_init(struct local_binding_data *); > > @@ -133,13 +135,4 @@ bool binding_handle_port_binding_changes(struct > binding_ctx_in *, > > void binding_tracked_dp_destroy(struct hmap *tracked_datapaths); > > > > void binding_dump_local_bindings(struct local_binding_data *, struct ds > *); > > - > > -/* Generates a sset of lport names from local_binding_data. > > - * Note: the caller is responsible for destroying and freeing the > returned > > - * sset, by calling binding_detroy_local_binding_lports(). */ > > -struct sset *binding_collect_local_binding_lports(struct > local_binding_data *); > > - > > -/* Destroy and free the lports sset returned by > > - * binding_collect_local_binding_lports(). */ > > -void binding_destroy_local_binding_lports(struct sset *lports); > > #endif /* controller/binding.h */ > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index 3968ef059..5675b97dd 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -1635,11 +1635,8 @@ en_port_groups_run(struct engine_node *node, void > *data) > > struct ed_type_runtime_data *rt_data = > > engine_get_input_data("runtime_data", node); > > > > - struct sset *local_b_lports = binding_collect_local_binding_lports( > > - &rt_data->lbinding_data); > > - port_groups_init(pg_table, local_b_lports, &pg->port_group_ssets, > > - &pg->port_groups_cs_local); > > - binding_destroy_local_binding_lports(local_b_lports); > > + port_groups_init(pg_table, &rt_data->lbinding_data.port_bindings, > > + &pg->port_group_ssets, &pg->port_groups_cs_local); > > > > engine_set_node_state(node, EN_UPDATED); > > } > > @@ -1656,12 +1653,9 @@ port_groups_sb_port_group_handler(struct > engine_node *node, void *data) > > struct ed_type_runtime_data *rt_data = > > engine_get_input_data("runtime_data", node); > > > > - struct sset *local_b_lports = binding_collect_local_binding_lports( > > - &rt_data->lbinding_data); > > - port_groups_update(pg_table, local_b_lports, &pg->port_group_ssets, > > - &pg->port_groups_cs_local, &pg->new, &pg->deleted, > > - &pg->updated); > > - binding_destroy_local_binding_lports(local_b_lports); > > + port_groups_update(pg_table, &rt_data->lbinding_data.port_bindings, > > + &pg->port_group_ssets, &pg->port_groups_cs_local, > > + &pg->new, &pg->deleted, &pg->updated); > > > > if (!sset_is_empty(&pg->new) || !sset_is_empty(&pg->deleted) || > > !sset_is_empty(&pg->updated)) { > > @@ -1694,9 +1688,6 @@ port_groups_runtime_data_handler(struct engine_node > *node, void *data) > > goto out; > > } > > > > - struct sset *local_b_lports = binding_collect_local_binding_lports( > > - &rt_data->lbinding_data); > > - > > const struct sbrec_port_group *pg_sb; > > SBREC_PORT_GROUP_TABLE_FOR_EACH (pg_sb, pg_table) { > > struct sset *pg_lports = shash_find_data(&pg->port_group_ssets, > > @@ -1723,13 +1714,12 @@ port_groups_runtime_data_handler(struct > engine_node *node, void *data) > > if (need_update) { > > expr_const_sets_add_strings(&pg->port_groups_cs_local, > pg_sb->name, > > (const char *const *) > pg_sb->ports, > > - pg_sb->n_ports, local_b_lports); > > + pg_sb->n_ports, > > + > &rt_data->lbinding_data.port_bindings); > > sset_add(&pg->updated, pg_sb->name); > > } > > } > > > > - binding_destroy_local_binding_lports(local_b_lports); > > - > > out: > > if (!sset_is_empty(&pg->new) || !sset_is_empty(&pg->deleted) || > > !sset_is_empty(&pg->updated)) { > > -- > > 2.27.0 > > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
