Merge pull request #5838 from ngoldbaum/fix-level3-thread-locks-2

Fix corruption due to lock sharding issues by centralizing locking

Author: martin-frbg

.github/workflows/dynamic_arch.yml

@@ -334,6 +334,255 @@ jobs:
           echo "::endgroup::"
 
 
+  linux_thread_stress:
+    if: "github.repository == 'OpenMathLib/OpenBLAS'"
+    name: ${{ matrix.check-name }}
+    runs-on: ubuntu-latest
+
+    strategy:
+      fail-fast: false
+      matrix:
+        include:
+          - backend: pthread
+            check-name: "linux_thread_stress (pthread)"
+          - backend: openmp
+            check-name: "linux_thread_stress (openmp)"
+          - backend: tsan
+            check-name: linux_thread_sanitizer
+
+    steps:
+      - name: Checkout repository
+        uses: actions/checkout@v6
+
+      - name: Install Dependencies
+        run: |
+          cat << EOF | sudo tee -a /etc/apt/apt.conf.d/01norecommend
+          APT::Install-Recommends "0";
+          APT::Install-Suggests "0";
+          EOF
+          sudo apt-get update
+          sudo apt-get install -y ccache cmake ninja-build
+          if [ "${{ matrix.backend }}" = "tsan" ]; then
+            sudo apt-get install -y clang llvm
+          fi
+
+      - name: Compilation cache
+        uses: actions/cache@v5
+        with:
+          path: ~/.ccache
+          key: ccache-${{ runner.os }}-thread-${{ matrix.backend }}-${{ github.ref }}-${{ github.sha }}
+          restore-keys: |
+            ccache-${{ runner.os }}-thread-${{ matrix.backend }}-${{ github.ref }}
+            ccache-${{ runner.os }}-thread-${{ matrix.backend }}
+
+      - name: Configure ccache
+        # Limit the maximum size and switch on compression to avoid exceeding the total disk or cache quota.
+        run: |
+          test -d ~/.ccache || mkdir -p ~/.ccache
+          echo "max_size = 250M" > ~/.ccache/ccache.conf
+          echo "compression = true" >> ~/.ccache/ccache.conf
+          ccache -s
+
+      - name: Configure OpenBLAS
+        run: |
+          mkdir build && cd build
+          build_type=Release
+          c_compiler=gcc
+          cxx_compiler=g++
+          dynamic_arch=ON
+          use_openmp=OFF
+          cpp_thread_safety_use_openmp=ON
+          dgemm_args="512;12;4"
+          dgemm_mixed_args="524288;16;20"
+          dgemv_args="512;12;4"
+          sanitizer_flags=
+          if [ "${{ matrix.backend }}" = "openmp" ]; then
+            use_openmp=ON
+          elif [ "${{ matrix.backend }}" = "tsan" ]; then
+            build_type=RelWithDebInfo
+            c_compiler=clang
+            cxx_compiler=clang++
+            dynamic_arch=OFF
+            cpp_thread_safety_use_openmp=OFF
+            dgemm_args="64;4;1"
+            dgemm_mixed_args="131072;8;10"
+            dgemv_args="64;4;1"
+            sanitizer_flags="-fsanitize=thread -g -O1 -fno-omit-frame-pointer"
+          fi
+          cmake_args=(
+            -G Ninja
+            "-DCMAKE_BUILD_TYPE=$build_type"
+            "-DCMAKE_C_COMPILER=$c_compiler"
+            "-DCMAKE_CXX_COMPILER=$cxx_compiler"
+            -DBUILD_SHARED_LIBS=ON
+            -DBUILD_STATIC_LIBS=OFF
+            -DBUILD_WITHOUT_LAPACK=ON
+            -DBUILD_SINGLE=OFF
+            -DBUILD_DOUBLE=ON
+            -DBUILD_COMPLEX=OFF
+            -DBUILD_COMPLEX16=OFF
+            "-DDYNAMIC_ARCH=$dynamic_arch"
+            -DNOFORTRAN=ON
+            -DUSE_THREAD=ON
+            "-DUSE_OPENMP=$use_openmp"
+            -DNUM_THREADS=32
+            -DNUM_PARALLEL=2
+            -DTARGET=CORE2
+            -DCPP_THREAD_SAFETY_TEST=ON
+            "-DCPP_THREAD_SAFETY_USE_OPENMP=$cpp_thread_safety_use_openmp"
+            "-DCPP_THREAD_SAFETY_DGEMM_ARGS=$dgemm_args"
+            "-DCPP_THREAD_SAFETY_DGEMM_MIXED_ARGS=$dgemm_mixed_args"
+            "-DCPP_THREAD_SAFETY_DGEMV_ARGS=$dgemv_args"
+            -DCMAKE_C_COMPILER_LAUNCHER=ccache
+            -DCMAKE_CXX_COMPILER_LAUNCHER=ccache
+          )
+          if [ "${{ matrix.backend }}" = "tsan" ]; then
+            cmake_args+=(
+              "-DCMAKE_C_FLAGS=$sanitizer_flags"
+              "-DCMAKE_CXX_FLAGS=$sanitizer_flags"
+              -DCMAKE_SHARED_LINKER_FLAGS=-fsanitize=thread
+              -DCMAKE_EXE_LINKER_FLAGS=-fsanitize=thread
+            )
+          fi
+          cmake "${cmake_args[@]}" ..
+
+      - name: Build OpenBLAS
+        run: |
+          cd build
+          cmake --build . --target dgemm_thread_safety dgemm_thread_safety_mixed dgemv_thread_safety
+
+      - name: Show ccache status
+        continue-on-error: true
+        run: ccache -s
+
+      - name: Run thread stress tests
+        timeout-minutes: 30
+        run: |
+          cd build
+          if [ "${{ matrix.backend }}" = "tsan" ]; then
+            export OPENBLAS_NUM_THREADS=2
+            export LLVM_SYMBOLIZER_PATH=/usr/bin/llvm-symbolizer
+            export TSAN_OPTIONS=halt_on_error=1:exitcode=66:second_deadlock_stack=1
+          else
+            export OPENBLAS_NUM_THREADS=8
+            export OMP_NUM_THREADS=16
+          fi
+          ctest -R 'dgemm_thread_safety|dgemm_thread_safety_mixed|dgemv_thread_safety' --output-on-failure
+
+
+  msys2_thread_stress:
+    if: "github.repository == 'OpenMathLib/OpenBLAS'"
+    runs-on: windows-latest
+
+    defaults:
+      run:
+        shell: msys2 {0}
+
+    env:
+      CHERE_INVOKING: 1
+
+    steps:
+      - name: Get CPU name
+        shell: pwsh
+        run : |
+          Get-CIMInstance -Class Win32_Processor | Select-Object -Property Name
+
+      - name: Install build dependencies
+        uses: msys2/setup-msys2@v2
+        with:
+          msystem: UCRT64
+          update: true
+          release: false  # Use pre-installed version
+          install: >-
+            base-devel
+            mingw-w64-ucrt-x86_64-cc
+            mingw-w64-ucrt-x86_64-cmake
+            mingw-w64-ucrt-x86_64-ninja
+            mingw-w64-ucrt-x86_64-ccache
+
+      - name: Checkout repository
+        uses: actions/checkout@v6
+
+      - name: Prepare ccache
+        # Get cache location of ccache
+        # Create key that is used in action/cache/restore and action/cache/save steps
+        id: ccache-prepare
+        run: |
+          echo "ccachedir=$(cygpath -m $(ccache -k cache_dir))" >> $GITHUB_OUTPUT
+          # We include the commit sha in the cache key, as new cache entries are
+          # only created if there is no existing entry for the key yet.
+          echo "key=ccache-msys2-thread-stress-${{ github.ref }}-${{ github.sha }}" >> $GITHUB_OUTPUT
+
+      - name: Restore ccache
+        uses: actions/cache/restore@v5
+        with:
+          path: ${{ steps.ccache-prepare.outputs.ccachedir }}
+          key: ${{ steps.ccache-prepare.outputs.key }}
+          # Restore a matching ccache cache entry. Prefer same branch.
+          restore-keys: |
+            ccache-msys2-thread-stress-${{ github.ref }}
+            ccache-msys2-thread-stress
+
+      - name: Configure ccache
+        # Limit the maximum size and switch on compression to avoid exceeding the total disk or cache quota.
+        run: |
+          which ccache
+          test -d ${{ steps.ccache-prepare.outputs.ccachedir }} || mkdir -p ${{ steps.ccache-prepare.outputs.ccachedir }}
+          echo "max_size = 250M" > ${{ steps.ccache-prepare.outputs.ccachedir }}/ccache.conf
+          echo "compression = true" >> ${{ steps.ccache-prepare.outputs.ccachedir }}/ccache.conf
+          ccache -p
+          ccache -s
+
+      - name: Configure OpenBLAS
+        run: |
+          mkdir build && cd build
+          cmake -G Ninja \
+                -DCMAKE_BUILD_TYPE=Release \
+                -DBUILD_SHARED_LIBS=ON \
+                -DBUILD_STATIC_LIBS=OFF \
+                -DBUILD_WITHOUT_LAPACK=ON \
+                -DBUILD_SINGLE=OFF \
+                -DBUILD_DOUBLE=ON \
+                -DBUILD_COMPLEX=OFF \
+                -DBUILD_COMPLEX16=OFF \
+                -DDYNAMIC_ARCH=OFF \
+                -DNOFORTRAN=ON \
+                -DUSE_THREAD=ON \
+                -DUSE_OPENMP=OFF \
+                -DNUM_THREADS=32 \
+                -DTARGET=CORE2 \
+                -DCPP_THREAD_SAFETY_TEST=ON \
+                -DCPP_THREAD_SAFETY_DGEMM_ARGS="384;8;4" \
+                -DCPP_THREAD_SAFETY_DGEMM_MIXED_ARGS="524288;16;20" \
+                -DCPP_THREAD_SAFETY_DGEMV_ARGS="384;8;4" \
+                -DCMAKE_C_COMPILER_LAUNCHER=ccache \
+                -DCMAKE_CXX_COMPILER_LAUNCHER=ccache \
+                ..
+
+      - name: Build OpenBLAS
+        run: |
+          cd build
+          cmake --build . --target dgemm_thread_safety dgemm_thread_safety_mixed dgemv_thread_safety
+
+      - name: Show ccache status
+        continue-on-error: true
+        run: ccache -s
+
+      - name: Save ccache
+        # Save the cache after we are done (successfully) building
+        uses: actions/cache/save@v5
+        with:
+          path: ${{ steps.ccache-prepare.outputs.ccachedir }}
+          key: ${{ steps.ccache-prepare.outputs.key }}
+
+      - name: Run thread stress tests
+        timeout-minutes: 30
+        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
+
+
   cross_build:
     if: "github.repository == 'OpenMathLib/OpenBLAS'"
     runs-on: ubuntu-22.04

