From 9550bd3ef86841e9a34a8ee08c0ad00c2078f379 Mon Sep 17 00:00:00 2001 From: Andre Puschmann Date: Fri, 17 Jul 2020 10:46:54 +0200 Subject: [PATCH] rlc: revisit interface for MAC and RRC * add locked and unlocked version of has_data() since one is called from stack and one from PHY threads * add comments in each interface section as to why locking is required or not * remove RLC rwlock when not required * move calls only used by RRC to RRC section --- lib/include/srslte/interfaces/ue_interfaces.h | 2 +- lib/include/srslte/test/ue_test_interfaces.h | 2 +- lib/include/srslte/upper/rlc.h | 5 +- lib/src/upper/rlc.cc | 53 +++++++++++-------- srsue/src/stack/mac/mux.cc | 2 +- srsue/test/mac_test.cc | 2 +- 6 files changed, 37 insertions(+), 29 deletions(-) diff --git a/lib/include/srslte/interfaces/ue_interfaces.h b/lib/include/srslte/interfaces/ue_interfaces.h index df5b867ad..d33ff2065 100644 --- a/lib/include/srslte/interfaces/ue_interfaces.h +++ b/lib/include/srslte/interfaces/ue_interfaces.h @@ -305,7 +305,7 @@ class rlc_interface_mac : public srslte::read_pdu_interface public: /* MAC calls has_data() to query whether a logical channel has data to transmit (without * knowing how much. This function should return quickly. */ - virtual bool has_data(const uint32_t lcid) = 0; + virtual bool has_data_locked(const uint32_t lcid) = 0; /* MAC calls RLC to get the buffer state for a logical channel. */ virtual uint32_t get_buffer_state(const uint32_t lcid) = 0; diff --git a/lib/include/srslte/test/ue_test_interfaces.h b/lib/include/srslte/test/ue_test_interfaces.h index f0cc568de..f60882c97 100644 --- a/lib/include/srslte/test/ue_test_interfaces.h +++ b/lib/include/srslte/test/ue_test_interfaces.h @@ -55,7 +55,7 @@ public: class rlc_dummy_interface : public rlc_interface_mac { public: - bool has_data(const uint32_t lcid) override { return false; } + bool has_data_locked(const uint32_t lcid) override { return false; } uint32_t get_buffer_state(const uint32_t lcid) override { return 0; } int read_pdu(uint32_t lcid, uint8_t* payload, uint32_t nof_bytes) override { return 0; } void write_pdu(uint32_t lcid, uint8_t* payload, uint32_t nof_bytes) override {} diff --git a/lib/include/srslte/upper/rlc.h b/lib/include/srslte/upper/rlc.h index b67b444db..1d4903a82 100644 --- a/lib/include/srslte/upper/rlc.h +++ b/lib/include/srslte/upper/rlc.h @@ -63,8 +63,7 @@ public: bool sdu_queue_is_full(uint32_t lcid); // MAC interface - bool has_data(const uint32_t lcid); - bool is_suspended(const uint32_t lcid); + bool has_data_locked(const uint32_t lcid); uint32_t get_buffer_state(const uint32_t lcid); uint32_t get_total_mch_buffer_state(uint32_t lcid); int read_pdu(uint32_t lcid, uint8_t* payload, uint32_t nof_bytes); @@ -77,6 +76,8 @@ public: void write_pdu_mch(uint32_t lcid, uint8_t* payload, uint32_t nof_bytes); // RRC interface + bool is_suspended(const uint32_t lcid); + bool has_data(const uint32_t lcid); void reestablish(); void reestablish(uint32_t lcid); void reset(); diff --git a/lib/src/upper/rlc.cc b/lib/src/upper/rlc.cc index 824b6c961..951ea8798 100644 --- a/lib/src/upper/rlc.cc +++ b/lib/src/upper/rlc.cc @@ -182,7 +182,7 @@ void rlc::empty_queue() } /******************************************************************************* - PDCP interface + PDCP interface (called from Stack thread and therefore no lock required) *******************************************************************************/ void rlc::write_sdu(uint32_t lcid, unique_byte_buffer_t sdu) @@ -244,27 +244,12 @@ bool rlc::sdu_queue_is_full(uint32_t lcid) } /******************************************************************************* - MAC interface + MAC interface (mostly called from PHY workers, lock needs to be hold) *******************************************************************************/ -bool rlc::has_data(uint32_t lcid) +bool rlc::has_data_locked(const uint32_t lcid) { - bool has_data = false; - - if (valid_lcid(lcid)) { - has_data = rlc_array.at(lcid)->has_data(); - } - - return has_data; -} -bool rlc::is_suspended(const uint32_t lcid) -{ - bool ret = false; - - if (valid_lcid(lcid)) { - ret = rlc_array.at(lcid)->is_suspended(); - } - - return ret; + rwlock_read_guard lock(rwlock); + return has_data(lcid); } uint32_t rlc::get_buffer_state(uint32_t lcid) @@ -288,7 +273,6 @@ uint32_t rlc::get_total_mch_buffer_state(uint32_t lcid) uint32_t ret = 0; rwlock_read_guard lock(rwlock); - if (valid_lcid_mrb(lcid)) { ret = rlc_array_mrb.at(lcid)->get_buffer_state(); } @@ -326,6 +310,7 @@ int rlc::read_pdu_mch(uint32_t lcid, uint8_t* payload, uint32_t nof_bytes) return ret; } +// Write PDU methods are called from Stack thread context, no need to acquire the lock void rlc::write_pdu(uint32_t lcid, uint8_t* payload, uint32_t nof_bytes) { if (valid_lcid(lcid)) { @@ -373,9 +358,31 @@ void rlc::write_pdu_mch(uint32_t lcid, uint8_t* payload, uint32_t nof_bytes) } /******************************************************************************* - RRC interface + RRC interface (write-lock ONLY needs to be hold for all calls modifying the RLC array) *******************************************************************************/ +bool rlc::is_suspended(const uint32_t lcid) +{ + bool ret = false; + if (valid_lcid(lcid)) { + ret = rlc_array.at(lcid)->is_suspended(); + } + + return ret; +} + +bool rlc::has_data(uint32_t lcid) +{ + bool has_data = false; + + if (valid_lcid(lcid)) { + has_data = rlc_array.at(lcid)->has_data(); + } + + return has_data; +} + +// Methods modifying the RLC array need to acquire the write-lock void rlc::add_bearer(uint32_t lcid, const rlc_config_t& cnfg) { rwlock_write_guard lock(rwlock); @@ -533,6 +540,7 @@ void rlc::change_lcid(uint32_t old_lcid, uint32_t new_lcid) } } +// Further RRC calls executed from Stack thread, no need to hold lock void rlc::suspend_bearer(uint32_t lcid) { if (valid_lcid(lcid)) { @@ -569,7 +577,6 @@ bool rlc::has_bearer(uint32_t lcid) /******************************************************************************* Helpers (Lock must be hold when calling those) *******************************************************************************/ - bool rlc::valid_lcid(uint32_t lcid) { if (lcid >= SRSLTE_N_RADIO_BEARERS) { diff --git a/srsue/src/stack/mac/mux.cc b/srsue/src/stack/mac/mux.cc index 627956f77..2e7dc949a 100644 --- a/srsue/src/stack/mac/mux.cc +++ b/srsue/src/stack/mac/mux.cc @@ -74,7 +74,7 @@ void mux::step() bool mux::is_pending_any_sdu() { for (auto& channel : logical_channels) { - if (rlc->has_data(channel.lcid)) { + if (rlc->has_data_locked(channel.lcid)) { return true; } } diff --git a/srsue/test/mac_test.cc b/srsue/test/mac_test.cc index 812463341..e5142ffe2 100644 --- a/srsue/test/mac_test.cc +++ b/srsue/test/mac_test.cc @@ -48,7 +48,7 @@ class rlc_dummy : public srsue::rlc_dummy_interface { public: rlc_dummy(srslte::log_filter* log_) : received_bytes(0), log(log_) {} - bool has_data(const uint32_t lcid) final { return ul_queues[lcid] > 0; } + bool has_data_locked(const uint32_t lcid) final { return ul_queues[lcid] > 0; } uint32_t get_buffer_state(const uint32_t lcid) final { return ul_queues[lcid]; } int read_pdu(uint32_t lcid, uint8_t* payload, uint32_t nof_bytes) final {