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

am: d671746a66

* commit 'd671746a66d87edf9598cfbe077b3430e50a09d3':
  Don't delete and add the binder fd in BaseMessageLoop.

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',