Skip to content
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

Jump to bottom

MemArena: Keep fastmem memory region mapped on Linux to ensure nothing allocates within the fastmem space. #9834

Merged
merged 1 commit into from Feb 28, 2022
Merged

MemArena: Keep fastmem memory region mapped on Linux to ensure nothing allocates within the fastmem space. #9834

merged 1 commit into from Feb 28, 2022

Conversation

AdmiralCurtiss
Copy link
Contributor

@AdmiralCurtiss AdmiralCurtiss commented Jun 21, 2021 •
edited

Same idea as #9544 but for Linux. As I understand correctly Linux allows this brutal clobbering of regions, but I honestly only have half an idea what I'm doing here. Please scrutinize.

This should fix the bug (on Linux) where large texture packs break emulation, such as 12404 or 11994, but I cannot 100% confirm it.

@AdmiralCurtiss AdmiralCurtiss marked this pull request as draft June 23, 2021 22:56
@AdmiralCurtiss AdmiralCurtiss marked this pull request as ready for review July 24, 2021 04:07
@AdmiralCurtiss AdmiralCurtiss changed the title MemArena: Keep fastmem region mapped on Linux. MemArena: Keep fastmem memory region mapped on Linux to ensure nothing allocates within the fastmem space. Jul 24, 2021
@AdmiralCurtiss AdmiralCurtiss force-pushed the fastmem-reserve-region-linux branch 2 times, most recently from 62782c6 to 22c3ed1 Compare July 24, 2021 04:22
@AdmiralCurtiss AdmiralCurtiss force-pushed the fastmem-reserve-region-linux branch 2 times, most recently from 5e52e6e to 3d21dc7 Compare December 14, 2021 23:48
@AdmiralCurtiss AdmiralCurtiss mentioned this pull request Dec 14, 2021
Merged
@AdmiralCurtiss AdmiralCurtiss force-pushed the fastmem-reserve-region-linux branch 2 times, most recently from a10ed2c to 1bc4a51 Compare December 18, 2021 23:53
@AdmiralCurtiss AdmiralCurtiss force-pushed the fastmem-reserve-region-linux branch 2 times, most recently from 2472fca to 26e4436 Compare December 30, 2021 00:07
v1993
v1993 reviewed Jan 12, 2022
View reviewed changes
Source/Core/Common/MemArena.h Outdated
#ifdef _WIN32
HANDLE hMemoryMapping;
#else
#ifdef ANDROID
int fd;
#else
int m_shm_fd;
void* m_reserved_region;
std::size_t m_reserved_region_size;
#endif
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#ifdef _WIN32
HANDLE hMemoryMapping;
#else
#ifdef ANDROID
int fd;
#else
int m_shm_fd;
void* m_reserved_region;
std::size_t m_reserved_region_size;
#endif
#endif
#if defined(_WIN32)
HANDLE hMemoryMapping;
#elif defined(ANDROID)
int fd;
#else
int m_shm_fd;
void* m_reserved_region;
std::size_t m_reserved_region_size;
#endif

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally - hMemoryMapping and fd lack m_ prefix, so they probably should be renamed for consistency (or avoid using prefix for new member variables too).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the variable prefixes, I've tried to avoid touching multiple platforms in a single PR, so they'll be done one at a time. The Windows equivalent PR is #9544. There is no Android one yet because I'm not familiar with that platform.

Regarding the defines, our codebase seems to use both variants. Not sure whether it's better to check for the symbol or for a nonzero definition.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does new Linux approach fail to work on Android?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Genuinely, no idea. I don't have an Android device capable of running Dolphin to test it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With android being Linux at its core I'd expect this approach to work, unless it is artificially restricted by permissions. Unfortunately, I don't have Dolphin-capable android device/VM/emulator either for quick testing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shm is not available on Android.

Warepire
Warepire reviewed Feb 9, 2022
View reviewed changes
Source/Core/Common/MemArenaUnix.cpp
@@ -28,30 +28,39 @@ MemArena::~MemArena() = default;
void MemArena::GrabSHMSegment(size_t size)
{
const std::string file_name = "/dolphin-emu." + std::to_string(getpid());
fd = shm_open(file_name.c_str(), O_RDWR | O_CREAT | O_EXCL, 0600);
if (fd == -1)
m_shm_fd = shm_open(file_name.c_str(), O_RDWR | O_CREAT | O_EXCL, 0600);
Copy link

@Warepire Warepire Feb 9, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If dolphin crashes, this file will stay until the system is rebooted. This is because it has the pid appended to it and Linux has shm persistence (normally /dev/shm is a tmpfs), so no other instance can come later and clean it up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not wrong, but it's unlinked literally the next line, so it would have to crash at a very specific point in time. Not sure if there's much we can do about this? This is also a pre-existing issue.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I completely missed that, then it's ok. Feel free to disregard this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also if you dig back in the history, the reason it has the pid attached is because at some point in time there were issues where multiple instances would grab the same if you had netplay with two instances on the same system. So I'd rather not change that.

Is there a way on Linux to say 'this file should not actually show up in the file system, I just need it for myself'?

Copy link

@Warepire Warepire Feb 9, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found https://man7.org/linux/man-pages/man2/memfd_create.2.html which might be what you could replace the shm_open / shm_unlink with. But that would be another PR as the shm_unlink works fine for now,

@AdmiralCurtiss
Copy link
Contributor Author

If no one has any objections I'd like to merge this sometime soon. The Windows variant has been merged for a few weeks now and apart from one issue of no longer launching on Win7 (which is fixed) I haven't heard any issues with it. I've also re-tested this today (on Xubuntu 20.04) and have confirmed that it does indeed fix freezing issues with large texture packs.

@JMC47 JMC47 merged commit ea116fb into dolphin-emu:master Feb 28, 2022
10 checks passed
@AdmiralCurtiss AdmiralCurtiss deleted the fastmem-reserve-region-linux branch February 28, 2022 17:46
@AdmiralCurtiss AdmiralCurtiss mentioned this pull request May 11, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JosJuice JosJuice JosJuice left review comments

@v1993 v1993 v1993 left review comments

@Warepire Warepire Warepire approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
5 participants
@AdmiralCurtiss @Warepire @JosJuice @v1993 @JMC47