On 10/08/2023 15:26, Ilya Maximets wrote:
> 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.
thanks for all the info and your thoughts.
ovs-system is there while there are ports but not being called ops on removal.
let me give an example by command.
$ service openvswitch start
$ ovs-vsctl add-br ov1
I see ovs-system being added and then ov1.
$ service openvswitch stop
I see ovs-system being removed and then ov1.
now second example starting clean.
$ service openvswitch start
$ ovs-vsctl add-br ov1
I see ovs-system being added and then ov1.
$ ovs-vsctl del-br ov1
I see ov1 being removed but not ovs-system.
$ service openvswitch stop
nothing here as well. ovs-system was not removed the same way as before.
netdev_ports_insert() was called to add ovs-system but netdev_ports_remove()
is not being called to remove ovs-system when I removed ov1.
>
>>
>> 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.
about valgrind. it did complain on new allocation i added to check it.
i added global var
static struct id_pool *meter_police_ids OVS_GUARDED_BY(meter_police_ids_mutex);
+static struct id_pool *tmp_data OVS_GUARDED_BY(meter_police_ids_mutex);
added allocation
+tmp_data = id_pool_create(METER_POLICE_IDS_BASE,
+METER_POLICE_IDS_MAX - METER_POLICE_IDS_BASE + 1);
meter_police_ids = id_pool_create(METER_POLICE_IDS_BASE,
METER_POLICE_IDS_MAX - METER_POLICE_IDS_BASE + 1);
tc_cleanup_policer_actions(meter_police_ids, METER_POLICE_IDS_BASE,
METER_POLICE_IDS_MAX);
and valgrind did complain on tmp_data allocation as a memory leak.
any idea what is the difference?
my test is only add bridge and del bridge to get to the initialization.
>
> Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
