Load with memory_order_acquire
std::atomic::load operation is with std::memory_order_seq_cst by
default. However, load operation in bufferhubd and buffer hub client
does not need sequentially-consistent ordering which is provided by
std::memory_order_seq_cst. This change changes our load operation with
std::memory_order_acquire so that no reads or writes in the current
thread can be reordered before this load, all writes in other threads
that release the same atomic variable are visible in the current thread.
Test: all tests are still passing.
Test: vega still working.
Bug: 112007999
Bug: 118718713
Change-Id: I2ac75cc306c3de35bf3d953b353f9a9442bdebbc
diff --git a/libs/vr/libbufferhub/buffer_hub_base.cpp b/libs/vr/libbufferhub/buffer_hub_base.cpp
index b4c5d42..68cc766 100644
--- a/libs/vr/libbufferhub/buffer_hub_base.cpp
+++ b/libs/vr/libbufferhub/buffer_hub_base.cpp
@@ -123,17 +123,17 @@
buffer_state_ = &metadata_header_->buffer_state;
ALOGD_IF(TRACE,
"BufferHubBase::ImportBuffer: id=%d, buffer_state=%" PRIx64 ".",
- id(), buffer_state_->load());
+ id(), buffer_state_->load(std::memory_order_acquire));
fence_state_ = &metadata_header_->fence_state;
ALOGD_IF(TRACE,
"BufferHubBase::ImportBuffer: id=%d, fence_state=%" PRIx64 ".", id(),
- fence_state_->load());
+ fence_state_->load(std::memory_order_acquire));
active_clients_bit_mask_ = &metadata_header_->active_clients_bit_mask;
ALOGD_IF(
TRACE,
"BufferHubBase::ImportBuffer: id=%d, active_clients_bit_mask=%" PRIx64
".",
- id(), active_clients_bit_mask_->load());
+ id(), active_clients_bit_mask_->load(std::memory_order_acquire));
return 0;
}
diff --git a/libs/vr/libbufferhub/consumer_buffer.cpp b/libs/vr/libbufferhub/consumer_buffer.cpp
index a91e842..83d6951 100644
--- a/libs/vr/libbufferhub/consumer_buffer.cpp
+++ b/libs/vr/libbufferhub/consumer_buffer.cpp
@@ -38,7 +38,7 @@
// Only check producer bit and this consumer buffer's particular consumer bit.
// The buffer is can be acquired iff: 1) producer bit is set; 2) consumer bit
// is not set.
- uint64_t buffer_state = buffer_state_->load();
+ uint64_t buffer_state = buffer_state_->load(std::memory_order_acquire);
if (!BufferHubDefs::IsBufferPosted(buffer_state, client_state_mask())) {
ALOGE("ConsumerBuffer::LocalAcquire: not posted, id=%d state=%" PRIx64
" client_state_mask=%" PRIx64 ".",
@@ -57,7 +57,7 @@
out_meta->user_metadata_ptr = 0;
}
- uint64_t fence_state = fence_state_->load();
+ uint64_t fence_state = fence_state_->load(std::memory_order_acquire);
// If there is an acquire fence from producer, we need to return it.
if (fence_state & BufferHubDefs::kProducerStateBit) {
*out_fence = shared_acquire_fence_.Duplicate();
@@ -118,7 +118,7 @@
return error;
// Check invalid state transition.
- uint64_t buffer_state = buffer_state_->load();
+ uint64_t buffer_state = buffer_state_->load(std::memory_order_acquire);
if (!BufferHubDefs::IsBufferAcquired(buffer_state)) {
ALOGE("ConsumerBuffer::LocalRelease: not acquired id=%d state=%" PRIx64 ".",
id(), buffer_state);
diff --git a/libs/vr/libbufferhub/include/private/dvr/buffer_hub_base.h b/libs/vr/libbufferhub/include/private/dvr/buffer_hub_base.h
index 1ea8302..09feb73 100644
--- a/libs/vr/libbufferhub/include/private/dvr/buffer_hub_base.h
+++ b/libs/vr/libbufferhub/include/private/dvr/buffer_hub_base.h
@@ -90,7 +90,9 @@
int cid() const { return cid_; }
// Returns the buffer buffer state.
- uint64_t buffer_state() { return buffer_state_->load(); };
+ uint64_t buffer_state() {
+ return buffer_state_->load(std::memory_order_acquire);
+ };
// A state mask which is unique to a buffer hub client among all its siblings
// sharing the same concrete graphic buffer.
diff --git a/libs/vr/libbufferhub/include/private/dvr/buffer_hub_defs.h b/libs/vr/libbufferhub/include/private/dvr/buffer_hub_defs.h
index 489692b..80a00be 100644
--- a/libs/vr/libbufferhub/include/private/dvr/buffer_hub_defs.h
+++ b/libs/vr/libbufferhub/include/private/dvr/buffer_hub_defs.h
@@ -40,7 +40,7 @@
uint64_t old_state;
uint64_t new_state;
do {
- old_state = buffer_state->load();
+ old_state = buffer_state->load(std::memory_order_acquire);
new_state = (old_state & ~clear_mask) | set_mask;
} while (!buffer_state->compare_exchange_weak(old_state, new_state));
}
diff --git a/libs/vr/libbufferhub/producer_buffer.cpp b/libs/vr/libbufferhub/producer_buffer.cpp
index 3730e7d..10517c5 100644
--- a/libs/vr/libbufferhub/producer_buffer.cpp
+++ b/libs/vr/libbufferhub/producer_buffer.cpp
@@ -80,7 +80,7 @@
return error;
// Check invalid state transition.
- uint64_t buffer_state = buffer_state_->load();
+ uint64_t buffer_state = buffer_state_->load(std::memory_order_acquire);
if (!BufferHubDefs::IsBufferGained(buffer_state)) {
ALOGE("ProducerBuffer::LocalPost: not gained, id=%d state=%" PRIx64 ".",
id(), buffer_state);
@@ -135,7 +135,7 @@
int ProducerBuffer::LocalGain(DvrNativeBufferMetadata* out_meta,
LocalHandle* out_fence, bool gain_posted_buffer) {
- uint64_t buffer_state = buffer_state_->load();
+ uint64_t buffer_state = buffer_state_->load(std::memory_order_acquire);
ALOGD_IF(TRACE, "ProducerBuffer::LocalGain: buffer=%d, state=%" PRIx64 ".",
id(), buffer_state);
@@ -168,7 +168,7 @@
out_meta->user_metadata_ptr = 0;
}
- uint64_t fence_state = fence_state_->load();
+ uint64_t fence_state = fence_state_->load(std::memory_order_acquire);
// If there is an release fence from consumer, we need to return it.
if (fence_state & BufferHubDefs::kConsumerStateMask) {
*out_fence = shared_release_fence_.Duplicate();
@@ -230,7 +230,7 @@
// TODO(b/112338294) Keep here for reference. Remove it after new logic is
// written.
- /* uint64_t buffer_state = buffer_state_->load();
+ /* uint64_t buffer_state = buffer_state_->load(std::memory_order_acquire);
if (!BufferHubDefs::IsBufferGained(buffer_state)) {
// Can only detach a ProducerBuffer when it's in gained state.
ALOGW("ProducerBuffer::Detach: The buffer (id=%d, state=0x%" PRIx64
diff --git a/services/vr/bufferhubd/include/private/dvr/producer_channel.h b/services/vr/bufferhubd/include/private/dvr/producer_channel.h
index c4c2ad2..5868b09 100644
--- a/services/vr/bufferhubd/include/private/dvr/producer_channel.h
+++ b/services/vr/bufferhubd/include/private/dvr/producer_channel.h
@@ -42,7 +42,9 @@
~ProducerChannel() override;
- uint64_t buffer_state() const { return buffer_state_->load(); }
+ uint64_t buffer_state() const {
+ return buffer_state_->load(std::memory_order_acquire);
+ }
bool HandleMessage(Message& message) override;
void HandleImpulse(Message& message) override;
diff --git a/services/vr/bufferhubd/producer_channel.cpp b/services/vr/bufferhubd/producer_channel.cpp
index 476eca3..162065b 100644
--- a/services/vr/bufferhubd/producer_channel.cpp
+++ b/services/vr/bufferhubd/producer_channel.cpp
@@ -161,7 +161,8 @@
ALOGD_IF(TRACE,
"ProducerChannel::~ProducerChannel: channel_id=%d buffer_id=%d "
"state=%" PRIx64 ".",
- channel_id(), buffer_id(), buffer_state_->load());
+ channel_id(), buffer_id(),
+ buffer_state_->load(std::memory_order_acquire));
for (auto consumer : consumer_channels_) {
consumer->OnProducerClosed();
}
@@ -177,7 +178,8 @@
return BufferInfo(buffer_id(), consumer_channels_.size(), buffer_.width(),
buffer_.height(), buffer_.layer_count(), buffer_.format(),
- buffer_.usage(), pending_consumers_, buffer_state_->load(),
+ buffer_.usage(), pending_consumers_,
+ buffer_state_->load(std::memory_order_acquire),
signaled_mask, metadata_header_->queue_index);
}
@@ -236,7 +238,7 @@
Message& /*message*/) {
ATRACE_NAME("ProducerChannel::OnGetBuffer");
ALOGD_IF(TRACE, "ProducerChannel::OnGetBuffer: buffer=%d, state=%" PRIx64 ".",
- buffer_id(), buffer_state_->load());
+ buffer_id(), buffer_state_->load(std::memory_order_acquire));
return {GetBuffer(BufferHubDefs::kProducerStateBit)};
}
@@ -304,8 +306,8 @@
return ErrorStatus(ENOMEM);
}
- if (!producer_owns_ &&
- !BufferHubDefs::IsBufferReleased(buffer_state_->load())) {
+ if (!producer_owns_ && !BufferHubDefs::IsBufferReleased(
+ buffer_state_->load(std::memory_order_acquire))) {
// Signal the new consumer when adding it to a posted producer.
if (consumer->OnProducerPosted())
pending_consumers_++;
@@ -406,7 +408,7 @@
ALOGD_IF(TRACE, "ProducerChannel::OnProducerDetach: buffer_id=%d",
buffer_id());
- uint64_t buffer_state = buffer_state_->load();
+ uint64_t buffer_state = buffer_state_->load(std::memory_order_acquire);
if (!BufferHubDefs::IsBufferGained(buffer_state)) {
// Can only detach a BufferProducer when it's in gained state.
ALOGW(
@@ -506,7 +508,7 @@
ALOGD_IF(TRACE,
"ProducerChannel::OnConsumerRelease: releasing last consumer: "
"buffer_id=%d state=%" PRIx64 ".",
- buffer_id(), buffer_state_->load());
+ buffer_id(), buffer_state_->load(std::memory_order_acquire));
if (orphaned_consumer_bit_mask_) {
ALOGW(
@@ -521,11 +523,12 @@
SignalAvailable();
}
- ALOGE_IF(pending_consumers_ &&
- BufferHubDefs::IsBufferReleased(buffer_state_->load()),
- "ProducerChannel::OnConsumerRelease: buffer state inconsistent: "
- "pending_consumers=%d, buffer buffer is in releaed state.",
- pending_consumers_);
+ ALOGE_IF(
+ pending_consumers_ && BufferHubDefs::IsBufferReleased(
+ buffer_state_->load(std::memory_order_acquire)),
+ "ProducerChannel::OnConsumerRelease: buffer state inconsistent: "
+ "pending_consumers=%d, buffer buffer is in releaed state.",
+ pending_consumers_);
return {};
}
@@ -564,7 +567,8 @@
"buffer_id=%d client_state_mask=%" PRIx64 " queue_index=%" PRIu64
" buffer_state=%" PRIx64 " fence_state=%" PRIx64 ".",
buffer_id(), client_state_mask, metadata_header_->queue_index,
- buffer_state_->load(), fence_state_->load());
+ buffer_state_->load(std::memory_order_acquire),
+ fence_state_->load(std::memory_order_acquire));
}
void ProducerChannel::AddConsumer(ConsumerChannel* channel) {
@@ -579,7 +583,7 @@
active_clients_bit_mask_->fetch_and(~channel->client_state_mask(),
std::memory_order_release);
- const uint64_t buffer_state = buffer_state_->load();
+ const uint64_t buffer_state = buffer_state_->load(std::memory_order_acquire);
if (BufferHubDefs::IsBufferPosted(buffer_state) ||
BufferHubDefs::IsBufferAcquired(buffer_state)) {
// The consumer client is being destoryed without releasing. This could
@@ -592,7 +596,8 @@
BufferHubDefs::IsBufferGained(buffer_state)) {
// The consumer is being close while it is suppose to signal a release
// fence. Signal the dummy fence here.
- if (fence_state_->load() & channel->client_state_mask()) {
+ if (fence_state_->load(std::memory_order_acquire) &
+ channel->client_state_mask()) {
epoll_event event;
event.events = EPOLLIN;
event.data.u64 = channel->client_state_mask();