Hi Damijan,
Thanks for finding the memory leak and patching it. To me,
ovnfield_by_name is a really strangely handled structure. There is an
explicit function to destroy the ovnfield_by_name structure, but its
creation is a side effect from ovn_symtab_init. The result is that
lflow.c calls ovn_symtab_init() and ovn_destroy_ovnfields(). But ofctrl
only calls ovn_symtab_init(). It doesn't call ovn_destroy_ovnfields()
because it would result in double-freeing ovnfield_by_name.
This lack of symmetry shows a poor design. Your patch fixes the memory
leak, but it doesn't fix the lack of symmetry, and it still allows for
potential double-freeing.
I have a couple of suggestions for a better design:
1) Instead of calling ovn_symbtab_init() in multiple places, call it
once in ovn_controller's main() function. Then when ovn_controller
exits, call ovn_destroy_ovnfields(). Then, pass the created symtab as a
parameter when it is necessary to. This works because the symtabs
created by lflow.c and ofctrl.c are identical, and their use of the
symtabs occurs in the same thread.
2) Initialize ovnfield_by_name in a separate function from
ovn_symtab_init(). You can call this new ovnfield_by_name_init()
function in ovn-controller's main function and then call the already
existing ovn_destroy_ovnfields() function when ovn-controller exits.
This works because ovnfield_by_name does not rely on any data from the
created symtab. It exists completely independently.
What do you think about this?
On 2/27/20 8:17 AM, Damijan Skvarc wrote:
ovnfield_by_name is hash of strings which is used to quickly find
field by name. This hash is initialized from ovn_init_symtab(). In case
the latter function is called multiple times then also ovnfield_by_name is
initialized multiple times but without freeing previously allocated
memory resources what cause memory leaks. This actually happens in
ovn-controller which calls ovn_init_symtab() function twice, once from
ofctrl.c and the other time from lflow.c files.
Problem was solved by initializing ovnfield_by_name entity only once
with the help of ovsthread_once_XXXX functionality.
Problem was reported by valgrind with flood of messages similar to this one:
==5999== 47 (32 direct, 15 indirect) bytes in 1 blocks are definitely lost in
loss record 86 of 102
==5999== at 0x4C2DB8F: malloc (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5999== by 0x50635D: xmalloc (util.c:138)
==5999== by 0x4F6513: shash_add_nocopy__ (shash.c:109)
==5999== by 0x4F6585: shash_add_nocopy (shash.c:121)
==5999== by 0x4F65BD: shash_add (shash.c:129)
==5999== by 0x4F6602: shash_add_once (shash.c:136)
==5999== by 0x4395B7: ovn_init_symtab (logical-fields.c:261)
==5999== by 0x406C91: main (ovn-controller.c:1750)
Signed-off-by: Damijan Skvarc <[email protected]>
---
lib/logical-fields.c | 25 +++++++++++++++++++------
1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/lib/logical-fields.c b/lib/logical-fields.c
index 25ace58..569e31e 100644
--- a/lib/logical-fields.c
+++ b/lib/logical-fields.c
@@ -254,12 +254,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);
}
@@ -295,3 +291,20 @@ ovn_destroy_ovnfields(void)
{
shash_destroy(&ovnfield_by_name);
}
+
+void
+ovn_init_ovnfields(void)
+{
+ static struct ovsthread_once field_by_name_once =
+ OVSTHREAD_ONCE_INITIALIZER;
+
+ if (ovsthread_once_start(&field_by_name_once)) {
+ 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);
+ }
+ ovsthread_once_done(&field_by_name_once);
+ }
+}
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
