From e89be4ec559f4eb83ec37c9a452f73383665f5c0 Mon Sep 17 00:00:00 2001 From: Dave Barach Date: Wed, 29 Aug 2018 08:50:40 -0400 Subject: [PATCH] Crude stat segment lock recovery Make sure that vpp_get_stats main heap does not address-collide with the stats segment, which lands "somewhere" in the vpp address space. Add mising MAP_ANONYMOUS flag in clib_mem_vm_map Change-Id: I8a671d174eefd8dd24771ad2ed9f1250e2c7a9f8 Signed-off-by: Dave Barach Signed-off-by: Ole Troan --- src/vpp/app/vpp_get_stats.c | 4 +++- src/vpp/app/vpp_prometheus_export.c | 4 +++- src/vpp/stats/stat_segment.c | 38 +++++++++++++++++++++++-------------- src/vppinfra/mem.h | 2 +- 4 files changed, 31 insertions(+), 17 deletions(-) diff --git a/src/vpp/app/vpp_get_stats.c b/src/vpp/app/vpp_get_stats.c index 86e511e56f2..c1f43a747ed 100644 --- a/src/vpp/app/vpp_get_stats.c +++ b/src/vpp/app/vpp_get_stats.c @@ -108,8 +108,10 @@ main (int argc, char **argv) u8 *stat_segment_name, *pattern = 0, **patterns = 0; int rv; enum stat_client_cmd_e cmd = STAT_CLIENT_CMD_UNKNOWN; + void *heap_base; - clib_mem_init (0, 128 << 20); + heap_base = clib_mem_vm_map ((void *) 0x10000000ULL, 128 << 20); + clib_mem_init (heap_base, 128 << 20); unformat_init_command_line (a, argv); diff --git a/src/vpp/app/vpp_prometheus_export.c b/src/vpp/app/vpp_prometheus_export.c index 57c65178c73..cc85b817063 100644 --- a/src/vpp/app/vpp_prometheus_export.c +++ b/src/vpp/app/vpp_prometheus_export.c @@ -223,8 +223,10 @@ main (int argc, char **argv) unformat_input_t _argv, *a = &_argv; u8 *stat_segment_name, *pattern = 0, **patterns = 0; int rv; + void *heap_base; - clib_mem_init (0, 128 << 20); + heap_base = clib_mem_vm_map ((void *) 0x10000000ULL, 128 << 20); + clib_mem_init (heap_base, 128 << 20); unformat_init_command_line (a, argv); diff --git a/src/vpp/stats/stat_segment.c b/src/vpp/stats/stat_segment.c index 8459138be34..dcaeb5078b3 100644 --- a/src/vpp/stats/stat_segment.c +++ b/src/vpp/stats/stat_segment.c @@ -18,7 +18,20 @@ void vlib_stat_segment_lock (void) { stats_main_t *sm = &stats_main; - clib_spinlock_lock (sm->stat_segment_lockp); + vlib_main_t *vm = vlib_get_main (); + f64 deadman; + + /* 3ms is WAY long enough to be reasonably sure something is wrong */ + deadman = vlib_time_now (vm) + 3e-3; + + while (__sync_lock_test_and_set (&((*sm->stat_segment_lockp)->lock), 1)) + { + if (vlib_time_now (vm) >= deadman) + { + clib_warning ("BUG: stat segment lock held too long..."); + break; + } + } } void @@ -66,7 +79,7 @@ vlib_stats_pop_heap (void *cm_arg, void *oldheap, stat_directory_type_t type) stat_segment_name = cm->stat_segment_name ? cm->stat_segment_name : cm->name; - clib_spinlock_lock (sm->stat_segment_lockp); + vlib_stat_segment_lock (); /* Update hash table. The name must be copied into the segment */ hp = hash_get_pair (sm->counter_vector_by_name, stat_segment_name); @@ -91,7 +104,7 @@ vlib_stats_pop_heap (void *cm_arg, void *oldheap, stat_directory_type_t type) /* Warn clients to refresh any pointers they might be holding */ shared_header->opaque[STAT_SEGMENT_OPAQUE_EPOCH] = (void *) ((u64) shared_header->opaque[STAT_SEGMENT_OPAQUE_EPOCH] + 1); - clib_spinlock_unlock (sm->stat_segment_lockp); + vlib_stat_segment_unlock (); } ssvm_pop_heap (oldheap); } @@ -111,8 +124,7 @@ vlib_stats_register_error_index (u8 * name, u64 index) shared_header = ssvmp->sh; - clib_spinlock_lock (sm->stat_segment_lockp); - + vlib_stat_segment_lock (); /* Update hash table. The name must be copied into the segment */ hp = hash_get_pair (sm->counter_vector_by_name, name); if (hp) @@ -136,7 +148,7 @@ vlib_stats_register_error_index (u8 * name, u64 index) /* Warn clients to refresh any pointers they might be holding */ shared_header->opaque[STAT_SEGMENT_OPAQUE_EPOCH] = (void *) ((u64) shared_header->opaque[STAT_SEGMENT_OPAQUE_EPOCH] + 1); - clib_spinlock_unlock (sm->stat_segment_lockp); + vlib_stat_segment_unlock (); } void @@ -155,8 +167,7 @@ vlib_stats_pop_heap2 (u64 * counter_vector, u32 thread_index, void *oldheap) shared_header = ssvmp->sh; - clib_spinlock_lock (sm->stat_segment_lockp); - + vlib_stat_segment_lock (); error_vector_name = format (0, "/err/%d/counter_vector%c", thread_index, 0); /* Update hash table. The name must be copied into the segment */ @@ -182,7 +193,7 @@ vlib_stats_pop_heap2 (u64 * counter_vector, u32 thread_index, void *oldheap) /* Warn clients to refresh any pointers they might be holding */ shared_header->opaque[STAT_SEGMENT_OPAQUE_EPOCH] = (void *) ((u64) shared_header->opaque[STAT_SEGMENT_OPAQUE_EPOCH] + 1); - clib_spinlock_unlock (sm->stat_segment_lockp); + vlib_stat_segment_unlock (); ssvm_pop_heap (oldheap); } @@ -359,7 +370,7 @@ show_stat_segment_command_fn (vlib_main_t * vm, if (unformat (input, "verbose")) verbose = 1; - clib_spinlock_lock (sm->stat_segment_lockp); + vlib_stat_segment_lock (); /* *INDENT-OFF* */ hash_foreach_pair (p, sm->counter_vector_by_name, @@ -371,7 +382,7 @@ show_stat_segment_command_fn (vlib_main_t * vm, })); /* *INDENT-ON* */ - clib_spinlock_unlock (sm->stat_segment_lockp); + vlib_stat_segment_unlock (); vec_sort_with_function (show_data, name_sort_cmp); @@ -427,8 +438,7 @@ update_serialized_nodes (stats_main_t * sm) oldheap = ssvm_push_heap (shared_header); - clib_spinlock_lock (sm->stat_segment_lockp); - + vlib_stat_segment_lock (); vlib_node_get_nodes (0 /* vm, for barrier sync */ , (u32) ~ 0 /* all threads */ , 1 /* include stats */ , @@ -471,7 +481,7 @@ update_serialized_nodes (stats_main_t * sm) ((u64) shared_header->opaque[STAT_SEGMENT_OPAQUE_EPOCH] + 1); } - clib_spinlock_unlock (sm->stat_segment_lockp); + vlib_stat_segment_unlock (); ssvm_pop_heap (oldheap); } diff --git a/src/vppinfra/mem.h b/src/vppinfra/mem.h index 1646072e72f..0702aabb90a 100644 --- a/src/vppinfra/mem.h +++ b/src/vppinfra/mem.h @@ -348,7 +348,7 @@ always_inline void * clib_mem_vm_map (void *addr, uword size) { void *mmap_addr; - uword flags = MAP_PRIVATE | MAP_FIXED; + uword flags = MAP_PRIVATE | MAP_FIXED | MAP_ANONYMOUS; mmap_addr = mmap (addr, size, (PROT_READ | PROT_WRITE), flags, -1, 0); if (mmap_addr == (void *) -1) -- 2.16.6