Define AreAllPacketsLost() #3290
Conversation
|
Also added the formatting fix from #3289 |
| return AreAllPacketsLost(largest_lost_packet, | ||
| congestion_period) | ||
| return congestion_period > | ||
| largest_lost_packet.time_sent - earliest_lost_packet.time_sent |
There was a problem hiding this comment.
I think it's possible a packet is acked between earliest and largest, so I believe you may need to pass in lost_packets and check that they're contiguous. Then you can do an early return if no packets are lost or if they're not contiguous.
ie:
all_packets_lost = acket_packets.length() ==
(acked_packets.last().packet_number - acked_packets.first().packet_number - 1)
There was a problem hiding this comment.
Done I think, though did s/acked_packets/lost_packets/ (also got a bit creative with the indentation).
There was a problem hiding this comment.
Sorry, I meant lost_packets.
There was a problem hiding this comment.
Also, is the - 1 really needed here?
There was a problem hiding this comment.
+1 is.
Think through the example where 0, 1 and 2 are lost. lost_packets has length 3. But 2 - 0 = 2, so you need to add 1.
There was a problem hiding this comment.
But also, now that I think about it, lost_packets only contains lost in-flight packets right? So checking lost_packets.length() against the packet number difference might fail because of other lost but not in-flight packets adding gaps, is this a problem?
There was a problem hiding this comment.
Good point, that is a problem. I think that was why we resorted to a helper method to begin with.
For a packet between smallest and largest to be acknowledged AND smallest not previously declared lost, I believe it must occur in the ACK frame that caused smallest and largest to be lost. Otherwise the time threshold would have kicked in long before persistent congestion.
Therefore, if the largest lost packet is smaller than the earliest newly acknowledged packet, we can guarantee the lost packets are contiguous. However, then we do not trigger persistent congestion if a packet sent at the very beginning of the congestion period is acknowledged in a long delayed ACK, and at the same time a much later packet is ACKed. I'm not sure we care about capturing that case, but it's worth considering.
If we were ok with more complex code, we could loop over all newly acked packets and ensure none were between the smallest and largest lost packets.
There was a problem hiding this comment.
Isn't it congestion_period <= largest_lost_packet.time_sent - earliest_lost_packet.time_sent because congestion_period is the threshold that defines duration of persistent congestion?
draft-ietf-quic-recovery.md
Outdated
| congestion_period) | ||
| all_packets_lost = | ||
| lost_packets.length() == (lost_packets.last().packet_number - | ||
| lost_packets.first().packet_number - 1) |
There was a problem hiding this comment.
My mistake on the example, this should be:
| lost_packets.first().packet_number - 1) | |
| lost_packets.first().packet_number + 1) |
| all_packets_lost = | ||
| lost_packets.length() == (lost_packets.last().packet_number - | ||
| lost_packets.first().packet_number + 1) |
There was a problem hiding this comment.
This assumes you don't send with gaps. I personally don't think we need complete psuedocode here. I think text should be enough. I feel like this logic could be very dependent on the implementation.
There was a problem hiding this comment.
I'm tending to agree with @nibanks, I'm not sure there's anything we can add here which will meaningfully clarify the pseudocode without making it quite implementation dependent.
There was a problem hiding this comment.
Yes, this gets too detailed very fast, and I don't think more code is the answer. @ghedo: Is there more we can add to the comment above to help implementers?
There was a problem hiding this comment.
IMO the text or the comment should at the very least include the points that were raised in this disucssion, otherwise we'll just keep revisiting this over and over, and discussions on GitHub really won't help implementers once the spec is final. Also, I think the slight restructuring of the pseudo-code, where InPersistentCongestion() takes the whole list of lost packets is also more helpful to clarify what the information needed to make the decision is.
I can look into making another PR to do that, while keeping AreAllPacketsLost() undefined, if no one else wants to try.
There was a problem hiding this comment.
@ghedo I think that's the best way forward, if you're willing to write a PR for that.
There was a problem hiding this comment.
Yeah, I can do that, just need to find some time :) Will close this PR in the meantime.
Fixes #3288.