| Message ID | a939b63a793e31c5bebbe158113971984ded823a.1634222132.git.lorenzo.bianconi@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | add memory accounting for if_status_mgr module | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
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@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
On 10/14/21 5:17 PM, Lorenzo Bianconi wrote: [...] > diff --git a/controller/local_data.h b/controller/local_data.h > index f6317e9ca..3defa9925 100644 > --- a/controller/local_data.h > +++ b/controller/local_data.h > @@ -154,5 +154,12 @@ bool get_chassis_tunnel_ofport(const struct hmap *chassis_tunnels, > ofp_port_t *ofport); > > void chassis_tunnels_destroy(struct hmap *chassis_tunnels); > +void local_datapath_memory_usage(struct simap *usage); > +void > +add_local_datapath_external_port(struct local_datapath *ld, > + char *logical_port, const void *data); Probably best to change this to 'const char *logical_port'. When reviewing I was in doubt about whether we should free the shash_node's data when removing external_ports. Changing the type to const would make it explicit, as free works only on non-const pointers. > +void > +remove_local_datapath_external_port(struct local_datapath *ld, > + char *logical_port); Same here. Thanks!
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 <lorenzo.bianconi@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); 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 >
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); +} + +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); +} + /* 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); } ld->peer_ports[ld->n_peer_ports - 1].local = pb; ld->peer_ports[ld->n_peer_ports - 1].remote = peer; @@ -563,3 +622,10 @@ tracked_datapath_create(const struct sbrec_datapath_binding *dp, hmap_insert(tracked_datapaths, &t_dp->node, uuid_hash(&dp->header_.uuid)); return t_dp; } + +void +local_datapath_memory_usage(struct simap *usage) +{ + simap_increase(usage, "local_datapath_usage-KB", + ROUND_UP(local_datapath_usage, 1024) / 1024); +} diff --git a/controller/local_data.h b/controller/local_data.h index f6317e9ca..3defa9925 100644 --- a/controller/local_data.h +++ b/controller/local_data.h @@ -154,5 +154,12 @@ bool get_chassis_tunnel_ofport(const struct hmap *chassis_tunnels, ofp_port_t *ofport); void chassis_tunnels_destroy(struct hmap *chassis_tunnels); +void local_datapath_memory_usage(struct simap *usage); +void +add_local_datapath_external_port(struct local_datapath *ld, + char *logical_port, const void *data); +void +remove_local_datapath_external_port(struct local_datapath *ld, + char *logical_port); #endif /* controller/local_data.h */ diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index e0fb0988f..a8e252e9c 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -3464,6 +3464,7 @@ main(int argc, char *argv[]) lflow_cache_get_memory_usage(ctrl_engine_ctx.lflow_cache, &usage); ofctrl_get_memory_usage(&usage); if_status_mgr_get_memory_usage(if_mgr, &usage); + local_datapath_memory_usage(&usage); memory_report(&usage); simap_destroy(&usage); }
Similar to if-status-mgr, track memory allocated for local_datapath. Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@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(-)