CMakeLists.txt

@@ -58,9 +58,10 @@ else()
   set(NO_AFFINITY 1)
 endif()
 
-option(CPP_THREAD_SAFETY_TEST "Run a massively parallel DGEMM test to confirm thread safety of the library (requires OpenMP and about 1.3GB of RAM)" OFF)
+option(CPP_THREAD_SAFETY_TEST "Run massively parallel DGEMM tests to confirm thread safety of the library (requires about 1.3GB of RAM)" OFF)
+option(CPP_THREAD_SAFETY_USE_OPENMP "Use OpenMP to launch the C++ thread safety tests" ON)
 
-option(CPP_THREAD_SAFETY_GEMV "Run a massively parallel DGEMV test to confirm thread safety of the library (requires OpenMP)" OFF)
+option(CPP_THREAD_SAFETY_GEMV "Run a massively parallel DGEMV test to confirm thread safety of the library" OFF)
 option(BUILD_STATIC_LIBS "Build static library" OFF)
 option(BUILD_SHARED_LIBS "Build shared library" OFF)
 if(NOT BUILD_STATIC_LIBS AND NOT BUILD_SHARED_LIBS)
@@ -820,4 +821,3 @@ install(FILES ${CMAKE_CURRENT_BINARY_DIR}/${PN}ConfigVersion.cmake
 install(EXPORT "${PN}${SUFFIX64}Targets"
         NAMESPACE "${PN}${SUFFIX64}::"
         DESTINATION ${CMAKECONFIG_INSTALL_DIR})
-

Makefile.install

@@ -355,7 +355,7 @@ endif
 endif
 ifeq ($(CPP_THREAD_SAFETY_TEST), 1)
 	@install -m 666 cpp_thread_test/dgemm_tester $(DESTDIR)$(OPENBLAS_BINARY_DIR)
+	@install -m 666 cpp_thread_test/dgemm_mixed_tester $(DESTDIR)$(OPENBLAS_BINARY_DIR)
 	@install -m 666 cpp_thread_test/dgemv_tester $(DESTDIR)$(OPENBLAS_BINARY_DIR)
 endif
 endif
-

Makefile.rule

@@ -290,10 +290,10 @@ COMMON_PROF = -pg
 # This is mostly intended as a developer feature to spot regressions, but users and
 # package maintainers can enable this if they have doubts about the thread safety of
 # the library, given the configuration in this file.
-# By default, the thread safety tester launches 52 concurrent calculations at the same
-# time.
+# By default, the thread safety testers launch many concurrent calculations at
+# the same time.
 #
-# Please note that the test uses ~1300 MiB of RAM for the DGEMM test.
+# Please note that the tests use ~1300 MiB of RAM for the DGEMM test.
 #
 # The test requires CBLAS to be built, a C++11 capable compiler and the presence of
 # an OpenMP implementation. If you are cross-compiling this test will probably not

common_thread.h

@@ -191,6 +191,9 @@ int exec_blas(BLASLONG num_cpu, blas_param_t *param, void *buffer);
 
 #ifndef ASSEMBLER
 
+void blas_level3_thread_enter(void);
+void blas_level3_thread_leave(void);
+
 int blas_level1_thread(int mode, BLASLONG m, BLASLONG n, BLASLONG k, void *alpha,
 		       void *a, BLASLONG lda,
 		       void *b, BLASLONG ldb,

cpp_thread_test/CMakeLists.txt

@@ -5,19 +5,35 @@ enable_language(CXX)
 
 set(CMAKE_CXX_FLAGS "${CMAKE_C_FLAGS} -DADD${BU} -DCBLAS")
 
-if (USE_OPENMP)
-if (CPP_THREAD_SAFETY_TEST)
-  message(STATUS building thread safety test)
-  add_executable(dgemm_thread_safety dgemm_thread_safety.cpp) 
-  target_link_libraries(dgemm_thread_safety ${OpenBLAS_LIBNAME})
-  add_test( dgemm_thread_safety ${CMAKE_CURRENT_BINARY_DIR}/dgemm_thread_safety)
+set(CPP_THREAD_SAFETY_LIBS ${OpenBLAS_LIBNAME})
+find_package(Threads REQUIRED)
+list(APPEND CPP_THREAD_SAFETY_LIBS Threads::Threads)
+add_definitions(-DOPENBLAS_USE_GENERATED_CBLAS_H)
+
+if ((CPP_THREAD_SAFETY_TEST OR CPP_THREAD_SAFETY_GEMV) AND CPP_THREAD_SAFETY_USE_OPENMP)
+  find_package(OpenMP REQUIRED COMPONENTS CXX)
+  list(APPEND CPP_THREAD_SAFETY_LIBS OpenMP::OpenMP_CXX)
+  add_definitions(-DCPP_THREAD_SAFETY_USE_OPENMP)
 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")
 
-if (CPP_THREAD_SAFETY_TEST OR CPP_THREAD_SAFETY_GEMV)
-  add_executable(dgemv_thread_safety dgemv_thread_safety.cpp) 
-  target_link_libraries(dgemv_thread_safety ${OpenBLAS_LIBNAME})
-  add_test(dgemv_thread_safety ${CMAKE_CURRENT_BINARY_DIR}/dgemv_thread_safety)
+if (CPP_THREAD_SAFETY_TEST)
+  message(STATUS "building thread safety test")
+  add_executable(dgemm_thread_safety dgemm_thread_safety.cpp)
+  target_link_libraries(dgemm_thread_safety ${CPP_THREAD_SAFETY_LIBS})
+  add_test(NAME dgemm_thread_safety COMMAND ${CMAKE_CURRENT_BINARY_DIR}/dgemm_thread_safety ${CPP_THREAD_SAFETY_DGEMM_ARGS})
+
+  add_executable(dgemm_thread_safety_mixed dgemm_thread_safety_mixed.cpp)
+  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})
 endif()
 
