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: fix thread priority #14252

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

Linux: fix thread priority #14252

wants to merge 14 commits into from

Conversation

hoholee12
Copy link

By default, unless specified, all userland processes run with the SCHED_OTHER scheduling policy, which only supports one priority value: 0. The original code currently does not work in setting priority for each thread AT ALL. However, we can implement an alternative method to change thread priorities using the niceness value, which can be used with the SCHED_OTHER policy.

Niceness value ranges from -20 (highest priority) to 19 (lowest priority). However, there are a few caveats to consider:

  1. Non-root users cannot set the niceness value to a lower value than 0, meaning they cannot assign a higher priority than the default.
  2. Non-root users are restricted from setting a lower value than the current niceness value. For example, if the priority is already set to 19 (lowest), it cannot be set back to 0. This limitation renders scoped priority adjustments ineffective.

Utilities/Thread.cpp Outdated Show resolved Hide resolved
@hoholee12
Copy link
Author

in order to compensate for not being able to use high priority in linux, i modified it to use high_prio: 0, stock_prio: 9, low_prio: 19

@hoholee12 hoholee12 marked this pull request as draft July 23, 2023 15:58
@@ -3028,6 +3028,24 @@ void thread_ctrl::set_native_priority(int priority)
{
sig_log.error("SetThreadPriority() failed: %s", fmt::win_error{GetLastError(), nullptr});
}
#elif defined(__linux__)
// available niceness for nonroot: 0~19
int linuxprio = 9;
Copy link
Contributor

Choose a reason for hiding this comment

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

this means that threads with previous normal priority now have lower priority than normal...
I don't see the point in these changes.
It seems like your only issue is with non-root. Then why add all this weird low priority logic?

Copy link
Author

Choose a reason for hiding this comment

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

It's important to note that the impact of niceness value is limited to threads within the same process group due to Linux autogrouping. When scheduled with other processes, the niceness value behaves the same as 0

Copy link
Contributor

Choose a reason for hiding this comment

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

How does that relate to my comment?

Copy link
Author

@hoholee12 hoholee12 Jul 23, 2023

Choose a reason for hiding this comment

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

Im trying to imply that although threads with normal priority now has lower priority, that doesnt mean other processes with normal priority get to interfere with the game thread.

But other high priority threads(in this case rsx thread with niceness of 0) inside the rpcs3 group gets to interfere, which is the desired behavior.

Copy link
Member

Choose a reason for hiding this comment

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

I think this approach is fine

rpcs3/Emu/CPU/CPUThread.cpp Outdated Show resolved Hide resolved
@hoholee12 hoholee12 marked this pull request as ready for review July 24, 2023 07:52
@hoholee12 hoholee12 marked this pull request as draft July 24, 2023 10:41
@hoholee12 hoholee12 marked this pull request as ready for review July 25, 2023 06:57
}

// nonroot cannot increase niceness value
if ((getpriority(PRIO_PROCESS, threadpid) < linuxprio) || (euid == 0))
Copy link
Contributor

Choose a reason for hiding this comment

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

So, if a thread sets lower priority, it cannot increase it afterwards if it is non-root?

Copy link
Author

Choose a reason for hiding this comment

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

unfortunately, yep

Copy link
Contributor

@elad335 elad335 Jul 25, 2023

Choose a reason for hiding this comment

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

This is a problem because, named_thread is recycled from a pool after use. So it would get stuck with lowered priority.

Copy link
Author

Choose a reason for hiding this comment

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

yes. i noticed this by testing alot and game threads are stuck with low priority. im thinking maybe we only should allow priority changes for root user and disregard nonroots...

Copy link
Contributor

Choose a reason for hiding this comment

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

I think so too.

Copy link
Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this handle the case where a user might have the appropriate rights (limits.conf or CAP_SYS_NICE)?

Utilities/Thread.cpp Outdated Show resolved Hide resolved
@Nekotekina
Copy link
Member

Welp. Maybe we should specify priority at named_thread startup and make separate thread pools for each priority.

@MastaG
Copy link

MastaG commented Aug 3, 2023

Is this ready to be tested? or does it need a rebase?

@elad335
Copy link
Contributor

elad335 commented Aug 12, 2023

Threadpool has been removed, this needs updating.

@AniLeo AniLeo marked this pull request as draft August 28, 2023 22:49
@AniLeo
Copy link
Member

AniLeo commented Aug 28, 2023

@hoholee12 needs updating due to the removal of thread pool, marked as draft in the meantime

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

Successfully merging this pull request may close these issues.

9 participants