Skip to content

Ensure that blas_thread_init has been called in openblas_set_num_threads #1837

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 30, 2018

Conversation

embray
Copy link
Contributor

@embray embray commented Oct 28, 2018

Not doing so can lead to bugs if the process happens to have been forked at some point. For example:

pid = fork();
if (pid) {
    openblas_set_num_thread(8);
    // <call some multi-threaded blas function (dgemm, etc.)>
}

This leads to segfaults because currently openblas_set_num_threads assumes blas_thread_init() has already been called and blas_server_avail == 1. But after a fork, due to the pthread_atfork handler which calls blas_thread_shutdown, this is no longer the case. But then when you get into exec_blas_async it does call blas_thread_init() even though there are already threads running, creating a likelihood that one or more of those threads segfault.

@brada4
Copy link
Contributor

brada4 commented Oct 30, 2018

Should work fine.
It is one-time code in general, "unlikely" tagging just impairs readability.

@martin-frbg martin-frbg merged commit 6af8e35 into OpenMathLib:develop Oct 30, 2018
@embray
Copy link
Contributor Author

embray commented Oct 30, 2018

Sure. It was literally just copy/pasted from elsewhere. I see no harm in including unlikely() but I also agree that in most cases it is not significant.

@embray embray deleted the set-num-thread-after-fork branch October 31, 2018 10:40
@martin-frbg martin-frbg added this to the 0.3.4 milestone Nov 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants