On 8/10/23 09:59, Roi Dayan wrote: > Hi, > > We noticed a possible memory leak in netdev_tc_init_flow_api() > as we allocate memory for meter_police_ids with id_pool_create() > but we never destroy it.
Hi, Roi. I'd not consider this as a leak. The pool is allocated once and stored in a static memory always accessible. Leak is when we loose a reference to an allocated object, and that is not a case here. > > I added a code to refcount netdev_tc_init_flow_api and so in > netdev_tc_uninit_flow_api() to destroy it and noticed some behavior > about ovs-system. > > As soon as a user adds first bridge "bridge1" the system actually adds > ovs-system as port0 and bridge1 as port1. > > Now stopping ovs-vswitchd removes all bridges in order and ovs-system > first. but removing the bridge1 doesn't expilicit removes ovs-system > and stopping ovs-vswitchd also doesn't try to remove it. > Re-adding - removing bridge1 in a loop shows ovs-system being added all the > time just the code returns as the bridge already added but never removed. > > In this behavior on purpose? The ovs-system interface is a meta interface for a datapath, it doesn't represent any particular bridge or interface, so it should remain unless full datapath destruction is requested. > it doesn't seem consistent if we stop > ovs-vswitchd while bridges exists as then ovs-system being removed. Seems strange, if there are ports in the datapath, the ovs-system should remain. > > So to fix the memory leak beside the addition of the refcount when to > allocate meter pool and on last refcount to destroy it will need a fix for > ovs-system to be also removed when removing last user added bridge. > > If the behavior is on purpose for ovs-system then maybe skip allocating > meter ids pool for ovs-system and do it on first user bridge as it will > probably always exists as ovs-system being added when user adds first bridge. > then removing user bridge will reach last refcount and destroy > the meter pool ids. > > what do you think? I think, there is no actual need to free the pool. It is accessible, so not leaked. While it's good to free all the allocated memory on exit, it's OK to have some globally accessible heap-allocated memory not being freed. Especially, if freeing it is not that simple. I believe, there are a few other places where we do the same thing. > > Another thing I noticed is valgrind doesn't detect the memory leak of > meter_police_ids. Though adding another duplicate allocation with a new > name is cought by valgrind. Not sure why. I'd say valgrind doesn't complain because it is considered OK to have some globally accessible memory not freed on exit. Best regards, Ilya Maximets. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
