Don't delete and add the binder fd in BaseMessageLoop.

The binder driver polling mechanism doesn't work well with epoll. In
particular, you can't add the binder file descriptor to epoll (with
EPOLL_CTL_ADD) when there's work to do in the binder driver, then
wait for it when there isn't any more work to do.

To avoid this situation, we only add the binder file descriptor at the
beginning and never try to remove and add it back when running
callbacks for it.

Bug: 26524111
TEST=unittests for parsing.
TEST=Tested with an example service, strace shows the binder fd is not added/removed each time.

Change-Id: I11d6af5593fa78631f80e9ba09d933f4b8ef70df

5 files changed, 138 insertions, 3 deletions

diff --git a/Android.mk b/Android.mk
index 2181a3a..89cbc73 100644
--- a/Android.mk
+++ b/Android.mk
@@ -107,6 +107,7 @@ libbrillo_test_sources := \
     brillo/http/http_utils_unittest.cc \
     brillo/key_value_store_unittest.cc \
     brillo/map_utils_unittest.cc \
+    brillo/message_loops/base_message_loop_unittest.cc \
     brillo/message_loops/fake_message_loop_unittest.cc \
     brillo/mime_utils_unittest.cc \
     brillo/osrelease_reader_unittest.cc \
diff --git a/brillo/message_loops/base_message_loop.cc b/brillo/message_loops/base_message_loop.cc
index a24c64b..49dba40 100644
--- a/brillo/message_loops/base_message_loop.cc
+++ b/brillo/message_loops/base_message_loop.cc
@@ -5,17 +5,37 @@
 #include <brillo/message_loops/base_message_loop.h>
 
 #include <fcntl.h>
+#include <linux/major.h>
+#include <sys/stat.h>
+#include <sys/types.h>
 #include <unistd.h>
 
+#include <vector>
+
 #include <base/bind.h>
+#include <base/files/file_path.h>
+#include <base/files/file_util.h>
 #include <base/run_loop.h>
+#include <base/strings/string_number_conversions.h>
+#include <base/strings/string_split.h>
 
 #include <brillo/location_logging.h>
+#include <brillo/strings/string_utils.h>
 
 using base::Closure;
 
+namespace {
+
+const char kMiscMinorPath[] = "/proc/misc";
+const char kBinderDriverName[] = "binder";
+
+}  // namespace
+
 namespace brillo {
 
+const int BaseMessageLoop::kInvalidMinor = -1;
+const int BaseMessageLoop::kUninitializedMinor = -2;
+
 BaseMessageLoop::BaseMessageLoop(base::MessageLoopForIO* base_loop)
     : base_loop_(base_loop),
       weak_ptr_factory_(this) {}
@@ -107,6 +127,23 @@ MessageLoop::TaskId BaseMessageLoop::WatchFileDescriptor(
     io_tasks_.erase(task_id);
     return MessageLoop::kTaskIdNull;
   }
+
+  // Determine if the passed fd is the binder file descriptor. For that, we need
+  // to check that is a special char device and that the major and minor device
+  // numbers match. The binder file descriptor can't be removed and added back
+  // to an epoll group when there's work available to be done by the file
+  // descriptor due to bugs in the binder driver (b/26524111) when used with
+  // epoll. Therefore, we flag the binder fd and never attempt to remove it.
+  // This may cause the binder file descriptor to be attended with higher
+  // priority and cause starvation of other events.
+  struct stat buf;
+  if (fstat(fd, &buf) == 0 &&
+      S_ISCHR(buf.st_mode) &&
+      major(buf.st_rdev) == MISC_MAJOR &&
+      minor(buf.st_rdev) == GetBinderMinor()) {
+    it_bool.first->second.RunImmediately();
+  }
+
   return task_id;
 }
 
@@ -219,6 +256,38 @@ void BaseMessageLoop::OnFileReadyPostedTask(MessageLoop::TaskId task_id) {
   task_it->second.OnFileReadyPostedTask();
 }
 
