From a863b1a04b972ed41fc99fb657e3f3fbbf7bb43a Mon Sep 17 00:00:00 2001 From: Tom Cherry Date: Wed, 10 Jun 2020 14:52:46 -0700 Subject: [PATCH] logd: simplify Clear() + Prune() logic Clear() and Prune() return a boolean indicating whether or not their operations failed because the log buffer was 'busy'. This means that they return false upon success and true upon failure, which is not intuitive. This change inverts their return value to simply be true if they were successful or false otherwise. It also simplifies the return value of ChattyLogBuffer::Prune() to true if the requested number of rows have been pruned or if all rows in the log buffer have been pruned, and otherwise return false. Test: logging unit tests Test: clearing works even under logging pressure Change-Id: I346bb945496ef62bf8e973298f81c5163f49bc57 --- logd/ChattyLogBuffer.cpp | 13 +++----- logd/CommandListener.cpp | 2 +- logd/SimpleLogBuffer.cpp | 68 ++++++++++++++++++---------------------- 3 files changed, 35 insertions(+), 48 deletions(-) diff --git a/logd/ChattyLogBuffer.cpp b/logd/ChattyLogBuffer.cpp index f92fe65c8..c21344866 100644 --- a/logd/ChattyLogBuffer.cpp +++ b/logd/ChattyLogBuffer.cpp @@ -327,7 +327,6 @@ class LogBufferElementLast { // bool ChattyLogBuffer::Prune(log_id_t id, unsigned long pruneRows, uid_t caller_uid) { LogReaderThread* oldest = nullptr; - bool busy = false; bool clearAll = pruneRows == ULONG_MAX; auto reader_threads_lock = std::lock_guard{reader_list()->reader_threads_lock()}; @@ -359,17 +358,16 @@ bool ChattyLogBuffer::Prune(log_id_t id, unsigned long pruneRows, uid_t caller_u } if (oldest && oldest->start() <= element.sequence()) { - busy = true; KickReader(oldest, id, pruneRows); - break; + return false; } it = Erase(it); if (--pruneRows == 0) { - break; + return true; } } - return busy; + return true; } // prune by worst offenders; by blacklist, UID, and by PID of system UID @@ -440,7 +438,6 @@ bool ChattyLogBuffer::Prune(log_id_t id, unsigned long pruneRows, uid_t caller_u LogBufferElement& element = *it; if (oldest && oldest->start() <= element.sequence()) { - busy = true; // Do not let chatty eliding trigger any reader mitigation break; } @@ -577,7 +574,6 @@ bool ChattyLogBuffer::Prune(log_id_t id, unsigned long pruneRows, uid_t caller_u } if (oldest && oldest->start() <= element.sequence()) { - busy = true; if (!whitelist) KickReader(oldest, id, pruneRows); break; } @@ -605,7 +601,6 @@ bool ChattyLogBuffer::Prune(log_id_t id, unsigned long pruneRows, uid_t caller_u } if (oldest && oldest->start() <= element.sequence()) { - busy = true; KickReader(oldest, id, pruneRows); break; } @@ -615,5 +610,5 @@ bool ChattyLogBuffer::Prune(log_id_t id, unsigned long pruneRows, uid_t caller_u } } - return (pruneRows > 0) && busy; + return pruneRows == 0 || it == logs().end(); } diff --git a/logd/CommandListener.cpp b/logd/CommandListener.cpp index 9764c443e..39c54904b 100644 --- a/logd/CommandListener.cpp +++ b/logd/CommandListener.cpp @@ -82,7 +82,7 @@ int CommandListener::ClearCmd::runCommand(SocketClient* cli, int argc, return 0; } - cli->sendMsg(buf()->Clear((log_id_t)id, uid) ? "busy" : "success"); + cli->sendMsg(buf()->Clear((log_id_t)id, uid) ? "success" : "busy"); return 0; } diff --git a/logd/SimpleLogBuffer.cpp b/logd/SimpleLogBuffer.cpp index 292a7e405..ec08d54d4 100644 --- a/logd/SimpleLogBuffer.cpp +++ b/logd/SimpleLogBuffer.cpp @@ -212,46 +212,38 @@ bool SimpleLogBuffer::FlushTo( return true; } -// clear all rows of type "id" from the buffer. bool SimpleLogBuffer::Clear(log_id_t id, uid_t uid) { - bool busy = true; - // If it takes more than 4 tries (seconds) to clear, then kill reader(s) - for (int retry = 4;;) { - if (retry == 1) { // last pass - // Check if it is still busy after the sleep, we say prune - // one entry, not another clear run, so we are looking for - // the quick side effect of the return value to tell us if - // we have a _blocked_ reader. - { - auto lock = std::lock_guard{lock_}; - busy = Prune(id, 1, uid); - } - // It is still busy, blocked reader(s), lets kill them all! - // otherwise, lets be a good citizen and preserve the slow - // readers and let the clear run (below) deal with determining - // if we are still blocked and return an error code to caller. - if (busy) { - auto reader_threads_lock = std::lock_guard{reader_list_->reader_threads_lock()}; - for (const auto& reader_thread : reader_list_->reader_threads()) { - if (reader_thread->IsWatching(id)) { - LOG(WARNING) << "Kicking blocked reader, " << reader_thread->name() - << ", from LogBuffer::clear()"; - reader_thread->release_Locked(); - } - } - } - } + // Try three times to clear, then disconnect the readers and try one final time. + for (int retry = 0; retry < 3; ++retry) { { auto lock = std::lock_guard{lock_}; - busy = Prune(id, ULONG_MAX, uid); + if (Prune(id, ULONG_MAX, uid)) { + return true; + } } - - if (!busy || !--retry) { - break; - } - sleep(1); // Let reader(s) catch up after notification + sleep(1); } - return busy; + // Check if it is still busy after the sleep, we try to prune one entry, not another clear run, + // so we are looking for the quick side effect of the return value to tell us if we have a + // _blocked_ reader. + bool busy = false; + { + auto lock = std::lock_guard{lock_}; + busy = !Prune(id, 1, uid); + } + // It is still busy, disconnect all readers. + if (busy) { + auto reader_threads_lock = std::lock_guard{reader_list_->reader_threads_lock()}; + for (const auto& reader_thread : reader_list_->reader_threads()) { + if (reader_thread->IsWatching(id)) { + LOG(WARNING) << "Kicking blocked reader, " << reader_thread->name() + << ", from LogBuffer::clear()"; + reader_thread->release_Locked(); + } + } + } + auto lock = std::lock_guard{lock_}; + return Prune(id, ULONG_MAX, uid); } // get the total space allocated to "id" @@ -313,16 +305,16 @@ bool SimpleLogBuffer::Prune(log_id_t id, unsigned long prune_rows, uid_t caller_ if (oldest && oldest->start() <= element.sequence()) { KickReader(oldest, id, prune_rows); - return true; + return false; } stats_->Subtract(element.ToLogStatisticsElement()); it = Erase(it); if (--prune_rows == 0) { - return false; + return true; } } - return false; + return true; } std::list::iterator SimpleLogBuffer::Erase(