rlc_tx_am: no ReTx of unsent bytes for NACK'ed SDU under segmentation

This prevents the ReTx overtaking the actual transmission of
an SDU under segmentation, as ReTx has priority.
If such ReTx is ACK'ed by the peer, the original SDU under
segmentation falls out of the tx_window.

Author: robertfalkenberg

lib/rlc/rlc_tx_am_entity.cpp

@@ -147,8 +147,8 @@ size_t rlc_tx_am_entity::pull_pdu(span<uint8_t> rlc_pdu_buf)
       pcap.push_pdu(pcap_context, rlc_pdu_buf.subspan(0, pdu_len));
       return pdu_len;
     }
-    sn_under_segmentation = INVALID_RLC_SN;
     logger.log_error("SDU under segmentation does not exist in tx_window. sn={}", sn_under_segmentation);
+    sn_under_segmentation = INVALID_RLC_SN;
     // attempt to send next SDU
   }
 
@@ -745,16 +745,27 @@ bool rlc_tx_am_entity::handle_nack(rlc_am_status_nack nack)
     return false;
   }
 
-  uint32_t sdu_length = (*tx_window)[nack.nack_sn].sdu.length();
+  const rlc_tx_am_sdu_info& sdu_info   = (*tx_window)[nack.nack_sn];
+  uint32_t                  sdu_length = sdu_info.sdu.length();
 
   // Convert NACK for full SDUs into NACK with segment offset and length
   if (!nack.has_so) {
     nack.so_start = 0;
     nack.so_end   = sdu_length - 1;
   }
-  // Replace "end"-mark with actual SDU length
   if (nack.so_end == rlc_am_status_nack::so_end_of_sdu) {
-    nack.so_end = sdu_length - 1;
+    if (nack.nack_sn != sn_under_segmentation) {
+      // Replace "end"-mark with actual SDU length
+      nack.so_end = sdu_length - 1;
+    } else {
+      // The SDU is still under segmentation; RETX only what was already sent; avoid RETX overtaking the original
+      if (sdu_info.next_so == 0) {
+        logger.log_error(
+            "Cannot RETX sn_under_segmentation={} with invalid next_so={}", sn_under_segmentation, sdu_info.next_so);
+        return false;
+      }
+      nack.so_end = sdu_info.next_so - 1;
+    }
   }
   // Sanity checks
   if (nack.so_start > nack.so_end) {

tests/unittests/rlc/rlc_tx_am_test.cpp

@@ -191,8 +191,12 @@ class rlc_tx_am_test : public ::testing::Test, public ::testing::WithParamInterf
   /// \param[in] pdu_size Maximum size of each PDU that is read from RLC AM entity
   /// \param[in] n_sdus Number of SDUs to push into RLC AM entity
   /// \param[in] sdu_size Size of each SDU that is pushed into RLC AM entity
-  void
-  tx_segmented_pdus(byte_buffer_chain* out_pdus, uint32_t n_pdus, uint32_t pdu_size, uint32_t n_sdus, uint32_t sdu_size)
+  void tx_segmented_pdus(byte_buffer_chain* out_pdus,
+                         uint32_t           n_pdus,
+                         uint32_t           pdu_size,
+                         uint32_t           n_sdus,
+                         uint32_t           sdu_size,
+                         uint32_t           expect_remaining_bytes = 0)
   {
     uint32_t header_min_size = sn_size == rlc_am_sn_size::size12bits ? 2 : 3;
     uint32_t header_so_size  = 2;
@@ -278,7 +282,7 @@ class rlc_tx_am_test : public ::testing::Test, public ::testing::WithParamInterf
         sdu_so += out_pdus[i].length() - header_size;
       }
     }
-    EXPECT_EQ(rlc->get_buffer_state(), 0);
+    EXPECT_EQ(rlc->get_buffer_state(), expect_remaining_bytes);
     EXPECT_EQ(tester->bsr, expect_buffer_state); // pull_pdu does not push BSR to lower layer
     EXPECT_EQ(tester->bsr_count, n_bsr);
   }
@@ -947,6 +951,56 @@ TEST_P(rlc_tx_am_test, retx_pdu_last_segment_without_segmentation)
   EXPECT_EQ(tester->highest_delivered_pdcp_sn_list.front(), 2);
 }
 
+TEST_P(rlc_tx_am_test, retx_sn_under_segmentation)
+{
+  const uint32_t n_sdus   = 5;
+  const uint32_t sdu_size = 9;
+
+  const uint32_t n_splits = 3;
+
+  const uint32_t    n_pdus      = n_sdus * n_splits - 1;
+  const uint32_t    header_size = (sn_size == rlc_am_sn_size::size12bits ? 2 : 3);
+  const uint32_t    so_size     = 2;
+  const uint32_t    pdu_size    = header_size + so_size + (sdu_size / n_splits);
+  byte_buffer_chain pdus[n_pdus];
+
+  const uint32_t unsent_sdu_bytes = sdu_size / n_splits - so_size; // subtract 2 extra bytes from 1st segment (no SO)
+  tx_segmented_pdus(pdus, n_pdus, pdu_size, n_sdus, sdu_size, unsent_sdu_bytes + header_size + so_size);
+
+  // NACK SN=4 2:65535 => this SN is currently under segmentation; ReTx only the parts that haven't been sent yet.
+  rlc_am_status_nack nack = {};
+  nack.nack_sn            = 4;
+  nack.has_so             = true;
+  nack.so_start           = so_size + (sdu_size / n_splits); // assume 1st segment is received properly
+  nack.so_end             = nack.so_end_of_sdu;              // the rest is missing
+  {
+    rlc_am_status_pdu status_pdu(sn_size);
+    status_pdu.ack_sn = n_sdus;
+    status_pdu.push_nack(nack);
+    rlc->on_status_pdu(std::move(status_pdu));
+  }
+  pcell_worker.run_pending_tasks();
+
+  const uint32_t retx_bytes = sdu_size - nack.so_start - unsent_sdu_bytes;
+  EXPECT_EQ(rlc->get_buffer_state(), unsent_sdu_bytes + header_size + so_size + header_size + so_size + retx_bytes);
+
+  // read the ReTx segment which shall only include bytes that were already sent
+  std::vector<uint8_t> pdu_buf;
+  size_t               pdu_len;
+  pdu_buf.resize(header_size + so_size + retx_bytes);
+  pdu_len = rlc->pull_pdu(pdu_buf);
+  pdu_buf.resize(pdu_len);
+  EXPECT_EQ(pdu_len, header_size + so_size + retx_bytes);
+
+  // read the rest of the SDU under segmentation that was not yet sent
+  EXPECT_EQ(rlc->get_buffer_state(), unsent_sdu_bytes + header_size + so_size);
+  pdu_buf.resize(unsent_sdu_bytes + header_size + so_size);
+  pdu_len = rlc->pull_pdu(pdu_buf);
+  pdu_buf.resize(pdu_len);
+
+  EXPECT_EQ(rlc->get_buffer_state(), 0);
+}
+
 TEST_P(rlc_tx_am_test, retx_pdu_segment_invalid_so_start_and_so_end)
 {
   const uint32_t    sdu_size        = 3;