Subject: [quicwg/base-drafts] Change design and and FIN to flow control (#730)

From: Subodh Iyengar <[email-masked]>
Date: Mon, 14 Aug 2017 19:17:15 +0000 (UTC)

This PR has 1 editorial change and 2 design changes to flow control:

* [editorial] Clarifies the language around stream and connection flow control and makes it match the flow control section. 

* [design] Defines flow control in terms of offsets instead of max data. This means something different, because previously, maxData = 10 indicated that you could only send offset till 9, however with the change, sending maxOffset = 10 means that you can send offset until 10, i.e. 11 bytes. We could alternatively change the language of maxOffset to be MUST be LESS than instead of MUST NOT EXCEED to match maximumData.

* [design]  Makes FIN bit to be flow controllable. This would reduce one edge case where a packet scheduler does not have to do a special check for whether it only needs to send a FIN when it checks for flow control. This has the advantage of being able to define flow control on a stream in terms of offsets rather than max data.
You can view, comment on, or merge this pull request online at:

  https://github.com/quicwg/base-drafts/pull/730

-- Commit Summary --

  * Clarify and and FIN to flow control

-- File Changes --

    M draft-ietf-quic-transport.md (31)

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/730.patch
https://github.com/quicwg/base-drafts/pull/730.diff

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/730

---

From: Subodh Iyengar <[email-masked]>
Date: Mon, 14 Aug 2017 19:52:23 +0000 (UTC)

@siyengar pushed 1 commit.

c22e84b  Update draft-ietf-quic-transport.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/730/files/f1b22608f9adcc6fbdc70fad9e7166fe9108a379..c22e84b1c2b0363bd3466e0387c935daea8a2f0e

---

From: ianswett <[email-masked]>
Date: Mon, 14 Aug 2017 23:56:20 +0000 (UTC)

ianswett commented on this pull request.

I'm not a fan of the change to offset because I honestly think it doesn't make sense in the concept of a QUIC connection(ie: it doesn't include control frames), but I think we should clarify the wording around "maximum amount of data".  I suggested something which can certainly be improved upon by others.

> @@ -1660,7 +1660,7 @@ Reason Phrase:
 ## MAX_DATA Frame {#frame-max-data}
 
 The MAX_DATA frame (type=0x04) is used in flow control to inform the peer of
-the maximum amount of data that can be sent on the connection as a whole.
+the maximum offset of data that can be sent on the connection as a whole.

I don't think the concept of an offset for a connection makes sense in QUIC.  It's not TCP, so what we're really talking about is the sum of all consumed stream flow controls.  So I'd suggest: "the maximum consumed stream flow control across all streams so far in the connection." or something similar?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/730#pullrequestreview-56234855

---

From: Martin Thomson <[email-masked]>
Date: Mon, 14 Aug 2017 17:05:38 -0700

I don't think that we should take either design change.

1. MAX_STREAM_DATA of 0 should mean that you can't send any data.  This prevents that.

2. The FIN bit doesn't take any space.  Having it take space is unnecessary.  See [my email to the list on this point](https://mailarchive.ietf.org/arch/msg/quic/zxDYecCajBYnGEKdh9mNLAk1ea0).

An editorial change might have some value.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/730#issuecomment-322342518

---

From: Subodh Iyengar <[email-masked]>
Date: Mon, 14 Aug 2017 17:16:22 -0700

@martinthomson 

1. could be solved by changing the condition to be MUST be LESS than as suggested in the summary. 
2. Ya let's discuss this point on the list.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/730#issuecomment-322343978

---

From: Subodh Iyengar <[email-masked]>
Date: Mon, 14 Aug 2017 17:20:26 -0700

siyengar commented on this pull request.

The text after this describes this in more detail.

I'm not a huge fan of "he maximum consumed stream flow control across all streams", This could be misinterpreted as 

offset = for all streams: max(stream offset)

Maybe "sum of maximum offsets of data that can be sent on all streams". However that seems a bit repetitive with the text below.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/730#discussion_r133093798

---

From: ianswett <[email-masked]>
Date: Mon, 14 Aug 2017 17:22:03 -0700

Good point.  In that case, I'm happy with the existing text.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/730#discussion_r133093979

---

From: Subodh Iyengar <[email-masked]>
Date: Mon, 14 Aug 2017 19:55:04 -0700

7b43058  Update draft-ietf-quic-transport.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/730/files/c22e84b1c2b0363bd3466e0387c935daea8a2f0e..7b430584bcc40636a82c3ffd01b581f3a7408709

---

From: Subodh Iyengar <[email-masked]>
Date: Mon, 14 Aug 2017 19:56:45 -0700

This PR should now be editorial

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/730#issuecomment-322364376

---

From: Martin Thomson <[email-masked]>
Date: Tue, 15 Aug 2017 03:51:53 +0000 (UTC)

martinthomson commented on this pull request.

> @@ -1728,11 +1727,11 @@ reordering can mean that the largest received offset on a stream can be greater
 than the total size of data received on that stream.  Receiving STREAM frames
 might not increase the largest received offset.
 
-The data sent on a stream MUST NOT exceed the largest maximum stream data value
-advertised by the receiver.  An endpoint MUST terminate a connection with a
-FLOW_CONTROL_ERROR error if it receives more data than the largest maximum
-stream data that it has sent for the affected stream, unless this is a result of
-a change in the initial limits (see {{zerortt-parameters}}).
+The largest offset sent on a stream MUST be less than the largest maximum stream offset

So I get MAX_STREAM_DATA with Maximum Stream Offset of 10.  I then send 10 octets (0..9) in one STREAM frame.  The maximum offset is therefore 9.  The empty STREAM frame I then send has an offset of 10, which would appear to violate this rule.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/730#pullrequestreview-56257510

---

From: Subodh Iyengar <[email-masked]>
Date: Mon, 14 Aug 2017 22:38:10 -0700

My understanding from our discussion on the mailing list is that that the intention of final offset is that it is not actually an offset, it just signals the number of bytes written so far.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/730#discussion_r133119754

---

From: Subodh Iyengar <[email-masked]>
Date: Mon, 14 Aug 2017 22:59:32 -0700

bb58a5e  Update draft-ietf-quic-transport.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/730/files/7b430584bcc40636a82c3ffd01b581f3a7408709..bb58a5eb746d450ee440c0dd1a8066cf3f263864

---

From: Subodh Iyengar <[email-masked]>
Date: Tue, 15 Aug 2017 06:38:50 +0000 (UTC)

7b6a98c  Update draft-ietf-quic-transport.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/730/files/bb58a5eb746d450ee440c0dd1a8066cf3f263864..7b6a98c54270ecaf9a6552bf1ad09a3489729d0a

---

From: Marten Seemann <[email-masked]>
Date: Mon, 14 Aug 2017 23:43:12 -0700

marten-seemann commented on this pull request.

> @@ -1669,19 +1669,18 @@ The frame is as follows:
  0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 |                                                               |
-+                        Maximum Data (64)                      +
++                        Maximum Offset (64)                    +

In order to be consistent, wouldn't we also have to rename the frame? 🙊

> @@ -1728,11 +1728,11 @@ reordering can mean that the largest received offset on a stream can be greater
 
-The data sent on a stream MUST NOT exceed the largest maximum stream data value
-advertised by the receiver.  An endpoint MUST terminate a connection with a
-FLOW_CONTROL_ERROR error if it receives more data than the largest maximum
-stream data that it has sent for the affected stream, unless this is a result of
-a change in the initial limits (see {{zerortt-parameters}}).
+The largest offset sent on a stream MUST be less than the largest maximum
+stream offset value advertised by the receiver.  An endpoint MUST terminate a

I find it counterintuitive to say that the largest offset that can be sent must be **less** than the maximum offset advertised.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/730#pullrequestreview-56253578

---

From: janaiyengar <[email-masked]>
Date: Wed, 16 Aug 2017 00:17:52 +0000 (UTC)

I don't think we want to flow control the FIN, which works out since the FIN does not consume an offset. The text actually seems clear on this point... if you keep everything in terms of bytes, and not in terms of offsets, the text is consistent. Subodh: Do you still see a need for clarification?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/730#issuecomment-322623734

---

From: Subodh Iyengar <[email-masked]>
Date: Tue, 15 Aug 2017 19:18:26 -0700

@janaiyengar  Sounds fair, I didn't see anyone on the list who really spoke up that this text is a significant improvement, and people generally seem to be ambivalent, so this PR doesn't seem worth it. I could reduce this to minor changes instead which doesn't rename the fields.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/730#issuecomment-322645467

---

From: Martin Thomson <[email-masked]>
Date: Tue, 15 Aug 2017 22:10:35 -0700

If you do have time to make those changes, a new PR would be best.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/730#issuecomment-322667906

---

From: Martin Thomson <[email-masked]>
Date: Wed, 16 Aug 2017 05:10:36 +0000 (UTC)

Closed #730.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/730#event-1207610504