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..4b29bba78c57e646350799de82ed19dbafd7f79d 100644
--- a/base/process_linux.cc
+++ b/base/process_linux.cc
@@ -8,121 +8,126 @@
 #include <sys/resource.h>
 
 #include "base/file_util.h"
+#include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/stringprintf.h"
+#include "base/string_split.h"
+#include "base/synchronization/lock.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;
-}
+//
+// TODO(davemoore) Refactor this by adding support for higher levels to set
+// the foregrounding / backgrounding process so we don't have to keep
+// chrome / chromeos specific logic here.
+const int kBackgroundPriority = 19;
+const char kControlPath[] = "/tmp/cgroup/cpu%s/cgroup.procs";
+const char kForeground[] = "/chrome_renderers/foreground";
+const char kBackground[] = "/chrome_renderers/background";
+const char kProcPath[] = "/proc/%d/cgroup";
 
-bool Process::SetProcessBackgrounded(bool background) {
-  DCHECK(process_);
+base::LazyInstance<FilePath> foreground_file = LAZY_INSTANCE_INITIALIZER;
+base::LazyInstance<FilePath> background_file = LAZY_INSTANCE_INITIALIZER;
 
-#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() {
+  base::Lock lock;

[willchan no longer on Chromium, 2011/12/01 06:26:32]

Is this lock used?

[DaveMoore, 2011/12/01 22:14:17]

I meant AutoLock, but it's gone now
On 2011/12/01 06:26:32, willchan wrote:

   static bool use_cgroups = 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.
-  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;

[willchan no longer on Chromium, 2011/12/01 06:26:32]

This cgroups_inited is still racy. I think you should just get rid of it. Create
a struct that encompasses both foreground_file and background_file. In the
struct's constructor, initialize the foreground_file and background_file.

[DaveMoore, 2011/12/01 22:14:17]

Done.

+    *foreground_file.Pointer() =
+        FilePath(StringPrintf(kControlPath, kForeground));
+    *background_file.Pointer() =
+        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.Get(), &foreground_type) &&
+        file_util::GetFileSystemType(background_file.Get(), &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.Get() : foreground_file.Get();
+    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() {
+  return false;

[willchan no longer on Chromium, 2011/12/01 06:26:32]

What's going on here? Always returning false? The lock is not needed, you should
use LazyInstance to wrap can_reraise_priority, and get rid of the
checked_for_permission.

[DaveMoore, 2011/12/01 22:14:17]

Accidentally left this in when I was testing. Changed as suggested.

On 2011/12/01 06:26:32, willchan wrote:

+  base::Lock lock;
+  static bool checked_for_permission = false;
+  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