From a539303e7165cf524dc5b5750da49cb4bd4be6d6 Mon Sep 17 00:00:00 2001 From: riperiperi Date: Sat, 6 Mar 2021 23:21:53 +0000 Subject: [PATCH] Remove unused physical region tracking (#2085) * Remove unused physical region tracking * Update comments --- Ryujinx.Cpu/MemoryManager.cs | 3 +- .../MultiRegionTrackingTests.cs | 10 +- Ryujinx.Memory.Tests/TrackingTests.cs | 53 ++-------- Ryujinx.Memory/Tracking/MemoryTracking.cs | 97 ++----------------- Ryujinx.Memory/Tracking/PhysicalRegion.cs | 97 ------------------- Ryujinx.Memory/Tracking/RegionHandle.cs | 2 +- Ryujinx.Memory/Tracking/VirtualRegion.cs | 78 +-------------- 7 files changed, 24 insertions(+), 316 deletions(-) delete mode 100644 Ryujinx.Memory/Tracking/PhysicalRegion.cs diff --git a/Ryujinx.Cpu/MemoryManager.cs b/Ryujinx.Cpu/MemoryManager.cs index 59654e8b..591299ca 100644 --- a/Ryujinx.Cpu/MemoryManager.cs +++ b/Ryujinx.Cpu/MemoryManager.cs @@ -115,9 +115,9 @@ namespace Ryujinx.Cpu AssertValidAddressAndSize(va, size); UnmapEvent?.Invoke(va, size); + Tracking.Unmap(va, size); ulong remainingSize = size; - ulong oVa = va; while (remainingSize != 0) { _pageTable.Write((va / PageSize) * PteSize, 0UL); @@ -125,7 +125,6 @@ namespace Ryujinx.Cpu va += PageSize; remainingSize -= PageSize; } - Tracking.Unmap(oVa, size); } /// diff --git a/Ryujinx.Memory.Tests/MultiRegionTrackingTests.cs b/Ryujinx.Memory.Tests/MultiRegionTrackingTests.cs index ff8ab749..6959b8c4 100644 --- a/Ryujinx.Memory.Tests/MultiRegionTrackingTests.cs +++ b/Ryujinx.Memory.Tests/MultiRegionTrackingTests.cs @@ -256,12 +256,12 @@ namespace Ryujinx.Memory.Tests const int pageCount = 32; const int overlapStart = 16; - Assert.AreEqual((0, 0), _tracking.GetRegionCounts()); + Assert.AreEqual(0, _tracking.GetRegionCount()); IMultiRegionHandle handleLow = GetGranular(smart, 0, PageSize * pageCount, PageSize); PreparePages(handleLow, pageCount); - Assert.AreEqual((pageCount, pageCount), _tracking.GetRegionCounts()); + Assert.AreEqual(pageCount, _tracking.GetRegionCount()); IMultiRegionHandle handleHigh = GetGranular(smart, PageSize * overlapStart, PageSize * pageCount, PageSize); PreparePages(handleHigh, pageCount, PageSize * overlapStart); @@ -269,15 +269,15 @@ namespace Ryujinx.Memory.Tests // Combined pages (and assuming overlapStart <= pageCount) should be pageCount after overlapStart. int totalPages = overlapStart + pageCount; - Assert.AreEqual((totalPages, totalPages), _tracking.GetRegionCounts()); + Assert.AreEqual(totalPages, _tracking.GetRegionCount()); handleLow.Dispose(); // After disposing one, the pages for the other remain. - Assert.AreEqual((pageCount, pageCount), _tracking.GetRegionCounts()); + Assert.AreEqual(pageCount, _tracking.GetRegionCount()); handleHigh.Dispose(); // After disposing the other, there are no pages left. - Assert.AreEqual((0, 0), _tracking.GetRegionCounts()); + Assert.AreEqual(0, _tracking.GetRegionCount()); } } } diff --git a/Ryujinx.Memory.Tests/TrackingTests.cs b/Ryujinx.Memory.Tests/TrackingTests.cs index a9cc6df3..37a2b867 100644 --- a/Ryujinx.Memory.Tests/TrackingTests.cs +++ b/Ryujinx.Memory.Tests/TrackingTests.cs @@ -32,17 +32,12 @@ namespace Ryujinx.Memory.Tests _memoryBlock.Dispose(); } - private bool TestSingleWrite(RegionHandle handle, ulong address, ulong size, bool physical = false) + private bool TestSingleWrite(RegionHandle handle, ulong address, ulong size) { handle.Reprotect(); - if (physical) - { - _tracking.PhysicalMemoryEvent(address, true); - } - else - { - _tracking.VirtualMemoryEvent(address, size, true); - } + + _tracking.VirtualMemoryEvent(address, size, true); + return handle.Dirty; } @@ -97,9 +92,6 @@ namespace Ryujinx.Memory.Tests bool dirtyAfterDispose = TestSingleWrite(handle, 0, 4); Assert.False(dirtyAfterDispose); // Handle cannot be triggered when disposed - - bool dirtyAfterDispose2 = TestSingleWrite(handle, 0, 4, true); - Assert.False(dirtyAfterDispose2); } [Test] @@ -362,33 +354,6 @@ namespace Ryujinx.Memory.Tests Assert.AreEqual(registeredCount, triggeredCount + 1); } - [Test] - public void PhysicalMemoryMapping() - { - // Tracking is done in the virtual space usually, but we also support tracking on physical regions. - // The physical regions that make up a virtual region are determined when the region is created, - // or when a mapping changes. - - // These tests verify that the region cannot be signalled after unmapping, and can after remapping. - - RegionHandle handle = _tracking.BeginTracking(PageSize, PageSize); - - Assert.True(handle.Dirty); - - bool trackedWriteTriggers = TestSingleWrite(handle, PageSize, 1, true); - Assert.True(trackedWriteTriggers); - - _memoryManager.NoMappings = true; - _tracking.Unmap(PageSize, PageSize); - bool unmappedWriteTriggers = TestSingleWrite(handle, PageSize, 1, true); - Assert.False(unmappedWriteTriggers); - - _memoryManager.NoMappings = false; - _tracking.Map(PageSize, PageSize, PageSize); - bool remappedWriteTriggers = TestSingleWrite(handle, PageSize, 1, true); - Assert.True(remappedWriteTriggers); - } - [Test] public void DisposeHandles() { @@ -397,11 +362,11 @@ namespace Ryujinx.Memory.Tests RegionHandle handle = _tracking.BeginTracking(0, PageSize); handle.Reprotect(); - Assert.AreEqual((1, 1), _tracking.GetRegionCounts()); + Assert.AreEqual(1, _tracking.GetRegionCount()); handle.Dispose(); - Assert.AreEqual((0, 0), _tracking.GetRegionCounts()); + Assert.AreEqual(0, _tracking.GetRegionCount()); // Two handles, small entirely contains big. // We expect there to be three regions after creating both, one for the small region and two covering the big one around it. @@ -410,16 +375,16 @@ namespace Ryujinx.Memory.Tests RegionHandle handleSmall = _tracking.BeginTracking(PageSize, PageSize); RegionHandle handleBig = _tracking.BeginTracking(0, PageSize * 4); - Assert.AreEqual((3, 3), _tracking.GetRegionCounts()); + Assert.AreEqual(3, _tracking.GetRegionCount()); // After disposing the big region, only the small one will remain. handleBig.Dispose(); - Assert.AreEqual((1, 1), _tracking.GetRegionCounts()); + Assert.AreEqual(1, _tracking.GetRegionCount()); handleSmall.Dispose(); - Assert.AreEqual((0, 0), _tracking.GetRegionCounts()); + Assert.AreEqual(0, _tracking.GetRegionCount()); } [Test] diff --git a/Ryujinx.Memory/Tracking/MemoryTracking.cs b/Ryujinx.Memory/Tracking/MemoryTracking.cs index 6485e566..425552f8 100644 --- a/Ryujinx.Memory/Tracking/MemoryTracking.cs +++ b/Ryujinx.Memory/Tracking/MemoryTracking.cs @@ -13,11 +13,9 @@ namespace Ryujinx.Memory.Tracking // Only use these from within the lock. private readonly NonOverlappingRangeList _virtualRegions; - private readonly NonOverlappingRangeList _physicalRegions; // Only use these from within the lock. private readonly VirtualRegion[] _virtualResults = new VirtualRegion[10]; - private readonly PhysicalRegion[] _physicalResults = new PhysicalRegion[10]; private readonly int _pageSize; @@ -43,7 +41,6 @@ namespace Ryujinx.Memory.Tracking _pageSize = pageSize; _virtualRegions = new NonOverlappingRangeList(); - _physicalRegions = new NonOverlappingRangeList(); } private (ulong address, ulong size) PageAlign(ulong address, ulong size) @@ -82,7 +79,6 @@ namespace Ryujinx.Memory.Tracking region.SignalMappingChanged(true); } - region.RecalculatePhysicalChildren(); region.UpdateProtection(); } } @@ -90,14 +86,14 @@ namespace Ryujinx.Memory.Tracking /// /// Indicate that a virtual region has been unmapped. - /// Should be called after the unmapping is complete. + /// Should be called before the unmapping is complete. /// /// Virtual memory address /// Size to be unmapped public void Unmap(ulong va, ulong size) { // An unmapping may mean we need to re-evaluate each VirtualRegion's affected area. - // Find all handles that overlap with the range, we need to recalculate their physical regions + // Find all handles that overlap with the range, we need to notify them that the region was unmapped. lock (TrackingLock) { @@ -107,8 +103,8 @@ namespace Ryujinx.Memory.Tracking for (int i = 0; i < count; i++) { VirtualRegion region = results[i]; + region.SignalMappingChanged(false); - region.RecalculatePhysicalChildren(); } } } @@ -127,31 +123,6 @@ namespace Ryujinx.Memory.Tracking return result; } - /// - /// Get a list of physical regions that a virtual region covers. - /// Note that this becomes outdated if the virtual or physical regions are unmapped or remapped. - /// - /// Virtual memory address - /// Size of the virtual region - /// A list of physical regions the virtual region covers - internal List GetPhysicalRegionsForVirtual(ulong va, ulong size) - { - List result = new List(); - - // Get a list of physical regions for this virtual region, from our injected virtual mapping function. - (ulong Address, ulong Size)[] physicalRegions = _memoryManager.GetPhysicalRegions(va, size); - - if (physicalRegions != null) - { - foreach (var region in physicalRegions) - { - _physicalRegions.GetOrAddRegions(result, region.Address, region.Size, (pa, size) => new PhysicalRegion(this, pa, size)); - } - } - - return result; - } - /// /// Remove a virtual region from the range list. This assumes that the lock has been acquired. /// @@ -161,15 +132,6 @@ namespace Ryujinx.Memory.Tracking _virtualRegions.Remove(region); } - /// - /// Remove a physical region from the range list. This assumes that the lock has been acquired. - /// - /// Region to remove - internal void RemovePhysical(PhysicalRegion region) - { - _physicalRegions.Remove(region); - } - /// /// Obtains a memory tracking handle for the given virtual region, with a specified granularity. This should be disposed when finished with. /// @@ -216,38 +178,6 @@ namespace Ryujinx.Memory.Tracking } } - /// - /// Signal that a physical memory event happened at the given location. - /// - /// Physical address accessed - /// Whether the region was written to or read - /// True if the event triggered any tracking regions, false otherwise - public bool PhysicalMemoryEvent(ulong address, bool write) - { - // Look up the physical region using the region list. - // Signal up the chain to relevant handles. - - lock (TrackingLock) - { - var results = _physicalResults; - int count = _physicalRegions.FindOverlapsNonOverlapping(address, 1, ref results); // TODO: get/use the actual access size? - - if (count == 0) - { - _block.Reprotect(address & ~(ulong)(_pageSize - 1), (ulong)_pageSize, MemoryPermission.ReadAndWrite); - return false; // We can't handle this - unprotect and return. - } - - for (int i = 0; i < count; i++) - { - PhysicalRegion region = results[i]; - region.Signal(address, 1, write); - } - } - - return true; - } - /// /// Signal that a virtual memory event happened at the given location (one byte). /// @@ -292,19 +222,6 @@ namespace Ryujinx.Memory.Tracking return true; } - /// - /// Reprotect a given physical region, if enabled. This is protected on the memory block provided during initialization. - /// - /// Region to reprotect - /// Memory permission to protect with - internal void ProtectPhysicalRegion(PhysicalRegion region, MemoryPermission permission) - { - if (EnablePhysicalProtection) - { - _block.Reprotect(region.Address, region.Size, permission); - } - } - /// /// Reprotect a given virtual region. The virtual memory manager will handle this. /// @@ -316,15 +233,15 @@ namespace Ryujinx.Memory.Tracking } /// - /// Returns the number of virtual and physical regions currently being tracked. + /// Returns the number of virtual regions currently being tracked. /// Useful for tests and metrics. /// - /// The number of virtual regions, and the number of physical regions - public (int VirtualCount, int PhysicalCount) GetRegionCounts() + /// The number of virtual regions + public int GetRegionCount() { lock (TrackingLock) { - return (_virtualRegions.Count, _physicalRegions.Count); + return _virtualRegions.Count; } } } diff --git a/Ryujinx.Memory/Tracking/PhysicalRegion.cs b/Ryujinx.Memory/Tracking/PhysicalRegion.cs deleted file mode 100644 index 683186b1..00000000 --- a/Ryujinx.Memory/Tracking/PhysicalRegion.cs +++ /dev/null @@ -1,97 +0,0 @@ -using Ryujinx.Memory.Range; -using System.Collections.Generic; - -namespace Ryujinx.Memory.Tracking -{ - /// - /// A region of physical memory. - /// - class PhysicalRegion : AbstractRegion - { - public List VirtualParents = new List(); - public MemoryPermission Protection { get; private set; } - public MemoryTracking Tracking; - - public PhysicalRegion(MemoryTracking tracking, ulong address, ulong size) : base(address, size) - { - Tracking = tracking; - Protection = MemoryPermission.ReadAndWrite; - } - - public override void Signal(ulong address, ulong size, bool write) - { - Protection = MemoryPermission.ReadAndWrite; - Tracking.ProtectPhysicalRegion(this, MemoryPermission.ReadAndWrite); // Remove our protection immedately. - foreach (var parent in VirtualParents) - { - parent.Signal(address, size, write); - } - } - - /// - /// Update the protection of this region, based on our parent's requested protection. - /// - public void UpdateProtection() - { - // Re-evaluate protection, and commit to the block. - - lock (Tracking.TrackingLock) - { - MemoryPermission result = MemoryPermission.ReadAndWrite; - foreach (var parent in VirtualParents) - { - result &= parent.GetRequiredPermission(); - if (result == 0) break; - } - - if (Protection != result) - { - Protection = result; - Tracking.ProtectPhysicalRegion(this, result); - } - } - } - - public override INonOverlappingRange Split(ulong splitAddress) - { - PhysicalRegion newRegion = new PhysicalRegion(Tracking, splitAddress, EndAddress - splitAddress); - Size = splitAddress - Address; - - // The new region inherits all of our parents. - newRegion.VirtualParents = new List(VirtualParents); - foreach (var parent in VirtualParents) - { - parent.AddChild(newRegion); - } - - return newRegion; - } - - /// - /// Remove a parent virtual region from this physical region. Assumes that the tracking lock has been obtained. - /// - /// Region to remove - /// True if there are no more parents and we should be removed, false otherwise. - public bool RemoveParent(VirtualRegion region) - { - VirtualParents.Remove(region); - UpdateProtection(); - if (VirtualParents.Count == 0) - { - return true; - } - return false; - } - - /// - /// Deletes this physical region if there are no more virtual parents. - /// - public void TryDelete() - { - if (VirtualParents.Count == 0) - { - Tracking.RemovePhysical(this); - } - } - } -} diff --git a/Ryujinx.Memory/Tracking/RegionHandle.cs b/Ryujinx.Memory/Tracking/RegionHandle.cs index da3ee99a..5c32fba4 100644 --- a/Ryujinx.Memory/Tracking/RegionHandle.cs +++ b/Ryujinx.Memory/Tracking/RegionHandle.cs @@ -159,7 +159,7 @@ namespace Ryujinx.Memory.Tracking } /// - /// Dispose the handle. Within the tracking lock, this removes references from virtual and physical regions. + /// Dispose the handle. Within the tracking lock, this removes references from virtual regions. /// public void Dispose() { diff --git a/Ryujinx.Memory/Tracking/VirtualRegion.cs b/Ryujinx.Memory/Tracking/VirtualRegion.cs index fcf2fbe0..696d3560 100644 --- a/Ryujinx.Memory/Tracking/VirtualRegion.cs +++ b/Ryujinx.Memory/Tracking/VirtualRegion.cs @@ -9,15 +9,12 @@ namespace Ryujinx.Memory.Tracking class VirtualRegion : AbstractRegion { public List Handles = new List(); - private List _physicalChildren; private readonly MemoryTracking _tracking; public VirtualRegion(MemoryTracking tracking, ulong address, ulong size) : base(address, size) { _tracking = tracking; - - UpdatePhysicalChildren(); } public override void Signal(ulong address, ulong size, bool write) @@ -30,42 +27,6 @@ namespace Ryujinx.Memory.Tracking UpdateProtection(); } - /// - /// Clears all physical children of this region. Assumes that the tracking lock has been obtained. - /// - private void ClearPhysicalChildren() - { - if (_physicalChildren != null) - { - foreach (PhysicalRegion child in _physicalChildren) - { - child.RemoveParent(this); - } - } - } - - /// - /// Updates the physical children of this region, assuming that they are clear and that the tracking lock has been obtained. - /// - private void UpdatePhysicalChildren() - { - _physicalChildren = _tracking.GetPhysicalRegionsForVirtual(Address, Size); - - foreach (PhysicalRegion child in _physicalChildren) - { - child.VirtualParents.Add(this); - } - } - - /// - /// Recalculates the physical children for this virtual region. Assumes that the tracking lock has been obtained. - /// - public void RecalculatePhysicalChildren() - { - ClearPhysicalChildren(); - UpdatePhysicalChildren(); - } - /// /// Signal that this region has been mapped or unmapped. /// @@ -98,20 +59,11 @@ namespace Ryujinx.Memory.Tracking } /// - /// Updates the protection for this virtual region, and all child physical regions. + /// Updates the protection for this virtual region. /// public void UpdateProtection() { - // Re-evaluate protection for all physical children. - _tracking.ProtectVirtualRegion(this, GetRequiredPermission()); - lock (_tracking.TrackingLock) - { - foreach (var child in _physicalChildren) - { - child.UpdateProtection(); - } - } } /// @@ -120,7 +72,6 @@ namespace Ryujinx.Memory.Tracking /// Handle to remove public void RemoveHandle(RegionHandle handle) { - bool removedRegions = false; lock (_tracking.TrackingLock) { Handles.Remove(handle); @@ -128,41 +79,14 @@ namespace Ryujinx.Memory.Tracking if (Handles.Count == 0) { _tracking.RemoveVirtual(this); - foreach (var child in _physicalChildren) - { - removedRegions |= child.RemoveParent(this); - } } } - - if (removedRegions) - { - // The first lock will unprotect any regions that have been removed. This second lock will remove them. - lock (_tracking.TrackingLock) - { - foreach (var child in _physicalChildren) - { - child.TryDelete(); - } - } - } - } - - /// - /// Add a child physical region to this virtual region. Assumes that the tracking lock has been obtained. - /// - /// Physical region to add as a child - public void AddChild(PhysicalRegion region) - { - _physicalChildren.Add(region); } public override INonOverlappingRange Split(ulong splitAddress) { - ClearPhysicalChildren(); VirtualRegion newRegion = new VirtualRegion(_tracking, splitAddress, EndAddress - splitAddress); Size = splitAddress - Address; - UpdatePhysicalChildren(); // The new region inherits all of our parents. newRegion.Handles = new List(Handles);