Skip to content
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

Fix JULIA_EXCLUSIVE setting affinity on non-worker threads #57136

Merged
merged 5 commits into from
Jan 25, 2025

Conversation

xal-0
Copy link
Contributor

@xal-0 xal-0 commented Jan 22, 2025

With JULIA_EXCLUSIVE=1, we would try to give both worker and interactive threads an exclusive CPU, causing --threads=auto to produce a "Too many threads requested" error.

Fixes #50702.

Copy link
Member

@topolarity topolarity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic is a little odd to me - e.g. if nthreadsi == 1 and nmutator_threads == 2 we no longer end up setting affinity for any thread

Is that intentional?

src/threading.c Outdated Show resolved Hide resolved
src/threading.c Outdated Show resolved Hide resolved
src/threading.c Outdated Show resolved Hide resolved
@xal-0
Copy link
Contributor Author

xal-0 commented Jan 24, 2025

This logic is a little odd to me - e.g. if nthreadsi == 1 and nmutator_threads == 2 we no longer end up setting affinity for any thread

Is that intentional?

It was not intentional! It should be fixed now:

$ JULIA_EXCLUSIVE=1 ./usr/bin/julia-debug -t 1 -e ''
setting affinity of master thread to 0
$ JULIA_EXCLUSIVE=1 ./usr/bin/julia-debug -t 2 -e ''
setting affinity of master thread to 0
setting affinity of thread 1 to 1
$ JULIA_EXCLUSIVE=1 ./usr/bin/julia-debug -t 3 -e ''
setting affinity of master thread to 0
setting affinity of thread 1 to 1
setting affinity of thread 2 to 2
$ JULIA_EXCLUSIVE=1 ./usr/bin/julia-debug -t 1,1 -e ''
setting affinity of thread 1 to 0
$ JULIA_EXCLUSIVE=1 ./usr/bin/julia-debug -t 1,2 -e ''
setting affinity of thread 2 to 0
$ JULIA_EXCLUSIVE=1 ./usr/bin/julia-debug -t 1,3 -e ''
setting affinity of thread 3 to 0
$ JULIA_EXCLUSIVE=1 ./usr/bin/julia-debug -t 2,1 -e ''
setting affinity of thread 1 to 0
setting affinity of thread 2 to 1
$ JULIA_EXCLUSIVE=1 ./usr/bin/julia-debug -t 2,2 -e ''
setting affinity of thread 2 to 0
setting affinity of thread 3 to 1
$ JULIA_EXCLUSIVE=1 ./usr/bin/julia-debug -t 3,3 -e ''
setting affinity of thread 3 to 0
setting affinity of thread 4 to 1
setting affinity of thread 5 to 2

@xal-0 xal-0 force-pushed the fix-exclusive-thread-count branch from fee6b4a to e8a391a Compare January 24, 2025 00:03
@@ -2058,6 +2058,9 @@ extern JL_DLLIMPORT _Atomic(int) jl_n_threads;
extern JL_DLLIMPORT int jl_n_gcthreads;
extern int jl_n_markthreads;
extern int jl_n_sweepthreads;

#define JL_THREADPOOL_ID_INTERACTIVE 0
#define JL_THREADPOOL_ID_DEFAULT 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I am well known for my hatred of magic integer constants).

src/threading.c Show resolved Hide resolved
src/threading.c Show resolved Hide resolved
Copy link
Member

@Keno Keno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work

@Keno Keno added the merge me PR is reviewed. Merge when all tests are passing label Jan 24, 2025
Copy link
Member

@topolarity topolarity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done!

src/threading.c Show resolved Hide resolved
src/threading.c Outdated Show resolved Hide resolved
@xal-0 xal-0 force-pushed the fix-exclusive-thread-count branch from ad2c2cd to 3814673 Compare January 24, 2025 20:21
@giordano
Copy link
Contributor

Git history and diff seem to be messed up

@xal-0 xal-0 force-pushed the fix-exclusive-thread-count branch from 3814673 to ce5a687 Compare January 24, 2025 20:38
@giordano giordano added the multithreading Base.Threads and related functionality label Jan 24, 2025
@topolarity
Copy link
Member

topolarity commented Jan 25, 2025

Our CI seems to have fallen over, but this has passed tests repeatedly on all platforms so I'll go ahead and merge it.

Thanks @xal-0 !

@topolarity topolarity merged commit 246e408 into JuliaLang:master Jan 25, 2025
5 of 8 checks passed
@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Jan 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
multithreading Base.Threads and related functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JULIA_EXCLUSIVE with auto threads errors on 1.10, mac/arm64
6 participants