From f7c14104673750445bd1c2553021fd61aaeb7f2c Mon Sep 17 00:00:00 2001 From: Andy Hung Date: Sat, 18 Apr 2020 14:54:08 -0700 Subject: [PATCH] MediaMetrics: Add thread-safety checking Test: atest mediametrics_tests Bug: 70398235 Bug: 149850236 Change-Id: I288ef92e6444785e02043c2629355c229c14e85c --- media/libmediametrics/Android.bp | 6 ++-- services/mediametrics/AnalyticsActions.h | 3 +- services/mediametrics/Android.bp | 1 + services/mediametrics/MediaMetricsService.cpp | 14 ++++---- services/mediametrics/MediaMetricsService.h | 22 ++++++------ services/mediametrics/TimeMachine.h | 36 ++++++++++++------- services/mediametrics/TransactionLog.h | 19 +++++----- 7 files changed, 57 insertions(+), 44 deletions(-) diff --git a/media/libmediametrics/Android.bp b/media/libmediametrics/Android.bp index 0cd5194f8f..03068c7fb0 100644 --- a/media/libmediametrics/Android.bp +++ b/media/libmediametrics/Android.bp @@ -22,9 +22,11 @@ cc_library_shared { export_include_dirs: ["include"], cflags: [ - "-Werror", - "-Wno-error=deprecated-declarations", "-Wall", + "-Werror", + "-Wextra", + "-Wthread-safety", + "-Wunreachable-code", ], sanitize: { diff --git a/services/mediametrics/AnalyticsActions.h b/services/mediametrics/AnalyticsActions.h index 5568c91577..015113472c 100644 --- a/services/mediametrics/AnalyticsActions.h +++ b/services/mediametrics/AnalyticsActions.h @@ -16,6 +16,7 @@ #pragma once +#include #include #include @@ -144,7 +145,7 @@ private: } mutable std::mutex mLock; - std::map mFilters; // GUARDED_BY mLock + std::map mFilters GUARDED_BY(mLock); }; } // namespace android::mediametrics diff --git a/services/mediametrics/Android.bp b/services/mediametrics/Android.bp index 58f3ea84b9..fb4022e49c 100644 --- a/services/mediametrics/Android.bp +++ b/services/mediametrics/Android.bp @@ -27,6 +27,7 @@ cc_binary { "-Wall", "-Werror", "-Wextra", + "-Wthread-safety", ], } diff --git a/services/mediametrics/MediaMetricsService.cpp b/services/mediametrics/MediaMetricsService.cpp index b9703ba6e7..4b84bea09b 100644 --- a/services/mediametrics/MediaMetricsService.cpp +++ b/services/mediametrics/MediaMetricsService.cpp @@ -282,8 +282,8 @@ status_t MediaMetricsService::dump(int fd, const Vector& args) } else { result.appendFormat("Dump of the %s process:\n", kServiceName); const char *prefixptr = prefix.size() > 0 ? prefix.c_str() : nullptr; - dumpHeaders_l(result, sinceNs, prefixptr); - dumpQueue_l(result, sinceNs, prefixptr); + dumpHeaders(result, sinceNs, prefixptr); + dumpQueue(result, sinceNs, prefixptr); // TODO: maybe consider a better way of dumping audio analytics info. const int32_t linesToDump = all ? INT32_MAX : 1000; @@ -312,7 +312,7 @@ status_t MediaMetricsService::dump(int fd, const Vector& args) } // dump headers -void MediaMetricsService::dumpHeaders_l(String8 &result, int64_t sinceNs, const char* prefix) +void MediaMetricsService::dumpHeaders(String8 &result, int64_t sinceNs, const char* prefix) { if (mediametrics::Item::isEnabled()) { result.append("Metrics gathering: enabled\n"); @@ -337,7 +337,7 @@ void MediaMetricsService::dumpHeaders_l(String8 &result, int64_t sinceNs, const } // TODO: should prefix be a set? -void MediaMetricsService::dumpQueue_l(String8 &result, int64_t sinceNs, const char* prefix) +void MediaMetricsService::dumpQueue(String8 &result, int64_t sinceNs, const char* prefix) { if (mItems.empty()) { result.append("empty\n"); @@ -364,7 +364,7 @@ void MediaMetricsService::dumpQueue_l(String8 &result, int64_t sinceNs, const ch // if item != NULL, it's the item we just inserted // true == more items eligible to be recovered -bool MediaMetricsService::expirations_l(const std::shared_ptr& item) +bool MediaMetricsService::expirations(const std::shared_ptr& item) { bool more = false; @@ -419,7 +419,7 @@ void MediaMetricsService::processExpirations() do { sleep(1); std::lock_guard _l(mLock); - more = expirations_l(nullptr); + more = expirations(nullptr); } while (more); } @@ -429,7 +429,7 @@ void MediaMetricsService::saveItem(const std::shared_ptr // IMediaMetricsService must include Vector, String16, Errors +#include #include #include #include @@ -81,12 +82,11 @@ private: bool isRateLimited(mediametrics::Item *) const; void saveItem(const std::shared_ptr& item); - // The following methods are GUARDED_BY(mLock) - bool expirations_l(const std::shared_ptr& item); + bool expirations(const std::shared_ptr& item) REQUIRES(mLock); // support for generating output - void dumpQueue_l(String8 &result, int64_t sinceNs, const char* prefix); - void dumpHeaders_l(String8 &result, int64_t sinceNs, const char* prefix); + void dumpQueue(String8 &result, int64_t sinceNs, const char* prefix) REQUIRES(mLock); + void dumpHeaders(String8 &result, int64_t sinceNs, const char* prefix) REQUIRES(mLock); // The following variables accessed without mLock @@ -102,22 +102,22 @@ private: mediautils::UidInfo mUidInfo; // mUidInfo can be accessed without lock (locked internally) - mediametrics::AudioAnalytics mAudioAnalytics; + mediametrics::AudioAnalytics mAudioAnalytics; // mAudioAnalytics is locked internally. std::mutex mLock; // statistics about our analytics - int64_t mItemsFinalized = 0; // GUARDED_BY(mLock) - int64_t mItemsDiscarded = 0; // GUARDED_BY(mLock) - int64_t mItemsDiscardedExpire = 0; // GUARDED_BY(mLock) - int64_t mItemsDiscardedCount = 0; // GUARDED_BY(mLock) + int64_t mItemsFinalized GUARDED_BY(mLock) = 0; + int64_t mItemsDiscarded GUARDED_BY(mLock) = 0; + int64_t mItemsDiscardedExpire GUARDED_BY(mLock) = 0; + int64_t mItemsDiscardedCount GUARDED_BY(mLock) = 0; // If we have a worker thread to garbage collect - std::future mExpireFuture; // GUARDED_BY(mLock) + std::future mExpireFuture GUARDED_BY(mLock); // Our item queue, generally (oldest at front) // TODO: Make separate class, use segmented queue, write lock only end. // Note: Another analytics module might have ownership of an item longer than the log. - std::deque> mItems; // GUARDED_BY(mLock) + std::deque> mItems GUARDED_BY(mLock); }; } // namespace android diff --git a/services/mediametrics/TimeMachine.h b/services/mediametrics/TimeMachine.h index 29adeae4d3..b1382334f8 100644 --- a/services/mediametrics/TimeMachine.h +++ b/services/mediametrics/TimeMachine.h @@ -23,6 +23,7 @@ #include #include +#include #include #include @@ -92,7 +93,8 @@ private: } template - status_t getValue(const std::string &property, T* value, int64_t time = 0) const { + status_t getValue(const std::string &property, T* value, int64_t time = 0) const + REQUIRES(mPseudoKeyHistoryLock) { if (time == 0) time = systemTime(SYSTEM_TIME_REALTIME); const auto tsptr = mPropertyMap.find(property); if (tsptr == mPropertyMap.end()) return BAD_VALUE; @@ -108,20 +110,22 @@ private: } template - status_t getValue(const std::string &property, T defaultValue, int64_t time = 0) const { + status_t getValue(const std::string &property, T defaultValue, int64_t time = 0) const + REQUIRES(mPseudoKeyHistoryLock){ T value; return getValue(property, &value, time) != NO_ERROR ? defaultValue : value; } void putProp( - const std::string &name, const mediametrics::Item::Prop &prop, int64_t time = 0) { + const std::string &name, const mediametrics::Item::Prop &prop, int64_t time = 0) + REQUIRES(mPseudoKeyHistoryLock) { //alternatively: prop.visit([&](auto value) { putValue(name, value, time); }); putValue(name, prop.get(), time); } template - void putValue(const std::string &property, - T&& e, int64_t time = 0) { + void putValue(const std::string &property, T&& e, int64_t time = 0) + REQUIRES(mPseudoKeyHistoryLock) { if (time == 0) time = systemTime(SYSTEM_TIME_REALTIME); mLastModificationTime = time; if (mPropertyMap.size() >= kKeyMaxProperties && @@ -144,7 +148,8 @@ private: } } - std::pair dump(int32_t lines, int64_t time) const { + std::pair dump(int32_t lines, int64_t time) const + REQUIRES(mPseudoKeyHistoryLock) { std::stringstream ss; int32_t ll = lines; for (auto& tsPair : mPropertyMap) { @@ -158,7 +163,9 @@ private: return { ss.str(), lines - ll }; } - int64_t getLastModificationTime() const { return mLastModificationTime; } + int64_t getLastModificationTime() const REQUIRES(mPseudoKeyHistoryLock) { + return mLastModificationTime; + } private: static std::string dump( @@ -267,7 +274,7 @@ public: if (it == mHistory.end()) { if (!isTrusted) return PERMISSION_DENIED; - (void)gc_l(garbage); + (void)gc(garbage); // no keylock needed here as we are sole owner // until placed on mHistory. @@ -435,7 +442,8 @@ public: private: // Obtains the lock for a KeyHistory. - std::mutex &getLockForKey(const std::string &key) const { + std::mutex &getLockForKey(const std::string &key) const + RETURN_CAPABILITY(mPseudoKeyHistoryLock) { return mKeyLocks[std::hash{}(key) % std::size(mKeyLocks)]; } @@ -459,7 +467,6 @@ private: return it->second; } - // GUARDED_BY mLock /** * Garbage collects if the TimeMachine size exceeds the high water mark. * @@ -471,7 +478,7 @@ private: * * \return true if garbage collection was done. */ - bool gc_l(std::vector& garbage) { + bool gc(std::vector& garbage) REQUIRES(mLock) { // TODO: something better than this for garbage collection. if (mHistory.size() < mKeyHighWaterMark) return false; @@ -540,12 +547,17 @@ private: */ mutable std::mutex mLock; // Lock for mHistory - History mHistory; // GUARDED_BY mLock + History mHistory GUARDED_BY(mLock); // KEY_LOCKS is the number of mutexes for keys. // It need not be a power of 2, but faster that way. static inline constexpr size_t KEY_LOCKS = 256; mutable std::mutex mKeyLocks[KEY_LOCKS]; // Hash-striped lock for KeyHistory based on key. + + // Used for thread-safety analysis, we create a fake mutex object to represent + // the hash stripe lock mechanism, which is then tracked by the compiler. + class CAPABILITY("mutex") PseudoLock {}; + static inline PseudoLock mPseudoKeyHistoryLock; }; } // namespace android::mediametrics diff --git a/services/mediametrics/TransactionLog.h b/services/mediametrics/TransactionLog.h index 4f09bb0857..d64acf39a9 100644 --- a/services/mediametrics/TransactionLog.h +++ b/services/mediametrics/TransactionLog.h @@ -21,6 +21,7 @@ #include #include +#include #include namespace android::mediametrics { @@ -92,7 +93,7 @@ public: std::vector garbage; // objects destroyed after lock. std::lock_guard lock(mLock); - (void)gc_l(garbage); + (void)gc(garbage); mLog.emplace(time, item); mItemMap[key].emplace(time, item); return NO_ERROR; // no errors for now. @@ -104,7 +105,7 @@ public: std::vector> get( int64_t startTime = 0, int64_t endTime = INT64_MAX) const { std::lock_guard lock(mLock); - return getItemsInRange_l(mLog, startTime, endTime); + return getItemsInRange(mLog, startTime, endTime); } /** @@ -116,7 +117,7 @@ public: std::lock_guard lock(mLock); auto mapIt = mItemMap.find(key); if (mapIt == mItemMap.end()) return {}; - return getItemsInRange_l(mapIt->second, startTime, endTime); + return getItemsInRange(mapIt->second, startTime, endTime); } /** @@ -204,7 +205,6 @@ private: return { ss.str(), lines - ll }; } - // GUARDED_BY mLock /** * Garbage collects if the TimeMachine size exceeds the high water mark. * @@ -213,7 +213,7 @@ private: * * \return true if garbage collection was done. */ - bool gc_l(std::vector& garbage) { + bool gc(std::vector& garbage) REQUIRES(mLock) { if (mLog.size() < mHighWaterMark) return false; ALOGD("%s: garbage collection", __func__); @@ -268,7 +268,7 @@ private: return true; } - static std::vector> getItemsInRange_l( + static std::vector> getItemsInRange( const MapTimeItem& map, int64_t startTime = 0, int64_t endTime = INT64_MAX) { auto it = map.lower_bound(startTime); @@ -289,11 +289,8 @@ private: mutable std::mutex mLock; - // GUARDED_BY mLock - MapTimeItem mLog; - - // GUARDED_BY mLock - std::map mItemMap; + MapTimeItem mLog GUARDED_BY(mLock); + std::map mItemMap GUARDED_BY(mLock); }; } // namespace android::mediametrics