pw_trace: improve locks
Add an internal queue with a separate lock, this is a small section
which just copies the arguments making it suitable for a critical
section.
Emptying this queue is handled from a separate lock, which is a trylock.
If the queue is already being emptied, it is safe to return after adding
to the queue (no need to block).
Change-Id: I45bbf749b2cb8687bb5d8e485adde2256b2c99a8
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/21881
Reviewed-by: Paul Mathieu <paulmathieu@google.com>
Commit-Queue: Rob Oliver <rgoliver@google.com>
diff --git a/pw_trace_tokenized/BUILD.gn b/pw_trace_tokenized/BUILD.gn
index 0ca9cf8..c031e89 100644
--- a/pw_trace_tokenized/BUILD.gn
+++ b/pw_trace_tokenized/BUILD.gn
@@ -146,12 +146,14 @@
":backend_config",
":public_include_path",
]
- public_deps = [ "$dir_pw_tokenizer" ]
+ public_deps = [
+ "$dir_pw_status",
+ "$dir_pw_tokenizer",
+ ]
deps = [
":config",
"$dir_pw_assert",
"$dir_pw_ring_buffer",
- "$dir_pw_status",
"$dir_pw_trace:facade",
"$dir_pw_varint",
]
diff --git a/pw_trace_tokenized/public/pw_trace_tokenized/trace_tokenized.h b/pw_trace_tokenized/public/pw_trace_tokenized/trace_tokenized.h
index 8ea52e4..300792a 100644
--- a/pw_trace_tokenized/public/pw_trace_tokenized/trace_tokenized.h
+++ b/pw_trace_tokenized/public/pw_trace_tokenized/trace_tokenized.h
@@ -28,6 +28,7 @@
#endif // __cplusplus
#endif // PW_TRACE_GET_TIME_DELTA
+#include "pw_status/status.h"
#include "pw_tokenizer/tokenize.h"
#include "pw_trace_tokenized/config.h"
#include "pw_trace_tokenized/internal/trace_tokenized_internal.h"
@@ -38,9 +39,88 @@
using EventType = pw_trace_EventType;
+namespace internal {
+
+// Simple ring buffer which is suitable for use in a critical section.
+template <size_t kSize>
+class TraceQueue {
+ public:
+ struct QueueEventBlock {
+ uint32_t trace_token;
+ EventType event_type;
+ const char* module;
+ uint32_t trace_id;
+ uint8_t flags;
+ size_t data_size;
+ std::byte data_buffer[PW_TRACE_BUFFER_MAX_DATA_SIZE_BYTES];
+ };
+
+ pw::Status TryPushBack(uint32_t trace_token,
+ EventType event_type,
+ const char* module,
+ uint32_t trace_id,
+ uint8_t flags,
+ const void* data_buffer,
+ size_t data_size) {
+ if (IsFull()) {
+ return pw::Status::RESOURCE_EXHAUSTED;
+ }
+ event_queue_[head_].trace_token = trace_token;
+ event_queue_[head_].event_type = event_type;
+ event_queue_[head_].module = module;
+ event_queue_[head_].trace_id = trace_id;
+ event_queue_[head_].flags = flags;
+ event_queue_[head_].data_size = data_size;
+ for (size_t i = 0; i < data_size; i++) {
+ event_queue_[head_].data_buffer[i] =
+ reinterpret_cast<const std::byte*>(data_buffer)[i];
+ }
+ head_ = (head_ + 1) % kSize;
+ is_empty_ = false;
+ return pw::Status::OK;
+ }
+
+ const volatile QueueEventBlock* PeekFront() const {
+ if (IsEmpty()) {
+ return nullptr;
+ }
+ return &event_queue_[tail_];
+ }
+
+ void PopFront() {
+ if (!IsEmpty()) {
+ tail_ = (tail_ + 1) % kSize;
+ is_empty_ = (tail_ == head_);
+ }
+ }
+
+ void Clear() {
+ head_ = 0;
+ tail_ = 0;
+ is_empty_ = true;
+ }
+
+ bool IsEmpty() const { return is_empty_; }
+ bool IsFull() const { return !is_empty_ && (head_ == tail_); }
+
+ private:
+ std::array<volatile QueueEventBlock, kSize> event_queue_;
+ volatile size_t head_ = 0; // Next write
+ volatile size_t tail_ = 0; // Next read
+ volatile bool is_empty_ =
+ true; // Used to distinquish if head==tail is empty or full
+};
+
+} // namespace internal
+
class TokenizedTraceImpl {
public:
- void Enable(bool enable) { enabled_ = enable; }
+ void Enable(bool enable) {
+ if (enable == enabled_ && enable) {
+ event_queue_.Clear();
+ }
+ enabled_ = enable;
+ }
bool IsEnabled() const { return enabled_; }
void HandleTraceEvent(uint32_t trace_token,
@@ -52,8 +132,13 @@
size_t data_size);
private:
+ using TraceQueue = internal::TraceQueue<PW_TRACE_QUEUE_SIZE_EVENTS>;
PW_TRACE_TIME_TYPE last_trace_time_ = 0;
bool enabled_ = false;
+ TraceQueue event_queue_;
+
+ void HandleNextItemInQueue(
+ const volatile TraceQueue::QueueEventBlock* event_block);
};
// A singleton object of the TokenizedTraceImpl class which can be used to
diff --git a/pw_trace_tokenized/trace.cc b/pw_trace_tokenized/trace.cc
index c9f9094..b017a73 100644
--- a/pw_trace_tokenized/trace.cc
+++ b/pw_trace_tokenized/trace.cc
@@ -40,10 +40,48 @@
return;
}
- pw_trace_TraceEventReturnFlags ret_flags = 0;
+ // Create trace event
+ PW_TRACE_QUEUE_LOCK();
+ if (!event_queue_
+ .TryPushBack(trace_token,
+ event_type,
+ module,
+ trace_id,
+ flags,
+ data_buffer,
+ data_size)
+ .Ok()) {
+ // Queue full dropping sample
+ // TODO(rgoliver): Allow other strategies, for example: drop oldest, try
+ // empty queue, or block.
+ }
+ PW_TRACE_QUEUE_UNLOCK();
- PW_TRACE_LOCK();
+ // Sample is now in queue (if not dropped), try to empty the queue if not
+ // already being emptied.
+ if (PW_TRACE_TRY_LOCK()) {
+ while (!event_queue_.IsEmpty()) {
+ HandleNextItemInQueue(event_queue_.PeekFront());
+ event_queue_.PopFront();
+ }
+ PW_TRACE_UNLOCK();
+ }
+}
+
+void TokenizedTraceImpl::HandleNextItemInQueue(
+ const volatile TraceQueue::QueueEventBlock* event_block) {
+ // Get next item in queue
+ uint32_t trace_token = event_block->trace_token;
+ EventType event_type = event_block->event_type;
+ const char* module = event_block->module;
+ uint32_t trace_id = event_block->trace_id;
+ uint8_t flags = event_block->flags;
+ const std::byte* data_buffer =
+ const_cast<const std::byte*>(event_block->data_buffer);
+ size_t data_size = event_block->data_size;
+
// Call any event callback which is registered to receive every event.
+ pw_trace_TraceEventReturnFlags ret_flags = 0;
ret_flags |=
Callbacks::Instance().CallEventCallbacks(CallbacksImpl::kCallOnEveryEvent,
trace_token,
@@ -53,11 +91,10 @@
flags);
// Return if disabled.
if ((PW_TRACE_EVENT_RETURN_FLAGS_SKIP_EVENT & ret_flags) || !enabled_) {
- PW_TRACE_UNLOCK();
return;
}
- // Call any event callback which is registered to receive every event.
+ // Call any event callback not already called.
ret_flags |= Callbacks::Instance().CallEventCallbacks(
CallbacksImpl::kCallOnlyWhenEnabled,
trace_token,
@@ -68,7 +105,6 @@
// Return if disabled (from a callback) or if a callback has indicated the
// sample should be skipped.
if ((PW_TRACE_EVENT_RETURN_FLAGS_SKIP_EVENT & ret_flags) || !enabled_) {
- PW_TRACE_UNLOCK();
return;
}
@@ -108,8 +144,6 @@
if (PW_TRACE_EVENT_RETURN_FLAGS_DISABLE_AFTER_PROCESSING & ret_flags) {
enabled_ = false;
}
-
- PW_TRACE_UNLOCK();
}
pw_trace_TraceEventReturnFlags CallbacksImpl::CallEventCallbacks(
diff --git a/pw_trace_tokenized/trace_test.cc b/pw_trace_tokenized/trace_test.cc
index fa8f64d..5e6d2a4 100644
--- a/pw_trace_tokenized/trace_test.cc
+++ b/pw_trace_tokenized/trace_test.cc
@@ -537,3 +537,73 @@
"i"); // TODO(rgoliver): check data
EXPECT_TRUE(test_interface.GetEvents().empty());
}
+
+// Create some helper macros that generated some test trace data based from a
+// number, and can check that it is correct.
+constexpr std::byte kTestData[] = {
+ std::byte{0}, std::byte{1}, std::byte{2}, std::byte{3}, std::byte{4}};
+#define QUEUE_TESTS_ARGS(num) \
+ (num), static_cast<pw_trace_EventType>((num) % 10), \
+ "module_" PW_STRINGIFY(num), (num), (num), kTestData, \
+ (num) % PW_ARRAY_SIZE(kTestData)
+#define QUEUE_CHECK_RESULT(queue_size, result, num) \
+ result && ((result->trace_token) == (num)) && \
+ ((result->event_type) == static_cast<pw_trace_EventType>((num) % 10)) && \
+ (strncmp(result->module, \
+ "module_" PW_STRINGIFY(num), \
+ strlen("module_" PW_STRINGIFY(num))) == 0) && \
+ ((result->trace_id) == (num)) && ((result->flags) == (num)) && \
+ (memcmp(const_cast<const pw::trace::internal::TraceQueue< \
+ queue_size>::QueueEventBlock*>(result) \
+ ->data_buffer, \
+ kTestData, \
+ result->data_size) == 0) && \
+ (result->data_size == (num) % PW_ARRAY_SIZE(kTestData))
+
+TEST(TokenizedTrace, QueueSimple) {
+ constexpr size_t kQueueSize = 5;
+ pw::trace::internal::TraceQueue<kQueueSize> queue;
+ constexpr size_t kTestNum = 1;
+ queue.TryPushBack(QUEUE_TESTS_ARGS(kTestNum));
+ EXPECT_FALSE(queue.IsEmpty());
+ EXPECT_FALSE(queue.IsFull());
+ EXPECT_TRUE(QUEUE_CHECK_RESULT(kQueueSize, queue.PeekFront(), kTestNum));
+ queue.PopFront();
+ EXPECT_TRUE(queue.IsEmpty());
+ EXPECT_TRUE(queue.PeekFront() == nullptr);
+ EXPECT_FALSE(queue.IsFull());
+}
+
+TEST(TokenizedTrace, QueueFull) {
+ constexpr size_t kQueueSize = 5;
+ pw::trace::internal::TraceQueue<kQueueSize> queue;
+ for (size_t i = 0; i < kQueueSize; i++) {
+ EXPECT_EQ(queue.TryPushBack(QUEUE_TESTS_ARGS(i)), pw::Status::OK);
+ }
+ EXPECT_FALSE(queue.IsEmpty());
+ EXPECT_TRUE(queue.IsFull());
+ EXPECT_EQ(queue.TryPushBack(QUEUE_TESTS_ARGS(1)),
+ pw::Status::RESOURCE_EXHAUSTED);
+
+ for (size_t i = 0; i < kQueueSize; i++) {
+ EXPECT_TRUE(QUEUE_CHECK_RESULT(kQueueSize, queue.PeekFront(), i));
+ queue.PopFront();
+ }
+ EXPECT_TRUE(queue.IsEmpty());
+ EXPECT_TRUE(queue.PeekFront() == nullptr);
+ EXPECT_FALSE(queue.IsFull());
+}
+
+TEST(TokenizedTrace, Clear) {
+ constexpr size_t kQueueSize = 5;
+ pw::trace::internal::TraceQueue<kQueueSize> queue;
+ for (size_t i = 0; i < kQueueSize; i++) {
+ EXPECT_EQ(queue.TryPushBack(QUEUE_TESTS_ARGS(i)), pw::Status::OK);
+ }
+ EXPECT_FALSE(queue.IsEmpty());
+ EXPECT_TRUE(queue.IsFull());
+ queue.Clear();
+ EXPECT_TRUE(queue.IsEmpty());
+ EXPECT_TRUE(queue.PeekFront() == nullptr);
+ EXPECT_FALSE(queue.IsFull());
+}
