logd: single std::mutex for locking log buffers and tracking readers
There are only three places where the log buffer lock is not already
held when the reader lock is taken:
1) In LogReader, when a new reader connects
2) In LogReader, when a misbehaving reader disconnects
3) LogReaderThread::ThreadFunction()
1) and 2) happen sufficiently rarely that there's no impact if they
additionally held a global lock.
3) is refactored in this CL. Previously, it would do the below in a loop
1) Lock the reader lock then wait on a condition variable
2) Unlock the reader lock
3) Lock the log buffer lock in LogBuffer::FlushTo()
4) In each iteration in the LogBuffer::FlushTo() loop
1) Lock then unlock the reader lock in FilterSecondPass()
2) Unlock the log buffer lock to send the message, then re-lock it
5) Unlock the log buffer lock when leaving LogBuffer::FlushTo()
If these locks are collapsed into a single lock, then this simplifies to:
1) Lock the single lock then wait on a condition variable
2) In each iteration in the LogBuffer::FlushTo() loop
1) Unlock the single lock to send the message, then re-lock it
Collapsing both these locks into a single lock simplifes the code and
removes the overhead of acquiring the second lock, in the majority of
use cases where the first lock is already held.
Secondly, this lock will be a plain std::mutex instead of a RwLock.
RwLock's are appropriate when there is a substantial imbalance between
readers and writers and high contention, neither are true for logd.
Bug: 169736426
Test: logging unit tests
Change-Id: Ia511506f2d0935a5321c1b2f65569066f91ecb06
diff --git a/logd/SerializedFlushToState.h b/logd/SerializedFlushToState.h
index c953a16..1a69f7b 100644
--- a/logd/SerializedFlushToState.h
+++ b/logd/SerializedFlushToState.h
@@ -21,6 +21,7 @@
#include <queue>
#include "LogBuffer.h"
+#include "LogdLock.h"
#include "SerializedLogChunk.h"
#include "SerializedLogEntry.h"
@@ -44,48 +45,45 @@
public:
// Initializes this state object. For each log buffer set in log_mask, this sets
// logs_needed_from_next_position_.
- SerializedFlushToState(uint64_t start, LogMask log_mask);
+ SerializedFlushToState(uint64_t start, LogMask log_mask, std::list<SerializedLogChunk>* logs)
+ REQUIRES(logd_lock);
// Decrease the reference of all referenced logs. This happens when a reader is disconnected.
~SerializedFlushToState() override;
- // We can't hold SerializedLogBuffer::lock_ in the constructor, so we must initialize logs here.
- void InitializeLogs(std::list<SerializedLogChunk>* logs) {
- if (logs_ == nullptr) logs_ = logs;
- }
-
// Updates the state of log_positions_ and logs_needed_from_next_position_ then returns true if
// there are any unread logs, false otherwise.
- bool HasUnreadLogs();
+ bool HasUnreadLogs() REQUIRES(logd_lock);
// Returns the next unread log and sets logs_needed_from_next_position_ to indicate that we're
// waiting for more logs from the associated log buffer.
- LogWithId PopNextUnreadLog();
+ LogWithId PopNextUnreadLog() REQUIRES(logd_lock);
// If the parent log buffer prunes logs, the reference that this class contains may become
// invalid, so this must be called first to drop the reference to buffer_it, if any.
- void Prune(log_id_t log_id, const std::list<SerializedLogChunk>::iterator& buffer_it);
+ void Prune(log_id_t log_id, const std::list<SerializedLogChunk>::iterator& buffer_it)
+ REQUIRES(logd_lock);
private:
// Set logs_needed_from_next_position_[i] to indicate if log_positions_[i] points to an unread
// log or to the point at which the next log will appear.
- void UpdateLogsNeeded(log_id_t log_id);
+ void UpdateLogsNeeded(log_id_t log_id) REQUIRES(logd_lock);
// Create a LogPosition object for the given log_id by searching through the log chunks for the
// first chunk and then first log entry within that chunk that is greater or equal to start().
- void CreateLogPosition(log_id_t log_id);
+ void CreateLogPosition(log_id_t log_id) REQUIRES(logd_lock);
// Checks to see if any log buffers set in logs_needed_from_next_position_ have new logs and
// calls UpdateLogsNeeded() if so.
- void CheckForNewLogs();
+ void CheckForNewLogs() REQUIRES(logd_lock);
- std::list<SerializedLogChunk>* logs_ = nullptr;
+ std::list<SerializedLogChunk>* logs_ GUARDED_BY(logd_lock) = nullptr;
// An optional structure that contains an iterator to the serialized log buffer and offset into
// it that this logger should handle next.
- std::optional<LogPosition> log_positions_[LOG_ID_MAX];
+ std::optional<LogPosition> log_positions_[LOG_ID_MAX] GUARDED_BY(logd_lock);
// A bit for each log that is set if a given log_id has no logs or if this client has read all
// of its logs. In order words: `logs_[i].empty() || (buffer_it == std::prev(logs_.end) &&
// next_log_position == logs_write_position_)`. These will be re-checked in each
// loop in case new logs came in.
- std::bitset<LOG_ID_MAX> logs_needed_from_next_position_ = {};
+ std::bitset<LOG_ID_MAX> logs_needed_from_next_position_ GUARDED_BY(logd_lock) = {};
};