Conversation
| #if defined(WIN32) | ||
| for (auto& entry : memory_info_) | ||
| { | ||
| auto& info = entry.second; | ||
| ResetWriteWatch(info.aligned_address, info.mapped_range + info.aligned_offset); | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Could this introduce a race condition in processing modified pages? For example:
PageGuardManager::ProcessMemoryEntriesis called. Memory A is found to be modified and is processed.ProcessMemoryEntriescontinues to processing the remaining modified pages.- After memory A is processed, but before
ProcessMemoryEntriescallsResetAllWriteWatch();(it could be processing other modified memory pages), the application completes another write to memory A. ResetAllWriteWatch();is called, clearing the new write watch flag on memory A, causing the application's latest write to be skipped at the next call toProcessMemoryEntries.
3129b5d to
fd0628c
Compare
|
@davidd-lunarg Upload a new solution. It updates the memory info on the same page in |
MarkY-LunarG
left a comment
There was a problem hiding this comment.
Good catch. I think this is a bug that has been bugging us for a while. So good thing you ran into it in this scenario.
Thanks! The changes look good to me. Since this touches code paths in the
|
Yes, I believe so. I think @ziga-lunarg was the one who ran into issues and suggested forcing it. |
Looking into it more, I think I don't think this PR should make any changes to |
When it uses DX12 tight alignment, the alignment could be 8B~256B. It's much smaller than a page of memory, like 4KB. One page could have two resources, so if it reset the WriteWatch, the another resource on the same page will miss its update.
This reverts commit 7df7643.
d91c8b3 to
664bb2e
Compare
|
@davidlunarg Fixed the bug that you messaged to me. #2853 We can revisit this issue later. |
* Reset WriteWatch later When it uses DX12 tight alignment, the alignment could be 8B~256B. It's much smaller than a page of memory, like 4KB. One page could have two resources, so if it reset the WriteWatch, the another resource on the same page will miss its update. * Revert "Reset WriteWatch later" This reverts commit 7df7643. * Update all memory info on the page * Bugfix 1. rename page_to_memory_infos_ to page_to_memory_infos_for_write_watch_. 2, Check use_write_watch before using. 3. Only add once by cheking entry.second 4. Add assert to check bounds Change-Id: Ib2f0f08836daea38e1b8cf34836e52fc2c5e2ee8

closed: https://github.com/LunarG/Projects/issues/1125
Ref: https://microsoft.github.io/DirectX-Specs/d3d/D3D12TightPlacedResourceAlignment.html
Regularly, the alignment of the resource should be
D3D12_DEFAULT_RESOURCE_PLACEMENT_ALIGNMENT(64KB). But when it uses tight alignment, it becomes 8B ~ 256B. It's smaller than a page size(system_page_size_: 4KB). In this case, one page could have two resources. ForWriteWatchcase, inLoadActiveWriteStates, if it resetsWriteWatchonGetWriteWatch, another resource on the same page will miss its update. AlthoughGetWriteWatchcould set precise address, OS operates on page by page. It shouldn't have resetWriteWatchuntil all memory infos get updated.There are the other two solutions that could fix it.
Force
ID3D12Device_Wrapper::GetResourceAllocationInfoto return alignment to multiple of a page size, so a page won't have two resources. But we shouldn't change the original value.GFXRECON_PAGE_GUARD_EXTERNAL_MEMORYset false could disableWriteWatch. It will use shadow memory, instead ofWriteWatch. But shadow memory consumes huge memory. AndPageGuardManager::HandleGuardPageViolationinterrupts the process to update memory info. It could impact the performance.