Relaxed memory order issues

[heiher]

We found that lazy initialization will cause errors on RMO(Relaxed Memory Order) platforms, which are used on both Roslyn and CoreFX.

https://github.com/dotnet/roslyn/blob/master/src/Compilers/CSharp/Portable/Binder/Binder.cs#L493-L505

runtime/src/libraries/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/PEReader.cs

Lines 338 to 351 in da90d08

	private AbstractMemoryBlock GetEntireImageBlock()
	{
	if (_lazyImageBlock == null)
	{
	var newBlock = GetPEImage().GetMemoryBlock();
	if (Interlocked.CompareExchange(ref _lazyImageBlock, newBlock, null) != null)
	{
	// another thread created the block already, we need to dispose ours:
	newBlock.Dispose();
	}
	}
	return _lazyImageBlock;
	}

We use a unit test case(https://github.com/dotnet-hev/load-load-reordering) to explain what happened:

private static object LazyInit(ref object _lazyObj)
{
	if (_lazyObj == null)
		Interlocked.CompareExchange(ref _lazyObj, new object(), null);

	// RMO: Prevent reordering for two loads for the same address:
	// {
	//	 * Line 3: if (_lazyObj == null)
	//	 * Line 20: return _lazyObj;
	// }
	// 
	// When LazyInit returns null, observe in global time, the actual
	// execution order of &_lazyObj access is:
	//  1. Thread 1: Load &_lazyObj for return, the value is null.
	//  2. Thread 2: Load &_lazyObj for branch, the value is null,
	//                      Store an new object to &_lazyObj.
	//  3. Thread 1: Load &_lazyObj for branch, the value is not null,
	//                      Return null.
	//
	// Interlocked.MemoryBarrier();
	return _lazyObj;
}

Root Cause:
In LazyInit, the two loads for &_lazyObj are out of ordered, the loads for return is executed before loads for branch, the stores on another thread happens to be between these two loads, so store updates only for loads for branch. although accesses are for the same address.

Why ARM no issue?
Because ARM/ARM64 is not a fully RMO micro-architecture, they have constraints on the order of loads for the same address.

Why isn't easy to observe in native(C/C++)?
Because native compilers usually do local assignment propagation optimization, the result of loads used for branch is also used for return.

How to fix?
We may need to think about questions first:

1. Is the C# code implementation correct for RMO? Memory barrier?
2. How to cover the earlier CIL binaries(from NuGet)?

Whether problem 1 is right or wrong is not easy to solve problem 2, unless it is fixed in coreclr, but it seems that it is difficult to accurately match the scene for local assignment propagation or memory barrier insertion.

Any suggestions please?
Thanks!

@jkotas

[jkotas]

@vargaz How does Mono deal with this issue today?

[vargaz]

After starting to use roslyn/corefx code, we had a lot of random failures on arm/arm64, so we started treating reference type stores as volatile, and emitted release memory barriers after the stores.
For our native/bcl code, we use explicit memory barriers in the pattern:

• construct
• memory barrier
• publish

[jkotas]

You can find some more discussion on this topic in #4906 .

I think that the simple rule should be to use volatile for anything lock-free. As you have observed, there is a lot of code out there that does not follow this rule. I do think there is a great solution to solve this problem. You can consider having two modes:

• Fast mode that assumes all lock-free code has proper barrier
• Slow mode that adds the barriers defensively everywhere

[heiher]

Thank you very much, I got the simple rule.

We should modify the code to follow this simple rule.

However, I don't have a better way for the released portable binary program (From NuGet), if we need to ensure safe to execute it on RMO platforms, it seems that we can only use the slow mode: if we cannot found whether IL is lock-free associated access, we need to automatically use volatile for all indir accesses at JIT compiling, this will generates a lot of unnecessary memory barrier instructions.

Is there any other way to reduce performance loss?