| Message ID | 20200218153159.843124-1-numans@ovn.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,ovn] ovn-controller: Don't clear the lflow resources ref during flow_output_run | expand |
On Tue, Feb 18, 2020 at 7:32 AM <numans@ovn.org> wrote: > > From: Numan Siddique <numans@ovn.org> > > 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. 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 <numans@ovn.org> > --- > 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Tue, Feb 18, 2020 at 10:34 PM Han Zhou <zhouhan@gmail.com> wrote: > > On Tue, Feb 18, 2020 at 7:32 AM <numans@ovn.org> wrote: > > > > From: Numan Siddique <numans@ovn.org> > > > > 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 <numans@ovn.org> > > --- > > 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 > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Tue, Feb 18, 2020 at 9:27 AM Numan Siddique <numans@ovn.org> wrote: > On Tue, Feb 18, 2020 at 10:34 PM Han Zhou <zhouhan@gmail.com> wrote: > > > > On Tue, Feb 18, 2020 at 7:32 AM <numans@ovn.org> wrote: > > > > > > From: Numan Siddique <numans@ovn.org> > > > > > > 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 <numans@ovn.org> > > > --- > > > 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 > > > dev@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > >
On Tue, Feb 18, 2020 at 11:50 PM Han Zhou <zhouhan@gmail.com> wrote: > > On Tue, Feb 18, 2020 at 9:27 AM Numan Siddique <numans@ovn.org> wrote: > > > On Tue, Feb 18, 2020 at 10:34 PM Han Zhou <zhouhan@gmail.com> wrote: > > > > > > On Tue, Feb 18, 2020 at 7:32 AM <numans@ovn.org> wrote: > > > > > > > > From: Numan Siddique <numans@ovn.org> > > > > > > > > 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. Hi Han, I've submitted v2 by taking a different approach as suggested by you. v2 will now store the addr set and port group name sets as part of lflow expr cache and add those to the lflow resource ref. Kindly take a look. I think this should work now without any issues :). Thanks Numan > > > > 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 <numans@ovn.org> > > > > --- > > > > 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 > > > > dev@openvswitch.org > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > _______________________________________________ > > > dev mailing list > > > dev@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Tue, Feb 18, 2020 at 11:56 AM Numan Siddique <numans@ovn.org> wrote: > > On Tue, Feb 18, 2020 at 11:50 PM Han Zhou <zhouhan@gmail.com> wrote: > > > > On Tue, Feb 18, 2020 at 9:27 AM Numan Siddique <numans@ovn.org> wrote: > > > > > On Tue, Feb 18, 2020 at 10:34 PM Han Zhou <zhouhan@gmail.com> wrote: > > > > > > > > On Tue, Feb 18, 2020 at 7:32 AM <numans@ovn.org> wrote: > > > > > > > > > > From: Numan Siddique <numans@ovn.org> > > > > > > > > > > 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. > > Hi Han, > > I've submitted v2 by taking a different approach as suggested by you. v2 will > now store the addr set and port group name sets as part of lflow expr cache > and add those to the lflow resource ref. > > Kindly take a look. I think this should work now without any issues :). > > Thanks > Numan > Hi Numan, Thanks for the revision. I acked. For the change-tracking problem I mentioned for commit 672508f6 (ovn-controller: Fix memory issues due to lflow expr caching.), do you plan to address that as well? It would still have memory leak in some corner cases, but the impact may be minor. Thanks, Han
On Wed, Feb 19, 2020 at 1:16 PM Han Zhou <zhouhan@gmail.com> wrote: > > On Tue, Feb 18, 2020 at 11:56 AM Numan Siddique <numans@ovn.org> wrote: > > > > On Tue, Feb 18, 2020 at 11:50 PM Han Zhou <zhouhan@gmail.com> wrote: > > > > > > On Tue, Feb 18, 2020 at 9:27 AM Numan Siddique <numans@ovn.org> wrote: > > > > > > > On Tue, Feb 18, 2020 at 10:34 PM Han Zhou <zhouhan@gmail.com> wrote: > > > > > > > > > > On Tue, Feb 18, 2020 at 7:32 AM <numans@ovn.org> wrote: > > > > > > > > > > > > From: Numan Siddique <numans@ovn.org> > > > > > > > > > > > > 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. > > > > Hi Han, > > > > I've submitted v2 by taking a different approach as suggested by you. v2 > will > > now store the addr set and port group name sets as part of lflow expr > cache > > and add those to the lflow resource ref. > > > > Kindly take a look. I think this should work now without any issues :). > > > > Thanks > > Numan > > > > Hi Numan, > > Thanks for the revision. I acked. > Thanks for the review. > For the change-tracking problem I mentioned for commit 672508f6 > (ovn-controller: Fix memory issues due to lflow expr caching.), do you plan > to address that as well? It would still have memory leak in some corner > cases, but the impact may be minor. I wouldn't say memory leak, but it would be in the cache lingering around. But I see the chances are less. Lets say few logical flows are deleted. If lflow_handle_changed_flows() handler is called, it'll clear the expr cache for these lflows. If full recompute is triggered, then lflow_run() will clear the expr cache if the deleted logical flows are still in the tracked buffer. If however, for some reason (may be due to engine abort) the deleted lflows are not tracked, the expr cache will not be cleared for these lflows. One way to address this would be to periodically clear the expr cache say every 'x' minutes. What do you think of this approach ? This would be a straightforward implementation. Thanks Numan > > Thanks, > Han > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Wed, Feb 19, 2020 at 2:11 AM Numan Siddique <numans@ovn.org> wrote: > > On Wed, Feb 19, 2020 at 1:16 PM Han Zhou <zhouhan@gmail.com> wrote: > > > > On Tue, Feb 18, 2020 at 11:56 AM Numan Siddique <numans@ovn.org> wrote: > > > > > > On Tue, Feb 18, 2020 at 11:50 PM Han Zhou <zhouhan@gmail.com> wrote: > > > > > > > > On Tue, Feb 18, 2020 at 9:27 AM Numan Siddique <numans@ovn.org> wrote: > > > > > > > > > On Tue, Feb 18, 2020 at 10:34 PM Han Zhou <zhouhan@gmail.com> wrote: > > > > > > > > > > > > On Tue, Feb 18, 2020 at 7:32 AM <numans@ovn.org> wrote: > > > > > > > > > > > > > > From: Numan Siddique <numans@ovn.org> > > > > > > > > > > > > > > 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. > > > > > > Hi Han, > > > > > > I've submitted v2 by taking a different approach as suggested by you. v2 > > will > > > now store the addr set and port group name sets as part of lflow expr > > cache > > > and add those to the lflow resource ref. > > > > > > Kindly take a look. I think this should work now without any issues :). > > > > > > Thanks > > > Numan > > > > > > > Hi Numan, > > > > Thanks for the revision. I acked. > > > > Thanks for the review. > > > For the change-tracking problem I mentioned for commit 672508f6 > > (ovn-controller: Fix memory issues due to lflow expr caching.), do you plan > > to address that as well? It would still have memory leak in some corner > > cases, but the impact may be minor. > > I wouldn't say memory leak, but it would be in the cache lingering around. > But I see the chances are less. > > Lets say few logical flows are deleted. If > lflow_handle_changed_flows() handler is called, > it'll clear the expr cache for these lflows. If full recompute is > triggered, then lflow_run() > will clear the expr cache if the deleted logical flows are still in > the tracked buffer. If however, > for some reason (may be due to engine abort) the deleted lflows are not tracked, > the expr cache will not be cleared for these lflows. > The common case for this to happen is when there is an update to SB from this chassis, e.g. for binding a port or mac-binding update, when the notification for the update comes, the ovnsb_idl_txn is NULL, so the round of engine_run() may be skipped. If in the same DB update notification there are also logical_flow deletions involved, the change tracking is lost and the cache is left lingering. With commit e2ab60e3a7 ("ovn-controller: Run I-P engine even when no SB txn is available.") the chance becomes smaller. It happens only if in the same iteration when ovnsb_idl_txn is NULL, another updates to SB is required, e.g. another port-binding change or mac-binding change happened on this chassis. > One way to address this would be to periodically clear the expr cache > say every 'x' minutes. > What do you think of this approach ? This would be a straightforward > implementation. > Given that the issue is not triggered frequently, the periodical cleanup sounds like a good idea. It may be undesirable to completely flush the cache, which can result in spike of CPU and latency every 'x' minutes. Instead, would it be better to do "garbage collection" every 'x' minutes, i.e. compare cache with all SB lflows, and delete only the non-existed entries? Thanks, Han
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) { 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'