| Message ID | adcdf302c4758905b0b74787522969aa33a3668e.1631796946.git.lorenzo.bianconi@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] controller: 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 9/16/21 2:56 PM, Lorenzo Bianconi wrote: > Introduce memory accounting for data structures in ovn-controller > if_status_mgr module. > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > --- Hi Lorenzo, > controller/if-status.c | 23 +++++++++++++++++++++++ > controller/if-status.h | 3 +++ > controller/ovn-controller.c | 1 + > 3 files changed, 27 insertions(+) > > diff --git a/controller/if-status.c b/controller/if-status.c > index 08fb50b87..cd544f9ff 100644 > --- a/controller/if-status.c > +++ b/controller/if-status.c > @@ -18,6 +18,7 @@ > #include "binding.h" > #include "if-status.h" > #include "ofctrl-seqno.h" > +#include "simap.h" > > #include "lib/hmapx.h" > #include "lib/util.h" > @@ -85,6 +86,8 @@ struct ovs_iface { > */ > }; > > +static uint64_t ifaces_usage; > + > /* State machine manager for all local OVS interfaces. */ > struct if_status_mgr { > /* All local interfaces, mapping from 'iface-id' to 'struct ovs_iface'. */ > @@ -345,6 +348,8 @@ ovs_iface_create(struct if_status_mgr *mgr, const char *iface_id, > iface->id = xstrdup(iface_id); > shash_add(&mgr->ifaces, iface_id, iface); > ovs_iface_set_state(mgr, iface, state); > + ifaces_usage += (strlen(iface_id) + sizeof *iface + > + sizeof(struct shash_node)); Looks like this is not really accurate, because shash_add(shash, name, data) will xstrdup(name), so we need to account for iface_id twice. > return iface; > } > > @@ -355,6 +360,8 @@ ovs_iface_destroy(struct if_status_mgr *mgr, struct ovs_iface *iface) > if_state_names[iface->state]); > hmapx_find_and_delete(&mgr->ifaces_per_state[iface->state], iface); > shash_find_and_delete(&mgr->ifaces, iface->id); > + ifaces_usage -= (strlen(iface->id) + sizeof *iface + > + sizeof(struct shash_node)); Same remark here about iface->id that needs to be accounted for twice. Maybe we should be using shash_add_nocopy() instead when adding to the shash. Also, to avoid duplicating code and potentially accounting for memory differently when allocating vs freeing, should we maybe add a helper function to compute this size? What do you think? Thanks, Dumitru
On 9/16/21 8:56 AM, Lorenzo Bianconi wrote: > Introduce memory accounting for data structures in ovn-controller > if_status_mgr module. > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > --- > controller/if-status.c | 23 +++++++++++++++++++++++ > controller/if-status.h | 3 +++ > controller/ovn-controller.c | 1 + > 3 files changed, 27 insertions(+) > > diff --git a/controller/if-status.c b/controller/if-status.c > index 08fb50b87..cd544f9ff 100644 > --- a/controller/if-status.c > +++ b/controller/if-status.c > @@ -18,6 +18,7 @@ > #include "binding.h" > #include "if-status.h" > #include "ofctrl-seqno.h" > +#include "simap.h" > > #include "lib/hmapx.h" > #include "lib/util.h" > @@ -85,6 +86,8 @@ struct ovs_iface { > */ > }; > > +static uint64_t ifaces_usage; > + > /* State machine manager for all local OVS interfaces. */ > struct if_status_mgr { > /* All local interfaces, mapping from 'iface-id' to 'struct ovs_iface'. */ > @@ -345,6 +348,8 @@ ovs_iface_create(struct if_status_mgr *mgr, const char *iface_id, > iface->id = xstrdup(iface_id); > shash_add(&mgr->ifaces, iface_id, iface); > ovs_iface_set_state(mgr, iface, state); > + ifaces_usage += (strlen(iface_id) + sizeof *iface + > + sizeof(struct shash_node)); > return iface; > } > > @@ -355,6 +360,8 @@ ovs_iface_destroy(struct if_status_mgr *mgr, struct ovs_iface *iface) > if_state_names[iface->state]); > hmapx_find_and_delete(&mgr->ifaces_per_state[iface->state], iface); > shash_find_and_delete(&mgr->ifaces, iface->id); > + ifaces_usage -= (strlen(iface->id) + sizeof *iface + > + sizeof(struct shash_node)); nit-picking: avoid mix matching sizeof. be consistent. either use it with parenthesis or without. with parenthesis is more readeable imho but i will leave that to you to decide. Same comment goes to the sizeof being used above > free(iface->id); > free(iface); > } > @@ -413,3 +420,19 @@ if_status_mgr_update_bindings(struct if_status_mgr *mgr, > local_binding_set_down(bindings, iface->id, sb_readonly, ovs_readonly); > } > } > + > +void > +if_status_mgr_get_memory_usage(struct if_status_mgr *mgr, > + struct simap *usage) > +{ > + uint64_t ifaces_state_usage = 0; > + for (size_t i = 0; i < ARRAY_SIZE(mgr->ifaces_per_state); i++) { > + ifaces_state_usage += sizeof(struct hmapx_node) * > + hmapx_count(&mgr->ifaces_per_state[i]); > + } > + > + simap_increase(usage, "if_status_mgr_ifaces_usage-KB", > + ROUND_UP(ifaces_usage, 1024) / 1024); > + simap_increase(usage, "if_status_mgr_ifaces_state_usage-KB", > + ROUND_UP(ifaces_state_usage, 1024) / 1024); > +} > diff --git a/controller/if-status.h b/controller/if-status.h > index 51fe7c684..ff4aa760e 100644 > --- a/controller/if-status.h > +++ b/controller/if-status.h > @@ -21,6 +21,7 @@ > #include "binding.h" > > struct if_status_mgr; > +struct simap; > > struct if_status_mgr *if_status_mgr_create(void); > void if_status_mgr_clear(struct if_status_mgr *); > @@ -33,5 +34,7 @@ void if_status_mgr_delete_iface(struct if_status_mgr *, const char *iface_id); > void if_status_mgr_update(struct if_status_mgr *, struct local_binding_data *); > void if_status_mgr_run(struct if_status_mgr *mgr, struct local_binding_data *, > bool sb_readonly, bool ovs_readonly); > +void if_status_mgr_get_memory_usage(struct if_status_mgr *mgr, > + struct simap *usage); > > # endif /* controller/if-status.h */ > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 0031a1035..fabe5e8c0 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -3457,6 +3457,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); > memory_report(&usage); > simap_destroy(&usage); > } >
> > > On 9/16/21 8:56 AM, Lorenzo Bianconi wrote: > > Introduce memory accounting for data structures in ovn-controller > > if_status_mgr module. > > > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > --- > > controller/if-status.c | 23 +++++++++++++++++++++++ > > controller/if-status.h | 3 +++ > > controller/ovn-controller.c | 1 + > > 3 files changed, 27 insertions(+) > > > > diff --git a/controller/if-status.c b/controller/if-status.c > > index 08fb50b87..cd544f9ff 100644 > > --- a/controller/if-status.c > > +++ b/controller/if-status.c > > @@ -18,6 +18,7 @@ > > #include "binding.h" > > #include "if-status.h" > > #include "ofctrl-seqno.h" > > +#include "simap.h" > > #include "lib/hmapx.h" > > #include "lib/util.h" > > @@ -85,6 +86,8 @@ struct ovs_iface { > > */ > > }; > > +static uint64_t ifaces_usage; > > + > > /* State machine manager for all local OVS interfaces. */ > > struct if_status_mgr { > > /* All local interfaces, mapping from 'iface-id' to 'struct ovs_iface'. */ > > @@ -345,6 +348,8 @@ ovs_iface_create(struct if_status_mgr *mgr, const char *iface_id, > > iface->id = xstrdup(iface_id); > > shash_add(&mgr->ifaces, iface_id, iface); > > ovs_iface_set_state(mgr, iface, state); > > + ifaces_usage += (strlen(iface_id) + sizeof *iface + > > + sizeof(struct shash_node)); > > return iface; > > } > > @@ -355,6 +360,8 @@ ovs_iface_destroy(struct if_status_mgr *mgr, struct ovs_iface *iface) > > if_state_names[iface->state]); > > hmapx_find_and_delete(&mgr->ifaces_per_state[iface->state], iface); > > shash_find_and_delete(&mgr->ifaces, iface->id); > > + ifaces_usage -= (strlen(iface->id) + sizeof *iface + > > + sizeof(struct shash_node)); > nit-picking: > avoid mix matching sizeof. be consistent. either use it with parenthesis or > without. with parenthesis is more readeable imho but i will leave that to > you to decide. Same comment goes to the sizeof being used above Hi Michael, I removed this codepath in v2 but I think expression in sizeof are w/o parenthesis in ovs codestyle, right? Regards, Lorenzo > > free(iface->id); > > free(iface); > > } > > @@ -413,3 +420,19 @@ if_status_mgr_update_bindings(struct if_status_mgr *mgr, > > local_binding_set_down(bindings, iface->id, sb_readonly, ovs_readonly); > > } > > } > > + > > +void > > +if_status_mgr_get_memory_usage(struct if_status_mgr *mgr, > > + struct simap *usage) > > +{ > > + uint64_t ifaces_state_usage = 0; > > + for (size_t i = 0; i < ARRAY_SIZE(mgr->ifaces_per_state); i++) { > > + ifaces_state_usage += sizeof(struct hmapx_node) * > > + hmapx_count(&mgr->ifaces_per_state[i]); > > + } > > + > > + simap_increase(usage, "if_status_mgr_ifaces_usage-KB", > > + ROUND_UP(ifaces_usage, 1024) / 1024); > > + simap_increase(usage, "if_status_mgr_ifaces_state_usage-KB", > > + ROUND_UP(ifaces_state_usage, 1024) / 1024); > > +} > > diff --git a/controller/if-status.h b/controller/if-status.h > > index 51fe7c684..ff4aa760e 100644 > > --- a/controller/if-status.h > > +++ b/controller/if-status.h > > @@ -21,6 +21,7 @@ > > #include "binding.h" > > struct if_status_mgr; > > +struct simap; > > struct if_status_mgr *if_status_mgr_create(void); > > void if_status_mgr_clear(struct if_status_mgr *); > > @@ -33,5 +34,7 @@ void if_status_mgr_delete_iface(struct if_status_mgr *, const char *iface_id); > > void if_status_mgr_update(struct if_status_mgr *, struct local_binding_data *); > > void if_status_mgr_run(struct if_status_mgr *mgr, struct local_binding_data *, > > bool sb_readonly, bool ovs_readonly); > > +void if_status_mgr_get_memory_usage(struct if_status_mgr *mgr, > > + struct simap *usage); > > # endif /* controller/if-status.h */ > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index 0031a1035..fabe5e8c0 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -3457,6 +3457,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); > > memory_report(&usage); > > simap_destroy(&usage); > > } > > >
diff --git a/controller/if-status.c b/controller/if-status.c index 08fb50b87..cd544f9ff 100644 --- a/controller/if-status.c +++ b/controller/if-status.c @@ -18,6 +18,7 @@ #include "binding.h" #include "if-status.h" #include "ofctrl-seqno.h" +#include "simap.h" #include "lib/hmapx.h" #include "lib/util.h" @@ -85,6 +86,8 @@ struct ovs_iface { */ }; +static uint64_t ifaces_usage; + /* State machine manager for all local OVS interfaces. */ struct if_status_mgr { /* All local interfaces, mapping from 'iface-id' to 'struct ovs_iface'. */ @@ -345,6 +348,8 @@ ovs_iface_create(struct if_status_mgr *mgr, const char *iface_id, iface->id = xstrdup(iface_id); shash_add(&mgr->ifaces, iface_id, iface); ovs_iface_set_state(mgr, iface, state); + ifaces_usage += (strlen(iface_id) + sizeof *iface + + sizeof(struct shash_node)); return iface; } @@ -355,6 +360,8 @@ ovs_iface_destroy(struct if_status_mgr *mgr, struct ovs_iface *iface) if_state_names[iface->state]); hmapx_find_and_delete(&mgr->ifaces_per_state[iface->state], iface); shash_find_and_delete(&mgr->ifaces, iface->id); + ifaces_usage -= (strlen(iface->id) + sizeof *iface + + sizeof(struct shash_node)); free(iface->id); free(iface); } @@ -413,3 +420,19 @@ if_status_mgr_update_bindings(struct if_status_mgr *mgr, local_binding_set_down(bindings, iface->id, sb_readonly, ovs_readonly); } } + +void +if_status_mgr_get_memory_usage(struct if_status_mgr *mgr, + struct simap *usage) +{ + uint64_t ifaces_state_usage = 0; + for (size_t i = 0; i < ARRAY_SIZE(mgr->ifaces_per_state); i++) { + ifaces_state_usage += sizeof(struct hmapx_node) * + hmapx_count(&mgr->ifaces_per_state[i]); + } + + simap_increase(usage, "if_status_mgr_ifaces_usage-KB", + ROUND_UP(ifaces_usage, 1024) / 1024); + simap_increase(usage, "if_status_mgr_ifaces_state_usage-KB", + ROUND_UP(ifaces_state_usage, 1024) / 1024); +} diff --git a/controller/if-status.h b/controller/if-status.h index 51fe7c684..ff4aa760e 100644 --- a/controller/if-status.h +++ b/controller/if-status.h @@ -21,6 +21,7 @@ #include "binding.h" struct if_status_mgr; +struct simap; struct if_status_mgr *if_status_mgr_create(void); void if_status_mgr_clear(struct if_status_mgr *); @@ -33,5 +34,7 @@ void if_status_mgr_delete_iface(struct if_status_mgr *, const char *iface_id); void if_status_mgr_update(struct if_status_mgr *, struct local_binding_data *); void if_status_mgr_run(struct if_status_mgr *mgr, struct local_binding_data *, bool sb_readonly, bool ovs_readonly); +void if_status_mgr_get_memory_usage(struct if_status_mgr *mgr, + struct simap *usage); # endif /* controller/if-status.h */ diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 0031a1035..fabe5e8c0 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -3457,6 +3457,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); memory_report(&usage); simap_destroy(&usage); }
Introduce memory accounting for data structures in ovn-controller if_status_mgr module. Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> --- controller/if-status.c | 23 +++++++++++++++++++++++ controller/if-status.h | 3 +++ controller/ovn-controller.c | 1 + 3 files changed, 27 insertions(+)