Proposed text for reordering threshold. #201
Conversation
|
@huitema there is duplicated text in your PR. I tried to resolve it but I don't think the text is fully correct yet. So I think you have to take another look yourself, please! |
|
@mirjak I fixed a Kramdown XML issue (missing curly bracket) in an unrelated part, then verified the text output. I do not see duplicated text. Is this some kind of merge conflict? |
draft-ietf-quic-ack-frequency.md
Outdated
| rely on the peer's `Ack-Eliciting Threshold` and `max_ack_delay` thresholds | ||
| for sending acknowledgements. | ||
| * when the packet number is lower than the Largest Acked. | ||
| (TODO: is that true? Should there be some kind of threshold?) |
There was a problem hiding this comment.
I think this is already true in RFC9000 and I don't think we were intending to change that.
| a Reordering Threshold of 2. | ||
| * when the packet number is larger than the Largest Unacked | ||
| packet and the difference between the this packet number and the Largest | ||
| Unacked is larger than the Reordering Threshold. |
There was a problem hiding this comment.
| Unacked is larger than the Reordering Threshold. | |
| Unacked is greater than or equal to the Reordering Threshold. |
draft-ietf-quic-ack-frequency.md
Outdated
| (TODO: is that true? Should there be some kind of threshold?) | ||
|
|
||
| * when the total number of Unreported Missing packets is larger | ||
| than the Reordering Threshold. |
There was a problem hiding this comment.
I don't think this is needed as it should be covered by case 1
There was a problem hiding this comment.
The case I am addressing there is:
- threshold is 2.
- the endpoint receives packet 1, no ACK yet because delays. Highest unacked is 1.
- the endpoint receives packet 3, no ACK yet because the gap to 1 is only one packet. Highest unacked is 3.
- the endpoint receives packet 5. There is just a one-packet gap to 3, so according to the first rule, no ACK yet. But there are no two holes, 2 and 4, so the endpoint should send an ACK.
There was a problem hiding this comment.
But then, I don't know whether that's what we actually want to say. Starting from the explanation, we want to send an ACK as soon as we detect a condition that cause the missing packet to be repeated. That condition is "The packet was sent kPacketThreshold packets before an acknowledged packet". That means the endpoint should track the lowest number missing packet number after the highest acked packet, and compare it to the highest unacked. If the difference is larger or equal to the threshold, it should send an acknowledgement. But the text does not say that.
There was a problem hiding this comment.
Yes, this is actually what the texts tries to say. It's not about the number of missing packets but the only the gap between the last in-order packet and the last to-be-acked packet. The number of missing packet is not important, just the distance.
So in your example above, packet 5 should trigger an ack but also if packet 4 would have arrived in order it should trigger an ack.
However, assuming your case where packet 4 is missing, you send an ACK when packet 5 arrives because of missing packet 2 (which then is a reported missing packet and does not count for future ack decision anymore). However, as soon as packet 6 arrives, you will send another ack to report of missing packet number 4.
This PR takes on the editorial changes from PR #201. The actual definition part of the recording threshold might need further discussion and clarification.
|
|
||
| * when the packet number is lower than the Largest Acked. | ||
|
|
||
| * when the total number of Unreported Missing packets is larger |
There was a problem hiding this comment.
I think this is redundant to the first bullet if I am reasoning about it correctly? If there were this many unreported missing packets, wouldn't the first condition be met?
There was a problem hiding this comment.
Check comment above, the case I am addressing there is:
- threshold is 2.
- the endpoint receives packet 1, no ACK yet because delays. Highest unacked is 1.
- the endpoint receives packet 3, no ACK yet because the gap to 1 is only one packet. Highest unacked is 3.
- the endpoint receives packet 5. There is just a one-packet gap to 3, so according to the first rule, no ACK yet. But there are no two holes, 2 and 4, so the endpoint should send an ACK.
But I agree that's still not right. Consider:
- threshold is 2.
- the endpoint receives packet 1, no ACK yet because delays. Highest unacked is 1.
- the endpoint receives packet 3, no ACK yet because the gap to 1 is only one packet. Highest unacked is 3.
- the endpoint receives packet 4, 5, 6, 7, 8, 9, 10.
They are all in sequence, so the "out of order" rule does not kick in. Also, there is only one hole, so the "number of holes" rule does not kick in either. Yet, if the endpoint acknowledged packet 6, the peer would immediately resend packet 2, because of the number gap rule in RFC 9002. If our goal is to somehow synchronize the sending of ACK with loss detection, then the text ought to focus on the distance between "lowest missing packet number" and "highest unacked".
But then we also need a definition of "lowest missing packet number", and that definition has to consider the "highest number acked". The easiest implementation is to define that as "lowest missing packet number among those larger than largest acknowledged number." That will cover the example above, but there will be corner cases:
- threshold is 2.
- the endpoint receives packet 1, no ACK yet because delays. Highest unacked is 1.
- the endpoint receives packet 3, the endpoint send ACK 1, 3, because of ack delay.
- the endpoint receives packet 4, 5, 6, 7, 8, 9, 10.
Again, if the endpoint sent ACK 1,3-10, then the peer would immediately repeat packet 2 because of the number rule. So maybe the definition should be "lowest missing packet number among those larger than largest acknowledged number minus threshold + 1."
There was a problem hiding this comment.
The first part is actually fixed in PR #208, which includes the correct test on the "lowest missing number". I will open a new issue for the corner case using the discussion above, and then close this PR.
fixes #200
My attempt at resolving issue #200