| Message ID | 20210625115038.15677-1-dceara@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] ovn-controller: Fix port group I-P when they contain non-vif ports. | expand |
On 6/25/21 1:50 PM, Dumitru Ceara 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. > > Reported-at: https://github.com/ovn-org/ovn/pull/61#issuecomment-865094163 > Reported-by: Antonio Ojea <aojea@redhat.com> > Fixes: 0cfeba6b55e3 ("ovn-controller: Fix port group conjunction flow explosion problem.") > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- Hi Han, It would be great if you could have a look at this patch, commit 0cfeba6b55e3 ("ovn-controller: Fix port group conjunction flow explosion problem.") breaks ACL use cases in ovn-kubernetes. Thanks, Dumitru
On Fri, Jun 25, 2021 at 7:51 AM Dumitru Ceara <dceara@redhat.com> 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. > > Reported-at: https://github.com/ovn-org/ovn/pull/61#issuecomment-865094163 > Reported-by: Antonio Ojea <aojea@redhat.com> > Fixes: 0cfeba6b55e3 ("ovn-controller: Fix port group conjunction flow explosion problem.") > Signed-off-by: Dumitru Ceara <dceara@redhat.com> Hi Dumitru, Thanks for the fix. I think it would be great to have a test case to exercise the scenario. I've one small nit comment. Otherwise the patch looks good to me. Thanks Numan > --- > 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; I'd suggest changing the name of this variable to "lport_names". Although sset would indicate that it represents a collection of port binding names, "lport_names" would be clear in my opinion. > }; > > 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 6/25/21 4:36 PM, Numan Siddique wrote: > On Fri, Jun 25, 2021 at 7:51 AM Dumitru Ceara <dceara@redhat.com> 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. >> >> Reported-at: https://github.com/ovn-org/ovn/pull/61#issuecomment-865094163 >> Reported-by: Antonio Ojea <aojea@redhat.com> >> Fixes: 0cfeba6b55e3 ("ovn-controller: Fix port group conjunction flow explosion problem.") >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> > > Hi Dumitru, Hi Numan, Thanks for the review! > > Thanks for the fix. I think it would be great to have a test case to > exercise the scenario. Definitely, I'll add one in v2. > > I've one small nit comment. Otherwise the patch looks good to me. I'll take care of the comment too in v2. > > Thanks > Numan > Regards, Dumitru
On Fri, Jun 25, 2021 at 4:50 AM Dumitru Ceara <dceara@redhat.com> 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; According to the code (and also the bug the patch is trying to fix), lbinding_data is supposed to maintain VIFs only. 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. 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)? (+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 <aojea@redhat.com> > Fixes: 0cfeba6b55e3 ("ovn-controller: Fix port group conjunction flow explosion problem.") > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > 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 >
On Fri, Jun 25, 2021 at 2:53 PM Han Zhou <hzhou@ovn.org> wrote: > > On Fri, Jun 25, 2021 at 4:50 AM Dumitru Ceara <dceara@redhat.com> 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 <aojea@redhat.com> > > Fixes: 0cfeba6b55e3 ("ovn-controller: Fix port group conjunction flow > explosion problem.") > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > > --- > > 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Fri, Jun 25, 2021 at 2:38 PM Numan Siddique <numans@ovn.org> wrote: > > On Fri, Jun 25, 2021 at 2:53 PM Han Zhou <hzhou@ovn.org> wrote: > > > > On Fri, Jun 25, 2021 at 4:50 AM Dumitru Ceara <dceara@redhat.com> 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. Agree. And I just noticed that the comments for local_lport_ids in ovn-controller.c is not correct any more (probably since very long time ago): /* Contains the same ports as local_lports, but in the format: * <datapath-tunnel-key>_<port-tunnel-key> */ struct sset local_lport_ids; It in fact contains more lports than local_lports, including patch ports, and they are used for different purposes. I think we should rename them. > > > 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. I checked the code again and realized that local_lports doesn't contain patch ports, so it cannot be used for the port-group I-P directly. I think you meant that it can be reused if we add patch ports to it, but then zone id allocation behavior will be affected. In my opinion we'd better not do so because they serve different purposes. It looks that what is required by port-group processing is close to what we have for local_lport_ids, but just names instead of tunnel keys, so we may combine with local_lport_ids as a struct, e.g.: struct related_lports { struct sset lport_names; struct sset lport_ids; /* the current local_lport_ids */ } "related_lports" represents lports that are related to this chassis, including patch ports, to distinguish from the "local_lports". Maybe the name is still not good, but I just couldn't think of a better one. The functions update_local_lport_ids()/remove_local_lport_ids() are indeed the right place to update it (except that we should rename them). Thanks, Han > 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 <aojea@redhat.com> > > > Fixes: 0cfeba6b55e3 ("ovn-controller: Fix port group conjunction flow > > explosion problem.") > > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > > > --- > > > 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 > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On 6/28/21 8:05 AM, Han Zhou wrote: > On Fri, Jun 25, 2021 at 2:38 PM Numan Siddique <numans@ovn.org> wrote: >> >> On Fri, Jun 25, 2021 at 2:53 PM Han Zhou <hzhou@ovn.org> wrote: >>> >>> On Fri, Jun 25, 2021 at 4:50 AM Dumitru Ceara <dceara@redhat.com> 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. > > Agree. And I just noticed that the comments for local_lport_ids in > ovn-controller.c is not correct any more (probably since very long time > ago): > /* Contains the same ports as local_lports, but in the format: > > * <datapath-tunnel-key>_<port-tunnel-key> */ > > struct sset local_lport_ids; > > It in fact contains more lports than local_lports, including patch ports, > and they are used for different purposes. I think we should rename them. > >> >>> 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. I checked the code again and realized that local_lports > doesn't contain patch ports, so it cannot be used for the port-group I-P > directly. I think you meant that it can be reused if we add patch ports to > it, but then zone id allocation behavior will be affected. In my opinion > we'd better not do so because they serve different purposes. It looks that > what is required by port-group processing is close to what we have for > local_lport_ids, but just names instead of tunnel keys, so we may combine > with local_lport_ids as a struct, e.g.: > > struct related_lports { > struct sset lport_names; > struct sset lport_ids; /* the current local_lport_ids */ > } > > "related_lports" represents lports that are related to this chassis, > including patch ports, to distinguish from the "local_lports". Maybe the > name is still not good, but I just couldn't think of a better one. > The functions update_local_lport_ids()/remove_local_lport_ids() are indeed > the right place to update it (except that we should rename them). > > Thanks, > Han > Thanks Han and Numan for the reviews and discussion. I went ahead and sent v2 with the "related_lports" structure suggested above: http://patchwork.ozlabs.org/project/ovn/list/?series=251337&state=* I added comments to the new structure but indeed we likely need a follow up patch to document more of the remaining structures in binding.h. Regards, Dumitru
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)) {
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. Reported-at: https://github.com/ovn-org/ovn/pull/61#issuecomment-865094163 Reported-by: Antonio Ojea <aojea@redhat.com> Fixes: 0cfeba6b55e3 ("ovn-controller: Fix port group conjunction flow explosion problem.") Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- controller/binding.c | 25 +++++-------------------- controller/binding.h | 11 ++--------- controller/ovn-controller.c | 24 +++++++----------------- 3 files changed, 14 insertions(+), 46 deletions(-)