base: Move renderer threads to the appropriate cpuset.

base: Allow renderer thread priorities to be changed.

This provides a mechanism for a renderer to change the priority of
its threads and as a result also the cpuset they are assigned to.

A ChildProcessHost IPC message from the renderer is used to request
a thread priority change and have the browser process identify the
renderer thread that is requesting a priority change by checking all
its threads to find a thread with NSpid field in
/proc/[pid]/task/[thread_id]/status that matches the namespace tid
from the renderer.

This is currently limited to Linux and ChromeOS but follow up work
will investigate the possibility and benefits of doing the same on
other platforms.

Note: Thread priorities in the renderer are already adjusted in a
similar way on Android as the sandbox is not strict enough to
prevent this on Android today.

BUG=chrome-os-partner:56550
TEST=

Committed: https://crrev.com/7b97c3240fb3da0826f244f9b592fc1761fb6554
Cr-Commit-Position: refs/heads/master@{#419649}

[Dmitry Torokhov, 2016-09-12 16:18:09]

dtor@chromium.org changed reviewers:
+ dtor@chromium.org

[Dmitry Torokhov, 2016-09-12 16:18:09]

https://codereview.chromium.org/2334533002/diff/1/base/threading/platform_thr...
File base/threading/platform_thread_linux.cc (right):

https://codereview.chromium.org/2334533002/diff/1/base/threading/platform_thr...
base/threading/platform_thread_linux.cc:73:
FILE_PATH_LITERAL("/sys/fs/cgroup/cpuset/chrome");
I wonder if, instead of having static cpuset placement, we should not be reading
"/proc/self/cpuset" on startup and then append urgent/non-urgent to the starting
cpuset.

We should need to make sure we actually start chrome in proper cpuset first
though.

[reveman, 2016-09-15 17:01:19]

Description was changed from

==========
base: Move renderer threads to the appropriate cpuset.

BUG=chrome-os-partner:56550
TEST=
==========

to

==========
base: Allow renderer thread priorities to be changed.

This provides a mechanism for a renderer to change the priority of
its threads and as a result also the cpuset they are assigned to.

A ChildProcessHost IPC message from the renderer is used to request
a thread priority change and have the browser process identify the
renderer thread that is requesting a priority change using a blocking
syscall in the renderer. For a given renderer process |pid|, browser
will look through all its threads and find the first thread with
/proc/[pid]/task/[thread_id]/syscall that matches the arguments
provided by the request from the renderer to change thread priority.

BUG=chrome-os-partner:56550
TEST=
==========

[reveman, 2016-09-15 17:01:33]

Description was changed from

==========
base: Allow renderer thread priorities to be changed.

This provides a mechanism for a renderer to change the priority of
its threads and as a result also the cpuset they are assigned to.

A ChildProcessHost IPC message from the renderer is used to request
a thread priority change and have the browser process identify the
renderer thread that is requesting a priority change using a blocking
syscall in the renderer. For a given renderer process |pid|, browser
will look through all its threads and find the first thread with
/proc/[pid]/task/[thread_id]/syscall that matches the arguments
provided by the request from the renderer to change thread priority.

BUG=chrome-os-partner:56550
TEST=
==========

to

==========
base: Allow renderer thread priorities to be changed.

This provides a mechanism for a renderer to change the priority of
its threads and as a result also the cpuset they are assigned to.

A ChildProcessHost IPC message from the renderer is used to request
a thread priority change and have the browser process identify the
renderer thread that is requesting a priority change using a blocking
syscall in the renderer. For a given renderer process |pid|, browser
will look through all its threads and find the first thread with
/proc/[pid]/task/[thread_id]/syscall that matches the arguments
provided by the request from the renderer to change thread priority.

BUG=chrome-os-partner:56550
TEST=
==========

[reveman, 2016-09-16 21:21:36]

Description was changed from

==========
base: Allow renderer thread priorities to be changed.

This provides a mechanism for a renderer to change the priority of
its threads and as a result also the cpuset they are assigned to.

A ChildProcessHost IPC message from the renderer is used to request
a thread priority change and have the browser process identify the
renderer thread that is requesting a priority change using a blocking
syscall in the renderer. For a given renderer process |pid|, browser
will look through all its threads and find the first thread with
/proc/[pid]/task/[thread_id]/syscall that matches the arguments
provided by the request from the renderer to change thread priority.

BUG=chrome-os-partner:56550
TEST=
==========

to

==========
base: Allow renderer thread priorities to be changed.

This provides a mechanism for a renderer to change the priority of
its threads and as a result also the cpuset they are assigned to.

A ChildProcessHost IPC message from the renderer is used to request
a thread priority change and have the browser process identify the
renderer thread that is requesting a priority change by checking all
its threads to find a thread with NSpid field in
/proc/[pid]/task/[thread_id]/status that matches the namespace tid
from the renderer.

BUG=chrome-os-partner:56550
TEST=
==========

[reveman, 2016-09-16 22:16:09]

reveman@chromium.org changed reviewers:
+ avi@chromium.org, jschuh@chromium.org, thakis@chromium.org

[reveman, 2016-09-16 22:16:11]

+avi for content/
+thakis for base/
+jschuh for *_messages.h and general security review

https://codereview.chromium.org/2334533002/diff/1/base/threading/platform_thr...
File base/threading/platform_thread_linux.cc (right):

https://codereview.chromium.org/2334533002/diff/1/base/threading/platform_thr...
base/threading/platform_thread_linux.cc:73:
FILE_PATH_LITERAL("/sys/fs/cgroup/cpuset/chrome");
On 2016/09/12 at 16:18:09, dtor wrote:
> I wonder if, instead of having static cpuset placement, we should not be
reading "/proc/self/cpuset" on startup and then append urgent/non-urgent to the
starting cpuset.
> 
> We should need to make sure we actually start chrome in proper cpuset first
though.

sgtm.

dtor, can you take care of starting chrome in the proper cpuset?

[Avi (use Gerrit), 2016-09-16 22:26:29]

lgtm

ok

https://codereview.chromium.org/2334533002/diff/80001/base/linux_util.cc
File base/linux_util.cc (right):

https://codereview.chromium.org/2334533002/diff/80001/base/linux_util.cc#newc...
base/linux_util.cc:168: i = tids.begin(); i != tids.end(); ++i) {
for (pid_t tid : tids)

https://codereview.chromium.org/2334533002/diff/80001/base/linux_util.cc#newc...
base/linux_util.cc:199: ++i) {
for (pid_t tid : tids)

https://codereview.chromium.org/2334533002/diff/80001/base/threading/platform...
File base/threading/platform_thread.h (right):

https://codereview.chromium.org/2334533002/diff/80001/base/threading/platform...
base/threading/platform_thread.h:213: #endif
You ifdef this to the platform, but don't ifdef any of the other code. Why?

[reveman, 2016-09-16 23:06:41]

thanks for the quick review!

https://codereview.chromium.org/2334533002/diff/80001/base/linux_util.cc
File base/linux_util.cc (right):

https://codereview.chromium.org/2334533002/diff/80001/base/linux_util.cc#newc...
base/linux_util.cc:168: i = tids.begin(); i != tids.end(); ++i) {
On 2016/09/16 at 22:26:29, Avi wrote:
> for (pid_t tid : tids)

Done

https://codereview.chromium.org/2334533002/diff/80001/base/linux_util.cc#newc...
base/linux_util.cc:199: ++i) {
On 2016/09/16 at 22:26:29, Avi wrote:
> for (pid_t tid : tids)

Done

https://codereview.chromium.org/2334533002/diff/80001/base/threading/platform...
File base/threading/platform_thread.h (right):

https://codereview.chromium.org/2334533002/diff/80001/base/threading/platform...
base/threading/platform_thread.h:213: #endif
On 2016/09/16 at 22:26:29, Avi wrote:
> You ifdef this to the platform, but don't ifdef any of the other code. Why?

What other code are you referring to? I didn't ifdef the IPC messages to linux
but can do that if preferred? The rest of the code should already be limited to
linux either through ifdefs or by being in a file that's only built on linux.

[Avi (use Gerrit), 2016-09-17 00:55:54]

https://codereview.chromium.org/2334533002/diff/80001/base/threading/platform...
File base/threading/platform_thread.h (right):

https://codereview.chromium.org/2334533002/diff/80001/base/threading/platform...
base/threading/platform_thread.h:213: #endif
On 2016/09/16 23:06:41, reveman wrote:
> On 2016/09/16 at 22:26:29, Avi wrote:
> > You ifdef this to the platform, but don't ifdef any of the other code. Why?
> 
> What other code are you referring to? I didn't ifdef the IPC messages to linux
> but can do that if preferred? The rest of the code should already be limited
to
> linux either through ifdefs or by being in a file that's only built on linux.

Yes, I'm referring to the RenderMessageFilter code, etc.

[reveman, 2016-09-17 09:13:36]

https://codereview.chromium.org/2334533002/diff/80001/base/threading/platform...
File base/threading/platform_thread.h (right):

https://codereview.chromium.org/2334533002/diff/80001/base/threading/platform...
base/threading/platform_thread.h:213: #endif
On 2016/09/17 at 00:55:54, Avi wrote:
> On 2016/09/16 23:06:41, reveman wrote:
> > On 2016/09/16 at 22:26:29, Avi wrote:
> > > You ifdef this to the platform, but don't ifdef any of the other code.
Why?
> > 
> > What other code are you referring to? I didn't ifdef the IPC messages to
linux
> > but can do that if preferred? The rest of the code should already be limited
to
> > linux either through ifdefs or by being in a file that's only built on
linux.
> 
> Yes, I'm referring to the RenderMessageFilter code, etc.

Latest patch will ifdef all the code to linux only. Makes sense until we use it
elsewhere.

[reveman, 2016-09-17 09:13:43]

The CQ bit was checked by reveman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-17 09:13:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-17 10:04:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-17 10:04:15]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[reveman, 2016-09-17 12:09:26]

