Reland "android: Optionally madvise(MADV_RANDOM) on .text."
This reverts commit 8e157d49242372de81d6804ffcb2e4be722efaa4.
Changes: Made ARM-only, as it doesn't compile on Android x86, and hasn't been
tested outside ARM.
Original change's description:
> android: Optionally madvise(MADV_RANDOM) on .text.
>
> This CL adds:
>
> - "anchor" functions at the beginning and end of .text: these are
> already used in lightweight_cygprofile.cc, and require the orderfile
> to be properly constructed, which is the case since
> https://chromium-review.googlesource.com/753882. These functions are
> trickier for regular builds, as they use --icf=all.
> - madvise(MADV_RANDOM): disable kernel readahead on a given range. This makes
> the reporting logic for code residency clearer, as it actually shows which
> pages are requested. This has to be called as early as possible and from all
> processes, and may be enabled in all builds at a later date. This is
> controlled by a new command-line flag, madvise-random-executable-code.
>
> Bug: 758566
> Change-Id: I8c7a5ca759b355b9e632d6ad027f8d9cd8fb84e8
> Reviewed-on: https://chromium-review.googlesource.com/776895
> Commit-Queue: Benoit L <lizeb@chromium.org>
> Reviewed-by: Camille Lamy <clamy@chromium.org>
> Reviewed-by: Lei Zhang <thestig@chromium.org>
> Reviewed-by: Egor Pasko <pasko@chromium.org>
> Reviewed-by: agrieve <agrieve@chromium.org>
> Reviewed-by: Matthew Cary <mattcary@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#518639}
TBR=pasko@chromium.org,thestig@chromium.org,mgersh@chromium.org,clamy@chromium.org,agrieve@chromium.org,lizeb@chromium.org,mattcary@chromium.org
Change-Id: I8e51971c2abeb1672ce8769ff55d4c3d69f1201a
Bug: 758566
Reviewed-on: https://chromium-review.googlesource.com/786031
Reviewed-by: Egor Pasko <pasko@chromium.org>
Reviewed-by: Benoit L <lizeb@chromium.org>
Commit-Queue: Benoit L <lizeb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#518945}
CrOS-Libchrome-Original-Commit: 3c35531820609841001af012580f85c0b6f1be73
diff --git a/base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java b/base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
index 512f5ec..9c88ff5 100644
--- a/base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
+++ b/base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
@@ -283,7 +283,7 @@
if (coldStart && CommandLine.getInstance().hasSwitch("log-native-library-residency")) {
// nativePeriodicallyCollectResidency() sleeps, run it on another thread,
// and not on the AsyncTask thread pool.
- new Thread(() -> nativePeriodicallyCollectResidency()).run();
+ new Thread(LibraryLoader::nativePeriodicallyCollectResidency).start();
return;
}
diff --git a/base/android/library_loader/anchor_functions.cc b/base/android/library_loader/anchor_functions.cc
new file mode 100644
index 0000000..7ec1012
--- /dev/null
+++ b/base/android/library_loader/anchor_functions.cc
@@ -0,0 +1,74 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/android/library_loader/anchor_functions.h"
+
+#include "base/logging.h"
+
+// asm() macros below don't compile on x86, and haven't been validated outside
+// ARM.
+#if defined(ARCH_CPU_ARMEL)
+// These functions are here to, respectively:
+// 1. Check that functions are ordered
+// 2. Delimit the start of .text
+// 3. Delimit the end of .text
+//
+// (2) and (3) require a suitably constructed orderfile, with these
+// functions at the beginning and end. (1) doesn't need to be in it.
+//
+// These functions are weird: this is due to ICF (Identical Code Folding).
+// The linker merges functions that have the same code, which would be the case
+// if these functions were empty, or simple.
+// Gold's flag --icf=safe will *not* alias functions when their address is used
+// in code, but as of November 2017, we use the default setting that
+// deduplicates function in this case as well.
+//
+// Thus these functions are made to be unique, using inline .word in assembly.
+//
+// Note that code |CheckOrderingSanity()| below will make sure that these
+// functions are not aliased, in case the toolchain becomes really clever.
+extern "C" {
+
+void dummy_function_to_check_ordering() {
+ asm(".word 0xe19c683d");
+ asm(".word 0xb3d2b56");
+}
+
+void dummy_function_to_anchor_text() {
+ asm(".word 0xe1f8940b");
+ asm(".word 0xd5190cda");
+}
+
+void dummy_function_at_the_end_of_text() {
+ asm(".word 0x133b9613");
+ asm(".word 0xdcd8c46a");
+}
+
+} // extern "C"
+
+namespace base {
+namespace android {
+
+const size_t kStartOfText =
+ reinterpret_cast<size_t>(dummy_function_to_anchor_text);
+const size_t kEndOfText =
+ reinterpret_cast<size_t>(dummy_function_at_the_end_of_text);
+
+void CheckOrderingSanity() {
+ // The linker usually keeps the input file ordering for symbols.
+ // dummy_function_to_anchor_text() should then be after
+ // dummy_function_to_check_ordering() without ordering.
+ // This check is thus intended to catch the lack of ordering.
+ CHECK_LT(kStartOfText,
+ reinterpret_cast<size_t>(&dummy_function_to_check_ordering));
+ CHECK_LT(kStartOfText, kEndOfText);
+ CHECK_LT(kStartOfText,
+ reinterpret_cast<size_t>(&dummy_function_to_check_ordering));
+ CHECK_LT(kStartOfText, reinterpret_cast<size_t>(&CheckOrderingSanity));
+ CHECK_GT(kEndOfText, reinterpret_cast<size_t>(&CheckOrderingSanity));
+}
+
+} // namespace android
+} // namespace base
+#endif // defined(ARCH_CPU_ARMEL)
diff --git a/base/android/library_loader/anchor_functions.h b/base/android/library_loader/anchor_functions.h
new file mode 100644
index 0000000..a713d68
--- /dev/null
+++ b/base/android/library_loader/anchor_functions.h
@@ -0,0 +1,27 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef BASE_ANDROID_LIBRARY_LOADER_ANCHOR_FUNCTIONS_H_
+#define BASE_ANDROID_LIBRARY_LOADER_ANCHOR_FUNCTIONS_H_
+
+#include <cstdint>
+
+#include "build/build_config.h"
+
+#if defined(ARCH_CPU_ARMEL)
+namespace base {
+namespace android {
+
+// Start and end of .text, respectively.
+extern const size_t kStartOfText;
+extern const size_t kEndOfText;
+
+// Basic CHECK()s ensuring that the symbols above are correctly set.
+void CheckOrderingSanity();
+
+} // namespace android
+} // namespace base
+#endif // defined(ARCH_CPU_ARMEL)
+
+#endif // BASE_ANDROID_LIBRARY_LOADER_ANCHOR_FUNCTIONS_H_
diff --git a/base/android/library_loader/library_loader_hooks.cc b/base/android/library_loader/library_loader_hooks.cc
index 228f3c1..09d681d 100644
--- a/base/android/library_loader/library_loader_hooks.cc
+++ b/base/android/library_loader/library_loader_hooks.cc
@@ -8,6 +8,8 @@
#include "base/android/library_loader/library_load_from_apk_status_codes.h"
#include "base/android/library_loader/library_prefetcher.h"
#include "base/at_exit.h"
+#include "base/base_switches.h"
+#include "base/command_line.h"
#include "base/metrics/histogram.h"
#include "base/metrics/histogram_macros.h"
#include "jni/LibraryLoader_jni.h"
@@ -167,13 +169,16 @@
static jboolean JNI_LibraryLoader_LibraryLoaded(
JNIEnv* env,
const JavaParamRef<jobject>& jcaller) {
- if (g_native_initialization_hook && !g_native_initialization_hook()) {
+ if (CommandLine::ForCurrentProcess()->HasSwitch(
+ switches::kMadviseRandomExecutableCode)) {
+ NativeLibraryPrefetcher::MadviseRandomText();
+ }
+
+ if (g_native_initialization_hook && !g_native_initialization_hook())
return false;
- }
- if (g_registration_callback == NULL) {
- return true;
- }
- return g_registration_callback(env, NULL);
+ if (g_registration_callback && !g_registration_callback(env, nullptr))
+ return false;
+ return true;
}
void LibraryLoaderExitHook() {
diff --git a/base/android/library_loader/library_prefetcher.cc b/base/android/library_loader/library_prefetcher.cc
index e475541..9defd6f 100644
--- a/base/android/library_loader/library_prefetcher.cc
+++ b/base/android/library_loader/library_prefetcher.cc
@@ -14,6 +14,8 @@
#include <utility>
#include <vector>
+#include "base/android/library_loader/anchor_functions.h"
+#include "base/bits.h"
#include "base/files/file.h"
#include "base/format_macros.h"
#include "base/macros.h"
@@ -95,11 +97,22 @@
residency->resize(size_in_pages);
int err = HANDLE_EINTR(
mincore(reinterpret_cast<void*>(range.first), size, &(*residency)[0]));
- if (err) {
- PLOG(ERROR) << "mincore() failed";
- return false;
- }
- return true;
+ PLOG_IF(ERROR, err) << "mincore() failed";
+ return !err;
+}
+
+#if defined(ARCH_CPU_ARMEL)
+// Returns the start and end of .text, aligned to the lower and upper page
+// boundaries, respectively.
+NativeLibraryPrefetcher::AddressRange GetTextRange() {
+ // |kStartOftext| may not be at the beginning of a page, since .plt can be
+ // before it, yet in the same mapping for instance.
+ size_t start_page = kStartOfText - kStartOfText % kPageSize;
+ // Set the end to the page on which the beginning of the last symbol is. The
+ // actual symbol may spill into the next page by a few bytes, but this is
+ // outside of the executable code range anyway.
+ size_t end_page = base::bits::Align(kEndOfText, kPageSize);
+ return {start_page, end_page};
}
// Timestamp in ns since Unix Epoch, and residency, as returned by mincore().
@@ -156,7 +169,7 @@
file.WriteAtCurrentPos(&dump[0], dump.size());
}
}
-
+#endif // defined(ARCH_CPU_ARMEL)
} // namespace
// static
@@ -277,28 +290,35 @@
// static
void NativeLibraryPrefetcher::PeriodicallyCollectResidency() {
+#if defined(ARCH_CPU_ARMEL)
CHECK_EQ(static_cast<long>(kPageSize), sysconf(_SC_PAGESIZE));
- std::vector<AddressRange> ranges;
- if (!FindRanges(&ranges))
- return;
- // To keep only the range containing .text, find out which one contains
- // ourself.
- const size_t here = reinterpret_cast<size_t>(
- &NativeLibraryPrefetcher::PeriodicallyCollectResidency);
- auto it =
- std::find_if(ranges.begin(), ranges.end(), [here](const AddressRange& r) {
- return r.first <= here && here <= r.second;
- });
- CHECK(ranges.end() != it);
-
+ const auto& range = GetTextRange();
auto data = std::make_unique<std::vector<TimestampAndResidency>>();
for (int i = 0; i < 60; ++i) {
- if (!CollectResidency(*it, data.get()))
+ if (!CollectResidency(range, data.get()))
return;
usleep(2e5);
}
DumpResidency(std::move(data));
+#else
+ CHECK(false) << "Only supported on ARM";
+#endif
+}
+
+// static
+void NativeLibraryPrefetcher::MadviseRandomText() {
+#if defined(ARCH_CPU_ARMEL)
+ CheckOrderingSanity();
+ const auto& range = GetTextRange();
+ size_t size = range.second - range.first;
+ int err = madvise(reinterpret_cast<void*>(range.first), size, MADV_RANDOM);
+ if (err) {
+ PLOG(ERROR) << "madvise() failed";
+ }
+#else
+ CHECK(false) << "Only supported on ARM.";
+#endif // defined(ARCH_CPU_ARMEL)
}
} // namespace android
diff --git a/base/android/library_loader/library_prefetcher.h b/base/android/library_loader/library_prefetcher.h
index ba001ee..bff90a4 100644
--- a/base/android/library_loader/library_prefetcher.h
+++ b/base/android/library_loader/library_prefetcher.h
@@ -43,6 +43,9 @@
// dumps it to disk.
static void PeriodicallyCollectResidency();
+ // Calls madvise(MADV_RANDOM) on the native library executable code range.
+ static void MadviseRandomText();
+
private:
// Returns true if the region matches native code or data.
static bool IsGoodToPrefetch(const base::debug::MappedMemoryRegion& region);
diff --git a/base/base_switches.cc b/base/base_switches.cc
index 00055e8..9554233 100644
--- a/base/base_switches.cc
+++ b/base/base_switches.cc
@@ -119,4 +119,10 @@
"enable-crash-reporter-for-testing";
#endif
+#if defined(OS_ANDROID)
+// Calls madvise(MADV_RANDOM) on executable code right after the library is
+// loaded, from all processes.
+const char kMadviseRandomExecutableCode[] = "madvise-random-executable-code";
+#endif
+
} // namespace switches
diff --git a/base/base_switches.h b/base/base_switches.h
index 34e3c8f..56be306 100644
--- a/base/base_switches.h
+++ b/base/base_switches.h
@@ -41,6 +41,10 @@
extern const char kEnableCrashReporterForTesting[];
#endif
+#if defined(OS_ANDROID)
+extern const char kMadviseRandomExecutableCode[];
+#endif
+
} // namespace switches
#endif // BASE_BASE_SWITCHES_H_