Fix tab backgrounding on Linux / ChromeOS

base/process_linux.cc

Index: base/process_linux.cc
diff --git a/base/process_linux.cc b/base/process_linux.cc
index bfa1e4a03cc4b2caf6f741c554e701dbe6062804..8232cc5af3d473371d25868d9442e9889774b7b7 100644
--- a/base/process_linux.cc
+++ b/base/process_linux.cc
@@ -10,119 +10,112 @@
 #include "base/file_util.h"
 #include "base/logging.h"
 #include "base/stringprintf.h"
+#include "base/string_split.h"
 
-namespace base {
+namespace {
+const int kForegroundPriority = 0;
 
 #if defined(OS_CHROMEOS)
 // We are more aggressive in our lowering of background process priority
 // for chromeos as we have much more control over other processes running
 // on the machine.
-const int kPriorityAdjustment = 19;
-#else
-const int kPriorityAdjustment = 5;
-#endif
-
-bool Process::IsProcessBackgrounded() const {
-  DCHECK(process_);
-  return saved_priority_ == kUnsetProcessPriority;
-}
+const int kBackgroundPriority = 19;
+const char kControlPath[] = "/tmp/cgroup/cpu%s/cgroup.procs";
+const char kForeground[] = "/chrome_renderers/foreground";

[willchan no longer on Chromium, 2011/11/29 22:23:35]

I'm not particularly happy about this renderer specific code. I guess this was
already the case, so it's not any worse than it was previously.

[DaveMoore, 2011/11/30 19:01:18]

Added todo() to refactor
On 2011/11/29 22:23:35, willchan wrote:

+const char kBackground[] = "/chrome_renderers/background";
+const char kProcPath[] = "/proc/%d/cgroup";
 
-bool Process::SetProcessBackgrounded(bool background) {
-  DCHECK(process_);
+FilePath foreground_file;

[willchan no longer on Chromium, 2011/11/29 22:23:35]

These are static initializers. Are you sure you want this?

[DaveMoore, 2011/11/30 19:01:18]

Changed to LazyInstance<>
On 2011/11/29 22:23:35, willchan wrote:

+FilePath background_file;
 
-#if defined(OS_CHROMEOS)
-  static bool cgroups_inited = false;
+// Check for cgroups files. ChromeOS supports these by default. It creates
+// a cgroup mount in /tmp/cgroup and then configures two cpu task groups,
+// one contains at most a single foreground renderer and the other contains
+// all background renderers. This allows us to limit the impact of background
+// renderers on foreground ones to a greater level than simple renicing.
+bool UseCGroups() {
   static bool use_cgroups = false;

[willchan no longer on Chromium, 2011/11/29 22:23:35]

This code is racy, right?

[DaveMoore, 2011/11/30 19:01:18]

Added Lock.
On 2011/11/29 22:23:35, willchan wrote:

-
-  // Check for cgroups files. ChromeOS supports these by default. It creates
-  // a cgroup mount in /tmp/cgroup and then configures two cpu task groups,
-  // one contains at most a single foreground renderer and the other contains
-  // all background renderers. This allows us to limit the impact of background
-  // renderers on foreground ones to a greater level than simple renicing.
-  FilePath foreground_tasks(
-      "/tmp/cgroup/cpu/chrome_renderers/foreground/tasks");
-  FilePath background_tasks(
-      "/tmp/cgroup/cpu/chrome_renderers/background/tasks");
+  static bool cgroups_inited = false;
   if (!cgroups_inited) {
     cgroups_inited = true;
+    foreground_file = FilePath(StringPrintf(kControlPath, kForeground));
+    background_file = FilePath(StringPrintf(kControlPath, kBackground));
     file_util::FileSystemType foreground_type;
     file_util::FileSystemType background_type;
     use_cgroups =
-        file_util::GetFileSystemType(foreground_tasks, &foreground_type) &&
-        file_util::GetFileSystemType(background_tasks, &background_type) &&
+        file_util::GetFileSystemType(foreground_file, &foreground_type) &&
+        file_util::GetFileSystemType(background_file, &background_type) &&
         foreground_type == file_util::FILE_SYSTEM_CGROUP &&
         background_type == file_util::FILE_SYSTEM_CGROUP;
   }
+  return use_cgroups;
+}
+
+#else
+const int kBackgroundPriority = 5;
+#endif
+}
+
+namespace base {
+
+bool Process::IsProcessBackgrounded() const {
+  DCHECK(process_);
 
-  if (use_cgroups) {
-    if (background) {
-      std::string pid = StringPrintf("%d", process_);
-      if (file_util::WriteFile(background_tasks, pid.c_str(), pid.size()) > 0) {
-        // With cgroups there's no real notion of priority as an int, but this
-        // will ensure we only move renderers back to the foreground group
-        // if we've ever put them in the background one.
-        saved_priority_ = 0;
-        return true;
-      } else {
-        return false;
-      }
+#if defined(OS_CHROMEOS)
+  if (UseCGroups()) {
+    std::string proc;
+    if (file_util::ReadFileToString(
+        FilePath(StringPrintf(kProcPath, process_)),
+        &proc)) {
+      std::vector<std::string> proc_parts;
+      base::SplitString(proc, ':', &proc_parts);
+      DCHECK(proc_parts.size() == 3);
+      bool ret = proc_parts[2] == std::string(kBackground);
+      return ret;
     } else {
-      if (saved_priority_ == kUnsetProcessPriority) {
-        // Can't restore if we were never backgrounded.
-        return false;
-      }
-      std::string pid = StringPrintf("%d", process_);
-      if (file_util::WriteFile(foreground_tasks, pid.c_str(), pid.size()) > 0) {
-        saved_priority_ = kUnsetProcessPriority;
-        return true;
-      } else {
-        return false;
-      }
+      return false;
     }
   }
+#endif
+  return GetPriority() == kBackgroundPriority;
+}
+
+bool Process::SetProcessBackgrounded(bool background) {
+  DCHECK(process_);
+
+#if defined(OS_CHROMEOS)
+  if (UseCGroups()) {
+    std::string pid = StringPrintf("%d", process_);
+    const FilePath file = background ? background_file : foreground_file;
+    return file_util::WriteFile(file, pid.c_str(), pid.size()) > 0;
+  }
 #endif // OS_CHROMEOS
 
-  if (background) {
+  if (!CanBackgroundProcesses())
+    return false;
+
+  int priority = background ? kBackgroundPriority : kForegroundPriority;
+  int result = setpriority(PRIO_PROCESS, process_, priority);
+  DPCHECK(result == 0);
+  return result == 0;
+}
+
+// static
+bool Process::CanBackgroundProcesses() {
+  static bool checked_for_permission = false;

[willchan no longer on Chromium, 2011/11/29 22:23:35]

This is racy.

[DaveMoore, 2011/11/30 19:01:18]

Added lock.
On 2011/11/29 22:23:35, willchan wrote:

+  static bool can_reraise_priority = false;
+
+  if (!checked_for_permission) {
+    checked_for_permission = true;
     // We won't be able to raise the priority if we don't have the right rlimit.
     // The limit may be adjusted in /etc/security/limits.conf for PAM systems.
     struct rlimit rlim;
-    if (getrlimit(RLIMIT_NICE, &rlim) != 0) {
-      // Call to getrlimit failed, don't background.
-      return false;
-    }
-    errno = 0;
-    int current_priority = GetPriority();
-    if (errno) {
-      // Couldn't get priority.
-      return false;
-    }
-    // {set,get}priority values are in the range -20 to 19, where -1 is higher
-    // priority than 0. But rlimit's are in the range from 0 to 39 where
-    // 1 is higher than 0.
-    if ((20 - current_priority) > static_cast<int>(rlim.rlim_cur)) {
-      // User is not allowed to raise the priority back to where it is now.
-      return false;
-    }
-    int result =
-        setpriority(
-            PRIO_PROCESS, process_, current_priority + kPriorityAdjustment);
-    if (result == -1) {
-      DLOG(ERROR) << "Failed to lower priority, errno: " << errno;
-      return false;
-    }
-    saved_priority_ = current_priority;
-    return true;
-  } else {
-    if (saved_priority_ == kUnsetProcessPriority) {
-      // Can't restore if we were never backgrounded.
-      return false;
+    if ((getrlimit(RLIMIT_NICE, &rlim) == 0) &&
+        (20 - kForegroundPriority) <= static_cast<int>(rlim.rlim_cur)) {
+        can_reraise_priority = true;
     }
-    int result = setpriority(PRIO_PROCESS, process_, saved_priority_);
-    // If we can't restore something has gone terribly wrong.
-    DPCHECK(result == 0);
-    saved_priority_ = kUnsetProcessPriority;
-    return true;
   }
+  return can_reraise_priority;
 }
 
 }  // namespace base