Release OpenMP resources in blas_thread_shutdown #4080
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks - I have not had time to review in detail, but perhaps it might be better to put the OpenMP version test in the c_check script rather than getarch |
OpenMP 5.0 introduced the function omp_pause_resource_all that instructs the runtime to "relinquish resources used by OpenMP on all devices". In practice, these resources include the locks that would otherwise trip up the runtime after a fork(). Releasing these resources in a function called by pthread_atfork() makes it possible for the child process to continue functioning after the runtime automatically re-acquires its resources. Thread safety: blas_thread_shutdown doesn't check whether there are other BLAS operations running in parallel, so this isn't any less safe than before with respect to OpenBLAS function calls. On the other hand, if there are other OpenMP operations in progress, asking the runtime to pause may result in unspecified behaviour. A hard pause is allowed to deallocate threadprivate variables too.
In addition to testing fork safety on non-OpenMP builds, test it on OpenMP >= 5.0, where we get the ability to release the locks at fork() time.
aitap
force-pushed
the
omp_pause_resource
branch
from
570d6ba to
645eabe
Compare
|
Thanks for your reply!
What do you think about skipping the test using the same mechanism that
currently skips it on builds without double precision support? It seems
to make for a smaller diff.
I could definitely use c_check if you prefer it for architectural reasons
and the omp_pause_resource_all() idea turns out to be sound. (I didn't
originally notice that you have a mechanism for checking preprocessor
symbols.)
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
OpenMP 5.0 gained an
omp_pause_resource_allfunction designed to release the locks beforeforking the process. The parameters are described in theomp_pause_resourcefunction. Using this function makes it possible for OpenMP builds of OpenBLAS to passforksafety tests.An important detail is that there exist OpenMP implementations that only claim OpenMP 4.5 compatibility (i.e. they
#define _OPENMP 201511) while they do have theomp_pause_resource_all. We currently don't have a way to useomp_pause_resource_allwith such implementations (e.g. GCC 10 on Debian 11).The first commit adds the
omp_pause_resource_allcall toblas_thread_shutdown. You may want not to merge this if you'd likeblas_thread_shutdownto be safe to call while other OpenMP operations are in progress.The second commit adds a convoluted way to test
forksafety of the resulting build on OpenMP ≥ 5.0 in addition to non-OpenMP builds. I'd be glad to see a better way of testing for OpenMP ≥ 5.0 in a Makefile.