SurfaceFlinger: Use a lockless stack for binder->tracing thread
Though the critical section is kept very small between the binder
and tracing threads, the tracing thread is not RT, and so we still
run the risk of it being descheduled and blocking one of our binder
threads excessively. In this thread we provide a simple lockless
stack implementation and use it to push pending transactions from
the binder thread to the main thread. We also looked up "layer IDs"
on the binder thread, which is no longer possible without the lock.
To work around this we store the pointer itself in the proto until
it reaches the tracing thread where we overwrite it with the mapped
value.
Bug: 200284593
Test: Existing tests pass
Change-Id: I408dc87ddfe088b68f65601455147a96b870627d
diff --git a/services/surfaceflinger/Tracing/LocklessStack.h b/services/surfaceflinger/Tracing/LocklessStack.h
new file mode 100644
index 0000000..20f2aa8
--- /dev/null
+++ b/services/surfaceflinger/Tracing/LocklessStack.h
@@ -0,0 +1,72 @@
+/*
+ * Copyright 2021 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#pragma once
+#include <atomic>
+
+template <typename T>
+// Single consumer multi producer stack. We can understand the two operations independently to see
+// why they are without race condition.
+//
+// push is responsible for maintaining a linked list stored in mPush, and called from multiple
+// threads without lock. We can see that if two threads never observe the same value from
+// mPush.load, it just functions as a normal linked list. In the case where two threads observe the
+// same value, one of them has to execute the compare_exchange first. The one that doesn't execute
+// the compare exchange first, will receive false from compare_exchange. previousHead is updated (by
+// compare_exchange) to the most recent value of mPush, and we try again. It's relatively clear to
+// see that the process can repeat with an arbitrary number of threads.
+//
+// Pop is much simpler. If mPop is empty (as it begins) it atomically exchanges
+// the entire push list with null. This is safe, since the only other reader (push)
+// of mPush will retry if it changes in between it's read and atomic compare. We
+// then store the list and pop one element.
+//
+// If we already had something in the pop list we just pop directly.
+class LocklessStack {
+public:
+ class Entry {
+ public:
+ T* mValue;
+ std::atomic<Entry*> mNext;
+ Entry(T* value): mValue(value) {}
+ };
+ std::atomic<Entry*> mPush = nullptr;
+ std::atomic<Entry*> mPop = nullptr;
+ void push(T* value) {
+ Entry* entry = new Entry(value);
+ Entry* previousHead = mPush.load(/*std::memory_order_relaxed*/);
+ do {
+ entry->mNext = previousHead;
+ } while (!mPush.compare_exchange_weak(previousHead, entry)); /*std::memory_order_release*/
+ }
+ T* pop() {
+ Entry* popped = mPop.load(/*std::memory_order_acquire*/);
+ if (popped) {
+ // Single consumer so this is fine
+ mPop.store(popped->mNext/* , std::memory_order_release */);
+ auto value = popped->mValue;
+ delete popped;
+ return value;
+ } else {
+ Entry *grabbedList = mPush.exchange(nullptr/* , std::memory_order_acquire */);
+ if (!grabbedList) return nullptr;
+ mPop.store(grabbedList->mNext/* , std::memory_order_release */);
+ auto value = grabbedList->mValue;
+ delete grabbedList;
+ return value;
+ }
+ }
+};
diff --git a/services/surfaceflinger/Tracing/TransactionProtoParser.cpp b/services/surfaceflinger/Tracing/TransactionProtoParser.cpp
index 1e5c3e7..c1112d2 100644
--- a/services/surfaceflinger/Tracing/TransactionProtoParser.cpp
+++ b/services/surfaceflinger/Tracing/TransactionProtoParser.cpp
@@ -180,13 +180,13 @@
}
if (layer.what & layer_state_t::eReparent) {
- int32_t layerId = layer.parentSurfaceControlForChild
+ int64_t layerId = layer.parentSurfaceControlForChild
? mMapper->getLayerId(layer.parentSurfaceControlForChild->getHandle())
: -1;
proto.set_parent_id(layerId);
}
if (layer.what & layer_state_t::eRelativeLayerChanged) {
- int32_t layerId = layer.relativeLayerSurfaceControl
+ int64_t layerId = layer.relativeLayerSurfaceControl
? mMapper->getLayerId(layer.relativeLayerSurfaceControl->getHandle())
: -1;
proto.set_relative_parent_id(layerId);
@@ -342,13 +342,13 @@
outState.merge(state);
if (state.what & layer_state_t::eReparent) {
- outState.parentId = proto.parent_id();
+ outState.parentId = static_cast<int32_t>(proto.parent_id());
}
if (state.what & layer_state_t::eRelativeLayerChanged) {
- outState.relativeParentId = proto.relative_parent_id();
+ outState.relativeParentId = static_cast<int32_t>(proto.relative_parent_id());
}
if (state.what & layer_state_t::eInputInfoChanged) {
- outState.inputCropId = proto.window_info_handle().crop_layer_id();
+ outState.inputCropId = static_cast<int32_t>(proto.window_info_handle().crop_layer_id());
}
if (state.what & layer_state_t::eBufferChanged) {
const proto::LayerState_BufferData& bufferProto = proto.buffer_data();
@@ -364,7 +364,7 @@
}
void TransactionProtoParser::fromProto(const proto::LayerState& proto, layer_state_t& layer) {
- layer.layerId = proto.layer_id();
+ layer.layerId = (int32_t)proto.layer_id();
layer.what |= proto.what();
layer.surface = mMapper->getLayerHandle(layer.layerId);
@@ -450,23 +450,25 @@
}
if (proto.what() & layer_state_t::eReparent) {
- int32_t layerId = proto.parent_id();
+ int64_t layerId = proto.parent_id();
if (layerId == -1) {
layer.parentSurfaceControlForChild = nullptr;
} else {
layer.parentSurfaceControlForChild =
new SurfaceControl(SurfaceComposerClient::getDefault(),
- mMapper->getLayerHandle(layerId), nullptr, layerId);
+ mMapper->getLayerHandle(static_cast<int32_t>(layerId)),
+ nullptr, static_cast<int32_t>(layerId));
}
}
if (proto.what() & layer_state_t::eRelativeLayerChanged) {
- int32_t layerId = proto.relative_parent_id();
+ int64_t layerId = proto.relative_parent_id();
if (layerId == -1) {
layer.relativeLayerSurfaceControl = nullptr;
} else {
layer.relativeLayerSurfaceControl =
new SurfaceControl(SurfaceComposerClient::getDefault(),
- mMapper->getLayerHandle(layerId), nullptr, layerId);
+ mMapper->getLayerHandle(static_cast<int32_t>(layerId)),
+ nullptr, static_cast<int32_t>(layerId));
}
layer.z = proto.z();
}
@@ -493,8 +495,9 @@
inputInfo.transform.set(transformProto.tx(), transformProto.ty());
inputInfo.replaceTouchableRegionWithCrop =
windowInfoProto.replace_touchable_region_with_crop();
- int32_t layerId = windowInfoProto.crop_layer_id();
- inputInfo.touchableRegionCropHandle = mMapper->getLayerHandle(layerId);
+ int64_t layerId = windowInfoProto.crop_layer_id();
+ inputInfo.touchableRegionCropHandle =
+ mMapper->getLayerHandle(static_cast<int32_t>(layerId));
layer.windowInfoHandle = sp<gui::WindowInfoHandle>::make(inputInfo);
}
if (proto.what() & layer_state_t::eBackgroundColorChanged) {
diff --git a/services/surfaceflinger/Tracing/TransactionProtoParser.h b/services/surfaceflinger/Tracing/TransactionProtoParser.h
index 2f70b27..872a901 100644
--- a/services/surfaceflinger/Tracing/TransactionProtoParser.h
+++ b/services/surfaceflinger/Tracing/TransactionProtoParser.h
@@ -80,7 +80,8 @@
public:
virtual ~FlingerDataMapper() = default;
virtual sp<IBinder> getLayerHandle(int32_t /* layerId */) const { return nullptr; }
- virtual int32_t getLayerId(const sp<IBinder>& /* layerHandle */) const { return -1; }
+ virtual int64_t getLayerId(const sp<IBinder>& /* layerHandle */) const { return -1; }
+ virtual int64_t getLayerId(BBinder* /* layerHandle */) const { return -1; }
virtual sp<IBinder> getDisplayHandle(int32_t /* displayId */) const { return nullptr; }
virtual int32_t getDisplayId(const sp<IBinder>& /* displayHandle */) const { return -1; }
virtual std::shared_ptr<BufferData> getGraphicData(uint64_t bufferId, uint32_t width,
@@ -106,6 +107,7 @@
TransactionState fromProto(const proto::TransactionState&);
void mergeFromProto(const proto::LayerState&, TracingLayerState& outState);
void fromProto(const proto::LayerCreationArgs&, TracingLayerCreationArgs& outArgs);
+ std::unique_ptr<FlingerDataMapper> mMapper;
private:
proto::LayerState toProto(const layer_state_t&);
@@ -113,7 +115,6 @@
void fromProto(const proto::LayerState&, layer_state_t& out);
DisplayState fromProto(const proto::DisplayState&);
- std::unique_ptr<FlingerDataMapper> mMapper;
};
} // namespace android::surfaceflinger
diff --git a/services/surfaceflinger/Tracing/TransactionTracing.cpp b/services/surfaceflinger/Tracing/TransactionTracing.cpp
index d5e837f..6381758 100644
--- a/services/surfaceflinger/Tracing/TransactionTracing.cpp
+++ b/services/surfaceflinger/Tracing/TransactionTracing.cpp
@@ -29,23 +29,16 @@
namespace android {
-class FlingerDataMapper : public TransactionProtoParser::FlingerDataMapper {
- std::unordered_map<BBinder* /* layerHandle */, int32_t /* layerId */>& mLayerHandles;
-
+// Keeps the binder address as the layer id so we can avoid holding the tracing lock in the
+// binder thread.
+class FlatDataMapper : public TransactionProtoParser::FlingerDataMapper {
public:
- FlingerDataMapper(std::unordered_map<BBinder* /* handle */, int32_t /* id */>& layerHandles)
- : mLayerHandles(layerHandles) {}
-
- int32_t getLayerId(const sp<IBinder>& layerHandle) const override {
+ virtual int64_t getLayerId(const sp<IBinder>& layerHandle) const {
if (layerHandle == nullptr) {
return -1;
}
- auto it = mLayerHandles.find(layerHandle->localBinder());
- if (it == mLayerHandles.end()) {
- ALOGW("Could not find layer handle %p", layerHandle->localBinder());
- return -1;
- }
- return it->second;
+
+ return reinterpret_cast<int64_t>(layerHandle->localBinder());
}
void getGraphicBufferPropertiesFromCache(client_cache_t cachedBuffer, uint64_t* outBufferId,
@@ -72,8 +65,33 @@
}
};
+class FlingerDataMapper : public FlatDataMapper {
+ std::unordered_map<BBinder* /* layerHandle */, int32_t /* layerId */>& mLayerHandles;
+
+public:
+ FlingerDataMapper(std::unordered_map<BBinder* /* handle */, int32_t /* id */>& layerHandles)
+ : mLayerHandles(layerHandles) {}
+
+ int64_t getLayerId(const sp<IBinder>& layerHandle) const override {
+ if (layerHandle == nullptr) {
+ return -1;
+ }
+ return getLayerId(layerHandle->localBinder());
+ }
+
+ int64_t getLayerId(BBinder* localBinder) const {
+ auto it = mLayerHandles.find(localBinder);
+ if (it == mLayerHandles.end()) {
+ ALOGW("Could not find layer handle %p", localBinder);
+ return -1;
+ }
+ return it->second;
+ }
+};
+
TransactionTracing::TransactionTracing()
- : mProtoParser(std::make_unique<FlingerDataMapper>(mLayerHandles)) {
+ : mProtoParser(std::make_unique<FlingerDataMapper>(mLayerHandles)),
+ mLockfreeProtoParser(std::make_unique<FlatDataMapper>()) {
std::scoped_lock lock(mTraceLock);
mBuffer.setSize(mBufferSizeInBytes);
@@ -129,9 +147,9 @@
}
void TransactionTracing::addQueuedTransaction(const TransactionState& transaction) {
- std::scoped_lock lock(mTraceLock);
- ATRACE_CALL();
- mQueuedTransactions[transaction.id] = mProtoParser.toProto(transaction);
+ proto::TransactionState* state =
+ new proto::TransactionState(mLockfreeProtoParser.toProto(transaction));
+ mTransactionQueue.push(state);
}
void TransactionTracing::addCommittedTransactions(std::vector<TransactionState>& transactions,
@@ -182,6 +200,38 @@
std::scoped_lock lock(mTraceLock);
std::vector<std::string> removedEntries;
proto::TransactionTraceEntry entryProto;
+
+ while (auto incomingTransaction = mTransactionQueue.pop()) {
+ auto transaction = *incomingTransaction;
+ int32_t layerCount = transaction.layer_changes_size();
+ for (int i = 0; i < layerCount; i++) {
+ auto layer = transaction.mutable_layer_changes(i);
+ layer->set_layer_id(
+ mProtoParser.mMapper->getLayerId(reinterpret_cast<BBinder*>(layer->layer_id())));
+ if ((layer->what() & layer_state_t::eReparent) && layer->parent_id() != -1) {
+ layer->set_parent_id(
+ mProtoParser.mMapper->getLayerId(reinterpret_cast<BBinder*>(
+ layer->parent_id())));
+ }
+
+ if ((layer->what() & layer_state_t::eRelativeLayerChanged) &&
+ layer->relative_parent_id() != -1) {
+ layer->set_relative_parent_id(
+ mProtoParser.mMapper->getLayerId(reinterpret_cast<BBinder*>(
+ layer->relative_parent_id())));
+ }
+
+ if (layer->has_window_info_handle() &&
+ layer->window_info_handle().crop_layer_id() != -1) {
+ auto input = layer->mutable_window_info_handle();
+ input->set_crop_layer_id(
+ mProtoParser.mMapper->getLayerId(reinterpret_cast<BBinder*>(
+ input->crop_layer_id())));
+ }
+ }
+ mQueuedTransactions[incomingTransaction->transaction_id()] = transaction;
+ delete incomingTransaction;
+ }
for (const CommittedTransactions& entry : committedTransactions) {
entryProto.set_elapsed_realtime_nanos(entry.timestamp);
entryProto.set_vsync_id(entry.vsyncId);
@@ -317,9 +367,9 @@
// Merge layer states to starting transaction state.
for (const proto::TransactionState& transaction : removedEntry.transactions()) {
for (const proto::LayerState& layerState : transaction.layer_changes()) {
- auto it = mStartingStates.find(layerState.layer_id());
+ auto it = mStartingStates.find((int32_t)layerState.layer_id());
if (it == mStartingStates.end()) {
- ALOGW("Could not find layer id %d", layerState.layer_id());
+ ALOGW("Could not find layer id %d", (int32_t)layerState.layer_id());
continue;
}
mProtoParser.mergeFromProto(layerState, it->second);
diff --git a/services/surfaceflinger/Tracing/TransactionTracing.h b/services/surfaceflinger/Tracing/TransactionTracing.h
index 95256c4..4c291f9 100644
--- a/services/surfaceflinger/Tracing/TransactionTracing.h
+++ b/services/surfaceflinger/Tracing/TransactionTracing.h
@@ -26,6 +26,7 @@
#include <thread>
#include "RingBuffer.h"
+#include "LocklessStack.h"
#include "TransactionProtoParser.h"
using namespace android::surfaceflinger;
@@ -78,6 +79,7 @@
size_t mBufferSizeInBytes GUARDED_BY(mTraceLock) = CONTINUOUS_TRACING_BUFFER_SIZE;
std::unordered_map<uint64_t, proto::TransactionState> mQueuedTransactions
GUARDED_BY(mTraceLock);
+ LocklessStack<proto::TransactionState> mTransactionQueue;
nsecs_t mStartingTimestamp GUARDED_BY(mTraceLock);
std::vector<proto::LayerCreationArgs> mCreatedLayers GUARDED_BY(mTraceLock);
std::unordered_map<BBinder* /* layerHandle */, int32_t /* layerId */> mLayerHandles
@@ -85,6 +87,9 @@
std::vector<int32_t /* layerId */> mRemovedLayerHandles GUARDED_BY(mTraceLock);
std::map<int32_t /* layerId */, TracingLayerState> mStartingStates GUARDED_BY(mTraceLock);
TransactionProtoParser mProtoParser GUARDED_BY(mTraceLock);
+ // Parses the transaction to proto without holding any tracing locks so we can generate proto
+ // in the binder thread without any contention.
+ TransactionProtoParser mLockfreeProtoParser;
// We do not want main thread to block so main thread will try to acquire mMainThreadLock,
// otherwise will push data to temporary container.
diff --git a/services/surfaceflinger/layerproto/transactions.proto b/services/surfaceflinger/layerproto/transactions.proto
index fcf4499..4f99b19 100644
--- a/services/surfaceflinger/layerproto/transactions.proto
+++ b/services/surfaceflinger/layerproto/transactions.proto
@@ -70,7 +70,7 @@
// Keep insync with layer_state_t
message LayerState {
- int32 layer_id = 1;
+ int64 layer_id = 1;
// Changes are split into ChangesLsb and ChangesMsb. First 32 bits are in ChangesLsb
// and the next 32 bits are in ChangesMsb. This is needed because enums have to be
// 32 bits and there's no nice way to put 64bit constants into .proto files.
@@ -161,8 +161,8 @@
Matrix22 matrix = 11;
float corner_radius = 12;
uint32 background_blur_radius = 13;
- int32 parent_id = 14;
- int32 relative_parent_id = 15;
+ int64 parent_id = 14;
+ int64 relative_parent_id = 15;
float alpha = 16;
message Color3 {
@@ -233,7 +233,7 @@
bool focusable = 5;
bool has_wallpaper = 6;
float global_scale_factor = 7;
- int32 crop_layer_id = 8;
+ int64 crop_layer_id = 8;
bool replace_touchable_region_with_crop = 9;
RectProto touchable_region_crop = 10;
Transform transform = 11;
diff --git a/services/surfaceflinger/tests/unittests/TransactionProtoParserTest.cpp b/services/surfaceflinger/tests/unittests/TransactionProtoParserTest.cpp
index ab893a3..f5e3b77 100644
--- a/services/surfaceflinger/tests/unittests/TransactionProtoParserTest.cpp
+++ b/services/surfaceflinger/tests/unittests/TransactionProtoParserTest.cpp
@@ -81,7 +81,7 @@
sp<IBinder> getLayerHandle(int32_t id) const override {
return (id == 42) ? layerHandle : nullptr;
}
- int32_t getLayerId(const sp<IBinder>& handle) const override {
+ int64_t getLayerId(const sp<IBinder>& handle) const override {
return (handle == layerHandle) ? 42 : -1;
}
sp<IBinder> getDisplayHandle(int32_t id) const {