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

Latency Spin Bit #609

Closed
wants to merge 8 commits into from
Closed

Latency Spin Bit #609

wants to merge 8 commits into from

Conversation

huitema
Copy link
Contributor

@huitema huitema commented Jun 8, 2017

Adding text to the transport spec to describe the "latency spin bit".

This is the result of the discussion of latency monitoring during the Paris interim meeting.
@britram
Copy link
Contributor

britram commented Jun 8, 2017

I like this approach, but would like to dig into how it breaks in corner cases.

It seems to me to be quite related to "coloring" based approaches, see e.g. https://datatracker.ietf.org/doc/draft-ietf-ippm-alt-mark/

@hardie
Copy link

hardie commented Jun 8, 2017

I also like this approach, and I think the one key corner case (out-of-order packet arrival) can be handled by description of how to smooth the base RTT signal. That would allow out of color packets to be seen as out-of-order indications rather than screwing up the RTT calculation.

Copy link
Member

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

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

Is this a per-path signal? I think that it is and that we need more text about that.

@@ -198,6 +198,8 @@ strengths of QUIC include:

* Version negotiation

* Support for monitoring
Copy link
Member

Choose a reason for hiding this comment

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

I would rather that we not include this section.

Copy link
Contributor

Choose a reason for hiding this comment

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

in any case "support for monitoring" is much broader than what this is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you completely remove the question, then the bit description just appears out of nowhere. But then, yes, we could be specific about latency monitoring, rather than monitoring in general.

@mikkelfj
Copy link
Contributor

mikkelfj commented Jun 8, 2017

I would suggest to make it optional. There are some privacy issues associated with this allowing to assocate packets with a connection. Though UDP also associates packets, alternate transport layers can obfuscate routing.

@mirjak
Copy link
Contributor

mirjak commented Jun 8, 2017

Instead of using a 'whole' bit, you can also have another packet type for protected packets. And if you define a well know patter that is used, you can even measure loss with it.

@mirjak
Copy link
Contributor

mirjak commented Jun 8, 2017

Or just have the flag on short packets...

Adding a reference to draft-ietf-ippm-alt-mark, as Brian suggested in his review.
@huitema
Copy link
Contributor Author

huitema commented Jun 8, 2017

I get Martin's point about per path signal, which it indeed is. But then, we don't really have an end-to-end vs path distinction yet. In the current transport draft, there seems to be just one path at a time. I think the distinction will only become apparent when we start specifying QUIC multipath.

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.

There's no discussion of what to do when marked packets are lost. I think there needs to be to ensure the signal doesn't die during the connection.

@@ -294,6 +296,15 @@ QUIC version negotiation allows for multiple versions of the protocol to be
deployed and used concurrently. Version negotiation is described in
{{version-negotiation}}.

## Support for Latency Monitoring

Network administrators often find difficult to monitor the behavior
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: how about "Network administrators have difficulty monitoring ..."?


Network administrators often find difficult to monitor the behavior
of encrypted protocols. Bandwidth can be monitored by observing the
flow of encrypted packets, but the usal tools for monitoring
Copy link
Contributor

Choose a reason for hiding this comment

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

usal/usual

flow of encrypted packets, but the usal tools for monitoring
latency require observing packet numbers and acknowledgements, which
in QUIC are encrypted.
QUIC packets include a clear text latency spin bit that enables
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "QUIC packets enable monitoring of latency by including a clear text latency spin bit that is reflected between peers."

the network path. This bit is set as follow:

* The connection responder sets the spin bit value to the value of the
spin bit in the last packet received from the connection initiator.
Copy link
Contributor

Choose a reason for hiding this comment

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

What if two packets were received and the first had the spin bit set and the second did not? I would think the spin bit would still be set by the responder in that case?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, can you add clarifying text about what to do when exiting quiescence?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ian, there are two references to quiescence in the current draft. One about the Ping frame, "The default is to send a PING frame after 15 seconds of quiescence." The other about flow control, "it is generally considered best to not let the sender go into quiescence if avoidable." But the concept of quiescence is not well defined. Is it just "has not sent packet for a long time"?
What would you suggest to do?

Copy link
Contributor

Choose a reason for hiding this comment

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

What if two packets were received and the first had the spin bit set and the second did not? I would think the spin bit would still be set by the responder in that case?

If an old packet race ahead this could add noise to signal. I think it would be good to explain how this would work, or alternatively filter out the noise by only flipping when a new highest packet number is received. Noise could probably be used effectively if understood by analyser.

Copy link
Contributor

Choose a reason for hiding this comment

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

Scratch my comment, I think if we define this correctly, we don't need to talk about quiescence, it'll just naturally work out.

Copy link
Contributor

Choose a reason for hiding this comment

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

One more comment: I think we need text on packet reordering here. Possibly this should be the largest packet received instead of the last?

@huitema
Copy link
Contributor Author

huitema commented Jun 8, 2017

Mirja, I hesitated between the "packet type" design and the "spin bit". I like having Red/Blue packet types, but I am concerned with the interaction with issue 311, "GREASE the packet type octet". If we do grease the packet type, we will need to leave the spin bit in clear text. This is much easier to do with a reserved bit than with the packet type.

