Restrict ACK-clocking to ACK-only frames. #1231

ekr · 2018-03-18T21:42:48Z

As written, this would forbid the case where A sends a single stream
frame and then B responds with three stream frames and piggybacks
ACKs along with each. That seems silly and a lot of work. Rather,
this should just be about ACK-only frames.

frame and then B responds with three stream frames and piggybacks ACKs along with each. That seems silly and a lot of work. Rather, this should just be about ACK-only frames.

ianswett · 2018-03-19T00:30:46Z

draft-ietf-quic-transport.md

-containing non-ACK frames MUST be acknowledged immediately or when a delayed
-ack timer expires.
+response to other packets.  Implementations MUST NOT send more than one packet
+containing only ACK frames per received packet that contains frames other than


I have no objection, but is the motivation for this the coming CRYPTO_ACK/ACK split?

No. It was purely that the text didn't match my expectations

marten-seemann

I'm not sure I really I understand the problem you're trying to solve.
If you really want to send duplicate ACK frames (without receiving any new packets, and without any timer expiring), the change of the first sentence in your PR would be sufficient.

marten-seemann · 2018-03-19T09:55:37Z

draft-ietf-quic-transport.md

-an endpoint MAY send a PING frame once per RTT to solicit an acknowledgment.
+Because ACK-only packets are not acknowledged, a receiver that is only
+sending ACK frames will only receive acknowledgements for its packets
+if the sender piggybacks them on other packets. In order to enable


Why does it play a role how the sender bundles frames into packets?

Consider the case where the receiver issued 1MB of flow credit and the sender sends 950K of data. In this case the receiver might just send ACK-only packets which the sender is forbidden to send its own ACK-only packets. Thus, if the sender never bundles ACKs, the receiver will never get any.

Where is the problem? You don't expect to get any ACK for ACK-only packets, they don't count towards bytes-in-flight, they are never retransmitted.

Well, the text I am updating assumes it is a problem, I'm just fixing it up.

With that said, while the packets don't count towards bytes-in-flight, you generally end up storing the data about the to-be-ACKed packets (as implied by the following text):

