On Fri, Feb 28, 2020 at 4:11 PM Damijan Skvarc <[email protected]> wrote: > > Hi Mark > > and thanks you made review of suggested patch. > > just a few words of myself... > I don't work professionally on this project, I started to look into this > code in my free hours just to satisfy my curiosity about how other people > work. I am following a couple of other projects just to learn about > different processes, tools and to keep my brains in good condition while > studying some others work. And if you get some things/knowledge it is also > kind to give back something. In case of ovs/ovn I found a nice suite of > automatic tests,. Unfortunately, by running them under valgrind control the > apparent reports become useless because of flood of memory leaks. Mostly, > these are small non-important leaks, however in a flood of duplicated > reports you can miss some serious issue. And here I found my personal goal > what I can give back. Unfortunately, I don't have much knowledge about > networking, nor about original design. And if you don't have that knowledge > then from the experience I know it is better not to touch anything for what > you don't know why it has been designed for. So every my patch is done in a > way that nothing has been changed in a design, just memory leak in valgrind > reports disappears. > > > On Thu, Feb 27, 2020 at 10:24 PM Mark Michelson <[email protected]> wrote: > > > 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. > > > > > yes, I noticed that asymmetry. > > 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? > > > > What is strange also is that (If I see correctly) the ovnfield_by_name is a > hash of one single entry. > I wouldn't use hash at all in this case. But since I don't know about the > design, probably some entries > were deleted in the past and only one remained or probably there is a plan > some more entries will be > added in the future. So let's assume we should not get rid of > ovnfield_by_name hash. >
This is the commit which added 'ovnfield_by_name' [1]. I only added one field at the time. The commit message has more details. I think when working on it I missed out on the fact that ofctrl.c also calls ovn_init_symtab(). I think we can address this issue by having a new function - ovn_init_ovnfields() which lflow.c would call. ovn_destroy_ovnfields() is called by lflow.c anyway. What do you think about this ? [1] - https://github.com/openvswitch/ovs/commit/086470cdbe66522a1cfff96eb68073e4176684be Thank Numan > I found very similar functionality in meta_flow.c file (ovs). There > mf_by_name hash is more rational, it is filled with > plenty of strings. Also initialization of this hash is more rational. It is > initialized only if it is needed (initialization is hidden > in mf_from_name() and mf_from_name_len() functions and using the same > "pthread_once" approach as in my case). What is strange there is that there > is no destroy action for mf_by_name and valgrind does not report any leak > there. Probably test suite does not cover this functionality. > > I believe some more alternatives are available, however to reduce the > number of changes I would vote for > - automatic and optional initialization similar as in meta_flow case > - to solve asymmetry issue I would propose atexit() approach, similar as it > is implemented in ovs/lib/stopwatch.c > > btw, I didn't understand your first proposal. ovn_symbtab_init() function > need to be called twice since two different > symtab entities are initialized. Is this probably also a design issue that > we could live with single symtab? > > thanks, > damijan > > > > > > _______________________________________________ > 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
