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

RFC: Never touch wineserver or the Steam client (do not merge, revision planned) #76

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

kakra
Copy link
Contributor

@kakra kakra commented Sep 24, 2018

With the arrival of SteamPlay Proton, we probably don't want to touch
wineserver at all. With staging patches, wineserver has it's own means
of gaining realtime priority and handling spawned processes.

Optimally, gamemode support would be integrated right into Proton itself
by changing the process spawning functions of wine when launching the
actual game exe. I'm currently working on such an idea.

This could be extended to ignore other well-known binaries which should
never be handled by gamemode (i.e. shells of scripted game launchers).

The following commits add support for properly handling handling Wine
games and the whole Steam client, including examples of how to do it.

Signed-off-by: Kai Krakow [email protected]

@kakra kakra changed the title gamemode: Never touch wineserver gamemode: Never touch wineserver or the Steam client Sep 24, 2018
@aejsmith
Copy link
Contributor

I wonder whether this would be better handled via a default blacklist option (when not explicitly set in a config file, and also update the example config to include them with an explanation), rather than hardcoding it, so that the behaviour can be disabled.

I can't think of any reasonable use case for entering game mode when wineserver is running, but it's possible that some users may want it when they have Steam running (the built-in functionality doesn't really make sense, but they may wish to do something with a custom script).

@kakra
Copy link
Contributor Author

kakra commented Sep 29, 2018

Does the default blacklist handle partial paths correctly?

@kakra
Copy link
Contributor Author

kakra commented Sep 29, 2018

Look here:

if (strstr(client, self->blacklist[i])) {

It does a simple test to check for the occurrence of a match in the exe path. But for correctly handling Wine and Steam processes, we need to match some of the entries with the end of the string, especially steam is not very unique. I don't want to match processes having "steam" in them because game exes may use that (e.g. some games ship exe names with "steam" in them).

EDIT: Updated to correct line number.

@kakra
Copy link
Contributor Author

kakra commented Sep 29, 2018

But I'm open to rebase and improve the blacklist handler instead...

@kakra
Copy link
Contributor Author

kakra commented Sep 29, 2018

My other pull request provides the infrastructure to do selective filtering of wine processes. That could act as a base for this idea.

@kakra kakra changed the title gamemode: Never touch wineserver or the Steam client RFC: Never touch wineserver or the Steam client (do not merge, revision planned) Oct 2, 2018
@aejsmith
Copy link
Contributor

aejsmith commented Oct 2, 2018

But I'm open to rebase and improve the blacklist handler instead...

Yes, I think that'd be best.

@kakra
Copy link
Contributor Author

kakra commented Oct 3, 2018

In spe of revisiting this: What's your suggestion on blacklisting wine processes by default?

With my other PR we'd be able to use blacklists like "windows/system32", "windows/syswow64", and "/wineserver".

It could be done as an example only (and many people would miss this optimization), or as a default blacklist hard-coded into the daemon binary unless overwritten by a blacklist config (which violates the principle of least surprise).

Because of this, the best option would probably be an on/off switch like "protonsupport=yes/no" or "winesupport=yes/no", defaulting to yes. And that adds the relevant blacklist entries in.

@aejsmith
Copy link
Contributor

aejsmith commented Oct 3, 2018

I like the idea of a winesupport option, which adds the blacklisting on top of any blacklist specified by the user.

Doesn't handle the Steam case, but I think if users want to preload it into the Steam client rather than per-game, we could just document that they might want to manually blacklist it themselves depending on whether or not they actually want game mode to be active while Steam is running or just when a game is.

@kakra
Copy link
Contributor Author

kakra commented Oct 3, 2018

So why not put this a level further and also add steamsupport=yes which does the same for steam? And then maybe protonsupport=yes/steamplaysupport=yes which combines both options.

Since we are both in the same position now how to handle this, I would rebase this work onto my wine support branch and push it here once the other branch is merged.

@kakra
Copy link
Contributor Author

kakra commented Oct 12, 2018

@aejsmith So I'm back with a fresh set of ideas. My currently idea (and I started working on an implementation) is to use so-called "filter sets". A filter set is a section like [filter] but stand-alone. The filter section would then allow to include filter sets (and that works recursively, as sets could also include other sets). So resolve the recursion, a two-pass resolver is used so we don't get into fancy endless recursion scenarios.

Here's an example of my idea:

; Defines default blacklist sets for easy blacklisting of common software
; which should not be managed by GameMode. The [set:default] section is evaluated
; if your custom gamemode.ini does not configure "filter-sets". Filter sets can
; be nested by repeating "filter-sets" within the section.
; Each set section evaluated doesn't overwrite a blacklist but adds the
; entries to the blacklist. Whitelists can also be used.

[set:default]
filter-sets = launchers wine

; Whitelist sets can also be used. If you define a whitelist everything else
; is blacklisted. This section is not used by default. This example would
; whitelist any game in your default Steam library.

[set:steamlib]
whitelist = /steam/SteamApps/common/

; Common shell wrappers that would mix up the desired results, we do
; not want these to leak scheduler settings into subprocesses.

[set:shellwrappers]
blacklist =
  /bin/ionice
  /bin/schedtool
  /bin/env
  /bin/python

; Game launchers should not be managed by GameMode even if you started
; the complete launcher under GameMode. Expectations are rather that
; each game would run under GameMode individually then. This blacklist
; set enables this expected behavior. GameMode can detect if the launcher
; leaks unwanted scheduler settings into its subprocesses and would
; complain in the log about this.

[set:launchers]
filter-sets = steam

[set:steam]
filter-sets = shellwrappers ; used by SteamPlay (Proton launcher)
blacklist =
  /steamerrorreporter
  /steamwebhelper
  /ubuntu12_32/steam

; Wine starts several subprocesses and manages these by itself.
; We should not interfere with that as it may result in priority
; inversion between the game and the Windows API. GameMode is still
; able to correctly evaluate the exe paths of programs running under
; Wine and ignores the Wine loaders until the .exe file is loaded.

[set:wine]
blacklist =
  /bin/ntlm_auth
  /windows/system32/
  /windows/syswow64/
  /wineserver

@aejsmith
Copy link
Contributor

Sounds like a good plan. Keeps things configurable without hardcoding a bunch of process names/paths, and makes it easier to enable/disable behaviour for Steam etc.

@nanonyme
Copy link

nanonyme commented Aug 1, 2019

This looks quite stale. Is anyone planning to follow-up?

@kakra
Copy link
Contributor Author

kakra commented Aug 1, 2019

Yes, I will follow-up but my whole development env is currently not usable. This is fixed in a few weeks.

@kakra
Copy link
Contributor Author

kakra commented Oct 14, 2019

Just a resync of my branch with current master because so much has changed... None of the commits changed functionality over my old version. This is purely for people using this as a patch: They now can for a current version of GameMode. :-)

@aejsmith However, you may want to cherry-pick a5390d8.

@terencode
Copy link
Contributor

Hey, it looks like the PR needs a new rebase.

@kakra
Copy link
Contributor Author

kakra commented Jan 30, 2020

I'll look into it, give me a few days...

@terencode
Copy link
Contributor

Just a friendly ping in case you forgot about it :)

