From f18b774d3d4143dd5b0c23a281a998d397907652 Mon Sep 17 00:00:00 2001 From: Wu Shangwei <2826256824@qq.com> Date: Wed, 1 Dec 2021 11:58:42 +0800 Subject: [PATCH 1/2] Use scoped_lock to avoid deaklock Signed-off-by: Wu Shangwei <2826256824@qq.com> --- services/hilogd/include/log_buffer.h | 7 +- services/hilogd/log_buffer.cpp | 294 +++++++++++++-------------- services/hilogd/log_collector.cpp | 16 +- 3 files changed, 156 insertions(+), 161 deletions(-) diff --git a/services/hilogd/include/log_buffer.h b/services/hilogd/include/log_buffer.h index a0ad2dd..80ca9cc 100644 --- a/services/hilogd/include/log_buffer.h +++ b/services/hilogd/include/log_buffer.h @@ -21,7 +21,6 @@ #include #include #include -#include #include #include "log_reader.h" @@ -34,7 +33,7 @@ public: ~HilogBuffer(); std::vector> logReaderList; - std::shared_mutex logReaderListMutex; + std::mutex logReaderListMutex; size_t Insert(const HilogMsg& msg); bool Query(LogReader* reader); bool Query(std::shared_ptr reader); @@ -47,13 +46,11 @@ public: int32_t GetStatisticInfoByDomain(uint32_t domain, uint64_t& printLen, uint64_t& cacheLen, int32_t& dropped); int32_t ClearStatisticInfoByLog(uint16_t logType); int32_t ClearStatisticInfoByDomain(uint32_t domain); - void GetBufferLock(); - void ReleaseBufferLock(); private: size_t size; size_t sizeByType[LOG_TYPE_MAX]; std::list hilogDataList; - std::shared_mutex hilogBufferMutex; + std::mutex hilogBufferMutex; std::map cacheLenByDomain; std::map printLenByDomain; std::map droppedByDomain; diff --git a/services/hilogd/log_buffer.cpp b/services/hilogd/log_buffer.cpp index 7a4e683..3688534 100644 --- a/services/hilogd/log_buffer.cpp +++ b/services/hilogd/log_buffer.cpp @@ -62,36 +62,38 @@ size_t HilogBuffer::Insert(const HilogMsg& msg) // Delete old entries when full if (eleSize + sizeByType[msg.type] >= (size_t)g_maxBufferSizeByType[msg.type]) { - hilogBufferMutex.lock(); - // Drop 5% of maximum log when full - std::list::iterator it = hilogDataList.begin(); - while (sizeByType[msg.type] > g_maxBufferSizeByType[msg.type] * (1 - DROP_RATIO) && - it != hilogDataList.end()) { - if ((*it).type != msg.type) { // Only remove old logs of the same type - ++it; - continue; - } - logReaderListMutex.lock_shared(); - for (auto &itr :logReaderList) { - if (itr.lock()->readPos == it) { - itr.lock()->readPos = std::next(it); + { + std::scoped_lock lock(hilogBufferMutex); + // Drop 5% of maximum log when full + std::list::iterator it = hilogDataList.begin(); + while (sizeByType[msg.type] > g_maxBufferSizeByType[msg.type] * (1 - DROP_RATIO) && + it != hilogDataList.end()) { + if ((*it).type != msg.type) { // Only remove old logs of the same type + ++it; + continue; } - if (itr.lock()->lastPos == it) { - itr.lock()->lastPos = std::next(it); + { + std::scoped_lock readerListLock(logReaderListMutex); + for (auto &itr :logReaderList) { + if (itr.lock()->readPos == it) { + itr.lock()->readPos = std::next(it); + } + if (itr.lock()->lastPos == it) { + itr.lock()->lastPos = std::next(it); + } + } } + size_t cLen = it->len - it->tag_len; + size -= cLen; + sizeByType[(*it).type] -= cLen; + it = hilogDataList.erase(it); } - logReaderListMutex.unlock_shared(); - size_t cLen = it->len - it->tag_len; - size -= cLen; - sizeByType[(*it).type] -= cLen; - it = hilogDataList.erase(it); - } - // Re-confirm if enough elements has been removed - if (sizeByType[msg.type] >= (size_t)g_maxBufferSizeByType[msg.type]) { - std::cout << "Failed to clean old logs." << std::endl; + // Re-confirm if enough elements has been removed + if (sizeByType[msg.type] >= (size_t)g_maxBufferSizeByType[msg.type]) { + std::cout << "Failed to clean old logs." << std::endl; + } } - hilogBufferMutex.unlock(); } // Insert new log into HilogBuffer @@ -102,13 +104,14 @@ size_t HilogBuffer::Insert(const HilogMsg& msg) // Find the place with right timestamp ++rit; for (; rit != hilogDataList.rend() && msg.tv_sec < rit->tv_sec; ++rit) { - logReaderListMutex.lock_shared(); - for (auto &itr :logReaderList) { - if (itr.lock()->readPos == std::prev(rit.base())) { - itr.lock()->oldData.emplace_front(msg); + { + std::scoped_lock readerListLock(logReaderListMutex); + for (auto &itr :logReaderList) { + if (itr.lock()->readPos == std::prev(rit.base())) { + itr.lock()->oldData.emplace_front(msg); + } } } - logReaderListMutex.unlock_shared(); } hilogDataList.emplace(rit.base(), msg); } @@ -127,54 +130,53 @@ size_t HilogBuffer::Insert(const HilogMsg& msg) bool HilogBuffer::Query(std::shared_ptr reader) { - hilogBufferMutex.lock_shared(); - if (reader->GetReload()) { - reader->readPos = hilogDataList.begin(); - reader->lastPos = hilogDataList.begin(); - reader->SetReload(false); - } - - if (reader->isNotified) { - if (reader->readPos == hilogDataList.end()) { - reader->readPos = std::next(reader->lastPos); + { + std::scoped_lock lock(hilogBufferMutex); + if (reader->GetReload()) { + reader->readPos = hilogDataList.begin(); + reader->lastPos = hilogDataList.begin(); + reader->SetReload(false); } - } - // Look up in oldData first - if (!reader->oldData.empty()) { - reader->SetSendId(SENDIDA); - reader->WriteData(&(reader->oldData.back())); - printLenByType[(reader->oldData.back()).type] += strlen(reader->oldData.back().content); - if (printLenByDomain.count(reader->oldData.back().domain) == 0) { - printLenByDomain.insert(pair(reader->oldData.back().domain, - strlen(reader->oldData.back().content))); - } else { - printLenByDomain[reader->oldData.back().domain] += strlen(reader->oldData.back().content); + + if (reader->isNotified) { + if (reader->readPos == hilogDataList.end()) { + reader->readPos = std::next(reader->lastPos); + } } - reader->oldData.pop_back(); - hilogBufferMutex.unlock_shared(); - return true; - } - while (reader->readPos != hilogDataList.end()) { - reader->lastPos = reader->readPos; - if (ConditionMatch(reader)) { + // Look up in oldData first + if (!reader->oldData.empty()) { reader->SetSendId(SENDIDA); - reader->WriteData(&*(reader->readPos)); - printLenByType[reader->readPos->type] += strlen(reader->readPos->content); - if (printLenByDomain.count(reader->readPos->domain) == 0) { - printLenByDomain.insert(pair(reader->readPos->domain, - strlen(reader->readPos->content))); + reader->WriteData(&(reader->oldData.back())); + printLenByType[(reader->oldData.back()).type] += strlen(reader->oldData.back().content); + if (printLenByDomain.count(reader->oldData.back().domain) == 0) { + printLenByDomain.insert(pair(reader->oldData.back().domain, + strlen(reader->oldData.back().content))); } else { - printLenByDomain[reader->readPos->domain] += strlen(reader->readPos->content); + printLenByDomain[reader->oldData.back().domain] += strlen(reader->oldData.back().content); } - reader->readPos++; - hilogBufferMutex.unlock_shared(); + reader->oldData.pop_back(); return true; } - reader->readPos++; + while (reader->readPos != hilogDataList.end()) { + reader->lastPos = reader->readPos; + if (ConditionMatch(reader)) { + reader->SetSendId(SENDIDA); + reader->WriteData(&*(reader->readPos)); + printLenByType[reader->readPos->type] += strlen(reader->readPos->content); + if (printLenByDomain.count(reader->readPos->domain) == 0) { + printLenByDomain.insert(pair(reader->readPos->domain, + strlen(reader->readPos->content))); + } else { + printLenByDomain[reader->readPos->domain] += strlen(reader->readPos->content); + } + reader->readPos++; + return true; + } + reader->readPos++; + } + reader->isNotified = false; + ReturnNoLog(reader); } - reader->isNotified = false; - ReturnNoLog(reader); - hilogBufferMutex.unlock_shared(); return false; } @@ -184,60 +186,62 @@ size_t HilogBuffer::Delete(uint16_t logType) return ERR_LOG_TYPE_INVALID; } size_t sum = 0; - hilogBufferMutex.lock(); - std::list::iterator it = hilogDataList.begin(); + { + std::scoped_lock lock(hilogBufferMutex); + std::list::iterator it = hilogDataList.begin(); - // Delete logs corresponding to queryCondition - while (it != hilogDataList.end()) { - // Only remove old logs of the same type - if ((*it).type != logType) { - ++it; - continue; - } - // Delete corresponding logs - logReaderListMutex.lock_shared(); - for (auto itr = logReaderList.begin(); itr != logReaderList.end();) { - if ((*itr).lock()->readPos == it) { - (*itr).lock()->readPos = std::next(it); + // Delete logs corresponding to queryCondition + while (it != hilogDataList.end()) { + // Only remove old logs of the same type + if ((*it).type != logType) { + ++it; + continue; + } + // Delete corresponding logs + { + std::scoped_lock readerListLock(logReaderListMutex); + for (auto itr = logReaderList.begin(); itr != logReaderList.end();) { + if ((*itr).lock()->readPos == it) { + (*itr).lock()->readPos = std::next(it); + } + if ((*itr).lock()->lastPos == it) { + (*itr).lock()->lastPos = std::next(it); + } + ++itr; } - if ((*itr).lock()->lastPos == it) { - (*itr).lock()->lastPos = std::next(it); } - ++itr; + size_t cLen = it->len - it->tag_len; + sum += cLen; + sizeByType[(*it).type] -= cLen; + size -= cLen; + it = hilogDataList.erase(it); } - logReaderListMutex.unlock_shared(); - - size_t cLen = it->len - it->tag_len; - sum += cLen; - sizeByType[(*it).type] -= cLen; - size -= cLen; - it = hilogDataList.erase(it); } - - hilogBufferMutex.unlock(); return sum; } void HilogBuffer::AddLogReader(std::weak_ptr reader) { - logReaderListMutex.lock(); - // If reader not in logReaderList - logReaderList.push_back(reader); - reader.lock()->lastPos = hilogDataList.end(); - logReaderListMutex.unlock(); + { + std::scoped_lock readerListLock(logReaderListMutex); + // If reader not in logReaderList + logReaderList.push_back(reader); + reader.lock()->lastPos = hilogDataList.end(); + } } void HilogBuffer::RemoveLogReader(std::shared_ptr reader) { - logReaderListMutex.lock(); - const auto findIter = std::find_if(logReaderList.begin(), logReaderList.end(), - [&reader](const std::weak_ptr& ptr0) { - return ptr0.lock() == reader; - }); - if (findIter != logReaderList.end()) { - logReaderList.erase(findIter); + { + std::scoped_lock readerListLock(logReaderListMutex); + const auto findIter = std::find_if(logReaderList.begin(), logReaderList.end(), + [&reader](const std::weak_ptr& ptr0) { + return ptr0.lock() == reader; + }); + if (findIter != logReaderList.end()) { + logReaderList.erase(findIter); + } } - logReaderListMutex.unlock(); } bool HilogBuffer::Query(LogReader* reader) @@ -262,41 +266,43 @@ size_t HilogBuffer::SetBuffLen(uint16_t logType, uint64_t buffSize) if (buffSize <= 0 || buffSize > MAX_BUFFER_SIZE) { return ERR_BUFF_SIZE_INVALID; } - hilogBufferMutex.lock(); - if (sizeByType[logType] > buffSize) { - // Drop old log when buffsize not enough - std::list::iterator it = hilogDataList.begin(); - while (sizeByType[logType] > buffSize && it != hilogDataList.end()) { - if ((*it).type != logType) { // Only remove old logs of the same type - ++it; - continue; - } - logReaderListMutex.lock_shared(); - for (auto &itr :logReaderList) { - if (itr.lock()->readPos == it) { - itr.lock()->readPos = std::next(it); + { + std::scoped_lock lock(hilogBufferMutex); + if (sizeByType[logType] > buffSize) { + // Drop old log when buffsize not enough + std::list::iterator it = hilogDataList.begin(); + while (sizeByType[logType] > buffSize && it != hilogDataList.end()) { + if ((*it).type != logType) { // Only remove old logs of the same type + ++it; + continue; } - if (itr.lock()->lastPos == it) { - itr.lock()->lastPos = std::next(it); + { + std::scoped_lock readerListLock(logReaderListMutex); + for (auto &itr :logReaderList) { + if (itr.lock()->readPos == it) { + itr.lock()->readPos = std::next(it); + } + if (itr.lock()->lastPos == it) { + itr.lock()->lastPos = std::next(it); + } + } } + size_t cLen = it->len - it->tag_len; + size -= cLen; + sizeByType[(*it).type] -= cLen; + it = hilogDataList.erase(it); } - logReaderListMutex.unlock_shared(); - size_t cLen = it->len - it->tag_len; - size -= cLen; - sizeByType[(*it).type] -= cLen; - it = hilogDataList.erase(it); - } - // Re-confirm if enough elements has been removed - if (sizeByType[logType] > (size_t)g_maxBufferSizeByType[logType] || size > (size_t)g_maxBufferSize) { - return ERR_BUFF_SIZE_EXP; + // Re-confirm if enough elements has been removed + if (sizeByType[logType] > (size_t)g_maxBufferSizeByType[logType] || size > (size_t)g_maxBufferSize) { + return ERR_BUFF_SIZE_EXP; + } + g_maxBufferSizeByType[logType] = buffSize; + g_maxBufferSize += (buffSize - sizeByType[logType]); + } else { + g_maxBufferSizeByType[logType] = buffSize; + g_maxBufferSize += (buffSize - sizeByType[logType]); } - g_maxBufferSizeByType[logType] = buffSize; - g_maxBufferSize += (buffSize - sizeByType[logType]); - } else { - g_maxBufferSizeByType[logType] = buffSize; - g_maxBufferSize += (buffSize - sizeByType[logType]); } - hilogBufferMutex.unlock(); return buffSize; } @@ -416,15 +422,5 @@ void HilogBuffer::ReturnNoLog(std::shared_ptr reader) reader->SetSendId(SENDIDN); reader->WriteData(nullptr); } - -void HilogBuffer::GetBufferLock() -{ - hilogBufferMutex.lock(); -} - -void HilogBuffer::ReleaseBufferLock() -{ - hilogBufferMutex.unlock(); -} } // namespace HiviewDFX } // namespace OHOS diff --git a/services/hilogd/log_collector.cpp b/services/hilogd/log_collector.cpp index dfb974a..ba01c73 100644 --- a/services/hilogd/log_collector.cpp +++ b/services/hilogd/log_collector.cpp @@ -99,15 +99,17 @@ size_t LogCollector::InsertLogToBuffer(const HilogMsg& msg) if (result <= 0) { return result; } - m_hilogBuffer->logReaderListMutex.lock_shared(); - auto it = m_hilogBuffer->logReaderList.begin(); - while (it != m_hilogBuffer->logReaderList.end()) { - if ((*it).lock()->GetType() != TYPE_CONTROL) { - (*it).lock()->NotifyForNewData(); + + { + std::scoped_lock readerListLock(m_hilogBuffer->logReaderListMutex); + auto it = m_hilogBuffer->logReaderList.begin(); + while (it != m_hilogBuffer->logReaderList.end()) { + if ((*it).lock()->GetType() != TYPE_CONTROL) { + (*it).lock()->NotifyForNewData(); + } + ++it; } - ++it; } - m_hilogBuffer->logReaderListMutex.unlock_shared(); return result; } } // namespace HiviewDFX -- Gitee From ff3306b13836dbcf6a92e33d04cd98a046f41d19 Mon Sep 17 00:00:00 2001 From: Wu Shangwei <2826256824@qq.com> Date: Wed, 1 Dec 2021 12:50:32 +0800 Subject: [PATCH 2/2] Shared_lock for read-only situation Signed-off-by: Wu Shangwei <2826256824@qq.com> --- services/hilogd/include/log_buffer.h | 5 +++-- services/hilogd/log_buffer.cpp | 10 +++++----- services/hilogd/log_collector.cpp | 2 +- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/services/hilogd/include/log_buffer.h b/services/hilogd/include/log_buffer.h index 80ca9cc..e9d611f 100644 --- a/services/hilogd/include/log_buffer.h +++ b/services/hilogd/include/log_buffer.h @@ -21,6 +21,7 @@ #include #include #include +#include #include #include "log_reader.h" @@ -33,7 +34,7 @@ public: ~HilogBuffer(); std::vector> logReaderList; - std::mutex logReaderListMutex; + std::shared_mutex logReaderListMutex; size_t Insert(const HilogMsg& msg); bool Query(LogReader* reader); bool Query(std::shared_ptr reader); @@ -50,7 +51,7 @@ private: size_t size; size_t sizeByType[LOG_TYPE_MAX]; std::list hilogDataList; - std::mutex hilogBufferMutex; + std::shared_mutex hilogBufferMutex; std::map cacheLenByDomain; std::map printLenByDomain; std::map droppedByDomain; diff --git a/services/hilogd/log_buffer.cpp b/services/hilogd/log_buffer.cpp index 3688534..58ef43d 100644 --- a/services/hilogd/log_buffer.cpp +++ b/services/hilogd/log_buffer.cpp @@ -73,7 +73,7 @@ size_t HilogBuffer::Insert(const HilogMsg& msg) continue; } { - std::scoped_lock readerListLock(logReaderListMutex); + std::shared_lock readerListLock(logReaderListMutex); for (auto &itr :logReaderList) { if (itr.lock()->readPos == it) { itr.lock()->readPos = std::next(it); @@ -105,7 +105,7 @@ size_t HilogBuffer::Insert(const HilogMsg& msg) ++rit; for (; rit != hilogDataList.rend() && msg.tv_sec < rit->tv_sec; ++rit) { { - std::scoped_lock readerListLock(logReaderListMutex); + std::shared_lock readerListLock(logReaderListMutex); for (auto &itr :logReaderList) { if (itr.lock()->readPos == std::prev(rit.base())) { itr.lock()->oldData.emplace_front(msg); @@ -131,7 +131,7 @@ size_t HilogBuffer::Insert(const HilogMsg& msg) bool HilogBuffer::Query(std::shared_ptr reader) { { - std::scoped_lock lock(hilogBufferMutex); + std::shared_lock lock(hilogBufferMutex); if (reader->GetReload()) { reader->readPos = hilogDataList.begin(); reader->lastPos = hilogDataList.begin(); @@ -199,7 +199,7 @@ size_t HilogBuffer::Delete(uint16_t logType) } // Delete corresponding logs { - std::scoped_lock readerListLock(logReaderListMutex); + std::shared_lock readerListLock(logReaderListMutex); for (auto itr = logReaderList.begin(); itr != logReaderList.end();) { if ((*itr).lock()->readPos == it) { (*itr).lock()->readPos = std::next(it); @@ -277,7 +277,7 @@ size_t HilogBuffer::SetBuffLen(uint16_t logType, uint64_t buffSize) continue; } { - std::scoped_lock readerListLock(logReaderListMutex); + std::shared_lock readerListLock(logReaderListMutex); for (auto &itr :logReaderList) { if (itr.lock()->readPos == it) { itr.lock()->readPos = std::next(it); diff --git a/services/hilogd/log_collector.cpp b/services/hilogd/log_collector.cpp index ba01c73..11b6ee6 100644 --- a/services/hilogd/log_collector.cpp +++ b/services/hilogd/log_collector.cpp @@ -101,7 +101,7 @@ size_t LogCollector::InsertLogToBuffer(const HilogMsg& msg) } { - std::scoped_lock readerListLock(m_hilogBuffer->logReaderListMutex); + std::shared_lock readerListLock(m_hilogBuffer->logReaderListMutex); auto it = m_hilogBuffer->logReaderList.begin(); while (it != m_hilogBuffer->logReaderList.end()) { if ((*it).lock()->GetType() != TYPE_CONTROL) { -- Gitee