Restructure packetization section #985

martinthomson · 2017-12-05T03:13:08Z

This includes the changes from #849, which I have simply moved around.

I think that moving out the discussion on packet size to a subsection helps focus the attention on the most important part of this section, which is the discussion of how retransmission of frames works.

Closes #849.

@martinthomson

Addressed @martinthomson's editorial comments.

@gloinul

Added a little language based on @gloinul's comments in #614.

martinduke

I like the editorial reorganization. That section was too long.

I have few nits, mostly about shifting terminology of packet types.

Note also that @gloinul has some outstanding issues with this text. We can commit, but we can't yet close #614.

martinduke · 2017-12-07T16:06:41Z

draft-ietf-quic-transport.md

+
+Clients MUST ensure that the first packet in a connection, and any
+retransmissions of those octets, has a QUIC packet size of least 1200 octets.
+The packet size for a QUIC packet includes the QUIC header and integrity check,


s/"packet size for QUIC packet"/"QUIC packet size"

Keep the terminology consistent, and shorter

I don't think we need this; "QUIC packet size" is defined in Section 2.

I never realized that "QUIC packet size" was defined there. I think that this is the better place to add that definition.

martinduke · 2017-12-07T16:07:35Z

draft-ietf-quic-transport.md

+The packet size for a QUIC packet includes the QUIC header and integrity check,
+but not the UDP or IP header.
+
+The initial client packet SHOULD be padded to exactly 1200 octets unless the


s/initial client/QUIC initlal

Perhaps "QUIC Initial and Handshake packets" ?

It's only Initial. I'll reword this.

martinduke · 2017-12-07T16:08:10Z

draft-ietf-quic-transport.md

+an MTU of this size and helps reduce the amplitude of amplification attacks
+caused by server responses toward an unverified client address.
+
+Servers MUST ignore an initial plaintext packet from a client if its total size


s/initial plaintext packet/QUIC initial packet

Moar rewording coming up.

martinduke · 2017-12-07T16:12:02Z

draft-ietf-quic-transport.md

+exist in other transports. PADDING frames generate acknowledgements, but their
+content need not be delivered reliably. PADDING frames may delay the delivery of
+application data, as they consume the congestion window. However, by definition
+their likely loss in a probe packet does not require delay- inducing


Eliminate space after dash.

janaiyengar

Looks good, a few minor things.

janaiyengar · 2017-12-07T21:32:04Z

draft-ietf-quic-transport.md

+## Packet Size
+
+Clients MUST ensure that the first packet in a connection, and any
+retransmissions of those octets, has a QUIC packet size of least 1200 octets.


grammar nit: s/retransmissions/retransmission/ , since it's followed by "has a"

janaiyengar · 2017-12-07T21:34:13Z

draft-ietf-quic-transport.md

+
+Clients MUST ensure that the first packet in a connection, and any
+retransmissions of those octets, has a QUIC packet size of least 1200 octets.
+The packet size for a QUIC packet includes the QUIC header and integrity check,


I don't think we need this; "QUIC packet size" is defined in Section 2.

janaiyengar · 2017-12-07T21:36:18Z

draft-ietf-quic-transport.md

+The packet size for a QUIC packet includes the QUIC header and integrity check,
+but not the UDP or IP header.
+
+The initial client packet SHOULD be padded to exactly 1200 octets unless the


Perhaps "QUIC Initial and Handshake packets" ?

janaiyengar · 2017-12-07T21:38:17Z

draft-ietf-quic-transport.md

+
+All QUIC packets SHOULD be sized to fit within the estimated PMTU to avoid IP
+fragmentation or packet drops. To optimize bandwidth efficiency, endpoints
+SHOULD use Packetization Layer PMTU Discovery ({{!PLPMTUD=RFC4821}}) and MAY use


readability nit: long sentence, which is fine, but I would split it at the "and": "... SHOULD use Packetization Layer PMTU Discovery ({{!PLPMTUD=RFC4821}}). Endpoints MAY use ..."

janaiyengar · 2017-12-07T21:40:19Z

draft-ietf-quic-transport.md

+
+QUIC endpoints that implement any kind of PMTU discovery SHOULD maintain an
+estimate for each combination of local and remote IP addresses (as each pairing
+could have a different maximum MTU in the path).


suggestion: remove the round brackets, and make the parenthetical phrase part of the main sentence.

or a new sentence

janaiyengar · 2017-12-07T21:41:05Z

draft-ietf-quic-transport.md

+could have a different maximum MTU in the path).
+
+QUIC depends on the network path supporting a MTU of at least 1280 octets. This
+is the IPv6 minimum and therefore also supported by most modern IPv4 networks.


nit: s/the IPv6 minimum/the IPv6 minimum packet size/

MTU, not packet size (a 1280 MTU implies a smaller packet size)

janaiyengar · 2017-12-07T21:43:52Z

draft-ietf-quic-transport.md

+
+QUIC depends on the network path supporting a MTU of at least 1280 octets. This
+is the IPv6 minimum and therefore also supported by most modern IPv4 networks.
+An endpoint MUST NOT reduce their MTU below this number, even if it receives


nit: s/their MTU/its MTU/

janaiyengar · 2017-12-07T21:51:50Z

draft-ietf-quic-transport.md

+An endpoint MUST NOT reduce their MTU below this number, even if it receives
+signals that indicate a smaller limit might exist.
+
+If a QUIC endpoint determines that the PMTU between any pair of local and remote


There's obviously a good reason for this, which I can't seem to remember. Why do we disallow smaller packets? Can you add a note here about that as well, so it's obvious to any reader?

