Handle EAGAIN error for the mincore syscall used

base/trace_event/process_memory_dump.cc

 // Copyright 2015 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/trace_event/process_memory_dump.h"
 
 #include <vector>
 
 #include "base/stl_util.h"
 #include "base/trace_event/process_memory_totals.h"
@@ skipping 12 matching lines @@
 
 std::string GetSharedGlobalAllocatorDumpName(
     const MemoryAllocatorDumpGuid& guid) {
   return "global/" + guid.ToString();
 }
 
 }  // namespace
 
 #if defined(COUNT_RESIDENT_BYTES_SUPPORTED)
 // static
-size_t ProcessMemoryDump::CountResidentBytes(void* start_address,
-                                             size_t mapped_size) {
+bool ProcessMemoryDump::CountResidentBytes(void* start_address,
+                                           size_t mapped_size,
+                                           size_t* resident_size) {
+  *resident_size = 0;
   const size_t page_size = GetPageSize();
   const uintptr_t start_pointer = reinterpret_cast<uintptr_t>(start_address);
   DCHECK_EQ(0u, start_pointer % page_size);
 
   // This function allocates a char vector of size number of pages in the given
   // mapped_size. To avoid allocating a large array, the memory is split into
   // chunks. Maximum size of vector allocated, will be
   // kPageChunkSize / page_size.
   const size_t kMaxChunkSize = 32 * 1024 * 1024;
   size_t offset = 0;
-  size_t total_resident_size = 0;
   while (offset < mapped_size) {
     void* chunk_start = reinterpret_cast<void*>(start_pointer + offset);
     const size_t chunk_size = std::min(mapped_size - offset, kMaxChunkSize);
     const size_t page_count = (chunk_size + page_size - 1) / page_size;
     size_t resident_page_count = 0;
 
 #if defined(OS_MACOSX) || defined(OS_IOS)
     std::vector<char> vec(page_count + 1);
     int res = mincore(chunk_start, chunk_size, vector_as_array(&vec));
-    DCHECK(!res);
+    if (res)
+      return false;
+
     for (size_t i = 0; i < page_count; i++)
       resident_page_count += vec[i] & MINCORE_INCORE ? 1 : 0;
 #else   // defined(OS_MACOSX) || defined(OS_IOS)
     std::vector<unsigned char> vec(page_count + 1);
-    int res = mincore(chunk_start, chunk_size, vector_as_array(&vec));
-    DCHECK(!res);
+    int error_counter = 0;
+    int res = 0;
+    do {

[reveman, 2015/10/14 12:31:28]

why do we allow retries here but not above?

[Primiano Tucci (use gerrit), 2015/10/14 12:41:33]

I guess because mac's manpage for mincore says it can only return EINVAL or
EFAULT

[ssid, 2015/10/14 12:44:29]

yes.

[reveman, 2015/10/14 13:03:53]

Got it. Maybe add a comment here or above to explain the difference.

[ssid, 2015/10/14 13:35:55]

Done.

+      res = mincore(chunk_start, chunk_size, vector_as_array(&vec));
+    } while (res == -1 && errno == EAGAIN && error_counter++ < 100);

[Primiano Tucci (use gerrit), 2015/10/14 11:27:08]

i'd cap this at 3. Typically either this is a temporary issue, or it will last
longer.

[ssid, 2015/10/14 12:44:29]

Added comment why it is 100.

[reveman, 2015/10/14 13:03:53]

is it worth retrying at all?

[ssid, 2015/10/14 13:35:55]

That is how eagain is handled in code. Like in case of EINTR.

[reveman, 2015/10/14 14:17:11]

Most of the EAGAIN code I see in the chromium code base doesn't retry. Debug
builds retry forever in the case of EINTR and release builds will ignore the
failure and pretend it worked fine after a number of attempts. Can we do the
same here? That would mean that this function doesn't need a bool return value
after all.

I think one of these two options should be used:

1. CountResidentBytes can fail and we don't bother retrying inside this function
when receiving EAGAIN. The caller can always retry if that makes sense. 
2. CountResidentBytes always succeeds and we retry inside this function when
receiving EAGAIN.

I prefer option 2) as that makes it easier for users of this function but I'm
not familiar with the reason EAGAIN would be returned to tell if this would be
OK.

+    if (res)
+      return false;
+
     for (size_t i = 0; i < page_count; i++)
       resident_page_count += vec[i];
 #endif  // defined(OS_MACOSX) || defined(OS_IOS)
 
-    total_resident_size += resident_page_count * page_size;
+    *resident_size += resident_page_count * page_size;
     offset += kMaxChunkSize;
   }
-  return total_resident_size;
+  return true;
 }
 #endif  // defined(COUNT_RESIDENT_BYTES_SUPPORTED)
 
 ProcessMemoryDump::ProcessMemoryDump(
     const scoped_refptr<MemoryDumpSessionState>& session_state)
     : has_process_totals_(false),
       has_process_mmaps_(false),
       session_state_(session_state) {
 }
 
@@ skipping 132 matching lines @@
 
 void ProcessMemoryDump::AddSuballocation(const MemoryAllocatorDumpGuid& source,
                                          const std::string& target_node_name) {
   std::string child_mad_name = target_node_name + "/__" + source.ToString();
   MemoryAllocatorDump* target_child_mad = CreateAllocatorDump(child_mad_name);
   AddOwnershipEdge(source, target_child_mad->guid());
 }
 
 }  // namespace trace_event
 }  // namespace base