+int BaseMessageLoop::ParseBinderMinor(
+    const std::string& file_contents) {
+  int result = kInvalidMinor;
+  // Split along '\n', then along the ' '. Note that base::SplitString trims all
+  // white spaces at the beginning and end after splitting.
+  std::vector<std::string> lines;
+  base::SplitString(file_contents, '\n', &lines);
+  for (const std::string& line : lines) {
+    if (line.empty())
+      continue;
+    std::string number;
+    std::string name;
+    if (!string_utils::SplitAtFirst(line, " ", &number, &name, false))
+      continue;
+
+    if (name == kBinderDriverName && base::StringToInt(number, &result))
+      break;
+  }
+  return result;
+}
+
+unsigned int BaseMessageLoop::GetBinderMinor() {
+  if (binder_minor_ != kUninitializedMinor)
+    return binder_minor_;
+
+  std::string proc_misc;
+  if (!base::ReadFileToString(base::FilePath(kMiscMinorPath), &proc_misc))
+    return binder_minor_;
+  binder_minor_ = ParseBinderMinor(proc_misc);
+  return binder_minor_;
+}
+
 BaseMessageLoop::IOTask::IOTask(const tracked_objects::Location& location,
                                 BaseMessageLoop* loop,
                                 MessageLoop::TaskId task_id,
@@ -248,9 +317,17 @@ void BaseMessageLoop::IOTask::OnFileCanWriteWithoutBlocking(int /* fd */) {
 }
 
 void BaseMessageLoop::IOTask::OnFileReady() {
+  // For file descriptors marked with the immediate_run flag, we don't call
+  // StopWatching() and wait, instead we dispatch the callback immediately.
+  if (immediate_run_) {
+    posted_task_pending_ = true;
+    OnFileReadyPostedTask();
+    return;
+  }
+
   // When the file descriptor becomes available we stop watching for it and
   // schedule a task to run the callback from the main loop. The callback will
-  // run using the same scheduler use to run other delayed tasks, avoiding
+  // run using the same scheduler used to run other delayed tasks, avoiding
   // starvation of the available posted tasks if there are file descriptors
   // always available. The new posted task will use the same TaskId as the
   // current file descriptor watching task an could be canceled in either state,
@@ -300,8 +377,10 @@ void BaseMessageLoop::IOTask::OnFileReadyPostedTask() {
   if (persistent_) {
     // In the persistent case we just run the callback. If this callback cancels
     // the task id, we can't access |this| anymore, so we re-start watching the
-    // file descriptor before running the callback.
-    StartWatching();
+    // file descriptor before running the callback, unless this is a fd where
+    // we didn't stop watching the file descriptor when it became available.
+    if (!immediate_run_)
+      StartWatching();
     closure_.Run();
   } else {
     // This will destroy |this|, the fd_watcher and therefore stop watching this
diff --git a/brillo/message_loops/base_message_loop.h b/brillo/message_loops/base_message_loop.h
index 529d476..eb5de54 100644
--- a/brillo/message_loops/base_message_loop.h
+++ b/brillo/message_loops/base_message_loop.h
@@ -14,11 +14,13 @@
 
 #include <map>
 #include <memory>
+#include <string>
 
 #include <base/location.h>
 #include <base/memory/weak_ptr.h>
 #include <base/message_loop/message_loop.h>
 #include <base/time/time.h>
+#include <gtest/gtest_prod.h>
 
 #include <brillo/brillo_export.h>
 #include <brillo/message_loops/message_loop.h>
@@ -51,6 +53,16 @@ class BRILLO_EXPORT BaseMessageLoop : public MessageLoop {
   base::Closure QuitClosure() const;
 
  private:
+  FRIEND_TEST(BaseMessageLoopTest, ParseBinderMinor);
+
+  static const int kInvalidMinor;
+  static const int kUninitializedMinor;
+
+  // Parses the contents of the file /proc/misc passed in |file_contents| and
+  // returns the minor device number reported for binder. On error or if not
+  // found, returns kInvalidMinor.
+  static int ParseBinderMinor(const std::string& file_contents);
+
   // Called by base::MessageLoopForIO when is time to call the callback
   // scheduled with Post*Task() of id |task_id|, even if it was canceled.
   void OnRanPostedTask(MessageLoop::TaskId task_id);
@@ -64,6 +76,9 @@ class BRILLO_EXPORT BaseMessageLoop : public MessageLoop {
   // Return a new unused task_id.
   TaskId NextTaskId();
 
+  // Returns binder minor device number.
+  unsigned int GetBinderMinor();
+
   struct DelayedTask {
     tracked_objects::Location location;
 
@@ -98,6 +113,10 @@ class BRILLO_EXPORT BaseMessageLoop : public MessageLoop {
     // same semantics as MessageLoop::CancelTask().
     bool CancelTask();
 
+    // Sets the closure to be run immediately whenever the file descriptor
+    // becomes ready.
+    void RunImmediately() { immediate_run_= true; }
+
    private:
     tracked_objects::Location location_;
     BaseMessageLoop* loop_;
@@ -116,6 +135,11 @@ class BRILLO_EXPORT BaseMessageLoop : public MessageLoop {
     // Tells whether there is a pending call to OnFileReadPostedTask().
     bool posted_task_pending_{false};
 
+    // Whether the registered callback should be running immediately when the
+    // file descriptor is ready, as opposed to posting a task to the main loop
+    // to prevent starvation.
+    bool immediate_run_{false};
+
     // base::MessageLoopForIO::Watcher overrides:
     void OnFileCanReadWithoutBlocking(int fd) override;
     void OnFileCanWriteWithoutBlocking(int fd) override;
@@ -143,6 +167,12 @@ class BRILLO_EXPORT BaseMessageLoop : public MessageLoop {
   // The RunLoop instance used to run the main loop from Run().
   base::RunLoop* base_run_loop_{nullptr};
 
+  // The binder minor device number. Binder is a "misc" char device with a
+  // dynamically allocated minor number. When uninitialized, this value will
+  // be negative, otherwise, it will hold the minor part of the binder device
+  // number. This is populated by GetBinderMinor().
+  int binder_minor_{kUninitializedMinor};
+
   // We use a WeakPtrFactory to schedule tasks with the base::MessageLoopForIO
   // since we can't cancel the callbacks we have scheduled there once this
   // instance is destroyed.
diff --git a/brillo/message_loops/base_message_loop_unittest.cc b/brillo/message_loops/base_message_loop_unittest.cc
new file mode 100644
index 0000000..9e052a8
--- /dev/null
+++ b/brillo/message_loops/base_message_loop_unittest.cc
@@ -0,0 +1,24 @@
+// Copyright 2016 The Chromium OS 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 <brillo/message_loops/base_message_loop.h>
+
+#include <gtest/gtest.h>
+
+#include <brillo/message_loops/message_loop.h>
+
+namespace brillo {
+
+class BaseMessageLoopTest : public ::testing::Test {};
+
+TEST(BaseMessageLoopTest, ParseBinderMinor) {
+  EXPECT_EQ(57, BaseMessageLoop::ParseBinderMinor(
+      "227 mcelog\n 58 sw_sync\n 59 ashmem\n 57 binder\n239 uhid\n"));
+  EXPECT_EQ(123, BaseMessageLoop::ParseBinderMinor("123 binder\n"));
+
+  EXPECT_EQ(BaseMessageLoop::kInvalidMinor,
+            BaseMessageLoop::ParseBinderMinor("227 foo\n239 bar\n"));
+}
+
+}  // namespace brillo
diff --git a/libbrillo.gypi b/libbrillo.gypi
index 03eb2d9..6c317d7 100644
--- a/libbrillo.gypi
+++ b/libbrillo.gypi
@@ -337,6 +337,7 @@
             'brillo/http/http_utils_unittest.cc',
             'brillo/key_value_store_unittest.cc',
             'brillo/map_utils_unittest.cc',
+            'brillo/message_loops/base_message_loop_unittest.cc',
             'brillo/message_loops/fake_message_loop_unittest.cc',
             'brillo/message_loops/glib_message_loop_unittest.cc',
             'brillo/message_loops/message_loop_unittest.cc',