On Tue, Feb 18, 2020 at 9:27 AM Numan Siddique <[email protected]> wrote:
> On Tue, Feb 18, 2020 at 10:34 PM Han Zhou <[email protected]> wrote: > > > > On Tue, Feb 18, 2020 at 7:32 AM <[email protected]> wrote: > > > > > > From: Numan Siddique <[email protected]> > > > > > > After the patch [1], which added caching of lflow expr, the > > lflow_resource_ref > > > is not rebuilt properly when lflow_run() is called. If a lflow is > already > > cached > > > in lflow expr cache, then the lflow_resource_ref is not updated. > > > But flow_output_run() clears the lflow_resource_ref cache before > calling > > lflow_run(). > > > > > > This results in incorrect OF flows present in the switch even if the > > > address set/port group references have changed. > > > > > > This patch fixes this issue by not clearing the lflow_resource_ref > cache. > > > This cache gets cleaned up during lflow change handler or in > lflow_run(). > > > > Hi Numan, > > > > This approach looks dangerous to me, since the lflow_resource_ref is not > a > > cache but part of the engine data. Originally, the life cycle of it > follows > > the same principle like other data of I-P engine, now if we change the > > principle we need to be very careful. > > At least one scenario would have problem. E.g. when there is a pending > > transaction, we cannot run engine in that iteration, and we will trigger > a > > complete recompute next time, because the tracking data would be lost in > > the next iteration. So it is not possible to call > > lflow_resource_destroy_lflow() for deleted rows in that case because > there > > won't be any deleted flows tracked. It seems the commit 672508f6 > > (ovn-controller: Fix memory issues due to lflow expr caching.) would have > > similar problem, too, in that case. > > > > I am not sure if there would be other corner cases that would have issue > > with this approach. Probably we can think more of it for making the data > > required to build the lflow_resource_ref part of the expr cache. > > Hi Han, > > I'll take a closer look at your comments and come back tomorrow. > Is it wise to disable or revert lflow expr caching commit ? And then > address all these > issues properly ? SInce we are close to 20.03 release . > Yes, reverting for 20.03 sounds good to me. We can always back port later when it’s mature enough. > > Thanks > Numan > > > > > Thanks, > > Han > > > > > > [1] - 8795bec737b9("ovn-controller: Cache logical flow expr tree for > each > > lflow.") > > > Fixes: 8795bec737b9("ovn-controller: Cache logical flow expr tree for > > each lflow.") > > > > > > Signed-off-by: Numan Siddique <[email protected]> > > > --- > > > controller/lflow.c | 3 +++ > > > controller/ovn-controller.c | 2 -- > > > tests/ovn.at | 4 ++++ > > > 3 files changed, 7 insertions(+), 2 deletions(-) > > > > > > diff --git a/controller/lflow.c b/controller/lflow.c > > > index 79d89131a..072b2f1f1 100644 > > > --- a/controller/lflow.c > > > +++ b/controller/lflow.c > > > @@ -924,6 +924,9 @@ lflow_run(struct lflow_ctx_in *l_ctx_in, struct > > lflow_ctx_out *l_ctx_out) > > > SBREC_LOGICAL_FLOW_TABLE_FOR_EACH_TRACKED (lflow, > > > > > l_ctx_in->logical_flow_table) { > > > > Use of FOR_EACH_TRACKED in recompute is not guaranteed to get tracked > > changes. > > > > > if (sbrec_logical_flow_is_deleted(lflow)) { > > > + /* Delete entries from lflow resource reference. */ > > > + lflow_resource_destroy_lflow(l_ctx_out->lfrr, > > > + &lflow->header_.uuid); > > > struct lflow_expr *le = > > > lflow_expr_get(l_ctx_out->lflow_expr_cache, lflow); > > > if (le) { > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > > index 4d245ca28..b3be5c2db 100644 > > > --- a/controller/ovn-controller.c > > > +++ b/controller/ovn-controller.c > > > @@ -1441,7 +1441,6 @@ en_flow_output_run(struct engine_node *node, void > > *data) > > > struct ovn_extend_table *group_table = &fo->group_table; > > > struct ovn_extend_table *meter_table = &fo->meter_table; > > > uint32_t *conj_id_ofs = &fo->conj_id_ofs; > > > - struct lflow_resource_ref *lfrr = &fo->lflow_resource_ref; > > > > > > static bool first_run = true; > > > if (first_run) { > > > @@ -1450,7 +1449,6 @@ en_flow_output_run(struct engine_node *node, void > > *data) > > > ovn_desired_flow_table_clear(flow_table); > > > ovn_extend_table_clear(group_table, false /* desired */); > > > ovn_extend_table_clear(meter_table, false /* desired */); > > > - lflow_resource_clear(lfrr); > > > } > > > > > > *conj_id_ofs = 1; > > > diff --git a/tests/ovn.at b/tests/ovn.at > > > index ea6760e1f..254645a3a 100644 > > > --- a/tests/ovn.at > > > +++ b/tests/ovn.at > > > @@ -13778,6 +13778,10 @@ for i in 1 2 3; do > > > n_flows_after=`ovs-ofctl dump-flows br-int | grep "10.1.2.10" | wc > > -l` > > > AT_CHECK([test $(expr $n_flows_before \* 2) = $n_flows_after], > [0], > > [ignore]) > > > > > > + # Trigger full recompute. Creating a chassis would trigger full > > recompute. > > > + ovn-sbctl chassis-add tst geneve 127.0.0.4 > > > + ovn-sbctl chassis-del tst > > > + > > > # Remove an ACL > > > ovn-nbctl --wait=hv acl-del ls1 to-lport 200 \ > > > 'outport=="lp2" && ip4 && ip4.src == $as1' > > > -- > > > 2.24.1 > > > > > > _______________________________________________ > > > 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 > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
