Datagram size, not packet size #4214

janaiyengar · 2020-10-14T23:40:21Z

Addresses #4196.

@gloinul points out in #4196 that we use maximum packet size in the transport draft and max_datagram_size in the recovery draft. In reading carefully, I realized that maximum packet size in the transport draft was actually historical, and maximum datagram size is the more appropriate term.

This is an entirely editorial PR, and it's largely simple changes, despite the fact that it looks a bit big. It is a bit subtle though. Sorry about that.

martinthomson

Thanks for doing this. This is fixing something that we really should have done ages ago.

draft-ietf-quic-transport.md

LPardue · 2020-10-15T00:15:45Z

draft-ietf-quic-transport.md


-QUIC provides an acknowledged PL, therefore a sender does not implement the
+QUIC provides an acknowledged PL, therefore a sender does not implement a


consistency: above you say "QUIC is an acknowledged PL"

ianswett

Some suggestions, but I'm not finding anything incorrect.

ianswett · 2020-10-15T00:49:57Z

draft-ietf-quic-transport.md

+A UDP datagram can include one or more QUIC packets. The datagram size refers to
+the total UDP payload size of a single UDP datagram carrying QUIC packets. That
+is, the datagram size includes includes the QUIC headers and protected payload,
+but not the UDP or IP headers.


Now that the name has changed, I don't think the extra explanation is necessary.

Suggested change

A UDP datagram can include one or more QUIC packets. The datagram size refers to

the total UDP payload size of a single UDP datagram carrying QUIC packets. That

is, the datagram size includes includes the QUIC headers and protected payload,

but not the UDP or IP headers.

A UDP datagram can include one or more QUIC packets. The datagram size refers to

the total UDP payload size of a UDP datagram carrying one or more QUIC packets.

I prefer the extra detail here, it's worth spelling it out precisely since we use the concept a fair bit below. I'll take the "one or more QUIC packets" though.

ianswett · 2020-10-15T00:54:13Z

draft-ietf-quic-transport.md

-fields.  The PMTU can depend on path characteristics, and can therefore change
-over time.  The largest UDP payload an endpoint sends at any given time is
-referred to as the endpoint's maximum packet size.
+includes QUIC packet headers, protected payload, and any authentication fields.


I'm unclear if this is written with a single QUIC packet in mind or multiple. Suggestion follows.

Suggested change

includes QUIC packet headers, protected payload, and any authentication fields.

includes one or more QUIC packets.

I changed "header" to "headers" to reflect that, but I can change it to "one or more QUIC packet headers, ...". I think it's worth spelling out the fields though.

Co-authored-by: Martin Thomson <mt@lowentropy.net>

martinthomson · 2020-10-15T01:49:32Z

draft-ietf-quic-transport.md

-is, the datagram size includes includes the QUIC headers and protected payload,
-but not the UDP or IP headers.
+the total UDP payload size of a single UDP datagram carrying QUIC packets. The
+datagram size includes includes one or more QUIC packet headers and the


Suggested change

datagram size includes includes one or more QUIC packet headers and the

datagram size includes includes one or more QUIC packet headers and

martinthomson · 2020-10-15T01:50:06Z

draft-ietf-quic-transport.md

+includes one or more QUIC packet headers, the protected payloads, and any
+authentication fields.  The PMTU can depend on path characteristics, and can


I think that we can drop "authentication fields" as that is just part of "protected payload".

gloinul

Except for the one erronous exchange of packet this looks good.

gloinul · 2020-10-15T12:23:32Z

draft-ietf-quic-transport.md

-networks.  Assuming the minimum IP header size, this results in a QUIC maximum
-packet size of 1232 bytes for IPv6 and 1252 bytes for IPv4.
+networks.  Assuming the minimum IP header size, this results in a maximum
+datagram size of 1232 bytes for IPv6 and 1252 bytes for IPv4.


Here it should say "IP packet" instead of datagram as it is the IP packet sizes for IPv4 and IPv6 that are given.

