1372543 - Changing priority on Linux processes

Status: NEW

(Core :: IPC, enhancement, P3)

Description

[Andrea Marchesini [:baku]]

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

Comment 1

[Andrea Marchesini [:baku]]

Attachment: 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.

Comment 2

[Andrea Marchesini [:baku]]

GCgroup can be another approach but it requires FF to run as run in order to create the correct cgroup.

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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?

Comment 4

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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 5

[Mike Hommey [:glandium]]

Comment on attachment 8877101 [details] [diff] [review]
priority_linux.patch

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

Good summary from jld.

Comment 6

[Gabriele Svelto [:gsvelto]]

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

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(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.

Comment 8

[Gabriele Svelto [:gsvelto]]

(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

Comment 9

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(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.

Comment 10

[Gabriele Svelto [:gsvelto]]

(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.

Comment 11

[Mike Hommey [:glandium]]

(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

[(no longer active)]

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?

Comment 13

[Andrea Marchesini [:baku]]

No, I'm not working on this, and yes, we should file a bug for windows and mac.