receiver SHOULD track which ACK frames have been acknowledged by its peer. Once an ACK frame has been acknowledged, the packets it acknowledges SHOULD NOT be acknowledged again.``` Thus, you can't garbage collect this state until those ACKs are ACKed.

I think that is an unfortunate requirement (SHOULD NOT be ack'ed again):
You want to ack packets again because you collapse gaps into larger blocks. Only the oldest ACK'ed block should not be acked again.

@mikkelfj: I suggest you file a separate issue.

The text as it stands makes sense: If a peer wants an ACK for an ACK (e.g. to prune the lowest acked packet), it can include a retransmittable frame. This depends on which ack-pruning algorithm it uses, and it has nothing to do with how frames are bundled.

That part of the text is correct. What's not correct is the claim that you will not receive ACKs if you do not do so.

marten-seemann · 2018-03-19T09:56:17Z

draft-ietf-quic-transport.md

+Because ACK-only packets are not acknowledged, a receiver that is only
+sending ACK frames will only receive acknowledgements for its packets
+if the sender piggybacks them on other packets. In order to enable
+this, senders SHOULD prioritize sending ACKs along with their data.


I don't think you should prioritize ACKs, if the ACKs don't ack any new retransmittable packets.

I agree with this. The point of changing this text is to make it clear what the consequences of particular design choices are. I would remove this sentence (starting with "In order to enable").

I think prioritize is a loaded word, maybe "senders SHOULD bundle ACKs with non-ACK frames when possible."

marten-seemann · 2018-03-19T09:57:37Z

draft-ietf-quic-transport.md

+if the sender piggybacks them on other packets. In order to enable
+this, senders SHOULD prioritize sending ACKs along with their data.
+In addition, if a receiver regularly issues additional flow control
+credit rather than just issuing infinite flow control credit at the


Issuing infinite flow control credit is a really bad idea. I don't think we should mention this as an option here.

Again, I would remove "rather than just issuing infinite flow control credit at the beginning". The point remains good though, so I'd keep the rest.

I would remove this sentence.

ianswett · 2018-03-19T10:50:56Z

draft-ietf-quic-transport.md

-its packets.  Sending an occasional MAX_DATA or MAX_STREAM_DATA frame as data is
-received will ensure that acknowledgements are generated by a peer.  Otherwise,
-an endpoint MAY send a PING frame once per RTT to solicit an acknowledgment.
+Because ACK-only packets are not acknowledged, a receiver that is only


ACK-only packets are acknowledged, but they do not cause an ACK frame to be sent.

Maybe: ""Because ACK frames are not sent in response to ACK-only packets, ..."

ianswett · 2018-03-19T10:53:48Z

draft-ietf-quic-transport.md

+if the sender piggybacks them on other packets. In order to enable
+this, senders SHOULD prioritize sending ACKs along with their data.
+In addition, if a receiver regularly issues additional flow control
+credit rather than just issuing infinite flow control credit at the


I would remove this sentence.

martinthomson

GitHub ate my homework, but let me take another look...

martinthomson · 2018-03-19T10:42:33Z

draft-ietf-quic-transport.md

+Because ACK-only packets are not acknowledged, a receiver that is only
+sending ACK frames will only receive acknowledgements for its packets
+if the sender piggybacks them on other packets. In order to enable
+this, senders SHOULD prioritize sending ACKs along with their data.


I agree with this. The point of changing this text is to make it clear what the consequences of particular design choices are. I would remove this sentence (starting with "In order to enable").

martinthomson · 2018-03-19T10:43:26Z

draft-ietf-quic-transport.md

+if the sender piggybacks them on other packets. In order to enable
+this, senders SHOULD prioritize sending ACKs along with their data.
+In addition, if a receiver regularly issues additional flow control
+credit rather than just issuing infinite flow control credit at the


Again, I would remove "rather than just issuing infinite flow control credit at the beginning". The point remains good though, so I'd keep the rest.

As written, this would forbid the case where A sends a single stream

503027d

frame and then B responds with three stream frames and piggybacks ACKs along with each. That seems silly and a lot of work. Rather, this should just be about ACK-only frames.

ekr requested a review from martinthomson March 18, 2018 21:42

ekr mentioned this pull request Mar 18, 2018

ACKs for receiver-only #1232

Closed

martinthomson requested a review from janaiyengar March 18, 2018 23:17

martinthomson approved these changes Mar 18, 2018

View reviewed changes

ianswett reviewed Mar 19, 2018

View reviewed changes

Rephrase the text about one-way sending

32f3bd9

ekr force-pushed the ack_frame_counting branch from 3a7437d to 32f3bd9 Compare March 19, 2018 09:43

marten-seemann reviewed Mar 19, 2018

View reviewed changes

ianswett reviewed Mar 19, 2018

View reviewed changes

Update per comments

0359cc7

martinthomson reviewed Mar 19, 2018

View reviewed changes

MikeBishop added the -transport label Mar 20, 2018

martinthomson added 2 commits April 4, 2018 08:26

Ian's tweak

0ad934a

Tweaking some more

88d27ac

ianswett approved these changes Apr 3, 2018

View reviewed changes

martinthomson merged commit 02d558a into quicwg:master Apr 9, 2018

martinthomson added the editorial An issue that does not affect the design of the protocol; does not require consensus. label Jun 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restrict ACK-clocking to ACK-only frames. #1231

Restrict ACK-clocking to ACK-only frames. #1231

ekr commented Mar 18, 2018

ianswett Mar 19, 2018

ekr Mar 19, 2018

marten-seemann left a comment

marten-seemann Mar 19, 2018

ekr Mar 19, 2018

marten-seemann Mar 19, 2018

ekr Mar 19, 2018

mikkelfj Mar 19, 2018

ekr Mar 19, 2018

mikkelfj Mar 19, 2018

marten-seemann Mar 19, 2018

ekr Mar 19, 2018

marten-seemann Mar 19, 2018

martinthomson Mar 19, 2018

ianswett Mar 19, 2018

marten-seemann Mar 19, 2018

martinthomson Mar 19, 2018

ianswett Mar 19, 2018

ianswett Mar 19, 2018

ianswett Mar 19, 2018

martinthomson left a comment

martinthomson Mar 19, 2018

martinthomson Mar 19, 2018

Restrict ACK-clocking to ACK-only frames. #1231

Restrict ACK-clocking to ACK-only frames. #1231

Conversation

ekr commented Mar 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marten-seemann 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

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