Partial ack TSO after retransmission + Fix TCP segments order in RTO after fast retransmission #177

iftahl · 2024-07-04T22:54:11Z

Fix ACK of partial TSO segment:
In the happy TSO path whenever we get an ack, we go over
the unacked list and shrink TSO segments accordingly.
The issue is when we get the ack after retransmission where
the segment/s move from the unacked list to unsent.
In this case, we don't shrink segments in the unsent list.
a. Shrink is done for all segments now, not only TSO
b. Avoid the corner case when partially ACKed and not shrunk segments block TCP progress.
Fix unsent queue after fast retransmission+RTO:
In fast retransmission, we move a single segment from the unacked list to the unsent list.
But if we have RTO right after, we move all the unacked list to the unsent list, but we don't
take into consideration the we might have a retransmitted segment already in the unsent list.

Example:
unacked: 1->2->3
unsent: 4->5->6

After fast retransmission reorder in LWIP:
unacked: 2->3
unsent: 1->4->5->6

After RTO:
unacked: ----
unsent: 2->3->1->4->5->6

Change type

What kind of change does this PR introduce?

Check list

Code follows the style de facto guidelines of this project
Comments have been inserted in hard to understand places
Documentation has been updated (if necessary)
Test has been added (if possible)

iftahl · 2024-07-31T11:57:04Z

bot:retest

iftahl · 2024-07-31T16:05:14Z

bot:retest

pasis · 2024-08-03T21:45:52Z

src/core/lwip/tcp_out.c

@@ -1161,276 +1161,6 @@ static void tcp_tso_segment(struct tcp_pcb *pcb, struct tcp_seg *seg, u32_t wnd)
    return;
 }

