From 1fdbf8d0f87d618deded242e9c22f1cebd9c7c74 Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Fri, 17 Feb 2023 15:34:49 -0800 Subject: [PATCH 1/6] init: Combine two if-statements Combine two if-statements. This change is fine because: * The code between the two if-statements does not queue actions. * If an action is queued from another thread then WakeMainInitThread() is called after the action has been queued. Bug: 266255006 Change-Id: Id4b9565ff4fdb3ee2a2bbca316c8c78e0f2d38dd Signed-off-by: Bart Van Assche --- init/init.cpp | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/init/init.cpp b/init/init.cpp index c965fe635..05a8a9cc5 100644 --- a/init/init.cpp +++ b/init/init.cpp @@ -1109,8 +1109,11 @@ int SecondStageMain(int argc, char** argv) { // Restore prio before main loop setpriority(PRIO_PROCESS, 0, 0); while (true) { - // By default, sleep until something happens. - std::optional epoll_timeout; + // By default, sleep until something happens. Do not convert far_future into + // std::chrono::milliseconds because that would trigger an overflow. The unit of boot_clock + // is 1ns. + const boot_clock::time_point far_future = boot_clock::time_point::max(); + boot_clock::time_point next_action_time = far_future; auto shutdown_command = shutdown_state.CheckShutdown(); if (shutdown_command) { @@ -1122,23 +1125,28 @@ int SecondStageMain(int argc, char** argv) { if (!(prop_waiter_state.MightBeWaiting() || Service::is_exec_service_running())) { am.ExecuteOneCommand(); + // If there's more work to do, wake up again immediately. + if (am.HasMoreCommands()) { + next_action_time = boot_clock::now(); + } } + // Since the above code examined pending actions, no new actions must be + // queued by the code between this line and the Epoll::Wait() call below + // without calling WakeMainInitThread(). if (!IsShuttingDown()) { auto next_process_action_time = HandleProcessActions(); // If there's a process that needs restarting, wake up in time for that. if (next_process_action_time) { - epoll_timeout = std::chrono::ceil( - *next_process_action_time - boot_clock::now()); - if (epoll_timeout < 0ms) epoll_timeout = 0ms; + next_action_time = std::min(next_action_time, *next_process_action_time); } } - if (!(prop_waiter_state.MightBeWaiting() || Service::is_exec_service_running())) { - // If there's more work to do, wake up again immediately. - if (am.HasMoreCommands()) epoll_timeout = 0ms; + std::optional epoll_timeout; + if (next_action_time != far_future) { + epoll_timeout = std::chrono::ceil( + std::max(next_action_time - boot_clock::now(), 0ns)); } - auto epoll_result = epoll.Wait(epoll_timeout); if (!epoll_result.ok()) { LOG(ERROR) << epoll_result.error(); From b4b1b75a35516bb27b854343d040aa9dcd2b436c Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Fri, 3 Mar 2023 13:21:19 -0800 Subject: [PATCH 2/6] init: Remove the DebugRebootLogging() function The DebugRebootLogging() function was introduced to help with root-causing b/150863651. Remove this function since this logging functionality is no longer needed. Also remove the functions and methods that are only used by DebugRebootLogging(). Change-Id: Ia150604c6cd70f42b13d655ba43b95445a55b6e2 Signed-off-by: Bart Van Assche --- init/init.cpp | 26 -------------------------- init/init.h | 2 -- init/property_service.cpp | 3 --- 3 files changed, 31 deletions(-) diff --git a/init/init.cpp b/init/init.cpp index 05a8a9cc5..a01ae8765 100644 --- a/init/init.cpp +++ b/init/init.cpp @@ -255,7 +255,6 @@ static class ShutdownState { return {}; } - bool do_shutdown() const { return do_shutdown_; } void set_do_shutdown(bool value) { do_shutdown_ = value; } private: @@ -264,31 +263,6 @@ static class ShutdownState { bool do_shutdown_ = false; } shutdown_state; -static void UnwindMainThreadStack() { - unwindstack::AndroidLocalUnwinder unwinder; - unwindstack::AndroidUnwinderData data; - if (!unwinder.Unwind(data)) { - LOG(ERROR) << __FUNCTION__ - << "sys.powerctl: Failed to unwind callstack: " << data.GetErrorString(); - } - for (const auto& frame : data.frames) { - LOG(ERROR) << "sys.powerctl: " << unwinder.FormatFrame(frame); - } -} - -void DebugRebootLogging() { - LOG(INFO) << "sys.powerctl: do_shutdown: " << shutdown_state.do_shutdown() - << " IsShuttingDown: " << IsShuttingDown(); - if (shutdown_state.do_shutdown()) { - LOG(ERROR) << "sys.powerctl set while a previous shutdown command has not been handled"; - UnwindMainThreadStack(); - } - if (IsShuttingDown()) { - LOG(ERROR) << "sys.powerctl set while init is already shutting down"; - UnwindMainThreadStack(); - } -} - void DumpState() { ServiceList::GetInstance().DumpState(); ActionManager::GetInstance().DumpState(); diff --git a/init/init.h b/init/init.h index 063632a66..9c7e91879 100644 --- a/init/init.h +++ b/init/init.h @@ -42,8 +42,6 @@ void SendLoadPersistentPropertiesMessage(); void PropertyChanged(const std::string& name, const std::string& value); bool QueueControlMessage(const std::string& message, const std::string& name, pid_t pid, int fd); -void DebugRebootLogging(); - int SecondStageMain(int argc, char** argv); int StopServicesFromApex(const std::string& apex_name); diff --git a/init/property_service.cpp b/init/property_service.cpp index 87ffdb917..8da69822c 100644 --- a/init/property_service.cpp +++ b/init/property_service.cpp @@ -550,9 +550,6 @@ std::optional HandlePropertySet(const std::string& name, const std::st } LOG(INFO) << "Received sys.powerctl='" << value << "' from pid: " << cr.pid << process_log_string; - if (!value.empty()) { - DebugRebootLogging(); - } if (value == "reboot,userspace" && !is_userspace_reboot_supported().value_or(false)) { *error = "Userspace reboot is not supported by this device"; return {PROP_ERROR_INVALID_VALUE}; From 071dbc17290ea1c9c4fa645fb5a2cee796009ae8 Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Fri, 17 Feb 2023 11:27:33 -0800 Subject: [PATCH 3/6] init: Combine the CheckShutdown() and set_do_shutdown() methods Let the CheckShutdown() method clear the do_shutdown_ member instead of clearing that member separately from calling CheckShutdown(). Bug: 266255006 Change-Id: Ifc1cff2be92a45db7f91be2fdb812930d2fd1ad5 Signed-off-by: Bart Van Assche --- init/init.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/init/init.cpp b/init/init.cpp index a01ae8765..be1ebeed7 100644 --- a/init/init.cpp +++ b/init/init.cpp @@ -247,16 +247,15 @@ static class ShutdownState { WakeMainInitThread(); } - std::optional CheckShutdown() { + std::optional CheckShutdown() __attribute__((warn_unused_result)) { auto lock = std::lock_guard{shutdown_command_lock_}; if (do_shutdown_ && !IsShuttingDown()) { + do_shutdown_ = false; return shutdown_command_; } return {}; } - void set_do_shutdown(bool value) { do_shutdown_ = value; } - private: std::mutex shutdown_command_lock_; std::string shutdown_command_ GUARDED_BY(shutdown_command_lock_); @@ -1094,7 +1093,6 @@ int SecondStageMain(int argc, char** argv) { LOG(INFO) << "Got shutdown_command '" << *shutdown_command << "' Calling HandlePowerctlMessage()"; HandlePowerctlMessage(*shutdown_command); - shutdown_state.set_do_shutdown(false); } if (!(prop_waiter_state.MightBeWaiting() || Service::is_exec_service_running())) { From 4c17a070532503993488bd4f6ab2ad871cc7d20f Mon Sep 17 00:00:00 2001 From: David Anderson Date: Mon, 6 Mar 2023 22:56:49 +0000 Subject: [PATCH 4/6] libsnapshot: Fix test failures on certain configurations. Due to how CF is built and tested, VABC is enabled even when not supported by the kernel. To work around this add some logic in libsnapshot and the test harness to recognize this situation and silently flip off the VABC flag. This also fixes the -force_mode option to vts_libsnapshot_test, so that it will skip tests that aren't supported by the device. Bug: 264279496 Test: vts_libsnapshot_test on 11-5.4 with vabc enabled Change-Id: I4c1e88fac54732c21c88169a7e0dd664e3858b89 --- fs_mgr/libsnapshot/snapshot.cpp | 2 ++ fs_mgr/libsnapshot/snapshot_test.cpp | 35 +++++++++++++++++++++++++++- fs_mgr/libsnapshot/utility.cpp | 11 ++++++++- fs_mgr/libsnapshot/utility.h | 2 ++ 4 files changed, 48 insertions(+), 2 deletions(-) diff --git a/fs_mgr/libsnapshot/snapshot.cpp b/fs_mgr/libsnapshot/snapshot.cpp index 15f025c9c..f655522c6 100644 --- a/fs_mgr/libsnapshot/snapshot.cpp +++ b/fs_mgr/libsnapshot/snapshot.cpp @@ -3216,6 +3216,8 @@ Return SnapshotManager::CreateUpdateSnapshots(const DeltaArchiveManifest& manife vabc_disable_reason = "recovery"; } else if (!cow_format_support) { vabc_disable_reason = "cow format not supported"; + } else if (!KernelSupportsCompressedSnapshots()) { + vabc_disable_reason = "kernel missing userspace block device support"; } if (!vabc_disable_reason.empty()) { diff --git a/fs_mgr/libsnapshot/snapshot_test.cpp b/fs_mgr/libsnapshot/snapshot_test.cpp index 13314daab..460d49db2 100644 --- a/fs_mgr/libsnapshot/snapshot_test.cpp +++ b/fs_mgr/libsnapshot/snapshot_test.cpp @@ -124,6 +124,10 @@ class SnapshotTest : public ::testing::Test { SKIP_IF_NON_VIRTUAL_AB(); SetupProperties(); + if (!DeviceSupportsMode()) { + GTEST_SKIP() << "Mode not supported on this device"; + } + InitializeState(); CleanupTestArtifacts(); FormatFakeSuper(); @@ -159,7 +163,13 @@ class SnapshotTest : public ::testing::Test { IPropertyFetcher::OverrideForTesting(std::move(fetcher)); if (GetLegacyCompressionEnabledProperty() || CanUseUserspaceSnapshots()) { - snapuserd_required_ = true; + // If we're asked to test the device's actual configuration, then it + // may be misconfigured, so check for kernel support as libsnapshot does. + if (FLAGS_force_mode.empty()) { + snapuserd_required_ = KernelSupportsCompressedSnapshots(); + } else { + snapuserd_required_ = true; + } } } @@ -176,6 +186,16 @@ class SnapshotTest : public ::testing::Test { LOG(INFO) << "Teardown complete for test: " << test_name_; } + bool DeviceSupportsMode() { + if (FLAGS_force_mode.empty()) { + return true; + } + if (snapuserd_required_ && !KernelSupportsCompressedSnapshots()) { + return false; + } + return true; + } + void InitializeState() { ASSERT_TRUE(sm->EnsureImageManager()); image_manager_ = sm->image_manager(); @@ -193,6 +213,11 @@ class SnapshotTest : public ::testing::Test { // get an accurate list to remove. lock_ = nullptr; + // If there is no image manager, the test was skipped. + if (!image_manager_) { + return; + } + std::vector snapshots = {"test-snapshot", "test_partition_a", "test_partition_b"}; for (const auto& snapshot : snapshots) { @@ -946,6 +971,11 @@ class SnapshotUpdateTest : public SnapshotTest { SKIP_IF_NON_VIRTUAL_AB(); SnapshotTest::SetUp(); + if (!image_manager_) { + // Test was skipped. + return; + } + Cleanup(); // Cleanup() changes slot suffix, so initialize it again. @@ -2680,6 +2710,9 @@ class ImageManagerTest : public SnapshotTest { CleanUp(); } void CleanUp() { + if (!image_manager_) { + return; + } EXPECT_TRUE(!image_manager_->BackingImageExists(kImageName) || image_manager_->DeleteBackingImage(kImageName)); } diff --git a/fs_mgr/libsnapshot/utility.cpp b/fs_mgr/libsnapshot/utility.cpp index a98bf0e5c..1ffa89cba 100644 --- a/fs_mgr/libsnapshot/utility.cpp +++ b/fs_mgr/libsnapshot/utility.cpp @@ -29,6 +29,7 @@ #include #include +using android::dm::DeviceMapper; using android::dm::kSectorSize; using android::fiemap::FiemapStatus; using android::fs_mgr::EnsurePathMounted; @@ -251,7 +252,10 @@ bool CanUseUserspaceSnapshots() { LOG(INFO) << "Userspace snapshots disabled for testing"; return false; } - + if (!KernelSupportsCompressedSnapshots()) { + LOG(ERROR) << "Userspace snapshots requested, but no kernel support is available."; + return false; + } return true; } @@ -278,5 +282,10 @@ bool IsDmSnapshotTestingEnabled() { return fetcher->GetBoolProperty("snapuserd.test.dm.snapshots", false); } +bool KernelSupportsCompressedSnapshots() { + auto& dm = DeviceMapper::Instance(); + return dm.GetTargetByName("user", nullptr); +} + } // namespace snapshot } // namespace android diff --git a/fs_mgr/libsnapshot/utility.h b/fs_mgr/libsnapshot/utility.h index 8c4c7c664..370f3c4fc 100644 --- a/fs_mgr/libsnapshot/utility.h +++ b/fs_mgr/libsnapshot/utility.h @@ -127,6 +127,8 @@ std::ostream& operator<<(std::ostream& os, const Now&); void AppendExtent(google::protobuf::RepeatedPtrField* extents, uint64_t start_block, uint64_t num_blocks); +bool KernelSupportsCompressedSnapshots(); + bool GetLegacyCompressionEnabledProperty(); bool GetUserspaceSnapshotsEnabledProperty(); bool GetIouringEnabledProperty(); From 65d8e53b606e4ae7387f421c5277a942f8e729d8 Mon Sep 17 00:00:00 2001 From: Suren Baghdasaryan Date: Thu, 2 Mar 2023 14:12:49 -0800 Subject: [PATCH 5/6] libprocessgroup: fix boot time performance regression The way processes are accounted in DoKillProcessGroupOnce has been changed recently, which affects retries in KillProcessGroup. More specifically, initialPid was not counted before and would not cause a retry with 5ms sleep. Restore previous behavior to avoid boot time regressions. Bug: 271198843 Signed-off-by: Suren Baghdasaryan Change-Id: Ibc1bdd855898688a4a03806671e6ac31570aedf9 (cherry picked from commit on android-review.googlesource.com host: 4f7cc8c3455b2a28984e2682c1b8075cb57d67fe) Merged-In: Ibc1bdd855898688a4a03806671e6ac31570aedf9 --- libprocessgroup/processgroup.cpp | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/libprocessgroup/processgroup.cpp b/libprocessgroup/processgroup.cpp index 92a27efb7..71ba7ced2 100644 --- a/libprocessgroup/processgroup.cpp +++ b/libprocessgroup/processgroup.cpp @@ -377,6 +377,7 @@ static int DoKillProcessGroupOnce(const char* cgroup, uid_t uid, int initialPid, std::set pgids; pgids.emplace(initialPid); std::set pids; + int processes = 0; std::unique_ptr fd(nullptr, fclose); @@ -395,6 +396,7 @@ static int DoKillProcessGroupOnce(const char* cgroup, uid_t uid, int initialPid, pid_t pid; bool file_is_empty = true; while (fscanf(fd.get(), "%d\n", &pid) == 1 && pid >= 0) { + processes++; file_is_empty = false; if (pid == 0) { // Should never happen... but if it does, trying to kill this @@ -424,15 +426,12 @@ static int DoKillProcessGroupOnce(const char* cgroup, uid_t uid, int initialPid, } } - int processes = 0; // Kill all process groups. for (const auto pgid : pgids) { LOG(VERBOSE) << "Killing process group " << -pgid << " in uid " << uid << " as part of process cgroup " << initialPid; - if (kill(-pgid, signal) == 0) { - processes++; - } else if (errno != ESRCH) { + if (kill(-pgid, signal) == -1 && errno != ESRCH) { PLOG(WARNING) << "kill(" << -pgid << ", " << signal << ") failed"; } } @@ -442,9 +441,7 @@ static int DoKillProcessGroupOnce(const char* cgroup, uid_t uid, int initialPid, LOG(VERBOSE) << "Killing pid " << pid << " in uid " << uid << " as part of process cgroup " << initialPid; - if (kill(pid, signal) == 0) { - processes++; - } else if (errno != ESRCH) { + if (kill(pid, signal) == -1 && errno != ESRCH) { PLOG(WARNING) << "kill(" << pid << ", " << signal << ") failed"; } } From f5e1533f2f19327f60723b5326c122b81066f2db Mon Sep 17 00:00:00 2001 From: zijunzhao Date: Fri, 3 Mar 2023 01:33:28 +0000 Subject: [PATCH 6/6] Suppress the error warning Bug: https://android-build.googleplex.com/builds/pending/P51300433/aosp_bramble-userdebug/latest/view/logs/build_error.log Test: None Change-Id: I2fbd3d8772c50ed9de1c2ba9eb2234966c7dcb84 --- debuggerd/crasher/crasher.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/debuggerd/crasher/crasher.cpp b/debuggerd/crasher/crasher.cpp index 4eb738205..4043a6e03 100644 --- a/debuggerd/crasher/crasher.cpp +++ b/debuggerd/crasher/crasher.cpp @@ -159,7 +159,8 @@ noinline void sigsegv_non_null() { } noinline void fprintf_null() { - fprintf(nullptr, "oops"); + FILE* sneaky_null = nullptr; + fprintf(sneaky_null, "oops"); } noinline void readdir_null() {