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.
On Thu, Feb 27, 2020 at 10:24 PM Mark Michelson <[email protected]
<mailto:[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
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.
thanks,
damijan
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
