On Tue, Aug 16, 2022 at 1:50 PM Olaf Seibert via dev <
ovs-dev@openvswitch.org> wrote:
> Hi! We discovered a memory leak in ovn-northd, which grows quickly, caused
> by
> commit 7b56f69580e1f390d9c6753a2cb8f0dbfbb4c467 (dated Wed Feb 23 09:30:00
> 2022
> +0000). We also found a fix for it. I am however not familiar with this
> code,
> so when I looked a bit further, I wondered if a different variant would be
> better. So here are three variants for consideration.
>
> Here is the first variant which is the simplest change, and the one we
> actually
> tested:
>
> From 491008b30fd2e7e09cc4cd3aab1d1a0a3f86dca7 Mon Sep 17 00:00:00 2001
> From: Olaf Seibert <o.seib...@syseleven.de>
> Date: Fri, 12 Aug 2022 11:15:47 +0200
> Subject: [PATCH ovn] Fix memory leak.
>
> Commit 7b56f69580e1f390d9c6753a2cb8f0dbfbb4c467 populated ids with
> names, but didn't clean them up. Do that here.
>
> Signed-off-by: Olaf Seibert <o.seib...@syseleven.de>
> ---
> northd/northd.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 09eccf4d9..e7beed8fb 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -3438,6 +3438,7 @@ ovn_port_update_sbrec(struct northd_input
> *input_data,
> struct smap ids = SMAP_INITIALIZER(&ids);
> smap_clone(&ids, &op->nbrp->external_ids);
> sbrec_port_binding_set_external_ids(op->sb, &ids);
> + smap_destroy(&ids);
>
> sbrec_port_binding_set_nat_addresses(op->sb, NULL, 0);
> } else {
> --
> 2.37.2
>
> I realised that the `ids` variable could possibly be removed, since these
> external_ids are in effect just passed on unmodified to
> sbrec_port_binding_set_external_ids(). But since I didn't find the source
> to
> that, I am not totally sure that this is safe to do.
>
> So variant 2 uses that shortcut:
>
> From 6472d24678d3af1d622bacf7da1c9ba77e058b10 Mon Sep 17 00:00:00 2001
> From: Olaf Seibert <o.seib...@syseleven.de>
> Date: Fri, 12 Aug 2022 11:17:12 +0200
> Subject: [PATCH ovn] Fix memory leak.
>
> Commit 7b56f69580e1f390d9c6753a2cb8f0dbfbb4c467 cloned the external_ids
> to ids, then passed them to sbrec_port_binding_set_external_ids().
> The intermediate step is unneeded (and caused a memory leak).
> Pass the external_ids directly.
>
> Signed-off-by: Olaf Seibert <o.seib...@syseleven.de>
> ---
> northd/northd.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 09eccf4d9..309b449dd 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -3435,9 +3435,7 @@ ovn_port_update_sbrec(struct northd_input
> *input_data,
> sbrec_port_binding_set_mac(op->sb, &addresses, 1);
> ds_destroy(&s);
>
> - struct smap ids = SMAP_INITIALIZER(&ids);
> - smap_clone(&ids, &op->nbrp->external_ids);
> - sbrec_port_binding_set_external_ids(op->sb, &ids);
> + sbrec_port_binding_set_external_ids(op->sb,
> &op->nbrp->external_ids);
>
> sbrec_port_binding_set_nat_addresses(op->sb, NULL, 0);
> } else {
> --
> 2.37.2
>
> Then, further down in northd.c, there is another use of
> sbrec_port_binding_set_nat_addresses(). But in that location, there is a
> manipulation of the names. Maybe the same is needed here. On the other
> hand,
> there the source of the ids is op->nbsp instead of op->nbrp here.
> So I'm not sure if it has to be the same.
>
> Variant 3:
>
> From 4a61a02009eb450c6dfadd156968023833e4f1a6 Mon Sep 17 00:00:00 2001
> From: Olaf Seibert <o.seib...@syseleven.de>
> Date: Fri, 12 Aug 2022 11:33:23 +0200
> Subject: [PATCH ovn] Fix a memory leak.
>
> Commit 7b56f69580e1f390d9c6753a2cb8f0dbfbb4c467 added another place
> where external_ids are copied to the SB database. This had a memory
> leak, and also differed from the other place where this happens.
> Make this case the same as the other one.
>
> Signed-off-by: Olaf Seibert <o.seib...@syseleven.de>
> ---
> northd/northd.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 09eccf4d9..4d34f7ebc 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -3437,7 +3437,12 @@ ovn_port_update_sbrec(struct northd_input
> *input_data,
>
> struct smap ids = SMAP_INITIALIZER(&ids);
> smap_clone(&ids, &op->nbrp->external_ids);
> + const char *name = smap_get(&ids, "neutron:port_name");
> + if (name && name[0]) {
> + smap_add(&ids, "name", name);
> + }
> sbrec_port_binding_set_external_ids(op->sb, &ids);
> + smap_destroy(&ids);
>
> sbrec_port_binding_set_nat_addresses(op->sb, NULL, 0);
> } else {
> --
> 2.37.2
>
>
> So which variant would be preferred? It looks like variant #3 might be
> needed,
> due to the "name" name. If not, #2 would be the shortest version. If that
> one
> is inappropriate, variant #1 would be it.
>
> Cheers,
> -Olaf Seibert
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Hi,
thank you for the patch. The variant 2 is the correct one to use IMO.
Thanks,
Ales
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA <https://www.redhat.com>
amu...@redhat.com IM: amusil
<https://red.ht/sig>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
