-
Notifications
You must be signed in to change notification settings - Fork 126
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Using userfaultfd in page guard manager #1161
Conversation
CI gfxreconstruct build queued with queue ID 22585. |
CI gfxreconstruct build # 2879 running. |
3b1e037
to
95afb44
Compare
CI gfxreconstruct build queued with queue ID 22599. |
95afb44
to
c5e1837
Compare
CI gfxreconstruct build queued with queue ID 22605. |
CI gfxreconstruct build # 2880 running. |
c5e1837
to
3d627b4
Compare
CI gfxreconstruct build queued with queue ID 22625. |
CI gfxreconstruct build # 2881 running. |
Exciting! Any data on the performance of this solution compared with mprotect? Why is it not enabled for Android? |
3d627b4
to
5af3a67
Compare
CI gfxreconstruct build queued with queue ID 22646. |
CI gfxreconstruct build # 2882 running. |
@per-mathisen-arm I am suspecting that this simply can't work on Android because ART is already using All I am seeing is, when run in an Android app, the faulting thread never get's halted and the fd GFXR creates never gets any notifications. I kinda verified this theory by creating multiple fds on a test app on Linux. Only the first one gets the notifications, while the rest |
CI gfxreconstruct build # 2882 failed. |
5af3a67
to
718709d
Compare
CI gfxreconstruct build queued with queue ID 8. |
CI gfxreconstruct build # 2884 running. |
CI gfxreconstruct-extended build queued with queue ID 29. |
CI gfxreconstruct-extended build # 211 running. |
CI gfxreconstruct build # 2884 failed. |
CI gfxreconstruct-extended build # 211 failed. |
718709d
to
1b962c1
Compare
CI gfxreconstruct build queued with queue ID 777. |
CI gfxreconstruct build # 2887 running. |
CI gfxreconstruct build # 2887 failed. |
UffdUnregisterMemory(memory_info->shadow_memory, memory_info->shadow_range); | ||
|
||
void* shadow_memory = memory_info->shadow_memory; | ||
if (munmap(memory_info->shadow_memory, memory_info->shadow_range)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you are supposed to unmap the memory first here. This introduces a race condition between the unmap and the new mmap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't aware of that and I am dealing with random segfaults that may be explained by this?
Any ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can just remove the unmap call. Redoing the mmap call with MAP_FIXED should work and throw out the previously mapped in pages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems you're right, the random segfaults are gone!
However I am seeing random pages that although they trigger a pagefault event, when populating them ioctl/uffdio_copy
triggers a File exists
error. Indeed checking beforehand the pages with mincore
these pages are already allocated.
I am not sure if these are responsible for the random visual artifacts (randomly corrupted textures and meshes) I am observing.
Also noting that I don't see neither the already allocated pages not the visual artifacts when trying this on a desktop game (recapturing a trace of a commercial title)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also on desktop the munmap
+ mmap
combination also seems to work.
Perhaps there's a difference in the strategy the two OSes allocate pages (or I'm just comparing apples to oranges)
{ | ||
if (msg[i].event == UFFD_EVENT_PAGEFAULT) | ||
{ | ||
UffdHandleFault(uffd_fd_, msg[i].arg.pagefault.address, msg[i].arg.pagefault.flags); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if you handle multiple events here, you need to use the DONTWAKE flags in the mode field for all but the latest event, otherwise you will get more race conditions, as the calling code wakes up and can do reads/writes that we presumably cannot catch while still processing more events.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good catch.
1b962c1
to
273a916
Compare
CI gfxreconstruct build # 3862 passed. |
CI gfxreconstruct-extended build queued with queue ID 150384. |
CI gfxreconstruct-extended build # 330 running. |
CI gfxreconstruct-extended build queued with queue ID 150989. |
CI gfxreconstruct-extended build # 331 running. |
CI gfxreconstruct-extended build # 331 failed. |
573aeee
to
c45178c
Compare
CI gfxreconstruct build # 3859 failed. |
c45178c
to
950f468
Compare
CI gfxreconstruct build queued with queue ID 154672. |
CI gfxreconstruct build # 3897 running. |
CI gfxreconstruct build # 3897 passed. |
CI gfxreconstruct-extended build queued with queue ID 155485. |
CI gfxreconstruct-extended build # 336 running. |
CI gfxreconstruct-extended build # 336 passed. |
Introducing an alternative memory tracking mode which utilizes the userfaultfd mechanism provided by the Linux kernel. This new mode should provide an alternative for Linux and Android applications that are not possible to be captured with the page_guard mode because they interfere with the SIGSEGV handler. The new mode is enabled by seeting GFXRECON_MEMORY_TRACKING_MODE/ebug.gfxrecon.memory_tracking_mode to uffd.
Converted the constructor with the default values into a delegating constructor
Also use uint64_t for thread ids instead of pid_t
950f468
to
d0d4af5
Compare
CI gfxreconstruct build queued with queue ID 155807. |
CI gfxreconstruct build # 3908 running. |
CI gfxreconstruct build # 3908 passed. |
CI gfxreconstruct-extended build queued with queue ID 155851. |
CI gfxreconstruct-extended build # 337 running. |
CI gfxreconstruct-extended build # 337 passed. |
No description provided.