From 14ce9e15672d03cb6fc067316f90d81471398ebc Mon Sep 17 00:00:00 2001 From: riperiperi Date: Sat, 30 Jul 2022 00:16:29 +0200 Subject: Move partial unmap handler to the native signal handler (#3437) * Initial commit with a lot of testing stuff. * Partial Unmap Cleanup Part 1 * Fix some minor issues, hopefully windows tests. * Disable partial unmap tests on macos for now Weird issue. * Goodbye magic number * Add COMPlus_EnableAlternateStackCheck for tests `COMPlus_EnableAlternateStackCheck` is needed for NullReferenceException handling to work on linux after registering the signal handler, due to how dotnet registers its own signal handler. * Address some feedback * Force retry when memory is mapped in memory tracking This case existed before, but returning `false` no longer retries, so it would crash immediately after unprotecting the memory... Now, we return `true` to deliberately retry. This case existed before (was just broken by this change) and I don't really want to look into fixing the issue right now. Technically, this means that on guest code partial unmaps will retry _due to this_ rather than hitting the handler. I don't expect this to cause any issues. This should fix random crashes in Xenoblade Chronicles 2. * Use IsRangeMapped * Suppress MockMemoryManager.UnmapEvent warning This event is not signalled by the mock memory manager. * Remove 4kb mapping --- Ryujinx.Memory/WindowsShared/PlaceholderManager.cs | 139 ++++++++++++--------- 1 file changed, 77 insertions(+), 62 deletions(-) (limited to 'Ryujinx.Memory/WindowsShared/PlaceholderManager.cs') diff --git a/Ryujinx.Memory/WindowsShared/PlaceholderManager.cs b/Ryujinx.Memory/WindowsShared/PlaceholderManager.cs index 1b425d66..0937d462 100644 --- a/Ryujinx.Memory/WindowsShared/PlaceholderManager.cs +++ b/Ryujinx.Memory/WindowsShared/PlaceholderManager.cs @@ -1,5 +1,7 @@ +using Ryujinx.Common.Memory.PartialUnmaps; using System; using System.Diagnostics; +using System.Runtime.CompilerServices; using System.Runtime.Versioning; using System.Threading; @@ -13,13 +15,10 @@ namespace Ryujinx.Memory.WindowsShared { private const ulong MinimumPageSize = 0x1000; - [ThreadStatic] - private static int _threadLocalPartialUnmapsCount; - private readonly IntervalTree _mappings; private readonly IntervalTree _protections; - private readonly ReaderWriterLock _partialUnmapLock; - private int _partialUnmapsCount; + private readonly IntPtr _partialUnmapStatePtr; + private readonly Thread _partialUnmapTrimThread; /// /// Creates a new instance of the Windows memory placeholder manager. @@ -28,7 +27,35 @@ namespace Ryujinx.Memory.WindowsShared { _mappings = new IntervalTree(); _protections = new IntervalTree(); - _partialUnmapLock = new ReaderWriterLock(); + + _partialUnmapStatePtr = PartialUnmapState.GlobalState; + + _partialUnmapTrimThread = new Thread(TrimThreadLocalMapLoop); + _partialUnmapTrimThread.Name = "CPU.PartialUnmapTrimThread"; + _partialUnmapTrimThread.IsBackground = true; + _partialUnmapTrimThread.Start(); + } + + /// + /// Gets a reference to the partial unmap state struct. + /// + /// A reference to the partial unmap state struct + private unsafe ref PartialUnmapState GetPartialUnmapState() + { + return ref Unsafe.AsRef((void*)_partialUnmapStatePtr); + } + + /// + /// Trims inactive threads from the partial unmap state's thread mapping every few seconds. + /// Should be run in a Background thread so that it doesn't stop the program from closing. + /// + private void TrimThreadLocalMapLoop() + { + while (true) + { + Thread.Sleep(2000); + GetPartialUnmapState().TrimThreads(); + } } /// @@ -98,7 +125,8 @@ namespace Ryujinx.Memory.WindowsShared /// Memory block that owns the mapping public void MapView(IntPtr sharedMemory, ulong srcOffset, IntPtr location, IntPtr size, MemoryBlock owner) { - _partialUnmapLock.AcquireReaderLock(Timeout.Infinite); + ref var partialUnmapLock = ref GetPartialUnmapState().PartialUnmapLock; + partialUnmapLock.AcquireReaderLock(); try { @@ -107,7 +135,7 @@ namespace Ryujinx.Memory.WindowsShared } finally { - _partialUnmapLock.ReleaseReaderLock(); + partialUnmapLock.ReleaseReaderLock(); } } @@ -221,7 +249,8 @@ namespace Ryujinx.Memory.WindowsShared /// Memory block that owns the mapping public void UnmapView(IntPtr sharedMemory, IntPtr location, IntPtr size, MemoryBlock owner) { - _partialUnmapLock.AcquireReaderLock(Timeout.Infinite); + ref var partialUnmapLock = ref GetPartialUnmapState().PartialUnmapLock; + partialUnmapLock.AcquireReaderLock(); try { @@ -229,7 +258,7 @@ namespace Ryujinx.Memory.WindowsShared } finally { - _partialUnmapLock.ReleaseReaderLock(); + partialUnmapLock.ReleaseReaderLock(); } } @@ -265,11 +294,6 @@ namespace Ryujinx.Memory.WindowsShared if (IsMapped(overlap.Value)) { - if (!WindowsApi.UnmapViewOfFile2(WindowsApi.CurrentProcessHandle, (IntPtr)overlap.Start, 2)) - { - throw new WindowsApiException("UnmapViewOfFile2"); - } - // Tree operations might modify the node start/end values, so save a copy before we modify the tree. ulong overlapStart = overlap.Start; ulong overlapEnd = overlap.End; @@ -291,30 +315,46 @@ namespace Ryujinx.Memory.WindowsShared // This is necessary because Windows does not support partial view unmaps. // That is, you can only fully unmap a view that was previously mapped, you can't just unmap a chunck of it. - LockCookie lockCookie = _partialUnmapLock.UpgradeToWriterLock(Timeout.Infinite); - - _partialUnmapsCount++; + ref var partialUnmapState = ref GetPartialUnmapState(); + ref var partialUnmapLock = ref partialUnmapState.PartialUnmapLock; + partialUnmapLock.UpgradeToWriterLock(); - if (overlapStartsBefore) + try { - ulong remapSize = startAddress - overlapStart; - - MapViewInternal(sharedMemory, overlapValue, (IntPtr)overlapStart, (IntPtr)remapSize); - RestoreRangeProtection(overlapStart, remapSize); + partialUnmapState.PartialUnmapsCount++; + + if (!WindowsApi.UnmapViewOfFile2(WindowsApi.CurrentProcessHandle, (IntPtr)overlapStart, 2)) + { + throw new WindowsApiException("UnmapViewOfFile2"); + } + + if (overlapStartsBefore) + { + ulong remapSize = startAddress - overlapStart; + + MapViewInternal(sharedMemory, overlapValue, (IntPtr)overlapStart, (IntPtr)remapSize); + RestoreRangeProtection(overlapStart, remapSize); + } + + if (overlapEndsAfter) + { + ulong overlappedSize = endAddress - overlapStart; + ulong remapBackingOffset = overlapValue + overlappedSize; + ulong remapAddress = overlapStart + overlappedSize; + ulong remapSize = overlapEnd - endAddress; + + MapViewInternal(sharedMemory, remapBackingOffset, (IntPtr)remapAddress, (IntPtr)remapSize); + RestoreRangeProtection(remapAddress, remapSize); + } } - - if (overlapEndsAfter) + finally { - ulong overlappedSize = endAddress - overlapStart; - ulong remapBackingOffset = overlapValue + overlappedSize; - ulong remapAddress = overlapStart + overlappedSize; - ulong remapSize = overlapEnd - endAddress; - - MapViewInternal(sharedMemory, remapBackingOffset, (IntPtr)remapAddress, (IntPtr)remapSize); - RestoreRangeProtection(remapAddress, remapSize); + partialUnmapLock.DowngradeFromWriterLock(); } - - _partialUnmapLock.DowngradeFromWriterLock(ref lockCookie); + } + else if (!WindowsApi.UnmapViewOfFile2(WindowsApi.CurrentProcessHandle, (IntPtr)overlapStart, 2)) + { + throw new WindowsApiException("UnmapViewOfFile2"); } } } @@ -394,7 +434,8 @@ namespace Ryujinx.Memory.WindowsShared /// True if the reprotection was successful, false otherwise public bool ReprotectView(IntPtr address, IntPtr size, MemoryPermission permission) { - _partialUnmapLock.AcquireReaderLock(Timeout.Infinite); + ref var partialUnmapLock = ref GetPartialUnmapState().PartialUnmapLock; + partialUnmapLock.AcquireReaderLock(); try { @@ -402,7 +443,7 @@ namespace Ryujinx.Memory.WindowsShared } finally { - _partialUnmapLock.ReleaseReaderLock(); + partialUnmapLock.ReleaseReaderLock(); } } @@ -659,31 +700,5 @@ namespace Ryujinx.Memory.WindowsShared ReprotectViewInternal((IntPtr)protAddress, (IntPtr)(protEndAddress - protAddress), protection.Value, true); } } - - /// - /// Checks if an access violation handler should retry execution due to a fault caused by partial unmap. - /// - /// - /// Due to Windows limitations, might need to unmap more memory than requested. - /// The additional memory that was unmapped is later remapped, however this leaves a time gap where the - /// memory might be accessed but is unmapped. Users of the API must compensate for that by catching the - /// access violation and retrying if it happened between the unmap and remap operation. - /// This method can be used to decide if retrying in such cases is necessary or not. - /// - /// True if execution should be retried, false otherwise - public bool RetryFromAccessViolation() - { - _partialUnmapLock.AcquireReaderLock(Timeout.Infinite); - - bool retry = _threadLocalPartialUnmapsCount != _partialUnmapsCount; - if (retry) - { - _threadLocalPartialUnmapsCount = _partialUnmapsCount; - } - - _partialUnmapLock.ReleaseReaderLock(); - - return retry; - } } } \ No newline at end of file -- cgit v1.2.3