| Message ID | 1600195226-123272-2-git-send-email-hzhou@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,v2,1/2] lflow.c: Avoid adding redundant resource refs for port-bindings. | expand |
On 9/15/20 8:40 PM, Han Zhou wrote: > If a resource doesn't have any lflows referencing it any more, the > node ref_lflow_node in lflow_resource_ref.ref_lflow_table should > be removed and released. Otherwise, the table could keep growing > in some scenarios, until a recompute is triggered. Now that the > chance of triggering recompute is lower and there are more resources > references maintained (for type port-binding), this problem is > more likely to happen than before. This patch fixes the problem > by releasing the node as soon as it is not needed. > > Fixes: d2aa2c7cafe ("ovn-controller: Maintain resource references for logical flows.") > Signed-off-by: Han Zhou <hzhou@ovn.org> Hi Han, > --- > controller/lflow.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/controller/lflow.c b/controller/lflow.c > index 86a5b76..9d54c73 100644 > --- a/controller/lflow.c > +++ b/controller/lflow.c > @@ -292,6 +292,10 @@ lflow_resource_destroy_lflow(struct lflow_resource_ref *lfrr, > LIST_FOR_EACH_SAFE (lrln, next, list_node, &lfrn->lflow_ref_head) { > ovs_list_remove(&lrln->list_node); > hmap_remove(&lrln->rlfn->lflow_uuids, &lrln->hmap_node); > + if (hmap_is_empty(&lrln->rlfn->lflow_uuids)) { I think part of the commit log message should also appear above this line as a comment explaining why we have the additional check. > + hmap_remove(&lfrr->ref_lflow_table, &lrln->rlfn->node); > + ref_lflow_node_destroy(lrln->rlfn); As mentioned on v1 (at least for me) it's really hard to keep track of what's going on due to the variable/field names, e.g., "lrln->rlfn". I'd really like it if we had somewhat more explicit names. Thanks, Dumitru > + } > free(lrln); > } > free(lfrn); >
On 9/16/20 2:18 PM, Dumitru Ceara wrote: [...] > >> + hmap_remove(&lfrr->ref_lflow_table, &lrln->rlfn->node); >> + ref_lflow_node_destroy(lrln->rlfn); > > As mentioned on v1 (at least for me) it's really hard to keep track of > what's going on due to the variable/field names, e.g., "lrln->rlfn". I'd > really like it if we had somewhat more explicit names. > This can be done as a follow up patch. I guess the goal is to fix the crash as soon as possible. Refactoring should come afterwards. Thanks, Dumitru
On Wed, Sep 16, 2020 at 10:51 AM Dumitru Ceara <dceara@redhat.com> wrote: > > On 9/16/20 2:18 PM, Dumitru Ceara wrote: > > [...] > > > > >> + hmap_remove(&lfrr->ref_lflow_table, &lrln->rlfn->node); > >> + ref_lflow_node_destroy(lrln->rlfn); > > > > As mentioned on v1 (at least for me) it's really hard to keep track of > > what's going on due to the variable/field names, e.g., "lrln->rlfn". I'd > > really like it if we had somewhat more explicit names. > > > > This can be done as a follow up patch. I guess the goal is to fix the > crash as soon as possible. Refactoring should come afterwards. > Agree. For the names, what do you suggest? Here rlfn means ref_lflow_node, lrln means lflow_ref_list_node. It is common to use short abbreviations for variables with local scope, but maybe the rules are not clear enough. Maybe lrln can be lfrln? Or maybe the name ref_lflow_node itself is confusing? 'ref' represents a resource which is identified by ref_type + ref_name. Let me know your suggestion. For the comment, I hope my explains in 1/2 helped understanding it. The reason why the check was needed is basically what the code is telling: when the lflow_uuids is empty (no lflows referencing this resource). > Thanks, > Dumitru >
On 9/16/20 8:36 PM, Han Zhou wrote: > > > On Wed, Sep 16, 2020 at 10:51 AM Dumitru Ceara <dceara@redhat.com > <mailto:dceara@redhat.com>> wrote: >> >> On 9/16/20 2:18 PM, Dumitru Ceara wrote: >> >> [...] >> >> > >> >> + hmap_remove(&lfrr->ref_lflow_table, &lrln->rlfn->node); >> >> + ref_lflow_node_destroy(lrln->rlfn); >> > >> > As mentioned on v1 (at least for me) it's really hard to keep track of >> > what's going on due to the variable/field names, e.g., "lrln->rlfn". I'd >> > really like it if we had somewhat more explicit names. >> > >> >> This can be done as a follow up patch. I guess the goal is to fix the >> crash as soon as possible. Refactoring should come afterwards. >> > Agree. For the names, what do you suggest? Here rlfn means > ref_lflow_node, lrln means lflow_ref_list_node. It is common to use > short abbreviations for variables with local scope, but maybe the rules > are not clear enough. Maybe lrln can be lfrln? Or maybe the name > ref_lflow_node itself is confusing? 'ref' represents a resource which is > identified by ref_type + ref_name. Let me know your suggestion. > I'm not sure to be honest. I understand the names but I still have a problem thinking about what they refer to when I look at statements using the field names. I know this is very subjective but I think it's because of all the common letters between "lrln" and "lfrln", for example. > For the comment, I hope my explains in 1/2 helped understanding it. The > reason why the check was needed is basically what the code is telling: > when the lflow_uuids is empty (no lflows referencing this resource). > Right that's clear now, thanks!
diff --git a/controller/lflow.c b/controller/lflow.c index 86a5b76..9d54c73 100644 --- a/controller/lflow.c +++ b/controller/lflow.c @@ -292,6 +292,10 @@ lflow_resource_destroy_lflow(struct lflow_resource_ref *lfrr, LIST_FOR_EACH_SAFE (lrln, next, list_node, &lfrn->lflow_ref_head) { ovs_list_remove(&lrln->list_node); hmap_remove(&lrln->rlfn->lflow_uuids, &lrln->hmap_node); + if (hmap_is_empty(&lrln->rlfn->lflow_uuids)) { + hmap_remove(&lfrr->ref_lflow_table, &lrln->rlfn->node); + ref_lflow_node_destroy(lrln->rlfn); + } free(lrln); } free(lfrn);
If a resource doesn't have any lflows referencing it any more, the node ref_lflow_node in lflow_resource_ref.ref_lflow_table should be removed and released. Otherwise, the table could keep growing in some scenarios, until a recompute is triggered. Now that the chance of triggering recompute is lower and there are more resources references maintained (for type port-binding), this problem is more likely to happen than before. This patch fixes the problem by releasing the node as soon as it is not needed. Fixes: d2aa2c7cafe ("ovn-controller: Maintain resource references for logical flows.") Signed-off-by: Han Zhou <hzhou@ovn.org> --- controller/lflow.c | 4 ++++ 1 file changed, 4 insertions(+)