From 72c36b60d4ee89beb8dfab72316a3717e97c20eb Mon Sep 17 00:00:00 2001 From: Yossi Itigin Date: Sat, 12 Dec 2020 14:52:40 +0200 Subject: [PATCH 1/5] TEST/APPS: Add test for multi-threaded call to ucp_init() --- contrib/test_jenkins.sh | 10 ++++++++ test/apps/Makefile.am | 8 +++++- test/apps/test_init_mt.c | 55 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 72 insertions(+), 1 deletion(-) create mode 100644 test/apps/test_init_mt.c diff --git a/contrib/test_jenkins.sh b/contrib/test_jenkins.sh index 610178ef89e..3cb54c70cd8 100755 --- a/contrib/test_jenkins.sh +++ b/contrib/test_jenkins.sh @@ -1207,6 +1207,15 @@ test_ucp_dlopen() { fi } +test_init_mt() { + echo "==== Running multi-thread init ====" + $MAKEP + for ((i=0;i<50;++i)) + do + $AFFINITY timeout 1m ./test/apps/test_init_mt + done +} + test_memtrack() { ../contrib/configure-devel --prefix=$ucx_inst $MAKEP clean @@ -1627,6 +1636,7 @@ run_tests() { do_distributed_task 2 4 test_env_var_aliases do_distributed_task 1 3 test_malloc_hook do_distributed_task 0 4 test_ucp_dlopen + do_distributed_task 1 4 test_init_mt # all are running gtest run_gtest_default diff --git a/test/apps/Makefile.am b/test/apps/Makefile.am index 414ae5d0d02..b5930cef66d 100644 --- a/test/apps/Makefile.am +++ b/test/apps/Makefile.am @@ -20,7 +20,8 @@ noinst_PROGRAMS = \ test_ucp_dlopen \ test_ucs_dlopen \ test_link_map \ - test_dlopen_cfg_print + test_dlopen_cfg_print \ + test_init_mt objdir = $(shell sed -n -e 's/^objdir=\(.*\)$$/\1/p' $(LIBTOOL)) @@ -48,6 +49,11 @@ test_dlopen_cfg_print_CPPFLAGS = $(BASE_CPPFLAGS) -g \ test_dlopen_cfg_print_CFLAGS = $(BASE_CFLAGS) test_dlopen_cfg_print_LDADD = -ldl +test_init_mt_SOURCES = test_init_mt.c +test_init_mt_CPPFLAGS = $(BASE_CPPFLAGS) +test_init_mt_CFLAGS = $(BASE_CFLAGS) $(OPENMP_CFLAGS) +test_init_mt_LDADD = $(top_builddir)/src/ucp/libucp.la + if HAVE_TCMALLOC noinst_PROGRAMS += test_tcmalloc test_tcmalloc_SOURCES = test_tcmalloc.c diff --git a/test/apps/test_init_mt.c b/test/apps/test_init_mt.c new file mode 100644 index 00000000000..612c506cc27 --- /dev/null +++ b/test/apps/test_init_mt.c @@ -0,0 +1,55 @@ +/** + * Copyright (C) Mellanox Technologies Ltd. 2020. ALL RIGHTS RESERVED. + * + * See file LICENSE for terms. + */ + +#ifdef HAVE_CONFIG_H +#include "config.h" +#endif + +#include + +#if _OPENMP +#include +#endif + + +int main(int argc, char **argv) +{ + int count = 0; + +#pragma omp parallel + { + ucs_status_t ctx_status, worker_status; + ucp_context_h context; + ucp_worker_h worker; + ucp_params_t params; + ucp_worker_params_t wparams; + + params.field_mask = UCP_PARAM_FIELD_FEATURES; + params.features = UCP_FEATURE_TAG | UCP_FEATURE_STREAM; + ctx_status = ucp_init(¶ms, NULL, &context); + if (ctx_status == UCS_OK) { + wparams.field_mask = 0; + worker_status = ucp_worker_create(context, &wparams, &worker); + if (worker_status == UCS_OK) { + __sync_add_and_fetch(&count, 1); + } + } + +#pragma omp barrier + + if (ctx_status == UCS_OK) { + if (worker_status == UCS_OK) { + ucp_worker_destroy(worker); + } + ucp_cleanup(context); + } + } + +#pragma omp barrier + + printf("finished %d threads\n", count); + return 0; +} From b17336da8b382e2e26d8dbc8b7f85bf4afe47de4 Mon Sep 17 00:00:00 2001 From: Yossi Itigin Date: Sat, 12 Dec 2020 14:53:33 +0200 Subject: [PATCH 2/5] UCM/LOG: Improve and fix logger + Fix printing of large numbers + Print thread id (tid - pid) --- src/ucm/util/log.c | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/src/ucm/util/log.c b/src/ucm/util/log.c index 8fe6b31454c..d15d165ac5f 100644 --- a/src/ucm/util/log.c +++ b/src/ucm/util/log.c @@ -23,6 +23,7 @@ #include #include #include +#include #define UCM_LOG_BUG_SIZE 256 @@ -64,7 +65,7 @@ static char *ucm_log_ltoa(char *p, char *end, long n, int base, int flags, int pad) { static const char digits[] = "0123456789abcdef"; - long divider; + long divider, top_divider; if (((n < 0) || (flags & UCM_LOG_LTOA_FLAG_SIGN)) && (p < end)) { *(p++) = (n < 0 ) ? '-' : '+'; @@ -79,9 +80,11 @@ static char *ucm_log_ltoa(char *p, char *end, long n, int base, int flags, n = labs(n); - divider = 1; - while ((n / divider) != 0) { - divider *= base; + divider = 1; + top_divider = 0; + while ((divider > 0) && ((n / divider) != 0)) { + top_divider = divider; + divider *= base; --pad; } @@ -90,7 +93,7 @@ static char *ucm_log_ltoa(char *p, char *end, long n, int base, int flags, (flags & UCM_LOG_LTOA_FLAG_PAD0) ? '0' : ' '); } - divider /= base; + divider = top_divider; while ((p < end) && (divider > 0)) { *(p++) = digits[(n / divider + base) % base]; divider /= base; @@ -242,6 +245,11 @@ static void ucm_log_vsnprintf(char *buf, size_t max, const char *fmt, va_list ap *pb = '\0'; } +static pid_t ucm_get_tid(void) +{ + return syscall(SYS_gettid); +} + static void ucm_log_snprintf(char *buf, size_t max, const char *fmt, ...) { va_list ap; @@ -259,11 +267,15 @@ void __ucm_log(const char *file, unsigned line, const char *function, va_list ap; struct timeval tv; ssize_t nwrite; + pid_t pid; gettimeofday(&tv, NULL); - ucm_log_snprintf(buf, UCM_LOG_BUG_SIZE - 1, "[%lu.%06lu] [%s:%d] %18s:%-4d UCX %s ", - tv.tv_sec, tv.tv_usec, ucm_log_hostname, getpid(), - ucs_basename(file), line, ucm_log_level_names[level]); + pid = getpid(); + ucm_log_snprintf(buf, UCM_LOG_BUG_SIZE - 1, + "[%lu.%06lu] [%s:%d:%d] %18s:%-4d UCX %s ", + tv.tv_sec, tv.tv_usec, ucm_log_hostname, pid, + ucm_get_tid() - pid, ucs_basename(file), line, + ucm_log_level_names[level]); buf[UCM_LOG_BUG_SIZE - 1] = '\0'; length = strlen(buf); From 6c35d982c7b2a57480369161cbeebc380a98f200 Mon Sep 17 00:00:00 2001 From: Yossi Itigin Date: Sat, 12 Dec 2020 23:43:53 +0200 Subject: [PATCH 3/5] UCM: Fix heap corruption caused by ucp_set_event_handler() Fix race between sbrk() from testing mmap events while they are being installed, and brk/sbrk() from any other thread in the program as part of normal heap operation. Such race leads to heap corruption and program abort/segfault. In general, we should not call brk/sbrk() directly. Fix method: 1. Initialize bistro hooks during library initialization. This makes sure no other thread would read bad machine instructions. 2. Call brk/sbrk() from events test only when it's from exclusive context (1). For non-exclusive case, make only dummy calls with invalid parameters, which should not have side effects on other threads. 3. Create separate event for brk(), to avoid emulating sbrk() event when the actual call was to brk(). 4. Fix brk() syscall to return full 64-bit value. --- src/ucm/api/ucm.h | 26 ++++++- src/ucm/event/event.c | 74 +++++++++++------- src/ucm/event/event.h | 5 +- src/ucm/malloc/malloc_hook.c | 2 +- src/ucm/mmap/install.c | 146 +++++++++++++++++++++-------------- src/ucm/mmap/mmap.h | 5 +- src/ucm/util/replace.c | 32 +++++--- src/ucm/util/sys.c | 62 ++++++++++----- src/ucm/util/sys.h | 13 ++++ src/ucs/config/ucm_opts.c | 7 +- 10 files changed, 246 insertions(+), 126 deletions(-) diff --git a/src/ucm/api/ucm.h b/src/ucm/api/ucm.h index 0e200106085..bba7422828c 100644 --- a/src/ucm/api/ucm.h +++ b/src/ucm/api/ucm.h @@ -37,6 +37,7 @@ typedef enum ucm_event_type { UCM_EVENT_SHMDT = UCS_BIT(4), UCM_EVENT_SBRK = UCS_BIT(5), UCM_EVENT_MADVISE = UCS_BIT(6), + UCM_EVENT_BRK = UCS_BIT(7), /* Aggregate events */ UCM_EVENT_VM_MAPPED = UCS_BIT(16), @@ -149,6 +150,15 @@ typedef union ucm_event { int advice; } madvise; + /* + * UCM_EVENT_BRK + * brk() is called. + */ + struct { + int result; + void *addr; + } brk; + /* * UCM_EVENT_VM_MAPPED, UCM_EVENT_VM_UNMAPPED * @@ -198,7 +208,10 @@ typedef struct ucm_global_config { } ucm_global_config_t; -/* Global UCM configuration */ +/* + * Global UCM configuration to be set externally. + * @deprecated replaced by @ref ucm_library_init. + */ extern ucm_global_config_t ucm_global_opts; @@ -240,6 +253,17 @@ typedef void (*ucm_event_callback_t)(ucm_event_type_t event_type, ucm_event_t *event, void *arg); +/** + * Initialize UCM library and set its configuration. + * + * @param [in] ucm_opts UCM library global configuration. If NULL, default + * configuration is applied. + * + * @note Calling this function more than once in the same process has no effect. + */ +void ucm_library_init(const ucm_global_config_t *ucm_opts); + + /** * @brief Install a handler for memory events. * diff --git a/src/ucm/event/event.c b/src/ucm/event/event.c index 0a0c05edfeb..bf94cf10296 100644 --- a/src/ucm/event/event.c +++ b/src/ucm/event/event.c @@ -93,6 +93,11 @@ static void ucm_event_call_orig(ucm_event_type_t event_type, ucm_event_t *event, event->shmdt.result = ucm_orig_shmdt(event->shmdt.shmaddr); } break; + case UCM_EVENT_BRK: + if (event->brk.result == -1) { + event->brk.result = ucm_orig_brk(event->brk.addr); + } + break; case UCM_EVENT_SBRK: if (event->sbrk.result == MAP_FAILED) { event->sbrk.result = ucm_orig_sbrk(event->sbrk.increment); @@ -119,8 +124,8 @@ static ucm_event_handler_t ucm_event_orig_handler = { .list = UCS_LIST_INITIALIZER(&ucm_event_handlers, &ucm_event_handlers), .events = UCM_EVENT_MMAP | UCM_EVENT_MUNMAP | UCM_EVENT_MREMAP | UCM_EVENT_SHMAT | UCM_EVENT_SHMDT | UCM_EVENT_SBRK | - UCM_EVENT_MADVISE, /* All events */ - .priority = 0, /* Between negative and positive handlers */ + UCM_EVENT_MADVISE | UCM_EVENT_BRK, /* All events */ + .priority = 0, /* Between negative and positive handlers */ .cb = ucm_event_call_orig }; static ucs_list_link_t ucm_event_handlers = @@ -351,7 +356,8 @@ void *ucm_sbrk(intptr_t increment) ucm_trace("ucm_sbrk(increment=%+ld)", increment); if (increment < 0) { - ucm_dispatch_vm_munmap(UCS_PTR_BYTE_OFFSET(ucm_orig_sbrk(0), increment), + ucm_dispatch_vm_munmap(UCS_PTR_BYTE_OFFSET(ucm_get_current_brk(), + increment), -increment); } @@ -360,7 +366,8 @@ void *ucm_sbrk(intptr_t increment) ucm_event_dispatch(UCM_EVENT_SBRK, &event); if ((increment > 0) && (event.sbrk.result != MAP_FAILED)) { - ucm_dispatch_vm_mmap(UCS_PTR_BYTE_OFFSET(ucm_orig_sbrk(0), -increment), + ucm_dispatch_vm_mmap(UCS_PTR_BYTE_OFFSET(ucm_get_current_brk(), + -increment), increment); } @@ -371,38 +378,36 @@ void *ucm_sbrk(intptr_t increment) int ucm_brk(void *addr) { -#if UCM_BISTRO_HOOKS - void *old_addr; - intptr_t increment; + ptrdiff_t increment; + void *current_brk; ucm_event_t event; - old_addr = ucm_brk_syscall(0); - /* in case if addr == NULL - it just returns current pointer */ - increment = addr ? ((intptr_t)addr - (intptr_t)old_addr) : 0; - ucm_event_enter(); ucm_trace("ucm_brk(addr=%p)", addr); + if (addr == NULL) { + increment = 0; + } else { + current_brk = ucm_get_current_brk(); + increment = UCS_PTR_BYTE_DIFF(current_brk, addr); + } + if (increment < 0) { - ucm_dispatch_vm_munmap(UCS_PTR_BYTE_OFFSET(old_addr, increment), - -increment); + ucm_dispatch_vm_munmap(addr, -increment); } - event.sbrk.result = (void*)-1; - event.sbrk.increment = increment; - ucm_event_dispatch(UCM_EVENT_SBRK, &event); + event.brk.result = -1; + event.brk.addr = addr; + ucm_event_dispatch(UCM_EVENT_BRK, &event); - if ((increment > 0) && (event.sbrk.result != MAP_FAILED)) { - ucm_dispatch_vm_mmap(old_addr, increment); + if ((increment > 0) && (event.brk.result != -1)) { + ucm_dispatch_vm_mmap(current_brk, increment); } ucm_event_leave(); - return event.sbrk.result == MAP_FAILED ? -1 : 0; -#else - return -1; -#endif + return event.brk.result; } int ucm_madvise(void *addr, size_t length, int advice) @@ -439,6 +444,18 @@ int ucm_madvise(void *addr, size_t length, int advice) return event.madvise.result; } +void ucm_library_init(const ucm_global_config_t *ucm_opts) +{ + static ucs_init_once_t init_once = UCS_INIT_ONCE_INITIALIZER; + + UCS_INIT_ONCE(&init_once) { + if (ucm_opts != NULL) { + ucm_global_opts = *ucm_opts; + } + ucm_mmap_init(); + } +} + void ucm_event_handler_add(ucm_event_handler_t *handler) { ucm_event_handler_t *elem; @@ -481,21 +498,18 @@ static int ucm_events_to_native_events(int events) static ucs_status_t ucm_event_install(int events) { - static ucs_init_once_t init_once = UCS_INIT_ONCE_INITIALIZER; UCS_MODULE_FRAMEWORK_DECLARE(ucm); ucm_event_installer_t *event_installer; int native_events, malloc_events; ucs_status_t status; - UCS_INIT_ONCE(&init_once) { - ucm_prevent_dl_unload(); - } + ucm_prevent_dl_unload(); /* Replace aggregate events with the native events which make them */ native_events = ucm_events_to_native_events(events); /* TODO lock */ - status = ucm_mmap_install(native_events); + status = ucm_mmap_install(native_events, 0); if (status != UCS_OK) { ucm_debug("failed to install mmap events"); goto out_unlock; @@ -526,7 +540,6 @@ static ucs_status_t ucm_event_install(int events) out_unlock: return status; - } ucs_status_t ucm_set_event_handler(int events, int priority, @@ -539,7 +552,7 @@ ucs_status_t ucm_set_event_handler(int events, int priority, if (events & ~(UCM_EVENT_MMAP|UCM_EVENT_MUNMAP|UCM_EVENT_MREMAP| UCM_EVENT_SHMAT|UCM_EVENT_SHMDT| - UCM_EVENT_SBRK| + UCM_EVENT_BRK|UCM_EVENT_SBRK| UCM_EVENT_MADVISE| UCM_EVENT_VM_MAPPED|UCM_EVENT_VM_UNMAPPED| UCM_EVENT_MEM_TYPE_ALLOC|UCM_EVENT_MEM_TYPE_FREE| @@ -552,6 +565,8 @@ ucs_status_t ucm_set_event_handler(int events, int priority, return UCS_ERR_UNSUPPORTED; } + ucm_library_init(NULL); + /* separate event flags from real events */ flags = events & (UCM_EVENT_FLAG_NO_INSTALL | UCM_EVENT_FLAG_EXISTING_ALLOC); @@ -626,6 +641,7 @@ void ucm_unset_event_handler(int events, ucm_event_callback_t cb, void *arg) ucs_status_t ucm_test_events(int events) { + ucm_library_init(NULL); return ucm_mmap_test_installed_events(ucm_events_to_native_events(events)); } diff --git a/src/ucm/event/event.h b/src/ucm/event/event.h index 763ac3b2098..e7ae14ec6ad 100644 --- a/src/ucm/event/event.h +++ b/src/ucm/event/event.h @@ -13,12 +13,13 @@ #include #define UCM_NATIVE_EVENT_VM_MAPPED (UCM_EVENT_MMAP | UCM_EVENT_MREMAP | \ - UCM_EVENT_SHMAT | UCM_EVENT_SBRK) + UCM_EVENT_SHMAT | UCM_EVENT_SBRK | \ + UCM_EVENT_BRK) #define UCM_NATIVE_EVENT_VM_UNMAPPED (UCM_EVENT_MMAP | UCM_EVENT_MUNMAP | \ UCM_EVENT_MREMAP | UCM_EVENT_SHMDT | \ UCM_EVENT_SHMAT | UCM_EVENT_SBRK | \ - UCM_EVENT_MADVISE) + UCM_EVENT_MADVISE | UCM_EVENT_BRK) typedef struct ucm_event_handler { diff --git a/src/ucm/malloc/malloc_hook.c b/src/ucm/malloc/malloc_hook.c index f073398e93c..13a3bf287db 100644 --- a/src/ucm/malloc/malloc_hook.c +++ b/src/ucm/malloc/malloc_hook.c @@ -557,7 +557,7 @@ static void ucm_malloc_sbrk(ucm_event_type_t event_type, if (ucm_malloc_hook_state.heap_start == (void*)-1) { ucm_malloc_hook_state.heap_start = event->sbrk.result; /* sbrk() returns the previous break */ } - ucm_malloc_hook_state.heap_end = ucm_orig_sbrk(0); + ucm_malloc_hook_state.heap_end = ucm_get_current_brk(); ucm_trace("sbrk(%+ld)=%p - adjusting heap to [%p..%p]", event->sbrk.increment, event->sbrk.result, diff --git a/src/ucm/mmap/install.c b/src/ucm/mmap/install.c index 6824a6247be..2e5d42768b2 100644 --- a/src/ucm/mmap/install.c +++ b/src/ucm/mmap/install.c @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include @@ -29,8 +30,6 @@ #include #include -#define UCM_IS_HOOK_ENABLED(_entry) \ - ((_entry)->hook_type & UCS_BIT(ucm_mmap_hook_mode())) #define UCM_HOOK_STR \ ((ucm_mmap_hook_mode() == UCM_MMAP_HOOK_RELOC) ? "reloc" : "bistro") @@ -47,17 +46,10 @@ extern const char *ucm_mmap_hook_modes[]; -typedef enum ucm_mmap_hook_type { - UCM_HOOK_RELOC = UCS_BIT(UCM_MMAP_HOOK_RELOC), - UCM_HOOK_BISTRO = UCS_BIT(UCM_MMAP_HOOK_BISTRO), - UCM_HOOK_BOTH = UCM_HOOK_RELOC | UCM_HOOK_BISTRO -} ucm_mmap_hook_type_t; - typedef struct ucm_mmap_func { ucm_reloc_patch_t patch; ucm_event_type_t event_type; ucm_event_type_t deps; - ucm_mmap_hook_type_t hook_type; } ucm_mmap_func_t; typedef struct ucm_mmap_test_events_data { @@ -66,18 +58,16 @@ typedef struct ucm_mmap_test_events_data { } ucm_mmap_test_events_data_t; static ucm_mmap_func_t ucm_mmap_funcs[] = { - { {"mmap", ucm_override_mmap}, UCM_EVENT_MMAP, UCM_EVENT_NONE, UCM_HOOK_BOTH}, - { {"munmap", ucm_override_munmap}, UCM_EVENT_MUNMAP, UCM_EVENT_NONE, UCM_HOOK_BOTH}, + { {"mmap", ucm_override_mmap}, UCM_EVENT_MMAP, UCM_EVENT_NONE}, + { {"munmap", ucm_override_munmap}, UCM_EVENT_MUNMAP, UCM_EVENT_NONE}, #if HAVE_MREMAP - { {"mremap", ucm_override_mremap}, UCM_EVENT_MREMAP, UCM_EVENT_NONE, UCM_HOOK_BOTH}, -#endif - { {"shmat", ucm_override_shmat}, UCM_EVENT_SHMAT, UCM_EVENT_NONE, UCM_HOOK_BOTH}, - { {"shmdt", ucm_override_shmdt}, UCM_EVENT_SHMDT, UCM_EVENT_SHMAT, UCM_HOOK_BOTH}, - { {"sbrk", ucm_override_sbrk}, UCM_EVENT_SBRK, UCM_EVENT_NONE, UCM_HOOK_RELOC}, -#if UCM_BISTRO_HOOKS - { {"brk", ucm_override_brk}, UCM_EVENT_SBRK, UCM_EVENT_NONE, UCM_HOOK_BISTRO}, + { {"mremap", ucm_override_mremap}, UCM_EVENT_MREMAP, UCM_EVENT_NONE}, #endif - { {"madvise", ucm_override_madvise}, UCM_EVENT_MADVISE, UCM_EVENT_NONE, UCM_HOOK_BOTH}, + { {"shmat", ucm_override_shmat}, UCM_EVENT_SHMAT, UCM_EVENT_NONE}, + { {"shmdt", ucm_override_shmdt}, UCM_EVENT_SHMDT, UCM_EVENT_SHMAT}, + { {"sbrk", ucm_override_sbrk}, UCM_EVENT_SBRK, UCM_EVENT_NONE}, + { {"brk", ucm_override_brk}, UCM_EVENT_BRK, UCM_EVENT_NONE}, + { {"madvise", ucm_override_madvise}, UCM_EVENT_MADVISE, UCM_EVENT_NONE}, { {NULL, NULL}, UCM_EVENT_NONE} }; @@ -97,6 +87,15 @@ static void ucm_mmap_event_test_callback(ucm_event_type_t event_type, ucs_atomic_or32(&data->fired_events, event_type); } +/* Call brk() and check return value, to avoid compile error of unused result */ +static void ucm_brk_checked(void *addr) +{ + int ret = brk(addr); + if ((ret != 0) && (addr != NULL)) { + ucm_debug("brk(addr=%p) failed: %m", addr); + } +} + /* Fire events with pre/post action. The problem is in call sequence: we * can't just fire single event - most of the system calls require set of * calls to eliminate resource leaks or data corruption, such sequence @@ -104,10 +103,10 @@ static void ucm_mmap_event_test_callback(ucm_event_type_t event_type, * exclude additional events from processing used pre/post actions where * set of handled events is cleared and evaluated for every system call */ static void -ucm_fire_mmap_events_internal(int events, ucm_mmap_test_events_data_t *data) +ucm_fire_mmap_events_internal(int events, ucm_mmap_test_events_data_t *data, + int exclusive) { size_t sbrk_size; - int sbrk_mask; int shmid; void *p; @@ -149,19 +148,29 @@ ucm_fire_mmap_events_internal(int events, ucm_mmap_test_events_data_t *data) data, shmdt(p)); } - if (events & (UCM_EVENT_SBRK|UCM_EVENT_VM_MAPPED|UCM_EVENT_VM_UNMAPPED)) { - if (RUNNING_ON_VALGRIND) { - /* on valgrind, doing a non-trivial sbrk() causes heap corruption */ - sbrk_size = 0; - sbrk_mask = UCM_EVENT_SBRK; - } else { - sbrk_size = ucm_get_page_size(); - sbrk_mask = UCM_EVENT_SBRK|UCM_EVENT_VM_MAPPED|UCM_EVENT_VM_UNMAPPED; + if (exclusive && !RUNNING_ON_VALGRIND) { + sbrk_size = ucm_get_page_size(); + if (events & (UCM_EVENT_BRK|UCM_EVENT_VM_MAPPED|UCM_EVENT_VM_UNMAPPED)) { + p = ucm_get_current_brk(); + UCM_FIRE_EVENT(events, UCM_EVENT_BRK|UCM_EVENT_VM_MAPPED, data, + ucm_brk_checked(UCS_PTR_BYTE_OFFSET(p, sbrk_size))); + UCM_FIRE_EVENT(events, UCM_EVENT_BRK|UCM_EVENT_VM_UNMAPPED, data, + ucm_brk_checked(p)); + } + if (events & (UCM_EVENT_SBRK|UCM_EVENT_VM_MAPPED|UCM_EVENT_VM_UNMAPPED)) { + UCM_FIRE_EVENT(events, UCM_EVENT_SBRK|UCM_EVENT_VM_MAPPED, + data, (void)sbrk(sbrk_size)); + UCM_FIRE_EVENT(events, UCM_EVENT_SBRK|UCM_EVENT_VM_UNMAPPED, + data, (void)sbrk(-sbrk_size)); + } + } else { + /* To avoid side effects on other threads and valgrind heap corruption, + * pass invalid parameters. We assume that if the natives events are + * delivered, it means VM_MAPPED/UNMAPPED would be delivered as well. + */ + if (events & UCM_EVENT_BRK) { + UCM_FIRE_EVENT(events, UCM_EVENT_BRK, data, ucm_brk_checked(NULL)); } - UCM_FIRE_EVENT(events, (UCM_EVENT_SBRK|UCM_EVENT_VM_MAPPED) & sbrk_mask, - data, (void)sbrk(sbrk_size)); - UCM_FIRE_EVENT(events, (UCM_EVENT_SBRK|UCM_EVENT_VM_UNMAPPED) & sbrk_mask, - data, (void)sbrk(-sbrk_size)); } if (events & (UCM_EVENT_MADVISE|UCM_EVENT_VM_UNMAPPED)) { @@ -183,11 +192,11 @@ void ucm_fire_mmap_events(int events) { ucm_mmap_test_events_data_t data; - ucm_fire_mmap_events_internal(events, &data); + ucm_fire_mmap_events_internal(events, &data, 0); } /* Called with lock held */ -static ucs_status_t ucm_mmap_test_events(int events) +static ucs_status_t ucm_mmap_test_events(int events, int exclusive) { ucm_event_handler_t handler; ucm_mmap_test_events_data_t data; @@ -199,7 +208,7 @@ static ucs_status_t ucm_mmap_test_events(int events) data.out_events = events; ucm_event_handler_add(&handler); - ucm_fire_mmap_events_internal(events, &data); + ucm_fire_mmap_events_internal(events, &data, exclusive); ucm_event_handler_remove(&handler); ucm_debug("mmap test: got 0x%x out of 0x%x", data.out_events, events); @@ -221,7 +230,7 @@ ucs_status_t ucm_mmap_test_installed_events(int events) * we don't check the status of events which were not successfully installed */ pthread_mutex_lock(&ucm_mmap_install_mutex); - status = ucm_mmap_test_events(events & ucm_mmap_installed_events); + status = ucm_mmap_test_events(events & ucm_mmap_installed_events, 0); pthread_mutex_unlock(&ucm_mmap_install_mutex); return status; @@ -250,30 +259,30 @@ static ucs_status_t ucs_mmap_install_reloc(int events) continue; } - if (UCM_IS_HOOK_ENABLED(entry)) { - ucm_debug("mmap: installing %s hook for %s = %p for event 0x%x", UCM_HOOK_STR, - entry->patch.symbol, entry->patch.value, entry->event_type); - - if (ucm_mmap_hook_mode() == UCM_MMAP_HOOK_RELOC) { - status = ucm_reloc_modify(&entry->patch); - } else { - ucs_assert(ucm_mmap_hook_mode() == UCM_MMAP_HOOK_BISTRO); - status = ucm_bistro_patch(entry->patch.symbol, entry->patch.value, NULL); - } - if (status != UCS_OK) { - ucm_warn("failed to install %s hook for '%s'", - UCM_HOOK_STR, entry->patch.symbol); - return status; - } - - installed_events |= entry->event_type; + ucm_debug("mmap: installing %s hook for %s = %p for event 0x%x", + UCM_HOOK_STR, entry->patch.symbol, entry->patch.value, + entry->event_type); + + if (ucm_mmap_hook_mode() == UCM_MMAP_HOOK_RELOC) { + status = ucm_reloc_modify(&entry->patch); + } else { + ucs_assert(ucm_mmap_hook_mode() == UCM_MMAP_HOOK_BISTRO); + status = ucm_bistro_patch(entry->patch.symbol, entry->patch.value, + NULL); + } + if (status != UCS_OK) { + ucm_warn("failed to install %s hook for '%s'", UCM_HOOK_STR, + entry->patch.symbol); + return status; } + + installed_events |= entry->event_type; } return UCS_OK; } -ucs_status_t ucm_mmap_install(int events) +ucs_status_t ucm_mmap_install(int events, int exclusive) { ucs_status_t status; @@ -283,7 +292,7 @@ ucs_status_t ucm_mmap_install(int events) /* if we already installed these events, check that they are still * working, and if not - reinstall them. */ - status = ucm_mmap_test_events(events); + status = ucm_mmap_test_events(events, exclusive); if (status == UCS_OK) { goto out_unlock; } @@ -295,7 +304,7 @@ ucs_status_t ucm_mmap_install(int events) goto out_unlock; } - status = ucm_mmap_test_events(events); + status = ucm_mmap_test_events(events, exclusive); if (status != UCS_OK) { ucm_debug("failed to install mmap events"); goto out_unlock; @@ -309,3 +318,26 @@ ucs_status_t ucm_mmap_install(int events) pthread_mutex_unlock(&ucm_mmap_install_mutex); return status; } + +void ucm_mmap_init() +{ + ucm_event_type_t native_events; + ucm_mmap_func_t *entry; + + if (!ucm_global_opts.enable_events || + (ucm_mmap_hook_mode() != UCM_MMAP_HOOK_BISTRO)) { + return; + } + + /* We must initialize bistro hooks during startup and not later, before + * other threads could execute the modified functions and fail on invalid + * instructions + */ + native_events = 0; + for (entry = ucm_mmap_funcs; entry->patch.symbol != NULL; ++entry) { + native_events |= entry->event_type; + } + + ucm_prevent_dl_unload(); + ucm_mmap_install(native_events, 1); +} diff --git a/src/ucm/mmap/mmap.h b/src/ucm/mmap/mmap.h index 58252de7dfc..fe0b4390eb0 100644 --- a/src/ucm/mmap/mmap.h +++ b/src/ucm/mmap/mmap.h @@ -21,7 +21,7 @@ # define UCM_DEFAULT_HOOK_MODE_STR UCM_MMAP_HOOK_RELOC_STR #endif -ucs_status_t ucm_mmap_install(int events); +ucs_status_t ucm_mmap_install(int events, int exclusive); void *ucm_override_mmap(void *addr, size_t length, int prot, int flags, int fd, off_t offset); int ucm_override_munmap(void *addr, size_t length); @@ -31,10 +31,11 @@ int ucm_override_shmdt(const void *shmaddr); void *ucm_override_sbrk(intptr_t increment); void *ucm_sbrk_select(intptr_t increment); int ucm_override_brk(void *addr); -void *ucm_brk_syscall(void *addr); int ucm_override_madvise(void *addr, size_t length, int advice); +void *ucm_get_current_brk(); void ucm_fire_mmap_events(int events); ucs_status_t ucm_mmap_test_installed_events(int events); +void ucm_mmap_init(); static UCS_F_ALWAYS_INLINE ucm_mmap_hook_mode_t ucm_mmap_hook_mode(void) { diff --git a/src/ucm/util/replace.c b/src/ucm/util/replace.c index 6d8abae9405..58a06325778 100644 --- a/src/ucm/util/replace.c +++ b/src/ucm/util/replace.c @@ -17,14 +17,20 @@ #include #include #include +#include #include #include #include + #ifndef MAP_FAILED #define MAP_FAILED ((void*)-1) #endif +#if HAVE___CURBRK +extern void *__curbrk; +#endif + #ifdef PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP pthread_mutex_t ucm_reloc_get_orig_lock = PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP; #else @@ -113,17 +119,8 @@ int ucm_orig_shmdt(const void *shmaddr) #endif -#if HAVE___CURBRK -extern void *__curbrk; -#endif - _UCM_DEFINE_DLSYM_FUNC(brk, ucm_orig_dlsym_brk, ucm_override_brk, int, -1, void*) -void *ucm_brk_syscall(void *addr) -{ - return (void*)syscall(SYS_brk, addr); -} - int ucm_orig_brk(void *addr) { void *new_addr; @@ -133,7 +130,7 @@ int ucm_orig_brk(void *addr) #endif new_addr = ucm_brk_syscall(addr); - if (new_addr < addr) { + if (new_addr != addr) { errno = ENOMEM; return -1; } else { @@ -151,15 +148,26 @@ void *ucm_orig_sbrk(intptr_t increment) if (ucm_mmap_hook_mode() == UCM_MMAP_HOOK_RELOC) { return ucm_orig_dlsym_sbrk(increment); } else { - prev = ucm_brk_syscall(0); - return ucm_orig_brk(UCS_PTR_BYTE_OFFSET(prev, increment)) ? (void*)-1 : prev; + prev = ucm_get_current_brk(); + return ucm_orig_brk(UCS_PTR_BYTE_OFFSET(prev, increment)) ? + (void*)-1 : prev; } } #else /* UCM_BISTRO_HOOKS */ +UCM_DEFINE_DLSYM_FUNC(brk, int, -1, void*) UCM_DEFINE_DLSYM_FUNC(sbrk, void*, MAP_FAILED, intptr_t) UCM_DEFINE_DLSYM_FUNC(shmat, void*, MAP_FAILED, int, const void*, int) UCM_DEFINE_DLSYM_FUNC(shmdt, int, -1, const void*) #endif /* UCM_BISTRO_HOOKS */ + +void *ucm_get_current_brk() +{ +#if HAVE___CURBRK + return __curbrk; +#else + return ucm_brk_syscall(0); +#endif +} diff --git a/src/ucm/util/sys.c b/src/ucm/util/sys.c index eebd58a190a..37a21b18493 100644 --- a/src/ucm/util/sys.c +++ b/src/ucm/util/sys.c @@ -17,10 +17,12 @@ #include #include #include +#include #include #include #include #include +#include #include #include #include @@ -282,33 +284,36 @@ void ucm_strerror(int eno, char *buf, size_t max) void ucm_prevent_dl_unload() { + static ucs_init_once_t init_once = UCS_INIT_ONCE_INITIALIZER; Dl_info info; void *dl; int ret; - /* Get the path to current library by current function pointer */ - (void)dlerror(); - ret = dladdr(ucm_prevent_dl_unload, &info); - if (ret == 0) { - ucm_warn("could not find address of current library: %s", dlerror()); - return; - } + UCS_INIT_ONCE(&init_once) { + /* Get the path to current library by current function pointer */ + (void)dlerror(); + ret = dladdr(ucm_prevent_dl_unload, &info); + if (ret == 0) { + ucm_warn("could not find address of current library: %s", dlerror()); + return; + } - /* Load the current library with NODELETE flag, to prevent it from being - * unloaded. This will create extra reference to the library, but also add - * NODELETE flag to the dynamic link map. - */ - (void)dlerror(); - dl = dlopen(info.dli_fname, RTLD_LOCAL|RTLD_LAZY|RTLD_NODELETE); - if (dl == NULL) { - ucm_warn("failed to load '%s': %s", info.dli_fname, dlerror()); - return; - } + /* Load the current library with NODELETE flag, to prevent it from being + * unloaded. This will create extra reference to the library, but also add + * NODELETE flag to the dynamic link map. + */ + (void)dlerror(); + dl = dlopen(info.dli_fname, RTLD_LOCAL|RTLD_LAZY|RTLD_NODELETE); + if (dl == NULL) { + ucm_warn("failed to load '%s': %s", info.dli_fname, dlerror()); + return; + } - ucm_debug("reloaded '%s' at %p with NODELETE flag", info.dli_fname, dl); + ucm_debug("loaded '%s' at %p with NODELETE flag", info.dli_fname, dl); - /* Now we drop our reference to the lib, and it won't be unloaded anymore */ - dlclose(dl); + /* coverity[overwrite_var] */ + dl = NULL; + } } char *ucm_concat_path(char *buffer, size_t max, const char *dir, const char *file) @@ -340,3 +345,20 @@ char *ucm_concat_path(char *buffer, size_t max, const char *dir, const char *fil return buffer; } + +void *ucm_brk_syscall(void *addr) +{ + void *result; + +#ifdef __x86_64__ + asm volatile("mov %1, %%rdi\n\t" + "mov $0xc, %%eax\n\t" + "syscall\n\t" + : "=a"(result) + : "m"(addr)); +#else + /* TODO implement 64-bit syscall for aarch64, ppc64le */ + result = (void*)syscall(SYS_brk, addr); +#endif + return result; +} diff --git a/src/ucm/util/sys.h b/src/ucm/util/sys.h index 37a1d927ef3..1384ef6664f 100644 --- a/src/ucm/util/sys.h +++ b/src/ucm/util/sys.h @@ -88,4 +88,17 @@ void ucm_prevent_dl_unload(); char *ucm_concat_path(char *buffer, size_t max, const char *dir, const char *file); +/** + * Perform brk() syscall + * + * @param addr Address to set as new program break. + * + * @return New program break. + * + * @note If the break could not be changed (for example, parameter was invalid + * or exceeds limits) the break remains unchanged. + */ +void *ucm_brk_syscall(void *addr); + + #endif diff --git a/src/ucs/config/ucm_opts.c b/src/ucs/config/ucm_opts.c index 663f722b187..cb4077724c3 100644 --- a/src/ucs/config/ucm_opts.c +++ b/src/ucs/config/ucm_opts.c @@ -87,6 +87,9 @@ UCS_CONFIG_REGISTER_TABLE(ucm_global_config_table, "UCM", UCM_CONFIG_PREFIX, ucm_global_config_t) UCS_STATIC_INIT { - (void)ucs_config_parser_fill_opts(&ucm_global_opts, ucm_global_config_table, - UCS_DEFAULT_ENV_PREFIX, UCM_CONFIG_PREFIX, 0); + ucm_global_config_t ucm_opts; + (void)ucs_config_parser_fill_opts(&ucm_opts, ucm_global_config_table, + UCS_DEFAULT_ENV_PREFIX, UCM_CONFIG_PREFIX, + 0); + ucm_library_init(&ucm_opts); } From 7f8fa94419be22fbc9977f962446485cf6fa8509 Mon Sep 17 00:00:00 2001 From: Yossi Itigin Date: Sun, 13 Dec 2020 17:18:14 +0200 Subject: [PATCH 4/5] UCM: Remove UCS dependencies - Define own ucm_assert() macro. - Define own ucm_get_tid() function. - Remove ucs_init_once_mutex_unlock function, use pthread_mutex_unlock() directly. --- src/ucm/bistro/bistro.c | 4 ++-- src/ucm/bistro/bistro_int.h | 2 +- src/ucm/bistro/bistro_ppc64.c | 8 ++++---- src/ucm/malloc/malloc_hook.c | 4 ++-- src/ucm/mmap/install.c | 2 +- src/ucm/util/log.c | 5 ----- src/ucm/util/log.h | 18 ++++++++++++++++++ src/ucm/util/sys.c | 5 +++++ src/ucm/util/sys.h | 6 ++++++ src/ucs/Makefile.am | 3 +-- src/ucs/type/init_once.c | 20 -------------------- src/ucs/type/init_once.h | 2 +- 12 files changed, 41 insertions(+), 38 deletions(-) delete mode 100644 src/ucs/type/init_once.c diff --git a/src/ucm/bistro/bistro.c b/src/ucm/bistro/bistro.c index 51a807e91d5..1eec37ee9ad 100644 --- a/src/ucm/bistro/bistro.c +++ b/src/ucm/bistro/bistro.c @@ -16,7 +16,7 @@ ucs_status_t ucm_bistro_remove_restore_point(ucm_bistro_restore_point_t *rp) { - ucs_assert(rp != NULL); + ucm_assert(rp != NULL); free(rp); return UCS_OK; } @@ -103,7 +103,7 @@ ucs_status_t ucm_bistro_restore(ucm_bistro_restore_point_t *rp) void *ucm_bistro_restore_addr(ucm_bistro_restore_point_t *rp) { - ucs_assert(rp != NULL); + ucm_assert(rp != NULL); return rp->addr; } diff --git a/src/ucm/bistro/bistro_int.h b/src/ucm/bistro/bistro_int.h index e6c08a4994a..021dbc10760 100644 --- a/src/ucm/bistro/bistro_int.h +++ b/src/ucm/bistro/bistro_int.h @@ -37,7 +37,7 @@ static inline void *ucm_bistro_lookup(const char *symbol) { void *addr; - ucs_assert(symbol != NULL); + ucm_assert(symbol != NULL); addr = dlsym(RTLD_NEXT, symbol); if (!addr) { diff --git a/src/ucm/bistro/bistro_ppc64.c b/src/ucm/bistro/bistro_ppc64.c index 4b14250cd97..8a08655b365 100644 --- a/src/ucm/bistro/bistro_ppc64.c +++ b/src/ucm/bistro/bistro_ppc64.c @@ -76,7 +76,7 @@ struct ucm_bistro_restore_point { static void ucm_bistro_fill_base_patch(ucm_bistro_base_patch_t *patch, uint32_t reg, uintptr_t value) { - ucs_assert(patch != NULL); + ucm_assert(patch != NULL); patch->addis = ADDIS ( reg, 0, (value >> 48)); patch->ori1 = ORI ( reg, reg, (value >> 32)); @@ -88,7 +88,7 @@ static void ucm_bistro_fill_base_patch(ucm_bistro_base_patch_t *patch, static void ucm_bistro_fill_patch(ucm_bistro_patch_t *patch, uint32_t reg, uintptr_t value) { - ucs_assert(patch != NULL); + ucm_assert(patch != NULL); ucm_bistro_fill_base_patch(&patch->super, reg, value); @@ -185,7 +185,7 @@ ucs_status_t ucm_bistro_restore(ucm_bistro_restore_point_t *rp) { ucs_status_t status; - ucs_assert(rp != NULL); + ucm_assert(rp != NULL); status = ucm_bistro_apply_patch(rp->func, &rp->func_patch, sizeof(rp->func_patch)); if (UCS_STATUS_IS_ERR(status)) { @@ -202,7 +202,7 @@ ucs_status_t ucm_bistro_restore(ucm_bistro_restore_point_t *rp) void *ucm_bistro_restore_addr(ucm_bistro_restore_point_t *rp) { - ucs_assert(rp != NULL); + ucm_assert(rp != NULL); return rp->entry; } diff --git a/src/ucm/malloc/malloc_hook.c b/src/ucm/malloc/malloc_hook.c index 13a3bf287db..7b8bac5163c 100644 --- a/src/ucm/malloc/malloc_hook.c +++ b/src/ucm/malloc/malloc_hook.c @@ -148,8 +148,8 @@ static void ucm_malloc_mmaped_ptr_add(void *ptr) hash_it = kh_put(mmap_ptrs, &ucm_malloc_hook_state.ptrs, ptr, &hash_extra_status); - ucs_assert_always(hash_extra_status >= 0); - ucs_assert_always(hash_it != kh_end(&ucm_malloc_hook_state.ptrs)); + ucm_assert_always(hash_extra_status >= 0); + ucm_assert_always(hash_it != kh_end(&ucm_malloc_hook_state.ptrs)); ucs_recursive_spin_unlock(&ucm_malloc_hook_state.lock); } diff --git a/src/ucm/mmap/install.c b/src/ucm/mmap/install.c index 2e5d42768b2..8d78b065b2e 100644 --- a/src/ucm/mmap/install.c +++ b/src/ucm/mmap/install.c @@ -266,7 +266,7 @@ static ucs_status_t ucs_mmap_install_reloc(int events) if (ucm_mmap_hook_mode() == UCM_MMAP_HOOK_RELOC) { status = ucm_reloc_modify(&entry->patch); } else { - ucs_assert(ucm_mmap_hook_mode() == UCM_MMAP_HOOK_BISTRO); + ucm_assert(ucm_mmap_hook_mode() == UCM_MMAP_HOOK_BISTRO); status = ucm_bistro_patch(entry->patch.symbol, entry->patch.value, NULL); } diff --git a/src/ucm/util/log.c b/src/ucm/util/log.c index d15d165ac5f..6fe2742a76c 100644 --- a/src/ucm/util/log.c +++ b/src/ucm/util/log.c @@ -245,11 +245,6 @@ static void ucm_log_vsnprintf(char *buf, size_t max, const char *fmt, va_list ap *pb = '\0'; } -static pid_t ucm_get_tid(void) -{ - return syscall(SYS_gettid); -} - static void ucm_log_snprintf(char *buf, size_t max, const char *fmt, ...) { va_list ap; diff --git a/src/ucm/util/log.h b/src/ucm/util/log.h index 6ba8b468895..f78bf5c693f 100644 --- a/src/ucm/util/log.h +++ b/src/ucm/util/log.h @@ -22,6 +22,7 @@ ## __VA_ARGS__); \ } + #define ucm_fatal(_message, ...) ucm_log(UCS_LOG_LEVEL_FATAL, _message, ## __VA_ARGS__) #define ucm_error(_message, ...) ucm_log(UCS_LOG_LEVEL_ERROR, _message, ## __VA_ARGS__) #define ucm_warn(_message, ...) ucm_log(UCS_LOG_LEVEL_WARN, _message, ## __VA_ARGS__) @@ -29,8 +30,25 @@ #define ucm_debug(_message, ...) ucm_log(UCS_LOG_LEVEL_DEBUG, _message, ## __VA_ARGS__) #define ucm_trace(_message, ...) ucm_log(UCS_LOG_LEVEL_TRACE, _message, ## __VA_ARGS__) + +#define ucm_assert_always(_expression) \ + do { \ + if (!(_expression)) { \ + ucm_fatal("Assertion `%s' failed", #_expression); \ + } \ + } while (0) + + +#if ENABLE_ASSERT +# define ucm_assert(...) ucm_assert_always(__VA_ARGS__) +#else +# define ucm_assert(...) {} +#endif + + extern const char *ucm_log_level_names[]; + void __ucm_log(const char *file, unsigned line, const char *function, ucs_log_level_t level, const char *message, ...) UCS_F_PRINTF(5, 6); diff --git a/src/ucm/util/sys.c b/src/ucm/util/sys.c index 37a21b18493..325ed103dca 100644 --- a/src/ucm/util/sys.c +++ b/src/ucm/util/sys.c @@ -362,3 +362,8 @@ void *ucm_brk_syscall(void *addr) #endif return result; } + +pid_t ucm_get_tid() +{ + return syscall(SYS_gettid); +} diff --git a/src/ucm/util/sys.h b/src/ucm/util/sys.h index 1384ef6664f..215de109ef7 100644 --- a/src/ucm/util/sys.h +++ b/src/ucm/util/sys.h @@ -8,6 +8,7 @@ #ifndef UCM_UTIL_SYS_H_ #define UCM_UTIL_SYS_H_ +#include #include @@ -101,4 +102,9 @@ char *ucm_concat_path(char *buffer, size_t max, const char *dir, const char *fil void *ucm_brk_syscall(void *addr); +/** + * @return System thread id of the current thread. + */ +pid_t ucm_get_tid(); + #endif diff --git a/src/ucs/Makefile.am b/src/ucs/Makefile.am index 6386d089cb9..b61a4df4bb2 100644 --- a/src/ucs/Makefile.am +++ b/src/ucs/Makefile.am @@ -157,8 +157,7 @@ libucs_la_SOURCES = \ time/timer_wheel.c \ time/timerq.c \ type/class.c \ - type/status.c \ - type/init_once.c + type/status.c if HAVE_AARCH64_THUNDERX2 libucs_la_SOURCES += \ diff --git a/src/ucs/type/init_once.c b/src/ucs/type/init_once.c deleted file mode 100644 index cfb05c9cb8c..00000000000 --- a/src/ucs/type/init_once.c +++ /dev/null @@ -1,20 +0,0 @@ -/** - * Copyright (C) Mellanox Technologies Ltd. 2001-2019. ALL RIGHTS RESERVED. - * - * See file LICENSE for terms. - */ - -#ifdef HAVE_CONFIG_H -# include "config.h" -#endif - -#include -#include - - -unsigned ucs_init_once_mutex_unlock(pthread_mutex_t *lock) -{ - int ret = pthread_mutex_unlock(lock); - ucs_assert_always(ret == 0); - return 0; -} diff --git a/src/ucs/type/init_once.h b/src/ucs/type/init_once.h index 4b7e967ccbc..fc75104f19f 100644 --- a/src/ucs/type/init_once.h +++ b/src/ucs/type/init_once.h @@ -51,7 +51,7 @@ unsigned ucs_init_once_mutex_unlock(pthread_mutex_t *lock); */ #define UCS_INIT_ONCE(_once) \ for (pthread_mutex_lock(&(_once)->lock); \ - !(_once)->initialized || ucs_init_once_mutex_unlock(&(_once)->lock); \ + !(_once)->initialized || pthread_mutex_unlock(&(_once)->lock); \ (_once)->initialized = 1) #endif From 8e41b3629a7926efb1d776faafb84a750a6c0329 Mon Sep 17 00:00:00 2001 From: Yossi Itigin Date: Sun, 13 Dec 2020 19:23:31 +0200 Subject: [PATCH 5/5] TEST/MEMHOOKS: Set UCM configuration after loading --- contrib/test_jenkins.sh | 21 +++++---- test/mpi/test_memhooks.c | 94 ++++++++++++++++++++++++++++++---------- 2 files changed, 85 insertions(+), 30 deletions(-) diff --git a/contrib/test_jenkins.sh b/contrib/test_jenkins.sh index 3cb54c70cd8..775eaad00e9 100755 --- a/contrib/test_jenkins.sh +++ b/contrib/test_jenkins.sh @@ -1091,16 +1091,21 @@ run_ucx_perftest() { # Test malloc hooks with mpi # test_malloc_hooks_mpi() { - for tname in malloc_hooks malloc_hooks_unmapped external_events flag_no_install + for mode in reloc bistro do - echo "==== Running memory hook (${tname}) on MPI ====" - $MPIRUN -np 1 $AFFINITY ./test/mpi/test_memhooks -t $tname - done + for tname in malloc_hooks malloc_hooks_unmapped external_events flag_no_install + do + echo "==== Running memory hook (${tname} mode ${mode}) on MPI ====" + $MPIRUN -np 1 $AFFINITY \ + ./test/mpi/test_memhooks -t $tname -m ${mode} + done - echo "==== Running memory hook (malloc_hooks) on MPI with LD_PRELOAD ====" - ucm_lib=$PWD/src/ucm/.libs/libucm.so - ls -l $ucm_lib - $MPIRUN -np 1 -x LD_PRELOAD=$ucm_lib $AFFINITY ./test/mpi/test_memhooks -t malloc_hooks + echo "==== Running memory hook (malloc_hooks mode ${mode}) on MPI with LD_PRELOAD ====" + ucm_lib=$PWD/src/ucm/.libs/libucm.so + ls -l $ucm_lib + $MPIRUN -np 1 -x LD_PRELOAD=$ucm_lib $AFFINITY \ + ./test/mpi/test_memhooks -t malloc_hooks -m ${mode} + done } # diff --git a/test/mpi/test_memhooks.c b/test/mpi/test_memhooks.c index e6366b14089..becbf5f4414 100644 --- a/test/mpi/test_memhooks.c +++ b/test/mpi/test_memhooks.c @@ -39,22 +39,22 @@ #define SHMAT_FAILED ((void*)-1) -void* open_dyn_lib(const char *lib_path); -void* flag_no_install_init(const char *path); +void *event_init(const char *path, ucm_mmap_hook_mode_t mmap_mode); +void *ext_event_init(const char *path, ucm_mmap_hook_mode_t mmap_mode); +void* flag_no_install_init(const char *path, ucm_mmap_hook_mode_t mmap_mode); int malloc_hooks_run_all(void *dl); int malloc_hooks_run_unmapped(void *dl); int ext_event_run(void *dl); -void *ext_event_init(const char *path); typedef struct memtest_type { const char *name; - void* (*init)(const char *path); + void* (*init)(const char *path, ucm_mmap_hook_mode_t mmap_mode); int (*run) (void *arg); } memtest_type_t; memtest_type_t tests[] = { - {"malloc_hooks", open_dyn_lib, malloc_hooks_run_all}, - {"malloc_hooks_unmapped", open_dyn_lib, malloc_hooks_run_unmapped}, + {"malloc_hooks", event_init, malloc_hooks_run_all}, + {"malloc_hooks_unmapped", event_init, malloc_hooks_run_unmapped}, {"external_events", ext_event_init, ext_event_run}, {"flag_no_install", flag_no_install_init, ext_event_run}, {NULL} @@ -72,6 +72,9 @@ static void usage() { printf(" malloc_hooks_unmapped : Test VM_UNMAPPED event only\n"); printf(" external_events : Test of ucm_set_external_event() API\n"); printf(" flag_no_install : Test of UCM_EVENT_FLAG_NO_INSTALL flag\n"); + printf(" -m Memory hooks mode (bistro)\n"); + printf(" reloc : Change .plt/.got tables\n"); + printf(" bistro : Binary code patching\n"); printf("\n"); } @@ -96,10 +99,27 @@ static ucs_status_t set_event_handler(void *dl, int events) return set_handler(events, 0, event_callback, NULL); } -static ucs_status_t disable_memory_hooks(void *dl) +static ucs_status_t init_ucm_config(void *dl_ucm, int enable_hooks, + ucm_mmap_hook_mode_t mmap_mode) { - setenv("UCX_MEM_MALLOC_HOOKS", "n", 1); - setenv("UCX_MEM_MMAP_RELOC", "n", 1); + void (*library_init)(const ucm_global_config_t *ucm_opts); + ucm_global_config_t *ucm_opts; + + DL_FIND_FUNC(dl_ucm, "ucm_library_init", library_init, + return UCS_ERR_NO_ELEM); + DL_FIND_FUNC(dl_ucm, "ucm_global_opts", ucm_opts, + return UCS_ERR_NO_ELEM); + + if (enable_hooks) { + ucm_opts->mmap_hook_mode = mmap_mode; + } else { + ucm_opts->enable_malloc_hooks = 0; + ucm_opts->enable_malloc_reloc = 0; + ucm_opts->mmap_hook_mode = UCM_MMAP_HOOK_NONE; + } + + library_init(NULL); + return UCS_OK; } @@ -116,8 +136,27 @@ void* open_dyn_lib(const char *lib_path) return dl; } +void *event_init(const char *path, ucm_mmap_hook_mode_t mmap_mode) +{ + ucs_status_t status; + void *dl_ucm; + + dl_ucm = open_dyn_lib(path); + if (dl_ucm == NULL) { + return NULL; + } + + status = init_ucm_config(dl_ucm, 1, mmap_mode); + CHKERR_JUMP(status != UCS_OK, "Failed to initialize UCM", fail); + + return dl_ucm; -void *ext_event_init(const char *path) +fail: + dlclose(dl_ucm); + return NULL; +} + +void *ext_event_init(const char *path, ucm_mmap_hook_mode_t mmap_mode) { void (*set_ext_event)(int events); ucs_status_t status; @@ -128,8 +167,8 @@ void *ext_event_init(const char *path) return NULL; } - status = disable_memory_hooks(dl_ucm); - CHKERR_JUMP(status != UCS_OK, "Failed to disable memory hooks", fail); + status = init_ucm_config(dl_ucm, 0, mmap_mode); + CHKERR_JUMP(status != UCS_OK, "Failed to initialize UCM", fail); DL_FIND_FUNC(dl_ucm, "ucm_set_external_event", set_ext_event, goto fail); set_ext_event(UCM_EVENT_VM_MAPPED | UCM_EVENT_VM_UNMAPPED); @@ -145,7 +184,7 @@ void *ext_event_init(const char *path) return NULL; } -void* flag_no_install_init(const char *path) +void* flag_no_install_init(const char *path, ucm_mmap_hook_mode_t mmap_mode) { void *dl_ucm; ucs_status_t status; @@ -155,8 +194,8 @@ void* flag_no_install_init(const char *path) return NULL; } - status = disable_memory_hooks(dl_ucm); - CHKERR_JUMP(status != UCS_OK, "Failed to disable memory hooks", fail); + status = init_ucm_config(dl_ucm, 0, mmap_mode); + CHKERR_JUMP(status != UCS_OK, "Failed to initialize UCM", fail); status = set_event_handler(dl_ucm, UCM_EVENT_VM_MAPPED | UCM_EVENT_VM_UNMAPPED | @@ -380,14 +419,14 @@ int ext_event_run(void *dl) total_mapped = 0; ptr_direct_mmap = mmap(NULL, size, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANON, -1, 0); - printf("totmapped %lu\n", total_mapped); + printf("total_mapped=%lu\n", total_mapped); /* No callback should be called as we registered events to be external */ CHKERR_JUMP(total_mapped != 0, "Callback for mmap invoked, while hooks were not set", fail); DL_FIND_FUNC(dl, "ucm_vm_mmap", ucm_event, goto fail); ucm_event(ptr_direct_mmap, size); CHKERR_JUMP(total_mapped == 0, "Callback for mmap is not called", fail); - printf("After ucm_vm_mmap called: mapped=%zu\n", total_mapped); + printf("After ucm_vm_mmap called: total_mapped=%zu\n", total_mapped); /* Call munmap directly */ total_unmapped = 0; @@ -398,7 +437,7 @@ int ext_event_run(void *dl) DL_FIND_FUNC(dl, "ucm_vm_munmap", ucm_event, goto fail); ucm_event(ptr_direct_mmap, size); CHKERR_JUMP(total_unmapped == 0, "Callback for mmap is not called", fail); - printf("After ucm_vm_munmap: unmapped=%zu\n", total_unmapped); + printf("After ucm_vm_munmap: total_unmapped=%zu\n", total_unmapped); ret = 0; @@ -409,13 +448,14 @@ int ext_event_run(void *dl) int main(int argc, char **argv) { - const char *ucm_path = UCS_PP_MAKE_STRING(UCM_LIB_DIR) "/" "libucm.so"; - memtest_type_t *test = tests; + const char *ucm_path = UCS_PP_MAKE_STRING(UCM_LIB_DIR)"/libucm.so"; + memtest_type_t *test = tests; + ucm_mmap_hook_mode_t mmap_mode = UCM_MMAP_HOOK_BISTRO; void *dl; int ret; int c; - while ((c = getopt(argc, argv, "t:h")) != -1) { + while ((c = getopt(argc, argv, "t:m:h")) != -1) { switch (c) { case 't': for (test = tests; test->name != NULL; ++test) { @@ -428,6 +468,16 @@ int main(int argc, char **argv) return -1; } break; + case 'm': + if (!strcasecmp(optarg, "bistro")) { + mmap_mode = UCM_MMAP_HOOK_BISTRO; + } else if (!strcasecmp(optarg, "reloc")) { + mmap_mode = UCM_MMAP_HOOK_RELOC; + } else { + fprintf(stderr, "Wrong mmap mode %s\n", optarg); + return -1; + } + break; case 'h': default: usage(); @@ -437,7 +487,7 @@ int main(int argc, char **argv) /* Some tests need to modify UCM config before to call ucp_init, * which may be called by MPI_Init */ - dl = test->init(ucm_path); + dl = test->init(ucm_path, mmap_mode); if (dl == NULL) { return -1; }