Add MemoryBarrier to SyncBlock Lock upgrade path #121355
Conversation
jkoritzinsky
added
area-VM-coreclr
NO-MERGE
|
Tagging subscribers to this area: @mangod9 |
|
/azp run runtime-coreclr gcstress-extra |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…that other threads observe the swapped-in and initialized lock before the thin-lock value is cleared.
jkoritzinsky
force-pushed
the
monitor-fix-testing
branch
from
b9ea6b5 to
ab31426
Compare
|
/azp run runtime-coreclr gcstress-extra |
|
Azure Pipelines successfully started running 1 pipeline(s). |
jkoritzinsky
changed the title
jkoritzinsky
removed
NO-MERGE
There was a problem hiding this comment.
Pull Request Overview
This PR adds a memory barrier after successfully storing a lock handle in a SyncBlock to ensure proper memory ordering when transitioning from thin lock to full lock semantics.
- Adds an explicit
MemoryBarrier()call after the successfulInterlockedCompareExchangeToperation inGetOrCreateLock
…GetOrCreateLock instead of trying to do something lock-free.
…teresting thin lock bits.
jkotas
left a comment
jkotas
left a comment
There was a problem hiding this comment.
Could you please trigger a few GC stress runs to see whether it is fixing the problem (and not introducing any new ones)?
jkoritzinsky
force-pushed
the
monitor-fix-testing
branch
from
44ff651 to
880afbc
Compare
|
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
Looks like I need to modify the timeout handling, which makes sense (especially if a GC happens). |
…ht get a GC on the thread that owns the lock, so spinning for a while makes sense. Also remove more store barriers.
|
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc |
|
Azure Pipelines successfully started running 2 pipeline(s). |
| // | ||
| // *************************************************************************** | ||
|
|
||
| #ifdef MP_LOCKS |
There was a problem hiding this comment.
Delete #define MP_LOCKS from gc.h as well?
| // destructor will reset the CLR debug state to what it was before entering the | ||
| // function, which will undo the BeginNoTriggerGC() call below. | ||
| STATIC_CONTRACT_GC_NOTRIGGER; | ||
| void EnterSpinLock(Volatile<DWORD>* pLock COMMA_INDEBUG(int spinTimeout = INFINITE_TIMEOUT)) |
There was a problem hiding this comment.
I would not bother with the debug-only timeout argument. Delete it instead.
| , m_thinLock() | ||
| , m_dwSyncIndex(indx) | ||
| #ifdef FEATURE_METADATA_UPDATER | ||
| , m_pEnCInfo(PTR_NULL) |
There was a problem hiding this comment.
Also delete ENTER_SPIN_LOCK /LEAVE_SPIN_LOCK macros? They are unnecessary indirection.
| if (result == curValue) | ||
| break; | ||
| } | ||
| if (g_SystemInfo.dwNumberOfProcessors > 1) |
There was a problem hiding this comment.
It looks like the spin loop with YieldProcessorNormalized was unintentionally disabled at some point. We may want to add it back.
2 participants
Fixes #121287
Validation: Every run of runtime-coreclr gcstress-extra has at least one hit of the failure in the link for some out-of-proc test. This PR has no failures of the sort. This is not my favorite form of validation, but I don't think I'm going to get much better as I can't repro locally.
May also fix #121285, but I'm not sure.