@mirjak
Copy link
Contributor

mirjak commented Jun 8, 2017

Yes, there is dependency on greasing. We could also just grease the long header because the short header has a bunch of flags that can anyway change all the time. I even thought about having only flags with the short header (and no type anymore) because that's nearly what we have already and that makes sense to me (and also looks slightly clearer to me) because all control packets are long headers. For me the point of the short header is basically a packet where you want to save some of the header bits and use it for payload instead and the flags simply define which part of the header bits you omit.

@mikkelfj
Copy link
Contributor

mikkelfj commented Jun 8, 2017

There is also the option to grease the type but reserve a single ungreased value for monitoring packets.

The benefit of a separate packet is both more payload for monitoring, and disassociation with other packets to better protect against timing analysis.

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.

I like the 'spin bit' approach personally.

the network path. This bit is set as follow:

* The connection responder sets the spin bit value to the value of the
spin bit in the last packet received from the connection initiator.
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, can you add clarifying text about what to do when exiting quiescence?

@martinthomson
Copy link
Member

There is also the option to grease the type but reserve a single ungreased value for monitoring packets.

This is unaffected by my favourite form of greasing (xor with the last octet of the unencrypted frame). We have a choice as to whether or not this is greased, of course.

@martinthomson
Copy link
Member

Thinking about this some more, this bit will flip lots in ways that aren't immediately obvious. I think that we can safely not grease this. (We could also grease it, but that makes it a teensy bit less accessible to the devices who we intend to consume it, so not greasing is probably best.) Either way, that's not a question we need to resolve with the change as it stands.

I still strongly think that we should avoid adding this to the list of transport features, and I think that you should address @ianswett's comments about dropping an entire flight (the answer being just "oh well" here).

@martinthomson
Copy link
Member

Oh, and I really think that this needs to be crisp about being per-path. I think that we need to explicitly sasy that we reset the signal when the IP or port at either end changes.

@martinthomson martinthomson added -transport design An issue that affects the design of the protocol; resolution requires consensus. labels Jun 12, 2017
Fixes suggested in Ian's review.
* The connection responder sets the spin bit value to the value of the
spin bit in the last packet received from the connection initiator.

* The connection initiator sets the spin bit value to the opposite
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused about why it's the opposite? My mental model is that the bit would be set to 1 on a packet, and then it would be bounced back and forth, so there would only be a single packet marked with the 1 spin bit at any given point in time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, ekr explained to me out of band that I misunderstood how the spin bit worked. I think the design here is more robust to loss than what I was thinking of, so I'm happy with it.

However, an overview sentence explaining what it's doing (ie: mark the entire flight with the spin bit, etc) would have prevented my confusion.

@janaiyengar
Copy link
Contributor

Can we please move discussions to issues, and not have them on PRs?

@britram
Copy link
Contributor

britram commented Jun 14, 2017

@janaiyengar I filed #631 for that purpose.

@britram
Copy link
Contributor

britram commented Jun 14, 2017

And the loss-detection inference ability of this proposal can be discussed in #632.

@janaiyengar
Copy link
Contributor

I've moved discussion to the issue (thanks for filing it, @britram).

@ianswett
Copy link
Contributor

@janaiyengar To clarify, I think my comments were editorial, and so those still belong here, not in the issue?

@janaiyengar
Copy link
Contributor

janaiyengar commented Jun 14, 2017 via email

@ianswett
Copy link
Contributor

Great, thanks for clarifying.

the network path. This bit is set as follow:

* The connection responder sets the spin bit value to the value of the
spin bit in the last packet received from the connection initiator.
Copy link
Contributor

Choose a reason for hiding this comment

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

The terminology elsewhere appears to be Client and Server, not Connecton Initiator and Connection Responder, although I prefer the latter since the protocol is not necessarily used in a client-server setup.

Copy link
Member

Choose a reason for hiding this comment

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

"connection initiator" and "connection responder" are just pretentious ways of saying client and server. We happily use client-server protocols in non-client-server arrangements all the time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Martin, initiator and responder are certainly equivalent to client and server when the application is HTTP. Not necessarily so is other applications, e.g. DNS or Web RTC. But as you seem to feel strongly about it, lets use Client and Server in the PR to make it consistent with the rest of the document.

Replacing initiator/responder by client and server for consistency with the rest of the document.
Adding some minimal explanation of the expected behavior.
The phrase is a bit clumsy, but this solves the issues of flipping the bit when packets are received out of order.
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.

Thanks Christian, I think this LGTM.


* The client sets the spin bit value to the opposite
of the last value received from the server, or to 0
of the value set in the packet received from the server with the
largest sequence number, or to 0
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think these two lines may fit on one line?

@janaiyengar janaiyengar added the parked An issue that we can't immediately address; for future discussion. label Aug 15, 2017
@mnot mnot removed the design An issue that affects the design of the protocol; resolution requires consensus. label Sep 25, 2017
@martinthomson
Copy link
Member

Aside from being bitrotten, #1046 covers this now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-transport parked An issue that we can't immediately address; for future discussion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants