minq icon indicating copy to clipboard operation
minq copied to clipboard

ACK delay & RTT estimation

Open pietdevaere opened this issue 8 years ago • 5 comments

Adding RTT estimation to the congestion controller.

I added both QUIC and TCP-style estimates. The TCP style estimates do not consider the ACK Delay field. They might be useful later to see what the influence is of moving the stamping of the ACK Delay and the recording of the Rx time of packets.

For this I had to populate the ACK Delay field in ACK frames. I currently use the IEEE 754 binary16 half precision format to encode the time. This is not exactly the format from draft-ietf-quic-transport-07, but as that format is under review, implementing it seems a waste of time. I borrowed the code for this from https://github.com/h2so5/half, which is CC0.

pietdevaere avatar Dec 03 '17 10:12 pietdevaere

How different is this from the existing format? I.e., is it going to produce crazy results when interpreted by other stacks?

ekr avatar Dec 04 '17 12:12 ekr

As per -07

The bit format is loosely modeled after IEEE 754

I didn't look at it in detail, but I think the main difference is that it is an unsigned float instead of a signed one.

Considering the crazy results: draft-ietf-quic-recovery specifies a sanity check to make sure that ACK delay > RTT, so if this value is wrong, the worst that can (should?) happen is that the endpoint assumes that the link RTT is 0.

In -08 the format will change completely.

pietdevaere avatar Dec 04 '17 12:12 pietdevaere

This looks basically sound, but given that we are now at -08, can you update the float format?

ekr avatar Dec 19 '17 21:12 ekr

The new format is a varint. Perhaps we should park this PR until varint support is added?

pietdevaere avatar Dec 22 '17 09:12 pietdevaere

Sure. I'll be adding varint support in the next week or so.

On Fri, Dec 22, 2017 at 1:47 AM, Piet De Vaere [email protected] wrote:

The new format is a varint. Perhaps we should park this PR until varint support is added?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/ekr/minq/pull/40#issuecomment-353562215, or mute the thread https://github.com/notifications/unsubscribe-auth/ABD1oTJr830ZNilUpV48j_zMcnqJJK_aks5tC3qYgaJpZM4Qzq3i .

ekr avatar Dec 22 '17 12:12 ekr