Changing priority on Linux processes

NEW
Unassigned

Status

()

P3
normal
a year ago
3 months ago

People

(Reporter: baku, Unassigned)

Tracking

(Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

a year ago
Bug 1366356 implements the support for changing the process priority but it doesn't include any platform-specific implementation. Here, the linux one.
(Reporter)

Comment 1

a year ago
Created attachment 8877101 [details] [diff] [review]
priority_linux.patch

By default, this doesn't work. We need to see if we can set the CAP_SYS_NICE capability. Note that this is needed also if we try to increase the priority to the current process, or set it back to 0.
Attachment #8877101 - Flags: review?(mh+mozilla)
(Reporter)

Comment 2

a year ago
GCgroup can be another approach but it requires FF to run as run in order to create the correct cgroup.
It looks like you'll need actual root privileges to raise priority — having CAP_SYS_NICE within an unprivileged user namespace doesn't seem to do anything (which is good, because that would be a kernel security bug if it did).  File capabilities can be used instead of a setuid root executable to gain only CAP_SYS_NICE, but they can't be set except by root (and are generally treated like setuid root for security purposes — e.g., the dynamic linker will ignore env vars), so it has the same set of problems.

Alternately: use CAP_SYS_RESOURCE to set the parent process's RLIMIT_NICE limits to 20 during startup; this will let it and its children be raised back to priority 0, and doesn't require keeping around a semi-privileged process to do the setpriority() calls.

But also, because you probably don't want the main firefox executable to have file capabilities (see above about the dynamic linker), you'd want to do this as a helper executable... which could potentially be applied to anything, not just Firefox.  Which means that installing Firefox like this will let anything on the system un-nice itself; people who care about security probably won't like that.  You could try to do something like checking /proc/%d/exe to see if it's the right executable, but then there's a time-of-check-to-time-of-use problem if the helper can be suspended while the adversary creates and destroys processes until the pid is reused.

In any case, if you do this, it won't be usable for anyone who downloads and runs Firefox as a normal user (which we support), only system-installed packages.  We don't have telemetry on how much of the userbase this might be, as far as I know.  And we'd probably need to reach out to downstream distributors to make sure they preserve the privileged metadata — and some might actively choose not to do that, for security reasons.  (Not that we have a communication channel that will reach our downstreams in the first place; you'd have to go down a list of popular distributions, which we also don't have, and track down points of contact one by one.)


With all that in mind: Do we have any data on how much this would help performance?
For reference, Chromium uses setpriority if it was started with enough RLIMIT_NICE to raise processes back to foreground priority (nice 0), but doesn't do anything to try to gain that ability if it doesn't have it: https://cs.chromium.org/chromium/src/base/process/process_linux.cc?l=84&rcl=ec7081e05094e3a9ee1467c21cf66f8c4d23637f
Comment on attachment 8877101 [details] [diff] [review]
priority_linux.patch

Review of attachment 8877101 [details] [diff] [review]:
-----------------------------------------------------------------

Good summary from jld.
Attachment #8877101 - Flags: review?(mh+mozilla)
As I already mentioned in bug 1366356 comment 9, calling setpriority() on a PID in Linux is not going to provide the desired effect. The only thing it will do is change the nice value of the kernel thread with a TID equal to the process PID, i.e. it will only adjust the nice value of a process' main thread, leaving the nice values of all the other threads unchanged. We've already run into this in b2g (see bug 861441 for details, or jlebar's post [1] on the matter).

Note that in that bug we implemented hal::SetProcessPriority() by calling setpriority() over all of a process' threads but it was a horrible contraption and it was racy by design.

[1] http://jlebar.com/2013/4/11/Beware_of_renice.html
(In reply to Gabriele Svelto [:gsvelto] from comment #6)
> As I already mentioned in bug 1366356 comment 9, calling setpriority() on a
> PID in Linux is not going to provide the desired effect. The only thing it
> will do is change the nice value of the kernel thread with a TID equal to
> the process PID, i.e. it will only adjust the nice value of a process' main
> thread, leaving the nice values of all the other threads unchanged. We've
> already run into this in b2g (see bug 861441 for details, or jlebar's post
> [1] on the matter).

The main thread is the most important thread by far, so having it only work on the main thread isn't really so bad.
(In reply to Bill McCloskey (:billm) from comment #7)
> The main thread is the most important thread by far, so having it only work
> on the main thread isn't really so bad.

The problem is that a non-root program cannot raise the priority of a thread above the value it started with, it can only lower it. Since we're already adjusting the priority of other threads [1] if you lower the main thread priority you can end up in a scenario where your main thread has lower priority than the DOM workers within the same process (which is the issue we run into back in the day, DOM workers in "background" processes were starving the main thread). Is that the behavior we're looking for here?

[1] https://dxr.mozilla.org/mozilla-central/search?q=PR_SetThreadPriority
(In reply to Gabriele Svelto [:gsvelto] from comment #6)
> Note that in that bug we implemented hal::SetProcessPriority() by calling
> setpriority() over all of a process' threads but it was a horrible
> contraption and it was racy by design.

I managed to do something similar but non-racy for bug 970676 (although not without some mistakes; see bug 1038900, bug 1046210, bug 1176085, bug 1257361, maybe more I'm forgetting), by signaling each thread to make the change to itself if needed, and repeating the loop until all threads reported no need to do anything.  This used tgkill(), so it can't accidentally affect another process's thread if it races with a thread exit and tid reuse, and the thread is altering itself so there's no chance for it to exit (or whatever) between checking if it's already been visited and doing the thing.
(In reply to Jed Davis [:jld] (⏰UTC-6) from comment #9)
> I managed to do something similar but non-racy for bug 970676 (although not
> without some mistakes; [...]

Nice! It looks like that could also be used for changing all threads' priority. Having the thread re-nice itself would also solve the second type of race we had, which was the process priority manager changing the nice value of a thread right after thread creation but just before it called PR_SetThreadPriority() on itself.

BTW, calling setpriority() on most BSDs re-prioritizes the whole process, so there are platforms that implement it that way, just not Linux.
(In reply to Gabriele Svelto [:gsvelto] from comment #8)
> (In reply to Bill McCloskey (:billm) from comment #7)
> > The main thread is the most important thread by far, so having it only work
> > on the main thread isn't really so bad.
> 
> The problem is that a non-root program cannot raise the priority of a thread
> above the value it started with, it can only lower it.

s/it started with/it has/

If you start with 0, and set it to 10, you can't get it back to 0. That's the whole problem and why that can't work without a setuid or file-capable program.

Comment 12

a year ago
Andrea, are you still working on this?  Also is there a bug for doing this on Windows?  Are you planning to work on that as well?

Updated

a year ago
Flags: needinfo?(amarchesini)
(Reporter)

Comment 13

a year ago
No, I'm not working on this, and yes, we should file a bug for windows and mac.
Assignee: amarchesini → nobody
Flags: needinfo?(amarchesini)
(Reporter)

Updated

a year ago
Blocks: 1394710
(Reporter)

Updated

a year ago
Blocks: 1394714

Updated

a year ago
Priority: -- → P3

Updated

7 months ago
Blocks: 1366358
You need to log in before you can comment on or make changes to this bug.