From a09fb95c25c4dd7a4e70bcf97231b1033fb41f6e Mon Sep 17 00:00:00 2001 From: Andre Puschmann Date: Wed, 13 Jan 2021 22:11:25 +0100 Subject: [PATCH] proc_ra: protect RA procedure from concurrent thread access that patch addreses issue #2199 by defering RA-related calls that are executed from within PHY workers. The time-critical bits of more complex functions such as tb_decoded() are still executed in the PHY worker thread but the state machine manipulation is defered. --- srsue/hdr/stack/mac/proc_ra.h | 110 +++++++++++++------------------- srsue/src/stack/mac/mac.cc | 3 +- srsue/src/stack/mac/proc_ra.cc | 113 ++++++++++++++++++++------------- srsue/test/mac_test.cc | 11 +++- 4 files changed, 125 insertions(+), 112 deletions(-) diff --git a/srsue/hdr/stack/mac/proc_ra.h b/srsue/hdr/stack/mac/proc_ra.h index 26f31d9f6..498c01ef2 100644 --- a/srsue/hdr/stack/mac/proc_ra.h +++ b/srsue/hdr/stack/mac/proc_ra.h @@ -13,6 +13,7 @@ #ifndef SRSUE_PROC_RA_H #define SRSUE_PROC_RA_H +#include #include #include @@ -30,30 +31,7 @@ namespace srsue { class ra_proc : public srslte::timer_callback { public: - ra_proc() : rar_pdu_msg(20) - { - bzero(&softbuffer_rar, sizeof(srslte_softbuffer_rx_t)); - pcap = NULL; - backoff_interval_start = 0; - backoff_interval = 0; - received_target_power_dbm = 0; - ra_rnti = 0; - current_ta = 0; - state = IDLE; - last_msg3_group = RA_GROUP_A; - phy_h = NULL; - mux_unit = NULL; - rrc = NULL; - transmitted_contention_id = 0; - transmitted_crnti = 0; - started_by_pdcch = false; - rar_grant_nbytes = 0; - - noncontention_enabled = false; - next_preamble_idx = 0; - next_prach_mask = 0; - current_task_id = 0; - }; + ra_proc() : rar_pdu_msg(20){}; ~ra_proc(); @@ -82,8 +60,8 @@ public: void timer_expired(uint32_t timer_id); void new_grant_dl(mac_interface_phy_lte::mac_grant_dl_t grant, mac_interface_phy_lte::tb_action_dl_t* action); void tb_decoded_ok(const uint8_t cc_idx, const uint32_t tti); + bool contention_resolution_id_received(uint64_t rx_contention_id); - bool contention_resolution_id_received(uint64_t uecri); void start_pcap(srslte::mac_pcap* pcap); void notify_ra_completed(uint32_t task_id); @@ -104,40 +82,42 @@ private: void response_error(); void complete(); + bool contention_resolution_id_received_unsafe(uint64_t rx_contention_id); + // Buffer to receive RAR PDU - static const uint32_t MAX_RAR_PDU_LEN = 2048; - uint8_t rar_pdu_buffer[MAX_RAR_PDU_LEN]; + static const uint32_t MAX_RAR_PDU_LEN = 2048; + uint8_t rar_pdu_buffer[MAX_RAR_PDU_LEN] = {}; srslte::rar_pdu rar_pdu_msg; // Random Access parameters provided by higher layers defined in 5.1.1 - srslte::rach_cfg_t rach_cfg, new_cfg; + srslte::rach_cfg_t rach_cfg = {}; - int delta_preamble_db; - uint32_t maskIndex; - int preambleIndex; - uint32_t new_ra_msg_len; + int delta_preamble_db = 0; + uint32_t maskIndex = 0; + int preambleIndex = 0; + uint32_t new_ra_msg_len = 0; - bool noncontention_enabled; - uint32_t next_preamble_idx; - uint32_t next_prach_mask; + bool noncontention_enabled = false; + uint32_t next_preamble_idx = 0; + uint32_t next_prach_mask = 0; // Internal variables - uint32_t preambleTransmissionCounter; - uint32_t backoff_param_ms; - uint32_t sel_maskIndex; - uint32_t sel_preamble; - int backoff_interval_start; - uint32_t backoff_interval; - int received_target_power_dbm; - uint32_t ra_rnti; - uint32_t ra_tti; - uint32_t current_ta; + uint32_t preambleTransmissionCounter = 0; + uint32_t backoff_param_ms = 0; + uint32_t sel_maskIndex = 0; + std::atomic sel_preamble = {0}; + int backoff_interval_start = 0; + uint32_t backoff_interval = 0; + int received_target_power_dbm = 0; + uint32_t ra_rnti = SRSLTE_INVALID_RNTI; + uint32_t ra_tti = 0; + uint32_t current_ta = 0; // The task_id is a unique number associated with each RA procedure used to track background tasks - uint32_t current_task_id; + uint32_t current_task_id = 0; - srslte_softbuffer_rx_t softbuffer_rar; + srslte_softbuffer_rx_t softbuffer_rar = {}; - enum { + enum ra_state_t { IDLE = 0, PDCCH_SETUP, RESPONSE_RECEPTION, @@ -145,36 +125,36 @@ private: CONTENTION_RESOLUTION, START_WAIT_COMPLETION, WAITING_COMPLETION - } state; + }; + std::atomic state = {IDLE}; typedef enum { RA_GROUP_A, RA_GROUP_B } ra_group_t; - ra_group_t last_msg3_group; + ra_group_t last_msg3_group = RA_GROUP_A; - uint32_t rar_window_st; + uint32_t rar_window_st = 0; void read_params(); - phy_interface_mac_lte* phy_h; - srslte::log_ref log_h; - mux* mux_unit; - srslte::mac_pcap* pcap; - rrc_interface_mac* rrc; - srslte::ext_task_sched_handle* task_sched = nullptr; + phy_interface_mac_lte* phy_h = nullptr; + srslte::log_ref log_h; + mux* mux_unit = nullptr; + srslte::mac_pcap* pcap = nullptr; + rrc_interface_mac* rrc = nullptr; + srslte::ext_task_sched_handle* task_sched = nullptr; + srslte::task_multiqueue::queue_handle task_queue; srslte::timer_handler::unique_timer* time_alignment_timer = nullptr; srslte::timer_handler::unique_timer contention_resolution_timer; - mac_interface_rrc::ue_rnti_t* rntis; - - uint64_t transmitted_contention_id; - uint16_t transmitted_crnti; + mac_interface_rrc::ue_rnti_t* rntis = nullptr; - std::mutex mutex; + std::atomic transmitted_contention_id = {0}; + std::atomic transmitted_crnti = {0}; - bool started_by_pdcch; - uint32_t rar_grant_nbytes; - bool rar_received; + bool started_by_pdcch = false; + uint32_t rar_grant_nbytes = 0; + bool rar_received = false; }; } // namespace srsue diff --git a/srsue/src/stack/mac/mac.cc b/srsue/src/stack/mac/mac.cc index e78c36184..42d918efd 100644 --- a/srsue/src/stack/mac/mac.cc +++ b/srsue/src/stack/mac/mac.cc @@ -306,7 +306,8 @@ uint16_t mac::get_dl_sched_rnti(uint32_t tti) return SRSLTE_SIRNTI; } } - if (ra_window_start > 0 && ra_window_length > 0 && is_in_window(tti, &ra_window_start, &ra_window_length)) { + if (uernti.rar_rnti && ra_window_start > 0 && ra_window_length > 0 && + is_in_window(tti, &ra_window_start, &ra_window_length)) { Debug("SCHED: Searching RAR-RNTI=0x%x, tti=%d\n", uernti.rar_rnti, tti); return uernti.rar_rnti; } diff --git a/srsue/src/stack/mac/proc_ra.cc b/srsue/src/stack/mac/proc_ra.cc index 630052fc7..9b89c4aa7 100644 --- a/srsue/src/stack/mac/proc_ra.cc +++ b/srsue/src/stack/mac/proc_ra.cc @@ -55,6 +55,8 @@ void ra_proc::init(phy_interface_mac_lte* phy_h_, rrc = rrc_; task_sched = task_sched_; + task_queue = task_sched->make_task_queue(); + time_alignment_timer = time_alignment_timer_; contention_resolution_timer = task_sched->get_unique_timer(); @@ -80,16 +82,16 @@ void ra_proc::start_pcap(srslte::mac_pcap* pcap_) pcap = pcap_; } -/* Sets a new configuration. The configuration is applied by initialization() function */ +// RRC calls to set a new PRACH configuration. +// The configuration is applied by initialization() function. void ra_proc::set_config(srslte::rach_cfg_t& rach_cfg_) { - std::unique_lock ul(mutex); - new_cfg = rach_cfg_; + rach_cfg = rach_cfg_; } +// RRC might also call this to set additional params during mobility void ra_proc::set_config_ded(uint32_t preamble_index, uint32_t prach_mask) { - std::unique_lock ul(mutex); next_preamble_idx = preamble_index; next_prach_mask = prach_mask; noncontention_enabled = true; @@ -98,10 +100,6 @@ void ra_proc::set_config_ded(uint32_t preamble_index, uint32_t prach_mask) /* Reads the configuration and configures internal variables */ void ra_proc::read_params() { - mutex.lock(); - rach_cfg = new_cfg; - mutex.unlock(); - // Read initialization parameters if (noncontention_enabled) { preambleIndex = next_preamble_idx; @@ -128,7 +126,7 @@ void ra_proc::read_params() */ void ra_proc::step(uint32_t tti_) { - switch (state) { + switch (state.load()) { case IDLE: break; case PDCCH_SETUP: @@ -160,8 +158,9 @@ void ra_proc::state_pdcch_setup() if (info.is_transmitted) { ra_tti = info.tti_ra; ra_rnti = 1 + (ra_tti % 10) + (10 * info.f_id); - rInfo("seq=%d, ra-rnti=0x%x, ra-tti=%d, f_id=%d\n", sel_preamble, ra_rnti, info.tti_ra, info.f_id); - srslte::console("Random Access Transmission: seq=%d, tti=%d, ra-rnti=0x%x\n", sel_preamble, info.tti_ra, ra_rnti); + rInfo("seq=%d, ra-rnti=0x%x, ra-tti=%d, f_id=%d\n", sel_preamble.load(), ra_rnti, info.tti_ra, info.f_id); + srslte::console( + "Random Access Transmission: seq=%d, tti=%d, ra-rnti=0x%x\n", sel_preamble.load(), info.tti_ra, ra_rnti); rar_window_st = ra_tti + 3; rntis->rar_rnti = ra_rnti; state = RESPONSE_RECEPTION; @@ -314,7 +313,7 @@ void ra_proc::resource_selection() } rDebug("Selected preambleIndex=%d maskIndex=%d GroupA=%d, GroupB=%d\n", - sel_preamble, + sel_preamble.load(), sel_maskIndex, rach_cfg.nof_groupA_preambles, nof_groupB_preambles); @@ -326,12 +325,11 @@ void ra_proc::resource_selection() /* Preamble transmission as defined in 5.1.3 */ void ra_proc::preamble_transmission() { - received_target_power_dbm = rach_cfg.iniReceivedTargetPower + delta_preamble_db + (preambleTransmissionCounter - 1) * rach_cfg.powerRampingStep; phy_h->prach_send(sel_preamble, sel_maskIndex - 1, received_target_power_dbm); - rntis->rar_rnti = 0; + rntis->rar_rnti = SRSLTE_INVALID_RNTI; ra_tti = 0; rar_received = false; backoff_interval_start = -1; @@ -385,8 +383,10 @@ void ra_proc::new_grant_dl(mac_interface_phy_lte::mac_grant_dl_t grant, mac_inte } } -/* Called upon the successful decoding of a TB addressed to RA-RNTI. - * Processes the reception of a RAR as defined in 5.1.4 +/* Called from PHY worker upon the successful decoding of a TB addressed to RA-RNTI. + * We extract the most relevant and time critical details from the RAR PDU, + * as definied in 5.1.4 and then defer the handling of the RA state machine to be + * executed on the Stack thread. */ void ra_proc::tb_decoded_ok(const uint8_t cc_idx, const uint32_t tti) { @@ -394,10 +394,12 @@ void ra_proc::tb_decoded_ok(const uint8_t cc_idx, const uint32_t tti) pcap->write_dl_ranti(rar_pdu_buffer, rar_grant_nbytes, ra_rnti, true, tti, cc_idx); } - rDebug("RAR decoded successfully TBS=%d\n", rar_grant_nbytes); - rar_pdu_msg.init_rx(rar_grant_nbytes); - rar_pdu_msg.parse_packet(rar_pdu_buffer); + if (rar_pdu_msg.parse_packet(rar_pdu_buffer) != SRSLTE_SUCCESS) { + rError("Error decoding RAR PDU\n"); + } + + rDebug("RAR decoded successfully TBS=%d\n", rar_grant_nbytes); // Set Backoff parameter if (rar_pdu_msg.has_backoff()) { @@ -417,24 +419,21 @@ void ra_proc::tb_decoded_ok(const uint8_t cc_idx, const uint32_t tti) // TODO: Indicate received target power // phy_h->set_target_power_rar(iniReceivedTargetPower, (preambleTransmissionCounter-1)*powerRampingStep); - uint8_t grant[srslte::rar_subh::RAR_GRANT_LEN]; + uint8_t grant[srslte::rar_subh::RAR_GRANT_LEN] = {}; rar_pdu_msg.get()->get_sched_grant(grant); - rntis->rar_rnti = 0; + rntis->rar_rnti = SRSLTE_INVALID_RNTI; phy_h->set_rar_grant(grant, rar_pdu_msg.get()->get_temp_crnti()); current_ta = rar_pdu_msg.get()->get_ta_cmd(); rInfo("RAPID=%d, TA=%d, T-CRNTI=0x%x\n", - sel_preamble, + sel_preamble.load(), rar_pdu_msg.get()->get_ta_cmd(), rar_pdu_msg.get()->get_temp_crnti()); - if (preambleIndex > 0) { - // Preamble selected by Network - complete(); - } else { - // Preamble selected by UE MAC + // Perform actions when preamble was selected by UE MAC + if (preambleIndex <= 0) { mux_unit->msg3_prepare(); rntis->temp_rnti = rar_pdu_msg.get()->get_temp_crnti(); @@ -446,7 +445,7 @@ void ra_proc::tb_decoded_ok(const uint8_t cc_idx, const uint32_t tti) // If we have a C-RNTI, tell Mux unit to append C-RNTI CE if no CCCH SDU transmission if (transmitted_crnti) { - rInfo("Appending C-RNTI MAC CE 0x%x in next transmission\n", transmitted_crnti); + rInfo("Appending C-RNTI MAC CE 0x%x in next transmission\n", transmitted_crnti.load()); mux_unit->append_crnti_ce_next_tx(transmitted_crnti); } } @@ -454,8 +453,13 @@ void ra_proc::tb_decoded_ok(const uint8_t cc_idx, const uint32_t tti) // Save transmitted UE contention id, as defined by higher layers transmitted_contention_id = rntis->contention_id; - rDebug("Waiting for Contention Resolution\n"); - state = CONTENTION_RESOLUTION; + task_queue.push([this]() { + rDebug("Waiting for Contention Resolution\n"); + state = CONTENTION_RESOLUTION; + }); + } else { + // Preamble selected by Network, defer result handling + task_queue.push([this]() { complete(); }); } } else { if (rar_pdu_msg.get()->has_rapid()) { @@ -549,9 +553,22 @@ void ra_proc::timer_expired(uint32_t timer_id) } /* Function called by MAC when a Contention Resolution ID CE is received. - * Performs the actions defined in 5.1.5 for Temporal C-RNTI Contention Resolution + * Since this is called from within a PHY worker thread, we enqueue the handling, + * check that the contention resolution IDs match and return so the DL TB can be acked. + * + * The RA-related actions are scheduled to be executed on the Stack thread, + * even if we realize later that we have received that in a wrong state. */ bool ra_proc::contention_resolution_id_received(uint64_t rx_contention_id) +{ + task_queue.push([this, rx_contention_id]() { contention_resolution_id_received_unsafe(rx_contention_id); }); + return (transmitted_contention_id == rx_contention_id); +} + +/* + * Performs the actions defined in 5.1.5 for Temporal C-RNTI Contention Resolution + */ +bool ra_proc::contention_resolution_id_received_unsafe(uint64_t rx_contention_id) { bool uecri_successful = false; @@ -571,7 +588,7 @@ bool ra_proc::contention_resolution_id_received(uint64_t rx_contention_id) complete(); } else { rInfo("Transmitted UE Contention Id differs from received Contention ID (0x%" PRIx64 " != 0x%" PRIx64 ")\n", - transmitted_contention_id, + transmitted_contention_id.load(), rx_contention_id); // Discard MAC PDU @@ -584,17 +601,21 @@ bool ra_proc::contention_resolution_id_received(uint64_t rx_contention_id) return uecri_successful; } +// Called from PHY worker context, defer actions therefore. void ra_proc::pdcch_to_crnti(bool is_new_uplink_transmission) { - // TS 36.321 Section 5.1.5 - rDebug("PDCCH to C-RNTI received %s new UL transmission\n", is_new_uplink_transmission ? "with" : "without"); - if ((!started_by_pdcch && is_new_uplink_transmission) || started_by_pdcch) { - rDebug("PDCCH for C-RNTI received\n"); - contention_resolution_timer.stop(); - complete(); - } + task_queue.push([this, is_new_uplink_transmission]() { + // TS 36.321 Section 5.1.5 + rDebug("PDCCH to C-RNTI received %s new UL transmission\n", is_new_uplink_transmission ? "with" : "without"); + if ((!started_by_pdcch && is_new_uplink_transmission) || started_by_pdcch) { + rDebug("PDCCH for C-RNTI received\n"); + contention_resolution_timer.stop(); + complete(); + } + }); } +// Called from the Stack thread void ra_proc::update_rar_window(int& rar_window_start, int& rar_window_length) { if (state != RESPONSE_RECEPTION) { @@ -611,13 +632,19 @@ void ra_proc::update_rar_window(int& rar_window_start, int& rar_window_length) // Restart timer at each Msg3 HARQ retransmission (5.1.5) void ra_proc::harq_retx() { - rInfo("Restarting ContentionResolutionTimer=%d ms\n", contention_resolution_timer.duration()); - contention_resolution_timer.run(); + task_queue.push([this]() { + rInfo("Restarting ContentionResolutionTimer=%d ms\n", contention_resolution_timer.duration()); + contention_resolution_timer.run(); + }); } +// Called from PHY worker thread void ra_proc::harq_max_retx() { - Warning("Contention Resolution is considered not successful. Stopping PDCCH Search and going to Response Error\n"); - response_error(); + task_queue.push([this]() { + Warning("Contention Resolution is considered not successful. Stopping PDCCH Search and going to Response Error\n"); + response_error(); + }); } + } // namespace srsue diff --git a/srsue/test/mac_test.cc b/srsue/test/mac_test.cc index 4137ab5c1..30823a5a1 100644 --- a/srsue/test/mac_test.cc +++ b/srsue/test/mac_test.cc @@ -2097,7 +2097,7 @@ int run_mac_ra_test(struct ra_test test, mac* mac, phy_dummy* phy, uint32_t* tti // Step to contention resolution. Make sure timer does not start until Msg3 is transmitted // and restarts on every retx - for (int k = 0; k < test.rach_cfg.ra_supervision_info.mac_contention_resolution_timer.to_number() - 1; k++) { + for (int k = 0; k < test.rach_cfg.ra_supervision_info.mac_contention_resolution_timer.to_number() - 2; k++) { stack->run_tti(tti); TESTASSERT(mac->get_dl_sched_rnti(tti) == (test.crnti ? test.crnti : test.temp_rnti)); tti++; @@ -2115,6 +2115,7 @@ int run_mac_ra_test(struct ra_test test, mac* mac, phy_dummy* phy, uint32_t* tti } if (test.nof_msg3_retx == test.rach_cfg.max_harq_msg3_tx) { + stack->run_tti(tti); // RNTI will be reset for next TTI TESTASSERT(mac->get_dl_sched_rnti(tti) != temp_rnti); break; } @@ -2164,6 +2165,7 @@ int run_mac_ra_test(struct ra_test test, mac* mac, phy_dummy* phy, uint32_t* tti // RA procedure should be completed here if (test.check_ra_successful) { + stack->run_tti(tti); stack->run_tti(tti); TESTASSERT(phy->get_crnti() == (test.crnti ? test.crnti : test.temp_rnti)); TESTASSERT(mac->get_dl_sched_rnti(tti) == (test.crnti ? test.crnti : test.temp_rnti)); @@ -2218,6 +2220,9 @@ int mac_random_access_test() set_mac_cfg_t_rach_cfg_common(&mac_cfg, rach_cfg); mac.set_config(mac_cfg); + uint32 tti = 0; + stack.run_tti(tti++); // make sure MAC/PRACH config is applied + // generate config for LCIDs in different LCGs than CCCH std::vector lcids; logical_channel_config_t config = {}; @@ -2239,8 +2244,6 @@ int mac_random_access_test() rlc.write_sdu(0, 6); // UL-CCCH with Msg3 rlc.write_sdu(3, 100); // DRB data on other LCG - uint32 tti = 0; - // Structure that defines the test to be executed struct ra_test my_test = {}; uint32_t test_id = 1; @@ -2339,6 +2342,7 @@ int mac_random_access_test() my_test.check_ra_successful = false; my_test.send_valid_ul_grant = false; TESTASSERT(!run_mac_ra_test(my_test, &mac, &phy, &tti, &stack)); + stack.run_tti(tti++); // need to wait until complete RA result is signalled TESTASSERT(!rrc.ho_finish_successful); TESTASSERT(rrc.rach_problem == 2); @@ -2358,6 +2362,7 @@ int mac_random_access_test() my_test.msg4_enable = true; my_test.send_valid_ul_grant = true; TESTASSERT(!run_mac_ra_test(my_test, &mac, &phy, &tti, &stack)); + stack.run_tti(tti++); // need to wait until complete RA result is signalled TESTASSERT(rrc.ho_finish_successful); // Test 9: Test non-Contention based HO. Used in HO but preamble is given by the network. In addition to checking