| Message ID | 1583389301-4259-1-git-send-email-damjan.skvarc@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v2,ovn] logical-fields: fix memory leak caused by initialize ovnfield_by_name twice | expand |
On Thu, Mar 05, 2020 at 07:21:41AM +0100, 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. Applied to master, thanks!
On Sat, Mar 7, 2020 at 4:42 AM Ben Pfaff <blp@ovn.org> wrote: > > On Thu, Mar 05, 2020 at 07:21:41AM +0100, 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. > > Applied to master, thanks! Hi Ben, Damijan, I applied this patch to branch-20.03 as well for the following reasons - This patch fixes memory leak - branch-20.03 compilation is broken when configured with the flag '--enable-sparse' Strangely, it used to compile for me last week. Even travis CI passed for the latest commit in branch-20.03. I may have updated my sparse version. I'm using sparse 0.6.1 now. Applying this patch resolves the compilation issue. I'm seeing the below compilation error ***** libtool: compile: env REAL_CC=gcc "CHECK=sparse -Wsparse-error -I /home/nusiddiq/workspace_cpp/ovn_fdp/ovn2.13/ovn/openvswitch-2.13.0/include/sparse -m64 -I /usr/local/include " cgcc -target=x86_64 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./ovn -I ./include -I /home/nusiddiq/workspace_cpp/ovn_fdp/ovn2.13/ovn/openvswitch-2.13.0/include -I /home/nusiddiq/workspace_cpp/ovn_fdp/ovn2.13/ovn/openvswitch-2.13.0/include -I /home/nusiddiq/workspace_cpp/ovn_fdp/ovn2.13/ovn/openvswitch-2.13.0/lib -I /home/nusiddiq/workspace_cpp/ovn_fdp/ovn2.13/ovn/openvswitch-2.13.0/lib -I /home/nusiddiq/workspace_cpp/ovn_fdp/ovn2.13/ovn/openvswitch-2.13.0 -I /home/nusiddiq/workspace_cpp/ovn_fdp/ovn2.13/ovn/openvswitch-2.13.0 -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wswitch-bool -Wlogical-not-parentheses -Wsizeof-array-argument -Wbool-compare -Wshift-negative-value -Wduplicated-cond -Wshadow -Wmultistatement-macros -Wcast-align=strict -Werror -Werror -g -O2 -MT lib/logical-fields.lo -MD -MP -MF lib/.deps/logical-fields.Tpo -c lib/logical-fields.c -o lib/logical-fields.o lib/logical-fields.c:294:1: error: symbol 'ovn_destroy_ovnfields' was not declared. Should it be static? make[1]: *** [Makefile:2070: lib/logical-fields.lo] Error 1 make[1]: *** Waiting for unfinished jobs.... *** Thanks Numan > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Tue, Mar 10, 2020 at 2:02 PM Numan Siddique <numans@ovn.org> wrote: > On Sat, Mar 7, 2020 at 4:42 AM Ben Pfaff <blp@ovn.org> wrote: > > > > On Thu, Mar 05, 2020 at 07:21:41AM +0100, 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. > > > > Applied to master, thanks! > > Hi Ben, Damijan, > > I applied this patch to branch-20.03 as well for the following reasons > - This patch fixes memory leak > - branch-20.03 compilation is broken when configured with the flag > '--enable-sparse' > Strangely, it used to compile for me last week. Even travis CI > passed for the latest commit in branch-20.03. > I may have updated my sparse version. I'm using sparse 0.6.1 now. > Applying this patch resolves the compilation issue. > > I'm seeing the below compilation error > ***** > libtool: compile: env REAL_CC=gcc "CHECK=sparse -Wsparse-error -I > > /home/nusiddiq/workspace_cpp/ovn_fdp/ovn2.13/ovn/openvswitch-2.13.0/include/sparse > -m64 -I /usr/local/include " cgcc -target=x86_64 -DHAVE_CONFIG_H -I. > -I ./include -I ./include -I ./ovn -I ./include -I > /home/nusiddiq/workspace_cpp/ovn_fdp/ovn2.13/ovn/openvswitch-2.13.0/include > -I > /home/nusiddiq/workspace_cpp/ovn_fdp/ovn2.13/ovn/openvswitch-2.13.0/include > -I /home/nusiddiq/workspace_cpp/ovn_fdp/ovn2.13/ovn/openvswitch-2.13.0/lib > -I /home/nusiddiq/workspace_cpp/ovn_fdp/ovn2.13/ovn/openvswitch-2.13.0/lib > -I /home/nusiddiq/workspace_cpp/ovn_fdp/ovn2.13/ovn/openvswitch-2.13.0 > -I /home/nusiddiq/workspace_cpp/ovn_fdp/ovn2.13/ovn/openvswitch-2.13.0 > -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare > -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum > -Wunused-parameter -Wbad-function-cast -Wcast-align > -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes > -Wmissing-field-initializers -fno-strict-aliasing -Wswitch-bool > -Wlogical-not-parentheses -Wsizeof-array-argument -Wbool-compare > -Wshift-negative-value -Wduplicated-cond -Wshadow > -Wmultistatement-macros -Wcast-align=strict -Werror -Werror -g -O2 -MT > lib/logical-fields.lo -MD -MP -MF lib/.deps/logical-fields.Tpo -c > lib/logical-fields.c -o lib/logical-fields.o > lib/logical-fields.c:294:1: error: symbol 'ovn_destroy_ovnfields' was > not declared. Should it be static? > hm, according to this message it is expected the ovn_destroy_ovnfields() should be static, but in fact it is static. In the past it used to be non-static function. Are you sure you are not dealing with some older object file artifact? make[1]: *** [Makefile:2070: lib/logical-fields.lo] Error 1 > make[1]: *** Waiting for unfinished jobs.... > *** > > Thanks > Numan > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > >
On Tue, Mar 10, 2020 at 7:10 PM Damijan Skvarc <damjan.skvarc@gmail.com> wrote: > > On Tue, Mar 10, 2020 at 2:02 PM Numan Siddique <numans@ovn.org> wrote: > > > On Sat, Mar 7, 2020 at 4:42 AM Ben Pfaff <blp@ovn.org> wrote: > > > > > > On Thu, Mar 05, 2020 at 07:21:41AM +0100, 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. > > > > > > Applied to master, thanks! > > > > Hi Ben, Damijan, > > > > I applied this patch to branch-20.03 as well for the following reasons > > - This patch fixes memory leak > > - branch-20.03 compilation is broken when configured with the flag > > '--enable-sparse' > > Strangely, it used to compile for me last week. Even travis CI > > passed for the latest commit in branch-20.03. > > I may have updated my sparse version. I'm using sparse 0.6.1 now. > > Applying this patch resolves the compilation issue. > > > > I'm seeing the below compilation error > > ***** > > libtool: compile: env REAL_CC=gcc "CHECK=sparse -Wsparse-error -I > > > > /home/nusiddiq/workspace_cpp/ovn_fdp/ovn2.13/ovn/openvswitch-2.13.0/include/sparse > > -m64 -I /usr/local/include " cgcc -target=x86_64 -DHAVE_CONFIG_H -I. > > -I ./include -I ./include -I ./ovn -I ./include -I > > /home/nusiddiq/workspace_cpp/ovn_fdp/ovn2.13/ovn/openvswitch-2.13.0/include > > -I > > /home/nusiddiq/workspace_cpp/ovn_fdp/ovn2.13/ovn/openvswitch-2.13.0/include > > -I /home/nusiddiq/workspace_cpp/ovn_fdp/ovn2.13/ovn/openvswitch-2.13.0/lib > > -I /home/nusiddiq/workspace_cpp/ovn_fdp/ovn2.13/ovn/openvswitch-2.13.0/lib > > -I /home/nusiddiq/workspace_cpp/ovn_fdp/ovn2.13/ovn/openvswitch-2.13.0 > > -I /home/nusiddiq/workspace_cpp/ovn_fdp/ovn2.13/ovn/openvswitch-2.13.0 > > -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare > > -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum > > -Wunused-parameter -Wbad-function-cast -Wcast-align > > -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes > > -Wmissing-field-initializers -fno-strict-aliasing -Wswitch-bool > > -Wlogical-not-parentheses -Wsizeof-array-argument -Wbool-compare > > -Wshift-negative-value -Wduplicated-cond -Wshadow > > -Wmultistatement-macros -Wcast-align=strict -Werror -Werror -g -O2 -MT > > lib/logical-fields.lo -MD -MP -MF lib/.deps/logical-fields.Tpo -c > > lib/logical-fields.c -o lib/logical-fields.o > > lib/logical-fields.c:294:1: error: symbol 'ovn_destroy_ovnfields' was > > not declared. Should it be static? > > > > hm, according to this message it is expected the ovn_destroy_ovnfields() > should be static, but in fact it is static. In the > past it used to be non-static function. Are you sure you are not dealing > with some older object file artifact? > Sorry for the confusion. I mean we see this compilation error without your patch. Without your patch, the function ovn_destroy_ovnfields() is in fact non-static and it is declared in include/ovn/logical-fields.h. But still sparse complains about this. Anyway, with your patch, the compilation is resolved because you've made ovn_destroy_ovnfields() static. And hence I backported your patch to branch-20.03. Thanks Numan > make[1]: *** [Makefile:2070: lib/logical-fields.lo] Error 1 > > make[1]: *** Waiting for unfinished jobs.... > > *** > > > > Thanks > > Numan > > > > > > > _______________________________________________ > > > dev mailing list > > > dev@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
thanks for sharing your information with me. Obviously I didn't understand you well. thanks, damijan On Tue, 10 Mar 2020, 16:51 Numan Siddique, <numans@ovn.org> wrote: > On Tue, Mar 10, 2020 at 7:10 PM Damijan Skvarc <damjan.skvarc@gmail.com> > wrote: > > > > On Tue, Mar 10, 2020 at 2:02 PM Numan Siddique <numans@ovn.org> wrote: > > > > > On Sat, Mar 7, 2020 at 4:42 AM Ben Pfaff <blp@ovn.org> wrote: > > > > > > > > On Thu, Mar 05, 2020 at 07:21:41AM +0100, 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. > > > > > > > > Applied to master, thanks! > > > > > > Hi Ben, Damijan, > > > > > > I applied this patch to branch-20.03 as well for the following reasons > > > - This patch fixes memory leak > > > - branch-20.03 compilation is broken when configured with the flag > > > '--enable-sparse' > > > Strangely, it used to compile for me last week. Even travis CI > > > passed for the latest commit in branch-20.03. > > > I may have updated my sparse version. I'm using sparse 0.6.1 now. > > > Applying this patch resolves the compilation issue. > > > > > > I'm seeing the below compilation error > > > ***** > > > libtool: compile: env REAL_CC=gcc "CHECK=sparse -Wsparse-error -I > > > > > > > /home/nusiddiq/workspace_cpp/ovn_fdp/ovn2.13/ovn/openvswitch-2.13.0/include/sparse > > > -m64 -I /usr/local/include " cgcc -target=x86_64 -DHAVE_CONFIG_H -I. > > > -I ./include -I ./include -I ./ovn -I ./include -I > > > > /home/nusiddiq/workspace_cpp/ovn_fdp/ovn2.13/ovn/openvswitch-2.13.0/include > > > -I > > > > /home/nusiddiq/workspace_cpp/ovn_fdp/ovn2.13/ovn/openvswitch-2.13.0/include > > > -I > /home/nusiddiq/workspace_cpp/ovn_fdp/ovn2.13/ovn/openvswitch-2.13.0/lib > > > -I > /home/nusiddiq/workspace_cpp/ovn_fdp/ovn2.13/ovn/openvswitch-2.13.0/lib > > > -I /home/nusiddiq/workspace_cpp/ovn_fdp/ovn2.13/ovn/openvswitch-2.13.0 > > > -I /home/nusiddiq/workspace_cpp/ovn_fdp/ovn2.13/ovn/openvswitch-2.13.0 > > > -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare > > > -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum > > > -Wunused-parameter -Wbad-function-cast -Wcast-align > > > -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes > > > -Wmissing-field-initializers -fno-strict-aliasing -Wswitch-bool > > > -Wlogical-not-parentheses -Wsizeof-array-argument -Wbool-compare > > > -Wshift-negative-value -Wduplicated-cond -Wshadow > > > -Wmultistatement-macros -Wcast-align=strict -Werror -Werror -g -O2 -MT > > > lib/logical-fields.lo -MD -MP -MF lib/.deps/logical-fields.Tpo -c > > > lib/logical-fields.c -o lib/logical-fields.o > > > lib/logical-fields.c:294:1: error: symbol 'ovn_destroy_ovnfields' was > > > not declared. Should it be static? > > > > > > > hm, according to this message it is expected the ovn_destroy_ovnfields() > > should be static, but in fact it is static. In the > > past it used to be non-static function. Are you sure you are not dealing > > with some older object file artifact? > > > > Sorry for the confusion. I mean we see this compilation error without > your patch. > Without your patch, the function ovn_destroy_ovnfields() is in fact > non-static and > it is declared in include/ovn/logical-fields.h. But still sparse > complains about this. > > Anyway, with your patch, the compilation is resolved because you've made > ovn_destroy_ovnfields() static. > > And hence I backported your patch to branch-20.03. > > Thanks > Numan > > > make[1]: *** [Makefile:2070: lib/logical-fields.lo] Error 1 > > > make[1]: *** Waiting for unfinished jobs.... > > > *** > > > > > > Thanks > > > Numan > > > > > > > > > > _______________________________________________ > > > > dev mailing list > > > > dev@openvswitch.org > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > >
diff --git a/controller/lflow.c b/controller/lflow.c index ee11fc6..143ea3c 100644 --- a/controller/lflow.c +++ b/controller/lflow.c @@ -827,7 +827,7 @@ lflow_handle_changed_neighbors( } } - + /* Translates logical flows in the Logical_Flow table in the OVN_SB database * into OpenFlow flows. See ovn-architecture(7) for more information. */ void @@ -846,5 +846,4 @@ lflow_destroy(void) { expr_symtab_destroy(&symtab); shash_destroy(&symtab); - ovn_destroy_ovnfields(); } diff --git a/include/ovn/logical-fields.h b/include/ovn/logical-fields.h index 9b7c34f..c7bd2db 100644 --- a/include/ovn/logical-fields.h +++ b/include/ovn/logical-fields.h @@ -130,5 +130,4 @@ ovn_field_from_id(enum ovn_field_id id) const char *event_to_string(enum ovn_controller_event event); int string_to_event(const char *s); const struct ovn_field *ovn_field_from_name(const char *name); -void ovn_destroy_ovnfields(void); #endif /* ovn/lib/logical-fields.h */ diff --git a/lib/logical-fields.c b/lib/logical-fields.c index 25ace58..a007085 100644 --- a/lib/logical-fields.c +++ b/lib/logical-fields.c @@ -254,12 +254,6 @@ 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); - } expr_symtab_add_ovn_field(symtab, "icmp4.frag_mtu", OVN_ICMP4_FRAG_MTU); } @@ -284,14 +278,35 @@ string_to_event(const char *s) return -1; } -const struct ovn_field * -ovn_field_from_name(const char *name) +static void +ovn_destroy_ovnfields(void) { - return shash_find_data(&ovnfield_by_name, name); + shash_destroy(&ovnfield_by_name); } -void -ovn_destroy_ovnfields(void) +static void +ovn_do_init_ovnfields(void) { - shash_destroy(&ovnfield_by_name); + 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); + } + atexit(ovn_destroy_ovnfields); +} + +static void +ovn_init_ovnfields(void) +{ + static pthread_once_t once = PTHREAD_ONCE_INIT; + pthread_once(&once, ovn_do_init_ovnfields); +} + +const struct ovn_field * +ovn_field_from_name(const char *name) +{ + ovn_init_ovnfields(); + + return shash_find_data(&ovnfield_by_name, name); }
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 and using design pattern from stopwatch.c or meta_flow.c files (ovs). Problem was reported by valgrind with flood of messages (190) while executing ovn test suite: ==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 <damjan.skvarc@gmail.com> --- controller/lflow.c | 3 +-- include/ovn/logical-fields.h | 1 - lib/logical-fields.c | 39 +++++++++++++++++++++++++++------------ 3 files changed, 28 insertions(+), 15 deletions(-)