The CQ bit was checked by reveman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-17 12:09:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-17 12:16:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-17 12:16:44]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[reveman, 2016-09-17 13:28:39]

The CQ bit was checked by reveman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-17 13:29:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-17 14:43:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-17 14:43:40]

Dry run: This issue passed the CQ dry run.

[Nico, 2016-09-19 20:49:30]

We used to have an api similar to this but removed it in
https://codereview.chromium.org/1193303002 'cause it interfered with the
sandbox.

I guess what's different here is that it uses cpusets instead of
pthread_setschedparam -- maybe the existing 
SetCurrentThreadPriorityForPlatform() could use cpusets if available and the
pthread function if not? Is it possible to create the threads with the right
priority in the first place instead of changing it later on?

Wouldn't it make sense to give the display thread higher priority on non-linux
platforms as well?

https://codereview.chromium.org/2334533002/diff/160001/base/threading/platfor...
File base/threading/platform_thread.h (right):

https://codereview.chromium.org/2334533002/diff/160001/base/threading/platfor...
base/threading/platform_thread.h:210: // whole thread group's (i.e. process)
priority.
Maybe add some words on how this compares to the previous
CurrentThreadPriority() funcion

https://codereview.chromium.org/2334533002/diff/160001/base/threading/platfor...
File base/threading/platform_thread_linux.cc (right):

