From 35078468722cdee94fe17e57fe5f300d3ee156f4 Mon Sep 17 00:00:00 2001 From: Suren Baghdasaryan Date: Tue, 25 Jul 2023 14:50:18 -0700 Subject: [PATCH 1/2] libprocessgroup: fix reset of file_v2_name ProfileAttribute::Reset does not reset file_v2_name, fix that. Also provide ProfileAttribute::file_name() to consolidate the code. Bug: 292636609 Signed-off-by: Suren Baghdasaryan Change-Id: I5b33ca47b4fa5cabf582c8804bd13f72f6e58411 --- libprocessgroup/task_profiles.cpp | 22 +++++++++++++--------- libprocessgroup/task_profiles.h | 8 +++++--- libprocessgroup/task_profiles_test.cpp | 7 +++---- 3 files changed, 21 insertions(+), 16 deletions(-) diff --git a/libprocessgroup/task_profiles.cpp b/libprocessgroup/task_profiles.cpp index 1276a72a6..59b135013 100644 --- a/libprocessgroup/task_profiles.cpp +++ b/libprocessgroup/task_profiles.cpp @@ -114,9 +114,16 @@ bool FdCacheHelper::IsAppDependentPath(const std::string& path) { IProfileAttribute::~IProfileAttribute() = default; -void ProfileAttribute::Reset(const CgroupController& controller, const std::string& file_name) { +const std::string& ProfileAttribute::file_name() const { + if (controller()->version() == 2 && !file_v2_name_.empty()) return file_v2_name_; + return file_name_; +} + +void ProfileAttribute::Reset(const CgroupController& controller, const std::string& file_name, + const std::string& file_v2_name) { controller_ = controller; file_name_ = file_name; + file_v2_name_ = file_v2_name; } bool ProfileAttribute::GetPathForTask(int tid, std::string* path) const { @@ -129,12 +136,11 @@ bool ProfileAttribute::GetPathForTask(int tid, std::string* path) const { return true; } - const std::string& file_name = - controller()->version() == 2 && !file_v2_name_.empty() ? file_v2_name_ : file_name_; if (subgroup.empty()) { - *path = StringPrintf("%s/%s", controller()->path(), file_name.c_str()); + *path = StringPrintf("%s/%s", controller()->path(), file_name().c_str()); } else { - *path = StringPrintf("%s/%s/%s", controller()->path(), subgroup.c_str(), file_name.c_str()); + *path = StringPrintf("%s/%s/%s", controller()->path(), subgroup.c_str(), + file_name().c_str()); } return true; } @@ -144,9 +150,7 @@ bool ProfileAttribute::GetPathForUID(uid_t uid, std::string* path) const { return true; } - const std::string& file_name = - controller()->version() == 2 && !file_v2_name_.empty() ? file_v2_name_ : file_name_; - *path = StringPrintf("%s/uid_%u/%s", controller()->path(), uid, file_name.c_str()); + *path = StringPrintf("%s/uid_%u/%s", controller()->path(), uid, file_name().c_str()); return true; } @@ -816,7 +820,7 @@ bool TaskProfiles::Load(const CgroupMap& cg_map, const std::string& file_name) { attributes_[name] = std::make_unique(controller, file_attr, file_v2_attr); } else { - iter->second->Reset(controller, file_attr); + iter->second->Reset(controller, file_attr, file_v2_attr); } } else { LOG(WARNING) << "Controller " << controller_name << " is not found"; diff --git a/libprocessgroup/task_profiles.h b/libprocessgroup/task_profiles.h index a62c5b0a9..ac8918e65 100644 --- a/libprocessgroup/task_profiles.h +++ b/libprocessgroup/task_profiles.h @@ -32,7 +32,8 @@ class IProfileAttribute { public: virtual ~IProfileAttribute() = 0; - virtual void Reset(const CgroupController& controller, const std::string& file_name) = 0; + virtual void Reset(const CgroupController& controller, const std::string& file_name, + const std::string& file_v2_name) = 0; virtual const CgroupController* controller() const = 0; virtual const std::string& file_name() const = 0; virtual bool GetPathForTask(int tid, std::string* path) const = 0; @@ -50,8 +51,9 @@ class ProfileAttribute : public IProfileAttribute { ~ProfileAttribute() = default; const CgroupController* controller() const override { return &controller_; } - const std::string& file_name() const override { return file_name_; } - void Reset(const CgroupController& controller, const std::string& file_name) override; + const std::string& file_name() const override; + void Reset(const CgroupController& controller, const std::string& file_name, + const std::string& file_v2_name) override; bool GetPathForTask(int tid, std::string* path) const override; bool GetPathForUID(uid_t uid, std::string* path) const override; diff --git a/libprocessgroup/task_profiles_test.cpp b/libprocessgroup/task_profiles_test.cpp index eadbe7697..da74bb012 100644 --- a/libprocessgroup/task_profiles_test.cpp +++ b/libprocessgroup/task_profiles_test.cpp @@ -102,7 +102,8 @@ class ProfileAttributeMock : public IProfileAttribute { public: ProfileAttributeMock(const std::string& file_name) : file_name_(file_name) {} ~ProfileAttributeMock() override = default; - void Reset(const CgroupController& controller, const std::string& file_name) override { + void Reset(const CgroupController& controller, const std::string& file_name, + const std::string& file_v2_name) override { CHECK(false); } const CgroupController* controller() const override { @@ -125,9 +126,7 @@ class ProfileAttributeMock : public IProfileAttribute { return true; }; - bool GetPathForUID(uid_t, std::string*) const override { - return false; - } + bool GetPathForUID(uid_t, std::string*) const override { return false; } private: const std::string file_name_; From 3483798fd9045f93e9095e5b09ffe4f59054c535 Mon Sep 17 00:00:00 2001 From: Suren Baghdasaryan Date: Tue, 25 Jul 2023 15:45:45 -0700 Subject: [PATCH 2/2] libprocessgroup: optimize SetAttributeAction::ExecuteForProcess performance Current implementation of SetAttributeAction::ExecuteForProcess reuses SetAttributeAction::ExecuteForTask while not utilizing available uid/pid information. This results in a call to GetPathForTask() which is an expensive function due to it reading and parsing /proc/$pid/cgroups. This can be avoided if we utilize available uid/pid info and the fact that cgroup v2 attributes share the cgroup v2 hierarchy as process groups, which use a known path template. Bug: 292636609 Change-Id: I02e3046bd85d0dfebc68ab444f1796bb54cc69c7 Signed-off-by: Suren Baghdasaryan --- libprocessgroup/task_profiles.cpp | 45 +++++++++++++++++++------- libprocessgroup/task_profiles.h | 4 +++ libprocessgroup/task_profiles_test.cpp | 3 ++ 3 files changed, 40 insertions(+), 12 deletions(-) diff --git a/libprocessgroup/task_profiles.cpp b/libprocessgroup/task_profiles.cpp index 59b135013..f51b07671 100644 --- a/libprocessgroup/task_profiles.cpp +++ b/libprocessgroup/task_profiles.cpp @@ -126,6 +126,16 @@ void ProfileAttribute::Reset(const CgroupController& controller, const std::stri file_v2_name_ = file_v2_name; } +bool ProfileAttribute::GetPathForProcess(uid_t uid, pid_t pid, std::string* path) const { + if (controller()->version() == 2) { + // all cgroup v2 attributes use the same process group hierarchy + *path = StringPrintf("%s/uid_%u/pid_%d/%s", controller()->path(), uid, pid, + file_name().c_str()); + return true; + } + return GetPathForTask(pid, path); +} + bool ProfileAttribute::GetPathForTask(int tid, std::string* path) const { std::string subgroup; if (!controller()->GetTaskGroup(tid, &subgroup)) { @@ -209,18 +219,7 @@ bool SetTimerSlackAction::ExecuteForTask(int) const { #endif -bool SetAttributeAction::ExecuteForProcess(uid_t, pid_t pid) const { - return ExecuteForTask(pid); -} - -bool SetAttributeAction::ExecuteForTask(int tid) const { - std::string path; - - if (!attribute_->GetPathForTask(tid, &path)) { - LOG(ERROR) << "Failed to find cgroup for tid " << tid; - return false; - } - +bool SetAttributeAction::WriteValueToFile(const std::string& path) const { if (!WriteStringToFile(value_, path)) { if (access(path.c_str(), F_OK) < 0) { if (optional_) { @@ -240,6 +239,28 @@ bool SetAttributeAction::ExecuteForTask(int tid) const { return true; } +bool SetAttributeAction::ExecuteForProcess(uid_t uid, pid_t pid) const { + std::string path; + + if (!attribute_->GetPathForProcess(uid, pid, &path)) { + LOG(ERROR) << "Failed to find cgroup for uid " << uid << " pid " << pid; + return false; + } + + return WriteValueToFile(path); +} + +bool SetAttributeAction::ExecuteForTask(int tid) const { + std::string path; + + if (!attribute_->GetPathForTask(tid, &path)) { + LOG(ERROR) << "Failed to find cgroup for tid " << tid; + return false; + } + + return WriteValueToFile(path); +} + bool SetAttributeAction::ExecuteForUID(uid_t uid) const { std::string path; diff --git a/libprocessgroup/task_profiles.h b/libprocessgroup/task_profiles.h index ac8918e65..4663f64e2 100644 --- a/libprocessgroup/task_profiles.h +++ b/libprocessgroup/task_profiles.h @@ -36,6 +36,7 @@ class IProfileAttribute { const std::string& file_v2_name) = 0; virtual const CgroupController* controller() const = 0; virtual const std::string& file_name() const = 0; + virtual bool GetPathForProcess(uid_t uid, pid_t pid, std::string* path) const = 0; virtual bool GetPathForTask(int tid, std::string* path) const = 0; virtual bool GetPathForUID(uid_t uid, std::string* path) const = 0; }; @@ -55,6 +56,7 @@ class ProfileAttribute : public IProfileAttribute { void Reset(const CgroupController& controller, const std::string& file_name, const std::string& file_v2_name) override; + bool GetPathForProcess(uid_t uid, pid_t pid, std::string* path) const override; bool GetPathForTask(int tid, std::string* path) const override; bool GetPathForUID(uid_t uid, std::string* path) const override; @@ -133,6 +135,8 @@ class SetAttributeAction : public ProfileAction { const IProfileAttribute* attribute_; std::string value_; bool optional_; + + bool WriteValueToFile(const std::string& path) const; }; // Set cgroup profile element diff --git a/libprocessgroup/task_profiles_test.cpp b/libprocessgroup/task_profiles_test.cpp index da74bb012..99d819a7c 100644 --- a/libprocessgroup/task_profiles_test.cpp +++ b/libprocessgroup/task_profiles_test.cpp @@ -111,6 +111,9 @@ class ProfileAttributeMock : public IProfileAttribute { return {}; } const std::string& file_name() const override { return file_name_; } + bool GetPathForProcess(uid_t uid, pid_t pid, std::string* path) const override { + return GetPathForTask(pid, path); + } bool GetPathForTask(int tid, std::string* path) const override { #ifdef __ANDROID__ CHECK(CgroupGetControllerPath(CGROUPV2_CONTROLLER_NAME, path));