From 92e3082199d10add866894e86a9762d79a3536c4 Mon Sep 17 00:00:00 2001 From: Ole Troan Date: Sun, 16 Jun 2019 12:33:51 +0200 Subject: stats: fix memory leakage when adding / deleting interfaces This fixes two leaks in registering errors in the stats segment. - The error name created by vlib_register_errors() was not freed. - Duplicate error names (when interface readded) was added to the vector. This fix also adds memory usage statistics for the statistics segment as /mem/statseg/{used, total} Change-Id: Ife98d5fc5baef5bdae426a5a1eef428af2b9ab8a Type: fix Signed-off-by: Ole Troan --- src/vlib/error.c | 2 +- src/vlib/stat_weak_inlines.h | 4 +-- src/vpp/stats/stat_segment.c | 68 +++++++++++++++++++++++++++++++++----------- src/vpp/stats/stat_segment.h | 6 +++- src/vppinfra/mem_dlmalloc.c | 15 ++++++++++ src/vppinfra/mheap.h | 1 + 6 files changed, 76 insertions(+), 20 deletions(-) (limited to 'src') diff --git a/src/vlib/error.c b/src/vlib/error.c index 7c61f319fc0..ef506635ad9 100644 --- a/src/vlib/error.c +++ b/src/vlib/error.c @@ -166,7 +166,7 @@ vlib_register_errors (vlib_main_t * vm, { error_name = format (0, "/err/%v/%s%c", n->name, error_strings[i], 0); /* Note: error_name consumed by the following call */ - vlib_stats_register_error_index (error_name, em->counters, + vlib_stats_register_error_index (oldheap, error_name, em->counters, n->error_heap_index + i); } } diff --git a/src/vlib/stat_weak_inlines.h b/src/vlib/stat_weak_inlines.h index 5c04610ef13..d288a04c477 100644 --- a/src/vlib/stat_weak_inlines.h +++ b/src/vlib/stat_weak_inlines.h @@ -32,10 +32,10 @@ void vlib_stats_pop_heap (void *notused, void *notused2, u32 i, int type) { }; -void vlib_stats_register_error_index (u8 *, u64 *, u64) +void vlib_stats_register_error_index (void *, u8 *, u64 *, u64) __attribute__ ((weak)); void -vlib_stats_register_error_index (u8 * notused, u64 * notused2, u64 notused3) +vlib_stats_register_error_index (void * notused, u8 * notused2, u64 * notused3, u64 notused4) { }; diff --git a/src/vpp/stats/stat_segment.c b/src/vpp/stats/stat_segment.c index cef792fc622..c3521cf3dcf 100644 --- a/src/vpp/stats/stat_segment.c +++ b/src/vpp/stats/stat_segment.c @@ -22,6 +22,7 @@ #undef HAVE_MEMFD_CREATE #include #include +#include stat_segment_main_t stat_segment_main; @@ -58,14 +59,16 @@ vlib_stats_push_heap (void *old) return clib_mem_set_heap (sm->heap); } -/* Name to vector index hash */ static u32 -lookup_or_create_hash_index (void *oldheap, char *name, u32 next_vector_index) +lookup_or_create_hash_index (u8 * name, u32 next_vector_index) { stat_segment_main_t *sm = &stat_segment_main; u32 index; hash_pair_t *hp; + /* Must be called in the context of the main heap */ + ASSERT (clib_mem_get_heap != sm->heap); + hp = hash_get_pair (sm->directory_vector_by_name, name); if (!hp) { @@ -106,7 +109,7 @@ vlib_stats_pop_heap (void *cm_arg, void *oldheap, u32 cindex, cm->stat_segment_name ? cm->stat_segment_name : cm->name; u32 next_vector_index = vec_len (sm->directory_vector); clib_mem_set_heap (oldheap); /* Exit stats segment */ - u32 vector_index = lookup_or_create_hash_index (oldheap, stat_segment_name, + u32 vector_index = lookup_or_create_hash_index ((u8 *) stat_segment_name, next_vector_index); /* Back to stats segment */ clib_mem_set_heap (sm->heap); /* Re-enter stat segment */ @@ -153,7 +156,8 @@ vlib_stats_pop_heap (void *cm_arg, void *oldheap, u32 cindex, } void -vlib_stats_register_error_index (u8 * name, u64 * em_vec, u64 index) +vlib_stats_register_error_index (void *oldheap, u8 * name, u64 * em_vec, + u64 index) { stat_segment_main_t *sm = &stat_segment_main; stat_segment_shared_header_t *shared_header = sm->shared_header; @@ -162,17 +166,32 @@ vlib_stats_register_error_index (u8 * name, u64 * em_vec, u64 index) ASSERT (shared_header); vlib_stat_segment_lock (); + u32 next_vector_index = vec_len (sm->directory_vector); + clib_mem_set_heap (oldheap); /* Exit stats segment */ - memcpy (e.name, name, vec_len (name)); - e.name[vec_len (name)] = '\0'; - e.type = STAT_DIR_TYPE_ERROR_INDEX; - e.offset = index; - e.offset_vector = 0; - vec_add1 (sm->directory_vector, e); + u32 vector_index = lookup_or_create_hash_index (name, + next_vector_index); - /* Warn clients to refresh any pointers they might be holding */ - shared_header->directory_offset = - stat_segment_offset (shared_header, sm->directory_vector); + /* Back to stats segment */ + clib_mem_set_heap (sm->heap); /* Re-enter stat segment */ + + if (next_vector_index == vector_index) + { + memcpy (e.name, name, vec_len (name)); + e.name[vec_len (name)] = '\0'; + e.type = STAT_DIR_TYPE_ERROR_INDEX; + e.offset = index; + e.offset_vector = 0; + vec_add1 (sm->directory_vector, e); + + /* Warn clients to refresh any pointers they might be holding */ + shared_header->directory_offset = + stat_segment_offset (shared_header, sm->directory_vector); + } + else + { + vec_free (name); + } vlib_stat_segment_unlock (); } @@ -298,6 +317,12 @@ vlib_map_stat_segment_init (void) clib_mem_set_heap (oldheap); + /* Total shared memory size */ + clib_mem_usage_t usage; + mheap_usage (sm->heap, &usage); + sm->directory_vector[STAT_COUNTER_MEM_STATSEG_TOTAL].value = + usage.bytes_total; + return 0; } @@ -581,6 +606,12 @@ do_stat_segment_updates (stat_segment_main_t * sm) sm->directory_vector[STAT_COUNTER_LAST_STATS_CLEAR].value = vm->node_main.time_last_runtime_stats_clear; + /* Stats segment memory heap counter */ + clib_mem_usage_t usage; + mheap_usage (sm->heap, &usage); + sm->directory_vector[STAT_COUNTER_MEM_STATSEG_USED].value = + usage.bytes_used; + if (sm->node_counters_enabled) update_node_counters (sm); @@ -707,11 +738,17 @@ stat_segment_register_gauge (u8 * name, stat_segment_update_fn update_fn, stat_segment_shared_header_t *shared_header = sm->shared_header; void *oldheap; stat_segment_directory_entry_t e; - u32 index; stat_segment_gauges_pool_t *gauge; ASSERT (shared_header); + u32 next_vector_index = vec_len (sm->directory_vector); + u32 vector_index = lookup_or_create_hash_index (name, + next_vector_index); + + if (vector_index < next_vector_index) /* Already registered */ + return clib_error_return (0, "%v is alreadty registered", name); + oldheap = vlib_stats_push_heap (NULL); vlib_stat_segment_lock (); @@ -719,7 +756,6 @@ stat_segment_register_gauge (u8 * name, stat_segment_update_fn update_fn, e.type = STAT_DIR_TYPE_SCALAR_INDEX; memcpy (e.name, name, vec_len (name)); - index = vec_len (sm->directory_vector); vec_add1 (sm->directory_vector, e); shared_header->directory_offset = @@ -732,7 +768,7 @@ stat_segment_register_gauge (u8 * name, stat_segment_update_fn update_fn, pool_get (sm->gauges, gauge); gauge->fn = update_fn; gauge->caller_index = caller_index; - gauge->directory_index = index; + gauge->directory_index = next_vector_index; return NULL; } diff --git a/src/vpp/stats/stat_segment.h b/src/vpp/stats/stat_segment.h index 113eb9a25ad..a67c59feed5 100644 --- a/src/vpp/stats/stat_segment.h +++ b/src/vpp/stats/stat_segment.h @@ -36,6 +36,8 @@ typedef enum STAT_COUNTER_NODE_SUSPENDS, STAT_COUNTER_INTERFACE_NAMES, STAT_COUNTER_NODE_NAMES, + STAT_COUNTER_MEM_STATSEG_TOTAL, + STAT_COUNTER_MEM_STATSEG_USED, STAT_COUNTERS } stat_segment_counter_t; @@ -53,7 +55,9 @@ typedef enum _(NODE_CALLS, COUNTER_VECTOR_SIMPLE, calls, /sys/node) \ _(NODE_SUSPENDS, COUNTER_VECTOR_SIMPLE, suspends, /sys/node) \ _(INTERFACE_NAMES, NAME_VECTOR, names, /if) \ - _(NODE_NAMES, NAME_VECTOR, names, /sys/node) + _(NODE_NAMES, NAME_VECTOR, names, /sys/node) \ + _(MEM_STATSEG_TOTAL, SCALAR_INDEX, total, /mem/statseg) \ + _(MEM_STATSEG_USED, SCALAR_INDEX, used, /mem/statseg) typedef struct { diff --git a/src/vppinfra/mem_dlmalloc.c b/src/vppinfra/mem_dlmalloc.c index 99f1c04f3eb..7a53a8bb43b 100644 --- a/src/vppinfra/mem_dlmalloc.c +++ b/src/vppinfra/mem_dlmalloc.c @@ -383,6 +383,21 @@ clib_mem_usage (clib_mem_usage_t * u) clib_warning ("unimp"); } +void +mheap_usage (void *heap, clib_mem_usage_t * usage) +{ + struct dlmallinfo mi = mspace_mallinfo (heap); + + /* TODO: Fill in some more values */ + usage->object_count = 0; + usage->bytes_total = mi.arena; + usage->bytes_overhead = 0; + usage->bytes_max = 0; + usage->bytes_used = mi.uordblks; + usage->bytes_free = mi.fordblks; + usage->bytes_free_reclaimed = 0; +} + /* Call serial number for debugger breakpoints. */ uword clib_mem_validate_serial = 0; diff --git a/src/vppinfra/mheap.h b/src/vppinfra/mheap.h index afc2bf485b5..61d262c5f33 100644 --- a/src/vppinfra/mheap.h +++ b/src/vppinfra/mheap.h @@ -90,6 +90,7 @@ int test_mheap_main (unformat_input_t * input); /* Format mheap data structures as string. */ u8 *format_mheap (u8 * s, va_list * va); void *mheap_alloc_with_lock (void *memory, uword size, int locked); +void mheap_usage (void *v, clib_mem_usage_t * usage); #endif /* USE_DLMALLOC */ -- cgit 1.2.3-korg