From 44eb705480fe7b134d6178746d5430ced247bc74 Mon Sep 17 00:00:00 2001 From: "T.J. Mercier" Date: Wed, 27 Mar 2024 23:40:43 +0000 Subject: [PATCH 1/4] Fix unused params and remove unneeded cflags We already get -Wall and -Werror from the build system, and we do not want/need -Wexit-time-destructors since it prevents local statics with non-trivial destructors. Test: m Change-Id: I8283bf223404d6c253861d3888c1b720c099386e --- libprocessgroup/Android.bp | 6 ------ libprocessgroup/task_profiles.h | 8 ++++---- libprocessgroup/task_profiles_test.cpp | 7 +++---- 3 files changed, 7 insertions(+), 14 deletions(-) diff --git a/libprocessgroup/Android.bp b/libprocessgroup/Android.bp index c6a073789..ca2e0ec7f 100644 --- a/libprocessgroup/Android.bp +++ b/libprocessgroup/Android.bp @@ -5,12 +5,6 @@ package { cc_defaults { name: "libprocessgroup_defaults", cpp_std: "gnu++20", - cflags: [ - "-Wall", - "-Werror", - "-Wexit-time-destructors", - "-Wno-unused-parameter", - ], } cc_library_headers { diff --git a/libprocessgroup/task_profiles.h b/libprocessgroup/task_profiles.h index 2fa193177..428447106 100644 --- a/libprocessgroup/task_profiles.h +++ b/libprocessgroup/task_profiles.h @@ -82,8 +82,8 @@ class ProfileAction { virtual void EnableResourceCaching(ResourceCacheType) {} virtual void DropResourceCaching(ResourceCacheType) {} - virtual bool IsValidForProcess(uid_t uid, pid_t pid) const { return false; } - virtual bool IsValidForTask(pid_t tid) const { return false; } + virtual bool IsValidForProcess(uid_t, pid_t) const { return false; } + virtual bool IsValidForTask(pid_t) const { return false; } protected: enum CacheUseResult { SUCCESS, FAIL, UNUSED }; @@ -109,8 +109,8 @@ class SetTimerSlackAction : public ProfileAction { const char* Name() const override { return "SetTimerSlack"; } bool ExecuteForTask(pid_t tid) const override; - bool IsValidForProcess(uid_t uid, pid_t pid) const override { return true; } - bool IsValidForTask(pid_t tid) const override { return true; } + bool IsValidForProcess(uid_t, pid_t) const override { return true; } + bool IsValidForTask(pid_t) const override { return true; } private: unsigned long slack_; diff --git a/libprocessgroup/task_profiles_test.cpp b/libprocessgroup/task_profiles_test.cpp index b17e695e5..d19da2b54 100644 --- a/libprocessgroup/task_profiles_test.cpp +++ b/libprocessgroup/task_profiles_test.cpp @@ -102,8 +102,7 @@ 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, - const std::string& file_v2_name) override { + void Reset(const CgroupController&, const std::string&, const std::string&) override { CHECK(false); } const CgroupController* controller() const override { @@ -111,10 +110,10 @@ 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 { + bool GetPathForProcess(uid_t, pid_t pid, std::string* path) const override { return GetPathForTask(pid, path); } - bool GetPathForTask(int tid, std::string* path) const override { + bool GetPathForTask(int, std::string* path) const override { #ifdef __ANDROID__ CHECK(CgroupGetControllerPath(CGROUPV2_HIERARCHY_NAME, path)); CHECK_GT(path->length(), 0); From d1e048f95618b6fb7f66c81e1c650482d95cd452 Mon Sep 17 00:00:00 2001 From: "T.J. Mercier" Date: Thu, 28 Mar 2024 00:33:44 +0000 Subject: [PATCH 2/4] Use ConvertUid{Pid}ToPath for all path generation Consolidate into a single implementation. Test: m Change-Id: I0fc52db2d4b2973a74bad24c0a5f77384a559cee --- libprocessgroup/processgroup.cpp | 8 -------- libprocessgroup/task_profiles.cpp | 18 ++++++++++++++---- libprocessgroup/task_profiles.h | 3 +++ 3 files changed, 17 insertions(+), 12 deletions(-) diff --git a/libprocessgroup/processgroup.cpp b/libprocessgroup/processgroup.cpp index 94d950209..8df2805d9 100644 --- a/libprocessgroup/processgroup.cpp +++ b/libprocessgroup/processgroup.cpp @@ -78,14 +78,6 @@ bool CgroupGetControllerPath(const std::string& cgroup_name, std::string* path) return true; } -static std::string ConvertUidToPath(const char* cgroup, uid_t uid) { - return StringPrintf("%s/uid_%u", cgroup, uid); -} - -static std::string ConvertUidPidToPath(const char* cgroup, uid_t uid, pid_t pid) { - return StringPrintf("%s/uid_%u/pid_%d", cgroup, uid, pid); -} - static bool CgroupKillAvailable() { static std::once_flag f; static bool cgroup_kill_available = false; diff --git a/libprocessgroup/task_profiles.cpp b/libprocessgroup/task_profiles.cpp index 2353cf18a..c376a0fe6 100644 --- a/libprocessgroup/task_profiles.cpp +++ b/libprocessgroup/task_profiles.cpp @@ -126,11 +126,19 @@ void ProfileAttribute::Reset(const CgroupController& controller, const std::stri file_v2_name_ = file_v2_name; } +std::string ConvertUidToPath(const char* root_cgroup_path, uid_t uid) { + return StringPrintf("%s/uid_%u", root_cgroup_path, uid); +} + +std::string ConvertUidPidToPath(const char* root_cgroup_path, uid_t uid, pid_t pid) { + const std::string uid_path = ConvertUidToPath(root_cgroup_path, uid); + return StringPrintf("%s/pid_%d", uid_path.c_str(), pid); +} + 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()); + const std::string cgroup_path = ConvertUidPidToPath(controller()->path(), uid, pid); + *path = cgroup_path + "/" + file_name(); return true; } return GetPathForTask(pid, path); @@ -155,12 +163,14 @@ bool ProfileAttribute::GetPathForTask(pid_t tid, std::string* path) const { return true; } +// NOTE: This function is for cgroup v2 only bool ProfileAttribute::GetPathForUID(uid_t uid, std::string* path) const { if (path == nullptr) { return true; } - *path = StringPrintf("%s/uid_%u/%s", controller()->path(), uid, file_name().c_str()); + const std::string cgroup_path = ConvertUidToPath(controller()->path(), uid); + *path = cgroup_path + "/" + file_name(); return true; } diff --git a/libprocessgroup/task_profiles.h b/libprocessgroup/task_profiles.h index 428447106..7e3c50d9f 100644 --- a/libprocessgroup/task_profiles.h +++ b/libprocessgroup/task_profiles.h @@ -252,3 +252,6 @@ class TaskProfiles { std::map, std::less<>> profiles_; std::map, std::less<>> attributes_; }; + +std::string ConvertUidToPath(const char* root_cgroup_path, uid_t uid); +std::string ConvertUidPidToPath(const char* root_cgroup_path, uid_t uid, pid_t pid); From f8901767e63636d1fa884255cf9bfe0c7ab368b9 Mon Sep 17 00:00:00 2001 From: "T.J. Mercier" Date: Tue, 5 Mar 2024 21:55:15 +0000 Subject: [PATCH 3/4] Add build flag to force memcg to the v2 cgroup hierarchy This flag prevents memcg from being mounted as a v1 hierarchy, even if cgroups.json specifies it should be mounted as v1. It will activate memcg in the v2 hierarchy even if cgroups.json does not specify that it should be activated in the v2 hierarchy. The cgroup_disable=memory kernel command line argument will still prevent memcg from being activated, as this forced memcg controller is marked as optional. Bug: 327480673 Change-Id: Iad2491dd0c1576156ee2346928d041d85af890f0 --- libprocessgroup/Android.bp | 27 ++++++-- libprocessgroup/build_flags.h | 29 +++++++++ libprocessgroup/setup/Android.bp | 5 +- libprocessgroup/setup/cgroup_map_write.cpp | 76 +++++++++++++++++++++- 4 files changed, 126 insertions(+), 11 deletions(-) create mode 100644 libprocessgroup/build_flags.h diff --git a/libprocessgroup/Android.bp b/libprocessgroup/Android.bp index ca2e0ec7f..f529ad76d 100644 --- a/libprocessgroup/Android.bp +++ b/libprocessgroup/Android.bp @@ -2,9 +2,28 @@ package { default_applicable_licenses: ["Android-Apache-2.0"], } -cc_defaults { - name: "libprocessgroup_defaults", +soong_config_module_type { + name: "libprocessgroup_flag_aware_cc_defaults", + module_type: "cc_defaults", + config_namespace: "ANDROID", + bool_variables: [ + "memcg_v2_force_enabled", + ], + properties: [ + "cflags", + ], +} + +libprocessgroup_flag_aware_cc_defaults { + name: "libprocessgroup_build_flags_cc", cpp_std: "gnu++20", + soong_config_variables: { + memcg_v2_force_enabled: { + cflags: [ + "-DMEMCG_V2_FORCE_ENABLED=true", + ], + }, + }, } cc_library_headers { @@ -67,7 +86,7 @@ cc_library { export_header_lib_headers: [ "libprocessgroup_headers", ], - defaults: ["libprocessgroup_defaults"], + defaults: ["libprocessgroup_build_flags_cc"], apex_available: [ "//apex_available:platform", "//apex_available:anyapex", @@ -78,7 +97,7 @@ cc_library { cc_test { name: "task_profiles_test", host_supported: true, - defaults: ["libprocessgroup_defaults"], + defaults: ["libprocessgroup_build_flags_cc"], srcs: [ "task_profiles_test.cpp", ], diff --git a/libprocessgroup/build_flags.h b/libprocessgroup/build_flags.h new file mode 100644 index 000000000..69fa76e1b --- /dev/null +++ b/libprocessgroup/build_flags.h @@ -0,0 +1,29 @@ +/* + * Copyright (C) 2024 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#pragma once + +#ifndef MEMCG_V2_FORCE_ENABLED +#define MEMCG_V2_FORCE_ENABLED false +#endif + +namespace android::libprocessgroup_flags { + +inline consteval bool force_memcg_v2() { + return MEMCG_V2_FORCE_ENABLED; +} + +} // namespace android::libprocessgroup_flags diff --git a/libprocessgroup/setup/Android.bp b/libprocessgroup/setup/Android.bp index ea6c24706..1e0783ab0 100644 --- a/libprocessgroup/setup/Android.bp +++ b/libprocessgroup/setup/Android.bp @@ -41,8 +41,5 @@ cc_library_shared { export_header_lib_headers: [ "libprocessgroup_headers", ], - cflags: [ - "-Wall", - "-Werror", - ], + defaults: ["libprocessgroup_build_flags_cc"], } diff --git a/libprocessgroup/setup/cgroup_map_write.cpp b/libprocessgroup/setup/cgroup_map_write.cpp index 4e44c91be..a9fd0e58b 100644 --- a/libprocessgroup/setup/cgroup_map_write.cpp +++ b/libprocessgroup/setup/cgroup_map_write.cpp @@ -29,7 +29,7 @@ #include #include -#include +#include #include #include @@ -43,6 +43,7 @@ #include #include +#include "../build_flags.h" #include "cgroup_descriptor.h" using android::base::GetUintProperty; @@ -57,6 +58,8 @@ static constexpr const char* CGROUPS_DESC_VENDOR_FILE = "/vendor/etc/cgroups.jso static constexpr const char* TEMPLATE_CGROUPS_DESC_API_FILE = "/etc/task_profiles/cgroups_%u.json"; +static const std::string CGROUP_V2_ROOT_DEFAULT = "/sys/fs/cgroup"; + static bool ChangeDirModeAndOwner(const std::string& path, mode_t mode, const std::string& uid, const std::string& gid, bool permissive_mode = false) { uid_t pw_uid = -1; @@ -182,6 +185,8 @@ static void MergeCgroupToDescriptors(std::map* de } } +static const bool force_memcg_v2 = android::libprocessgroup_flags::force_memcg_v2(); + static bool ReadDescriptorsFromFile(const std::string& file_name, std::map* descriptors) { std::vector result; @@ -205,22 +210,41 @@ static bool ReadDescriptorsFromFile(const std::string& file_name, const Json::Value& cgroups = root["Cgroups"]; for (Json::Value::ArrayIndex i = 0; i < cgroups.size(); ++i) { std::string name = cgroups[i]["Controller"].asString(); + + if (force_memcg_v2 && name == "memory") continue; + MergeCgroupToDescriptors(descriptors, cgroups[i], name, "", 1); } } + bool memcgv2_present = false; + std::string root_path; if (root.isMember("Cgroups2")) { const Json::Value& cgroups2 = root["Cgroups2"]; - std::string root_path = cgroups2["Path"].asString(); + root_path = cgroups2["Path"].asString(); MergeCgroupToDescriptors(descriptors, cgroups2, CGROUPV2_HIERARCHY_NAME, "", 2); const Json::Value& childGroups = cgroups2["Controllers"]; for (Json::Value::ArrayIndex i = 0; i < childGroups.size(); ++i) { std::string name = childGroups[i]["Controller"].asString(); + + if (force_memcg_v2 && name == "memory") memcgv2_present = true; + MergeCgroupToDescriptors(descriptors, childGroups[i], name, root_path, 2); } } + if (force_memcg_v2 && !memcgv2_present) { + LOG(INFO) << "Forcing memcg to v2 hierarchy"; + Json::Value memcgv2; + memcgv2["Controller"] = "memory"; + memcgv2["NeedsActivation"] = true; + memcgv2["Path"] = "."; + memcgv2["Optional"] = true; // In case of cgroup_disabled=memory, so we can still boot + MergeCgroupToDescriptors(descriptors, memcgv2, "memory", + root_path.empty() ? CGROUP_V2_ROOT_DEFAULT : root_path, 2); + } + return true; } @@ -308,7 +332,8 @@ static bool ActivateV2CgroupController(const CgroupDescriptor& descriptor) { if (!base::WriteStringToFile(str, path)) { if (IsOptionalController(controller)) { - PLOG(INFO) << "Failed to activate optional controller " << controller->name(); + PLOG(INFO) << "Failed to activate optional controller " << controller->name() + << " at " << path; return true; } PLOG(ERROR) << "Failed to activate controller " << controller->name(); @@ -424,6 +449,40 @@ void CgroupDescriptor::set_mounted(bool mounted) { } // namespace cgrouprc } // namespace android +static std::optional MGLRUDisabled() { + const std::string file_name = "/sys/kernel/mm/lru_gen/enabled"; + std::string content; + if (!android::base::ReadFileToString(file_name, &content)) { + PLOG(ERROR) << "Failed to read MGLRU state from " << file_name; + return {}; + } + + return content == "0x0000"; +} + +static std::optional MEMCGDisabled( + const std::map& descriptors) { + std::string cgroup_v2_root = android::cgrouprc::CGROUP_V2_ROOT_DEFAULT; + const auto it = descriptors.find(CGROUPV2_HIERARCHY_NAME); + if (it == descriptors.end()) { + LOG(WARNING) << "No Cgroups2 path found in cgroups.json. Vendor has modified Android, and " + << "kernel memory use will be higher than intended."; + } else if (it->second.controller()->path() != cgroup_v2_root) { + cgroup_v2_root = it->second.controller()->path(); + } + + const std::string file_name = cgroup_v2_root + "/cgroup.controllers"; + std::string content; + if (!android::base::ReadFileToString(file_name, &content)) { + PLOG(ERROR) << "Failed to read cgroup controllers from " << file_name; + return {}; + } + + // If we've forced memcg to v2 and it's not available, then it could only have been disabled + // on the kernel command line (GKI sets CONFIG_MEMCG). + return content.find("memory") == std::string::npos; +} + bool CgroupSetup() { using namespace android::cgrouprc; @@ -457,6 +516,17 @@ bool CgroupSetup() { } } + if (force_memcg_v2) { + if (MGLRUDisabled().value_or(false)) { + LOG(WARNING) << "Memcg forced to v2 hierarchy with MGLRU disabled! " + << "Global reclaim performance will suffer."; + } + if (MEMCGDisabled(descriptors).value_or(false)) { + LOG(WARNING) << "Memcg forced to v2 hierarchy while memcg is disabled by kernel " + << "command line!"; + } + } + // mkdir 0711 system system if (!Mkdir(android::base::Dirname(CGROUPS_RC_PATH), 0711, "system", "system")) { LOG(ERROR) << "Failed to create directory for " << CGROUPS_RC_PATH << " file"; From 1cfa2c4111f563c12b292bd066f651ddc8d1c0c7 Mon Sep 17 00:00:00 2001 From: "T.J. Mercier" Date: Mon, 8 Apr 2024 21:14:32 +0000 Subject: [PATCH 4/4] Add build flag to split the cgroup v2 hierarchy into apps/system This flag adds "apps" and "system" cgroups underneath the v2 hierarchy root. Cgroups with UIDs < 10000 (AID_APP_START) will be placed under "system" and others will be placed under "apps". UIDs under 10000 are reserved for core Android subsystems. This allows us to apply different cgroup controls collectively to system processes and normal applications. Bug: 327480673 Change-Id: I40837dee27a59691f81fef48e66a86c5eacda892 --- libprocessgroup/Android.bp | 6 +++ libprocessgroup/build_flags.h | 8 ++++ libprocessgroup/setup/cgroup_map_write.cpp | 51 ++++++++++++++++++++++ libprocessgroup/task_profiles.cpp | 12 +++++ 4 files changed, 77 insertions(+) diff --git a/libprocessgroup/Android.bp b/libprocessgroup/Android.bp index f529ad76d..bb855d58c 100644 --- a/libprocessgroup/Android.bp +++ b/libprocessgroup/Android.bp @@ -8,6 +8,7 @@ soong_config_module_type { config_namespace: "ANDROID", bool_variables: [ "memcg_v2_force_enabled", + "cgroup_v2_sys_app_isolation", ], properties: [ "cflags", @@ -23,6 +24,11 @@ libprocessgroup_flag_aware_cc_defaults { "-DMEMCG_V2_FORCE_ENABLED=true", ], }, + cgroup_v2_sys_app_isolation: { + cflags: [ + "-DCGROUP_V2_SYS_APP_ISOLATION=true", + ], + }, }, } diff --git a/libprocessgroup/build_flags.h b/libprocessgroup/build_flags.h index 69fa76e1b..bc3e7dff1 100644 --- a/libprocessgroup/build_flags.h +++ b/libprocessgroup/build_flags.h @@ -20,10 +20,18 @@ #define MEMCG_V2_FORCE_ENABLED false #endif +#ifndef CGROUP_V2_SYS_APP_ISOLATION +#define CGROUP_V2_SYS_APP_ISOLATION false +#endif + namespace android::libprocessgroup_flags { inline consteval bool force_memcg_v2() { return MEMCG_V2_FORCE_ENABLED; } +inline consteval bool cgroup_v2_sys_app_isolation() { + return CGROUP_V2_SYS_APP_ISOLATION; +} + } // namespace android::libprocessgroup_flags diff --git a/libprocessgroup/setup/cgroup_map_write.cpp b/libprocessgroup/setup/cgroup_map_write.cpp index a9fd0e58b..1b26fbcc6 100644 --- a/libprocessgroup/setup/cgroup_map_write.cpp +++ b/libprocessgroup/setup/cgroup_map_write.cpp @@ -483,6 +483,42 @@ static std::optional MEMCGDisabled( return content.find("memory") == std::string::npos; } +static bool CreateV2SubHierarchy( + const std::string& path, + const std::map& descriptors) { + using namespace android::cgrouprc; + + const auto cgv2_iter = descriptors.find(CGROUPV2_HIERARCHY_NAME); + if (cgv2_iter == descriptors.end()) return false; + const android::cgrouprc::CgroupDescriptor cgv2_descriptor = cgv2_iter->second; + + if (!Mkdir(path, cgv2_descriptor.mode(), cgv2_descriptor.uid(), cgv2_descriptor.gid())) { + PLOG(ERROR) << "Failed to create directory for " << path; + return false; + } + + // Activate all v2 controllers in path so they can be activated in + // children as they are created. + for (const auto& [name, descriptor] : descriptors) { + const format::CgroupController* controller = descriptor.controller(); + std::uint32_t flags = controller->flags(); + if (controller->version() == 2 && name != CGROUPV2_HIERARCHY_NAME && + flags & CGROUPRC_CONTROLLER_FLAG_NEEDS_ACTIVATION) { + std::string str("+"); + str += controller->name(); + if (!android::base::WriteStringToFile(str, path + "/cgroup.subtree_control")) { + if (flags & CGROUPRC_CONTROLLER_FLAG_OPTIONAL) { + PLOG(WARNING) << "Activation of cgroup controller " << str << " failed in path " + << path; + } else { + return false; + } + } + } + } + return true; +} + bool CgroupSetup() { using namespace android::cgrouprc; @@ -527,6 +563,21 @@ bool CgroupSetup() { } } + // System / app isolation. + // This really belongs in early-init in init.rc, but we cannot use the flag there. + if (android::libprocessgroup_flags::cgroup_v2_sys_app_isolation()) { + const auto it = descriptors.find(CGROUPV2_HIERARCHY_NAME); + const std::string cgroup_v2_root = (it == descriptors.end()) + ? CGROUP_V2_ROOT_DEFAULT + : it->second.controller()->path(); + + LOG(INFO) << "Using system/app isolation under: " << cgroup_v2_root; + if (!CreateV2SubHierarchy(cgroup_v2_root + "/apps", descriptors) || + !CreateV2SubHierarchy(cgroup_v2_root + "/system", descriptors)) { + return false; + } + } + // mkdir 0711 system system if (!Mkdir(android::base::Dirname(CGROUPS_RC_PATH), 0711, "system", "system")) { LOG(ERROR) << "Failed to create directory for " << CGROUPS_RC_PATH << " file"; diff --git a/libprocessgroup/task_profiles.cpp b/libprocessgroup/task_profiles.cpp index c376a0fe6..0c2252b17 100644 --- a/libprocessgroup/task_profiles.cpp +++ b/libprocessgroup/task_profiles.cpp @@ -33,6 +33,8 @@ #include #include +#include + // To avoid issues in sdk_mac build #if defined(__ANDROID__) #include @@ -126,7 +128,17 @@ void ProfileAttribute::Reset(const CgroupController& controller, const std::stri file_v2_name_ = file_v2_name; } +static bool isSystemApp(uid_t uid) { + return uid < AID_APP_START; +} + std::string ConvertUidToPath(const char* root_cgroup_path, uid_t uid) { + if (android::libprocessgroup_flags::cgroup_v2_sys_app_isolation()) { + if (isSystemApp(uid)) + return StringPrintf("%s/system/uid_%u", root_cgroup_path, uid); + else + return StringPrintf("%s/apps/uid_%u", root_cgroup_path, uid); + } return StringPrintf("%s/uid_%u", root_cgroup_path, uid); }