On Wed, Aug 17, 2022 at 12:58 AM Ales Musil <[email protected]> wrote: > > On Tue, Aug 16, 2022 at 1:50 PM Olaf Seibert via dev < > [email protected]> 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 <[email protected]> > > 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 <[email protected]> > > --- > > 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 <[email protected]> > > 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 <[email protected]> > > --- > > 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 <[email protected]> > > 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 <[email protected]> > > --- > > 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 > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > Hi, > > thank you for the patch. The variant 2 is the correct one to use IMO.
I agree with Ales. Thanks for finding the leak. Please let us know if you face any difficulties in submitting the formal patch. Numan > > Thanks, > Ales > > -- > > Ales Musil > > Senior Software Engineer - OVN Core > > Red Hat EMEA <https://www.redhat.com> > > [email protected] IM: amusil > <https://red.ht/sig> > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