https://codereview.chromium.org/2334533002/diff/160001/base/threading/platfor...
base/threading/platform_thread_linux.cc:140: if (thread_id == getpid())
This should probably be a CHECK()

https://codereview.chromium.org/2334533002/diff/160001/content/browser/render...
File content/browser/renderer_host/render_message_filter.cc (right):

https://codereview.chromium.org/2334533002/diff/160001/content/browser/render...
content/browser/renderer_host/render_message_filter.cc:439: DLOG(WARNING) <<
"Changing priority of main thread is not allowed";
remove (?)

https://codereview.chromium.org/2334533002/diff/160001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2334533002/diff/160001/content/renderer/rende...
content/renderer/render_thread_impl.cc:864:
categorized_worker_pool_->background_worker_thread_id(),
Doesn't the worker pool have more than one id?

[reveman, 2016-09-19 21:43:39]

On 2016/09/19 at 20:49:30, thakis wrote:
> We used to have an api similar to this but removed it in
https://codereview.chromium.org/1193303002 'cause it interfered with the
sandbox.
> 
> I guess what's different here is that it uses cpusets instead of
pthread_setschedparam -- maybe the existing 
SetCurrentThreadPriorityForPlatform() could use cpusets if available and the
pthread function if not? Is it possible to create the threads with the right
priority in the first place instead of changing it later on?

We can't initialize the sandbox after creating threads and the sandbox doesn't
allow access to cpusets or increasing priority of threads.

Long term we might want to change the sandbox instead and figure out a way for
the renderer to be allowed to change cpusets for its threads in a secure way.
That needs more investigation so we've decided that this was a good short term
solution.

> 
> Wouldn't it make sense to give the display thread higher priority on non-linux
platforms as well?

Yes, we're already doing it on Android where we can access Android's thread
priority APIs from the sandboxed renderer. It makes the most sense on ChromeOS
where we control the full system but I think other platforms can benefit as
well. I'd like to start with ChromeOS/Linux for now and then look at doing
something similar on other platforms as a follow up. Does that sound OK?

https://codereview.chromium.org/2334533002/diff/160001/base/threading/platfor...
File base/threading/platform_thread.h (right):

https://codereview.chromium.org/2334533002/diff/160001/base/threading/platfor...
base/threading/platform_thread.h:210: // whole thread group's (i.e. process)
priority.
On 2016/09/19 at 20:49:30, Nico wrote:
> Maybe add some words on how this compares to the previous
CurrentThreadPriority() funcion

Done.

https://codereview.chromium.org/2334533002/diff/160001/base/threading/platfor...
File base/threading/platform_thread_linux.cc (right):

https://codereview.chromium.org/2334533002/diff/160001/base/threading/platfor...
base/threading/platform_thread_linux.cc:140: if (thread_id == getpid())
On 2016/09/19 at 20:49:30, Nico wrote:
> This should probably be a CHECK()

Done.

https://codereview.chromium.org/2334533002/diff/160001/content/browser/render...
File content/browser/renderer_host/render_message_filter.cc (right):

https://codereview.chromium.org/2334533002/diff/160001/content/browser/render...
content/browser/renderer_host/render_message_filter.cc:439: DLOG(WARNING) <<
"Changing priority of main thread is not allowed";
On 2016/09/19 at 20:49:30, Nico wrote:
> remove (?)

Remove the warning log for this case? I'd like to keep it in case we
accidentally write some code in the renderer that tries to do this and this
would at least give some warning to the developer that it's not working as
expected. It's a DLOG to prevent a malicious renderer from flooding the log.

https://codereview.chromium.org/2334533002/diff/160001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2334533002/diff/160001/content/renderer/rende...
content/renderer/render_thread_impl.cc:864:
categorized_worker_pool_->background_worker_thread_id(),
On 2016/09/19 at 20:49:30, Nico wrote:
> Doesn't the worker pool have more than one id?

It has a single thread for background work and N number of threads for
foreground work. Reducing the priority of the thread for background work is
useful to ensure that things like prepaint of contents far away from the
viewport doesn't result in unnecessary power usage or missed frames.

[Nico, 2016-09-19 21:46:11]

Thanks, that makes sense, lgtm. Maybe mention some of what you just said in the
Cl description as well.

[reveman, 2016-09-19 21:58:05]

Description was changed from

==========
base: Allow renderer thread priorities to be changed.

This provides a mechanism for a renderer to change the priority of
its threads and as a result also the cpuset they are assigned to.

A ChildProcessHost IPC message from the renderer is used to request
a thread priority change and have the browser process identify the
renderer thread that is requesting a priority change by checking all
its threads to find a thread with NSpid field in
/proc/[pid]/task/[thread_id]/status that matches the namespace tid
from the renderer.

BUG=chrome-os-partner:56550
TEST=
==========

to

==========
base: Allow renderer thread priorities to be changed.

This provides a mechanism for a renderer to change the priority of
its threads and as a result also the cpuset they are assigned to.

A ChildProcessHost IPC message from the renderer is used to request
a thread priority change and have the browser process identify the
renderer thread that is requesting a priority change by checking all
its threads to find a thread with NSpid field in
/proc/[pid]/task/[thread_id]/status that matches the namespace tid
from the renderer.

This is currently limited to Linux and ChromeOS but follow up work
will investigate the possibility and benefits of doing the same on
other platforms.

Note: Thread priorities in the renderer are already adjusted in a
similar way on Android as the sandbox is not strict enough to
prevent this on Android today.

BUG=chrome-os-partner:56550
TEST=
==========

[reveman, 2016-09-19 21:58:33]

On 2016/09/19 at 21:46:11, thakis wrote:
> Thanks, that makes sense, lgtm. Maybe mention some of what you just said in
the Cl description as well.

Done. Thanks for the quick review.

[reveman, 2016-09-19 22:39:42]

reveman@chromium.org changed reviewers:
+ tsepez@chromium.org

[reveman, 2016-09-19 22:39:43]

+tsepez for *_messages.h

[Tom Sepez, 2016-09-19 22:54:04]

https://codereview.chromium.org/2334533002/diff/180001/base/threading/platfor...
File base/threading/platform_thread_linux.cc (right):

https://codereview.chromium.org/2334533002/diff/180001/base/threading/platfor...
base/threading/platform_thread_linux.cc:140: CHECK_NE(thread_id, getpid());
Since this is running in the browser process, we don't want to let a
less-privileged render DoS the browser by deliberately sending a 0. Let's log
and return doing nothing.

https://codereview.chromium.org/2334533002/diff/180001/content/child/child_pr...
File content/child/child_process.cc (right):

https://codereview.chromium.org/2334533002/diff/180001/content/child/child_pr...
content/child/child_process.cc:109: DCHECK(main_thread_.get());
DCHECK doesn't buy us much, we'll segv in the next line anyways if its null.

[Tom Sepez, 2016-09-19 22:54:32]

LGTM otherwise

[Nico, 2016-09-19 23:01:46]

https://codereview.chromium.org/2334533002/diff/180001/base/threading/platfor...
File base/threading/platform_thread_linux.cc (right):

https://codereview.chromium.org/2334533002/diff/180001/base/threading/platfor...
base/threading/platform_thread_linux.cc:140: CHECK_NE(thread_id, getpid());
On 2016/09/19 22:54:04, Tom Sepez wrote:
> Since this is running in the browser process, we don't want to let a
> less-privileged render DoS the browser by deliberately sending a 0. Let's log
> and return doing nothing.

content checks this before calling. base should CHECK since that's the api
contract

[reveman, 2016-09-19 23:14:22]

Thanks for quick reviews.

https://codereview.chromium.org/2334533002/diff/180001/base/threading/platfor...
File base/threading/platform_thread_linux.cc (right):

https://codereview.chromium.org/2334533002/diff/180001/base/threading/platfor...
base/threading/platform_thread_linux.cc:140: CHECK_NE(thread_id, getpid());
On 2016/09/19 at 23:01:45, Nico wrote:
> On 2016/09/19 22:54:04, Tom Sepez wrote:
> > Since this is running in the browser process, we don't want to let a
> > less-privileged render DoS the browser by deliberately sending a 0. Let's
log
> > and return doing nothing.
> 
> content checks this before calling. base should CHECK since that's the api
contract

Keeping the CHECK as thakis@ suggests. tsepez@, I assume you're OK with this.

https://codereview.chromium.org/2334533002/diff/180001/content/child/child_pr...
File content/child/child_process.cc (right):

https://codereview.chromium.org/2334533002/diff/180001/content/child/child_pr...
content/child/child_process.cc:109: DCHECK(main_thread_.get());
On 2016/09/19 at 22:54:04, Tom Sepez wrote:
> DCHECK doesn't buy us much, we'll segv in the next line anyways if its null.

Makes sense. Done.

[reveman, 2016-09-19 23:15:21]

The CQ bit was checked by reveman@chromium.org

[reveman, 2016-09-19 23:15:21]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
tsepez@chromium.org, thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2334533002/#ps220001
(title: "revert to CHECK_NE")

[commit-bot: I haz the power, 2016-09-19 23:15:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 02:11:22]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-09-20 02:12:41]

Description was changed from

==========
base: Allow renderer thread priorities to be changed.

This provides a mechanism for a renderer to change the priority of
its threads and as a result also the cpuset they are assigned to.

A ChildProcessHost IPC message from the renderer is used to request
a thread priority change and have the browser process identify the
renderer thread that is requesting a priority change by checking all
its threads to find a thread with NSpid field in
/proc/[pid]/task/[thread_id]/status that matches the namespace tid
from the renderer.

This is currently limited to Linux and ChromeOS but follow up work
will investigate the possibility and benefits of doing the same on
other platforms.

Note: Thread priorities in the renderer are already adjusted in a
similar way on Android as the sandbox is not strict enough to
prevent this on Android today.

BUG=chrome-os-partner:56550
TEST=
==========

to

==========
base: Allow renderer thread priorities to be changed.

This provides a mechanism for a renderer to change the priority of
its threads and as a result also the cpuset they are assigned to.

A ChildProcessHost IPC message from the renderer is used to request
a thread priority change and have the browser process identify the
renderer thread that is requesting a priority change by checking all
its threads to find a thread with NSpid field in
/proc/[pid]/task/[thread_id]/status that matches the namespace tid
from the renderer.

This is currently limited to Linux and ChromeOS but follow up work
will investigate the possibility and benefits of doing the same on
other platforms.

Note: Thread priorities in the renderer are already adjusted in a
similar way on Android as the sandbox is not strict enough to
prevent this on Android today.

BUG=chrome-os-partner:56550
TEST=

Committed: https://crrev.com/7b97c3240fb3da0826f244f9b592fc1761fb6554
Cr-Commit-Position: refs/heads/master@{#419649}
==========

[commit-bot: I haz the power, 2016-09-20 02:12:42]

Patchset 12 (id:??) landed as
https://crrev.com/7b97c3240fb3da0826f244f9b592fc1761fb6554
Cr-Commit-Position: refs/heads/master@{#419649}