From 70e716fc3bb6159947423078ea7ba56f33649a6c Mon Sep 17 00:00:00 2001 From: Austin Foxley Date: Fri, 6 Dec 2024 20:14:02 +0000 Subject: [PATCH] pw_bluetooth_proxy: Remove static mutex on l2cap channel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Now the underlying bug in Vector is fixed we can remove this workaround. Bug: 381942905 Change-Id: Ic66361eecb12625915fefef23547b5b4ac3a8aee Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/252852 Reviewed-by: Ben Lawson Docs-Not-Needed: Austin Foxley Reviewed-by: David Rees Lint: Lint 🤖 Commit-Queue: Auto-Submit Pigweed-Auto-Submit: Austin Foxley --- pw_bluetooth_proxy/l2cap_channel.cc | 37 ++++++++----------- .../internal/l2cap_channel.h | 21 ++++------- 2 files changed, 23 insertions(+), 35 deletions(-) diff --git a/pw_bluetooth_proxy/l2cap_channel.cc b/pw_bluetooth_proxy/l2cap_channel.cc index ca8207062..8f41c2342 100644 --- a/pw_bluetooth_proxy/l2cap_channel.cc +++ b/pw_bluetooth_proxy/l2cap_channel.cc @@ -27,6 +27,15 @@ namespace pw::bluetooth::proxy { +void L2capChannel::MoveLockedFields(L2capChannel& other) { + std::lock_guard lock(send_queue_mutex_); + std::lock_guard other_lock(other.send_queue_mutex_); + send_queue_ = std::move(other.send_queue_); + notify_on_dequeue_ = other.notify_on_dequeue_; + l2cap_channel_manager_.ReleaseChannel(other); + l2cap_channel_manager_.RegisterChannel(*this); +} + L2capChannel::L2capChannel(L2capChannel&& other) : l2cap_channel_manager_(other.l2cap_channel_manager_), connection_handle_(other.connection_handle()), @@ -36,15 +45,7 @@ L2capChannel::L2capChannel(L2capChannel&& other) queue_space_available_fn_(std::move(other.queue_space_available_fn_)), payload_from_controller_fn_( std::move(other.payload_from_controller_fn_)) { - // All L2capChannels share a static mutex, so only one lock needs to be - // acquired here. - // TODO: https://pwbug.dev/381942905 - Once mutex no longer needs to be - // static, elide this operator or acquire a lock on both channels' mutexes. - std::lock_guard lock(global_send_queue_mutex_); - send_queue_ = std::move(other.send_queue_); - notify_on_dequeue_ = other.notify_on_dequeue_; - l2cap_channel_manager_.ReleaseChannel(other); - l2cap_channel_manager_.RegisterChannel(*this); + MoveLockedFields(other); } L2capChannel& L2capChannel::operator=(L2capChannel&& other) { @@ -56,17 +57,9 @@ L2capChannel& L2capChannel::operator=(L2capChannel&& other) { transport_ = other.transport(); local_cid_ = other.local_cid(); remote_cid_ = other.remote_cid(); - payload_from_controller_fn_ = std::move(other.payload_from_controller_fn_); queue_space_available_fn_ = std::move(other.queue_space_available_fn_); - // All L2capWriteChannels share a static mutex, so only one lock needs to be - // acquired here. - // TODO: https://pwbug.dev/369849508 - Once mutex is no longer static, - // elide this operator or acquire a lock on both channels' mutexes. - std::lock_guard lock(global_send_queue_mutex_); - send_queue_ = std::move(other.send_queue_); - notify_on_dequeue_ = other.notify_on_dequeue_; - l2cap_channel_manager_.ReleaseChannel(other); - l2cap_channel_manager_.RegisterChannel(*this); + payload_from_controller_fn_ = std::move(other.payload_from_controller_fn_); + MoveLockedFields(other); return *this; } @@ -78,7 +71,7 @@ L2capChannel::~L2capChannel() { Status L2capChannel::QueuePacket(H4PacketWithH4&& packet) { Status status; { - std::lock_guard lock(global_send_queue_mutex_); + std::lock_guard lock(send_queue_mutex_); if (send_queue_.full()) { status = Status::Unavailable(); notify_on_dequeue_ = true; @@ -95,7 +88,7 @@ std::optional L2capChannel::DequeuePacket() { std::optional packet; bool should_notify = false; { - std::lock_guard lock(global_send_queue_mutex_); + std::lock_guard lock(send_queue_mutex_); if (!send_queue_.empty()) { packet.emplace(std::move(send_queue_.front())); send_queue_.pop(); @@ -200,7 +193,7 @@ void L2capChannel::ReportPacketsMayBeReadyToSend() { } void L2capChannel::ClearQueue() { - std::lock_guard lock(global_send_queue_mutex_); + std::lock_guard lock(send_queue_mutex_); send_queue_.clear(); } diff --git a/pw_bluetooth_proxy/public/pw_bluetooth_proxy/internal/l2cap_channel.h b/pw_bluetooth_proxy/public/pw_bluetooth_proxy/internal/l2cap_channel.h index ef49087d6..8abefd7c8 100644 --- a/pw_bluetooth_proxy/public/pw_bluetooth_proxy/internal/l2cap_channel.h +++ b/pw_bluetooth_proxy/public/pw_bluetooth_proxy/internal/l2cap_channel.h @@ -132,8 +132,7 @@ class L2capChannel : public IntrusiveForwardList::Item { // Alert `L2capChannelManager` that queued packets may be ready to send. // When calling this method, ensure no locks are held that are also acquired // in `Dequeue()` overrides. - void ReportPacketsMayBeReadyToSend() - PW_LOCKS_EXCLUDED(global_send_queue_mutex_); + void ReportPacketsMayBeReadyToSend() PW_LOCKS_EXCLUDED(send_queue_mutex_); // Remove all packets from queue. void ClearQueue(); @@ -154,6 +153,10 @@ class L2capChannel : public IntrusiveForwardList::Item { // TODO: https://pwbug.dev/349700888 - Make capacity configurable. static constexpr size_t kQueueCapacity = 5; + // Helper for move constructor and move assignment. + void MoveLockedFields(L2capChannel& other) + PW_LOCKS_EXCLUDED(send_queue_mutex_); + L2capChannelManager& l2cap_channel_manager_; // ACL connection handle. @@ -174,26 +177,18 @@ class L2capChannel : public IntrusiveForwardList::Item { // `L2capChannelManager` and channel may concurrently call functions that // access queue. // - // TODO: https://pwbug.dev/381942905 - This mutex is static to avoid it being - // destroyed when an L2capChannel is erased from a container. When an - // L2capChannel is erased, it is std::destroyed then overwritten by the - // subsequent L2capChannel in the container via the move assignment - // operator. After this, the previously destroyed L2capChannel object is - // again in an operational state, so its mutex needs to remain valid. This is - // a bug in pw::Vector::erase(). Once this behavior is fixed, we can remove - // the static to avoid cross-channel contention. - inline static sync::Mutex global_send_queue_mutex_; + sync::Mutex send_queue_mutex_; // Stores Tx L2CAP packets. InlineQueue send_queue_ - PW_GUARDED_BY(global_send_queue_mutex_); + PW_GUARDED_BY(send_queue_mutex_); // Callback to notify writers after queue space opens up. Function queue_space_available_fn_; // True if the last queue attempt didn't have space. Will be cleared on // successful dequeue. - bool notify_on_dequeue_ PW_GUARDED_BY(global_send_queue_mutex_) = false; + bool notify_on_dequeue_ PW_GUARDED_BY(send_queue_mutex_) = false; //------- // Rx: