From ce6a13a6cb87833a82a87a3087dddbbcb01a2092 Mon Sep 17 00:00:00 2001 From: Hansong Zhang Date: Mon, 2 Mar 2020 16:09:01 -0800 Subject: [PATCH] L2CAP: Minor fixes * Change some data member order to their correct dependency * Use atomic_bool to indicate some queue registration state * Synchronize DataController::OnSdu on L2cap Handler context Bug: 150174451 Test: cert/run --host Change-Id: Ifb1313c98267d4e47460e28b4ba167610203ba11 --- gd/l2cap/classic/internal/link.h | 2 +- gd/l2cap/internal/data_pipeline_manager.h | 2 +- gd/l2cap/internal/scheduler_fifo.cc | 13 ++++++------- gd/l2cap/internal/scheduler_fifo.h | 3 ++- gd/l2cap/internal/sender.cc | 14 ++++++++------ gd/l2cap/internal/sender.h | 3 ++- 6 files changed, 20 insertions(+), 17 deletions(-) diff --git a/gd/l2cap/classic/internal/link.h b/gd/l2cap/classic/internal/link.h index fe40b5268..daf21a747 100644 --- a/gd/l2cap/classic/internal/link.h +++ b/gd/l2cap/classic/internal/link.h @@ -170,9 +170,9 @@ class Link : public l2cap::internal::ILink, public hci::ConnectionManagementCall l2cap::internal::ParameterProvider* parameter_provider_; DynamicChannelServiceManagerImpl* dynamic_service_manager_; FixedChannelServiceManagerImpl* fixed_service_manager_; - ClassicSignallingManager signalling_manager_; std::unordered_map local_cid_to_pending_dynamic_channel_connection_map_; os::Alarm link_idle_disconnect_alarm_{l2cap_handler_}; + ClassicSignallingManager signalling_manager_; Mtu remote_connectionless_mtu_ = kMinimumClassicMtu; bool remote_supports_ertm_ = false; bool remote_supports_fcs_ = false; diff --git a/gd/l2cap/internal/data_pipeline_manager.h b/gd/l2cap/internal/data_pipeline_manager.h index 0424de9ca..aea8d7c35 100644 --- a/gd/l2cap/internal/data_pipeline_manager.h +++ b/gd/l2cap/internal/data_pipeline_manager.h @@ -71,9 +71,9 @@ class DataPipelineManager { private: os::Handler* handler_; ILink* link_; + std::unordered_map sender_map_; std::unique_ptr scheduler_; Receiver receiver_; - std::unordered_map sender_map_; }; } // namespace internal } // namespace l2cap diff --git a/gd/l2cap/internal/scheduler_fifo.cc b/gd/l2cap/internal/scheduler_fifo.cc index 8d2a64a1d..1396bc14e 100644 --- a/gd/l2cap/internal/scheduler_fifo.cc +++ b/gd/l2cap/internal/scheduler_fifo.cc @@ -30,12 +30,12 @@ Fifo::Fifo(DataPipelineManager* data_pipeline_manager, LowerQueueUpEnd* link_que ASSERT(link_queue_up_end_ != nullptr && handler_ != nullptr); } +// Invoked from some external Handler context Fifo::~Fifo() { - if (link_queue_enqueue_registered_) { - link_queue_up_end_->UnregisterEnqueue(); - } + // TODO(hsz): notify Sender don't send callback to me } +// Invoked within L2CAP Handler context void Fifo::OnPacketsReady(Cid cid, int number_packets) { if (number_packets == 0) { return; @@ -44,6 +44,7 @@ void Fifo::OnPacketsReady(Cid cid, int number_packets) { try_register_link_queue_enqueue(); } +// Invoked from some external Queue Reactable context std::unique_ptr Fifo::link_queue_enqueue_callback() { ASSERT(!next_to_dequeue_and_num_packets.empty()); auto& channel_id_and_number_packets = next_to_dequeue_and_num_packets.front(); @@ -55,20 +56,18 @@ std::unique_ptr Fifo::link_queue_enqueue_callback() { auto packet = data_pipeline_manager_->GetDataController(channel_id)->GetNextPacket(); data_pipeline_manager_->OnPacketSent(channel_id); - if (next_to_dequeue_and_num_packets.empty()) { + if (next_to_dequeue_and_num_packets.empty() && link_queue_enqueue_registered_.exchange(false)) { link_queue_up_end_->UnregisterEnqueue(); - link_queue_enqueue_registered_ = false; } return packet; } void Fifo::try_register_link_queue_enqueue() { - if (link_queue_enqueue_registered_) { + if (link_queue_enqueue_registered_.exchange(true)) { return; } link_queue_up_end_->RegisterEnqueue(handler_, common::Bind(&Fifo::link_queue_enqueue_callback, common::Unretained(this))); - link_queue_enqueue_registered_ = true; } } // namespace internal diff --git a/gd/l2cap/internal/scheduler_fifo.h b/gd/l2cap/internal/scheduler_fifo.h index f6fcf7f8c..e893a4e47 100644 --- a/gd/l2cap/internal/scheduler_fifo.h +++ b/gd/l2cap/internal/scheduler_fifo.h @@ -16,6 +16,7 @@ #pragma once +#include #include #include @@ -44,7 +45,7 @@ class Fifo : public Scheduler { LowerQueueUpEnd* link_queue_up_end_; os::Handler* handler_; std::queue> next_to_dequeue_and_num_packets; - bool link_queue_enqueue_registered_ = false; + std::atomic_bool link_queue_enqueue_registered_ = false; void try_register_link_queue_enqueue(); std::unique_ptr link_queue_enqueue_callback(); diff --git a/gd/l2cap/internal/sender.cc b/gd/l2cap/internal/sender.cc index c031cb5ff..2c740ca49 100644 --- a/gd/l2cap/internal/sender.cc +++ b/gd/l2cap/internal/sender.cc @@ -55,7 +55,7 @@ Sender::Sender(os::Handler* handler, ILink* link, Scheduler* scheduler, std::sha } Sender::~Sender() { - if (is_dequeue_registered_) { + if (is_dequeue_registered_.exchange(false)) { queue_end_->UnregisterDequeue(); } } @@ -73,19 +73,21 @@ DataController* Sender::GetDataController() { } void Sender::try_register_dequeue() { - if (is_dequeue_registered_) { + if (is_dequeue_registered_.exchange(true)) { return; } queue_end_->RegisterDequeue(handler_, common::Bind(&Sender::dequeue_callback, common::Unretained(this))); - is_dequeue_registered_ = true; } +// From external context void Sender::dequeue_callback() { auto packet = queue_end_->TryDequeue(); ASSERT(packet != nullptr); - data_controller_->OnSdu(std::move(packet)); - queue_end_->UnregisterDequeue(); - is_dequeue_registered_ = false; + handler_->Post( + common::BindOnce(&DataController::OnSdu, common::Unretained(data_controller_.get()), std::move(packet))); + if (is_dequeue_registered_.exchange(false)) { + queue_end_->UnregisterDequeue(); + } } void Sender::UpdateClassicConfiguration(classic::internal::ChannelConfigurationState config) { diff --git a/gd/l2cap/internal/sender.h b/gd/l2cap/internal/sender.h index 540064f23..3e541a96b 100644 --- a/gd/l2cap/internal/sender.h +++ b/gd/l2cap/internal/sender.h @@ -16,6 +16,7 @@ #pragma once +#include #include #include @@ -81,7 +82,7 @@ class Sender { Scheduler* scheduler_; const Cid channel_id_; const Cid remote_channel_id_; - bool is_dequeue_registered_ = false; + std::atomic_bool is_dequeue_registered_ = false; RetransmissionAndFlowControlModeOption mode_ = RetransmissionAndFlowControlModeOption::L2CAP_BASIC; std::unique_ptr data_controller_; -- 2.11.0