@kakra
Copy link
Contributor Author

kakra commented Apr 14, 2020

Just a friendly ping in case you forgot about it :)

Thanks... Yeah, I actually forgot about it. ;-)

@kakra kakra marked this pull request as draft April 16, 2020 18:31
@terencode
Copy link
Contributor

I just tested your rebased PR, is this the correct behaviour ?
log.txt

I see all the wine processes with the same priority.

@kakra
Copy link
Contributor Author

kakra commented Apr 17, 2020

This is expected as designed at this point. The PR is mainly about detecting the executable running in the wine container, so you could actually blacklist based on the exe name. It also ignores wineserver via hardcoded exception because wineserver should not be touched: It is sensible to priority changes and may bring it's own handler for adjusting its main thread (staging patches needed). Fiddling with wineserver can result in priority inversions.

So if you didn't expect all the internal wine processes being changed to another priority, you may want to blacklist windows/system32 (which you can do now thanks to this PR).

@terencode
Copy link
Contributor

Ah ok thanks.
So if wineserver isn't touched why does it have the renice value I set?
image

@kakra
Copy link
Contributor Author

kakra commented Apr 17, 2020

So if wineserver isn't touched why does it have the renice value I set?

Probably because you used a shell wrapper to start wine and applied LD_PRELOAD to it. The result is: The wrapper becomes reniced and its children will inherit it. You should follow advice of the log and blacklist /usr/bin/wine64.

@terencode
Copy link
Contributor

terencode commented Apr 17, 2020

I used gamemoderun wine64 program is that the same?

Blacklisting worked.
Do you plan to include more things into this PR with the "draft" mark?

EDIT: replacing wine64 by wine worked as well. Could there be something gamemode do when using wine64 instead of wine?

@kakra
Copy link
Contributor Author

kakra commented Apr 17, 2020

There's an improved black/whitelist extension planned which also supports sets which can be easily distributed with gamemode. But I'm currently lacking a lot of time.

@terencode
Copy link
Contributor

Nice and ok, this is understandable.

@kakra
Copy link
Contributor Author

kakra commented Apr 17, 2020

Nice and ok, this is understandable.

The basic idea is outlined here: #76 (comment)

Copy link
Contributor Author

@kakra kakra left a comment

Choose a reason for hiding this comment

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

Rebase to 1.6

Actually, it wasn't planned to only use this code by the wine
detection, there's more code in my queue that uses this.

This reverts commit 42a803c.
This enables us to reuse the open function for different purposes.

Signed-off-by: Kai Krakow <[email protected]>
With the arrival of SteamPlay Proton, we probably don't want to touch
wineserver at all. With staging patches, wineserver has it's own means
of gaining realtime priority and handling spawned processes.

Optimally, gamemode support would be integrated right into Proton itself
by changing the process spawning functions of wine when launching the
actual game exe. I'm currently working on such an idea.

This could be extended to ignore other well-known binaries which should
never be handled by gamemode (i.e. shells of scripted game launchers).

Signed-off-by: Kai Krakow <[email protected]>
If you want to run your whole Steam client under gamemode to not adjust
each game individually, we certainly should leave the Steam client
processes alone as we don't want them to take resources away from the
game. Otherwise, Steam client processes may run with `SCHED_ISO`.

Closes: FeralInteractive#67
Signed-off-by: Kai Krakow <[email protected]>
Let's document the changes to automatically handle Steam, SteamPlay, and
Wine. Also, give an example how to run the Steam client or Wine games in
GameMode.

Signed-off-by: Kai Krakow <[email protected]>
Copy link
Contributor Author

@kakra kakra left a comment

Choose a reason for hiding this comment

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

Keep subprojects accidentally removed in revert commit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants