base: Add ashmem support to base::DiscardableSharedMemory implementation.

base/memory/discardable_shared_memory.cc

Index: base/memory/discardable_shared_memory.cc
diff --git a/base/memory/discardable_shared_memory.cc b/base/memory/discardable_shared_memory.cc
index 851f1add97f0326bd1ce28bb43c228a5b5aafe77..6dc651c729e830b7aad98b4e60d6447fd5954745 100644
--- a/base/memory/discardable_shared_memory.cc
+++ b/base/memory/discardable_shared_memory.cc
@@ -13,6 +13,11 @@
 #include "base/atomicops.h"
 #include "base/logging.h"
 #include "base/numerics/safe_math.h"
+#include "base/process/process_metrics.h"
+
+#if defined(OS_ANDROID)
+#include "third_party/ashmem/ashmem.h"
+#endif

[Avi (use Gerrit), 2014/12/16 23:20:30]

We need to deal with one of the implementations in the shared code file? :(

[reveman, 2014/12/17 16:34:05]

We can introduce discardable_shared_memory_posix/android.cc files with a bit of
refactoring but with the limited amount of platform specific code needed here
right now I think that would just make the code harder to understand and review.

[Avi (use Gerrit), 2014/12/17 16:58:17]

Acknowledged.

 
 namespace base {
 namespace {
@@ -84,14 +89,28 @@ SharedState* SharedStateFromSharedMemory(const SharedMemory& shared_memory) {
   return static_cast<SharedState*>(shared_memory.memory());
 }
 
+// Round up |size| to a multiple of alignment, which must be a power of two.
+size_t Align(size_t alignment, size_t size) {
+  DCHECK_EQ(alignment & (alignment - 1), 0u);

[Avi (use Gerrit), 2014/12/16 23:20:30]

DCHECK_EQ(0u, ...)

[reveman, 2014/12/17 16:34:06]

Done. But out curiosity, why? I used to write code as DCHECK_EQ(expected, value)
based on the idea of being consistent with EXPECT_EQ which assumes this
ordering. However, DCHECK_EQ doesn't care and after finding that most people use
DCHECK_EQ(value, expected), I switched. Are there any other reasons for why
DCHECK_EQ(expected, value) would be preferred? I find DCHECK_EQ(value, expected)
order easier to read..

[Avi (use Gerrit), 2014/12/17 16:58:17]

The printout, mostly. When it fails, it explicitly calls out the first argument
as what's expected.

[reveman, 2014/12/17 18:38:49]

I don't think it does for DCHECK_*. It will just say something like:

Check failed: alignment & (alignment - 1) == 0u (1 vs. 0)

There are some other DCHECKs in this file that use (value, expected) ordering so
I changed this back to that order for consistency. I can change all of them if
there's a good reason for using the other order.

+  return (size + alignment - 1) & ~(alignment - 1);
+}
+
+// Round up |size| to a multiple of page size.
+size_t AlignToPageSize(size_t size) {
+  return Align(base::GetPageSize(), size);
+}
+
 }  // namespace
 
-DiscardableSharedMemory::DiscardableSharedMemory() {
+DiscardableSharedMemory::DiscardableSharedMemory()
+    : mapped_size_(0), locked_page_count_(0) {
 }
 
 DiscardableSharedMemory::DiscardableSharedMemory(
     SharedMemoryHandle shared_memory_handle)
-    : shared_memory_(shared_memory_handle, false) {
+    : shared_memory_(shared_memory_handle, false),
+      mapped_size_(0),
+      locked_page_count_(0) {
 }
 
 DiscardableSharedMemory::~DiscardableSharedMemory() {
@@ -99,13 +118,22 @@ DiscardableSharedMemory::~DiscardableSharedMemory() {
 
 bool DiscardableSharedMemory::CreateAndMap(size_t size) {
   CheckedNumeric<size_t> checked_size = size;
-  checked_size += sizeof(SharedState);
+  checked_size += AlignToPageSize(sizeof(SharedState));
   if (!checked_size.IsValid())
     return false;
 
   if (!shared_memory_.CreateAndMapAnonymous(checked_size.ValueOrDie()))
     return false;
 
+  mapped_size_ =
+      shared_memory_.mapped_size() - AlignToPageSize(sizeof(SharedState));
+
+  locked_page_count_ = AlignToPageSize(mapped_size_) / base::GetPageSize();
+#ifndef NDEBUG
+  for (size_t page = 0; page < locked_page_count_; ++page)
+    locked_pages_.insert(page);
+#endif
+
   DCHECK(last_known_usage_.is_null());
   SharedState new_state(SharedState::LOCKED, Time());
   subtle::Release_Store(&SharedStateFromSharedMemory(shared_memory_)->value.i,
@@ -114,35 +142,124 @@ bool DiscardableSharedMemory::CreateAndMap(size_t size) {
 }
 
 bool DiscardableSharedMemory::Map(size_t size) {
-  return shared_memory_.Map(sizeof(SharedState) + size);
+  if (!shared_memory_.Map(AlignToPageSize(sizeof(SharedState)) + size))
+    return false;
+
+  mapped_size_ =
+      shared_memory_.mapped_size() - AlignToPageSize(sizeof(SharedState));
+
+  locked_page_count_ = AlignToPageSize(mapped_size_) / base::GetPageSize();
+#ifndef NDEBUG
+  for (size_t page = 0; page < locked_page_count_; ++page)
+    locked_pages_.insert(page);
+#endif
+
+  return true;
 }
 
-bool DiscardableSharedMemory::Lock() {
-  DCHECK(shared_memory_.memory());
+bool DiscardableSharedMemory::Lock(size_t offset, size_t length) {
+  DCHECK_EQ(AlignToPageSize(offset), offset);
+  DCHECK_EQ(AlignToPageSize(length), length);
 
   // Return false when instance has been purged or not initialized properly by
   // checking if |last_known_usage_| is NULL.
   if (last_known_usage_.is_null())
     return false;
 
-  SharedState old_state(SharedState::UNLOCKED, last_known_usage_);
-  SharedState new_state(SharedState::LOCKED, Time());
-  SharedState result(subtle::Acquire_CompareAndSwap(
-      &SharedStateFromSharedMemory(shared_memory_)->value.i,
-      old_state.value.i,
-      new_state.value.i));
-  if (result.value.u == old_state.value.u)
-    return true;
+  DCHECK(shared_memory_.memory());
+
+  // We need to successfully acquire the platform independent lock before
+  // individual pages can be locked.
+  if (!locked_page_count_) {

[Avi (use Gerrit), 2014/12/16 23:20:29]

I have a bad gut feeling about this.

It used to be that the only state was in SharedState, and we were using CAS to
touch it, so we got atomicity even across threads. But now, what's the lock on
locked_page_count_? How do we protect ourselves with multiple calls
simultaneously to DiscardableSharedMemory::Lock when locked_page_count_ is
already > 0 and both threads pass by this if() block?

[reveman, 2014/12/17 16:34:05]

Multiple calls on the same instance? This class is not thread-safe so that would
result in undefined behavior. That was the case before this change too. The
client is generally responsible for synchronizing locking and unlocking of
memory from multiple threads/processes.

Multiple simultaneously calls on different instances that represent the same
memory is not really supported either but it's true that this change makes the
effect of such invalid usage worse and harder to support to some degree long
term if necessary. I'm not sure what is the best way to handle this. I
considered added shared state for each page but I'm not sure that's a great
idea. One CAS for each page we lock/unlock or try to purge might have serious
performance implications.

Any ideas?

[Avi (use Gerrit), 2014/12/17 16:58:17]

If this isn't thread-safe, can you at least do a thread check? If you want to
#ifndef NDEBUG it that would be fine.

[reveman, 2014/12/17 18:38:48]

Added a ThreadCollisionWarner.

+    SharedState old_state(SharedState::UNLOCKED, last_known_usage_);
+    SharedState new_state(SharedState::LOCKED, Time());
+    SharedState result(subtle::Acquire_CompareAndSwap(
+        &SharedStateFromSharedMemory(shared_memory_)->value.i,
+        old_state.value.i,
+        new_state.value.i));
+    if (result.value.u != old_state.value.u) {
+      // Update |last_known_usage_| in case the above CAS failed because of
+      // an incorrect timestamp.
+      last_known_usage_ = result.GetTimestamp();
+      return false;
+    }
+  }
+
+  // Zero for length means "everything onward".

[Avi (use Gerrit), 2014/12/16 23:20:30]

What? Why is this not in the .h file? That's too important a piece of
information to bury in the .cc file.

BTW, I have a sketchy feeling with 0 meaning "to the end".

[reveman, 2014/12/17 16:34:05]

Sorry, I meant to also add a comment about this to the .h file but forgot. Done
now.
I stole this idea from ashmem. We can avoid it but it will require us to add
AlignToPage logic to every client, even the ones that have no interest in
unlocking subregions. Let me know what you think. I can go either way but
decided to use this in the initial patch as I think it made the patch easier to
read.

[Avi (use Gerrit), 2014/12/17 16:58:17]

Acknowledged.

+  if (!length)
+    length = AlignToPageSize(mapped_size_) - offset;
+
+  size_t start = offset / base::GetPageSize();
+  size_t end = start + length / base::GetPageSize();
+  DCHECK_LT(start, AlignToPageSize(mapped_size_));
+  DCHECK_LE(end, AlignToPageSize(mapped_size_));
+
+  // Add pages to |locked_page_count_|.
+  // Note: Locking a page that is already locked is an error.
+  locked_page_count_ += end - start;
+#ifndef NDEBUG
+  // Detect incorrect usage by keeping track of exactly what pages are locked.
+  for (size_t page = start; page < end; ++page) {

[Avi (use Gerrit), 2014/12/16 23:20:29]

auto page

[reveman, 2014/12/17 16:34:05]

Done.

+    DCHECK(locked_pages_.find(page) == locked_pages_.end());
+    locked_pages_.insert(page);

[Avi (use Gerrit), 2014/12/16 23:20:30]

A smidge faster to sub in the two lines:

auto result = locked_pages_.insert(page);
DCHECK(result.second);

as it wouldn't have to find twice.

[reveman, 2014/12/17 16:34:05]

Done.

+  }
+  DCHECK_EQ(locked_pages_.size(), locked_page_count_);
+#endif
+
+#if defined(OS_ANDROID)
+  SharedMemoryHandle handle = shared_memory_.handle();
+  DCHECK(SharedMemory::IsHandleValid(handle));
+  if (ashmem_pin_region(
+          handle.fd, AlignToPageSize(sizeof(SharedState)) + offset, length)) {
+    return false;
+  }

[Avi (use Gerrit), 2014/12/16 23:20:30]

Is there any way we can not deal with ashmem here in the common file?

[reveman, 2014/12/17 16:34:05]

I think it's better to include android/posix specific code using ifdefs in this
file rather than moving it out into _posix.cc/_android.cc files right now but
that might change if more platform specific code is added to this file.

+#endif
 
-  // Update |last_known_usage_| in case the above CAS failed because of
-  // an incorrect timestamp.
-  last_known_usage_ = result.GetTimestamp();
-  return false;
+  return true;
 }
 
-void DiscardableSharedMemory::Unlock() {
+void DiscardableSharedMemory::Unlock(size_t offset, size_t length) {
+  DCHECK_EQ(AlignToPageSize(offset), offset);
+  DCHECK_EQ(AlignToPageSize(length), length);
+
+  // Zero for length means "everything onward".
+  if (!length)
+    length = AlignToPageSize(mapped_size_) - offset;
+
   DCHECK(shared_memory_.memory());
 
+#if defined(OS_ANDROID)
+  SharedMemoryHandle handle = shared_memory_.handle();
+  DCHECK(SharedMemory::IsHandleValid(handle));
+  if (ashmem_unpin_region(
+          handle.fd, AlignToPageSize(sizeof(SharedState)) + offset, length)) {
+    DPLOG(ERROR) << "ashmem_unpin_region() failed";
+  }
+#endif
+
+  size_t start = offset / base::GetPageSize();
+  size_t end = start + length / base::GetPageSize();
+  DCHECK_LT(start, AlignToPageSize(mapped_size_));
+  DCHECK_LE(end, AlignToPageSize(mapped_size_));
+
+  // Remove pages from |locked_page_count_|.
+  // Note: Unlocking a page that is not locked is an error.
+  DCHECK_GE(locked_page_count_, end - start);
+  locked_page_count_ -= end - start;
+#ifndef NDEBUG
+  // Detect incorrect usage by keeping track of exactly what pages are locked.
+  for (uintptr_t page = start; page < end; ++page) {

[Avi (use Gerrit), 2014/12/16 23:20:30]

auto page

Might as well use auto in for loop declarations, easier and keeps you from
making a mistake like this one; uintptr_t?

[reveman, 2014/12/17 16:34:06]

Done.

+    DCHECK(locked_pages_.find(page) != locked_pages_.end());
+    locked_pages_.erase(page);

[Avi (use Gerrit), 2014/12/16 23:20:30]

A smidge faster to sub in the two lines:

auto erased_count = locked_pages_.erase(page);
DCHECK_EQ(1u, erased_count);

as it wouldn't have to find twice.

[reveman, 2014/12/17 16:34:05]

Done.

+  }
+  DCHECK_EQ(locked_pages_.size(), locked_page_count_);
+#endif
+
+  // Early out and avoid releasing the platform independent lock if some pages
+  // are still locked.
+  if (locked_page_count_)

[Avi (use Gerrit), 2014/12/16 23:20:30]

Same freakout over concurrency. Unlock used to use a CAS on the SharedState to
keep things atomic; what is guarding locked_page_count_?

[reveman, 2014/12/17 16:34:06]

Same reply. Instances of this class are not thread-safe. ie. you would have a
race between reading and writing |last_known_usage_| prior to this change if
used from multiple threads.

The current answer (maybe not very satisfactory) to multi-instance
locking/unlocking is simply that it's not supported. Maybe the solution is to
just make that more explicit somehow..

+    return;
+
   Time current_time = Now();
   DCHECK(!current_time.is_null());
 
@@ -151,7 +268,7 @@ void DiscardableSharedMemory::Unlock() {
   // Note: timestamp cannot be NULL as that is a unique value used when
   // locked or purged.
   DCHECK(!new_state.GetTimestamp().is_null());
-  // Timestamps precision should at least be accurate to the second.
+  // Timestamp precision should at least be accurate to the second.
   DCHECK_EQ((new_state.GetTimestamp() - Time::UnixEpoch()).InSeconds(),
             (current_time - Time::UnixEpoch()).InSeconds());
   SharedState result(subtle::Release_CompareAndSwap(
@@ -165,7 +282,8 @@ void DiscardableSharedMemory::Unlock() {
 }
 
 void* DiscardableSharedMemory::memory() const {
-  return SharedStateFromSharedMemory(shared_memory_) + 1;
+  return reinterpret_cast<uint8*>(shared_memory_.memory()) +
+         AlignToPageSize(sizeof(SharedState));
 }
 
 bool DiscardableSharedMemory::Purge(Time current_time) {