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

Linux: Automatically increase vm.max_map_count if it's too low #4702

Merged
merged 8 commits into from May 29, 2023

Conversation

TSRBerry
Copy link
Member

@TSRBerry TSRBerry commented Apr 20, 2023

This PR adds a workaround for #3372 and result checking for memory related pinvoke calls.

If a game adds too many memory map areas Ryujinx will crash with a segmentation fault, which makes these issues difficult to debug.
But since @gdkchan was able to find the cause of this type of crash, we are able to temporarily increase vm.max_map_count while Ryujinx is running and reset it to the original value once Ryujinx was closed.

The threshold value in the script is just the default value that the kernel documentation for vm.max_map_count suggested. This check is just supposed to make sure we aren't messing with the settings if the user has modified them already.


Closes #4941

@TSRBerry TSRBerry added miscellaneous Related to some project not listed in labels os:linux Issues affecting Linux exclusively labels Apr 20, 2023
@TSRBerry TSRBerry requested a review from a team April 20, 2023 22:07
@TSRBerry TSRBerry marked this pull request as ready for review April 20, 2023 22:08
Copy link
Member

@riperiperi riperiperi left a comment

Choose a reason for hiding this comment

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

Looks sensible, though I'm not an expert on whether 262144 is high enough for all games. It would be nice if this stuff could be set when opening the main Ryujinx executable, too bad it can't.


madvise(address, size, MADV_REMOVE);
if (madvise(address, size, MADV_REMOVE) != 0)
Copy link
Member

Choose a reason for hiding this comment

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

I hope nothing else races to change permissions, otherwise these could crash the emulator where otherwise they did not. I suppose Decommit isn't used right now, so it's more a worry for another time...

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we just return false instead of throwing everywhere here?

that kind of change the behavior of the function entirely... and the return value doesn't meaning anything anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

Afaik Ryujinx would crash either way even if we return false here.

@TSRBerry
Copy link
Member Author

I mean it could be, but we might not be able to revert to defaults if Ryujinx crashes.

@TSRBerry TSRBerry requested a review from a team April 22, 2023 19:15
distribution/linux/Ryujinx.sh Outdated Show resolved Hide resolved
Ryujinx.Memory/WindowsShared/WindowsApi.cs Outdated Show resolved Hide resolved
@TSRBerry TSRBerry marked this pull request as draft April 25, 2023 10:55
@TSRBerry TSRBerry marked this pull request as ready for review April 25, 2023 17:55
@TSRBerry
Copy link
Member Author

Requesting both reviews, since I essentially changed the whole thing again.

@TSRBerry TSRBerry added the needs-feedback The needs feedbacks from the community label Apr 25, 2023
Comment on lines 11 to 12
// NOTE: This value was determined by manual tests and might need to be increased again.
public const int RecommendedVmMaxMapCount = 524288;
Copy link
Member Author

@TSRBerry TSRBerry Apr 25, 2023

Choose a reason for hiding this comment

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

I'm pretty sure we can go lower here, but I'll need community testing to figure this out.
I don't have any of the games in the referenced issue and can't trigger it on my side.

Copy link
Member

@riperiperi riperiperi left a comment

Choose a reason for hiding this comment

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

How will this behave when Ryujinx is installed as a flatpak? I doubt that it'll be able to write sysctl.d config, not sure about the vm map count.

@riperiperi riperiperi self-requested a review May 6, 2023 00:12
@TSRBerry
Copy link
Member Author

TSRBerry commented May 6, 2023

The flatpak probably wouldn't be able to do any of these things. 🤔

I'll need to think of something for that. :/

@TSRBerry TSRBerry marked this pull request as draft May 6, 2023 00:44
@TSRBerry
Copy link
Member Author

TSRBerry commented May 6, 2023

@riperiperi Currently flatpak users would see this warning (even if pkexec is installed, because it doesn't have access to it).
Would this be okay?

image

Ideally we should use dbus to talk to polkitd, but I have no idea how to do that yet and it doesn't seem to be easy.

@TSRBerry TSRBerry marked this pull request as ready for review May 6, 2023 23:02
@TSRBerry TSRBerry removed the needs-feedback The needs feedbacks from the community label May 28, 2023
@TSRBerry TSRBerry merged commit 35d91a0 into Ryujinx:master May 29, 2023
8 checks passed
@TSRBerry TSRBerry deleted the max_map-workaround branch May 29, 2023 23:48
noxifoxi added a commit to noxifoxi/Ryujinx that referenced this pull request May 30, 2023
drizzt added a commit to drizzt/batocera-switch that referenced this pull request May 31, 2023
Since this Ryujunx PR [1] ot returns a warning if vm.max_map_count < 524288,
so just set an higher value.

To be coherent with what other Linux distrubitions are doing, use
2147483642.

This is the value used by Batocera 37 itself [2], SteamDeck and Fedora 39 [3].

There is not need to re-set the value each time yuzu or Ryujinx is
started, so just set it once on startup.

[1] Ryujinx/Ryujinx#4702
[2] batocera-linux/batocera.linux#8649
[3] https://fedoraproject.org/wiki/Changes/IncreaseVmMaxMapCount
drizzt added a commit to drizzt/batocera-switch that referenced this pull request May 31, 2023
Since this Ryujunx PR [1] ot returns a warning if vm.max_map_count < 524288,
so just set an higher value.

To be coherent with what other Linux distrubitions are doing, use
2147483642.

This is the value used by Batocera 37 itself [2], SteamDeck and Fedora 39 [3].

There is not need to re-set the value each time yuzu or Ryujinx is
started, so just set it once on startup.

[1] Ryujinx/Ryujinx#4702
[2] batocera-linux/batocera.linux#8649
[3] https://fedoraproject.org/wiki/Changes/IncreaseVmMaxMapCount
drizzt added a commit to drizzt/batocera-switch that referenced this pull request May 31, 2023
Since this Ryujunx PR [1] it returns a warning if vm.max_map_count < 524288,
so just set an higher value.

To be coherent with what other Linux distrubitions are doing, use
2147483642.

This is the value used by Batocera 37 itself [2], SteamDeck and Fedora 39 [3].

There is not need to re-set the value each time yuzu or Ryujinx is
started, so just set it once on startup.

[1] Ryujinx/Ryujinx#4702
[2] batocera-linux/batocera.linux#8649
[3] https://fedoraproject.org/wiki/Changes/IncreaseVmMaxMapCount
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
miscellaneous Related to some project not listed in labels os:linux Issues affecting Linux exclusively
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ryujinx Crashes During TotK Gameplay
4 participants