From a3573abb32c561bcbdcd010e1f03da226421d695 Mon Sep 17 00:00:00 2001 From: Tri Vo Date: Tue, 28 Aug 2018 13:31:04 -0700 Subject: [PATCH] Add debugName parameter to acquireWakeLock(). Bug: 78888165 Test: SystemSuspendV1_0UnitTest Change-Id: Ia6570af63eb1eb01145bd7870c196a0eed91883a --- suspend/1.0/ISystemSuspend.hal | 5 ++- suspend/1.0/default/Android.bp | 36 ++++++++++++++- suspend/1.0/default/SystemSuspend.cpp | 36 +++++++++++++-- suspend/1.0/default/SystemSuspend.h | 10 ++++- suspend/1.0/default/SystemSuspendStats.proto | 26 +++++++++++ suspend/1.0/default/SystemSuspendUnitTest.cpp | 64 ++++++++++++++++++++++++--- 6 files changed, 165 insertions(+), 12 deletions(-) create mode 100644 suspend/1.0/default/SystemSuspendStats.proto diff --git a/suspend/1.0/ISystemSuspend.hal b/suspend/1.0/ISystemSuspend.hal index c9677e0..3452d14 100644 --- a/suspend/1.0/ISystemSuspend.hal +++ b/suspend/1.0/ISystemSuspend.hal @@ -31,7 +31,10 @@ interface ISystemSuspend { * device from suspending. This method must be able to be called * independently of enableAutosuspend(). * + * @param debugName debug string attached to the acquired IWakeLock. Wake + * lock names are not necessarily unique. + * * @return lock the interface for the created wake lock. */ - acquireWakeLock() generates (IWakeLock lock); + acquireWakeLock(string debugName) generates (IWakeLock lock); }; diff --git a/suspend/1.0/default/Android.bp b/suspend/1.0/default/Android.bp index 7959f4f..10842f5 100644 --- a/suspend/1.0/default/Android.bp +++ b/suspend/1.0/default/Android.bp @@ -30,15 +30,46 @@ cc_defaults { cpp_std: "c++17", } +cc_defaults { + name: "system_suspend_stats_defaults", + shared_libs: [ + "libprotobuf-cpp-full", + ], + static_libs: ["SystemSuspendStatsProto"], + cflags: [ + "-Wall", + "-Werror", + "-Wno-unused-parameter", + ], +} + +cc_library { + name: "SystemSuspendStatsProto", + srcs: [ + "SystemSuspendStats.proto", + ], + proto: { + export_proto_headers: true, + type: "full", + }, + cflags: [ + "-Wall", + "-Werror", + "-Wno-unused-parameter", + ], +} + cc_binary { name: "android.system.suspend@1.0-service", relative_install_path: "hw", defaults: [ "system_suspend_defaults", + "system_suspend_stats_defaults", ], init_rc: ["android.system.suspend@1.0-service.rc"], vintf_fragments: ["android.system.suspend@1.0-service.xml"], shared_libs: ["android.system.suspend@1.0"], + static_libs: ["SystemSuspendStatsProto"], srcs: [ "SystemSuspend.cpp", "main.cpp", @@ -49,7 +80,10 @@ cc_binary { // Do *NOT* use for compliance with *TS. cc_test { name: "SystemSuspendV1_0UnitTest", - defaults: ["system_suspend_defaults"], + defaults: [ + "system_suspend_defaults", + "system_suspend_stats_defaults", + ], static_libs: ["android.system.suspend@1.0"], srcs: [ "SystemSuspend.cpp", diff --git a/suspend/1.0/default/SystemSuspend.cpp b/suspend/1.0/default/SystemSuspend.cpp index ba3dbeb..c27081f 100644 --- a/suspend/1.0/default/SystemSuspend.cpp +++ b/suspend/1.0/default/SystemSuspend.cpp @@ -19,7 +19,9 @@ #include #include #include +#include #include +#include #include #include @@ -30,6 +32,7 @@ using ::android::base::ReadFdToString; using ::android::base::WriteStringToFd; +using ::android::hardware::IPCThreadState; using ::android::hardware::Void; using ::std::string; @@ -50,12 +53,17 @@ string readFd(int fd) { return string{buf, static_cast(n)}; } +static inline int getCallingPid() { + return IPCThreadState::self()->getCallingPid(); +} + WakeLock::WakeLock(SystemSuspend* systemSuspend) : mSystemSuspend(systemSuspend) { mSystemSuspend->incSuspendCounter(); } WakeLock::~WakeLock() { mSystemSuspend->decSuspendCounter(); + mSystemSuspend->deleteWakeLockStatsEntry(reinterpret_cast(this)); } SystemSuspend::SystemSuspend(unique_fd wakeupCountFd, unique_fd stateFd) @@ -63,7 +71,8 @@ SystemSuspend::SystemSuspend(unique_fd wakeupCountFd, unique_fd stateFd) mCounterCondVar(), mSuspendCounter(0), mWakeupCountFd(std::move(wakeupCountFd)), - mStateFd(std::move(stateFd)) {} + mStateFd(std::move(stateFd)), + mStats() {} Return SystemSuspend::enableAutosuspend() { static bool initialized = false; @@ -77,8 +86,17 @@ Return SystemSuspend::enableAutosuspend() { return true; } -Return> SystemSuspend::acquireWakeLock() { - return new WakeLock{this}; +Return> SystemSuspend::acquireWakeLock(const hidl_string& name) { + IWakeLock* wl = new WakeLock{this}; + { + auto l = std::lock_guard(mStatsLock); + WakeLockStats wlStats{}; + wlStats.set_name(name); + wlStats.set_pid(getCallingPid()); + // Use WakeLock's address as a unique identifier. + (*mStats.mutable_wake_lock_stats())[reinterpret_cast(wl)] = wlStats; + } + return wl; } Return SystemSuspend::debug(const hidl_handle& handle, @@ -88,7 +106,12 @@ Return SystemSuspend::debug(const hidl_handle& handle, return Void(); } int fd = handle->data[0]; - WriteStringToFd(std::to_string(mSuspendCounter) + "\n", fd); + string debugStr; + { + auto l = std::lock_guard(mStatsLock); + google::protobuf::TextFormat::PrintToString(mStats, &debugStr); + } + WriteStringToFd(debugStr, fd); fsync(fd); return Void(); } @@ -105,6 +128,11 @@ void SystemSuspend::decSuspendCounter() { } } +void SystemSuspend::deleteWakeLockStatsEntry(uint64_t id) { + auto l = std::lock_guard(mStatsLock); + mStats.mutable_wake_lock_stats()->erase(id); +} + void SystemSuspend::initAutosuspend() { std::thread autosuspendThread([this] { while (true) { diff --git a/suspend/1.0/default/SystemSuspend.h b/suspend/1.0/default/SystemSuspend.h index a488bd0..5c148fd 100644 --- a/suspend/1.0/default/SystemSuspend.h +++ b/suspend/1.0/default/SystemSuspend.h @@ -19,6 +19,7 @@ #include #include +#include #include #include @@ -52,10 +53,11 @@ class SystemSuspend : public ISystemSuspend { public: SystemSuspend(unique_fd wakeupCountFd, unique_fd stateFd); Return enableAutosuspend() override; - Return> acquireWakeLock() override; + Return> acquireWakeLock(const hidl_string& name) override; Return debug(const hidl_handle& handle, const hidl_vec& options) override; void incSuspendCounter(); void decSuspendCounter(); + void deleteWakeLockStatsEntry(uint64_t id); private: void initAutosuspend(); @@ -65,6 +67,12 @@ class SystemSuspend : public ISystemSuspend { uint32_t mSuspendCounter; unique_fd mWakeupCountFd; unique_fd mStateFd; + + // mStats can be inconsistent with with mSuspendCounter since we use two separate locks to + // protect these. However, since mStats is only for debugging we prioritize performance. + // Never hold both locks at the same time to avoid deadlock. + std::mutex mStatsLock; + SystemSuspendStats mStats; }; } // namespace V1_0 diff --git a/suspend/1.0/default/SystemSuspendStats.proto b/suspend/1.0/default/SystemSuspendStats.proto new file mode 100644 index 0000000..76d6c0b --- /dev/null +++ b/suspend/1.0/default/SystemSuspendStats.proto @@ -0,0 +1,26 @@ +// Copyright 2018 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. + +syntax = "proto3"; + +// Collects information about individual WakeLock instances. +message WakeLockStats { + string name = 1; + int32 pid = 2; +} + +message SystemSuspendStats { + // Maps a unique id of a WakeLock instance to the corresponding WakeLockStats message. + map wake_lock_stats = 1; +} diff --git a/suspend/1.0/default/SystemSuspendUnitTest.cpp b/suspend/1.0/default/SystemSuspendUnitTest.cpp index 7d317e8..122fb02 100644 --- a/suspend/1.0/default/SystemSuspendUnitTest.cpp +++ b/suspend/1.0/default/SystemSuspendUnitTest.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include #include @@ -128,6 +129,28 @@ class SystemSuspendTest : public ::testing::Test { bool isSystemSuspendBlocked() { return isReadBlocked(stateFd); } + sp acquireWakeLock() { return suspendService->acquireWakeLock("TestLock"); } + + SystemSuspendStats getDebugDump() { + // Index 0 corresponds to the read end of the pipe; 1 to the write end. + int fds[2]; + pipe2(fds, O_NONBLOCK); + native_handle_t* handle = native_handle_create(1, 0); + handle->data[0] = fds[1]; + + suspendService->debug(handle, {}); + SystemSuspendStats stats{}; + google::protobuf::TextFormat::ParseFromString(readFd(fds[0]), &stats); + + native_handle_close(handle); + close(fds[0]); + close(fds[1]); + + return stats; + } + + size_t getWakeLockCount() { return getDebugDump().wake_lock_stats().size(); } + sp suspendService; int stateFd; int wakeupCountFd; @@ -152,7 +175,7 @@ TEST_F(SystemSuspendTest, AutosuspendLoop) { // Tests that upon WakeLock destruction SystemSuspend HAL is unblocked. TEST_F(SystemSuspendTest, WakeLockDestructor) { { - sp wl = suspendService->acquireWakeLock(); + sp wl = acquireWakeLock(); ASSERT_NE(wl, nullptr); unblockSystemSuspendFromWakeupCount(); ASSERT_TRUE(isSystemSuspendBlocked()); @@ -163,12 +186,12 @@ TEST_F(SystemSuspendTest, WakeLockDestructor) { // Tests that multiple WakeLocks correctly block SystemSuspend HAL. TEST_F(SystemSuspendTest, MultipleWakeLocks) { { - sp wl1 = suspendService->acquireWakeLock(); + sp wl1 = acquireWakeLock(); ASSERT_NE(wl1, nullptr); ASSERT_TRUE(isSystemSuspendBlocked()); unblockSystemSuspendFromWakeupCount(); { - sp wl2 = suspendService->acquireWakeLock(); + sp wl2 = acquireWakeLock(); ASSERT_NE(wl2, nullptr); ASSERT_TRUE(isSystemSuspendBlocked()); } @@ -180,7 +203,7 @@ TEST_F(SystemSuspendTest, MultipleWakeLocks) { // Tests that upon thread deallocation WakeLock is destructed and SystemSuspend HAL is unblocked. TEST_F(SystemSuspendTest, ThreadCleanup) { std::thread clientThread([this] { - sp wl = suspendService->acquireWakeLock(); + sp wl = acquireWakeLock(); ASSERT_NE(wl, nullptr); unblockSystemSuspendFromWakeupCount(); ASSERT_TRUE(isSystemSuspendBlocked()); @@ -194,7 +217,7 @@ TEST_F(SystemSuspendTest, ThreadCleanup) { TEST_F(SystemSuspendTest, CleanupOnAbort) { ASSERT_EXIT( { - sp wl = suspendService->acquireWakeLock(); + sp wl = acquireWakeLock(); ASSERT_NE(wl, nullptr); std::abort(); }, @@ -204,6 +227,37 @@ TEST_F(SystemSuspendTest, CleanupOnAbort) { ASSERT_FALSE(isSystemSuspendBlocked()); } +// Test that debug dump has correct information about acquired WakeLocks. +TEST_F(SystemSuspendTest, DebugDump) { + { + sp wl = suspendService->acquireWakeLock("TestLock"); + SystemSuspendStats debugDump = getDebugDump(); + ASSERT_EQ(debugDump.wake_lock_stats().size(), 1); + ASSERT_EQ(debugDump.wake_lock_stats().begin()->second.name(), "TestLock"); + } + ASSERT_EQ(getWakeLockCount(), 0); +} + +// Stress test acquiring/releasing WakeLocks. +TEST_F(SystemSuspendTest, WakeLockStressTest) { + // numThreads threads will acquire/release numLocks locks each. + constexpr int numThreads = 10; + constexpr int numLocks = 10000; + std::thread tds[numThreads]; + + for (int i = 0; i < numThreads; i++) { + tds[i] = std::thread([this] { + for (int i = 0; i < numLocks; i++) { + sp wl = acquireWakeLock(); + } + }); + } + for (int i = 0; i < numThreads; i++) { + tds[i].join(); + } + ASSERT_EQ(getWakeLockCount(), 0); +} + } // namespace android int main(int argc, char** argv) { -- 2.11.0