diff --git a/.github/workflows/dynamic_arch.yml b/.github/workflows/dynamic_arch.yml index 67919abe49..f7d63d8c8d 100644 --- a/.github/workflows/dynamic_arch.yml +++ b/.github/workflows/dynamic_arch.yml @@ -593,7 +593,7 @@ jobs: - name: Build OpenBLAS run: | cd build - cmake --build . --target dgemm_thread_safety dgemm_thread_safety_mixed dgemv_thread_safety + cmake --build . --target dgemm_thread_safety dgemm_thread_safety_mixed dgemm_thread_safety_shutdown dgemv_thread_safety - name: Show ccache status continue-on-error: true @@ -611,7 +611,7 @@ jobs: run: | cd build export PATH="$PWD/lib:$PATH" - OPENBLAS_NUM_THREADS=8 OMP_NUM_THREADS=16 ctest -R 'dgemm_thread_safety|dgemm_thread_safety_mixed|dgemv_thread_safety' --output-on-failure + OPENBLAS_NUM_THREADS=8 OMP_NUM_THREADS=16 ctest -R 'dgemm_thread_safety|dgemm_thread_safety_mixed|dgemm_thread_safety_shutdown|dgemv_thread_safety' --output-on-failure cross_build: diff --git a/cpp_thread_test/CMakeLists.txt b/cpp_thread_test/CMakeLists.txt index 13307611e0..c81aacd836 100644 --- a/cpp_thread_test/CMakeLists.txt +++ b/cpp_thread_test/CMakeLists.txt @@ -19,6 +19,7 @@ endif() set(CPP_THREAD_SAFETY_DGEMM_ARGS "" CACHE STRING "Arguments passed to the DGEMM thread safety test") set(CPP_THREAD_SAFETY_DGEMM_MIXED_ARGS "" CACHE STRING "Arguments passed to the mixed DGEMM thread safety test") set(CPP_THREAD_SAFETY_DGEMV_ARGS "" CACHE STRING "Arguments passed to the DGEMV thread safety test") +set(CPP_THREAD_SAFETY_SHUTDOWN_ARGS "" CACHE STRING "Arguments passed to the DGEMM shutdown safety test") if (CPP_THREAD_SAFETY_TEST) message(STATUS "building thread safety test") @@ -30,6 +31,12 @@ if (CPP_THREAD_SAFETY_TEST) target_link_libraries(dgemm_thread_safety_mixed ${CPP_THREAD_SAFETY_LIBS}) add_test(NAME dgemm_thread_safety_mixed COMMAND ${CMAKE_CURRENT_BINARY_DIR}/dgemm_thread_safety_mixed ${CPP_THREAD_SAFETY_DGEMM_MIXED_ARGS}) + if (WIN32) + add_executable(dgemm_thread_safety_shutdown dgemm_thread_safety_shutdown.cpp) + target_link_libraries(dgemm_thread_safety_shutdown ${CPP_THREAD_SAFETY_LIBS}) + add_test(NAME dgemm_thread_safety_shutdown COMMAND ${CMAKE_CURRENT_BINARY_DIR}/dgemm_thread_safety_shutdown ${CPP_THREAD_SAFETY_SHUTDOWN_ARGS}) + set_tests_properties(dgemm_thread_safety_shutdown PROPERTIES TIMEOUT 900) + endif() if (USE_THREAD AND (USE_OPENMP OR (NOT WIN32 AND NOT CYGWIN))) add_test(NAME dgemm_thread_safety_mixed_callback COMMAND ${CMAKE_CURRENT_BINARY_DIR}/dgemm_thread_safety_mixed ${CPP_THREAD_SAFETY_DGEMM_MIXED_ARGS} --callback) endif() diff --git a/cpp_thread_test/Makefile b/cpp_thread_test/Makefile index 8c2d347b00..56c4759d0a 100644 --- a/cpp_thread_test/Makefile +++ b/cpp_thread_test/Makefile @@ -3,6 +3,10 @@ include $(TOPDIR)/Makefile.system all :: dgemv_tester dgemm_tester dgemm_mixed_tester +ifeq ($(OSNAME), WINNT) +all :: dgemm_shutdown_tester +endif + CALLBACK_THREAD_TEST = ifeq ($(SMP),1) ifeq ($(USE_OPENMP),1) @@ -31,10 +35,14 @@ dgemm_mixed_tester : dgemm_tester $(CXX) $(COMMON_OPT) -Wall -Wextra -Wshadow -std=c++11 dgemm_thread_safety_mixed.cpp ../$(LIBNAME) $(EXTRALIB) $(FEXTRALIB) -o dgemm_mixed_tester ./dgemm_mixed_tester +dgemm_shutdown_tester : dgemm_mixed_tester + $(CXX) $(COMMON_OPT) -Wall -Wextra -Wshadow -std=c++11 dgemm_thread_safety_shutdown.cpp ../$(LIBNAME) $(EXTRALIB) $(FEXTRALIB) -o dgemm_shutdown_tester + ./dgemm_shutdown_tester + ifeq ($(CALLBACK_THREAD_TEST),1) dgemm_mixed_callback_tester : dgemm_mixed_tester ./dgemm_mixed_tester --callback endif clean :: - rm -f dgemv_tester dgemm_tester dgemm_mixed_tester + rm -f dgemv_tester dgemm_tester dgemm_mixed_tester dgemm_shutdown_tester diff --git a/cpp_thread_test/dgemm_thread_safety_shutdown.cpp b/cpp_thread_test/dgemm_thread_safety_shutdown.cpp new file mode 100644 index 0000000000..2b24aa640a --- /dev/null +++ b/cpp_thread_test/dgemm_thread_safety_shutdown.cpp @@ -0,0 +1,165 @@ +/* Stress test for library shutdown racing with in-flight BLAS calls + * (https://github.com/OpenMathLib/OpenBLAS/issues/5954). + * + * Windows only. On POSIX, exit() runs the library destructor while worker + * threads are still computing into OpenBLAS-owned buffers, which no amount of + * locking inside blas_shutdown can make safe, so there is nothing to assert + * there; CMakeLists.txt only registers this test on WIN32. + * + * The parent re-executes itself as short-lived children and checks that each + * one terminates cleanly, turning shutdown-path crashes and deadlocks into + * ordinary test failures. Each child (--child-storm N) starts N callers that + * allocate their matrices and park on a gate, releases them so they all enter + * their first dgemm at once, and exits a millisecond later while that + * allocation storm is still in flight. + * + * N must exceed NUM_BUFFERS = MAX(50, NUM_THREADS * 2 * NUM_PARALLEL) for the + * build under test; below that every slot is already mapped and the race is + * unreachable. + */ +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#ifdef OPENBLAS_USE_GENERATED_CBLAS_H +#include "generated/cblas.h" +#else +#include "../cblas.h" +#endif + +#include + +namespace { + +const blasint stormM = 200, stormK = 120, stormN = 90; /* the gh-5954 shape */ +const blasint poolDim = 320; /* above the multithreading threshold, so the pool spins up */ +const uint32_t defaultStormCallers = 128; +const uint32_t stormDelayMs = 3; /* gate to sweep; at 0 the sweep beats the allocations */ +const int stormBlasThreads = 4; +const int stormTimeoutSec = 15; +const int numStormChildren = 40; + +std::atomic parked(0); /* callers built and waiting on the gate */ +std::atomic gate(false); + +void fillOperands(std::vector& A, std::vector& B) { + for (size_t i = 0; i < A.size(); i++) A[i] = (i % 1000) / 1000.0; + for (size_t i = 0; i < B.size(); i++) B[i] = (i % 997) / 997.0; +} + +void dgemmOnce(blasint m, blasint k, blasint n) { + std::vector A(m * k), B(k * n), C(m * n); + fillOperands(A, B); + cblas_dgemm(CblasColMajor, CblasNoTrans, CblasNoTrans, m, n, k, + 1.0, A.data(), m, B.data(), k, 0.1, C.data(), m); +} + +/* Allocate before parking, so that when the gate opens nothing stands between + the thread and its first dgemm. */ +void gatedWorker(blasint m, blasint k, blasint n) { + std::vector A(m * k), B(k * n), C(m * n); + fillOperands(A, B); + parked.fetch_add(1, std::memory_order_release); + while (!gate.load(std::memory_order_acquire)) std::this_thread::yield(); + for (;;) + cblas_dgemm(CblasColMajor, CblasNoTrans, CblasNoTrans, m, n, k, + 1.0, A.data(), m, B.data(), k, 0.1, C.data(), m); +} + +int ChildStorm(uint32_t nCallers) { + SetErrorMode(SEM_FAILCRITICALERRORS | SEM_NOGPFAULTERRORBOX); + openblas_set_num_threads(stormBlasThreads); + + /* Build the OpenBLAS worker pool first, so the storm is buffer allocation + and not pool startup. */ + dgemmOnce(poolDim, poolDim, poolDim); + + for (uint32_t i = 0; i < nCallers; i++) + std::thread(gatedWorker, stormM, stormK, stormN).detach(); + for (int ms = 0; parked.load(std::memory_order_acquire) < nCallers && ms < 10000; ms++) + std::this_thread::sleep_for(std::chrono::milliseconds(1)); + + gate.store(true, std::memory_order_release); + std::this_thread::sleep_for(std::chrono::milliseconds(stormDelayMs)); + std::exit(0); +} + +/* Returns 0 if the child exited cleanly, nonzero otherwise; fills outcome. */ +int RunChild(const std::string& args, int timeoutSec, std::string& outcome) { + char exe[MAX_PATH]; + if (GetModuleFileNameA(NULL, exe, MAX_PATH) == 0) { + outcome = "GetModuleFileName failed"; + return 1; + } + std::string cmd = "\"" + std::string(exe) + "\" " + args; + + STARTUPINFOA si; + PROCESS_INFORMATION pi; + ZeroMemory(&si, sizeof(si)); + si.cb = sizeof(si); + ZeroMemory(&pi, sizeof(pi)); + if (!CreateProcessA(NULL, &cmd[0], NULL, NULL, FALSE, 0, NULL, NULL, &si, &pi)) { + outcome = "CreateProcess failed"; + return 1; + } + + int ret = 1; + char buf[64]; + if (WaitForSingleObject(pi.hProcess, timeoutSec * 1000) != WAIT_OBJECT_0) { + TerminateProcess(pi.hProcess, 1); + WaitForSingleObject(pi.hProcess, 5000); + snprintf(buf, sizeof(buf), "HANG (killed after %ds)", timeoutSec); + } else { + DWORD code = 1; + GetExitCodeProcess(pi.hProcess, &code); + if (code == 0) { + snprintf(buf, sizeof(buf), "clean exit"); + ret = 0; + } else { + snprintf(buf, sizeof(buf), "CRASH (exit code 0x%08lX)", (unsigned long)code); + } + } + outcome = buf; + + CloseHandle(pi.hThread); + CloseHandle(pi.hProcess); + return ret; +} + +} // namespace + +int main(int argc, char* argv[]) { + if (argc >= 3 && std::strcmp(argv[1], "--child-storm") == 0) + return ChildStorm(uint32_t(std::atoi(argv[2]))); + SetErrorMode(SEM_FAILCRITICALERRORS | SEM_NOGPFAULTERRORBOX); + + uint32_t callers = defaultStormCallers; + if (argc >= 2) { + int n = std::atoi(argv[1]); + if (n > 0) callers = uint32_t(n); + } + + int failures = 0; + std::cout << "Testing process exit during an allocation storm (" << callers << " callers)" + << std::endl; + for (int i = 0; i < numStormChildren; i++) { + std::string outcome; + failures += RunChild("--child-storm " + std::to_string(callers), stormTimeoutSec, outcome); + std::cout << " storm child " << i << ": " << outcome << std::endl; + } + + if (failures) { + std::cout << "CBLAS DGEMM shutdown safety test FAILED! (" << failures + << " child processes)" << std::endl; + return 1; + } + std::cout << "CBLAS DGEMM shutdown safety test PASSED!" << std::endl; + return 0; +} diff --git a/driver/others/memory.c b/driver/others/memory.c index a28edeaa63..8e24773841 100644 --- a/driver/others/memory.c +++ b/driver/others/memory.c @@ -2099,7 +2099,7 @@ int openblas_get_num_threads(void) { struct release_t { void *address; - void (*func)(struct release_t *); + void (* _Atomic func)(struct release_t *); long attr; }; @@ -2123,6 +2123,37 @@ static pthread_spinlock_t alloc_lock = 0; static BLASULONG alloc_lock = 0UL; #endif +static void blas_release_register(void *address, void (*func)(struct release_t *), long attr) { + + struct release_t *release; + int rpos; + +#if (defined(SMP) || defined(USE_LOCKING)) && !defined(USE_OPENMP) + LOCK_COMMAND(&alloc_lock); +#endif +#if defined(HAVE_C11) && !defined(__cplusplus) + rpos = atomic_fetch_add(&release_pos, 1); +#elif defined(__GNUC__) + rpos = __sync_fetch_and_add(&release_pos, 1); +#elif defined(OS_WINDOWS) + rpos = InterlockedIncrement((LONG volatile *)&release_pos) - 1; +#else + rpos = release_pos++; +#endif + if (likely(rpos < NUM_BUFFERS)) { + release = &release_info[rpos]; + } else { + release = &new_release_info[rpos - NUM_BUFFERS]; + } + release->address = address; + release->attr = attr; + WMB; + release->func = func; +#if (defined(SMP) || defined(USE_LOCKING)) && !defined(USE_OPENMP) + UNLOCK_COMMAND(&alloc_lock); +#endif +} + #ifdef ALLOC_MMAP static void alloc_mmap_free(struct release_t *release){ @@ -2154,20 +2185,7 @@ static void *alloc_mmap(void *address){ } if (map_address != (void *)-1) { -#if (defined(SMP) || defined(USE_LOCKING)) && !defined(USE_OPENMP) - LOCK_COMMAND(&alloc_lock); -#endif - int rpos = release_pos++; - if (likely(rpos < NUM_BUFFERS)) { - release_info[rpos].address = map_address; - release_info[rpos].func = alloc_mmap_free; - } else { - new_release_info[rpos-NUM_BUFFERS].address = map_address; - new_release_info[rpos-NUM_BUFFERS].func = alloc_mmap_free; - } -#if (defined(SMP) || defined(USE_LOCKING)) && !defined(USE_OPENMP) - UNLOCK_COMMAND(&alloc_lock); -#endif + blas_release_register(map_address, alloc_mmap_free, 0); } else { #ifdef DEBUG int errsv=errno; @@ -2323,20 +2341,7 @@ static void *alloc_mmap(void *address){ #endif if (map_address != (void *)-1) { -#if (defined(SMP) || defined(USE_LOCKING)) && !defined(USE_OPENMP) - LOCK_COMMAND(&alloc_lock); -#endif - int rpos = release_pos++; - if (likely(rpos < NUM_BUFFERS)) { - release_info[rpos].address = map_address; - release_info[rpos].func = alloc_mmap_free; - } else { - new_release_info[rpos-NUM_BUFFERS].address = map_address; - new_release_info[rpos-NUM_BUFFERS].func = alloc_mmap_free; - } -#if (defined(SMP) || defined(USE_LOCKING)) && !defined(USE_OPENMP) - UNLOCK_COMMAND(&alloc_lock); -#endif + blas_release_register(map_address, alloc_mmap_free, 0); } return map_address; @@ -2364,14 +2369,7 @@ static void *alloc_malloc(void *address){ if (map_address == (void *)NULL) map_address = (void *)-1; if (map_address != (void *)-1) { - int rpos = release_pos++; - if (likely(rpos < NUM_BUFFERS)) { - release_info[rpos].address = map_address; - release_info[rpos].func = alloc_malloc_free; - } else { - new_release_info[rpos-NUM_BUFFERS].address = map_address; - new_release_info[rpos-NUM_BUFFERS].func = alloc_malloc_free; - } + blas_release_register(map_address, alloc_malloc_free, 0); } return map_address; @@ -2403,14 +2401,7 @@ static void *alloc_qalloc(void *address){ if (map_address == (void *)NULL) map_address = (void *)-1; if (map_address != (void *)-1) { - int rpos = release_pos++; - if (likely(rpos < NUM_BUFFERS)) { - release_info[rpos].address = map_address; - release_info[rpos].func = alloc_qalloc_free; - } else { - new_release_info[rpos-NUM_BUFFERS].address = map_address; - new_release_info[rpos-NUM_BUFFERS].func = alloc_qalloc_free; - } + blas_release_register(map_address, alloc_qalloc_free, 0); } return (void *)(((BLASULONG)map_address + FIXED_PAGESIZE - 1) & ~(FIXED_PAGESIZE - 1)); @@ -2437,14 +2428,7 @@ static void *alloc_windows(void *address){ if (map_address == (void *)NULL) map_address = (void *)-1; if (map_address != (void *)-1) { - int rpos = release_pos++; - if (likely(rpos < NUM_BUFFERS)) { - release_info[rpos].address = map_address; - release_info[rpos].func = alloc_windows_free; - } else { - new_release_info[rpos-NUM_BUFFERS].address = map_address; - new_release_info[rpos-NUM_BUFFERS].func = alloc_windows_free; - } + blas_release_register(map_address, alloc_windows_free, 0); } return map_address; @@ -2486,16 +2470,7 @@ static void *alloc_devicedirver(void *address){ fd, 0); if (map_address != (void *)-1) { - int rpos = release_pos++; - if (likely(rpos < NUM_BUFFERS)) { - release_info[rpos].address = map_address; - release_info[rpos].attr = fd; - release_info[rpos].func = alloc_devicedirver_free; - } else { - new_release_info[rpos-NUM_BUFFERS].address = map_address; - new_release_info[rpos-NUM_BUFFERS].attr = fd; - new_release_info[rpos-NUM_BUFFERS].func = alloc_devicedirver_free; - } + blas_release_register(map_address, alloc_devicedirver_free, fd); } return map_address; @@ -2530,16 +2505,7 @@ static void *alloc_shm(void *address){ shmctl(shmid, IPC_RMID, 0); - int rpos = release_pos++; - if (likely(rpos < NUM_BUFFERS)) { - release_info[rpos].address = map_address; - release_info[rpos].attr = shmid; - release_info[rpos].func = alloc_shm_free; - } else { - new_release_info[rpos-NUM_BUFFERS].address = map_address; - new_release_info[rpos-NUM_BUFFERS].attr = shmid; - new_release_info[rpos-NUM_BUFFERS].func = alloc_shm_free; - } + blas_release_register(map_address, alloc_shm_free, shmid); } return map_address; @@ -2647,14 +2613,7 @@ fprintf(stderr,"alloc_hugetlb got called\n"); #endif if (map_address != (void *)-1){ - int rpos = release_pos++; - if (likely(rpos < NUM_BUFFERS)) { - release_info[rpos].address = map_address; - release_info[rpos].func = alloc_hugetlb_free; - } else { - new_release_info[rpos-NUM_BUFFERS].address = map_address; - new_release_info[rpos-NUM_BUFFERS].func = alloc_hugetlb_free; - } + blas_release_register(map_address, alloc_hugetlb_free, 0); } return map_address; @@ -2699,16 +2658,7 @@ static void *alloc_hugetlbfile(void *address){ fd, 0); if (map_address != (void *)-1) { - int rpos = release_pos++; - if (likely(rpos < NUM_BUFFERS)) { - release_info[rpos].address = map_address; - release_info[rpos].attr = fd; - release_info[rpos].func = alloc_hugetlbfile_free; - } else { - new_release_info[rpos-NUM_BUFFERS].address = map_address; - new_release_info[rpos-NUM_BUFFERS].attr = fd; - new_release_info[rpos-NUM_BUFFERS].func = alloc_hugetlbfile_free; - } + blas_release_register(map_address, alloc_hugetlbfile_free, fd); } return map_address; @@ -3056,7 +3006,9 @@ void *blas_memory_alloc(int procpos){ #endif memory_overflowed=1; MB; - new_release_info = (struct release_t*) malloc(NEW_BUFFERS * sizeof(struct release_t)); + /* zeroed so blas_shutdown sees NULL func in slots that were reserved but + never published */ + new_release_info = (struct release_t*) calloc(NEW_BUFFERS, sizeof(struct release_t)); newmemory = (struct newmemstruct*) malloc(NEW_BUFFERS * sizeof(struct newmemstruct)); for (i = 0; i < NEW_BUFFERS; i++) { newmemory[i].addr = (void *)0; @@ -3240,9 +3192,41 @@ void blas_memory_free_nolock(void * map_address) { free(map_address); } +#if defined(OS_WINDOWS) && !defined(OS_CYGWIN_NT) +/* During process termination Windows has already killed every other thread, + possibly while one held alloc_lock or a blas server lock, so any cleanup + here can only deadlock or crash; the OS reclaims the memory anyway. + + Nothing in the SDK reports this from a destructor. DllMain's lpReserved + distinguishes the two cases, but outside MSVC gotoblas_quit runs from the + CRT's fini array via __attribute__((destructor)) and never sees + it. RtlDllShutdownInProgress is documented under Win32 Dev Notes but + deliberately absent from the SDK headers, so callers declare it themselves + If it cannot be resolved we fall back to the previous behaviour. */ +typedef BOOLEAN (WINAPI *rtl_dll_shutdown_in_progress_t)(VOID); +static rtl_dll_shutdown_in_progress_t rtl_dll_shutdown_in_progress = NULL; + +/* Resolved at init, not on the way out: GetModuleHandle takes LdrpSnapsLock, + which ExitProcess does not release before it kills the other threads. */ +static void blas_shutdown_check_init(void) { + HMODULE ntdll = GetModuleHandleA("ntdll.dll"); + if (!ntdll) return; + rtl_dll_shutdown_in_progress = (rtl_dll_shutdown_in_progress_t)(void *) + GetProcAddress(ntdll, "RtlDllShutdownInProgress"); +} + +static int blas_process_is_terminating(void) { + return rtl_dll_shutdown_in_progress && rtl_dll_shutdown_in_progress(); +} +#endif + void blas_shutdown(void){ - int pos; + int pos, release_count; + +#if defined(OS_WINDOWS) && !defined(OS_CYGWIN_NT) + if (blas_process_is_terminating()) return; +#endif #ifdef SMP BLASFUNC(blas_thread_shutdown)(); @@ -3250,12 +3234,18 @@ void blas_shutdown(void){ LOCK_COMMAND(&alloc_lock); - for (pos = 0; pos < release_pos; pos ++) { - if (likely(pos < NUM_BUFFERS)) - release_info[pos].func(&release_info[pos]); - else - new_release_info[pos-NUM_BUFFERS].func(&new_release_info[pos-NUM_BUFFERS]); + release_count = release_pos; + for (pos = 0; pos < release_count; pos ++) { + struct release_t *release = likely(pos < NUM_BUFFERS) ? + &release_info[pos] : &new_release_info[pos-NUM_BUFFERS]; + void (*func)(struct release_t *) = release->func; + RMB; + if (func == NULL) continue; /* reserved but never published: owner died mid-allocation */ + func(release); + release->func = NULL; + release->address = NULL; } + release_pos = 0; #ifdef SEEK_ADDRESS base_address = 0UL; @@ -3282,6 +3272,8 @@ void blas_shutdown(void){ } free((void*)newmemory); newmemory = NULL; + free(new_release_info); + new_release_info = NULL; memory_overflowed = 0; } @@ -3406,6 +3398,10 @@ void CONSTRUCTOR gotoblas_init(void) { if (gotoblas_initialized) return; +#if defined(OS_WINDOWS) && !defined(OS_CYGWIN_NT) + blas_shutdown_check_init(); +#endif + #ifdef SMP openblas_fork_handler(); #endif