[ovs-dev] [PATCH v4 ovn 3/3] controller: add memory accounting for local_datapath

[Dumitru Ceara]

On 10/14/21 5:17 PM, Lorenzo Bianconi wrote:
> Similar to if-status-mgr, track memory allocated for local_datapath.
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi at redhat.com>
> ---
>  controller/binding.c        |  6 ++--
>  controller/local_data.c     | 66 +++++++++++++++++++++++++++++++++++++
>  controller/local_data.h     |  7 ++++
>  controller/ovn-controller.c |  1 +
>  4 files changed, 77 insertions(+), 3 deletions(-)
> 
> diff --git a/controller/binding.c b/controller/binding.c
> index c037b2352..329d0d12b 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -374,8 +374,8 @@ update_ld_external_ports(const struct sbrec_port_binding *binding_rec,
>      struct local_datapath *ld = get_local_datapath(
>          local_datapaths, binding_rec->datapath->tunnel_key);
>      if (ld) {
> -        shash_replace(&ld->external_ports, binding_rec->logical_port,
> -                      binding_rec);
> +        add_local_datapath_external_port(ld, binding_rec->logical_port,
> +                                         binding_rec);
>      }
>  }
>  
> @@ -1756,7 +1756,7 @@ remove_pb_from_local_datapath(const struct sbrec_port_binding *pb,
>              ld->localnet_port = NULL;
>          }
>      } else if (!strcmp(pb->type, "external")) {
> -        shash_find_and_delete(&ld->external_ports, pb->logical_port);
> +        remove_local_datapath_external_port(ld, pb->logical_port);
>      }
>  }
>  
> diff --git a/controller/local_data.c b/controller/local_data.c
> index 6efed2de0..48e6958b7 100644
> --- a/controller/local_data.c
> +++ b/controller/local_data.c
> @@ -47,6 +47,30 @@ static struct tracked_datapath *tracked_datapath_create(
>      enum en_tracked_resource_type tracked_type,
>      struct hmap *tracked_datapaths);
>  
> +static uint64_t local_datapath_usage;
> +
> +static void
> +local_datapath_peer_ports_mem_add(struct local_datapath *ld,
> +                                  size_t n_peer_ports)
> +{
> +    /* memory accounting - peer_ports. */
> +    local_datapath_usage +=
> +        (ld->n_allocated_peer_ports - n_peer_ports) * sizeof *ld->peer_ports;
> +}
> +
> +static void
> +local_datapath_ext_port_mem_update(struct local_datapath *ld, const char *name,
> +                                   bool erase)
> +{
> +    struct shash_node *node = shash_find(&ld->external_ports, name);
> +
> +    if (!node && !erase) { /* add a new element */
> +        local_datapath_usage += (sizeof *node + strlen(name));
> +    } else if (node && erase) { /* remove an element */
> +        local_datapath_usage -= (sizeof *node + strlen(name));
> +    }
> +}
> +
>  struct local_datapath *
>  get_local_datapath(const struct hmap *local_datapaths, uint32_t tunnel_key)
>  {
> @@ -63,6 +87,9 @@ local_datapath_alloc(const struct sbrec_datapath_binding *dp)
>      ld->datapath = dp;
>      ld->is_switch = datapath_is_switch(dp);
>      shash_init(&ld->external_ports);
> +    /* memory accounting - common part. */
> +    local_datapath_usage += sizeof *ld;
> +
>      return ld;
>  }
>  
> @@ -80,6 +107,16 @@ local_datapaths_destroy(struct hmap *local_datapaths)
>  void
>  local_datapath_destroy(struct local_datapath *ld)
>  {
> +    /* memory accounting. */
> +    struct shash_node *node;
> +    SHASH_FOR_EACH (node, &ld->external_ports) {
> +        local_datapath_usage -= strlen(node->name);
> +    }
> +    local_datapath_usage -= shash_count(&ld->external_ports) * sizeof *node;
> +    local_datapath_usage -= sizeof *ld;
> +    local_datapath_usage -=
> +        ld->n_allocated_peer_ports * sizeof *ld->peer_ports;
> +
>      free(ld->peer_ports);
>      shash_destroy(&ld->external_ports);
>      free(ld);
> @@ -175,10 +212,12 @@ add_local_datapath_peer_port(
>      if (!present) {
>          ld->n_peer_ports++;
>          if (ld->n_peer_ports > ld->n_allocated_peer_ports) {
> +            size_t n_peer_ports = ld->n_allocated_peer_ports;
>              ld->peer_ports =
>                  x2nrealloc(ld->peer_ports,
>                             &ld->n_allocated_peer_ports,
>                             sizeof *ld->peer_ports);
> +            local_datapath_peer_ports_mem_add(ld, n_peer_ports);
>          }
>          ld->peer_ports[ld->n_peer_ports - 1].local = pb;
>          ld->peer_ports[ld->n_peer_ports - 1].remote = peer;
> @@ -204,10 +243,12 @@ add_local_datapath_peer_port(
>  
>      peer_ld->n_peer_ports++;
>      if (peer_ld->n_peer_ports > peer_ld->n_allocated_peer_ports) {
> +        size_t n_peer_ports = peer_ld->n_allocated_peer_ports;
>          peer_ld->peer_ports =
>              x2nrealloc(peer_ld->peer_ports,
>                          &peer_ld->n_allocated_peer_ports,
>                          sizeof *peer_ld->peer_ports);
> +            local_datapath_peer_ports_mem_add(peer_ld, n_peer_ports);
>      }
>      peer_ld->peer_ports[peer_ld->n_peer_ports - 1].local = peer;
>      peer_ld->peer_ports[peer_ld->n_peer_ports - 1].remote = pb;
> @@ -248,6 +289,22 @@ remove_local_datapath_peer_port(const struct sbrec_port_binding *pb,
>      }
>  }
>  
> +void
> +add_local_datapath_external_port(struct local_datapath *ld,
> +                                 char *logical_port, const void *data)
> +{
> +    local_datapath_ext_port_mem_update(ld, logical_port, false);
> +    shash_replace(&ld->external_ports, logical_port, data);


We can simplify this and remove local_datapath_ext_port_mem_update()
if we rewrite it like:

if (!shash_replace(&ld->external_ports, logical_port, data)) {
    local_datapath_usage += sizeof(struct shash_node) + strlen(logical_port);
}

> +}
> +
> +void
> +remove_local_datapath_external_port(struct local_datapath *ld,
> +                                    char *logical_port)
> +{
> +    local_datapath_ext_port_mem_update(ld, logical_port, true);
> +    shash_find_and_delete(&ld->external_ports, logical_port);

Here too:

if (shash_find_and_delete(&ld->external_ports, logical_port)) {
    local_datapath_usage -= sizeof(struct shash_node) + strlen(logical_port);
}

> +}
> +
>  /* track datapath functions. */
>  struct tracked_datapath *
>  tracked_datapath_add(const struct sbrec_datapath_binding *dp,
> @@ -537,10 +594,12 @@ add_local_datapath__(struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
>                      }
>                      ld->n_peer_ports++;
>                      if (ld->n_peer_ports > ld->n_allocated_peer_ports) {
> +                        size_t n_peer_ports = ld->n_allocated_peer_ports;
>                          ld->peer_ports =
>                              x2nrealloc(ld->peer_ports,
>                                         &ld->n_allocated_peer_ports,
>                                         sizeof *ld->peer_ports);
> +                        local_datapath_peer_ports_mem_add(ld, n_peer_ports);

There's quite some code duplication when adding a new datapath peer port
(3 different places), we can probably refactor it and also include
memory accounting directly in there if we add a new function:

static void
local_datapath_peer_port_add(struct local_datapath *ld,
                             const struct sbrec_port_binding *local,
                             const struct sbrec_port_binding *remote)
{
    ld->n_peer_ports++;
    if (ld->n_peer_ports > ld->n_allocated_peer_ports) {
        size_t old_n_ports = ld->n_allocated_peer_ports;
        ld->peer_ports =
            x2nrealloc(ld->peer_ports,
                       &ld->n_allocated_peer_ports,
                       sizeof *ld->peer_ports);
        local_datapath_usage +=
            (ld->n_allocated_peer_ports - old_n_ports) *
            sizeof *ld->peer_ports;
    }
    ld->peer_ports[ld->n_peer_ports - 1].local = local;
    ld->peer_ports[ld->n_peer_ports - 1].remote = remote;
}

What do you think?

Thanks,
Dumitru