From 949f10d7ef21b60a93b1fa386613c9ff4f56129e Mon Sep 17 00:00:00 2001 From: Suren Baghdasaryan Date: Thu, 14 Feb 2019 14:40:41 -0800 Subject: [PATCH 1/2] Re-enable file descriptor caching and add option to skip caching This reverts commit bee9f5718bd2dfedb615767bbd25147b4f3eed15 "libprocessgroup: Disable file descriptor caching temporarily" and adds option to use SetTaskProfiles and SetProcessProfiles without file caching. This option is used from JNI to avoid access denials because cached files are not whitelisted for JNI usage. Bug: 123868658 Bug: 123043091 Test: boot using svelte target Change-Id: I76b9d6af8a1dd4464cb3cf3e6dc327980efdf361 Signed-off-by: Suren Baghdasaryan --- .../include/processgroup/processgroup.h | 5 +- libprocessgroup/processgroup.cpp | 15 +++- libprocessgroup/sched_policy.cpp | 34 ++++---- libprocessgroup/task_profiles.cpp | 77 ++++++++++--------- libprocessgroup/task_profiles.h | 19 ++++- 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& profiles); -bool SetProcessProfiles(uid_t uid, pid_t pid, const std::vector& profiles); +bool SetTaskProfiles(int tid, const std::vector& profiles, bool use_fd_cache = false); +bool SetProcessProfiles(uid_t uid, pid_t pid, const std::vector& 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& profiles) { +bool SetProcessProfiles(uid_t uid, pid_t pid, const std::vector& 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& pr return true; } -bool SetTaskProfiles(int tid, const std::vector& profiles) { +bool SetTaskProfiles(int tid, const std::vector& 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 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> 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: From 41616d88a2d951c226777574829ff45e28212b88 Mon Sep 17 00:00:00 2001 From: Suren Baghdasaryan Date: Mon, 22 Apr 2019 13:12:13 -0700 Subject: [PATCH 2/2] libprocessgroup: limit libprocessgroup's VNDK API surface Limit libprocessgroup VNDK API to the minimum set required for task profiles usage. This API allows vendors to use cgroups without accessing cgroup files directly, therefore allowing Android to change cgroup arrangement details without breaking vendor code. Bug: 131098932 Test: build and boot Change-Id: I92463dfb44a108a133bafd2fe52237b6b1d50a69 Signed-off-by: Suren Baghdasaryan --- libprocessgroup/include/processgroup/processgroup.h | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/libprocessgroup/include/processgroup/processgroup.h b/libprocessgroup/include/processgroup/processgroup.h index 8d150adb0..7e6bf45cf 100644 --- a/libprocessgroup/include/processgroup/processgroup.h +++ b/libprocessgroup/include/processgroup/processgroup.h @@ -24,18 +24,21 @@ __BEGIN_DECLS static constexpr const char* CGROUPV2_CONTROLLER_NAME = "cgroup2"; -static constexpr const char* CGROUPS_RC_PATH = "/dev/cgroup_info/cgroup.rc"; bool CgroupGetControllerPath(const std::string& cgroup_name, std::string* path); bool CgroupGetAttributePath(const std::string& attr_name, std::string* path); bool CgroupGetAttributePathForTask(const std::string& attr_name, int tid, std::string* path); -bool UsePerAppMemcg(); - bool SetTaskProfiles(int tid, const std::vector& profiles, bool use_fd_cache = false); bool SetProcessProfiles(uid_t uid, pid_t pid, const std::vector& profiles, bool use_fd_cache = false); +#ifndef __ANDROID_VNDK__ + +static constexpr const char* CGROUPS_RC_PATH = "/dev/cgroup_info/cgroup.rc"; + +bool UsePerAppMemcg(); + // 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 // even after retrying for up to 200ms. @@ -55,4 +58,6 @@ bool setProcessGroupLimit(uid_t uid, int initialPid, int64_t limitInBytes); void removeAllProcessGroups(void); +#endif // __ANDROID_VNDK__ + __END_DECLS