Skip to content
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

Redefine outstanding #299

Merged
merged 2 commits into from Feb 17, 2017
Merged

Redefine outstanding #299

merged 2 commits into from Feb 17, 2017

Conversation

martinthomson
Copy link
Member

In #288, I tried to keep the calculations intact. This builds on that and changes the calculation to what we agreed on in the discussion on #286.

Closes #286.

Copy link
Contributor

@ianswett ianswett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two suggestions, but looks good.

The packet number is a 64-bit unsigned number and is used as part of a
cryptographic nonce for packet encryption. Each endpoint maintains a separate
packet number for sending and receiving. The packet number for sending MUST
increase by one after sending any packet.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least one.

protocol version.
version, the client selects that version and reattempts to created a connection
using that version. The client MUST update packet numbers when attempting to
connect with the updated version. Packets MUST continue to have the VERSION
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "The client MUST update packet numbers when attempting to continue to connect with the updated version." is unclear. How about "The client must select a new initial random packet number.", since you mention including the version in the next sentence?

successfully authenticated packet.

A packet number is decoded by finding the packet number value that is closest to
the next expected packet. The next expected packet is the highest received
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A packet number is decoded by finding the packet number value that is closest to the next expected packet.

The definition of "the next expected packet" is unclear. If following example suggest that "the next expected" is "highest successfully authenticated packet + 1", this will not work in cases of occasional random packet delay.

Imagine we started with no loss/delay and few packets outstanding. The sender would then use an 8-bit packet number to send packet 0x0a. Now that packet is very delayed, so the sender is increasing its packet number length to 16 bits and sends packet 0x010a. If that delayed 0x0a packet finally arrives, the rule would say to treat 0x0a as 0x010a (since 0x0109 has been successfully decoded).

It seems that the receiver should extend packet numbers with higher order bits from the smallest unacknowledged packet instead of looking for closest numbers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe, "highest successfully authenticated packet + 1" is the most robust definition, as well as how QUIC was originally intended to be specified.

Smallest unacknowledged packet is undefined. A receiver has to give up on packets at some point, and though currently there is stop waiting(which is going away), even now the receiver must be able to unilaterally give up on waiting for packets, due to memory pressure/etc.

The sender should budget enough packet number space to send any available CWND in addition to the space needed for current outstanding packets. If it fails to do so, then the cost is slightly increased packet loss under relatively large scale(>256 packets at a minimum) reordering.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Smallest unacknowledged packet is undefined. A receiver has to give up on packets at some point, [...]

By "smallest unacknowledged packet" I meant the "Smallest unacknowledged packet that has not been marked as lost (see {{QUIC-RECOVERY}})". I was trying to be brief but caused some confusion.

The sender should budget enough packet number space to send any available CWND in addition to the space needed for current outstanding packets.

That would be an alternative way of solving this -- to always assume that you may need to distinctly address a large number of packets, even if right now you have few packets on the wire. I am afraid that this defeats the purpose of variable-length packet numbers. If you must always be ready for the worst case of packet loss/delay, you must always use the max possible packet number length for your connection.

Instead I am proposing that to each incoming packet number, the receiver alway prepends missing high-order bits from the "smallest unacknowledged packet that has not been marked as lost (i.e. still waited-for) (see {{QUIC-RECOVERY}})". This is very easy to implement and solves the problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loss recovery is implemented on the sender side, not the receiver side, so the receiver would not know what the smallest unacknowledged packet that has not been marked as lost is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Receiver would still need logic to stop reporting missing packets in ACKs at some point. STOPWAITING was the mechanism for Sender to notify the Receiver of this. W/o STOPWAITING there would be some alternative mechanism (based on the number of packets, rtt estimate vs elapsed time since receipt of a packet with a higher number, etc.). It could feed from the same algorithm.

Anyway, I am open to any other reasonable specification that works for delayed packets.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The smallest packet a receiver is waiting for is relatively well defined, but I don't think it's what we want. The goal of latching to the largest packet is to favor forward progress over handling unlimited reordering. Otherwise a receiver may never give up on waiting for a packet, and the sender won't know that, causing issues. Whereas the sender knows the largest packet the receiver has acked.

The sender has all the information to decide when it sends a packet how large the packet number space needs to be. When coming out of quiescence, it needs to make the packet number slightly larger than otherwise necessary to accommodate large scale reordering, but that's a transitory issue and easy to implement.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. So you are worried that in the absence of STOPWAITING, the Sender may decide that packet 10 has been lost, and not consider it outstanding, and use short packet numbers; while the Receiver is still waiting for packet 10.

Maybe I missed it in the updated spec, but I thought that the Receiver will keep mentioning packets it is still waiting for in Acks. If that's not true, it makes sense to keep the current behavior.

@ianswett
Copy link
Contributor

Comment for Igor.

Copy link
Contributor

@janaiyengar janaiyengar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this PR needs to be rebased.

@martinthomson
Copy link
Member Author

Rebased. De-conflicting multiple in-flight pull requests is hard.

@janaiyengar janaiyengar merged commit 6123667 into master Feb 17, 2017
@martinthomson martinthomson deleted the redefine_outstanding branch February 20, 2017 00:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants