New issue
Advanced search Search tips

Issue 913700 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug



Sign in to add a comment

Make Chrome mlock itself

Project Member Reported by g...@chromium.org, Dec 10

Issue description

I'm told we have hacks in the kernel that try to keep us from swapping Chrome out.

I think the consensus is that these are undesirable, and that we should teach Chrome how to mlock itself instead.
 
Status: Assigned (was: Untriaged)
CL to lift ulimits in `ui` (which spawns session_manager, which spawns Chrome): https://chromium-review.googlesource.com/c/chromiumos/platform2/+/1370846

As noted, it's not really tested so far. Not poking a reviewer until I've actually tested it with an actual patch to Chrome/etc.
i'm not sure the ui job is the right place for this as that affects not just the browser processes but the entire process tree from session manager down.

you probably want to do something like we do for fds:
  https://chromium-review.googlesource.com/448356

and leave it to the browser to pick sane soft limits for itself on a per-process basis.
Cc: tcwang@chromium.org sonnyrao@chromium.org
Yeah I made a few prototype CLs to do this -- I'll put them up
https://chromium-review.googlesource.com/c/chromiumos/platform2/+/1370851

I got a strange error while uploading that CL:

Errors in PROJECT *chromiumos/platform2*!                                                                
    COMMIT 9f088818:
        Description:
            >libchromeos-ui: increase limit on mlockable memory
            >
            >This increases the limit on mlockable memory so that Chrome may lock
            >text pages that can be backed by huge pages.  This should help prevent
            >the loss of the huge pages when we get into memory pressure
            >situations.
            >
            >BUG=chromium:913700
            >TEST=build chrome with mlock patches and see that chrome was able to
            >lock text
            >
            >Change-Id: Iaee105d1a85b3f99ee13714e4602d14cdcd63856
            >
            >
        Errors:
            * clang-format.py errors/warnings
            
            /mnt/host/source/chromium/src/buildtools/linux64/clang-format: /lib64/libtinfo.so.5: no version information available (required by /mnt/host/source/chromium/src/buildtools/linux64/clang-format)
            /mnt/host/source/chromium/src/buildtools/linux64/clang-format: /usr/lib/gcc/x86_64-pc-linux-gnu/4.9.x/libstdc++.so.6: version `GLIBCXX_3.4.21' not found (required by /mnt/host/source/chromium/src/buildtools/linux64/clang-format)
            error: `/mnt/host/source/chromium/src/buildtools/linux64/clang-format -style=file -lines=227:233 -assume-filename=libchromeos-ui/chromeos/ui/chromium_command_builder.cc` failed
            clang-format failed:
            return code: 2; command: /mnt/host/source/chromium/src/buildtools/clang_format/script/git-clang-format --binary /mnt/host/source/chromium/src/buildtools/linux64/clang-format --diff --style file '9f08881867525b95c789b1e0e0f7d3aa8f236fad^' 9f08881867525b95c789b1e0e0f7d3aa8f236fad -- arc/ authpolicy/ biod/ bluetooth/ buffet/ cfm-device-updater/ crash-reporter/ cros-disks/ crosdns/ diagnostics/ dlcservice/ goldfishd/ imageloader/ installer/ libchromeos-ui/ libcontainer/ libpasswordprovider/ login_manager/ metrics/ mist/ modemfwd/ oobe_config/ portier/ power_manager/ run_oci/ smbprovider/ vm_tools/ vpn-manager/ wimax_manager/
            cmd=['/mnt/host/source/chromium/src/buildtools/clang_format/script/git-clang-format', '--binary', '/mnt/host/source/chromium/src/buildtools/linux64/clang-format', '--diff', '--style', 'file', '9f08881867525b95c789b1e0e0f7d3aa8f236fad^', '9f08881867525b95c789b1e0e0f7d3aa8f236fad', '--', 'arc/', 'authpolicy/', 'biod/', 'bluetooth/', 'buffet/', 'cfm-device-updater/', 'crash-reporter/', 'cros-disks/', 'crosdns/', 'diagnostics/', 'dlcservice/', 'goldfishd/', 'imageloader/', 'installer/', 'libchromeos-ui/', 'libcontainer/', 'libpasswordprovider/', 'login_manager/', 'metrics/', 'mist/', 'modemfwd/', 'oobe_config/', 'portier/', 'power_manager/', 'run_oci/', 'smbprovider/', 'vm_tools/', 'vpn-manager/', 'wimax_manager/']
            
            Please report this to the clang team.
Thanks for the patches!

Did we want to only mlock hugepages, or did we want to mlock everything in the binary? I was thinking that HP-only made a lot of sense <4.4, but mlock2+MLOCK_ONFAULT looked pretty attractive for everything that supports it.

I ask because in the 'mlock all the things' case, we'd probably want to bump the 64MB to 256MB, or some other suitably > sizeof(chrome's .text) number.
I was only locking what's in the huge pages.  I saw the mlock2 + MLOCK_ONFAULT stuff but I'm not sure if it's necessary, so I was being conservative here.  I'd want to avoid locking things in that say only get used on startup or at certain times, because that would just be a waste.  

The chrome patch is here:
https://chromium-review.googlesource.com/c/chromium/src/+/1371206
> I got a strange error while uploading that CL:

make sure you sync your entire tree so you get a newer clang-format.  or run repo upload outside of the sdk.
Project Member

Comment 10 by bugdroid1@chromium.org, Dec 11

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/07f4d1a58bc25e0c57abee9441c11e847b1c0683

commit 07f4d1a58bc25e0c57abee9441c11e847b1c0683
Author: Sonny Rao <sonnyrao@chromium.org>
Date: Tue Dec 11 23:37:40 2018

chromeos_hugepage: mlock in the area covered by huge pages

We are putting our most used text into a region that can be backed by
huge pages, however we can lose those pages when we encounter memory
pressure, so we should be locking them into place to avoid that.

R=llozano@chromium.org

Bug: 913700
Change-Id: If9ae32ae046a21d5bf7af4e43624323b3429d9f8
Reviewed-on: https://chromium-review.googlesource.com/c/1371206
Reviewed-by: George Burgess <gbiv@chromium.org>
Commit-Queue: Sonny Rao <sonnyrao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615724}
[modify] https://crrev.com/07f4d1a58bc25e0c57abee9441c11e847b1c0683/chromeos/hugepage_text/hugepage_text.cc

One other thing I'm noticing is that it seems like we don't have huge page text on the browser process, does that seem correct?  I was testing with some of these changes and I noticed the huge page setup is happening in the zygote, which is spawning all of the renderers and some other processes, but if I look in /proc/<pid>/smaps for the browser process, I'm not seeing any usage of huge pages.

I think to remove our kernel hack with the file pages, we'll need to do something for the browser process as well.
To give some context, most of the UI including handling of input events is happening in the browser process, so if we lose text pages there, the user generally notices janky UI and cursor.  After chatting with Luis, it sounds like it's possible to just to mlock in the same code pages in browser process without any loader changes.  George, do you know how to do that?
yes, by design huge pages were only enabled on the renderer process. That is where we saw a perf improvement.

We could just do the mlock trick on the browser, without huge pages.


Project Member

Comment 14 by bugdroid1@chromium.org, Dec 13

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/c4851b7d352f32a0ad84b9ca6eb5e32b7171486d

commit c4851b7d352f32a0ad84b9ca6eb5e32b7171486d
Author: Sonny Rao <sonnyrao@chromium.org>
Date: Thu Dec 13 01:04:54 2018

libchromeos-ui: increase limit on mlockable memory

This increases the limit on mlockable memory so that Chrome may lock
text pages that can be backed by huge pages.  This should help prevent
the loss of the huge pages when we get into memory pressure
situations.

BUG=chromium:913700
TEST=build chrome with mlock patches and see that chrome was able to
lock text

Change-Id: Iaee105d1a85b3f99ee13714e4602d14cdcd63856
Reviewed-on: https://chromium-review.googlesource.com/1370851
Commit-Ready: Sonny Rao <sonnyrao@chromium.org>
Tested-by: Sonny Rao <sonnyrao@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/c4851b7d352f32a0ad84b9ca6eb5e32b7171486d/libchromeos-ui/chromeos/ui/chromium_command_builder.cc

Project Member

Comment 15 by bugdroid1@chromium.org, Dec 13

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5e93420c2ed448d09b16cbb6c4800d825d1d7a96

commit 5e93420c2ed448d09b16cbb6c4800d825d1d7a96
Author: George Burgess IV <gbiv@chromium.org>
Date: Thu Dec 13 20:56:53 2018

hugepage_text: note that we now mlock things

CL 1371206 made us mlock hugepages. I think this deserves a callout in
docs, and tweaks to function names so they more accurately describe what
we're doing.

This CL does all of that.

No functional change is intended.

Bug: 913700
Test: Chrome still builds for falco, which runs this code.
Change-Id: If8dfe93e147bec47c100bb2eb58ba05625c9bcfd
Reviewed-on: https://chromium-review.googlesource.com/c/1374960
Reviewed-by: Ken Rockot <rockot@google.com>
Reviewed-by: George Burgess <gbiv@chromium.org>
Commit-Queue: George Burgess <gbiv@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616425}
[modify] https://crrev.com/5e93420c2ed448d09b16cbb6c4800d825d1d7a96/chrome/app/chrome_main_delegate.cc
[modify] https://crrev.com/5e93420c2ed448d09b16cbb6c4800d825d1d7a96/chromeos/hugepage_text/hugepage_text.cc
[modify] https://crrev.com/5e93420c2ed448d09b16cbb6c4800d825d1d7a96/chromeos/hugepage_text/hugepage_text.h

Project Member

Comment 16 by bugdroid1@chromium.org, Dec 23

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast-tests/+/bca375e4615ac617d7f652d9bc802c300f38a8bd

commit bca375e4615ac617d7f652d9bc802c300f38a8bd
Author: George Burgess IV <gbiv@chromium.org>
Date: Sun Dec 23 22:46:12 2018

tast-tests: Be sure that we're mlocking memory in Chrome

For consistent performance under memory pressure, we try to mlock pieces
of Chrome. This tast ensures that this mlocking still happens in the
future, since the only indication we otherwise have of it disappearing
is a degraded UX and a few LOG(INFO) messages.

BUG=chromium:913700
TEST=tast run $(cat /tmp/cros_ip) platform.ChromeMlocked

Change-Id: I5bc90effc49e562cbf7e1770238cc10afa2ba5c6
Reviewed-on: https://chromium-review.googlesource.com/1381891
Commit-Ready: George Burgess <gbiv@chromium.org>
Tested-by: George Burgess <gbiv@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[add] https://crrev.com/bca375e4615ac617d7f652d9bc802c300f38a8bd/src/chromiumos/tast/local/bundles/cros/platform/chrome_mlocked.go

Sign in to add a comment