From eb4b464c58be39fc90f62b823ace017e88867feb Mon Sep 17 00:00:00 2001 From: Paul Chang Date: Thu, 28 May 2020 22:05:47 +0800 Subject: Remove unnecessary code for new BugreportManager BUG: 154298410 Test: Build pass Change-Id: I88c1080fa714a84f708fc73cc5a59f44a7610859 --- cmds/dumpstate/dumpstate.cpp | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index d4c161609c..93f8b68aed 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -2686,18 +2686,18 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, // TODO(nandana) reduce code repetition in if branches if (options_->telephony_only) { MaybeTakeEarlyScreenshot(); - onUiIntensiveBugreportDumpsFinished(calling_uid, calling_package); + onUiIntensiveBugreportDumpsFinished(calling_uid); MaybeCheckUserConsent(calling_uid, calling_package); DumpstateTelephonyOnly(calling_package); DumpstateBoard(); } else if (options_->wifi_only) { MaybeTakeEarlyScreenshot(); - onUiIntensiveBugreportDumpsFinished(calling_uid, calling_package); + onUiIntensiveBugreportDumpsFinished(calling_uid); MaybeCheckUserConsent(calling_uid, calling_package); DumpstateWifiOnly(); } else if (options_->arc_only) { MaybeTakeEarlyScreenshot(); - onUiIntensiveBugreportDumpsFinished(calling_uid, calling_package); + onUiIntensiveBugreportDumpsFinished(calling_uid); MaybeCheckUserConsent(calling_uid, calling_package); DumpstateArcOnly(); } else { @@ -2706,7 +2706,7 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, // Take screenshot and get consent only after critical dumpsys has finished. MaybeTakeEarlyScreenshot(); - onUiIntensiveBugreportDumpsFinished(calling_uid, calling_package); + onUiIntensiveBugreportDumpsFinished(calling_uid); MaybeCheckUserConsent(calling_uid, calling_package); // Dump state for the default case. This also drops root. @@ -2796,16 +2796,14 @@ void Dumpstate::MaybeTakeEarlyScreenshot() { TakeScreenshot(); } -void Dumpstate::onUiIntensiveBugreportDumpsFinished(int32_t calling_uid, - const std::string& calling_package) { +void Dumpstate::onUiIntensiveBugreportDumpsFinished(int32_t calling_uid) { if (calling_uid == AID_SHELL || !CalledByApi()) { return; } if (listener_ != nullptr) { // Let listener know ui intensive bugreport dumps are finished, then it can do event // handling if required. - android::String16 package(calling_package.c_str()); - listener_->onUiIntensiveBugreportDumpsFinished(package); + listener_->onUiIntensiveBugreportDumpsFinished(); } } -- cgit v1.2.3-59-g8ed1b From 5377d797fff94e0e070fdbbadc89288d90cc5f2c Mon Sep 17 00:00:00 2001 From: Rhed Jao Date: Thu, 16 Jul 2020 17:37:39 +0800 Subject: Faster bugreports (2/n) - Having a system property to disable parallel run function. - Post 'DumpTraces' to the thread pool. 3% is improved compared with single thread run. Bug: 136262402 Test: atest dumpstate_test Test: atest dumpstate_smoke_test Test: Manual enable and disable parallel run Test: Manual trigger report using hardware key Test: Manual trigger using bugreport shortcut Change-Id: I2185cc3e2f429a150605d6268324c52d137db385 --- cmds/dumpstate/DumpPool.cpp | 23 ++++++++- cmds/dumpstate/DumpPool.h | 76 ++++++++++++++++++++++-------- cmds/dumpstate/DumpstateUtil.cpp | 9 ++++ cmds/dumpstate/DumpstateUtil.h | 8 ++++ cmds/dumpstate/dumpstate.cpp | 82 +++++++++++++++++++++++++++++---- cmds/dumpstate/dumpstate.h | 13 +++++- cmds/dumpstate/tests/dumpstate_test.cpp | 74 +++++++++++++++++++++++++---- 7 files changed, 247 insertions(+), 38 deletions(-) (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/DumpPool.cpp b/cmds/dumpstate/DumpPool.cpp index 7324ead7c6..e174c8eff3 100644 --- a/cmds/dumpstate/DumpPool.cpp +++ b/cmds/dumpstate/DumpPool.cpp @@ -33,7 +33,8 @@ namespace dumpstate { const std::string DumpPool::PREFIX_TMPFILE_NAME = "dump-tmp."; -DumpPool::DumpPool(const std::string& tmp_root) : tmp_root_(tmp_root), shutdown_(false) { +DumpPool::DumpPool(const std::string& tmp_root) : tmp_root_(tmp_root), shutdown_(false), + log_duration_(true) { assert(!tmp_root.empty()); deleteTempFiles(tmp_root_); } @@ -99,6 +100,26 @@ void DumpPool::waitForTask(const std::string& task_name, const std::string& titl } } +void DumpPool::setLogDuration(bool log_duration) { + log_duration_ = log_duration; +} + +template <> +void DumpPool::invokeTask>(std::function dump_func, + const std::string& duration_title, int out_fd) { + DurationReporter duration_reporter(duration_title, /*logcat_only =*/!log_duration_, + /*verbose =*/false, out_fd); + std::invoke(dump_func); +} + +template <> +void DumpPool::invokeTask>(std::function dump_func, + const std::string& duration_title, int out_fd) { + DurationReporter duration_reporter(duration_title, /*logcat_only =*/!log_duration_, + /*verbose =*/false, out_fd); + std::invoke(dump_func, out_fd); +} + std::unique_ptr DumpPool::createTempFile() { auto tmp_file_ptr = std::make_unique(); std::string file_name_format = "%s/" + PREFIX_TMPFILE_NAME + "XXXXXX"; diff --git a/cmds/dumpstate/DumpPool.h b/cmds/dumpstate/DumpPool.h index 266d519b6d..a4ea875541 100644 --- a/cmds/dumpstate/DumpPool.h +++ b/cmds/dumpstate/DumpPool.h @@ -29,26 +29,32 @@ namespace android { namespace os { namespace dumpstate { +class DumpPoolTest; + /* * A thread pool with the fixed number of threads to execute multiple dump tasks - * simultaneously for the dumpstate. The dump task is a callable function - * included a file descriptor as a parameter, and the task could dump results to - * that fd. For example: + * simultaneously for the dumpstate. The dump task is a callable function. It + * could include a file descriptor as a parameter to redirect dump results, if + * it needs to output results to the bugreport. This can avoid messing up + * bugreport's results when multiple dump tasks are running at the same time. + * Takes an example below for the usage of the DumpPool: * - * void DumpXXXX(int out_fd) { + * void DumpFoo(int out_fd) { * dprintf(out_fd, "Dump result to out_fd ..."); * } * ... * DumpPool pool(tmp_root); - * pool.enqueueTask("TaskName", &DumpXXXX, std::placeholders::_1); + * pool.enqueueTaskWithFd("TaskName", &DumpFoo, std::placeholders::_1); * ... * pool.waitForTask("TaskName"); * - * DumpXXXX is a callable function included a out_fd parameter. Using the - * enqueueTask method in DumpPool to enqueue the task to the pool. The - * std::placeholders::_1 is placeholder for DumpPool to pass a fd argument. + * DumpFoo is a callable function included a out_fd parameter. Using the + * enqueueTaskWithFd method in DumpPool to enqueue the task to the pool. The + * std::placeholders::_1 is a placeholder for DumpPool to pass a fd argument. */ class DumpPool { + friend class android::os::dumpstate::DumpPoolTest; + public: /* * Creates a thread pool. @@ -72,17 +78,40 @@ class DumpPool { void shutdown(); /* - * Adds a task with a task name into the queue of the thread pool. + * Adds a task into the queue of the thread pool. * - * |task_name| The name of the task. - * |f| Callable function to execute the task. This function must - * include a parameter of file descriptor to output dump result. + * |task_name| The name of the task. It's also the title of the + * DurationReporter log. + * |f| Callable function to execute the task. * |args| A list of arguments. + * + * TODO(b/164369078): remove this api to have just one enqueueTask for consistency. */ - template void enqueueTask(const std::string& task_name, - F&& f, Args&&... args) { - auto func = std::bind(std::forward(f), std::forward(args)...); - futures_map_[task_name] = post(func); + template void enqueueTask(const std::string& task_name, F&& f, + Args&&... args) { + std::function func = std::bind(std::forward(f), + std::forward(args)...); + futures_map_[task_name] = post(task_name, func); + if (threads_.empty()) { + start(); + } + } + + /* + * Adds a task into the queue of the thread pool. The task takes a file + * descriptor as a parameter to redirect dump results to a temporary file. + * + * |task_name| The name of the task. It's also the title of the + * DurationReporter log. + * |f| Callable function to execute the task. + * |args| A list of arguments. A placeholder std::placeholders::_1 as a fd + * argument needs to be included here. + */ + template void enqueueTaskWithFd(const std::string& task_name, F&& f, + Args&&... args) { + std::function func = std::bind(std::forward(f), + std::forward(args)...); + futures_map_[task_name] = post(task_name, func); if (threads_.empty()) { start(); } @@ -111,13 +140,15 @@ class DumpPool { using Task = std::packaged_task; using Future = std::shared_future; - template Future post(T dump_func) { + template void invokeTask(T dump_func, const std::string& duration_title, int out_fd); + + template Future post(const std::string& task_name, T dump_func) { Task packaged_task([=]() { std::unique_ptr tmp_file_ptr = createTempFile(); if (!tmp_file_ptr) { return std::string(""); } - std::invoke(dump_func, tmp_file_ptr->fd.get()); + invokeTask(dump_func, task_name, tmp_file_ptr->fd.get()); fsync(tmp_file_ptr->fd.get()); return std::string(tmp_file_ptr->path); }); @@ -138,12 +169,21 @@ class DumpPool { void setThreadName(const pthread_t thread, int id); void loop(); + /* + * For test purpose only. Enables or disables logging duration of the task. + * + * |log_duration| if true, DurationReporter is initiated to log duration of + * the task. + */ + void setLogDuration(bool log_duration); + private: static const int MAX_THREAD_COUNT = 4; /* A path to a temporary folder for threads to create temporary files. */ std::string tmp_root_; bool shutdown_; + bool log_duration_; // For test purpose only, the default value is true. std::mutex lock_; // A lock for the tasks_. std::condition_variable condition_variable_; diff --git a/cmds/dumpstate/DumpstateUtil.cpp b/cmds/dumpstate/DumpstateUtil.cpp index 4b69607156..eeaa5a3de0 100644 --- a/cmds/dumpstate/DumpstateUtil.cpp +++ b/cmds/dumpstate/DumpstateUtil.cpp @@ -180,6 +180,7 @@ CommandOptions::CommandOptionsBuilder CommandOptions::WithTimeoutInMs(int64_t ti std::string PropertiesHelper::build_type_ = ""; int PropertiesHelper::dry_run_ = -1; int PropertiesHelper::unroot_ = -1; +int PropertiesHelper::parallel_run_ = -1; bool PropertiesHelper::IsUserBuild() { if (build_type_.empty()) { @@ -202,6 +203,14 @@ bool PropertiesHelper::IsUnroot() { return unroot_ == 1; } +bool PropertiesHelper::IsParallelRun() { + if (parallel_run_ == -1) { + parallel_run_ = android::base::GetBoolProperty("dumpstate.parallel_run", + /* default_value = */true) ? 1 : 0; + } + return parallel_run_ == 1; +} + int DumpFileToFd(int out_fd, const std::string& title, const std::string& path) { android::base::unique_fd fd(TEMP_FAILURE_RETRY(open(path.c_str(), O_RDONLY | O_NONBLOCK | O_CLOEXEC))); if (fd.get() < 0) { diff --git a/cmds/dumpstate/DumpstateUtil.h b/cmds/dumpstate/DumpstateUtil.h index b7ac25c81e..b099443e32 100644 --- a/cmds/dumpstate/DumpstateUtil.h +++ b/cmds/dumpstate/DumpstateUtil.h @@ -176,10 +176,18 @@ class PropertiesHelper { */ static bool IsUnroot(); + /* + * Whether or not the parallel run is enabled. Setting the system property + * 'dumpstate.parallel_run' to false to disable it, otherwise it returns + * true by default. + */ + static bool IsParallelRun(); + private: static std::string build_type_; static int dry_run_; static int unroot_; + static int parallel_run_; }; /* diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index af41643d5d..7d195b4fa1 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -95,6 +95,7 @@ using ::android::hardware::dumpstate::V1_1::DumpstateStatus; using ::android::hardware::dumpstate::V1_1::toString; using ::std::literals::chrono_literals::operator""ms; using ::std::literals::chrono_literals::operator""s; +using ::std::placeholders::_1; // TODO: remove once moved to namespace using android::defaultServiceManager; @@ -113,6 +114,7 @@ using android::base::StringPrintf; using android::os::IDumpstateListener; using android::os::dumpstate::CommandOptions; using android::os::dumpstate::DumpFileToFd; +using android::os::dumpstate::DumpPool; using android::os::dumpstate::PropertiesHelper; // Keep in sync with @@ -196,8 +198,26 @@ static const std::string ANR_FILE_PREFIX = "anr_"; func_ptr(__VA_ARGS__); \ RETURN_IF_USER_DENIED_CONSENT(); +// Runs func_ptr, and logs a duration report after it's finished. +#define RUN_SLOW_FUNCTION_AND_LOG(log_title, func_ptr, ...) \ + { \ + DurationReporter duration_reporter_in_macro(log_title); \ + func_ptr(__VA_ARGS__); \ + } + +// Similar with RUN_SLOW_FUNCTION_WITH_CONSENT_CHECK, an additional duration report +// is output after a slow function is finished. +#define RUN_SLOW_FUNCTION_WITH_CONSENT_CHECK_AND_LOG(log_title, func_ptr, ...) \ + RETURN_IF_USER_DENIED_CONSENT(); \ + RUN_SLOW_FUNCTION_AND_LOG(log_title, func_ptr, __VA_ARGS__); \ + RETURN_IF_USER_DENIED_CONSENT(); + static const char* WAKE_LOCK_NAME = "dumpstate_wakelock"; +// Names of parallel tasks, they are used for the DumpPool to identify the dump +// task and the log title of the duration report. +static const std::string DUMP_TRACES_TASK = "DUMP TRACES"; + namespace android { namespace os { namespace { @@ -762,8 +782,9 @@ void Dumpstate::PrintHeader() const { RunCommandToFd(STDOUT_FILENO, "", {"uptime", "-p"}, CommandOptions::WithTimeout(1).Always().Build()); printf("Bugreport format version: %s\n", version_.c_str()); - printf("Dumpstate info: id=%d pid=%d dry_run=%d args=%s bugreport_mode=%s\n", id_, pid_, - PropertiesHelper::IsDryRun(), options_->args.c_str(), options_->bugreport_mode.c_str()); + printf("Dumpstate info: id=%d pid=%d dry_run=%d parallel_run=%d args=%s bugreport_mode=%s\n", + id_, pid_, PropertiesHelper::IsDryRun(), PropertiesHelper::IsParallelRun(), + options_->args.c_str(), options_->bugreport_mode.c_str()); printf("\n"); } @@ -1680,7 +1701,18 @@ Dumpstate::RunStatus Dumpstate::DumpstateDefaultAfterCritical() { time_t logcat_ts = time(nullptr); /* collect stack traces from Dalvik and native processes (needs root) */ - RUN_SLOW_FUNCTION_WITH_CONSENT_CHECK(ds.DumpTraces, &dump_traces_path); + if (dump_pool_) { + RETURN_IF_USER_DENIED_CONSENT(); + // One thread is enough since we only need to enqueue DumpTraces here. + dump_pool_->start(/* thread_counts = */1); + + // DumpTraces takes long time, post it to the another thread in the + // pool, if pool is available + dump_pool_->enqueueTask(DUMP_TRACES_TASK, &Dumpstate::DumpTraces, &ds, &dump_traces_path); + } else { + RUN_SLOW_FUNCTION_WITH_CONSENT_CHECK_AND_LOG(DUMP_TRACES_TASK, ds.DumpTraces, + &dump_traces_path); + } /* Run some operations that require root. */ ds.tombstone_data_ = GetDumpFds(TOMBSTONE_DIR, TOMBSTONE_FILE_PREFIX, !ds.IsZipping()); @@ -1723,6 +1755,15 @@ Dumpstate::RunStatus Dumpstate::DumpstateDefaultAfterCritical() { DumpFile("PSI memory", "/proc/pressure/memory"); DumpFile("PSI io", "/proc/pressure/io"); + if (dump_pool_) { + RETURN_IF_USER_DENIED_CONSENT(); + dump_pool_->waitForTask(DUMP_TRACES_TASK); + + // Current running thread in the pool is the root user also. Shutdown + // the pool and restart later to ensure all threads in the pool could + // drop the root user. + dump_pool_->shutdown(); + } if (!DropRootUser()) { return Dumpstate::RunStatus::ERROR; } @@ -1881,8 +1922,6 @@ static void DumpstateWifiOnly() { } Dumpstate::RunStatus Dumpstate::DumpTraces(const char** path) { - DurationReporter duration_reporter("DUMP TRACES"); - const std::string temp_file_pattern = "/data/anr/dumptrace_XXXXXX"; const size_t buf_size = temp_file_pattern.length() + 1; std::unique_ptr file_name_buf(new char[buf_size]); @@ -2554,6 +2593,7 @@ void Dumpstate::Cancel() { */ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, const std::string& calling_package) { + DurationReporter duration_reporter("RUN INTERNAL", /* logcat_only = */true); LogDumpOptions(*options_); if (!options_->ValidateOptions()) { MYLOGE("Invalid options specified\n"); @@ -2715,6 +2755,13 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, // Don't buffer stdout setvbuf(stdout, nullptr, _IONBF, 0); + // Enable the parallel run if the client requests to output to a file. + EnableParallelRunIfNeeded(); + // Using scope guard to make sure the dump pool can be shut down correctly. + auto scope_guard_to_shutdown_pool = android::base::make_scope_guard([=]() { + ShutdownDumpPool(); + }); + // NOTE: there should be no stdout output until now, otherwise it would break the header. // In particular, DurationReport objects should be created passing 'title, NULL', so their // duration is logged into MYLOG instead. @@ -2881,6 +2928,23 @@ void Dumpstate::CleanupTmpFiles() { android::os::UnlinkAndLogOnError(path_); } +void Dumpstate::EnableParallelRunIfNeeded() { + // The thread pool needs to create temporary files to receive dump results. + // That's why we only enable it when the bugreport client chooses to output + // to a file. + if (!PropertiesHelper::IsParallelRun() || !options_->OutputToFile()) { + return; + } + dump_pool_ = std::make_unique(bugreport_internal_dir_); +} + +void Dumpstate::ShutdownDumpPool() { + if (dump_pool_) { + dump_pool_->shutdown(); + dump_pool_ = nullptr; + } +} + Dumpstate::RunStatus Dumpstate::HandleUserConsentDenied() { MYLOGD("User denied consent; deleting files and returning\n"); CleanupTmpFiles(); @@ -2999,8 +3063,9 @@ Dumpstate& Dumpstate::GetInstance() { return singleton_; } -DurationReporter::DurationReporter(const std::string& title, bool logcat_only, bool verbose) - : title_(title), logcat_only_(logcat_only), verbose_(verbose) { +DurationReporter::DurationReporter(const std::string& title, bool logcat_only, bool verbose, + int duration_fd) : title_(title), logcat_only_(logcat_only), verbose_(verbose), + duration_fd_(duration_fd) { if (!title_.empty()) { started_ = Nanotime(); } @@ -3014,7 +3079,8 @@ DurationReporter::~DurationReporter() { } if (!logcat_only_) { // Use "Yoda grammar" to make it easier to grep|sort sections. - printf("------ %.3fs was the duration of '%s' ------\n", elapsed, title_.c_str()); + dprintf(duration_fd_, "------ %.3fs was the duration of '%s' ------\n", + elapsed, title_.c_str()); } } } diff --git a/cmds/dumpstate/dumpstate.h b/cmds/dumpstate/dumpstate.h index 0d25d307a6..d400dc7127 100644 --- a/cmds/dumpstate/dumpstate.h +++ b/cmds/dumpstate/dumpstate.h @@ -35,6 +35,7 @@ #include #include "DumpstateUtil.h" +#include "DumpPool.h" // Workaround for const char *args[MAX_ARGS_ARRAY_SIZE] variables until they're converted to // std::vector @@ -75,7 +76,7 @@ extern "C" { class DurationReporter { public: explicit DurationReporter(const std::string& title, bool logcat_only = false, - bool verbose = false); + bool verbose = false, int duration_fd = STDOUT_FILENO); ~DurationReporter(); @@ -84,6 +85,7 @@ class DurationReporter { bool logcat_only_; bool verbose_; uint64_t started_; + int duration_fd_; DISALLOW_COPY_AND_ASSIGN(DurationReporter); }; @@ -193,7 +195,7 @@ struct DumpData { * that are spread accross utils.cpp and dumpstate.cpp will be moved to it. */ class Dumpstate { - friend class DumpstateTest; + friend class android::os::dumpstate::DumpstateTest; public: enum RunStatus { OK, HELP, INVALID_INPUT, ERROR, USER_CONSENT_DENIED, USER_CONSENT_TIMED_OUT }; @@ -490,6 +492,9 @@ class Dumpstate { // List of open ANR dump files. std::vector anr_data_; + // A thread pool to execute dump tasks simultaneously if the parallel run is enabled. + std::unique_ptr dump_pool_; + // A callback to IncidentCompanion service, which checks user consent for sharing the // bugreport with the calling app. If the user has not responded yet to the dialog it will // be neither confirmed nor denied. @@ -528,6 +533,10 @@ class Dumpstate { // but leaves the log file alone. void CleanupTmpFiles(); + // Create the thread pool to enable the parallel run function. + void EnableParallelRunIfNeeded(); + void ShutdownDumpPool(); + RunStatus HandleUserConsentDenied(); // Copies bugreport artifacts over to the caller's directories provided there is user consent or diff --git a/cmds/dumpstate/tests/dumpstate_test.cpp b/cmds/dumpstate/tests/dumpstate_test.cpp index d0a48266f2..b3cb4349f3 100644 --- a/cmds/dumpstate/tests/dumpstate_test.cpp +++ b/cmds/dumpstate/tests/dumpstate_test.cpp @@ -97,6 +97,10 @@ class DumpstateBaseTest : public Test { PropertiesHelper::unroot_ = unroot; } + void SetParallelRun(bool parallel_run) const { + PropertiesHelper::parallel_run_ = parallel_run; + } + bool IsStandalone() const { return calls_ == 1; } @@ -544,6 +548,10 @@ class DumpstateTest : public DumpstateBaseTest { ds.options_.reset(new Dumpstate::DumpOptions()); } + void TearDown() { + ds.ShutdownDumpPool(); + } + // Runs a command and capture `stdout` and `stderr`. int RunCommand(const std::string& title, const std::vector& full_command, const CommandOptions& options = CommandOptions::DEFAULT) { @@ -571,6 +579,10 @@ class DumpstateTest : public DumpstateBaseTest { ds.progress_.reset(new Progress(initial_max, progress, 1.2)); } + void EnableParallelRunIfNeeded() { + ds.EnableParallelRunIfNeeded(); + } + std::string GetProgressMessage(int progress, int max, int old_max = 0, bool update_progress = true) { EXPECT_EQ(progress, ds.progress_->Get()) << "invalid progress"; @@ -1009,6 +1021,27 @@ TEST_F(DumpstateTest, DumpFileUpdateProgress) { ds.listener_.clear(); } +TEST_F(DumpstateTest, DumpPool_withOutputToFileAndParallelRunEnabled_notNull) { + ds.options_->use_socket = false; + SetParallelRun(true); + EnableParallelRunIfNeeded(); + EXPECT_TRUE(ds.options_->OutputToFile()); + EXPECT_TRUE(ds.dump_pool_); +} + +TEST_F(DumpstateTest, DumpPool_withNotOutputToFile_isNull) { + ds.options_->use_socket = true; + EnableParallelRunIfNeeded(); + EXPECT_FALSE(ds.options_->OutputToFile()); + EXPECT_FALSE(ds.dump_pool_); +} + +TEST_F(DumpstateTest, DumpPool_withParallelRunDisabled_isNull) { + SetParallelRun(false); + EnableParallelRunIfNeeded(); + EXPECT_FALSE(ds.dump_pool_); +} + class DumpstateServiceTest : public DumpstateBaseTest { public: DumpstateService dss; @@ -1623,6 +1656,7 @@ TEST_F(DumpstateUtilTest, DumpFileOnDryRun) { class DumpPoolTest : public DumpstateBaseTest { public: void SetUp() { + dump_pool_ = std::make_unique(kTestDataPath); DumpstateBaseTest::SetUp(); CreateOutputFile(); } @@ -1662,12 +1696,16 @@ class DumpPoolTest : public DumpstateBaseTest { return count; } + void setLogDuration(bool log_duration) { + dump_pool_->setLogDuration(log_duration); + } + + std::unique_ptr dump_pool_; android::base::unique_fd out_fd_; std::string out_path_; }; -TEST_F(DumpPoolTest, EnqueueTask) { - DumpPool pool(kTestDataPath); +TEST_F(DumpPoolTest, EnqueueTaskWithFd) { auto dump_func_1 = [](int out_fd) { dprintf(out_fd, "A"); }; @@ -1678,19 +1716,37 @@ TEST_F(DumpPoolTest, EnqueueTask) { auto dump_func_3 = [](int out_fd) { dprintf(out_fd, "C"); }; - pool.enqueueTask(/* task_name = */"1", dump_func_1, std::placeholders::_1); - pool.enqueueTask(/* task_name = */"2", dump_func_2, std::placeholders::_1); - pool.enqueueTask(/* task_name = */"3", dump_func_3, std::placeholders::_1); + setLogDuration(/* log_duration = */false); + dump_pool_->enqueueTaskWithFd(/* task_name = */"1", dump_func_1, std::placeholders::_1); + dump_pool_->enqueueTaskWithFd(/* task_name = */"2", dump_func_2, std::placeholders::_1); + dump_pool_->enqueueTaskWithFd(/* task_name = */"3", dump_func_3, std::placeholders::_1); - pool.waitForTask("1", "", out_fd_.get()); - pool.waitForTask("2", "", out_fd_.get()); - pool.waitForTask("3", "", out_fd_.get()); + dump_pool_->waitForTask("1", "", out_fd_.get()); + dump_pool_->waitForTask("2", "", out_fd_.get()); + dump_pool_->waitForTask("3", "", out_fd_.get()); + dump_pool_->shutdown(); std::string result; ReadFileToString(out_path_, &result); EXPECT_THAT(result, StrEq("A\nB\nC\n")); EXPECT_THAT(getTempFileCounts(kTestDataPath), Eq(0)); - pool.shutdown(); +} + +TEST_F(DumpPoolTest, EnqueueTask_withDurationLog) { + bool run_1 = false; + auto dump_func_1 = [&]() { + run_1 = true; + }; + + dump_pool_->enqueueTask(/* task_name = */"1", dump_func_1); + dump_pool_->waitForTask("1", "", out_fd_.get()); + dump_pool_->shutdown(); + + std::string result; + ReadFileToString(out_path_, &result); + EXPECT_TRUE(run_1); + EXPECT_THAT(result, StrEq("------ 0.000s was the duration of '1' ------\n")); + EXPECT_THAT(getTempFileCounts(kTestDataPath), Eq(0)); } -- cgit v1.2.3-59-g8ed1b From 3c2fdbd82090e0c1c3996c5f5091df309592699c Mon Sep 17 00:00:00 2001 From: Rhed Jao Date: Mon, 20 Jul 2020 17:46:29 +0800 Subject: Faster bugreports (3/n) Post 'DumpHals', 'DumpBoard' and 'DumpIncidentReport' to the thread pool. 30% is improved compared with single thread run. Bug: 136262402 Test: atest dumpstate_test Test: atest dumpstate_smoke_test Test: atest BugreportManagerTestCases Test: atest CtsBugreportTestCases Test: Manual enable and disable parallel run Test: Manual trigger report using hardware key and shortcut Change-Id: Iea3a6e676f05149a3f2cfc1fcd475c70ea253d08 --- cmds/dumpstate/Android.bp | 3 + cmds/dumpstate/TaskQueue.cpp | 40 ++++++++++ cmds/dumpstate/TaskQueue.h | 76 ++++++++++++++++++ cmds/dumpstate/dumpstate.cpp | 132 ++++++++++++++++++++++++-------- cmds/dumpstate/dumpstate.h | 28 ++++++- cmds/dumpstate/tests/dumpstate_test.cpp | 54 +++++++++++++ 6 files changed, 301 insertions(+), 32 deletions(-) create mode 100644 cmds/dumpstate/TaskQueue.cpp create mode 100644 cmds/dumpstate/TaskQueue.h (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/Android.bp b/cmds/dumpstate/Android.bp index 1f055f3be7..80d14ac3c4 100644 --- a/cmds/dumpstate/Android.bp +++ b/cmds/dumpstate/Android.bp @@ -106,6 +106,7 @@ cc_binary { defaults: ["dumpstate_defaults"], srcs: [ "DumpPool.cpp", + "TaskQueue.cpp", "dumpstate.cpp", "main.cpp", ], @@ -134,6 +135,7 @@ cc_test { defaults: ["dumpstate_defaults"], srcs: [ "DumpPool.cpp", + "TaskQueue.cpp", "dumpstate.cpp", "tests/dumpstate_test.cpp", ], @@ -151,6 +153,7 @@ cc_test { defaults: ["dumpstate_defaults"], srcs: [ "DumpPool.cpp", + "TaskQueue.cpp", "dumpstate.cpp", "tests/dumpstate_smoke_test.cpp", ], diff --git a/cmds/dumpstate/TaskQueue.cpp b/cmds/dumpstate/TaskQueue.cpp new file mode 100644 index 0000000000..8550aec4cc --- /dev/null +++ b/cmds/dumpstate/TaskQueue.cpp @@ -0,0 +1,40 @@ +/* + * Copyright (C) 2020 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. + */ + +#include "TaskQueue.h" + +namespace android { +namespace os { +namespace dumpstate { + +TaskQueue::~TaskQueue() { + run(/* do_cancel = */true); +} + +void TaskQueue::run(bool do_cancel) { + std::unique_lock lock(lock_); + while (!tasks_.empty()) { + auto task = tasks_.front(); + tasks_.pop(); + lock.unlock(); + std::invoke(task, do_cancel); + lock.lock(); + } +} + +} // namespace dumpstate +} // namespace os +} // namespace android diff --git a/cmds/dumpstate/TaskQueue.h b/cmds/dumpstate/TaskQueue.h new file mode 100644 index 0000000000..b7e72f1c26 --- /dev/null +++ b/cmds/dumpstate/TaskQueue.h @@ -0,0 +1,76 @@ +/* + * Copyright (C) 2020 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. + */ + +#ifndef FRAMEWORK_NATIVE_CMD_TASKQUEUE_H_ +#define FRAMEWORK_NATIVE_CMD_TASKQUEUE_H_ + +#include +#include + +#include + +namespace android { +namespace os { +namespace dumpstate { + +/* + * A task queue for dumpstate to collect tasks such as adding file to the zip + * which are needed to run in a single thread. The task is a callable function + * included a cancel task boolean parameter. The TaskQueue could + * cancel the task in the destructor if the task has never been called. + */ +class TaskQueue { + public: + TaskQueue() = default; + ~TaskQueue(); + + /* + * Adds a task into the queue. + * + * |f| Callable function to execute the task. The function must include a + * boolean parameter for TaskQueue to notify whether the task is + * cancelled or not. + * |args| A list of arguments. + */ + template void add(F&& f, Args&&... args) { + auto func = std::bind(std::forward(f), std::forward(args)...); + std::unique_lock lock(lock_); + tasks_.emplace([=](bool cancelled) { + std::invoke(func, cancelled); + }); + } + + /* + * Invokes all tasks in the task queue. + * + * |do_cancel| true to cancel all tasks in the queue. + */ + void run(bool do_cancel); + + private: + using Task = std::function; + + std::mutex lock_; + std::queue tasks_; + + DISALLOW_COPY_AND_ASSIGN(TaskQueue); +}; + +} // namespace dumpstate +} // namespace os +} // namespace android + +#endif //FRAMEWORK_NATIVE_CMD_TASKQUEUE_H_ diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index 7d195b4fa1..e267ce336b 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -116,6 +116,7 @@ using android::os::dumpstate::CommandOptions; using android::os::dumpstate::DumpFileToFd; using android::os::dumpstate::DumpPool; using android::os::dumpstate::PropertiesHelper; +using android::os::dumpstate::TaskQueue; // Keep in sync with // frameworks/base/services/core/java/com/android/server/am/ActivityManagerService.java @@ -128,8 +129,8 @@ static const int32_t WEIGHT_FILE = 5; static Dumpstate& ds = Dumpstate::GetInstance(); static int RunCommand(const std::string& title, const std::vector& full_command, const CommandOptions& options = CommandOptions::DEFAULT, - bool verbose_duration = false) { - return ds.RunCommand(title, full_command, options, verbose_duration); + bool verbose_duration = false, int out_fd = STDOUT_FILENO) { + return ds.RunCommand(title, full_command, options, verbose_duration, out_fd); } // Reasonable value for max stats. @@ -212,11 +213,19 @@ static const std::string ANR_FILE_PREFIX = "anr_"; RUN_SLOW_FUNCTION_AND_LOG(log_title, func_ptr, __VA_ARGS__); \ RETURN_IF_USER_DENIED_CONSENT(); +#define WAIT_TASK_WITH_CONSENT_CHECK(task_name, pool_ptr) \ + RETURN_IF_USER_DENIED_CONSENT(); \ + pool_ptr->waitForTask(task_name); \ + RETURN_IF_USER_DENIED_CONSENT(); + static const char* WAKE_LOCK_NAME = "dumpstate_wakelock"; // Names of parallel tasks, they are used for the DumpPool to identify the dump // task and the log title of the duration report. static const std::string DUMP_TRACES_TASK = "DUMP TRACES"; +static const std::string DUMP_INCIDENT_REPORT_TASK = "INCIDENT REPORT"; +static const std::string DUMP_HALS_TASK = "DUMP HALS"; +static const std::string DUMP_BOARD_TASK = "dumpstate_board()"; namespace android { namespace os { @@ -1014,7 +1023,6 @@ static void DumpIncidentReport() { MYLOGD("Not dumping incident report because it's not a zipped bugreport\n"); return; } - DurationReporter duration_reporter("INCIDENT REPORT"); const std::string path = ds.bugreport_internal_dir_ + "/tmp_incident_report"; auto fd = android::base::unique_fd(TEMP_FAILURE_RETRY(open(path.c_str(), O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC | O_NOFOLLOW, @@ -1029,9 +1037,11 @@ static void DumpIncidentReport() { // Use a different name from "incident.proto" // /proto/incident.proto is reserved for incident service dump // i.e. metadata for debugging. - ds.AddZipEntry(kProtoPath + "incident_report" + kProtoExt, path); + ds.EnqueueAddZipEntryAndCleanupIfNeeded(kProtoPath + "incident_report" + kProtoExt, + path); + } else { + unlink(path.c_str()); } - unlink(path.c_str()); } static void DumpVisibleWindowViews() { @@ -1326,15 +1336,21 @@ static Dumpstate::RunStatus RunDumpsysNormal() { /* timeout= */ 90s, /* service_timeout= */ 10s); } -static void DumpHals() { +/* + * |out_fd| A fd to support the DumpPool to output results to a temporary file. + * Dumpstate can pick up later and output to the bugreport. Using STDOUT_FILENO + * if it's not running in the parallel task. + */ +static void DumpHals(int out_fd = STDOUT_FILENO) { if (!ds.IsZipping()) { RunCommand("HARDWARE HALS", {"lshal", "--all", "--types=all", "--debug"}, - CommandOptions::WithTimeout(10).AsRootIfAvailable().Build()); + CommandOptions::WithTimeout(10).AsRootIfAvailable().Build(), + false, out_fd); return; } - DurationReporter duration_reporter("DUMP HALS"); RunCommand("HARDWARE HALS", {"lshal", "--all", "--types=all"}, - CommandOptions::WithTimeout(10).AsRootIfAvailable().Build()); + CommandOptions::WithTimeout(10).AsRootIfAvailable().Build(), + false, out_fd); using android::hidl::manager::V1_0::IServiceManager; using android::hardware::defaultServiceManager; @@ -1356,6 +1372,7 @@ static void DumpHals() { }, '_'); const std::string path = ds.bugreport_internal_dir_ + "/lshal_debug_" + cleanName; + bool empty = false; { auto fd = android::base::unique_fd( TEMP_FAILURE_RETRY(open(path.c_str(), @@ -1370,13 +1387,14 @@ static void DumpHals() { {"lshal", "debug", "-E", interface}, CommandOptions::WithTimeout(2).AsRootIfAvailable().Build()); - bool empty = 0 == lseek(fd, 0, SEEK_END); - if (!empty) { - ds.AddZipEntry("lshal-debug/" + cleanName + ".txt", path); - } + empty = 0 == lseek(fd, 0, SEEK_END); + } + if (!empty) { + ds.EnqueueAddZipEntryAndCleanupIfNeeded("lshal-debug/" + cleanName + ".txt", + path); + } else { + unlink(path.c_str()); } - - unlink(path.c_str()); } }); @@ -1471,6 +1489,17 @@ static void DumpstateLimitedOnly() { static Dumpstate::RunStatus dumpstate() { DurationReporter duration_reporter("DUMPSTATE"); + // Enqueue slow functions into the thread pool, if the parallel run is enabled. + if (ds.dump_pool_) { + // Pool was shutdown in DumpstateDefaultAfterCritical method in order to + // drop root user. Restarts it with two threads for the parallel run. + ds.dump_pool_->start(/* thread_counts = */2); + + ds.dump_pool_->enqueueTaskWithFd(DUMP_HALS_TASK, &DumpHals, _1); + ds.dump_pool_->enqueueTask(DUMP_INCIDENT_REPORT_TASK, &DumpIncidentReport); + ds.dump_pool_->enqueueTaskWithFd(DUMP_BOARD_TASK, &Dumpstate::DumpstateBoard, &ds, _1); + } + // Dump various things. Note that anything that takes "long" (i.e. several seconds) should // check intermittently (if it's intrerruptable like a foreach on pids) and/or should be wrapped // in a consent check (via RUN_SLOW_FUNCTION_WITH_CONSENT_CHECK). @@ -1502,7 +1531,11 @@ static Dumpstate::RunStatus dumpstate() { RUN_SLOW_FUNCTION_WITH_CONSENT_CHECK(RunCommand, "LIBRANK", {"librank"}, CommandOptions::AS_ROOT); - DumpHals(); + if (ds.dump_pool_) { + WAIT_TASK_WITH_CONSENT_CHECK(DUMP_HALS_TASK, ds.dump_pool_); + } else { + RUN_SLOW_FUNCTION_WITH_CONSENT_CHECK_AND_LOG(DUMP_HALS_TASK, DumpHals); + } RunCommand("PRINTENV", {"printenv"}); RunCommand("NETSTAT", {"netstat", "-nW"}); @@ -1583,7 +1616,11 @@ static Dumpstate::RunStatus dumpstate() { ds.AddDir(SNAPSHOTCTL_LOG_DIR, false); - RUN_SLOW_FUNCTION_WITH_CONSENT_CHECK(ds.DumpstateBoard); + if (ds.dump_pool_) { + WAIT_TASK_WITH_CONSENT_CHECK(DUMP_BOARD_TASK, ds.dump_pool_); + } else { + RUN_SLOW_FUNCTION_WITH_CONSENT_CHECK_AND_LOG(DUMP_BOARD_TASK, ds.DumpstateBoard); + } /* Migrate the ril_dumpstate to a device specific dumpstate? */ int rilDumpstateTimeout = android::base::GetIntProperty("ril.dumpstate.timeout", 0); @@ -1680,7 +1717,12 @@ static Dumpstate::RunStatus dumpstate() { // Add linker configuration directory ds.AddDir(LINKERCONFIG_DIR, true); - RUN_SLOW_FUNCTION_WITH_CONSENT_CHECK(DumpIncidentReport); + if (ds.dump_pool_) { + WAIT_TASK_WITH_CONSENT_CHECK(DUMP_INCIDENT_REPORT_TASK, ds.dump_pool_); + } else { + RUN_SLOW_FUNCTION_WITH_CONSENT_CHECK_AND_LOG(DUMP_INCIDENT_REPORT_TASK, + DumpIncidentReport); + } return Dumpstate::RunStatus::OK; } @@ -1799,7 +1841,8 @@ static void DumpstateRadioCommon(bool include_sensitive_info = true) { // Too broad for connectivity problems. DoKmsg(); // Contains unrelated hardware info (camera, NFC, biometrics, ...). - DumpHals(); + // TODO(b/136262402) Using thread pool for DumpHals + RUN_SLOW_FUNCTION_AND_LOG(DUMP_HALS_TASK, DumpHals); } DumpPacketStats(); @@ -2029,11 +2072,10 @@ Dumpstate::RunStatus Dumpstate::DumpTraces(const char** path) { return RunStatus::OK; } -void Dumpstate::DumpstateBoard() { - DurationReporter duration_reporter("dumpstate_board()"); - printf("========================================================\n"); - printf("== Board\n"); - printf("========================================================\n"); +void Dumpstate::DumpstateBoard(int out_fd) { + dprintf(out_fd, "========================================================\n"); + dprintf(out_fd, "== Board\n"); + dprintf(out_fd, "========================================================\n"); if (!IsZipping()) { MYLOGD("Not dumping board info because it's not a zipped bugreport\n"); @@ -2159,8 +2201,9 @@ void Dumpstate::DumpstateBoard() { MYLOGE("Ignoring empty %s\n", kDumpstateBoardFiles[i].c_str()); continue; } - AddZipEntry(kDumpstateBoardFiles[i], paths[i]); - printf("*** See %s entry ***\n", kDumpstateBoardFiles[i].c_str()); + remover[i].Disable(); + EnqueueAddZipEntryAndCleanupIfNeeded(kDumpstateBoardFiles[i], paths[i]); + dprintf(out_fd, "*** See %s entry ***\n", kDumpstateBoardFiles[i].c_str()); } } @@ -2190,6 +2233,11 @@ static void register_sig_handler() { } bool Dumpstate::FinishZipFile() { + // Runs all enqueued adding zip entry and cleanup tasks before finishing the zip file. + if (zip_entry_tasks_) { + zip_entry_tasks_->run(/* do_cancel = */false); + } + std::string entry_name = base_name_ + "-" + name_ + ".txt"; MYLOGD("Adding main entry (%s) from %s to .zip bugreport\n", entry_name.c_str(), tmp_path_.c_str()); @@ -2773,7 +2821,8 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, onUiIntensiveBugreportDumpsFinished(calling_uid, calling_package); MaybeCheckUserConsent(calling_uid, calling_package); DumpstateTelephonyOnly(calling_package); - DumpstateBoard(); + // TODO(b/136262402) Using thread pool for DumpstateBoard + RUN_SLOW_FUNCTION_AND_LOG(DUMP_BOARD_TASK, DumpstateBoard); } else if (options_->wifi_only) { MaybeTakeEarlyScreenshot(); onUiIntensiveBugreportDumpsFinished(calling_uid, calling_package); @@ -2936,6 +2985,7 @@ void Dumpstate::EnableParallelRunIfNeeded() { return; } dump_pool_ = std::make_unique(bugreport_internal_dir_); + zip_entry_tasks_ = std::make_unique(); } void Dumpstate::ShutdownDumpPool() { @@ -2943,6 +2993,27 @@ void Dumpstate::ShutdownDumpPool() { dump_pool_->shutdown(); dump_pool_ = nullptr; } + if (zip_entry_tasks_) { + zip_entry_tasks_->run(/* do_cancel = */true); + zip_entry_tasks_ = nullptr; + } +} + +void Dumpstate::EnqueueAddZipEntryAndCleanupIfNeeded(const std::string& entry_name, + const std::string& entry_path) { + auto func_add_zip_entry_and_cleanup = [=](bool task_cancelled) { + if (!task_cancelled) { + AddZipEntry(entry_name, entry_path); + } + android::os::UnlinkAndLogOnError(entry_path); + }; + if (zip_entry_tasks_) { + // Enqueues AddZipEntryAndCleanup function if the parallel run is enabled. + zip_entry_tasks_->add(func_add_zip_entry_and_cleanup, _1); + } else { + // Invokes AddZipEntryAndCleanup immediately + std::invoke(func_add_zip_entry_and_cleanup, /* task_cancelled = */false); + } } Dumpstate::RunStatus Dumpstate::HandleUserConsentDenied() { @@ -3666,10 +3737,11 @@ int dump_file_from_fd(const char *title, const char *path, int fd) { } int Dumpstate::RunCommand(const std::string& title, const std::vector& full_command, - const CommandOptions& options, bool verbose_duration) { - DurationReporter duration_reporter(title, false /* logcat_only */, verbose_duration); + const CommandOptions& options, bool verbose_duration, int out_fd) { + DurationReporter duration_reporter(title, false /* logcat_only */, + verbose_duration, out_fd); - int status = RunCommandToFd(STDOUT_FILENO, title, full_command, options); + int status = RunCommandToFd(out_fd, title, full_command, options); /* TODO: for now we're simplifying the progress calculation by using the * timeout as the weight. It's a good approximation for most cases, except when calling dumpsys, diff --git a/cmds/dumpstate/dumpstate.h b/cmds/dumpstate/dumpstate.h index d400dc7127..0f0927cc29 100644 --- a/cmds/dumpstate/dumpstate.h +++ b/cmds/dumpstate/dumpstate.h @@ -36,6 +36,7 @@ #include "DumpstateUtil.h" #include "DumpPool.h" +#include "TaskQueue.h" // Workaround for const char *args[MAX_ARGS_ARRAY_SIZE] variables until they're converted to // std::vector @@ -229,11 +230,13 @@ class Dumpstate { * |full_command| array containing the command (first entry) and its arguments. * Must contain at least one element. * |options| optional argument defining the command's behavior. + * |out_fd| A fd to support the DumpPool to output results to a temporary + * file. Using STDOUT_FILENO if it's not running in the parallel task. */ int RunCommand(const std::string& title, const std::vector& fullCommand, const android::os::dumpstate::CommandOptions& options = android::os::dumpstate::CommandOptions::DEFAULT, - bool verbose_duration = false); + bool verbose_duration = false, int out_fd = STDOUT_FILENO); /* * Runs `dumpsys` with the given arguments, automatically setting its timeout @@ -306,7 +309,12 @@ class Dumpstate { // Returns OK in all other cases. RunStatus DumpTraces(const char** path); - void DumpstateBoard(); + /* + * |out_fd| A fd to support the DumpPool to output results to a temporary file. + * Dumpstate can pick up later and output to the bugreport. Using STDOUT_FILENO + * if it's not running in the parallel task. + */ + void DumpstateBoard(int out_fd = STDOUT_FILENO); /* * Updates the overall progress of the bugreport generation by the given weight increment. @@ -362,6 +370,18 @@ class Dumpstate { */ bool CalledByApi() const; + /* + * Enqueues a task to the dumpstate's TaskQueue if the parallel run is enabled, + * otherwise invokes it immediately. The task adds file at path entry_path + * as a zip file entry with name entry_name. Unlinks entry_path when done. + * + * All enqueued tasks will be executed in the dumpstate's FinishZipFile method + * before the zip file is finished. Tasks will be cancelled in dumpstate's + * ShutdownDumpPool method if they have never been called. + */ + void EnqueueAddZipEntryAndCleanupIfNeeded(const std::string& entry_name, + const std::string& entry_path); + /* * Structure to hold options that determine the behavior of dumpstate. */ @@ -495,6 +515,10 @@ class Dumpstate { // A thread pool to execute dump tasks simultaneously if the parallel run is enabled. std::unique_ptr dump_pool_; + // A task queue to collect adding zip entry tasks inside dump tasks if the + // parallel run is enabled. + std::unique_ptr zip_entry_tasks_; + // A callback to IncidentCompanion service, which checks user consent for sharing the // bugreport with the calling app. If the user has not responded yet to the dialog it will // be neither confirmed nor denied. diff --git a/cmds/dumpstate/tests/dumpstate_test.cpp b/cmds/dumpstate/tests/dumpstate_test.cpp index b3cb4349f3..65d6b9bc72 100644 --- a/cmds/dumpstate/tests/dumpstate_test.cpp +++ b/cmds/dumpstate/tests/dumpstate_test.cpp @@ -1026,6 +1026,7 @@ TEST_F(DumpstateTest, DumpPool_withOutputToFileAndParallelRunEnabled_notNull) { SetParallelRun(true); EnableParallelRunIfNeeded(); EXPECT_TRUE(ds.options_->OutputToFile()); + EXPECT_TRUE(ds.zip_entry_tasks_); EXPECT_TRUE(ds.dump_pool_); } @@ -1033,12 +1034,14 @@ TEST_F(DumpstateTest, DumpPool_withNotOutputToFile_isNull) { ds.options_->use_socket = true; EnableParallelRunIfNeeded(); EXPECT_FALSE(ds.options_->OutputToFile()); + EXPECT_FALSE(ds.zip_entry_tasks_); EXPECT_FALSE(ds.dump_pool_); } TEST_F(DumpstateTest, DumpPool_withParallelRunDisabled_isNull) { SetParallelRun(false); EnableParallelRunIfNeeded(); + EXPECT_FALSE(ds.zip_entry_tasks_); EXPECT_FALSE(ds.dump_pool_); } @@ -1749,6 +1752,57 @@ TEST_F(DumpPoolTest, EnqueueTask_withDurationLog) { EXPECT_THAT(getTempFileCounts(kTestDataPath), Eq(0)); } +class TaskQueueTest : public DumpstateBaseTest { +public: + void SetUp() { + DumpstateBaseTest::SetUp(); + } + + TaskQueue task_queue_; +}; + +TEST_F(TaskQueueTest, runTask) { + bool is_task1_run = false; + bool is_task2_run = false; + auto task_1 = [&](bool task_cancelled) { + if (task_cancelled) { + return; + } + is_task1_run = true; + }; + auto task_2 = [&](bool task_cancelled) { + if (task_cancelled) { + return; + } + is_task2_run = true; + }; + task_queue_.add(task_1, std::placeholders::_1); + task_queue_.add(task_2, std::placeholders::_1); + + task_queue_.run(/* do_cancel = */false); + + EXPECT_TRUE(is_task1_run); + EXPECT_TRUE(is_task2_run); +} + +TEST_F(TaskQueueTest, runTask_withCancelled) { + bool is_task1_cancelled = false; + bool is_task2_cancelled = false; + auto task_1 = [&](bool task_cancelled) { + is_task1_cancelled = task_cancelled; + }; + auto task_2 = [&](bool task_cancelled) { + is_task2_cancelled = task_cancelled; + }; + task_queue_.add(task_1, std::placeholders::_1); + task_queue_.add(task_2, std::placeholders::_1); + + task_queue_.run(/* do_cancel = */true); + + EXPECT_TRUE(is_task1_cancelled); + EXPECT_TRUE(is_task2_cancelled); +} + } // namespace dumpstate } // namespace os -- cgit v1.2.3-59-g8ed1b From bf63d8a54e4b8dbcb26fe55b50c64223a232c511 Mon Sep 17 00:00:00 2001 From: Rhed Jao Date: Tue, 21 Jul 2020 15:42:55 +0800 Subject: Faster bugreports (4/n) Makes update progress thread safe. Bug: 136262402 Test: atest dumpstate_test Test: atest dumpstate_smoke_test Change-Id: I1cb593d236b86122d19a5a6c11496a449e519d03 --- cmds/dumpstate/dumpstate.cpp | 6 +++++- cmds/dumpstate/dumpstate.h | 2 ++ 2 files changed, 7 insertions(+), 1 deletion(-) (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index e267ce336b..d7700acbc2 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -3880,12 +3880,16 @@ void dump_route_tables() { fclose(fp); } -// TODO: make this function thread safe if sections are generated in parallel. void Dumpstate::UpdateProgress(int32_t delta_sec) { if (progress_ == nullptr) { MYLOGE("UpdateProgress: progress_ not set\n"); return; } + // This function updates progress related members of the dumpstate and reports + // progress percentage to the bugreport client. Since it could be called by + // different dump tasks at the same time if the parallel run is enabled, a + // mutex lock is necessary here to synchronize the call. + std::lock_guard lock(mutex_); // Always update progess so stats can be tuned... progress_->Inc(delta_sec); diff --git a/cmds/dumpstate/dumpstate.h b/cmds/dumpstate/dumpstate.h index 0f0927cc29..75b3140b27 100644 --- a/cmds/dumpstate/dumpstate.h +++ b/cmds/dumpstate/dumpstate.h @@ -572,6 +572,8 @@ class Dumpstate { android::sp consent_callback_; + std::recursive_mutex mutex_; + DISALLOW_COPY_AND_ASSIGN(Dumpstate); }; -- cgit v1.2.3-59-g8ed1b From e017f988dfa7a1e2968894dba2bf22bf2e122ac6 Mon Sep 17 00:00:00 2001 From: Rhed Jao Date: Tue, 21 Jul 2020 17:58:41 +0800 Subject: Faster bugreports (5/n) Post 'DumpCheckins' and 'DumpAppInfos' to the thread pool. 10% is improved compared with single thread run. Bug: 136262402 Test: atest dumpstate_test Test: atest dumpstate_smoke_test Test: Manual enable and disable parallel run Test: Manual trigger report using hardware key Test: Manual trigger using bugreport shortcut Change-Id: I107b3de9d49309fbfa1256203bfe705ca6c7608d --- cmds/dumpstate/dumpstate.cpp | 143 +++++++++++++++++++++++++++---------------- cmds/dumpstate/dumpstate.h | 4 +- 2 files changed, 92 insertions(+), 55 deletions(-) (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index d7700acbc2..764d706e73 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -226,6 +226,8 @@ static const std::string DUMP_TRACES_TASK = "DUMP TRACES"; static const std::string DUMP_INCIDENT_REPORT_TASK = "INCIDENT REPORT"; static const std::string DUMP_HALS_TASK = "DUMP HALS"; static const std::string DUMP_BOARD_TASK = "dumpstate_board()"; +static const std::string DUMP_CHECKINS_TASK = "DUMP CHECKINS"; +static const std::string DUMP_APP_INFOS_TASK = "DUMP APP INFOS"; namespace android { namespace os { @@ -335,8 +337,12 @@ static bool CopyFileToFile(const std::string& input_file, const std::string& out static void RunDumpsys(const std::string& title, const std::vector& dumpsysArgs, const CommandOptions& options = Dumpstate::DEFAULT_DUMPSYS, - long dumpsysTimeoutMs = 0) { - return ds.RunDumpsys(title, dumpsysArgs, options, dumpsysTimeoutMs); + long dumpsysTimeoutMs = 0, int out_fd = STDOUT_FILENO) { + return ds.RunDumpsys(title, dumpsysArgs, options, dumpsysTimeoutMs, out_fd); +} +static void RunDumpsys(const std::string& title, const std::vector& dumpsysArgs, + int out_fd) { + return ds.RunDumpsys(title, dumpsysArgs, Dumpstate::DEFAULT_DUMPSYS, 0, out_fd); } static int DumpFile(const std::string& title, const std::string& path) { return ds.DumpFile(title, path); @@ -1482,6 +1488,73 @@ static void DumpstateLimitedOnly() { printf("========================================================\n"); } +/* + * |out_fd| A fd to support the DumpPool to output results to a temporary file. + * Dumpstate can pick up later and output to the bugreport. Using STDOUT_FILENO + * if it's not running in the parallel task. + */ +static void DumpCheckins(int out_fd = STDOUT_FILENO) { + dprintf(out_fd, "========================================================\n"); + dprintf(out_fd, "== Checkins\n"); + dprintf(out_fd, "========================================================\n"); + + RunDumpsys("CHECKIN BATTERYSTATS", {"batterystats", "-c"}, out_fd); + RunDumpsys("CHECKIN MEMINFO", {"meminfo", "--checkin"}, out_fd); + RunDumpsys("CHECKIN NETSTATS", {"netstats", "--checkin"}, out_fd); + RunDumpsys("CHECKIN PROCSTATS", {"procstats", "-c"}, out_fd); + RunDumpsys("CHECKIN USAGESTATS", {"usagestats", "-c"}, out_fd); + RunDumpsys("CHECKIN PACKAGE", {"package", "--checkin"}, out_fd); +} + +/* + * Runs dumpsys on activity service to dump all application activities, services + * and providers in the device. + * + * |out_fd| A fd to support the DumpPool to output results to a temporary file. + * Dumpstate can pick up later and output to the bugreport. Using STDOUT_FILENO + * if it's not running in the parallel task. + */ +static void DumpAppInfos(int out_fd = STDOUT_FILENO) { + dprintf(out_fd, "========================================================\n"); + dprintf(out_fd, "== Running Application Activities\n"); + dprintf(out_fd, "========================================================\n"); + + // The following dumpsys internally collects output from running apps, so it can take a long + // time. So let's extend the timeout. + + const CommandOptions DUMPSYS_COMPONENTS_OPTIONS = CommandOptions::WithTimeout(60).Build(); + + RunDumpsys("APP ACTIVITIES", {"activity", "-v", "all"}, DUMPSYS_COMPONENTS_OPTIONS, 0, out_fd); + + dprintf(out_fd, "========================================================\n"); + dprintf(out_fd, "== Running Application Services (platform)\n"); + dprintf(out_fd, "========================================================\n"); + + RunDumpsys("APP SERVICES PLATFORM", {"activity", "service", "all-platform-non-critical"}, + DUMPSYS_COMPONENTS_OPTIONS, 0, out_fd); + + dprintf(out_fd, "========================================================\n"); + dprintf(out_fd, "== Running Application Services (non-platform)\n"); + dprintf(out_fd, "========================================================\n"); + + RunDumpsys("APP SERVICES NON-PLATFORM", {"activity", "service", "all-non-platform"}, + DUMPSYS_COMPONENTS_OPTIONS, 0, out_fd); + + dprintf(out_fd, "========================================================\n"); + dprintf(out_fd, "== Running Application Providers (platform)\n"); + dprintf(out_fd, "========================================================\n"); + + RunDumpsys("APP PROVIDERS PLATFORM", {"activity", "provider", "all-platform"}, + DUMPSYS_COMPONENTS_OPTIONS, out_fd); + + dprintf(out_fd, "========================================================\n"); + dprintf(out_fd, "== Running Application Providers (non-platform)\n"); + dprintf(out_fd, "========================================================\n"); + + RunDumpsys("APP PROVIDERS NON-PLATFORM", {"activity", "provider", "all-non-platform"}, + DUMPSYS_COMPONENTS_OPTIONS, 0, out_fd); +} + // Dumps various things. Returns early with status USER_CONSENT_DENIED if user denies consent // via the consent they are shown. Ignores other errors that occur while running various // commands. The consent checking is currently done around long running tasks, which happen to @@ -1498,6 +1571,8 @@ static Dumpstate::RunStatus dumpstate() { ds.dump_pool_->enqueueTaskWithFd(DUMP_HALS_TASK, &DumpHals, _1); ds.dump_pool_->enqueueTask(DUMP_INCIDENT_REPORT_TASK, &DumpIncidentReport); ds.dump_pool_->enqueueTaskWithFd(DUMP_BOARD_TASK, &Dumpstate::DumpstateBoard, &ds, _1); + ds.dump_pool_->enqueueTaskWithFd(DUMP_CHECKINS_TASK, &DumpCheckins, _1); + ds.dump_pool_->enqueueTaskWithFd(DUMP_APP_INFOS_TASK, &DumpAppInfos, _1); } // Dump various things. Note that anything that takes "long" (i.e. several seconds) should @@ -1642,57 +1717,17 @@ static Dumpstate::RunStatus dumpstate() { RUN_SLOW_FUNCTION_WITH_CONSENT_CHECK(RunDumpsysNormal); - printf("========================================================\n"); - printf("== Checkins\n"); - printf("========================================================\n"); - - RunDumpsys("CHECKIN BATTERYSTATS", {"batterystats", "-c"}); - - RUN_SLOW_FUNCTION_WITH_CONSENT_CHECK(RunDumpsys, "CHECKIN MEMINFO", {"meminfo", "--checkin"}); - - RunDumpsys("CHECKIN NETSTATS", {"netstats", "--checkin"}); - RunDumpsys("CHECKIN PROCSTATS", {"procstats", "-c"}); - RunDumpsys("CHECKIN USAGESTATS", {"usagestats", "-c"}); - RunDumpsys("CHECKIN PACKAGE", {"package", "--checkin"}); - - printf("========================================================\n"); - printf("== Running Application Activities\n"); - printf("========================================================\n"); - - // The following dumpsys internally collects output from running apps, so it can take a long - // time. So let's extend the timeout. - - const CommandOptions DUMPSYS_COMPONENTS_OPTIONS = CommandOptions::WithTimeout(60).Build(); - - RunDumpsys("APP ACTIVITIES", {"activity", "-v", "all"}, DUMPSYS_COMPONENTS_OPTIONS); - - printf("========================================================\n"); - printf("== Running Application Services (platform)\n"); - printf("========================================================\n"); - - RunDumpsys("APP SERVICES PLATFORM", {"activity", "service", "all-platform-non-critical"}, - DUMPSYS_COMPONENTS_OPTIONS); - - printf("========================================================\n"); - printf("== Running Application Services (non-platform)\n"); - printf("========================================================\n"); - - RunDumpsys("APP SERVICES NON-PLATFORM", {"activity", "service", "all-non-platform"}, - DUMPSYS_COMPONENTS_OPTIONS); - - printf("========================================================\n"); - printf("== Running Application Providers (platform)\n"); - printf("========================================================\n"); - - RunDumpsys("APP PROVIDERS PLATFORM", {"activity", "provider", "all-platform"}, - DUMPSYS_COMPONENTS_OPTIONS); - - printf("========================================================\n"); - printf("== Running Application Providers (non-platform)\n"); - printf("========================================================\n"); + if (ds.dump_pool_) { + WAIT_TASK_WITH_CONSENT_CHECK(DUMP_CHECKINS_TASK, ds.dump_pool_); + } else { + RUN_SLOW_FUNCTION_WITH_CONSENT_CHECK_AND_LOG(DUMP_CHECKINS_TASK, DumpCheckins); + } - RunDumpsys("APP PROVIDERS NON-PLATFORM", {"activity", "provider", "all-non-platform"}, - DUMPSYS_COMPONENTS_OPTIONS); + if (ds.dump_pool_) { + WAIT_TASK_WITH_CONSENT_CHECK(DUMP_APP_INFOS_TASK, ds.dump_pool_); + } else { + RUN_SLOW_FUNCTION_WITH_CONSENT_CHECK_AND_LOG(DUMP_APP_INFOS_TASK, DumpAppInfos); + } printf("========================================================\n"); printf("== Dropbox crashes\n"); @@ -3753,11 +3788,11 @@ int Dumpstate::RunCommand(const std::string& title, const std::vector& dumpsys_args, - const CommandOptions& options, long dumpsysTimeoutMs) { + const CommandOptions& options, long dumpsysTimeoutMs, int out_fd) { long timeout_ms = dumpsysTimeoutMs > 0 ? dumpsysTimeoutMs : options.TimeoutInMs(); std::vector dumpsys = {"/system/bin/dumpsys", "-T", std::to_string(timeout_ms)}; dumpsys.insert(dumpsys.end(), dumpsys_args.begin(), dumpsys_args.end()); - RunCommand(title, dumpsys, options); + RunCommand(title, dumpsys, options, false, out_fd); } int open_socket(const char *service) { diff --git a/cmds/dumpstate/dumpstate.h b/cmds/dumpstate/dumpstate.h index 75b3140b27..030d2ec476 100644 --- a/cmds/dumpstate/dumpstate.h +++ b/cmds/dumpstate/dumpstate.h @@ -249,10 +249,12 @@ class Dumpstate { * |options| optional argument defining the command's behavior. * |dumpsys_timeout| when > 0, defines the value passed to `dumpsys -T` (otherwise it uses the * timeout from `options`) + * |out_fd| A fd to support the DumpPool to output results to a temporary + * file. Using STDOUT_FILENO if it's not running in the parallel task. */ void RunDumpsys(const std::string& title, const std::vector& dumpsys_args, const android::os::dumpstate::CommandOptions& options = DEFAULT_DUMPSYS, - long dumpsys_timeout_ms = 0); + long dumpsys_timeout_ms = 0, int out_fd = STDOUT_FILENO); /* * Prints the contents of a file. -- cgit v1.2.3-59-g8ed1b From b5685b33a856c839c6fb672d83b6a81c6dc1918e Mon Sep 17 00:00:00 2001 From: Rhed Jao Date: Fri, 14 Aug 2020 17:19:17 +0800 Subject: Faster bugreports (6/n) Posts 'DumpHals' and 'DumpBoard' to the thread pool in the telephony and wifi mode. A 20% speed is improved in for telephony mode, and 8% speed is improved in wifi mode. Bug: 136262402 Test: atest dumpstate_test Test: atest dumpstate_smoke_test Test: atest BugreportManagerTestCases Test: atest CtsBugreportTestCases Test: Manual enable and disable parallel run Test: Manual trigger report using hardware key and shortcut Change-Id: I85abef8a242abf07013cc4eacc342293898082e8 --- cmds/dumpstate/dumpstate.cpp | 63 +++++++++++++++++++++++++++++++++++--------- 1 file changed, 50 insertions(+), 13 deletions(-) (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index 764d706e73..f987c00d05 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -1852,32 +1852,39 @@ Dumpstate::RunStatus Dumpstate::DumpstateDefaultAfterCritical() { return status; } +// Common states for telephony and wifi which are needed to be collected before +// dumpstate drop the root user. +static void DumpstateRadioAsRoot() { + DumpIpTablesAsRoot(); + ds.AddDir(LOGPERSIST_DATA_DIR, false); +} + // This method collects common dumpsys for telephony and wifi. Typically, wifi // reports are fine to include all information, but telephony reports on user // builds need to strip some content (see DumpstateTelephonyOnly). static void DumpstateRadioCommon(bool include_sensitive_info = true) { - DumpIpTablesAsRoot(); - - ds.AddDir(LOGPERSIST_DATA_DIR, false); - - if (!DropRootUser()) { - return; - } - // We need to be picky about some stuff for telephony reports on user builds. if (!include_sensitive_info) { // Only dump the radio log buffer (other buffers and dumps contain too much unrelated info). DoRadioLogcat(); } else { + // DumpHals takes long time, post it to the another thread in the pool, + // if pool is available. + if (ds.dump_pool_) { + ds.dump_pool_->enqueueTaskWithFd(DUMP_HALS_TASK, &DumpHals, _1); + } // Contains various system properties and process startup info. do_dmesg(); // Logs other than the radio buffer may contain package/component names and potential PII. DoLogcat(); // Too broad for connectivity problems. DoKmsg(); - // Contains unrelated hardware info (camera, NFC, biometrics, ...). - // TODO(b/136262402) Using thread pool for DumpHals - RUN_SLOW_FUNCTION_AND_LOG(DUMP_HALS_TASK, DumpHals); + // DumpHals contains unrelated hardware info (camera, NFC, biometrics, ...). + if (ds.dump_pool_) { + ds.dump_pool_->waitForTask(DUMP_HALS_TASK); + } else { + RUN_SLOW_FUNCTION_AND_LOG(DUMP_HALS_TASK, DumpHals); + } } DumpPacketStats(); @@ -1901,6 +1908,21 @@ static void DumpstateTelephonyOnly(const std::string& calling_package) { const bool include_sensitive_info = !PropertiesHelper::IsUserBuild(); + DumpstateRadioAsRoot(); + if (!DropRootUser()) { + return; + } + + // Starts thread pool after the root user is dropped, and two additional threads + // are created for DumpHals in the DumpstateRadioCommon and DumpstateBoard. + if (ds.dump_pool_) { + ds.dump_pool_->start(/*thread_counts =*/2); + + // DumpstateBoard takes long time, post it to the another thread in the pool, + // if pool is available. + ds.dump_pool_->enqueueTaskWithFd(DUMP_BOARD_TASK, &Dumpstate::DumpstateBoard, &ds, _1); + } + DumpstateRadioCommon(include_sensitive_info); if (include_sensitive_info) { @@ -1977,12 +1999,29 @@ static void DumpstateTelephonyOnly(const std::string& calling_package) { printf("========================================================\n"); printf("== dumpstate: done (id %d)\n", ds.id_); printf("========================================================\n"); + + if (ds.dump_pool_) { + ds.dump_pool_->waitForTask(DUMP_BOARD_TASK); + } else { + RUN_SLOW_FUNCTION_AND_LOG(DUMP_BOARD_TASK, ds.DumpstateBoard); + } } // This method collects dumpsys for wifi debugging only static void DumpstateWifiOnly() { DurationReporter duration_reporter("DUMPSTATE"); + DumpstateRadioAsRoot(); + if (!DropRootUser()) { + return; + } + + // Starts thread pool after the root user is dropped. Only one additional + // thread is needed for DumpHals in the DumpstateRadioCommon. + if (ds.dump_pool_) { + ds.dump_pool_->start(/*thread_counts =*/1); + } + DumpstateRadioCommon(); printf("========================================================\n"); @@ -2856,8 +2895,6 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, onUiIntensiveBugreportDumpsFinished(calling_uid, calling_package); MaybeCheckUserConsent(calling_uid, calling_package); DumpstateTelephonyOnly(calling_package); - // TODO(b/136262402) Using thread pool for DumpstateBoard - RUN_SLOW_FUNCTION_AND_LOG(DUMP_BOARD_TASK, DumpstateBoard); } else if (options_->wifi_only) { MaybeTakeEarlyScreenshot(); onUiIntensiveBugreportDumpsFinished(calling_uid, calling_package); -- cgit v1.2.3-59-g8ed1b From 25f50e00ebe47ff9067c37da3b3919dcffb0b7ed Mon Sep 17 00:00:00 2001 From: Rhed Jao Date: Thu, 20 Aug 2020 00:10:32 +0800 Subject: Remove a redundant call of starting DumpstateService A new DumpstateService instance is created and published, when the start service is invoked. The member field 'ds_' in DumpstateService would be null, and a crash could happen when DumpstateService tried to access it. Bug: 164340819 Test: atest dumpstate_test Test: atest dumpstate_smoke_test Test: Manual cancel report via the notification Change-Id: Ib77f114414ed0f4f916bd74de151665d65fe4118 --- cmds/dumpstate/dumpstate.cpp | 15 ++------------- cmds/dumpstate/dumpstate.h | 1 - cmds/dumpstate/tests/dumpstate_test.cpp | 3 --- 3 files changed, 2 insertions(+), 17 deletions(-) (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index f987c00d05..d108e0028c 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -2515,7 +2515,6 @@ static void SetOptionsFromMode(Dumpstate::BugreportMode mode, Dumpstate::DumpOpt break; case Dumpstate::BugreportMode::BUGREPORT_INTERACTIVE: // Currently, the dumpstate binder is only used by Shell to update progress. - options->do_start_service = true; options->do_progress_updates = true; options->do_screenshot = is_screenshot_requested; options->dumpstate_hal_mode = DumpstateMode::INTERACTIVE; @@ -2527,7 +2526,6 @@ static void SetOptionsFromMode(Dumpstate::BugreportMode mode, Dumpstate::DumpOpt options->dumpstate_hal_mode = DumpstateMode::REMOTE; break; case Dumpstate::BugreportMode::BUGREPORT_WEAR: - options->do_start_service = true; options->do_progress_updates = true; options->do_zip_file = true; options->do_screenshot = is_screenshot_requested; @@ -2554,12 +2552,12 @@ static void SetOptionsFromMode(Dumpstate::BugreportMode mode, Dumpstate::DumpOpt static void LogDumpOptions(const Dumpstate::DumpOptions& options) { MYLOGI( "do_zip_file: %d do_vibrate: %d use_socket: %d use_control_socket: %d do_screenshot: %d " - "is_remote_mode: %d show_header_only: %d do_start_service: %d telephony_only: %d " + "is_remote_mode: %d show_header_only: %d telephony_only: %d " "wifi_only: %d do_progress_updates: %d fd: %d bugreport_mode: %s dumpstate_hal_mode: %s " "limited_only: %d args: %s\n", options.do_zip_file, options.do_vibrate, options.use_socket, options.use_control_socket, options.do_screenshot, options.is_remote_mode, options.show_header_only, - options.do_start_service, options.telephony_only, options.wifi_only, + options.telephony_only, options.wifi_only, options.do_progress_updates, options.bugreport_fd.get(), options.bugreport_mode.c_str(), toString(options.dumpstate_hal_mode).c_str(), options.limited_only, options.args.c_str()); } @@ -2775,15 +2773,6 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, register_sig_handler(); - // TODO(b/111441001): maybe skip if already started? - if (options_->do_start_service) { - MYLOGI("Starting 'dumpstate' service\n"); - android::status_t ret; - if ((ret = android::os::DumpstateService::Start()) != android::OK) { - MYLOGE("Unable to start DumpstateService: %d\n", ret); - } - } - if (PropertiesHelper::IsDryRun()) { MYLOGI("Running on dry-run mode (to disable it, call 'setprop dumpstate.dry_run false')\n"); } diff --git a/cmds/dumpstate/dumpstate.h b/cmds/dumpstate/dumpstate.h index 030d2ec476..9582c9df7b 100644 --- a/cmds/dumpstate/dumpstate.h +++ b/cmds/dumpstate/dumpstate.h @@ -398,7 +398,6 @@ class Dumpstate { bool is_screenshot_copied = false; bool is_remote_mode = false; bool show_header_only = false; - bool do_start_service = false; bool telephony_only = false; bool wifi_only = false; // Trimmed-down version of dumpstate to only include whitelisted logs. diff --git a/cmds/dumpstate/tests/dumpstate_test.cpp b/cmds/dumpstate/tests/dumpstate_test.cpp index 65d6b9bc72..6b9369239c 100644 --- a/cmds/dumpstate/tests/dumpstate_test.cpp +++ b/cmds/dumpstate/tests/dumpstate_test.cpp @@ -258,7 +258,6 @@ TEST_F(DumpOptionsTest, InitializeFullBugReport) { EXPECT_FALSE(options_.do_progress_updates); EXPECT_FALSE(options_.is_remote_mode); EXPECT_FALSE(options_.use_socket); - EXPECT_FALSE(options_.do_start_service); EXPECT_FALSE(options_.limited_only); } @@ -267,7 +266,6 @@ TEST_F(DumpOptionsTest, InitializeInteractiveBugReport) { EXPECT_TRUE(options_.do_add_date); EXPECT_TRUE(options_.do_zip_file); EXPECT_TRUE(options_.do_progress_updates); - EXPECT_TRUE(options_.do_start_service); EXPECT_TRUE(options_.do_screenshot); EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::INTERACTIVE); @@ -303,7 +301,6 @@ TEST_F(DumpOptionsTest, InitializeWearBugReport) { EXPECT_TRUE(options_.do_screenshot); EXPECT_TRUE(options_.do_zip_file); EXPECT_TRUE(options_.do_progress_updates); - EXPECT_TRUE(options_.do_start_service); EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::WEAR); // Other options retain default values -- cgit v1.2.3-59-g8ed1b From 0daac91e7b4b2b9a108be1f57dd77c9813abb90f Mon Sep 17 00:00:00 2001 From: Rhed Jao Date: Fri, 21 Aug 2020 14:48:20 +0800 Subject: Deletes temporary files from thread pool when report is cancel Bug: 163027506 Test: atest dumpstate_test Test: atest dumpstate_smoke_test Test: Manual cancel the bureport from notification. Change-Id: Ic4ab0531e0f598c2d2b820293cd2e6444bbf1abb --- cmds/dumpstate/DumpPool.cpp | 4 ++++ cmds/dumpstate/DumpPool.h | 5 +++++ cmds/dumpstate/dumpstate.cpp | 9 +++++++++ 3 files changed, 18 insertions(+) (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/DumpPool.cpp b/cmds/dumpstate/DumpPool.cpp index e174c8eff3..e15ac3fe82 100644 --- a/cmds/dumpstate/DumpPool.cpp +++ b/cmds/dumpstate/DumpPool.cpp @@ -100,6 +100,10 @@ void DumpPool::waitForTask(const std::string& task_name, const std::string& titl } } +void DumpPool::deleteTempFiles() { + deleteTempFiles(tmp_root_); +} + void DumpPool::setLogDuration(bool log_duration) { log_duration_ = log_duration; } diff --git a/cmds/dumpstate/DumpPool.h b/cmds/dumpstate/DumpPool.h index a4ea875541..0c3c2cc0d7 100644 --- a/cmds/dumpstate/DumpPool.h +++ b/cmds/dumpstate/DumpPool.h @@ -134,6 +134,11 @@ class DumpPool { */ void waitForTask(const std::string& task_name, const std::string& title, int out_fd); + /* + * Deletes temporary files created by DumpPool. + */ + void deleteTempFiles(); + static const std::string PREFIX_TMPFILE_NAME; private: diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index d108e0028c..c8277feb5b 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -2688,6 +2688,15 @@ void Dumpstate::Cancel() { } tombstone_data_.clear(); anr_data_.clear(); + + // Instead of shutdown the pool, we delete temporary files directly since + // shutdown blocking the call. + if (dump_pool_) { + dump_pool_->deleteTempFiles(); + } + if (zip_entry_tasks_) { + zip_entry_tasks_->run(/*do_cancel =*/ true); + } } /* -- cgit v1.2.3-59-g8ed1b From d3646e03eeeb368059ea19f4fffd3a802c620c55 Mon Sep 17 00:00:00 2001 From: Steven Moreland Date: Wed, 23 Sep 2020 17:26:33 +0000 Subject: dumpstate: more time for non-zipping lshal debug In a normal zipped bugreport, each individual HAL is given 2s to dump debug information. However, there may be dozens of HALs on a device. In order to dump all of this information when not zipping, increasing the timeout of debugging all of the HALs to 60s. Bug: 169025960 Test: adb bugreport > bug.txt Change-Id: If9e8169ff15842ea4ae4a9ff3d8a641216dc93bd --- cmds/dumpstate/dumpstate.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index c8277feb5b..91d5524062 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -1350,7 +1350,7 @@ static Dumpstate::RunStatus RunDumpsysNormal() { static void DumpHals(int out_fd = STDOUT_FILENO) { if (!ds.IsZipping()) { RunCommand("HARDWARE HALS", {"lshal", "--all", "--types=all", "--debug"}, - CommandOptions::WithTimeout(10).AsRootIfAvailable().Build(), + CommandOptions::WithTimeout(60).AsRootIfAvailable().Build(), false, out_fd); return; } -- cgit v1.2.3-59-g8ed1b From 51156967b9636e32e5d798529e44bd2be6a4f3d3 Mon Sep 17 00:00:00 2001 From: Lais Andrade Date: Mon, 22 Feb 2021 19:21:35 +0000 Subject: Use vibrator_service cmd for dumpstate The local service "vibrator" was replaced by the new "vibrator_manager". Fix: 180924179 Test: manual Change-Id: I9d3921af2b3b0c31767090c86f0a3350a6d45b1a --- cmds/dumpstate/dumpstate.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index 990aa53071..23986dd376 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -2417,7 +2417,9 @@ static void SendBroadcast(const std::string& action, const std::vector args = {"cmd", "vibrator_manager", "synced", "-f", "-d", "dumpstate", + "oneshot", std::to_string(duration_ms)}; + RunCommand("", args, CommandOptions::WithTimeout(10) .Log("Vibrate: '%s'\n") .Always() -- cgit v1.2.3-59-g8ed1b From ce64421d5ea57fdf269e98c021908f2eb2bfa6c1 Mon Sep 17 00:00:00 2001 From: Paul Chang Date: Tue, 11 May 2021 16:06:45 +0800 Subject: Rename the bugreport from .tmp to .zip when the user consent is unavailable BUG: 185825114 Test: Confirm that the bugreport is .zip when the user consent is unavailable Change-Id: I6fc28aa3e040152ed16a2807afcef3516ebbc210 --- cmds/dumpstate/dumpstate.cpp | 5 +++++ 1 file changed, 5 insertions(+) (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index 336c5a2b3d..527f3d74ea 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -3183,6 +3183,11 @@ Dumpstate::RunStatus Dumpstate::CopyBugreportIfUserConsented(int32_t calling_uid // Since we do not have user consent to share the bugreport it does not get // copied over to the calling app but remains in the internal directory from // where the user can manually pull it. + std::string final_path = GetPath(".zip"); + bool copy_succeeded = android::os::CopyFileToFile(path_, final_path); + if (copy_succeeded) { + android::os::UnlinkAndLogOnError(path_); + } return Dumpstate::RunStatus::USER_CONSENT_TIMED_OUT; } // Unknown result; must be a programming error. -- cgit v1.2.3-59-g8ed1b From 668ede4b6197f8ef467737cb6b2e1fa361c44bee Mon Sep 17 00:00:00 2001 From: Chiachang Wang Date: Mon, 17 May 2021 17:14:20 +0800 Subject: Run distinct command to dump HIGH connectivity The dumpsys design in connectivity was updated due to mainline modularization work. Module can only register with default priority which is NORMAL. RunDumpSysHigh will not be able to dump the priority high information in ConnectivityService. Thus, dump HIGH connectivity specifically to align with legacy behavior. Bug: 188387185 Test: adb bugreport Change-Id: I0b712143527b866d2d749e37aa3d7f0f85317b17 Merged-In: I0b712143527b866d2d749e37aa3d7f0f85317b17 --- cmds/dumpstate/dumpstate.cpp | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index 8ac4ff8f9c..25e6dc95c7 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -1696,6 +1696,12 @@ static Dumpstate::RunStatus dumpstate() { RUN_SLOW_FUNCTION_WITH_CONSENT_CHECK(RunDumpsysHigh); + // The dump mechanism in connectivity is refactored due to modularization work. Connectivity can + // only register with a default priority(NORMAL priority). Dumpstate has to call connectivity + // dump with priority parameters to dump high priority information. + RunDumpsys("SERVICE HIGH connectivity", {"connectivity", "--dump-priority", "HIGH"}, + CommandOptions::WithTimeout(10).Build()); + RunCommand("SYSTEM PROPERTIES", {"getprop"}); RunCommand("STORAGED IO INFO", {"storaged", "-u", "-p"}); -- cgit v1.2.3-59-g8ed1b From 2eedd41aaf09ecfbaa956289ff7e144c0a84d7b5 Mon Sep 17 00:00:00 2001 From: Li Li Date: Wed, 30 Jun 2021 15:11:53 -0700 Subject: Include freezer cgroupfs info in bugreport Bug: 192493418 Bug: 192512069 Test: adb bugreport Change-Id: Ia95d8b14062e65248a0f4d97fe70b5d1010a9299 --- cmds/dumpstate/dumpstate.cpp | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index 25e6dc95c7..4eb601dd01 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -176,6 +176,7 @@ void add_mountinfo(); #define LINKERCONFIG_DIR "/linkerconfig" #define PACKAGE_DEX_USE_LIST "/data/system/package-dex-usage.list" #define SYSTEM_TRACE_SNAPSHOT "/data/misc/perfetto-traces/bugreport/systrace.pftrace" +#define CGROUPFS_DIR "/sys/fs/cgroup" // TODO(narayan): Since this information has to be kept in sync // with tombstoned, we should just put it in a common header. @@ -1785,6 +1786,9 @@ static Dumpstate::RunStatus dumpstate() { // Add linker configuration directory ds.AddDir(LINKERCONFIG_DIR, true); + /* Dump cgroupfs */ + ds.AddDir(CGROUPFS_DIR, true); + if (ds.dump_pool_) { WAIT_TASK_WITH_CONSENT_CHECK(DUMP_INCIDENT_REPORT_TASK, ds.dump_pool_); } else { -- cgit v1.2.3-59-g8ed1b