My reading of this is that the maximum must not be reduced, but if you have less to send, go ahead. This will trigger packet loss and eventual connection failure when sending bulk data on a network that does not honor the 1200 minimum and therefore possibly trigger a connection migration if possible, or otherwise stop a hopeless quest.

Another reason may be to prevent DoS attacks by injection of unverifiable network congestion control / status data.

I'm going to request that you open an issue if you care about this. This is old text and I'm not really equipped to deal with it right now. I think that @mikkelfj is approximately right. It might also be to avoid using paths that will drop random packets.

Frankly, I don't think that we need the requirement. If you know that this has happened, you can deal. The only real cost at that point is that you might be unable to create a new connection on that path.

martinthomson

Ugh, this is what you get when you move text. I swear I didn't change anything here.

Rewording coming up.

martinthomson · 2017-12-08T01:15:23Z

draft-ietf-quic-transport.md

+
+Clients MUST ensure that the first packet in a connection, and any
+retransmissions of those octets, has a QUIC packet size of least 1200 octets.
+The packet size for a QUIC packet includes the QUIC header and integrity check,


I never realized that "QUIC packet size" was defined there. I think that this is the better place to add that definition.

martinthomson · 2017-12-08T01:18:31Z

draft-ietf-quic-transport.md

+The packet size for a QUIC packet includes the QUIC header and integrity check,
+but not the UDP or IP header.
+
+The initial client packet SHOULD be padded to exactly 1200 octets unless the


It's only Initial. I'll reword this.

martinthomson · 2017-12-08T01:20:47Z

draft-ietf-quic-transport.md

+an MTU of this size and helps reduce the amplitude of amplification attacks
+caused by server responses toward an unverified client address.
+
+Servers MUST ignore an initial plaintext packet from a client if its total size


Moar rewording coming up.

martinthomson · 2017-12-08T01:22:06Z

draft-ietf-quic-transport.md

+
+QUIC endpoints that implement any kind of PMTU discovery SHOULD maintain an
+estimate for each combination of local and remote IP addresses (as each pairing
+could have a different maximum MTU in the path).


or a new sentence

martinthomson · 2017-12-08T01:22:38Z

draft-ietf-quic-transport.md

+could have a different maximum MTU in the path).
+
+QUIC depends on the network path supporting a MTU of at least 1280 octets. This
+is the IPv6 minimum and therefore also supported by most modern IPv4 networks.


MTU, not packet size (a 1280 MTU implies a smaller packet size)

martinthomson · 2017-12-08T01:26:21Z

draft-ietf-quic-transport.md

+An endpoint MUST NOT reduce their MTU below this number, even if it receives
+signals that indicate a smaller limit might exist.
+
+If a QUIC endpoint determines that the PMTU between any pair of local and remote


I'm going to request that you open an issue if you care about this. This is old text and I'm not really equipped to deal with it right now. I think that @mikkelfj is approximately right. It might also be to avoid using paths that will drop random packets.

Frankly, I don't think that we need the requirement. If you know that this has happened, you can deal. The only real cost at that point is that you might be unable to create a new connection on that path.

janaiyengar

LGTM. Merge when you're ready.

martinduke and others added 5 commits December 5, 2017 14:12

Add some suggestions for RFC 4821 probe packets.

f822437

Update draft-ietf-quic-transport.md

776debf

Addressed @martinthomson's editorial comments.

Update draft-ietf-quic-transport.md

1cf9d55

Added a little language based on @gloinul's comments in #614.

Move PLPMTUD section a little, fix Initial

6443843

Restructure packetization section

7ae32c7

martinthomson requested review from janaiyengar and martinduke December 5, 2017 03:13

Take changes from 776debf

a73dea0

martinthomson mentioned this pull request Dec 5, 2017

Add some suggestions for RFC 4821 probe packets. #849

Closed

martinduke approved these changes Dec 7, 2017

View reviewed changes

janaiyengar reviewed Dec 7, 2017

View reviewed changes

Some editorial changes

2de0c05

martinthomson commented Dec 8, 2017

View reviewed changes

janaiyengar approved these changes Dec 13, 2017

View reviewed changes

martinthomson merged commit 150ea51 into master Dec 13, 2017

martinthomson deleted the mduke-plpmtu branch December 13, 2017 21:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restructure packetization section #985

Restructure packetization section #985

martinthomson commented Dec 5, 2017

martinduke left a comment

martinduke Dec 7, 2017

janaiyengar Dec 7, 2017

martinthomson Dec 8, 2017

martinduke Dec 7, 2017

janaiyengar Dec 7, 2017

martinthomson Dec 8, 2017

martinduke Dec 7, 2017

martinthomson Dec 8, 2017

martinduke Dec 7, 2017

janaiyengar left a comment

janaiyengar Dec 7, 2017

janaiyengar Dec 7, 2017

janaiyengar Dec 7, 2017

janaiyengar Dec 7, 2017

janaiyengar Dec 7, 2017

martinthomson Dec 8, 2017

janaiyengar Dec 7, 2017

martinthomson Dec 8, 2017

janaiyengar Dec 7, 2017

janaiyengar Dec 7, 2017

mikkelfj Dec 7, 2017

mikkelfj Dec 7, 2017

martinthomson Dec 8, 2017

martinthomson left a comment

martinthomson Dec 8, 2017

martinthomson Dec 8, 2017

martinthomson Dec 8, 2017

martinthomson Dec 8, 2017

martinthomson Dec 8, 2017

martinthomson Dec 8, 2017

janaiyengar left a comment

Restructure packetization section #985

Restructure packetization section #985

Conversation

martinthomson commented Dec 5, 2017

martinduke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janaiyengar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinthomson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janaiyengar left a comment

Choose a reason for hiding this comment