Re-reading, I think this might have been better as QUIC maximum packet size, since that is accurate.

It isn't the IP packet size or the QUIC packet size -- this really is about the UDP payload size (1280 - 20/40 - 8 = 1252/1232). I'm leaving this as is.

I'm now spelling this out a bit more, so the computation is crystal clear.

ianswett · 2020-10-15T14:24:29Z

draft-ietf-quic-transport.md

-networks.  Assuming the minimum IP header size, this results in a QUIC maximum
-packet size of 1232 bytes for IPv6 and 1252 bytes for IPv4.
+networks.  Assuming the minimum IP header size, this results in a maximum
+datagram size of 1232 bytes for IPv6 and 1252 bytes for IPv4.


Re-reading, I think this might have been better as QUIC maximum packet size, since that is accurate.

Datagram size, not packet size

d42f455

janaiyengar requested a review from martinthomson as a code owner October 14, 2020 23:40

janaiyengar added -transport editorial An issue that does not affect the design of the protocol; does not require consensus. labels Oct 14, 2020

janaiyengar requested a review from gloinul October 15, 2020 00:02

martinthomson approved these changes Oct 15, 2020

View reviewed changes

draft-ietf-quic-transport.md Outdated Show resolved Hide resolved

draft-ietf-quic-transport.md Show resolved Hide resolved

draft-ietf-quic-transport.md Outdated Show resolved Hide resolved

LPardue reviewed Oct 15, 2020

View reviewed changes

ianswett reviewed Oct 15, 2020

View reviewed changes

janaiyengar and others added 2 commits October 14, 2020 18:06

Apply suggestions from code review

d8816f9

Co-authored-by: Martin Thomson <mt@lowentropy.net>

review comments

ad6d533

martinthomson approved these changes Oct 15, 2020

View reviewed changes

larseggert mentioned this pull request Oct 15, 2020

Recovery-31: Minor editorial issues found in AD review #4196

Closed

larseggert linked an issue Oct 15, 2020 that may be closed by this pull request

Recovery-31: Minor editorial issues found in AD review #4196

Closed

gloinul reviewed Oct 15, 2020

View reviewed changes

ianswett approved these changes Oct 15, 2020

View reviewed changes

janaiyengar added 2 commits October 15, 2020 12:55

review

02eb08c

slight moving of text

5216eb1

ianswett approved these changes Oct 15, 2020

View reviewed changes

janaiyengar merged commit 59ac29d into master Oct 15, 2020

janaiyengar deleted the jri/max-packet-size branch October 15, 2020 20:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Datagram size, not packet size #4214

Datagram size, not packet size #4214

janaiyengar commented Oct 14, 2020 •

edited

martinthomson left a comment

LPardue Oct 15, 2020

ianswett left a comment

ianswett Oct 15, 2020

janaiyengar Oct 15, 2020

ianswett Oct 15, 2020

janaiyengar Oct 15, 2020

martinthomson Oct 15, 2020

martinthomson Oct 15, 2020

gloinul left a comment

gloinul Oct 15, 2020

ianswett Oct 15, 2020

janaiyengar Oct 15, 2020

janaiyengar Oct 15, 2020

ianswett Oct 15, 2020


		QUIC provides an acknowledged PL, therefore a sender does not implement the
		QUIC provides an acknowledged PL, therefore a sender does not implement a

	includes QUIC packet headers, protected payload, and any authentication fields.
	includes one or more QUIC packets.

	datagram size includes includes one or more QUIC packet headers and the
	datagram size includes includes one or more QUIC packet headers and

		includes one or more QUIC packet headers, the protected payloads, and any
		authentication fields. The PMTU can depend on path characteristics, and can

Datagram size, not packet size #4214

Datagram size, not packet size #4214

Conversation

janaiyengar commented Oct 14, 2020 • edited

martinthomson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianswett 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

gloinul 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

janaiyengar commented Oct 14, 2020 •

edited