diff mbox series

[ovs-dev,1/1,v3,ovn] logical-fields: fix memory leak while initializing ovnfield_by_name

Message ID 1566297055-13248-1-git-send-email-damjan.skvarc@gmail.com
State Superseded
Headers show
Series [ovs-dev,1/1,v3,ovn] logical-fields: fix memory leak while initializing ovnfield_by_name | expand

Commit Message

Damijan Skvarc Aug. 20, 2019, 10:30 a.m. UTC 
>This patch named "fix memory leak ...", but I don't see any leaks fixed
>in the code. It seems that you've sent diff between v1 and v2 instead of
>the patch itself. If so, you need to squash this into your v1 patch and
>send as v3.
>
>Best regards, Ilya Maximets.

Yes, Ilya, you are right. I didn't realize patch file must contain cumulative
changes (and not partial one) in case they are spread through several commits.

In our case the memory leak occurs because ovnfield_by_name hash is initialized
twice without destroying previously allocated memory. 

I know the problem can be solved in different ways (ovsthread_once, OVS_CONSTRUCTOR,
calling ovn_destroy_ovnfields() before reinitializing  it again,...). However my
intention in this patch is just to solve memory leak and not changing anything in 
the behaviour. I am also aware that memory leak is not important since
it happens only once during lifecycle of ovn-controller. Thus the one, who will get 
the most from this patch is the observer of valgrind reports while running test suite. 
Namely, while running test suite ovn-controller is restarted so many times and this 
memory leak is reported so many times that it obscure much more important issues.

If anything else needs to be done regarding initialization of ovnfield_by_name hash I 
would reather think about getting rid of it, since it maps into ovn_fields array which
is currently contituted from one entry only. I believe searching for field name in this
single entry table directly would be even quicker then using hash. But since I don't 
know about the future/plans of ovn_fields array I am not able to do this change.


Signed-off-by: Damijan Skvarc <damjan.skvarc@gmail.com>
---
 lib/logical-fields.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)
diff mbox series

Patch

diff --git a/lib/logical-fields.c b/lib/logical-fields.c
index 8fb591c..13e5b83 100644
--- a/lib/logical-fields.c
+++ b/lib/logical-fields.c
@@ -57,6 +57,22 @@  add_ct_bit(const char *name, int index, struct shash *symtab)
     free(expansion);
 }
 
+static void
+ovn_init_ovnfields(void)
+{
+    static bool initialized = false;
+
+    if (!initialized) {
+        shash_init(&ovnfield_by_name);
+        for (int i = 0; i < OVN_FIELD_N_IDS; i++) {
+            const struct ovn_field *of = &ovn_fields[i];
+            ovs_assert(of->id == i); /* Fields must be in the enum order. */
+            shash_add_once(&ovnfield_by_name, of->name, of);
+        }
+        initialized = true;
+    }
+}
+
 void
 ovn_init_symtab(struct shash *symtab)
 {
@@ -220,12 +236,8 @@  ovn_init_symtab(struct shash *symtab)
     expr_symtab_add_field(symtab, "sctp.src", MFF_SCTP_SRC, "sctp", false);
     expr_symtab_add_field(symtab, "sctp.dst", MFF_SCTP_DST, "sctp", false);
 
-    shash_init(&ovnfield_by_name);
-    for (int i = 0; i < OVN_FIELD_N_IDS; i++) {
-        const struct ovn_field *of = &ovn_fields[i];
-        ovs_assert(of->id == i); /* Fields must be in the enum order. */
-        shash_add_once(&ovnfield_by_name, of->name, of);
-    }
+    ovn_init_ovnfields();
+
     expr_symtab_add_ovn_field(symtab, "icmp4.frag_mtu", OVN_ICMP4_FRAG_MTU);
 }