Openblas: Segfault when NUM_THREADS is smaller than host vCPUs

[jonringer]

Most context can be found here: https://github.com/xianyi/OpenBLAS/pull/2982

But in summary:

A segfault will occur if the NUM_THREADS used to build openblas is lower than the host's vCPU count when using OMP.

[Switching to Thread 0x7ffebba6e640 (LWP 104473)]
0x00007fffe95cbe70 in dgemm_itcopy_ZEN () from /nix/store/3v2i5ga27y9al3z0d8ccwaa389qz3ma6-lapack-3/lib/liblapack.so.3
(gdb) backtrace
#0  0x00007fffe95cbe70 in dgemm_itcopy_ZEN () from /nix/store/3v2i5ga27y9al3z0d8ccwaa389qz3ma6-lapack-3/lib/liblapack.so.3
#1  0x00007fffe882416a in inner_thread () from /nix/store/3v2i5ga27y9al3z0d8ccwaa389qz3ma6-lapack-3/lib/liblapack.so.3
#2  0x00007fffe8956bf6 in exec_blas._omp_fn () from /nix/store/3v2i5ga27y9al3z0d8ccwaa389qz3ma6-lapack-3/lib/liblapack.so.3
#3  0x00007fffe347f916 in ?? () from /nix/store/f23sq7lk6xfrvz467ffkpzackyk5q8dm-gfortran-9.3.0-lib/lib/libgomp.so.1
#4  0x00007ffff7c20e9e in start_thread () from /nix/store/5didcr1sjp2rlx8abbzv92rgahsarqd9-glibc-2.32/lib/libpthread.so.0
#5  0x00007ffff793866f in clone () from /nix/store/5didcr1sjp2rlx8abbzv92rgahsarqd9-glibc-2.32/lib/libc.so.6

This currently has a "workaround" by setting the environment variable OMP_NUM_THREADS to a value equal or lower to the NUM_THREADS used during build. However, desired behavior is that it doesn't crash.

EDIT:
Also of note, this fails when running the numpy test suite. Openblas is able to run it's test suite without crashing.
blas, openblas, and lapack are all aliased to openblas

$ cat numpy/site.cfg
[blas]
include_dirs=/nix/store/bm0mjpbnaxixma2wvmj99zq2kfq1017h-blas-3-dev/include
library_dirs=/nix/store/35lvfykv13mnva6sipnrqks05mvrldi0-blas-3/lib
runtime_library_dirs=/nix/store/35lvfykv13mnva6sipnrqks05mvrldi0-blas-3/lib

[lapack]
include_dirs=/nix/store/s9rb0jhplgsfcnay1mp7kn1zhxyppq0n-lapack-3-dev/include
library_dirs=/nix/store/w6xjrzfnxsi7jwrlvfy10qcljm6gyhss-lapack-3/lib
runtime_library_dirs=/nix/store/w6xjrzfnxsi7jwrlvfy10qcljm6gyhss-lapack-3/lib

[openblas]
include_dirs=/nix/store/bm0mjpbnaxixma2wvmj99zq2kfq1017h-blas-3-dev/include:/nix/store/s9rb0jhplgsfcnay1mp7kn1zhxyppq0n-lapack-3-dev/include
libraries=lapack,lapacke,blas,cblas
library_dirs=/nix/store/35lvfykv13mnva6sipnrqks05mvrldi0-blas-3/lib:/nix/store/w6xjrzfnxsi7jwrlvfy10qcljm6gyhss-lapack-3/lib
runtime_library_dirs=/nix/store/35lvfykv13mnva6sipnrqks05mvrldi0-blas-3/lib:/nix/store/w6xjrzfnxsi7jwrlvfy10qcljm6gyhss-lapack-3/lib

System / Build Info

$ nix eval -f default.nix openblas.makeFlags
[ "BINARY=64" "CC=cc" "CROSS=0" "DYNAMIC_ARCH=1" "FC=gfortran" "HOSTCC=cc" "INTERFACE64=1" "NO_AVX512=1" "NO_BINARY_MODE=" "NO_SHARED=0" "NO_STATIC=1" "NUM_THREADS=64" "TARGET=ATHLON" "USE_OPENMP=1" ]

CC = gcc-9.3.0
arch = x86_64
platform = linux
CPU = 3990X ( 64 cores / 128 thread)

[martin-frbg]

