[ovs-dev] [PATCH ovn] logical-fields: fix memory leak caused by initialize ovnfield_by_name twice
damjan.skvarc at gmail.com
Thu Mar 5 06:33:28 UTC 2020
On Fri, Feb 28, 2020 at 9:50 PM Mark Michelson <mmichels at redhat.com> wrote:
> On 2/28/20 5:40 AM, Damijan Skvarc 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.
> Yep, and contributions like yours are very much appreciated. If my
> criticism of your contribution came across as harsh, I apologize.
Sorry about making you feel you need to apologize me. It was not my
intention but just an
excuse of myself that my contribution to the project is very limited both
in time an scope.
> > On Thu, Feb 27, 2020 at 10:24 PM Mark Michelson <mmichels at redhat.com
> > <mailto:mmichels at redhat.com>> 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
> > 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
> > 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
> > 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
> That is an interesting data point. My guess is that the lack of a
> destructor for mf_by_name is an oversight. I'm not sure why valgrind
> doesn't report the leak.
> > 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
> That's an excellent suggestion. That contains the allocation and
> destruction all within logical-fields.c without any other code having to
> know about its internals.
> > 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?
> That's what I was thinking. Since the symtabs are identical and are
> treated as read-only (I think...), we could just have lflow and ofctrl
> share the same symtab. But I like your suggestion above better.
I've sent v2 ( twice, because of forgotten ovn token, robot is complaining
More information about the dev