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. 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
