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

Some refactoring of thread suspension. #34666

Merged
18 commits merged into from
Oct 27, 2020
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
m_dwForbidSuspendThread
  • Loading branch information
VSadov committed Oct 25, 2020
commit 69a7c527aa42f5d78de5e9151dc7509f6eb13053
22 changes: 15 additions & 7 deletions src/coreclr/src/vm/threads.h
Original file line number Diff line number Diff line change
@@ -1656,9 +1656,12 @@ class Thread
EEThreadId m_Creater;
#endif

// After we suspend a thread, we may need to call EEJitManager::JitCodeToMethodInfo
// or StressLog which may waits on a spinlock. It is unsafe to suspend a thread while it
// is in this state.
// A thread may forbid its own suspension. For example when holding certain locks.
// The state is a counter to allow nested forbids.
// The state is only modified by the current thread.
// Other threads may read this state, but must mind the races.
// It must be assumed that a running thread (or one that may start running) may change this state at any time.
// One exception: SuspendThread can read this "reliably" from other threads by temporarily suspending them, reading, and releasing if != 0.
Volatile<LONG> m_dwForbidSuspendThread;

public:
@@ -1684,7 +1687,8 @@ class Thread
STRESS_LOG2(LF_SYNC, LL_INFO100000, "Set forbid suspend [%d] for thread %p.\n", pThread->m_dwForbidSuspendThread.Load(), pThread);
}
#endif
FastInterlockIncrement(&pThread->m_dwForbidSuspendThread);
// modified only by the current thread, so ++ is ok
pThread->m_dwForbidSuspendThread.RawValue()++;
}
#endif //!DACCESS_COMPILE
}
@@ -1703,8 +1707,10 @@ class Thread
Thread * pThread = GetThreadNULLOk();
if (pThread)
{
_ASSERTE (pThread->m_dwForbidSuspendThread != (LONG)0);
FastInterlockDecrement(&pThread->m_dwForbidSuspendThread);
_ASSERTE (pThread->m_dwForbidSuspendThread >= (LONG)0);

// modified only by the current thread, so -- is ok
pThread->m_dwForbidSuspendThread.RawValue()--;
#ifdef _DEBUG
{
//DEBUG_ONLY;
@@ -1715,9 +1721,11 @@ class Thread
#endif //!DACCESS_COMPILE
}

// Returns the state of m_dwForbidSuspendThread as it was at the time of the call.
// It may asynchronously change if there are no additional guarantees (i.e. it is the current thread or the thread is suspended)
bool IsInForbidSuspendRegion()
{
return m_dwForbidSuspendThread != (LONG)0;
return m_dwForbidSuspendThread.LoadWithoutBarrier() != (LONG)0;
}

typedef StateHolder<Thread::IncForbidSuspendThread, Thread::DecForbidSuspendThread> ForbidSuspendThreadHolder;
14 changes: 11 additions & 3 deletions src/coreclr/src/vm/threadsuspend.cpp
Original file line number Diff line number Diff line change
@@ -191,7 +191,7 @@ Thread::SuspendThreadResult Thread::SuspendThread(BOOL fOneTryOnly, DWORD *pdwSu
}
#endif

Volatile<HANDLE> hThread;
HANDLE hThread;
SuspendThreadResult str = (SuspendThreadResult) -1;
DWORD dwSuspendCount = 0;
DWORD tries = 1;
@@ -237,13 +237,19 @@ Thread::SuspendThreadResult Thread::SuspendThread(BOOL fOneTryOnly, DWORD *pdwSu
{
// We do not want to suspend the target thread while it is holding the log lock.
// By acquiring the lock ourselves, we know that this is not the case.
// Note: LogLock is a noop when not logging, do not assume the rest is running under a lock.
LogLockHolder.Acquire();

// It is important to avoid two threads suspending each other.
// Before a thread suspends another, it increments its own m_dwForbidSuspendThread count first,
// then it checks the target thread's m_dwForbidSuspendThread.
ForbidSuspendThreadHolder forbidSuspend;
if ((m_dwForbidSuspendThread != 0))

// We need a total order between write/read of our/their m_dwForbidSuspendThread here, thus a full fence.
// Note - this is just to prevent mutual suspension of two threads executing this same sequence.
// The other thread may still set its m_dwForbidSuspendThread for other reasons asynchronously.
// We will check on that once the thread is suspended.
if (InterlockedOr(&m_dwForbidSuspendThread, 0))
Copy link
Member Author

@VSadov VSadov Oct 16, 2020

Choose a reason for hiding this comment

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

Another way of looking at this is to consider the following scenario:

  • thread A has incremented its forbid count (but the write is still in the cpu write buffer)
  • thread B has incremented its forbid count (but the write is still in the cpu write buffer)
  • threads A checks the forbid count of thread B. It does not matter volatile read or not, it can't see the changes which are not yet committed to memory on another core.
  • threads B checks the forbid count of thread A. It does not matter volatile read or not, it can't see the changes which are not yet committed to memory on another core.
  • and in some rare case both A and B could proceed to suspend each other at the same time and result in a deadlock. (even on x64)

The part that m_dwForbidSuspendThread is Volatile is not helping. It is not necessary in other places where we read it from the current thread of from a suspended thread, and it is not sufficient here. When two live threads try to coordinate their forbids, they need to make sure that the reads do not happen earlier than writes.
InterlockedOr is a one way to provide that. Full memory fence immediately before the read could be another option.

I did not remove Volatile from m_dwForbidSuspendThread, though it does not seems very useful. #Resolved

{
#if defined(_DEBUG)
// Enable the diagnostic ::SuspendThread() if the
@@ -273,11 +279,13 @@ Thread::SuspendThreadResult Thread::SuspendThread(BOOL fOneTryOnly, DWORD *pdwSu
}
}
}

if ((int)dwSuspendCount >= 0)
{
if (hThread == GetThreadHandle())
{
if (m_dwForbidSuspendThread != 0)
// read m_dwForbidSuspendThread again after suspension, it may have changed.
if (m_dwForbidSuspendThread.LoadWithoutBarrier() != 0)
{
#if defined(_DEBUG)
// Log diagnostic below 8 times during the i64TimestampTicksMax period