From 07f00fd438a0c10bc6b2487352d09eb0a648db40 Mon Sep 17 00:00:00 2001 From: Than McIntosh Date: Fri, 17 Apr 2015 15:10:43 -0400 Subject: [PATCH] Second round of changes to 'perf' profile collection daemon. Details: - avoid use of system() in favor of fork/exec. - add option to selectively disable/enable mpdecision service around perf collection runs to improve profile quality and avoid kernel pmuevents issues. - default to using 'simpleperf' instead of 'perf' Change-Id: I27928d8bb647fd852ec944158ebfd8efa38c01b4 --- perfprofd/Android.mk | 1 + perfprofd/cpuconfig.cc | 105 +++++++++++++++++++ perfprofd/cpuconfig.h | 50 +++++++++ perfprofd/perfprofdcore.cc | 185 +++++++++++++++++++++++++++------- perfprofd/tests/perfprofd_test.cc | 34 +++++-- perfprofd/tests/perfprofdmockutils.cc | 1 + 6 files changed, 330 insertions(+), 46 deletions(-) create mode 100644 perfprofd/cpuconfig.cc create mode 100644 perfprofd/cpuconfig.h diff --git a/perfprofd/Android.mk b/perfprofd/Android.mk index 4ae70317..3bacc81c 100644 --- a/perfprofd/Android.mk +++ b/perfprofd/Android.mk @@ -26,6 +26,7 @@ LOCAL_SRC_FILES := \ quipper/perf_reader.cc \ quipper/perf_parser.cc \ perf_data_converter.cc \ + cpuconfig.cc \ perfprofdcore.cc \ LOCAL_CPPFLAGS += $(perfprofd_cppflags) diff --git a/perfprofd/cpuconfig.cc b/perfprofd/cpuconfig.cc new file mode 100644 index 00000000..4b3cc36b --- /dev/null +++ b/perfprofd/cpuconfig.cc @@ -0,0 +1,105 @@ +/* +** +** Copyright 2015, 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 +#include +#include +#include +#include +#include +#include +#include + +#include + +#include "cpuconfig.h" +#include "perfprofdutils.h" + +#define SYSFSCPU "/sys/devices/system/cpu" + +HardwireCpuHelper::HardwireCpuHelper(bool perform) + : mpdecision_stopped_(false) +{ + if (perform && GetMpdecisionRunning()) { + mpdecision_stopped_ = true; + StopMpdecision(); + int ncores = GetNumCores(); + for (int i = 0; i < ncores; ++i) { + OnlineCore(i, 1); + } + } +} + +HardwireCpuHelper::~HardwireCpuHelper() +{ + if (mpdecision_stopped_) { + RestartMpdecision(); + } +} + +bool HardwireCpuHelper::GetMpdecisionRunning() +{ + char propBuf[PROPERTY_VALUE_MAX]; + property_get("init.svc.mpdecision", propBuf, ""); + return strcmp(propBuf, "running") == 0; +} + + +int HardwireCpuHelper::GetNumCores() +{ + int ncores = -1; + std::string possible(SYSFSCPU "/possible"); + FILE *fp = fopen(possible.c_str(), "re"); + if (fp) { + unsigned lo = 0, hi = 0; + if (fscanf(fp, "%u-%u", &lo, &hi) == 2) { + ncores = hi - lo + 1; + } + fclose(fp); + } + return ncores; +} + +void HardwireCpuHelper::OnlineCore(int i, int onoff) +{ + std::stringstream ss; + ss << SYSFSCPU "/cpu" << i << "/online"; + FILE *fp = fopen(ss.str().c_str(), "we"); + if (fp) { + fprintf(fp, onoff ? "1\n" : "0\n"); + fclose(fp); + } else { + W_ALOGW("open failed for %s", ss.str().c_str()); + } +} + +void HardwireCpuHelper::StopMpdecision() +{ + if (property_set("ctl.stop", "mpdecision")) { + W_ALOGE("setprop ctl.stop mpdecision failed"); + } +} + +void HardwireCpuHelper::RestartMpdecision() +{ + // Don't try to offline the cores we previously onlined -- let + // mpdecision figure out what to do + + if (property_set("ctl.start", "mpdecision")) { + W_ALOGE("setprop ctl.start mpdecision failed"); + } +} diff --git a/perfprofd/cpuconfig.h b/perfprofd/cpuconfig.h new file mode 100644 index 00000000..11a52f05 --- /dev/null +++ b/perfprofd/cpuconfig.h @@ -0,0 +1,50 @@ +/* +** +** Copyright 2015, 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. +*/ + +// +// Helper class to perform cpu setup (if needed) prior to a profile collection. +// +class HardwireCpuHelper { + public: + + // The constructor for this class checks to see if the 'mpdecision' + // service is running; if so (and if 'perform' is TRUE), then it + // disables the service and on-lines all of the available cores/cpus + // (anything listed in /sys/devices/system/cpu/possible). The + // destructor will re-enable the mpdecision service if it was + // previously disabled. + HardwireCpuHelper(bool perform); + virtual ~HardwireCpuHelper(); + + private: + bool mpdecision_stopped_; + + // Collect the number of available cpus/cores from /sys/devices/system/cpu/possible + int GetNumCores(); + + // Returns TRUE if the system service 'mpdecision' is running + bool GetMpdecisionRunning(); + + // Online/offline the specified cpu + void OnlineCore(int whichCore, int onoff); + + // Enable/disable the mpdecision service via the equivalent of + // setprop ctl.start mpdecision + // setprop ctl.stop mpdecision + void StopMpdecision(); + void RestartMpdecision(); +}; diff --git a/perfprofd/perfprofdcore.cc b/perfprofd/perfprofdcore.cc index 62cff186..797ba6f8 100644 --- a/perfprofd/perfprofdcore.cc +++ b/perfprofd/perfprofdcore.cc @@ -25,9 +25,11 @@ #include #include #include +#include #include #include #include +#include #include #include @@ -36,11 +38,12 @@ #include "perfprofdcore.h" #include "perfprofdutils.h" #include "perf_data_converter.h" +#include "cpuconfig.h" // // Perf profiling daemon -- collects system-wide profiles using // -// /system/bin/perf record -a +// simpleperf record -a // // and encodes them so that they can be uploaded by a separate service. // @@ -48,8 +51,7 @@ //...................................................................... // -// Output file from 'perf record'. The linux 'perf' tool by default -// creates a file with this name. +// Output file from 'perf record'. // #define PERF_OUTPUT "perf.data" @@ -119,8 +121,8 @@ class ConfigReader { ~ConfigReader(); // Ask for the current setting of a config item - unsigned getUnsignedValue(const char *key); - std::string getStringValue(const char *key); + unsigned getUnsignedValue(const char *key) const; + std::string getStringValue(const char *key) const; // read the specified config file, applying any settings it contains void readFile(const char *configFilePath); @@ -175,8 +177,8 @@ void ConfigReader::addDefaultEntries() addStringEntry("destination_directory", "/data/data/com.google.android.gms/files"); - // Path to 'perf' executable. - addStringEntry("perf_path", "/system/bin/perf"); + // Full path to 'perf' executable. + addStringEntry("perf_path", "/system/bin/simpleperf"); // Desired sampling period (passed to perf -c option). Small // sampling periods can perturb the collected profiles, so enforce @@ -192,7 +194,15 @@ void ConfigReader::addDefaultEntries() // Currently defaults to 1 (true). addUnsignedEntry("only_debug_build", 1, 0, 1); - // If set to 1, pass the -g option when invoking perf (requests + // If the "mpdecision" service is running at the point we are ready + // to kick off a profiling run, then temporarily disable the service + // and hard-wire all cores on prior to the collection run, provided + // that the duration of the recording is less than or equal to the value of + // 'hardwire_cpus_max_duration'. + addUnsignedEntry("hardwire_cpus", 1, 0, 1); + addUnsignedEntry("hardwire_cpus_max_duration", 5, 1, UINT32_MAX); + + // If set to 1, pass the -g option when invoking 'perf' (requests // stack traces as opposed to flat profile). addUnsignedEntry("stack_profile", 0, 0, 1); @@ -227,14 +237,14 @@ void ConfigReader::addStringEntry(const char *key, const char *default_value) W_ALOGE("internal error -- duplicate entry for key %s", key); exit(9); } - if (! default_value) { + if (default_value == nullptr) { W_ALOGE("internal error -- bad default value for key %s", key); exit(9); } s_entries[ks] = std::string(default_value); } -unsigned ConfigReader::getUnsignedValue(const char *key) +unsigned ConfigReader::getUnsignedValue(const char *key) const { std::string ks(key); auto it = u_entries.find(ks); @@ -242,7 +252,7 @@ unsigned ConfigReader::getUnsignedValue(const char *key) return it->second; } -std::string ConfigReader::getStringValue(const char *key) +std::string ConfigReader::getStringValue(const char *key) const { std::string ks(key); auto it = s_entries.find(ks); @@ -265,7 +275,7 @@ void ConfigReader::parseLine(const char *key, auto uit = u_entries.find(key); if (uit != u_entries.end()) { unsigned uvalue = 0; - if (! isdigit(value[0]) || sscanf(value, "%u", &uvalue) != 1) { + if (isdigit(value[0]) == 0 || sscanf(value, "%u", &uvalue) != 1) { W_ALOGW("line %d: malformed unsigned value (ignored)", linecount); } else { values vals; @@ -303,7 +313,7 @@ static bool isblank(const std::string &line) { for (std::string::const_iterator it = line.begin(); it != line.end(); ++it) { - if (! isspace(*it)) { + if (isspace(*it) == 0) { return false; } } @@ -384,7 +394,7 @@ static void parse_args(int argc, char** argv) // const char *ckprofile_result_to_string(CKPROFILE_RESULT result) { - switch(result) { + switch (result) { case DO_COLLECT_PROFILE: return "DO_COLLECT_PROFILE"; case DONT_PROFILE_MISSING_DESTINATION_DIR: @@ -474,7 +484,7 @@ static CKPROFILE_RESULT check_profiling_enabled(ConfigReader &config) // Reread aux config file -- it may have changed read_aux_config(config); - // Check for existence of perf executable + // Check for existence of simpleperf/perf executable std::string pp = config.getStringValue("perf_path"); if (access(pp.c_str(), R_OK|X_OK) == -1) { W_ALOGW("unable to access/execute %s", pp.c_str()); @@ -515,9 +525,6 @@ PROFILE_RESULT encode_to_proto(const std::string &data_file_path, const wireless_android_play_playlog::AndroidPerfProfile &encodedProfile = wireless_android_logging_awp::RawPerfDataToAndroidPerfProfile(data_file_path); - fprintf(stderr, "data file path is %s\n", data_file_path.c_str()); - fprintf(stderr, "encoded file path is %s\n", encoded_file_path.c_str()); - // // Issue error if no samples // @@ -525,7 +532,6 @@ PROFILE_RESULT encode_to_proto(const std::string &data_file_path, return ERR_PERF_ENCODE_FAILED; } - // // Serialize protobuf to array // @@ -554,6 +560,99 @@ PROFILE_RESULT encode_to_proto(const std::string &data_file_path, } // +// Invoke "perf record". Return value is OK_PROFILE_COLLECTION for +// success, or some other error code if something went wrong. +// +static PROFILE_RESULT invoke_perf(const std::string &perf_path, + unsigned sampling_period, + const char *stack_profile_opt, + unsigned duration, + const std::string &data_file_path, + const std::string &perf_stderr_path) +{ + pid_t pid = fork(); + + if (pid == -1) { + return ERR_FORK_FAILED; + } + + if (pid == 0) { + // child + + // Open file to receive stderr/stdout from perf + FILE *efp = fopen(perf_stderr_path.c_str(), "w"); + if (efp) { + dup2(fileno(efp), STDERR_FILENO); + dup2(fileno(efp), STDOUT_FILENO); + } else { + W_ALOGW("unable to open %s for writing", perf_stderr_path.c_str()); + } + + // marshall arguments + constexpr unsigned max_args = 12; + const char *argv[max_args]; + unsigned slot = 0; + argv[slot++] = perf_path.c_str(); + argv[slot++] = "record"; + + // -o perf.data + argv[slot++] = "-o"; + argv[slot++] = data_file_path.c_str(); + + // -c N + argv[slot++] = "-c"; + char pbuf[64]; snprintf(pbuf, 64, "%u", sampling_period); + argv[slot++] = pbuf; + + // -g if desired + if (stack_profile_opt) + argv[slot++] = stack_profile_opt; + + // system wide profiling + argv[slot++] = "-a"; + + // sleep + argv[slot++] = "/system/bin/sleep"; + char dbuf[64]; snprintf(dbuf, 64, "%u", duration); + argv[slot++] = dbuf; + + // terminator + argv[slot++] = nullptr; + assert(slot < max_args); + + // record the final command line in the error output file for + // posterity/debugging purposes + fprintf(stderr, "perf invocation (pid=%d):\n", getpid()); + for (unsigned i = 0; argv[i] != nullptr; ++i) { + fprintf(stderr, "%s%s", i ? " " : "", argv[i]); + } + fprintf(stderr, "\n"); + + // exec + execvp(argv[0], (char * const *)argv); + fprintf(stderr, "exec failed: %s\n", strerror(errno)); + exit(1); + + } else { + // parent + int st = 0; + pid_t reaped = TEMP_FAILURE_RETRY(waitpid(pid, &st, 0)); + + if (reaped == -1) { + W_ALOGW("waitpid failed: %s", strerror(errno)); + } else if (WIFSIGNALED(st)) { + W_ALOGW("perf killed by signal %d", WTERMSIG(st)); + } else if (WEXITSTATUS(st) != 0) { + W_ALOGW("perf bad exit status %d", WEXITSTATUS(st)); + } else { + return OK_PROFILE_COLLECTION; + } + } + + return ERR_PERF_RECORD_FAILED; +} + +// // Collect a perf profile. Steps for this operation are: // - kick off 'perf record' // - read perf.data, convert to protocol buf @@ -582,28 +681,44 @@ static PROFILE_RESULT collect_profile(ConfigReader &config) } // - // NB: it would probably be better to use explicit fork/exec with an - // alarm timeout in case of funny business. For now, call system(). + // The "mpdecision" daemon can cause problems for profile + // collection: if it decides to online a CPU partway through the + // 'perf record' run, the activity on that CPU will be invisible to + // perf, and if it offlines a CPU during the recording this can + // sometimes leave the PMU in an unusable state (dmesg errors of the + // form "perfevents: unable to request IRQXXX for ..."). To avoid + // these issues, if "mpdecision" is running the helper below will + // stop the service and then online all available CPUs. The object + // destructor (invoked when this routine terminates) will then + // restart the service again when needed. // - std::string perf_path = config.getStringValue("perf_path"); unsigned duration = config.getUnsignedValue("sample_duration"); + unsigned hardwire = config.getUnsignedValue("hardwire_cpus"); + unsigned max_duration = config.getUnsignedValue("hardwire_cpus_max_duration"); + bool take_action = (hardwire && duration <= max_duration); + HardwireCpuHelper helper(take_action); + + // + // Invoke perf + // + const char *stack_profile_opt = + (config.getUnsignedValue("stack_profile") != 0 ? "-g" : nullptr); + std::string perf_path = config.getStringValue("perf_path"); unsigned period = config.getUnsignedValue("sampling_period"); - const char *gopt = (config.getUnsignedValue("stack_profile") != 0 ? "-g" : ""); - char cmd[8192]; - snprintf(cmd, 8192, "%s record %s -c %u -o %s -a -- sleep %d 1> %s 2>&1 ", - perf_path.c_str(), gopt, period, data_file_path.c_str(), duration, - perf_stderr_path.c_str()); - int rc = system(cmd); - if (rc == -1) { - return ERR_FORK_FAILED; - } - if (rc != 0) { - return ERR_PERF_RECORD_FAILED; + + PROFILE_RESULT ret = invoke_perf(perf_path.c_str(), + period, + stack_profile_opt, + duration, + data_file_path, + perf_stderr_path); + if (ret != OK_PROFILE_COLLECTION) { + return ret; } // // Read the resulting perf.data file, encode into protocol buffer, then write - // the result to a *.pdb file + // the result to the file perf.data.encoded // std::string encoded_file_path(data_file_path); encoded_file_path += ".encoded"; @@ -705,7 +820,7 @@ int perfprofd_main(int argc, char** argv) } unsigned iterations = 0; - while(!config.getUnsignedValue("main_loop_iterations") || + while(config.getUnsignedValue("main_loop_iterations") == 0 || iterations < config.getUnsignedValue("main_loop_iterations")) { // Figure out where in the collection interval we're going to actually diff --git a/perfprofd/tests/perfprofd_test.cc b/perfprofd/tests/perfprofd_test.cc index 94c2814c..7e51e0d1 100644 --- a/perfprofd/tests/perfprofd_test.cc +++ b/perfprofd/tests/perfprofd_test.cc @@ -65,6 +65,7 @@ class PerfProfdTest : public testing::Test { virtual void SetUp() { mock_perfprofdutils_init(); create_dest_dir(); + yesclean(); } virtual void TearDown() { @@ -72,11 +73,24 @@ class PerfProfdTest : public testing::Test { remove_dest_dir(); } + void noclean() { + clean_ = false; + } + void yesclean() { + clean_ = true; + } + private: + bool clean_; void create_dest_dir() { setup_dirs(); ASSERT_FALSE(dest_dir == ""); + if (clean_) { + std::string cmd("rm -rf "); + cmd += dest_dir; + system(cmd.c_str()); + } std::string cmd("mkdir -p "); cmd += dest_dir; system(cmd.c_str()); @@ -85,9 +99,6 @@ class PerfProfdTest : public testing::Test { void remove_dest_dir() { setup_dirs(); ASSERT_FALSE(dest_dir == ""); - std::string cmd("rm -rf "); - cmd += dest_dir; - system(cmd.c_str()); } void setup_dirs() @@ -182,6 +193,7 @@ class PerfProfdRunner { writeConfigFile(aux_config_path_, aux_config_text_); } + // execute daemon main return perfprofd_main(3, (char **) argv); } @@ -348,9 +360,10 @@ TEST_F(PerfProfdTest, MissingOptInSemaphoreFile) TEST_F(PerfProfdTest, MissingPerfExecutable) { // - // AWP currently relies on the 'perf' tool to collect profiles (although - // this may conceivably change in the future). This test checks to make - // sure that if 'perf' is not present we bail out from collecting profiles. + // Perfprofd uses the 'simpleperf' tool to collect profiles + // (although this may conceivably change in the future). This test + // checks to make sure that if 'simpleperf' is not present we bail out + // from collecting profiles. // PerfProfdRunner runner; runner.addToConfig("only_debug_build=0"); @@ -383,10 +396,10 @@ TEST_F(PerfProfdTest, MissingPerfExecutable) TEST_F(PerfProfdTest, BadPerfRun) { // - // The linux 'perf' tool tends to be tightly coupled with a - // specific kernel version -- if things are out of sync perf could - // easily fail or crash. This test makes sure that we detect such a - // case and log the error. + // Perf tools tend to be tightly coupled with a specific kernel + // version -- if things are out of sync perf could fail or + // crash. This test makes sure that we detect such a case and log + // the error. // PerfProfdRunner runner; runner.addToConfig("only_debug_build=0"); @@ -593,7 +606,6 @@ TEST_F(PerfProfdTest, BasicRunWithLivePerf) runner.addToConfig("main_loop_iterations=1"); runner.addToConfig("use_fixed_seed=12345678"); runner.addToConfig("collection_interval=9999"); - runner.addToConfig("stack_profile=1"); runner.addToConfig("sample_duration=5"); // Create semaphore file diff --git a/perfprofd/tests/perfprofdmockutils.cc b/perfprofd/tests/perfprofdmockutils.cc index cc4ea94a..5af58c42 100644 --- a/perfprofd/tests/perfprofdmockutils.cc +++ b/perfprofd/tests/perfprofdmockutils.cc @@ -54,6 +54,7 @@ std::string mock_perfprofdutils_getlogged() for (const std::string &s : (*mock_log)) { result += s; } + mock_log->clear(); return result; } -- 2.11.0