Of OpenBLAS' testsuite, test and ctest are executed with OPENBLAS_NUM_THREADS=2, and the utest uses tiny matrix sizes so is unlikely to expose this particular problem. There are already guards in memory.c that _should_ limit the number of threads to what the library has room for (the compile-time constant BLAS_CPU_NUMBER), it looks like they are overriden in the OpenMP case by the "evil" call to omp_get_max_threads() in num_cpu_avail()

[Flamefire]

I tested that on an AMD system with 256 threads and built with OpenBLAS with 16 threads and see no crash. Using the non-ILP64 build though. Will test if that makes a difference

[jonringer]

I was also able to repro this using gcc-8.4.0 as well.

[jonringer]

Unrelated to this issue, but I noticed that the source code has a mix of space, tabs, and some extraneous whitespace. I think it would be beneficial to have some linting gates to keep the style consistent.

[martin-frbg]

Suspect there is a loophole somewhere in the code that allows the thread allocator to come around for a second helping after having been denied the overallocation. Still a crash should be accompanied by the semi-helpful "terminated because you tried to allocate too many memory regions". Whether that makes it through to the user may not always be a given, but I am not sure if the library can do anything better than that. (Please see previous discussion in #2304).

And yes, whitespace handling is inconsistent at best, and I may be a major culprit. Still making it right should have higher priority than making it look pretty I guess. (If you happen to know of some GH plugin that would automagically keep the source in shape though...)

[jonringer]

And yes, whitespace handling is inconsistent at best, and I may be a major culprit. Still making it right should have higher priority than making it look pretty I guess. (If you happen to know of some GH plugin that would automagically keep the source in shape though...)

We use https://github.com/NixOS/nixpkgs/blob/master/.github/workflows/editorconfig.yml to do that. But it's kind of annoying in that it will error if any file you changed doesn't conform to the config. For example, I add a single line, it will complain that there's 30 lines I never touched which aren't conforming. I'm sure there's other linting options as well.

EDIT:
However, the code base is slowly converging to the ideal syntax. So, linting errors are becoming less common.

[brada4]

• GNU indent
• clang-style
• (Open)BSD indent

Just to guess the right parameters which make smaller diff when applied to all code base.

[Flamefire]

@jonringer Trying to repro as well, but failed yet. Any details how you build it that you could share?

[jonringer]

I laid out my testing methodology here: https://github.com/xianyi/OpenBLAS/issues/2970#issuecomment-725069635

If you install https://nixos.org/download.html (can be done on any linux distro, you can uninstall with sudo rm -rf /nix), you should be able to reproduce my exact dependency graph.

you should only need to copy the default.nix from the other post and run:

nix-build default.nix -A python3Packages.numpy

and it will build openblas and numpy from source using your local changes.

relevant build information is included in my original post at the bottom

EDIT:
actually, it's only exactly reproducible if I reference the same commit, and I already patched the tests so that I could review PRs again.

If you care to use the nix tooling to repo the issue, then let me know. But nixpkgs has a non-trivial learning curve, so I understand not wanting to use it.

[brada4]

What are CFLAGS of build coming from multiple places of hardeningCFlags+= ?
They should enter as CCOMMON_OPT and COMMON_OPT with CFLAGS not set.
At present it seems that at least -fPIC never gets to fortran flags.

[Flamefire]

Ok, I got it to segfault with OpenBLAS 0.3.12 & numpy 1.19.4 on a 256 thread AMD system when build with 28 or 128 threads, but not with 24 or less or 256

[brada4]

Can you get t a a bt from same GDB so to see what was called in main thread - GEMM alone or via some lapack....

[Flamefire]

Crashing thread bt:

#0  0x00002aaab426ecd8 in dgemm_itcopy_ZEN (m=16, n=16, a=0x2aacc0074c70, lda=32, b=0x0) at ../kernel/x86_64/../generic/gemm_tcopy_4.c:99
#1  0x00002aaab2e8fcd6 in inner_thread (args=0x7ffffffea260, range_m=0x7ffffffe7658, range_n=0x7ffffffe7448, sa=0x0, sb=0x100000, mypos=63) at level3_thread.c:345
#2  0x00002aaab30531ac in exec_threads (queue=0x7ffffffea1b8, buf_index=0) at blas_server_omp.c:370
#3  0x00002aaab30533aa in exec_blas._omp_fn.0 () at blas_server_omp.c:418
#4  0x00002aaaaaca387e in gomp_thread_start (xdata=<optimized out>) at ../../../libgomp/team.c:120
#5  0x00002aaaab7f9e65 in start_thread () from /lib64/libpthread.so.0
#6  0x00002aaaabd0f88d in clone () from /lib64/libc.so.6

Full: backtrace.txt

[martin-frbg]

Pretty sure this is the buffer allocation in interface/gemm.c failing - there is a first round of buffer allocations for each thread happening in blas_thread_init.c on startup, and the GEMM call in the user code then initiates another (which is where things are currently bound to go awry even though we limited the first set to the MAX_CPU_NUMBER assumed at compile time).

[Flamefire]

Sounds like the right track. The b=0x0 is the fault: It tries to access that address.
I also tried to run the test in question manually (test_dot_3args) but that didn't fail. Maybe some memory leak or something where process state gets corrupted earlier?

[brada4]

Thanks, last thread in the list has the call chain and wonderfully with dimensions passed to cblas_dgemm call...
#8 0x00002aaab2d2f50d in cblas_dgemm (order=CblasRowMajor, TransA=CblasNoTrans, TransB=CblasNoTrans, m=1024, n=32, k=16, alpha=1, a=0x2aacc0054be0, lda=16, b=0x2aacc0074bf0, ldb=32, beta=0, c=0x2aacc00f0e60, ldc=32) at gemm.c:453
Crash happens during legal op reading 0x80 from the begining of array B of size 0x2000 , disasm at crash site would be nice, but I would bet at some alignment mishap.

[martin-frbg]

@brada4 not sure what you are seeing, and no disassembly needed I guess as the gemm_tcopy_4 is plain C - it is trying to copy data from the buffer normally pointed to by b that simply is not there as there was no slot available for it in the buffers array.

[Flamefire]

Yes, just checked a bit of the control flow. In blas_server_omp.c it enters the branch if ((sa == NULL) && (sb == NULL) && ((queue -> mode & BLAS_PTHREAD) == 0)) { but for some reason both blas_thread_buffer[buf_index][pos]; and blas_memory_alloc(2); return NULL for the 64th thread. Will build with debug output to see what happens here

[brada4]

Arguments are decoded well, no need for more debug detail.

[Flamefire]

Well I do have some more detail from debugging with relation to the patch (blas_thread_init): I set a breakpoint to blas_thread_init and when the test_dot_3args is run this function is entered as expected. However for some reason the blas_thread_buffer array is already filled!
As blas_memory_alloc(2) is called unconditionally the old buffer is lost (memory leak) and hence the application runs out of free buffers. I have no idea how it can happen that the array is not zeroed but blas_server_avail is zero. The fork handler is called at some point and I see zeroes in the blas_thread_buffer array afterwards.
After the blas_thread_init has run and I break in the exec_threads function the last entry of blas_thread_buffer is zero, all others are filled

[martin-frbg]

Hm. Not sure how this could happen. Can you confirm by setting a watchpoint on the blas_thread_buffer array instead ?

[Flamefire]

Ok found the problem. What happens is:

• the fork handler resets the blas_thread_buffer array and the blas_server_avail flag
• A BLAS function (in this case cblas_ssyrk) calls num_cpu_avail
• This detects a mismatch: blas_cpu_number=64 (capped in previous settings) and openmp_nthreads=256
• This then calls goto_set_num_threads(openmp_nthreads)
• That function adjusts the blas_thread_buffer allocating memory where required
• exec_blas is called a bit later (by gemm) which finds blas_server_avail=0 and calls blas_thread_init which reinits the blas_thread_buffer without checking it first
• This then runs out of buffers to allocate: NUM_BUFFERS=64*2, 32 are allocated by goto_set_num_threads, 1 by gem directly, then 32 further ones are requested by blas_thread_init totalling 65 -> last entry is zero

This also doesn't come up for small thread numbers because the min. for NUM_BUFFERS is 50, so I guess below 25 cores there won't be an observable problem. And finally: OpenBLAS does show an error/warning that the allocation fails but the numpy test suite swallows it

PR coming up.

[jonringer]

thanks @Flamefire :)

[Flamefire]

Sure. As it turned out it was indeed my commit you PRed to revert which caused the issue although the reason was much more complicated than thought.