Merge "Re-enable file descriptor caching and add option to skip caching"

5 files changed, 91 insertions, 59 deletions

diff --git a/libprocessgroup/include/processgroup/processgroup.h b/libprocessgroup/include/processgroup/processgroup.h
index 86e60356b..8d150adb0 100644
--- a/libprocessgroup/include/processgroup/processgroup.h
+++ b/libprocessgroup/include/processgroup/processgroup.h
@@ -32,8 +32,9 @@ bool CgroupGetAttributePathForTask(const std::string& attr_name, int tid, std::s
 
 bool UsePerAppMemcg();
 
-bool SetTaskProfiles(int tid, const std::vector<std::string>& profiles);
-bool SetProcessProfiles(uid_t uid, pid_t pid, const std::vector<std::string>& profiles);
+bool SetTaskProfiles(int tid, const std::vector<std::string>& profiles, bool use_fd_cache = false);
+bool SetProcessProfiles(uid_t uid, pid_t pid, const std::vector<std::string>& profiles,
+                        bool use_fd_cache = false);
 
 // Return 0 and removes the cgroup if there are no longer any processes in it.
 // Returns -1 in the case of an error occurring or if there are processes still running
diff --git a/libprocessgroup/processgroup.cpp b/libprocessgroup/processgroup.cpp
index abe63dd70..1485ae986 100644
--- a/libprocessgroup/processgroup.cpp
+++ b/libprocessgroup/processgroup.cpp
@@ -112,12 +112,16 @@ static bool isMemoryCgroupSupported() {
     return memcg_supported;
 }
 
-bool SetProcessProfiles(uid_t uid, pid_t pid, const std::vector<std::string>& profiles) {
+bool SetProcessProfiles(uid_t uid, pid_t pid, const std::vector<std::string>& profiles,
+                        bool use_fd_cache) {
     const TaskProfiles& tp = TaskProfiles::GetInstance();
 
     for (const auto& name : profiles) {
-        const TaskProfile* profile = tp.GetProfile(name);
+        TaskProfile* profile = tp.GetProfile(name);
         if (profile != nullptr) {
+            if (use_fd_cache) {
+                profile->EnableResourceCaching();
+            }
             if (!profile->ExecuteForProcess(uid, pid)) {
                 PLOG(WARNING) << "Failed to apply " << name << " process profile";
             }
@@ -129,12 +133,15 @@ bool SetProcessProfiles(uid_t uid, pid_t pid, const std::vector<std::string>& pr
     return true;
 }
 
-bool SetTaskProfiles(int tid, const std::vector<std::string>& profiles) {
+bool SetTaskProfiles(int tid, const std::vector<std::string>& profiles, bool use_fd_cache) {
     const TaskProfiles& tp = TaskProfiles::GetInstance();
 
     for (const auto& name : profiles) {
-        const TaskProfile* profile = tp.GetProfile(name);
+        TaskProfile* profile = tp.GetProfile(name);
         if (profile != nullptr) {
+            if (use_fd_cache) {
+                profile->EnableResourceCaching();
+            }
             if (!profile->ExecuteForTask(tid)) {
                 PLOG(WARNING) << "Failed to apply " << name << " task profile";
             }
diff --git a/libprocessgroup/sched_policy.cpp b/libprocessgroup/sched_policy.cpp
index ab0f1ca35..fe4f93b1e 100644
--- a/libprocessgroup/sched_policy.cpp
+++ b/libprocessgroup/sched_policy.cpp
@@ -46,26 +46,34 @@ int set_cpuset_policy(int tid, SchedPolicy policy) {
 
     switch (policy) {
         case SP_BACKGROUND:
-            return SetTaskProfiles(tid, {"HighEnergySaving", "ProcessCapacityLow", "LowIoPriority",
-                                         "TimerSlackHigh"})
+            return SetTaskProfiles(tid,
+                                   {"HighEnergySaving", "ProcessCapacityLow", "LowIoPriority",
+                                    "TimerSlackHigh"},
+                                   true)
                            ? 0
                            : -1;
         case SP_FOREGROUND:
         case SP_AUDIO_APP:
         case SP_AUDIO_SYS:
-            return SetTaskProfiles(tid, {"HighPerformance", "ProcessCapacityHigh", "HighIoPriority",
-                                         "TimerSlackNormal"})
+            return SetTaskProfiles(tid,
+                                   {"HighPerformance", "ProcessCapacityHigh", "HighIoPriority",
+                                    "TimerSlackNormal"},
+                                   true)
                            ? 0
                            : -1;
         case SP_TOP_APP:
-            return SetTaskProfiles(tid, {"MaxPerformance", "ProcessCapacityMax", "MaxIoPriority",
-                                         "TimerSlackNormal"})
+            return SetTaskProfiles(tid,
+                                   {"MaxPerformance", "ProcessCapacityMax", "MaxIoPriority",
+                                    "TimerSlackNormal"},
+                                   true)
                            ? 0
                            : -1;
         case SP_SYSTEM:
-            return SetTaskProfiles(tid, {"ServiceCapacityLow", "TimerSlackNormal"}) ? 0 : -1;
+            return SetTaskProfiles(tid, {"ServiceCapacityLow", "TimerSlackNormal"}, true) ? 0 : -1;
         case SP_RESTRICTED:
-            return SetTaskProfiles(tid, {"ServiceCapacityRestricted", "TimerSlackNormal"}) ? 0 : -1;
+            return SetTaskProfiles(tid, {"ServiceCapacityRestricted", "TimerSlackNormal"}, true)
+                           ? 0
+                           : -1;
         default:
             break;
     }
@@ -126,17 +134,17 @@ int set_sched_policy(int tid, SchedPolicy policy) {
 
     switch (policy) {
         case SP_BACKGROUND:
-            return SetTaskProfiles(tid, {"HighEnergySaving", "TimerSlackHigh"}) ? 0 : -1;
+            return SetTaskProfiles(tid, {"HighEnergySaving", "TimerSlackHigh"}, true) ? 0 : -1;
         case SP_FOREGROUND:
         case SP_AUDIO_APP:
         case SP_AUDIO_SYS:
-            return SetTaskProfiles(tid, {"HighPerformance", "TimerSlackNormal"}) ? 0 : -1;
+            return SetTaskProfiles(tid, {"HighPerformance", "TimerSlackNormal"}, true) ? 0 : -1;
         case SP_TOP_APP:
-            return SetTaskProfiles(tid, {"MaxPerformance", "TimerSlackNormal"}) ? 0 : -1;
+            return SetTaskProfiles(tid, {"MaxPerformance", "TimerSlackNormal"}, true) ? 0 : -1;
         case SP_RT_APP:
-            return SetTaskProfiles(tid, {"RealtimePerformance", "TimerSlackNormal"}) ? 0 : -1;
+            return SetTaskProfiles(tid, {"RealtimePerformance", "TimerSlackNormal"}, true) ? 0 : -1;
         default:
-            return SetTaskProfiles(tid, {"TimerSlackNormal"}) ? 0 : -1;
+            return SetTaskProfiles(tid, {"TimerSlackNormal"}, true) ? 0 : -1;
     }
 
     return 0;
diff --git a/libprocessgroup/task_profiles.cpp b/libprocessgroup/task_profiles.cpp
index 4b45c8765..40d8d902c 100644
--- a/libprocessgroup/task_profiles.cpp
+++ b/libprocessgroup/task_profiles.cpp
@@ -138,31 +138,38 @@ bool SetCgroupAction::IsAppDependentPath(const std::string& path) {
 
 SetCgroupAction::SetCgroupAction(const CgroupController& c, const std::string& p)
     : controller_(c), path_(p) {
-#ifdef CACHE_FILE_DESCRIPTORS
-    // cache file descriptor only if path is app independent
+    // file descriptors for app-dependent paths can't be cached
     if (IsAppDependentPath(path_)) {
         // file descriptor is not cached
-        fd_.reset(-2);
+        fd_.reset(FDS_APP_DEPENDENT);
         return;
     }
 
-    std::string tasks_path = c.GetTasksFilePath(p);
+    // file descriptor can be cached later on request
+    fd_.reset(FDS_NOT_CACHED);
+}
+
+void SetCgroupAction::EnableResourceCaching() {
+    if (fd_ != FDS_NOT_CACHED) {
+        return;
+    }
+
+    std::string tasks_path = controller_.GetTasksFilePath(path_);
 
     if (access(tasks_path.c_str(), W_OK) != 0) {
         // file is not accessible
-        fd_.reset(-1);
+        fd_.reset(FDS_INACCESSIBLE);
         return;
     }
 
     unique_fd fd(TEMP_FAILURE_RETRY(open(tasks_path.c_str(), O_WRONLY | O_CLOEXEC)));
     if (fd < 0) {
         PLOG(ERROR) << "Failed to cache fd '" << tasks_path << "'";
-        fd_.reset(-1);
+        fd_.reset(FDS_INACCESSIBLE);
         return;
     }
 
     fd_ = std::move(fd);
-#endif
 }
 
 bool SetCgroupAction::AddTidToCgroup(int tid, int fd) {
@@ -184,8 +191,7 @@ bool SetCgroupAction::AddTidToCgroup(int tid, int fd) {
 }
 
 bool SetCgroupAction::ExecuteForProcess(uid_t uid, pid_t pid) const {
-#ifdef CACHE_FILE_DESCRIPTORS
-    if (fd_ >= 0) {
+    if (IsFdValid()) {
         // fd is cached, reuse it
         if (!AddTidToCgroup(pid, fd_)) {
             LOG(ERROR) << "Failed to add task into cgroup";
@@ -194,12 +200,12 @@ bool SetCgroupAction::ExecuteForProcess(uid_t uid, pid_t pid) const {
         return true;
     }
 
-    if (fd_ == -1) {
+    if (fd_ == FDS_INACCESSIBLE) {
         // no permissions to access the file, ignore
         return true;
     }
 
-    // this is app-dependent path, file descriptor is not cached
+    // this is app-dependent path and fd is not cached or cached fd can't be used
     std::string procs_path = controller()->GetProcsFilePath(path_, uid, pid);
     unique_fd tmp_fd(TEMP_FAILURE_RETRY(open(procs_path.c_str(), O_WRONLY | O_CLOEXEC)));
     if (tmp_fd < 0) {
@@ -212,25 +218,10 @@ bool SetCgroupAction::ExecuteForProcess(uid_t uid, pid_t pid) const {
     }
 
     return true;
-#else
-    std::string procs_path = controller()->GetProcsFilePath(path_, uid, pid);
-    unique_fd tmp_fd(TEMP_FAILURE_RETRY(open(procs_path.c_str(), O_WRONLY | O_CLOEXEC)));
-    if (tmp_fd < 0) {
-        // no permissions to access the file, ignore
-        return true;
-    }
-    if (!AddTidToCgroup(pid, tmp_fd)) {
-        LOG(ERROR) << "Failed to add task into cgroup";
-        return false;
-    }
-
-    return true;
-#endif
 }
 
 bool SetCgroupAction::ExecuteForTask(int tid) const {
-#ifdef CACHE_FILE_DESCRIPTORS
-    if (fd_ >= 0) {
+    if (IsFdValid()) {
         // fd is cached, reuse it
         if (!AddTidToCgroup(tid, fd_)) {
             LOG(ERROR) << "Failed to add task into cgroup";
@@ -239,20 +230,23 @@ bool SetCgroupAction::ExecuteForTask(int tid) const {
         return true;
     }
 
-    if (fd_ == -1) {
+    if (fd_ == FDS_INACCESSIBLE) {
         // no permissions to access the file, ignore
         return true;
     }
 
-    // application-dependent path can't be used with tid
-    LOG(ERROR) << "Application profile can't be applied to a thread";
-    return false;
-#else
+    if (fd_ == FDS_APP_DEPENDENT) {
+        // application-dependent path can't be used with tid
+        PLOG(ERROR) << "Application profile can't be applied to a thread";
+        return false;
+    }
+
+    // fd was not cached because cached fd can't be used
     std::string tasks_path = controller()->GetTasksFilePath(path_);
     unique_fd tmp_fd(TEMP_FAILURE_RETRY(open(tasks_path.c_str(), O_WRONLY | O_CLOEXEC)));
     if (tmp_fd < 0) {
-        // no permissions to access the file, ignore
-        return true;
+        PLOG(WARNING) << "Failed to open " << tasks_path << ": " << strerror(errno);
+        return false;
     }
     if (!AddTidToCgroup(tid, tmp_fd)) {
         LOG(ERROR) << "Failed to add task into cgroup";
@@ -260,7 +254,6 @@ bool SetCgroupAction::ExecuteForTask(int tid) const {
     }
 
     return true;
-#endif
 }
 
 bool TaskProfile::ExecuteForProcess(uid_t uid, pid_t pid) const {
@@ -284,6 +277,18 @@ bool TaskProfile::ExecuteForTask(int tid) const {
     return true;
 }
 
+void TaskProfile::EnableResourceCaching() {
+    if (res_cached_) {
+        return;
+    }
+
+    for (auto& element : elements_) {
+        element->EnableResourceCaching();
+    }
+
+    res_cached_ = true;
+}
+
 TaskProfiles& TaskProfiles::GetInstance() {
     // Deliberately leak this object to avoid a race between destruction on
     // process exit and concurrent access from another thread.
@@ -411,7 +416,7 @@ bool TaskProfiles::Load(const CgroupMap& cg_map, const std::string& file_name) {
     return true;
 }
 
-const TaskProfile* TaskProfiles::GetProfile(const std::string& name) const {
+TaskProfile* TaskProfiles::GetProfile(const std::string& name) const {
     auto iter = profiles_.find(name);
 
     if (iter != profiles_.end()) {
diff --git a/libprocessgroup/task_profiles.h b/libprocessgroup/task_profiles.h
index 37cc305d5..445647dea 100644
--- a/libprocessgroup/task_profiles.h
+++ b/libprocessgroup/task_profiles.h
@@ -48,6 +48,8 @@ class ProfileAction {
     // Default implementations will fail
     virtual bool ExecuteForProcess(uid_t, pid_t) const { return false; };
     virtual bool ExecuteForTask(int) const { return false; };
+
+    virtual void EnableResourceCaching() {}
 };
 
 // Profile actions
@@ -110,31 +112,40 @@ class SetCgroupAction : public ProfileAction {
 
     virtual bool ExecuteForProcess(uid_t uid, pid_t pid) const;
     virtual bool ExecuteForTask(int tid) const;
+    virtual void EnableResourceCaching();
 
     const CgroupController* controller() const { return &controller_; }
     std::string path() const { return path_; }
 
   private:
+    enum FdState {
+        FDS_INACCESSIBLE = -1,
+        FDS_APP_DEPENDENT = -2,
+        FDS_NOT_CACHED = -3,
+    };
+
     CgroupController controller_;
     std::string path_;
-#ifdef CACHE_FILE_DESCRIPTORS
     android::base::unique_fd fd_;
-#endif
 
     static bool IsAppDependentPath(const std::string& path);
     static bool AddTidToCgroup(int tid, int fd);
+
+    bool IsFdValid() const { return fd_ > FDS_INACCESSIBLE; }
 };
 
 class TaskProfile {
   public:
-    TaskProfile() {}
+    TaskProfile() : res_cached_(false) {}
 
     void Add(std::unique_ptr<ProfileAction> e) { elements_.push_back(std::move(e)); }
 
     bool ExecuteForProcess(uid_t uid, pid_t pid) const;
     bool ExecuteForTask(int tid) const;
+    void EnableResourceCaching();
 
   private:
+    bool res_cached_;
     std::vector<std::unique_ptr<ProfileAction>> elements_;
 };
 
@@ -143,7 +154,7 @@ class TaskProfiles {
     // Should be used by all users
     static TaskProfiles& GetInstance();
 
-    const TaskProfile* GetProfile(const std::string& name) const;
+    TaskProfile* GetProfile(const std::string& name) const;
     const ProfileAttribute* GetAttribute(const std::string& name) const;
 
   private: