On Oct 18, Dumitru Ceara wrote:
> 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 <[email protected]>
> > ---
> > 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);
ack, I will fix it in v5
> }
>
> > +}
> > +
> > +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);
ditto
> }
>
> > +}
> > +
> > /* 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?
ack, right. I will add it to v5.
Regards,
Lorenzo
>
> Thanks,
> Dumitru
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
