From 3e4526a6ad02021888277aab588a352991dd17ee Mon Sep 17 00:00:00 2001 From: Laurent THOMAS Date: Thu, 16 May 2024 14:05:39 +0200 Subject: [PATCH 1/5] rename persisted ul harq status per pid with a uniq name as this variable is persisted in memory and accessed by multiple threads, a uniq name helps to debug --- openair1/PHY/INIT/nr_init_ue.c | 2 +- openair1/PHY/NR_UE_TRANSPORT/nr_transport_ue.h | 2 +- openair1/PHY/NR_UE_TRANSPORT/nr_ulsch_ue.c | 2 +- openair1/PHY/defs_nr_UE.h | 1 + openair1/SCHED_NR_UE/fapi_nr_ue_l1.c | 2 +- openair1/SCHED_NR_UE/phy_procedures_nr_ue.c | 2 +- 6 files changed, 6 insertions(+), 5 deletions(-) diff --git a/openair1/PHY/INIT/nr_init_ue.c b/openair1/PHY/INIT/nr_init_ue.c index 83b2bd75ac..5c7c667e29 100644 --- a/openair1/PHY/INIT/nr_init_ue.c +++ b/openair1/PHY/INIT/nr_init_ue.c @@ -645,7 +645,7 @@ void clean_UE_harq(PHY_VARS_NR_UE *UE) for (int harq_pid = 0; harq_pid < NR_MAX_ULSCH_HARQ_PROCESSES; harq_pid++) { NR_UL_UE_HARQ_t *ul_harq_process = &UE->ul_harq_processes[harq_pid]; ul_harq_process->tx_status = NEW_TRANSMISSION_HARQ; - ul_harq_process->status = SCH_IDLE; + ul_harq_process->ULstatus = SCH_IDLE; ul_harq_process->round = 0; } } diff --git a/openair1/PHY/NR_UE_TRANSPORT/nr_transport_ue.h b/openair1/PHY/NR_UE_TRANSPORT/nr_transport_ue.h index 8fea51ad75..05b7c9afd2 100644 --- a/openair1/PHY/NR_UE_TRANSPORT/nr_transport_ue.h +++ b/openair1/PHY/NR_UE_TRANSPORT/nr_transport_ue.h @@ -47,7 +47,7 @@ typedef struct { /// HARQ tx status harq_result_t tx_status; /// Status Flag indicating for this ULSCH (idle,active,disabled) - SCH_status_t status; + SCH_status_t ULstatus; /// Last TPC command uint8_t TPC; /// Length of ACK information (bits) diff --git a/openair1/PHY/NR_UE_TRANSPORT/nr_ulsch_ue.c b/openair1/PHY/NR_UE_TRANSPORT/nr_ulsch_ue.c index 2ac06bfe89..225a20aa36 100644 --- a/openair1/PHY/NR_UE_TRANSPORT/nr_ulsch_ue.c +++ b/openair1/PHY/NR_UE_TRANSPORT/nr_ulsch_ue.c @@ -557,7 +557,7 @@ void nr_ue_ulsch_procedures(PHY_VARS_NR_UE *UE, NR_UL_UE_HARQ_t *harq_process_ulsch = NULL; harq_process_ulsch = &UE->ul_harq_processes[harq_pid]; - harq_process_ulsch->status = SCH_IDLE; + harq_process_ulsch->ULstatus = SCH_IDLE; /////////// //////////////////////////////////////////////////////////////////////// } diff --git a/openair1/PHY/defs_nr_UE.h b/openair1/PHY/defs_nr_UE.h index c746bd69b8..93b9f360e5 100644 --- a/openair1/PHY/defs_nr_UE.h +++ b/openair1/PHY/defs_nr_UE.h @@ -642,6 +642,7 @@ typedef struct nr_rxtx_thread_data_s { nr_phy_data_t phy_data; int tx_wait_for_dlsch; int rx_offset; + int stream_status; } nr_rxtx_thread_data_t; typedef struct LDPCDecode_ue_s { diff --git a/openair1/SCHED_NR_UE/fapi_nr_ue_l1.c b/openair1/SCHED_NR_UE/fapi_nr_ue_l1.c index 8560e3de82..84dd690a64 100644 --- a/openair1/SCHED_NR_UE/fapi_nr_ue_l1.c +++ b/openair1/SCHED_NR_UE/fapi_nr_ue_l1.c @@ -473,7 +473,7 @@ static void nr_ue_scheduled_response_ul(PHY_VARS_NR_UE *phy, fapi_nr_ul_config_r pdu->pusch_config_pdu.tx_request_body.pdu_length); } - harq_process_ul_ue->status = ACTIVE; + harq_process_ul_ue->ULstatus = ACTIVE; pdu->pdu_type = FAPI_NR_UL_CONFIG_TYPE_DONE; // not handle it any more } break; diff --git a/openair1/SCHED_NR_UE/phy_procedures_nr_ue.c b/openair1/SCHED_NR_UE/phy_procedures_nr_ue.c index 935563c35f..05c70542a5 100644 --- a/openair1/SCHED_NR_UE/phy_procedures_nr_ue.c +++ b/openair1/SCHED_NR_UE/phy_procedures_nr_ue.c @@ -294,7 +294,7 @@ void phy_procedures_nrUE_TX(PHY_VARS_NR_UE *ue, const UE_nr_rxtx_proc_t *proc, n start_meas(&ue->phy_proc_tx); for (uint8_t harq_pid = 0; harq_pid < NR_MAX_ULSCH_HARQ_PROCESSES; harq_pid++) { - if (ue->ul_harq_processes[harq_pid].status == ACTIVE) { + if (ue->ul_harq_processes[harq_pid].ULstatus == ACTIVE) { nr_ue_ulsch_procedures(ue, harq_pid, frame_tx, slot_tx, gNB_id, phy_data, (c16_t **)&txdataF); } } From e217980249241234bad82e61ea9a7899b3c13bd9 Mon Sep 17 00:00:00 2001 From: Laurent THOMAS Date: Thu, 16 May 2024 14:06:24 +0200 Subject: [PATCH 2/5] simplify and make better performance the mutual exclusion of processSlotTX() function --- executables/nr-ue.c | 36 +++++++++++++++++------------------- 1 file changed, 17 insertions(+), 19 deletions(-) diff --git a/executables/nr-ue.c b/executables/nr-ue.c index fc9debee2b..68b747de32 100644 --- a/executables/nr-ue.c +++ b/executables/nr-ue.c @@ -526,6 +526,14 @@ void processSlotTX(void *arg) PHY_VARS_NR_UE *UE = rxtxD->UE; nr_phy_data_tx_t phy_data = {0}; + // Force sequential execution, even if we launch in // for all slots + // at least ULstatus variable is a pure race condition that is quickly detected by assert() in the code because one thread sets it + // to active, so the other thread try to steal&run the ul work + if (rxtxD->stream_status == 2) { + notifiedFIFO_elt_t *res = pullNotifiedFIFO(UE->tx_resume_ind_fifo + proc->nr_slot_tx); + delNotifiedFIFO_elt(res); + } + if (UE->if_inst) UE->if_inst->slot_indication(UE->Mod_id); @@ -560,11 +568,6 @@ void processSlotTX(void *arg) instead, we may run in place the processSlotTX() when the conditions are met (when a decreasing tx_wait_for_dlsch[slot] will become 0) It will remove the condition signals (for a thread safe semaphore or counter) and make the system simpler - This require also other modifications to - remove txFifo that is also a big issue - add out of order RF board sending, because, - if we encode and send tx slot as soon as we can, - it will be thrown out of order, especially in TDD mode */ notifiedFIFO_elt_t *res = pollNotifiedFIFO(UE->tx_resume_ind_fifo + proc->nr_slot_tx); if (res) @@ -590,6 +593,11 @@ void processSlotTX(void *arg) phy_procedures_nrUE_TX(UE, proc, &phy_data); } + notifiedFIFO_elt_t *newElt = newNotifiedFIFO_elt(sizeof(int), 0, NULL, NULL); + int *msgData = (int *)NotifiedFifoData(newElt); + int newslot = (proc->nr_slot_tx + 1) % UE->frame_parms.slots_per_frame; + *msgData = newslot; + pushNotifiedFIFO(UE->tx_resume_ind_fifo + newslot, newElt); RU_write(rxtxD); } @@ -755,9 +763,6 @@ void *UE_thread(void *arg) notifiedFIFO_t nf; initNotifiedFIFO(&nf); - notifiedFIFO_t txFifo; - initNotifiedFIFO(&txFifo); - notifiedFIFO_t freeBlocks; initNotifiedFIFO_nothreadSafe(&freeBlocks); @@ -937,25 +942,18 @@ void *UE_thread(void *arg) // Start TX slot processing here. It runs in parallel with RX slot processing // in current code, DURATION_RX_TO_TX constant is the limit to get UL data to encode from a RX slot - notifiedFIFO_elt_t *newTx = newNotifiedFIFO_elt(sizeof(nr_rxtx_thread_data_t), curMsg.proc.nr_slot_tx, &txFifo, processSlotTX); + notifiedFIFO_elt_t *newTx = newNotifiedFIFO_elt(sizeof(nr_rxtx_thread_data_t), curMsg.proc.nr_slot_tx, NULL, processSlotTX); nr_rxtx_thread_data_t *curMsgTx = (nr_rxtx_thread_data_t *)NotifiedFifoData(newTx); curMsgTx->proc = curMsg.proc; curMsgTx->writeBlockSize = writeBlockSize; curMsgTx->proc.timestamp_tx = writeTimestamp; curMsgTx->UE = UE; curMsgTx->tx_wait_for_dlsch = tx_wait_for_dlsch[curMsgTx->proc.nr_slot_tx]; + curMsgTx->stream_status = start_rx_stream; + start_rx_stream = 2; tx_wait_for_dlsch[curMsgTx->proc.nr_slot_tx] = 0; pushTpool(&(get_nrUE_params()->Tpool), newTx); - - // Wait for TX slot processing to finish - // Should be removed when bugs, race conditions, will be fixed - notifiedFIFO_elt_t *res; - res = pullTpool(&txFifo, &(get_nrUE_params()->Tpool)); - if (res == NULL) - LOG_E(PHY, "Tpool has been aborted\n"); - else - delNotifiedFIFO_elt(res); - } // while !oai_exit + } return NULL; } From 7a631ce3a29ebbaef0ebe6391d6b5036b22c193a Mon Sep 17 00:00:00 2001 From: Robert Schmidt Date: Thu, 23 May 2024 16:41:20 +0200 Subject: [PATCH 3/5] Use an enum for stream_status states --- executables/nr-ue.c | 14 +++++++------- openair1/PHY/defs_nr_UE.h | 4 +++- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/executables/nr-ue.c b/executables/nr-ue.c index 68b747de32..5b4326239b 100644 --- a/executables/nr-ue.c +++ b/executables/nr-ue.c @@ -529,7 +529,7 @@ void processSlotTX(void *arg) // Force sequential execution, even if we launch in // for all slots // at least ULstatus variable is a pure race condition that is quickly detected by assert() in the code because one thread sets it // to active, so the other thread try to steal&run the ul work - if (rxtxD->stream_status == 2) { + if (rxtxD->stream_status == STREAM_STATUS_SYNCED) { notifiedFIFO_elt_t *res = pullNotifiedFIFO(UE->tx_resume_ind_fifo + proc->nr_slot_tx); delNotifiedFIFO_elt(res); } @@ -751,7 +751,7 @@ void *UE_thread(void *arg) PHY_VARS_NR_UE *UE = (PHY_VARS_NR_UE *) arg; // int tx_enabled = 0; void *rxp[NB_ANTENNAS_RX]; - int start_rx_stream = 0; + enum stream_status_e stream_status = STREAM_STATUS_UNSYNC; fapi_nr_config_request_t *cfg = &UE->nrUE_config; int tmp = openair0_device_load(&(UE->rfdevice), &openair0_cfg[0]); AssertFatal(tmp == 0, "Could not load the device\n"); @@ -801,7 +801,7 @@ void *UE_thread(void *arg) intialSyncOffset = syncMsg->rx_offset; } delNotifiedFIFO_elt(res); - start_rx_stream = 0; + stream_status = STREAM_STATUS_UNSYNC; } else { if (IS_SOFTMODEM_IQPLAYER || IS_SOFTMODEM_IQRECORDER) { // For IQ recorder-player we force synchronization to happen in 280 ms @@ -831,8 +831,8 @@ void *UE_thread(void *arg) continue; } - if (start_rx_stream == 0) { - start_rx_stream=1; + if (stream_status == STREAM_STATUS_UNSYNC) { + stream_status = STREAM_STATUS_SYNCING; syncInFrame(UE, &sync_timestamp, intialSyncOffset); shiftForNextFrame = 0; // will be used to track clock drift // read in first symbol @@ -949,8 +949,8 @@ void *UE_thread(void *arg) curMsgTx->proc.timestamp_tx = writeTimestamp; curMsgTx->UE = UE; curMsgTx->tx_wait_for_dlsch = tx_wait_for_dlsch[curMsgTx->proc.nr_slot_tx]; - curMsgTx->stream_status = start_rx_stream; - start_rx_stream = 2; + curMsgTx->stream_status = stream_status; + stream_status = STREAM_STATUS_SYNCED; tx_wait_for_dlsch[curMsgTx->proc.nr_slot_tx] = 0; pushTpool(&(get_nrUE_params()->Tpool), newTx); } diff --git a/openair1/PHY/defs_nr_UE.h b/openair1/PHY/defs_nr_UE.h index 93b9f360e5..f7eedcef3b 100644 --- a/openair1/PHY/defs_nr_UE.h +++ b/openair1/PHY/defs_nr_UE.h @@ -632,6 +632,8 @@ typedef struct nr_phy_data_s { sl_nr_rx_config_type_enum_t sl_rx_action; } nr_phy_data_t; + +enum stream_status_e { STREAM_STATUS_UNSYNC, STREAM_STATUS_SYNCING, STREAM_STATUS_SYNCED}; /* this structure is used to pass both UE phy vars and * proc to the function UE_thread_rxn_txnp4 */ @@ -642,7 +644,7 @@ typedef struct nr_rxtx_thread_data_s { nr_phy_data_t phy_data; int tx_wait_for_dlsch; int rx_offset; - int stream_status; + enum stream_status_e stream_status; } nr_rxtx_thread_data_t; typedef struct LDPCDecode_ue_s { From b5159b5e47e98da9d02545dbb0bf101fbb78ae9c Mon Sep 17 00:00:00 2001 From: Robert Schmidt Date: Fri, 24 May 2024 06:54:15 +0200 Subject: [PATCH 4/5] Revert "ci: set min_rxtxtime = 2 in OAIUE pipeline" This reverts commit 31b4f48e29170e25e5375e781860cf14eaebee43. --- ci-scripts/conf_files/gnb.sa.band78.106prb.n310.7ds2u.conf | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci-scripts/conf_files/gnb.sa.band78.106prb.n310.7ds2u.conf b/ci-scripts/conf_files/gnb.sa.band78.106prb.n310.7ds2u.conf index ce7b354eba..540774e2d1 100644 --- a/ci-scripts/conf_files/gnb.sa.band78.106prb.n310.7ds2u.conf +++ b/ci-scripts/conf_files/gnb.sa.band78.106prb.n310.7ds2u.conf @@ -19,7 +19,7 @@ gNBs = ////////// Physical parameters: - min_rxtxtime = 2; + min_rxtxtime = 5; do_SRS = 0; force_256qam_off = 1; pdsch_AntennaPorts_N1 = 1; From 58335443b132cbdfa2e473ca840ad7a7bfe5a143 Mon Sep 17 00:00:00 2001 From: Robert Schmidt Date: Fri, 24 May 2024 06:54:23 +0200 Subject: [PATCH 5/5] Revert "remove asserts on min RX/TX, set the RX/TX shift as 2 in the UE" This reverts commit 0d0e1111354877464a213663c60dfb45872e399e. It was set to UE RX to TX time to 2 as it was deemed that the UE could sustain this time. However, we noticed a degradation of performance, so reset the RX to TX time to something higher to ensure proper performance. --- openair1/PHY/impl_defs_top.h | 2 +- openair2/LAYER2/NR_MAC_UE/nr_ue_procedures.c | 10 ++++++++++ openair2/LAYER2/NR_MAC_UE/nr_ue_scheduler.c | 18 ++++++++++++++++++ 3 files changed, 29 insertions(+), 1 deletion(-) diff --git a/openair1/PHY/impl_defs_top.h b/openair1/PHY/impl_defs_top.h index e43100d32a..c06cd13ff7 100644 --- a/openair1/PHY/impl_defs_top.h +++ b/openair1/PHY/impl_defs_top.h @@ -272,7 +272,7 @@ /* - between reception of pdsch and tarnsmission of its acknowlegment */ /* - between reception of un uplink grant and its related transmission */ // should be 2 as per NR standard, but current UE is not able to perform this value -#define NR_UE_CAPABILITY_SLOT_RX_TO_TX (2) +#define NR_UE_CAPABILITY_SLOT_RX_TO_TX (3) #define DURATION_RX_TO_TX (NR_UE_CAPABILITY_SLOT_RX_TO_TX) diff --git a/openair2/LAYER2/NR_MAC_UE/nr_ue_procedures.c b/openair2/LAYER2/NR_MAC_UE/nr_ue_procedures.c index 60f0d2d5d3..490235445f 100644 --- a/openair2/LAYER2/NR_MAC_UE/nr_ue_procedures.c +++ b/openair2/LAYER2/NR_MAC_UE/nr_ue_procedures.c @@ -785,6 +785,10 @@ static int nr_ue_process_dci_dl_10(NR_UE_MAC_INST_t *mac, } if (dci_ind->rnti != mac->ra.ra_rnti && dci_ind->rnti != SI_RNTI) + AssertFatal(1 + dci->pdsch_to_harq_feedback_timing_indicator.val > DURATION_RX_TO_TX, + "PDSCH to HARQ feedback time (%d) needs to be higher than DURATION_RX_TO_TX (%d).\n", + 1 + dci->pdsch_to_harq_feedback_timing_indicator.val, + DURATION_RX_TO_TX); // set the harq status at MAC for feedback set_harq_status(mac, @@ -1112,6 +1116,12 @@ static int nr_ue_process_dci_dl_11(NR_UE_MAC_INST_t *mac, // according to TS 38.213 Table 9.2.3-1 uint8_t feedback_ti = pucch_Config->dl_DataToUL_ACK->list.array[dci->pdsch_to_harq_feedback_timing_indicator.val][0]; + AssertFatal(feedback_ti > DURATION_RX_TO_TX, + "PDSCH to HARQ feedback time (%d) needs to be higher than DURATION_RX_TO_TX (%d). Min feedback time set in config " + "file (min_rxtxtime).\n", + feedback_ti, + DURATION_RX_TO_TX); + // set the harq status at MAC for feedback set_harq_status(mac, dci->pucch_resource_indicator, diff --git a/openair2/LAYER2/NR_MAC_UE/nr_ue_scheduler.c b/openair2/LAYER2/NR_MAC_UE/nr_ue_scheduler.c index c18189a683..d04fe1a295 100644 --- a/openair2/LAYER2/NR_MAC_UE/nr_ue_scheduler.c +++ b/openair2/LAYER2/NR_MAC_UE/nr_ue_scheduler.c @@ -1007,6 +1007,9 @@ void nr_ue_aperiodic_srs_scheduling(NR_UE_MAC_INST_t *mac, long resource_trigger return; } + AssertFatal(slot_offset > DURATION_RX_TO_TX, + "Slot offset between DCI and aperiodic SRS (%d) needs to be higher than DURATION_RX_TO_TX (%d)\n", + slot_offset, DURATION_RX_TO_TX); int n_slots_frame = nr_slots_per_frame[current_UL_BWP->scs]; int sched_slot = (slot + slot_offset) % n_slots_frame; NR_TDD_UL_DL_ConfigCommon_t *tdd_config = mac->tdd_UL_DL_ConfigurationCommon; @@ -1465,6 +1468,13 @@ int nr_ue_pusch_scheduler(const NR_UE_MAC_INST_t *mac, AssertFatal(1 == 0, "Invalid numerology %i\n", mu); } + AssertFatal((k2 + delta) > DURATION_RX_TO_TX, + "Slot offset (%ld) for Msg3 needs to be higher than DURATION_RX_TO_TX (%d). Please set min_rxtxtime at least to %d in gNB config file or gNBs.[0].min_rxtxtime=%d via command line.\n", + k2, + DURATION_RX_TO_TX, + DURATION_RX_TO_TX, + DURATION_RX_TO_TX); + *slot_tx = (current_slot + k2 + delta) % nr_slots_per_frame[mu]; if (current_slot + k2 + delta >= nr_slots_per_frame[mu]){ *frame_tx = (current_frame + 1) % 1024; @@ -1473,6 +1483,14 @@ int nr_ue_pusch_scheduler(const NR_UE_MAC_INST_t *mac, } } else { + + AssertFatal(k2 > DURATION_RX_TO_TX, + "Slot offset K2 (%ld) needs to be higher than DURATION_RX_TO_TX (%d). Please set min_rxtxtime at least to %d in gNB config file or gNBs.[0].min_rxtxtime=%d via command line.\n", + k2, + DURATION_RX_TO_TX, + DURATION_RX_TO_TX, + DURATION_RX_TO_TX); + if (k2 < 0) { // This can happen when a false DCI is received LOG_W(PHY, "%d.%d. Received k2 %ld\n", current_frame, current_slot, k2); return -1;