-static struct tcp_seg *tcp_split_one_segment(struct tcp_pcb *pcb, struct tcp_seg *seg,


[note] #199 contains duplicate of removing unused code. The PR, which is merged second, need to remove the duplicate commit.

pasis · 2024-08-03T21:48:16Z

src/core/lwip/tcp_in.c

@@ -947,6 +940,32 @@ static u32_t tcp_shrink_zc_segment(struct tcp_pcb *pcb, struct tcp_seg *seg, u32
    return count;
 }

+void ack_partial_or_whole_segment(struct tcp_pcb *pcb, u32_t ackno, struct tcp_seg **seg)


Looks like this function can be static.

pasis · 2024-08-03T22:06:34Z

src/core/lwip/tcp_out.c

+        unacked: 1->2->3
+        unsent: 4->5->6
+
+        After fast retransmission of seg 1,2:
+        unacked: 3
+        unsent: 1->2->4->5->6
+
+        After RTO without handling correct order:
+        unacked: ----
+        unsent: 3->1->2->4->5->6
+
+        Therefore - we first move back retransmitted segments to unacked:
+        unacked: 1->2->3
+        unsent: 4->5->6
+        And then move all unacked to unsent:
+        unacked: ----
+        unsent: 1->2->3->4->5->6


No need to keep a long comment. It's enough to mention that "fast retransmission can insert out of order segment to the unsent queue".

Also fix the commit message : after a single fast retransmission the context will be:
unacked: 2->3
unsent:: 1->4->5->6
There is no straightforward way to get 1->2->4->5->6

pasis · 2024-08-03T22:14:31Z

src/core/lwip/tcp_out.c

+    if (pcb->unsent != NULL && TCP_SEQ_GT(pcb->unacked->seqno, pcb->unsent->seqno)) {
+        // Move retransmitted segments to unacked
+        struct tcp_seg *orig_unsent = pcb->unsent;
+        struct tcp_seg *last_unsent_move_to_unacked = pcb->unsent;
+        while (last_unsent_move_to_unacked->next != NULL &&
+               TCP_SEQ_GT(pcb->unacked->seqno, last_unsent_move_to_unacked->next->seqno)) {
+            last_unsent_move_to_unacked = last_unsent_move_to_unacked->next;
+        }
+
+        pcb->unsent = last_unsent_move_to_unacked->next;
+        if (pcb->unsent == NULL) {
+            pcb->last_unsent = NULL;
+        }
+
+        last_unsent_move_to_unacked->next = pcb->unacked;
+        pcb->unacked = orig_unsent;
+    }


Is it guaranteed that the retransmitted segments in the unsent are in order? If there is a way to call fast retransmission twice, the order would be reversed. So, either each retransmitted segment needs to be inserted to the right place individually or only the single head element needs to be moved.

It's working properly.

pasis · 2024-08-03T22:15:46Z

src/core/lwip/tcp_out.c

+    if (pcb->unsent != NULL && TCP_SEQ_GT(pcb->unacked->seqno, pcb->unsent->seqno)) {
+        // Move retransmitted segments to unacked
+        struct tcp_seg *orig_unsent = pcb->unsent;
+        struct tcp_seg *last_unsent_move_to_unacked = pcb->unsent;


Long variable names can complicate code. For short code blocks it's better to keep short names .

pasis

I'd suggest to update commit message for the shrink commit:

Mention that shrink is done for all segments now, not only TSO
Mention that the patch avoids the corner case when a partially ACKed and not shrunk segments blocks TCP progress.

AlexanderGrissik · 2024-08-13T06:50:30Z

bot:retest

dpressle · 2024-08-13T11:03:57Z

bot:retest

AlexanderGrissik · 2024-08-14T10:46:44Z

bot:retest

In the happy TSO path whenever we get an ack, we go over the unacked list and shrink TSO segments accordingly. The issue is when we get the ack after retransmission where the segment/s move from the unacked list to unsent. In this case, we don't shrink segments in the unsent list. The change: 1. Shrink every segment and not only TSO segments. 2. Avoid the corner case when a partially ACKed and not shrunk segments block TCP progress. Signed-off-by: Iftah Levi <iftahl@nvidia.com>

In fast retransmission, we move a single segment from the unacked list to the unsent list. But if we have RTO right after, we move all the unacked list to the unsent list, but we don't take into consideration the we might have a retransmitted segment already in the unsent list. Example: unacked: 1->2->3 unsent: 4->5->6 After fast retransmission (segments 1 and 2) reorder in LWIP: unacked: 3 unsent: 1->2->4->5->6 After RTO: unacked: ---- unsent: 3->1->2->4->5->6 Signed-off-by: Iftah Levi <iftahl@nvidia.com>

iftahl added bug Something isn't working draft Not to review yet labels Jul 4, 2024

iftahl force-pushed the rexmit_lwip branch from dc22826 to 0a606b3 Compare July 10, 2024 12:26

iftahl removed the draft Not to review yet label Jul 10, 2024

iftahl requested a review from pasis July 10, 2024 12:27

iftahl force-pushed the rexmit_lwip branch from 0a606b3 to ec8f097 Compare July 31, 2024 08:14

iftahl changed the title ~~Rexmit lwip~~ Partial ack TSO after retransmission + Fix TCP segments order in RTO after fast retransmission Jul 31, 2024

iftahl force-pushed the rexmit_lwip branch from ec8f097 to 94f5174 Compare July 31, 2024 08:59

pasis reviewed Aug 3, 2024

View reviewed changes

iftahl force-pushed the rexmit_lwip branch 2 times, most recently from 6b41015 to 6320417 Compare August 8, 2024 13:49

pasis approved these changes Aug 12, 2024

View reviewed changes

iftahl force-pushed the rexmit_lwip branch 2 times, most recently from ddad3af to edeccd2 Compare August 13, 2024 07:46

AlexanderGrissik force-pushed the vNext branch from cfce2e8 to 51c2340 Compare August 18, 2024 08:58

iftahl added 2 commits August 20, 2024 13:35

iftahl force-pushed the rexmit_lwip branch from edeccd2 to 1259fe6 Compare August 20, 2024 10:35

AlexanderGrissik merged commit a6b571f into Mellanox:vNext Aug 21, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Partial ack TSO after retransmission + Fix TCP segments order in RTO after fast retransmission #177

Partial ack TSO after retransmission + Fix TCP segments order in RTO after fast retransmission #177

iftahl commented Jul 4, 2024 •

edited

Loading

iftahl commented Jul 31, 2024

iftahl commented Jul 31, 2024

pasis Aug 3, 2024

iftahl Aug 11, 2024

pasis Aug 3, 2024

iftahl Aug 11, 2024

pasis Aug 3, 2024

iftahl Aug 11, 2024

pasis Aug 3, 2024

iftahl Aug 11, 2024

pasis Aug 3, 2024

iftahl Aug 11, 2024

pasis left a comment

AlexanderGrissik commented Aug 13, 2024

dpressle commented Aug 13, 2024

AlexanderGrissik commented Aug 14, 2024

Partial ack TSO after retransmission + Fix TCP segments order in RTO after fast retransmission #177

Partial ack TSO after retransmission + Fix TCP segments order in RTO after fast retransmission #177

Conversation

iftahl commented Jul 4, 2024 • edited Loading

Change type

Check list

iftahl commented Jul 31, 2024

iftahl commented Jul 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pasis left a comment

Choose a reason for hiding this comment

AlexanderGrissik commented Aug 13, 2024

dpressle commented Aug 13, 2024

AlexanderGrissik commented Aug 14, 2024

iftahl commented Jul 4, 2024 •

edited

Loading