Uh oh!
There was an error while loading. Please reload this page.
This is an attempt to fix the remaining GC stress bugs on ARM64. - #40931
This is an attempt to fix the remaining GC stress bugs on ARM64.#40931PeterSolMS wants to merge 1 commit into
Conversation
I found a dump for an AV in background_sweep where we had crashed due to a null method table, yet the object pointed at was a valid free object. So the theory is that a foreground GC allocated an object in gen 2, but the background GC thread doesn't have an up-to-date picture of memory. The synchronization between foreground and background GC is via allow_fgc() which switches from cooperative GC mode to preemptive mode and back. If the timing is such that the background GC thread never sees m_fPreemptiveGCDisabled being true, then we may not execute a a memory barrier. The fix tries to remedy this situation by introducing an interlocked instruction in DisablePreemptiveGC for non Intel-architecture targets. Whether this actually fixes the issues we are seeing is not clear yet.
ghost
commented
Aug 17, 2020
Tagging subscribers to this area: @dotnet/gc |
echesakov
commented
Aug 17, 2020
I triggered gcstress-extra job - https://dev.azure.com/dnceng/public/_build/results?buildId=775013&view=results Let's see if it passes with this change |
PeterSolMS
commented
Aug 18, 2020
No, we still observe the same failures, e.g. in the type generator tests. Back to the drawing board... |
AndyAyersMS
commented
Aug 18, 2020
Wonder if we are hitting this case in the inlined pinvoke stubs too -- we flip the thread in and out of preemptive mode there too by simply doing stores. |
| m_fPreemptiveGCDisabled.StoreWithoutBarrier(1); | ||
| #else | ||
| // weaker memory models need an interlocked operation to ensure consistency | ||
| FastInterlockOr(&m_fPreemptiveGCDisabled, 1); |
There was a problem hiding this comment.
We should not be using any barrier here even for weak architectures. It would have significant performance consequences. @jkotas mentioned just recently that it is not unusual to see workloads that flip this flag million or more times per second. Reading that flag happens on many orders slower rate, basically just during GC suspension.
So our synchronization model is based on writing to the m_fPreemptiveGCDisabled without any barrier and executing process wide memory barrier (FlushProcessWriteBuffers) at the read time.
There was a problem hiding this comment.
right we don't need to do this. if a barrier is actually needed (which I'm not sure that it is...still thinking), we can add one on the GC side only when a suspension is required.
In reply to: 472863596 [](ancestors = 472863596)
PeterSolMS
commented
Aug 21, 2020
As it turns out, the AV in gc_heap::background_sweep had a different cause - a race condition caused by inserting an object in GCHeap::StressHeap. Nevertheless, I still worry about issues with on architectures with weak ordering constraints either with background_gc or pinvoke. For this to become an issue, we'd have to just miss g_TrapReturningThreads being true on both the way in and the way out - if we see as being true, we will execute an interlocked instruction. I actually don't know if there's a problem - I'll see if I can write a test that provokes it. |
I found a dump for an AV in background_sweep where we had crashed due to a null method table, yet the object pointed at was a valid free object. The AV was at coreclr!WKS::gc_heap::background_sweep+0x668, we load the method table to compute the size of the object:
As a free object can only be generated by the GC itself, the theory is that a foreground GC allocated an object in gen 2 and created a free object for the unused space, but the background GC thread doesn't have an up-to-date picture of memory. The synchronization between foreground and background GC is via allow_fgc() which switches from cooperative GC mode to preemptive mode and back. If the timing is such that the background GC thread never sees m_fPreemptiveGCDisabled being true, then we may not execute a a memory barrier.
The fix tries to remedy this situation by introducing an interlocked instruction in DisablePreemptiveGC for non Intel-architecture targets. Whether this actually fixes the issues we are seeing is not clear yet.