+
+if (CPP_THREAD_SAFETY_TEST OR CPP_THREAD_SAFETY_GEMV)
+  add_executable(dgemv_thread_safety dgemv_thread_safety.cpp)
+  target_link_libraries(dgemv_thread_safety ${CPP_THREAD_SAFETY_LIBS})
+  add_test(NAME dgemv_thread_safety COMMAND ${CMAKE_CURRENT_BINARY_DIR}/dgemv_thread_safety ${CPP_THREAD_SAFETY_DGEMV_ARGS})
 endif()

cpp_thread_test/Makefile

@@ -1,15 +1,19 @@
 TOPDIR = ..
 include $(TOPDIR)/Makefile.system
 
-all :: dgemv_tester dgemm_tester
+all :: dgemv_tester dgemm_tester dgemm_mixed_tester
 
 dgemv_tester :
-	$(CXX) $(COMMON_OPT) -Wall -Wextra -Wshadow -fopenmp -std=c++11 dgemv_thread_safety.cpp ../$(LIBNAME) $(EXTRALIB) $(FEXTRALIB) -o dgemv_tester
+	$(CXX) $(COMMON_OPT) -Wall -Wextra -Wshadow -std=c++11 dgemv_thread_safety.cpp ../$(LIBNAME) $(EXTRALIB) $(FEXTRALIB) -o dgemv_tester
 	./dgemv_tester
 
 dgemm_tester : dgemv_tester
-	$(CXX) $(COMMON_OPT) -Wall -Wextra -Wshadow -fopenmp -std=c++11 dgemm_thread_safety.cpp ../$(LIBNAME) $(EXTRALIB) $(FEXTRALIB) -o dgemm_tester
+	$(CXX) $(COMMON_OPT) -Wall -Wextra -Wshadow -std=c++11 dgemm_thread_safety.cpp ../$(LIBNAME) $(EXTRALIB) $(FEXTRALIB) -o dgemm_tester
 	./dgemm_tester
 
+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
+
 clean ::
-	rm -f dgemv_tester dgemm_tester
+	rm -f dgemv_tester dgemm_tester dgemm_mixed_tester