>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.skv...@gmail.com>
---
lib/logical-fields.c | 24 ++++++++++++++++++------
1 file changed, 18 insertions(+), 6 deletions(-)
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);
}
--
2.7.4
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
