| Message ID | 20200212111131.374234-1-numans@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,ovn] ovn-controller: Fix memory issues due to lflow expr caching. | expand |
On 2/12/20 12:11 PM, numans@ovn.org wrote: > From: Numan Siddique <numans@ovn.org> > > The patch [1], which added caching of lflow expr introduced a memory > leak. The patch [1] also didn't take care of deleting the expr from the cache > for the deleted lflows. This results in those lflow exprs in cache hanging in > forever. This patch also addresses these 2 issues. > > [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> Hi Numan, Looks good to me. I also gave it a run on our scale testing setup and I don't see the memleak anymore. Acked-by: Dumitru Ceara <dceara@redhat.com> Thanks, Dumitru > --- > controller/lflow.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/controller/lflow.c b/controller/lflow.c > index 780aa9331..79d89131a 100644 > --- a/controller/lflow.c > +++ b/controller/lflow.c > @@ -402,6 +402,11 @@ lflow_handle_changed_flows(struct lflow_ctx_in *l_ctx_in, > /* 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) { > + lflow_expr_delete(l_ctx_out->lflow_expr_cache, le); > + } > } > } > > @@ -660,6 +665,8 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow, > expr = expr_normalize(expr); > > lflow_expr_add(l_ctx_out->lflow_expr_cache, lflow, expr); > + } else { > + expr_destroy(prereqs); > } > > struct condition_aux cond_aux = { > @@ -910,6 +917,21 @@ lflow_run(struct lflow_ctx_in *l_ctx_in, struct lflow_ctx_out *l_ctx_out) > { > COVERAGE_INC(lflow_run); > > + /* when lflow_run is called, it's possible that some of the logical flows > + * are deleted. We need to delete the lflow expr cache for these lflows, > + * otherwise, they will not be deleted at all. */ > + const struct sbrec_logical_flow *lflow; > + SBREC_LOGICAL_FLOW_TABLE_FOR_EACH_TRACKED (lflow, > + l_ctx_in->logical_flow_table) { > + if (sbrec_logical_flow_is_deleted(lflow)) { > + struct lflow_expr *le = > + lflow_expr_get(l_ctx_out->lflow_expr_cache, lflow); > + if (le) { > + lflow_expr_delete(l_ctx_out->lflow_expr_cache, le); > + } > + } > + } > + > add_logical_flows(l_ctx_in, l_ctx_out); > add_neighbor_flows(l_ctx_in->sbrec_port_binding_by_name, > l_ctx_in->mac_binding_table, l_ctx_out->flow_table); >
On Wed, Feb 12, 2020 at 5:44 PM Dumitru Ceara <dceara@redhat.com> wrote: > > On 2/12/20 12:11 PM, numans@ovn.org wrote: > > From: Numan Siddique <numans@ovn.org> > > > > The patch [1], which added caching of lflow expr introduced a memory > > leak. The patch [1] also didn't take care of deleting the expr from the cache > > for the deleted lflows. This results in those lflow exprs in cache hanging in > > forever. This patch also addresses these 2 issues. > > > > [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> > > Hi Numan, > > Looks good to me. I also gave it a run on our scale testing setup and I > don't see the memleak anymore. > > Acked-by: Dumitru Ceara <dceara@redhat.com> Thanks Dumitru for the review and testing it out. I applied this to master and branch-20.03. Thanks Numan > > Thanks, > Dumitru > > > --- > > controller/lflow.c | 22 ++++++++++++++++++++++ > > 1 file changed, 22 insertions(+) > > > > diff --git a/controller/lflow.c b/controller/lflow.c > > index 780aa9331..79d89131a 100644 > > --- a/controller/lflow.c > > +++ b/controller/lflow.c > > @@ -402,6 +402,11 @@ lflow_handle_changed_flows(struct lflow_ctx_in *l_ctx_in, > > /* 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) { > > + lflow_expr_delete(l_ctx_out->lflow_expr_cache, le); > > + } > > } > > } > > > > @@ -660,6 +665,8 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow, > > expr = expr_normalize(expr); > > > > lflow_expr_add(l_ctx_out->lflow_expr_cache, lflow, expr); > > + } else { > > + expr_destroy(prereqs); > > } > > > > struct condition_aux cond_aux = { > > @@ -910,6 +917,21 @@ lflow_run(struct lflow_ctx_in *l_ctx_in, struct lflow_ctx_out *l_ctx_out) > > { > > COVERAGE_INC(lflow_run); > > > > + /* when lflow_run is called, it's possible that some of the logical flows > > + * are deleted. We need to delete the lflow expr cache for these lflows, > > + * otherwise, they will not be deleted at all. */ > > + const struct sbrec_logical_flow *lflow; > > + SBREC_LOGICAL_FLOW_TABLE_FOR_EACH_TRACKED (lflow, > > + l_ctx_in->logical_flow_table) { > > + if (sbrec_logical_flow_is_deleted(lflow)) { > > + struct lflow_expr *le = > > + lflow_expr_get(l_ctx_out->lflow_expr_cache, lflow); > > + if (le) { > > + lflow_expr_delete(l_ctx_out->lflow_expr_cache, le); > > + } > > + } > > + } > > + > > add_logical_flows(l_ctx_in, l_ctx_out); > > add_neighbor_flows(l_ctx_in->sbrec_port_binding_by_name, > > l_ctx_in->mac_binding_table, l_ctx_out->flow_table); > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/controller/lflow.c b/controller/lflow.c index 780aa9331..79d89131a 100644 --- a/controller/lflow.c +++ b/controller/lflow.c @@ -402,6 +402,11 @@ lflow_handle_changed_flows(struct lflow_ctx_in *l_ctx_in, /* 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) { + lflow_expr_delete(l_ctx_out->lflow_expr_cache, le); + } } } @@ -660,6 +665,8 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow, expr = expr_normalize(expr); lflow_expr_add(l_ctx_out->lflow_expr_cache, lflow, expr); + } else { + expr_destroy(prereqs); } struct condition_aux cond_aux = { @@ -910,6 +917,21 @@ lflow_run(struct lflow_ctx_in *l_ctx_in, struct lflow_ctx_out *l_ctx_out) { COVERAGE_INC(lflow_run); + /* when lflow_run is called, it's possible that some of the logical flows + * are deleted. We need to delete the lflow expr cache for these lflows, + * otherwise, they will not be deleted at all. */ + const struct sbrec_logical_flow *lflow; + SBREC_LOGICAL_FLOW_TABLE_FOR_EACH_TRACKED (lflow, + l_ctx_in->logical_flow_table) { + if (sbrec_logical_flow_is_deleted(lflow)) { + struct lflow_expr *le = + lflow_expr_get(l_ctx_out->lflow_expr_cache, lflow); + if (le) { + lflow_expr_delete(l_ctx_out->lflow_expr_cache, le); + } + } + } + add_logical_flows(l_ctx_in, l_ctx_out); add_neighbor_flows(l_ctx_in->sbrec_port_binding_by_name, l_ctx_in->mac_binding_table, l_ctx_out->flow_table);