[ovs-dev] [PATCH ovn v3 2/2] lflow.c: Release ref_lflow_node as soon as it is not needed.

[Han Zhou]

On Thu, Sep 17, 2020 at 5:45 AM Numan Siddique <numans at ovn.org> wrote:
>
>
>
> On Thu, Sep 17, 2020 at 1:15 PM Dumitru Ceara <dceara at 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 at 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 at redhat.com>
>
>
> Acked-by: Numan Siddique <numans at 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