-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Partial ack TSO after retransmission + Fix TCP segments order in RTO after fast retransmission #177
Conversation
bot:retest |
1 similar comment
bot:retest |
src/core/lwip/tcp_out.c
Outdated
@@ -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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[note] #199 contains duplicate of removing unused code. The PR, which is merged second, need to remove the duplicate commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/core/lwip/tcp_in.c
Outdated
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this function can be static.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/core/lwip/tcp_out.c
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/core/lwip/tcp_out.c
Outdated
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; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's working properly.
src/core/lwip/tcp_out.c
Outdated
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Long variable names can complicate code. For short code blocks it's better to keep short names .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
6b41015
to
6320417
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
bot:retest |
ddad3af
to
edeccd2
Compare
bot:retest |
1 similar comment
bot:retest |
cfce2e8
to
51c2340
Compare
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 <[email protected]>
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 <[email protected]>
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