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.

Sign up for GitHub

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

Jump to bottom

Change new thread's affinity after thread starts, from the same thread, as a workaround for Snap #40205

Merged
merged 1 commit into from
Aug 4, 2020
Merged

Change new thread's affinity after thread starts, from the same thread, as a workaround for Snap #40205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/coreclr/src/gc/unix/config.gc.h.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@
#cmakedefine01 HAVE_PTHREAD_CONDATTR_SETCLOCK
#cmakedefine01 HAVE_MACH_ABSOLUTE_TIME
#cmakedefine01 HAVE_SCHED_GETAFFINITY
#cmakedefine01 HAVE_PTHREAD_GETAFFINITY_NP
#cmakedefine01 HAVE_SCHED_SETAFFINITY
#cmakedefine01 HAVE_PTHREAD_SETAFFINITY_NP
#cmakedefine01 HAVE_PTHREAD_NP_H
#cmakedefine01 HAVE_CPUSET_T
#cmakedefine01 HAVE__SC_AVPHYS_PAGES
Expand Down
3 changes: 2 additions & 1 deletion src/coreclr/src/gc/unix/configure.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ check_cxx_source_runs("


check_library_exists(c sched_getaffinity "" HAVE_SCHED_GETAFFINITY)
check_library_exists(c sched_setaffinity "" HAVE_SCHED_SETAFFINITY)
check_library_exists(pthread pthread_create "" HAVE_LIBPTHREAD)

if (HAVE_LIBPTHREAD)
Expand All @@ -94,7 +95,7 @@ elseif (HAVE_PTHREAD_IN_LIBC)
set(PTHREAD_LIBRARY c)
endif()

check_library_exists(${PTHREAD_LIBRARY} pthread_getaffinity_np "" HAVE_PTHREAD_GETAFFINITY_NP)
check_library_exists(${PTHREAD_LIBRARY} pthread_setaffinity_np "" HAVE_PTHREAD_SETAFFINITY_NP)

check_cxx_symbol_exists(_SC_PHYS_PAGES unistd.h HAVE__SC_PHYS_PAGES)
check_cxx_symbol_exists(_SC_AVPHYS_PAGES unistd.h HAVE__SC_AVPHYS_PAGES)
Expand Down
19 changes: 16 additions & 3 deletions src/coreclr/src/gc/unix/gcenv.unix.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -985,19 +985,32 @@ size_t GCToOSInterface::GetCacheSizePerLogicalCpu(bool trueSize)
// true if setting the affinity was successful, false otherwise.
bool GCToOSInterface::SetThreadAffinity(uint16_t procNo)
{
#if HAVE_PTHREAD_GETAFFINITY_NP
#if HAVE_SCHED_SETAFFINITY || HAVE_PTHREAD_SETAFFINITY_NP
cpu_set_t cpuSet;
CPU_ZERO(&cpuSet);
CPU_SET((int)procNo, &cpuSet);

// Snap's default strict confinement does not allow sched_setaffinity(<nonzeroPid>, ...) without manually connecting the
// process-control plug. sched_setaffinity(<currentThreadPid>, ...) is also currently not allowed, only
// sched_setaffinity(0, ...). pthread_setaffinity_np(pthread_self(), ...) seems to call
// sched_setaffinity(<currentThreadPid>, ...) in at least one implementation, and does not work. To work around those
// issues, use sched_setaffinity(0, ...) if available and only otherwise fall back to pthread_setaffinity_np(). See the
// following for more information:
// - https://github.com/dotnet/runtime/pull/38795
// - https://github.com/dotnet/runtime/issues/1634
// - https://forum.snapcraft.io/t/requesting-autoconnect-for-interfaces-in-pigmeat-process-control-home/17987/13
#if HAVE_SCHED_SETAFFINITY
int st = sched_setaffinity(0, sizeof(cpu_set_t), &cpuSet);
#else
int st = pthread_setaffinity_np(pthread_self(), sizeof(cpu_set_t), &cpuSet);
#endif

return (st == 0);

#else // HAVE_PTHREAD_GETAFFINITY_NP
#else // !(HAVE_SCHED_SETAFFINITY || HAVE_PTHREAD_SETAFFINITY_NP)
// There is no API to manage thread affinity, so let's ignore the request
return false;
#endif // HAVE_PTHREAD_GETAFFINITY_NP
#endif // HAVE_SCHED_SETAFFINITY || HAVE_PTHREAD_SETAFFINITY_NP
}

// Boosts the calling thread's thread priority to a level higher than the default
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/src/pal/src/config.h.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
#cmakedefine01 HAVE_PTHREAD_GETCPUCLOCKID
#cmakedefine01 HAVE_PTHREAD_SIGQUEUE
#cmakedefine01 HAVE_PTHREAD_GETAFFINITY_NP
#cmakedefine01 HAVE_PTHREAD_ATTR_SETAFFINITY_NP
#cmakedefine01 HAVE_CPUSET_T
#cmakedefine01 HAVE_SIGRETURN
#cmakedefine01 HAVE__THREAD_SYS_SIGRETURN
Expand Down Expand Up @@ -66,6 +65,7 @@
#cmakedefine01 HAVE_TTRACE
#cmakedefine01 HAVE_PIPE2
#cmakedefine01 HAVE_SCHED_GETAFFINITY
#cmakedefine01 HAVE_SCHED_SETAFFINITY
#cmakedefine HAVE_UNW_GET_SAVE_LOC
#cmakedefine HAVE_UNW_GET_ACCESSORS
#cmakedefine01 HAVE_XSWDEV
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/src/pal/src/configure.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ check_include_files(gnu/lib-names.h HAVE_GNU_LIBNAMES_H)
check_function_exists(kqueue HAVE_KQUEUE)

check_library_exists(c sched_getaffinity "" HAVE_SCHED_GETAFFINITY)
check_library_exists(c sched_setaffinity "" HAVE_SCHED_SETAFFINITY)
check_library_exists(pthread pthread_create "" HAVE_LIBPTHREAD)
check_library_exists(c pthread_create "" HAVE_PTHREAD_IN_LIBC)

Expand All @@ -100,7 +101,6 @@ check_library_exists(${PTHREAD_LIBRARY} pthread_getattr_np "" HAVE_PTHREAD_GETAT
check_library_exists(${PTHREAD_LIBRARY} pthread_getcpuclockid "" HAVE_PTHREAD_GETCPUCLOCKID)
check_library_exists(${PTHREAD_LIBRARY} pthread_sigqueue "" HAVE_PTHREAD_SIGQUEUE)
check_library_exists(${PTHREAD_LIBRARY} pthread_getaffinity_np "" HAVE_PTHREAD_GETAFFINITY_NP)
check_library_exists(${PTHREAD_LIBRARY} pthread_attr_setaffinity_np "" HAVE_PTHREAD_ATTR_SETAFFINITY_NP)

check_function_exists(sigreturn HAVE_SIGRETURN)
check_function_exists(_thread_sys_sigreturn HAVE__THREAD_SYS_SIGRETURN)
Expand Down
96 changes: 57 additions & 39 deletions src/coreclr/src/pal/src/thread/thread.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -740,41 +740,6 @@ CorUnix::InternalCreateThread(
storedErrno = errno;
#endif // PTHREAD_CREATE_MODIFIES_ERRNO

#if HAVE_PTHREAD_ATTR_SETAFFINITY_NP && HAVE_SCHED_GETAFFINITY
{
// Threads inherit their parent's affinity mask on Linux. This is not desired, so we reset
// the current thread's affinity mask to the mask of the current process.
cpu_set_t cpuSet;
CPU_ZERO(&cpuSet);

int st = sched_getaffinity(gPID, sizeof(cpu_set_t), &cpuSet);
if (st != 0)
{
ASSERT("sched_getaffinity failed!\n");
// the sched_getaffinity should never fail for getting affinity of the current process
palError = ERROR_INTERNAL_ERROR;
goto EXIT;
}

st = pthread_attr_setaffinity_np(&pthreadAttr, sizeof(cpu_set_t), &cpuSet);
if (st != 0)
{
if (st == ENOMEM)
{
palError = ERROR_NOT_ENOUGH_MEMORY;
}
else
{
ASSERT("pthread_attr_setaffinity_np failed!\n");
// The pthread_attr_setaffinity_np should never fail except of OOM when
// passed the mask extracted using sched_getaffinity.
palError = ERROR_INTERNAL_ERROR;
}
goto EXIT;
}
}
#endif // HAVE_PTHREAD_GETAFFINITY_NP && HAVE_SCHED_GETAFFINITY

iError = pthread_create(&pthread, &pthreadAttr, CPalThread::ThreadEntry, pNewThread);

#if PTHREAD_CREATE_MODIFIES_ERRNO
Expand Down Expand Up @@ -1754,6 +1719,10 @@ CPalThread::ThreadEntry(
PTHREAD_START_ROUTINE pfnStartRoutine;
LPVOID pvPar;
DWORD retValue;
#if HAVE_SCHED_GETAFFINITY && HAVE_SCHED_SETAFFINITY
cpu_set_t cpuSet;
int st;
#endif

pThread = reinterpret_cast<CPalThread*>(pvParam);

Expand All @@ -1763,6 +1732,42 @@ CPalThread::ThreadEntry(
goto fail;
}

#if HAVE_SCHED_GETAFFINITY && HAVE_SCHED_SETAFFINITY
// Threads inherit their parent's affinity mask on Linux. This is not desired, so we reset
// the current thread's affinity mask to the mask of the current process.
//
// Typically, we would use pthread_attr_setaffinity_np() and have pthread_create() create the thread with the specified
// affinity. At least one implementation of pthread_create() following a pthread_attr_setaffinity_np() calls
// sched_setaffinity(<newThreadPid>, ...), which is not allowed under Snap's default strict confinement without manually
// connecting the process-control plug. To work around that, have the thread set the affinity after it starts.
// sched_setaffinity(<currentThreadPid>, ...) is also currently not allowed, only sched_setaffinity(0, ...).
// pthread_setaffinity_np(pthread_self(), ...) seems to call sched_setaffinity(<currentThreadPid>, ...) in at least one
// implementation, and does not work. Use sched_setaffinity(0, ...) instead. See the following for more information:
// - https://github.com/dotnet/runtime/pull/38795
// - https://github.com/dotnet/runtime/issues/1634
// - https://forum.snapcraft.io/t/requesting-autoconnect-for-interfaces-in-pigmeat-process-control-home/17987/13

CPU_ZERO(&cpuSet);

st = sched_getaffinity(gPID, sizeof(cpu_set_t), &cpuSet);
Copy link
Member

Choose a reason for hiding this comment

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

A nit - zero instead of gPID should work here too.

Copy link
Member Author

@kouvel kouvel Jul 31, 2020 •
edited

Choose a reason for hiding this comment

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

Based on the docs I'm not completely sure that they're the same thing. The previous intention was to get the affinity of the process (or perhaps the thread corresponding with gPID) and apply the same affinity to the new thread. The doc for sched_getaffinity only talks about "process" and not "thread", but I wonder if there is ambiguity there since threads have their own pid the the same way as processes.

Copy link
Member

Choose a reason for hiding this comment

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

You are actually right, my mistake. The goal of this code is to set the affinity to be the same as the affinity of the main thread (which is identified by the same PID as the process). If we used zero here, the operation would be effectively noop, as it would just get the affinity of the current thread and then set it to the same value.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was looking at https://linux.die.net/man/2/sched_getaffinity, it does mention a bit about threads:

The affinity mask is actually a per-thread attribute that can be adjusted independently for each of the threads in a thread group. The value returned from a call to gettid(2) can be passed in the argument pid. Specifying pid as 0 will set the attribute for the calling thread, and passing the value returned from a call to getpid(2) will set the attribute for the main thread of the thread group. (If you are using the POSIX threads API, then use pthread_setaffinity_np(3) instead of sched_setaffinity().)

According to https://man7.org/linux/man-pages/man2/sched_getaffinity.2.html, which talks about threads, it's pretty clear from:

If pid is zero, then the mask of the calling thread is returned.

It doesn't sound like it would mean the same thing.

Copy link

Choose a reason for hiding this comment

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

@jdstrand, is sched_getaffinity restricted in process-id usage, too? i.e. would the usage above fall foul of the same situation that we have with sched_setaffinity that this PR aims to fix?

Copy link
Member Author

@kouvel kouvel Aug 10, 2020 •
edited

Choose a reason for hiding this comment

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

It does not seem to be restricted, only sched_setaffinity is failing from my tests

Copy link

@jdstrand jdstrand Aug 10, 2020 •
edited

Choose a reason for hiding this comment

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

@jdstrand, is sched_getaffinity restricted in process-id usage, too? i.e. would the usage above fall foul of the same situation that we have with sched_setaffinity that this PR aims to fix?

sched_getaffinity is allowed as part of the default template (since it doesn't allow interfering with other snaps and the small info leak isn't worth the developer friction of putting it in another interface).

Copy link

Choose a reason for hiding this comment

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

Awesome :-) thanks for this!

if (st != 0)
{
ASSERT("sched_getaffinity failed!\n");
// The sched_getaffinity should never fail for getting affinity of the current process
palError = ERROR_INTERNAL_ERROR;
goto fail;
}

st = sched_setaffinity(0, sizeof(cpu_set_t), &cpuSet);
if (st != 0)
{
ASSERT("sched_setaffinity failed!\n");
// The sched_setaffinity should never fail when passed the mask extracted using sched_getaffinity
palError = ERROR_INTERNAL_ERROR;
goto fail;
}
#endif // HAVE_SCHED_GETAFFINITY && HAVE_SCHED_SETAFFINITY

#if !HAVE_MACH_EXCEPTIONS
if (!pThread->EnsureSignalAlternateStack())
{
Expand Down Expand Up @@ -2946,18 +2951,31 @@ BOOL
PALAPI
PAL_SetCurrentThreadAffinity(WORD procNo)
{
#if HAVE_PTHREAD_GETAFFINITY_NP
#if HAVE_SCHED_SETAFFINITY || HAVE_PTHREAD_SETAFFINITY_NP
cpu_set_t cpuSet;
CPU_ZERO(&cpuSet);

CPU_SET(procNo, &cpuSet);

// Snap's default strict confinement does not allow sched_setaffinity(<nonzeroPid>, ...) without manually connecting the
// process-control plug. sched_setaffinity(<currentThreadPid>, ...) is also currently not allowed, only
// sched_setaffinity(0, ...). pthread_setaffinity_np(pthread_self(), ...) seems to call
// sched_setaffinity(<currentThreadPid>, ...) in at least one implementation, and does not work. To work around those
// issues, use sched_setaffinity(0, ...) if available and only otherwise fall back to pthread_setaffinity_np(). See the
// following for more information:
// - https://github.com/dotnet/runtime/pull/38795
// - https://github.com/dotnet/runtime/issues/1634
// - https://forum.snapcraft.io/t/requesting-autoconnect-for-interfaces-in-pigmeat-process-control-home/17987/13
#if HAVE_SCHED_SETAFFINITY
int st = sched_setaffinity(0, sizeof(cpu_set_t), &cpuSet);
#else
int st = pthread_setaffinity_np(pthread_self(), sizeof(cpu_set_t), &cpuSet);
#endif

return st == 0;
#else // HAVE_PTHREAD_GETAFFINITY_NP
#else // !(HAVE_SCHED_SETAFFINITY || HAVE_PTHREAD_SETAFFINITY_NP)
// There is no API to manage thread affinity, so let's ignore the request
return FALSE;
#endif // HAVE_PTHREAD_GETAFFINITY_NP
#endif // HAVE_SCHED_SETAFFINITY || HAVE_PTHREAD_SETAFFINITY_NP
}

/*++
Expand Down