| Message ID | 1600279283-7597-2-git-send-email-hzhou@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v3,1/2] lflow.c: Avoid adding redundant resource refs for port-bindings. | expand |
On 9/16/20 8:01 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> > --- > controller/lflow.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/controller/lflow.c b/controller/lflow.c > index db078d2..b549067 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); I still think a short comment would be useful here, e.g.: /* Resources that are not referred by any logical flows would be * cleaned up in any case during a recompute so it's better to * remove them early. */ > + 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); > In any case, Acked-by: Dumitru Ceara <dceara@redhat.com> Thanks, Dumitru
On Thu, Sep 17, 2020 at 1:15 PM Dumitru Ceara <dceara@redhat.com> wrote: > On 9/16/20 8:01 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> > > --- > > controller/lflow.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/controller/lflow.c b/controller/lflow.c > > index db078d2..b549067 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); > > I still think a short comment would be useful here, e.g.: > > /* Resources that are not referred by any logical flows would be > * cleaned up in any case during a recompute so it's better to > * remove them early. > */ > > > + 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); > > > > In any case, > > Acked-by: Dumitru Ceara <dceara@redhat.com> > Acked-by: Numan Siddique <numans@ovn.org> Thanks Numan > > Thanks, > Dumitru > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On Thu, Sep 17, 2020 at 5:45 AM Numan Siddique <numans@ovn.org> wrote: > > > > On Thu, Sep 17, 2020 at 1:15 PM Dumitru Ceara <dceara@redhat.com> wrote: >> >> On 9/16/20 8:01 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> >> > --- >> > controller/lflow.c | 4 ++++ >> > 1 file changed, 4 insertions(+) >> > >> > diff --git a/controller/lflow.c b/controller/lflow.c >> > index db078d2..b549067 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); >> >> I still think a short comment would be useful here, e.g.: >> >> /* Resources that are not referred by any logical flows would be >> * cleaned up in any case during a recompute so it's better to >> * remove them early. >> */ >> >> > + 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); >> > >> >> In any case, >> >> Acked-by: Dumitru Ceara <dceara@redhat.com> > > > Acked-by: Numan Siddique <numans@ovn.org> > Thank you both. I applied to master, branch 20.09 and 20.06, with a comment at the place suggested by Dumitru: + /* Clean up the node in ref_lflow_table if the resource is not + * referred by any logical flows. */ Just to explain a little bit here: I didn't add the exact statement from Dumitru because the reason why it is cleaned here is not because recompute will clean it anyway. Instead, it is cleaned because it is natural to clean it when it is not needed (the node was created on-demand, so should be released when not needed). A consequence of not doing it is that there is a chance the recompute does not happen before this table grows unreasonably large, which is bad. Thanks, Han
diff --git a/controller/lflow.c b/controller/lflow.c index db078d2..b549067 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(+)