On Mon, Apr 11, 2022 at 2:05 PM Numan Siddique <num...@ovn.org> wrote: > > On Sat, Feb 5, 2022 at 12:12 AM Han Zhou <hz...@ovn.org> wrote: > > > > On Fri, Feb 4, 2022 at 7:58 AM Mark Michelson <mmich...@redhat.com> wrote: > > > > > > As far as I can tell, this looks correct. > > > > > > Acked-by: Mark Michelson <mmich...@redhat.com> > > > > Thanks Mark. I applied it to the main branch. > > > Hi Han, > > We have an issue in OVN 21.12 (and OVN 21.09) where some of the > conjunction flows are missing and recompute resolves the issue. > > And applying this patch to branch-21.12 fixes the issue. > > I need to dig further. But before that I want to check with you if > you have any comments on this. > > The details of the issue can be found here - > https://bugzilla.redhat.com/show_bug.cgi?id=2071272#c12 > Hi Numan,
Thanks a lot for the details of the problem and such great analysis and reproducing steps. I reproduced the problem following your steps and figured out the root cause. In the test, it generates combined conjunctions and repeatedly processes (flood remove and then adding back) the logical flows that generated those combined conjunction flows in the same I-P run. Precondition: logical flow A has a conjunction flow OF_A logical flow B has two disjunctions but one of them has only one member (e.g. PG1 has only one LSP locally), so it doesn't have any conjunction flow yet The change: PG1 has a new LSP binded locally Execution of the I-P run: 1) port-group change handler recomputes logical flow B, which adds a combined conjunction to OF_A, because logical flow B has an overlapping disjunction set with logical flow A (e.g. the udp port range) - so OF_A is updated 2) port-binding change handler recomputes logical flow B, which flood-removes OVS flows including OF_A, and then re-add OF_A and then update it again with the combined conjunction 3) The sequence of change-tracking for OF_A is: update->delete->add->update. When adding to tracking, it is merged on-the-fly, to: delete OF_A -> add/update OF_A(new) 4) Before installing the flows incrementally, the function merge_tracked_flows() compares the *desired-flow* of OF_A(new) and OF_A and they are exactly the same, so they are both removed from the tracking, and simply link the OF_A(new) to the installed-flow of OF_A, as if OF_A doesn't need any change. So we can see that the problem here is that in 4) the merge_tracked_flows() assumes the OF_A's desired-flow is the same as the installed-flow, which is not correct in this scenario, because OF_A has been updated before being deleted, which means the installed-flow doesn't match the desired flow being deleted. The fix is in the merge_tracked_flows(), when finding an exact match between the added/updated flow and the deleted flows, we not only compare the desired flows but also the installed flows. Please take a look: https://patchwork.ozlabs.org/project/ovn/patch/20220413063412.2972298-1-hz...@ovn.org/ > I'm just wondering if backporting this commit would hide the real issue or not. Backporting this commit would definitely help, because it is less likely to trigger the "delete and add/update" change sequence for the same OVS flow in the same run. But I believe it is better to fix the root cause directly, without making assumptions of the flow change sequences. Thanks, Han > > Thanks > Numan > > > > > > > On 1/20/22 13:06, Han Zhou wrote: > > > > For I-P node lflow_output, different change handlers can trigger > > > > reprocessing lflows. However, it is a waste to reprocess the same lflow > > > > multiple times in the same run, because each processing of the lflow is > > > > against the same input data. > > > > > > > > For example, if a lflow references an addresset A and a port-group P, it > > > > is possible that both A and P are changed in the same run, the same > > > > lflow reprocess would be triggered by both > > > > lflow_output_addr_sets_handler() and lflow_output_port_groups_handler(). > > > > Another example may incur a even bigger waste is that when a NB > > port-group > > > > include ports across a big number of datapaths, so the lflow is > > > > referencing a big number of SB port-groups with DP group including all > > > > those DPs. When there are multiple port changes in the NB port-group, > > > > there can be multiple small SB port-group changes, and so the lflows can > > > > be reprocessed multiple times. > > > > > > > > This patch avoid reprocessing the same lflow in the same I-P run, which > > > > should improve performance in above scenarios. > > > > > > > > There is only one exception when a lflow may still be reprocessed more > > > > than once: if a lflow A is processed, which results in a compound > > > > conjunction added to an existed flow generated by an exited lflow B, and > > > > then lflow B needs to be reprocessed in the same run, which would > > > > cause flood-remove and reprocess lflow A. In this case lflow A is > > > > processed twice. > > > > > > > > Apart from the performance gains, there is also a potential problem of > > > > DP group fixed by this patch. If there is addrset/pg change and at the > > > > same run there is also a new local datapath monitored, then the existed > > > > code would firstly handle the addrset/pg changes causing a lflow being > > > > processed against the DP group including the new DP, which could have > > > > conjunction flows, but later the same lflow is reprocessed by > > > > lflow_output_runtime_data_handler()->lflow_add_flows_for_datapath() for > > > > the new DP only. Because lflows adding conjunction flows will not be > > > > checked against redundancy but only tries to combine the conjunction > > > > actions, it would result in redundanct conjunction actions added to the > > > > same flow, which is also linked to the same SB lflow twice. The > > > > mechanism of this patch will avoid this problem. > > > > > > > > Signed-off-by: Han Zhou <hz...@ovn.org> > > > > --- > > > > controller/lflow.c | 143 +++++++++++++++++++++++++++++++----- > > > > controller/lflow.h | 7 ++ > > > > controller/ovn-controller.c | 18 ++++- > > > > 3 files changed, 147 insertions(+), 21 deletions(-) > > > > > > > > diff --git a/controller/lflow.c b/controller/lflow.c > > > > index 933e2f3cc..b976c7d56 100644 > > > > --- a/controller/lflow.c > > > > +++ b/controller/lflow.c > > > > @@ -78,8 +78,16 @@ consider_logical_flow(const struct > > sbrec_logical_flow *lflow, > > > > struct hmap *dhcp_opts, struct hmap > > *dhcpv6_opts, > > > > struct hmap *nd_ra_opts, > > > > struct controller_event_options > > *controller_event_opts, > > > > + bool is_recompute, > > > > struct lflow_ctx_in *l_ctx_in, > > > > struct lflow_ctx_out *l_ctx_out); > > > > +static struct lflow_processed_node * > > > > +lflows_processed_find(struct hmap *lflows_processed, > > > > + const struct uuid *lflow_uuid); > > > > +static void lflows_processed_add(struct hmap *lflows_processed, > > > > + const struct uuid *lflow_uuid); > > > > +static void lflows_processed_remove(struct hmap *lflows_processed, > > > > + struct lflow_processed_node *node); > > > > static void lflow_resource_add(struct lflow_resource_ref *, enum > > ref_type, > > > > const char *ref_name, const struct > > uuid *); > > > > static struct ref_lflow_node *ref_lflow_lookup(struct hmap > > *ref_lflow_table, > > > > @@ -366,7 +374,7 @@ add_logical_flows(struct lflow_ctx_in *l_ctx_in, > > > > > > > > SBREC_LOGICAL_FLOW_TABLE_FOR_EACH (lflow, > > l_ctx_in->logical_flow_table) { > > > > consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts, > > > > - &nd_ra_opts, &controller_event_opts, > > > > + &nd_ra_opts, &controller_event_opts, > > true, > > > > l_ctx_in, l_ctx_out); > > > > } > > > > > > > > @@ -413,6 +421,12 @@ lflow_handle_changed_flows(struct lflow_ctx_in > > *l_ctx_in, > > > > struct ofctrl_flood_remove_node *ofrn, *next; > > > > SBREC_LOGICAL_FLOW_TABLE_FOR_EACH_TRACKED (lflow, > > > > > > l_ctx_in->logical_flow_table) { > > > > + if (lflows_processed_find(l_ctx_out->lflows_processed, > > > > + &lflow->header_.uuid)) { > > > > + VLOG_DBG("lflow "UUID_FMT"has been processed, skip.", > > > > + UUID_ARGS(&lflow->header_.uuid)); > > > > + continue; > > > > + } > > > > VLOG_DBG("delete lflow "UUID_FMT, > > UUID_ARGS(&lflow->header_.uuid)); > > > > ofrn = xmalloc(sizeof *ofrn); > > > > ofrn->sb_uuid = lflow->header_.uuid; > > > > @@ -437,8 +451,20 @@ lflow_handle_changed_flows(struct lflow_ctx_in > > *l_ctx_in, > > > > if (lflow) { > > > > VLOG_DBG("re-add lflow "UUID_FMT, > > > > UUID_ARGS(&lflow->header_.uuid)); > > > > + > > > > + /* For the extra lflows that need to be reprocessed > > because of the > > > > + * flood remove, remove it from lflows_processed. */ > > > > + struct lflow_processed_node *lfp_node = > > > > + lflows_processed_find(l_ctx_out->lflows_processed, > > > > + &lflow->header_.uuid); > > > > + if (lfp_node) { > > > > + VLOG_DBG("lflow "UUID_FMT"has been processed, now > > reprocess.", > > > > + UUID_ARGS(&lflow->header_.uuid)); > > > > + lflows_processed_remove(l_ctx_out->lflows_processed, > > lfp_node); > > > > + } > > > > + > > > > consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts, > > > > - &nd_ra_opts, &controller_event_opts, > > > > + &nd_ra_opts, &controller_event_opts, > > false, > > > > l_ctx_in, l_ctx_out); > > > > } > > > > } > > > > @@ -473,15 +499,22 @@ lflow_handle_changed_ref(enum ref_type ref_type, > > const char *ref_name, > > > > *changed = false; > > > > bool ret = true; > > > > > > > > - hmap_remove(&l_ctx_out->lfrr->ref_lflow_table, &rlfn->node); > > > > + struct ovs_list lflows_todo = OVS_LIST_INITIALIZER(&lflows_todo); > > > > > > > > - struct lflow_ref_list_node *lrln, *next; > > > > - /* Detach the rlfn->lflow_uuids nodes from the lfrr table and clean > > > > - * up all other nodes related to the lflows that uses the resource, > > > > - * so that the old nodes won't interfere with updating the lfrr > > table > > > > - * when reparsing the lflows. */ > > > > + struct lflow_ref_list_node *lrln, *lrln_uuid, *lrln_uuid_next; > > > > HMAP_FOR_EACH (lrln, hmap_node, &rlfn->lflow_uuids) { > > > > - ovs_list_remove(&lrln->list_node); > > > > + if (lflows_processed_find(l_ctx_out->lflows_processed, > > > > + &lrln->lflow_uuid)) { > > > > + continue; > > > > + } > > > > + /* Use lflow_ref_list_node as list node to store the uuid. > > > > + * Other fields are not used here. */ > > > > + lrln_uuid = xmalloc(sizeof *lrln_uuid); > > > > + lrln_uuid->lflow_uuid = lrln->lflow_uuid; > > > > + ovs_list_push_back(&lflows_todo, &lrln_uuid->list_node); > > > > + } > > > > + if (ovs_list_is_empty(&lflows_todo)) { > > > > + return true; > > > > } > > > > > > > > struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts); > > > > @@ -509,17 +542,19 @@ lflow_handle_changed_ref(enum ref_type ref_type, > > const char *ref_name, > > > > /* Re-parse the related lflows. */ > > > > /* Firstly, flood remove the flows from desired flow table. */ > > > > struct hmap flood_remove_nodes = > > HMAP_INITIALIZER(&flood_remove_nodes); > > > > - struct ofctrl_flood_remove_node *ofrn, *ofrn_next; > > > > - HMAP_FOR_EACH (lrln, hmap_node, &rlfn->lflow_uuids) { > > > > + LIST_FOR_EACH_SAFE (lrln_uuid, lrln_uuid_next, list_node, > > &lflows_todo) { > > > > VLOG_DBG("Reprocess lflow "UUID_FMT" for resource type: %d," > > > > " name: %s.", > > > > - UUID_ARGS(&lrln->lflow_uuid), > > > > + UUID_ARGS(&lrln_uuid->lflow_uuid), > > > > ref_type, ref_name); > > > > - ofctrl_flood_remove_add_node(&flood_remove_nodes, > > &lrln->lflow_uuid); > > > > + ofctrl_flood_remove_add_node(&flood_remove_nodes, > > > > + &lrln_uuid->lflow_uuid); > > > > + free(lrln_uuid); > > > > } > > > > ofctrl_flood_remove_flows(l_ctx_out->flow_table, > > &flood_remove_nodes); > > > > > > > > /* Secondly, for each lflow that is actually removed, > > reprocessing it. */ > > > > + struct ofctrl_flood_remove_node *ofrn, *ofrn_next; > > > > HMAP_FOR_EACH (ofrn, hmap_node, &flood_remove_nodes) { > > > > lflow_resource_destroy_lflow(l_ctx_out->lfrr, &ofrn->sb_uuid); > > > > lflow_conj_ids_free(l_ctx_out->conj_ids, &ofrn->sb_uuid); > > > > @@ -535,8 +570,19 @@ lflow_handle_changed_ref(enum ref_type ref_type, > > const char *ref_name, > > > > continue; > > > > } > > > > > > > > + /* For the extra lflows that need to be reprocessed because of > > the > > > > + * flood remove, remove it from lflows_processed. */ > > > > + struct lflow_processed_node *lfp_node = > > > > + lflows_processed_find(l_ctx_out->lflows_processed, > > > > + &lflow->header_.uuid); > > > > + if (lfp_node) { > > > > + VLOG_DBG("lflow "UUID_FMT"has been processed, now > > reprocess.", > > > > + UUID_ARGS(&lflow->header_.uuid)); > > > > + lflows_processed_remove(l_ctx_out->lflows_processed, > > lfp_node); > > > > + } > > > > + > > > > consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts, > > > > - &nd_ra_opts, &controller_event_opts, > > > > + &nd_ra_opts, &controller_event_opts, > > false, > > > > l_ctx_in, l_ctx_out); > > > > *changed = true; > > > > } > > > > @@ -546,12 +592,6 @@ lflow_handle_changed_ref(enum ref_type ref_type, > > const char *ref_name, > > > > } > > > > hmap_destroy(&flood_remove_nodes); > > > > > > > > - HMAP_FOR_EACH_SAFE (lrln, next, hmap_node, &rlfn->lflow_uuids) { > > > > - hmap_remove(&rlfn->lflow_uuids, &lrln->hmap_node); > > > > - free(lrln); > > > > - } > > > > - ref_lflow_node_destroy(rlfn); > > > > - > > > > dhcp_opts_destroy(&dhcp_opts); > > > > dhcp_opts_destroy(&dhcpv6_opts); > > > > nd_ra_opts_destroy(&nd_ra_opts); > > > > @@ -969,11 +1009,54 @@ done: > > > > free(matches); > > > > } > > > > > > > > +static struct lflow_processed_node * > > > > +lflows_processed_find(struct hmap *lflows_processed, > > > > + const struct uuid *lflow_uuid) > > > > +{ > > > > + struct lflow_processed_node *node; > > > > + HMAP_FOR_EACH_WITH_HASH (node, hmap_node, uuid_hash(lflow_uuid), > > > > + lflows_processed) { > > > > + if (uuid_equals(&node->lflow_uuid, lflow_uuid)) { > > > > + return node; > > > > + } > > > > + } > > > > + return NULL; > > > > +} > > > > + > > > > +static void > > > > +lflows_processed_add(struct hmap *lflows_processed, > > > > + const struct uuid *lflow_uuid) > > > > +{ > > > > + struct lflow_processed_node *node = xmalloc(sizeof *node); > > > > + node->lflow_uuid = *lflow_uuid; > > > > + hmap_insert(lflows_processed, &node->hmap_node, > > uuid_hash(lflow_uuid)); > > > > +} > > > > + > > > > +static void > > > > +lflows_processed_remove(struct hmap *lflows_processed, > > > > + struct lflow_processed_node *node) > > > > +{ > > > > + hmap_remove(lflows_processed, &node->hmap_node); > > > > + free(node); > > > > +} > > > > + > > > > +void > > > > +lflows_processed_destroy(struct hmap *lflows_processed) > > > > +{ > > > > + struct lflow_processed_node *node, *next; > > > > + HMAP_FOR_EACH_SAFE (node, next, hmap_node, lflows_processed) { > > > > + hmap_remove(lflows_processed, &node->hmap_node); > > > > + free(node); > > > > + } > > > > + hmap_destroy(lflows_processed); > > > > +} > > > > + > > > > static void > > > > consider_logical_flow(const struct sbrec_logical_flow *lflow, > > > > struct hmap *dhcp_opts, struct hmap > > *dhcpv6_opts, > > > > struct hmap *nd_ra_opts, > > > > struct controller_event_options > > *controller_event_opts, > > > > + bool is_recompute, > > > > struct lflow_ctx_in *l_ctx_in, > > > > struct lflow_ctx_out *l_ctx_out) > > > > { > > > > @@ -987,6 +1070,13 @@ consider_logical_flow(const struct > > sbrec_logical_flow *lflow, > > > > } > > > > ovs_assert(!dp_group || !dp); > > > > > > > > + if (!is_recompute) { > > > > + ovs_assert(!lflows_processed_find(l_ctx_out->lflows_processed, > > > > + &lflow->header_.uuid)); > > > > + lflows_processed_add(l_ctx_out->lflows_processed, > > > > + &lflow->header_.uuid); > > > > + } > > > > + > > > > if (dp) { > > > > consider_logical_flow__(lflow, dp, > > > > dhcp_opts, dhcpv6_opts, nd_ra_opts, > > > > @@ -1923,6 +2013,12 @@ lflow_add_flows_for_datapath(const struct > > sbrec_datapath_binding *dp, > > > > const struct sbrec_logical_flow *lflow; > > > > SBREC_LOGICAL_FLOW_FOR_EACH_EQUAL ( > > > > lflow, lf_row, > > l_ctx_in->sbrec_logical_flow_by_logical_datapath) { > > > > + if (lflows_processed_find(l_ctx_out->lflows_processed, > > > > + &lflow->header_.uuid)) { > > > > + continue; > > > > + } > > > > + lflows_processed_add(l_ctx_out->lflows_processed, > > > > + &lflow->header_.uuid); > > > > consider_logical_flow__(lflow, dp, &dhcp_opts, &dhcpv6_opts, > > > > &nd_ra_opts, &controller_event_opts, > > > > l_ctx_in, l_ctx_out); > > > > @@ -1949,6 +2045,13 @@ lflow_add_flows_for_datapath(const struct > > sbrec_datapath_binding *dp, > > > > sbrec_logical_flow_index_set_logical_dp_group(lf_row, ldpg); > > > > SBREC_LOGICAL_FLOW_FOR_EACH_EQUAL ( > > > > lflow, lf_row, > > l_ctx_in->sbrec_logical_flow_by_logical_dp_group) { > > > > + if (lflows_processed_find(l_ctx_out->lflows_processed, > > > > + &lflow->header_.uuid)) { > > > > + continue; > > > > + } > > > > + /* Don't call lflows_processed_add() because here we > > process the > > > > + * lflow only for one of the DPs in the DP group, which > > may be > > > > + * incomplete. */ > > > > consider_logical_flow__(lflow, dp, &dhcp_opts, > > &dhcpv6_opts, > > > > &nd_ra_opts, > > &controller_event_opts, > > > > l_ctx_in, l_ctx_out); > > > > diff --git a/controller/lflow.h b/controller/lflow.h > > > > index 489dd70fb..d6dad17ec 100644 > > > > --- a/controller/lflow.h > > > > +++ b/controller/lflow.h > > > > @@ -159,10 +159,17 @@ struct lflow_ctx_out { > > > > struct lflow_resource_ref *lfrr; > > > > struct lflow_cache *lflow_cache; > > > > struct conj_ids *conj_ids; > > > > + struct hmap *lflows_processed; > > > > struct simap *hairpin_lb_ids; > > > > struct id_pool *hairpin_id_pool; > > > > }; > > > > > > > > +struct lflow_processed_node { > > > > + struct hmap_node hmap_node; /* In > > ed_type_lflow_output.lflows_processed. */ > > > > + struct uuid lflow_uuid; > > > > +}; > > > > +void lflows_processed_destroy(struct hmap *lflows_processed); > > > > + > > > > void lflow_init(void); > > > > void lflow_run(struct lflow_ctx_in *, struct lflow_ctx_out *); > > > > void lflow_handle_cached_flows(struct lflow_cache *, > > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > > > index 5069aedfc..8631bccbc 100644 > > > > --- a/controller/ovn-controller.c > > > > +++ b/controller/ovn-controller.c > > > > @@ -2176,6 +2176,11 @@ struct ed_type_lflow_output { > > > > /* conjunciton ID usage information of lflows */ > > > > struct conj_ids conj_ids; > > > > > > > > + /* lflows processed in the current engine execution. > > > > + * Cleared by en_lflow_output_clear_tracked_data before each engine > > > > + * execution. */ > > > > + struct hmap lflows_processed; > > > > + > > > > /* Data which is persistent and not cleared during > > > > * full recompute. */ > > > > struct lflow_output_persistent_data pd; > > > > @@ -2307,6 +2312,7 @@ init_lflow_ctx(struct engine_node *node, > > > > l_ctx_out->meter_table = &fo->meter_table; > > > > l_ctx_out->lfrr = &fo->lflow_resource_ref; > > > > l_ctx_out->conj_ids = &fo->conj_ids; > > > > + l_ctx_out->lflows_processed = &fo->lflows_processed; > > > > l_ctx_out->lflow_cache = fo->pd.lflow_cache; > > > > l_ctx_out->hairpin_id_pool = fo->hd.pool; > > > > l_ctx_out->hairpin_lb_ids = &fo->hd.ids; > > > > @@ -2322,11 +2328,20 @@ en_lflow_output_init(struct engine_node *node > > OVS_UNUSED, > > > > ovn_extend_table_init(&data->meter_table); > > > > lflow_resource_init(&data->lflow_resource_ref); > > > > lflow_conj_ids_init(&data->conj_ids); > > > > + hmap_init(&data->lflows_processed); > > > > simap_init(&data->hd.ids); > > > > data->hd.pool = id_pool_create(1, UINT32_MAX - 1); > > > > return data; > > > > } > > > > > > > > +static void > > > > +en_lflow_output_clear_tracked_data(void *data) > > > > +{ > > > > + struct ed_type_lflow_output *flow_output_data = data; > > > > + lflows_processed_destroy(&flow_output_data->lflows_processed); > > > > + hmap_init(&flow_output_data->lflows_processed); > > > > +} > > > > + > > > > static void > > > > en_lflow_output_cleanup(void *data) > > > > { > > > > @@ -2336,6 +2351,7 @@ en_lflow_output_cleanup(void *data) > > > > ovn_extend_table_destroy(&flow_output_data->meter_table); > > > > lflow_resource_destroy(&flow_output_data->lflow_resource_ref); > > > > lflow_conj_ids_destroy(&flow_output_data->conj_ids); > > > > + lflows_processed_destroy(&flow_output_data->lflows_processed); > > > > lflow_cache_destroy(flow_output_data->pd.lflow_cache); > > > > simap_destroy(&flow_output_data->hd.ids); > > > > id_pool_destroy(flow_output_data->hd.pool); > > > > @@ -3213,7 +3229,7 @@ main(int argc, char *argv[]) > > > > ENGINE_NODE(mff_ovn_geneve, "mff_ovn_geneve"); > > > > ENGINE_NODE(ofctrl_is_connected, "ofctrl_is_connected"); > > > > ENGINE_NODE(pflow_output, "physical_flow_output"); > > > > - ENGINE_NODE(lflow_output, "logical_flow_output"); > > > > + ENGINE_NODE_WITH_CLEAR_TRACK_DATA(lflow_output, > > "logical_flow_output"); > > > > ENGINE_NODE(flow_output, "flow_output"); > > > > ENGINE_NODE(addr_sets, "addr_sets"); > > > > ENGINE_NODE_WITH_CLEAR_TRACK_DATA(port_groups, "port_groups"); > > > > > > > > > _______________________________________________ > > dev mailing list > > d...@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev