[ovs-dev] [PATCH v3 ovn 4/4] controller: binding: add memory accounting for local_lports and related_lports

[Dumitru Ceara]

On 10/14/21 1:01 PM, Lorenzo Bianconi wrote:
>> On 10/14/21 12:45 PM, Lorenzo Bianconi wrote:
>>>> On 9/25/21 12:19 AM, Lorenzo Bianconi wrote:
>>>>> Introduce memory accounting for:
>>>>> - binding local_lports map
>>>>> - binding related_lports map
>>>>>
>>>>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi at redhat.com>
>>>>> ---
>>>>>  controller/binding.c        | 59 +++++++++++++++++++++++++++++++++++++
>>>>>  controller/binding.h        |  5 ++++
>>>>>  controller/ovn-controller.c |  5 ++--
>>>>>  3 files changed, 67 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/controller/binding.c b/controller/binding.c
>>>>> index 661b4bb24..ec1e8bec7 100644
>>>>> --- a/controller/binding.c
>>>>> +++ b/controller/binding.c
>>>>> @@ -39,6 +39,31 @@
>>>>>  
>>>>>  VLOG_DEFINE_THIS_MODULE(binding);
>>>>>  
>>>>> +static uint64_t local_lports_usage;
>>>>> +static uint64_t related_lports_usage;
>>>>> +
>>>>> +static void
>>>>> +sset_mem_update(uint64_t *usage, struct sset *set,
>>>>> +                const char *name, bool erase)
>>>>> +{
>>>>> +    struct sset_node *node = sset_find(set, name);
>>>>> +
>>>>> +    if (!node && !erase) { /* add new element */
>>>>> +        *usage += (sizeof *node + strlen(name));
>>>>> +    } else if (node && erase) { /* remove an element */
>>>>> +        *usage -= (sizeof *node + strlen(name));
>>>>> +    }
>>>>> +}
>>>>> +
>>>>> +static void
>>>>> +sset_mem_clear(struct sset *set, uint64_t *usage)
>>>>> +{
>>>>> +    const char *name;
>>>>> +    SSET_FOR_EACH (name, set) {
>>>>> +        sset_mem_update(usage, set, name, true);
>>>>> +    }
>>>>> +}
>>>>> +
>>>>
>>>> I'm not too sure about this.  These functions are very generic.  I think
>>>> this should be part of the sset code itself.  Maybe a sset should just
>>>> automatically (or if configured) track all the memory it owns?  What do
>>>> you think?
>>>
>>> +Ilya
>>>
>>> I guess it would be a nice to have. sset are widely used but I do not think
>>> this patch will introduce a huge overhead.
>>>
>>> Regarding the specific patch we can remove it from the series in order to
>>> unlock it or we can have a local routines for the moment and then remove them
>>> when we have sset support. What do you think?
>>
>> I don't really expect the local_lports and related_lports ssets to grow
>> too big so I think we can probably wait with this patch until we have a
>> more generic solution, maybe directly in sset.
> 
> so we can just remove the patch from the series and repost it w/o 4/4
> 

Sounds good to me, thanks!