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 .
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
