-
Notifications
You must be signed in to change notification settings - Fork 205
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
Clarify when to send acks #907
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this structure.
I apologize for probably re-reviewing text that you probably just moved, but it's hard to track the actual changes and we could improve on things more.
Feel free to push back or open new issues if you disagree.
draft-ietf-quic-transport.md
Outdated
### Sending ACK Frames | ||
|
||
Implementations MUST NOT generate packets that only contain ACK frames in | ||
response to packets which only contain ACK frames. However, they SHOULD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we want to MUST here. Requiring that all received packets be acknowledged is good for the protocol.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, agreed. I don't think there's a good reason to not ack these packets when an ACK frame is being sent anyways.
draft-ietf-quic-transport.md
Outdated
response to packets which only contain ACK frames. However, they SHOULD | ||
acknowledge packets containing only ACK frames when sending ACK frames in | ||
response to other packets. Implementations MUST NOT send more than one ACK | ||
frame per received packet that contains frames other than ACK frames. Packets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't make sense to me:
I can receive a packet that contains other than ACK frames (which is the norm, I would expect). But I might send more than one ACK frame over time. I think that what you intend to say here is part of a different statement.
That is, every time a packet that contains more than just ACK frames is received, an implementation schedules an ACK frame to be sent in a new packet. This might be immediately, but more often it will be delayed slightly. Delaying acknowledgment in this way ensures that packets arriving in rapid succession only induce a single packet in response or that acknowledgments can be sent together with stream data. Either way, fewer packets are sent, and fewer packets containing only ACK frames. Packets containing only ACK frames that are generated as a result of incoming packets MUST NOT be generated at a rate higher than the rate of incoming packets. Otherwise there would be in amplification in the number of packets, if not the number of bytes being exchanged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is new text, but I think it's correct. There's no case where receiving one packet generates two ack frames.
draft-ietf-quic-transport.md
Outdated
ack timer expires. | ||
|
||
To limit ACK blocks to those that have not yet been received by the sender, the | ||
receiver SHOULD track which ACK frames have been acknowledged by its peer. Once |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we also recommend or at least suggest tracking what acknowledgments are outstanding (sent but not yet acknowledged)? There might be some value in not re-sending all unacknowledged acknowledgments (where ack_delay_timer << RTT especially).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps a suggestion, but I think we should encourage receivers to re-ack as much as they can, for resilience and performance. How about something like: "In every ACK frame, a receiver SHOULD attempt to acknowledge all received packets whose acknowledgement is not yet known to have reached the sender. However, a receiver MAY avoid re-acknowledging all of them in subsequent ACK frames, and it may track outstanding acknowledgements to do so. Note that doing so reduces the connection's resilience to loss of acknowledgements." I think the wording could be improved, but something like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. I think this is captured in the "to limit receiver state below". Do we need any more text here? I think it may still be valuable to state the first sentence in my suggestion above perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't an example of limiting state, it's limiting bytes transmitted. I think that your formulation here (MAY avoid sending an acknowledgment if an ACK frame containing the same acknowledgment is already in flight) is a good way to deal with this.
The limiting of state does end up having the same net effect (fewer ACK ranges), but it's because the receiver is discarding information. This would require more effort, but would not inherently discard state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this limiting of state is described a few paragraphs above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
limiting of state is below. I'm making a different point. We don't send stream data that we just sent, so it is possible that duplicating acknowledgments in the same way is inefficient. Now, Jana rightly observes that the cost is low and the benefits high, which is an excellent point, but there might be cases where the cost is high enough (large BDP connections for example) to warrant trimming ACK frames down in size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, understood. In that case, I think the text below is sufficient for now?
number of ACK blocks it sends. A receiver can do this even without receiving | ||
acknowledgment of its ACK frames, with the knowledge this could cause the sender | ||
to unnecessarily retransmit some data. When this is necessary, the receiver | ||
SHOULD acknowledge newly received packets and stop acknowledging packets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that s/SHOULD/can/ I don't think that we want to mandate anything here.
That is, unless your intent was to say that acknowledging the most recently received packets is preferable, but then I would say "if acknowledgments are removed rather than retransmitted, a receiver SHOULD discard acknowledgments with the lowest packet number first". Or something along those lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lowest packet number isn't necessarily the one that was the oldest. What you want really is to discard acks for the earliest received packets. I'm not concerned about the SHOULD here, I think that's fine... but I'm also fine with replacing that with a "can".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's oldest at the sender, so I am assuming that it will be marked as lost first (or might have already).
Separately, should we also note that dropping ACKs causes packets to be marked as lost causes congestion control to receive a warped signal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think SHOULD is still good here, since this is what we recommend implementations do, and as Jana said, in theory it's not as simple as dropping smallest packet number.
I'll add a note that dropping ack blocks early increases the chance the peer never receives an acknowledgement and may be forced to spuriously retransmit the data it contains.
draft-ietf-quic-transport.md
Outdated
acknowledge packets containing only ACK frames when sending ACK frames in | ||
response to other packets. Implementations MUST NOT send more than one ACK | ||
frame per received packet that contains frames other than ACK frames. Packets | ||
containing non ACK frames MUST be acknowledged immediately or when a delayed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"non-ACK", or just "other"
Ok, I think this is enough of an improvement I'm going to merge it and we can follow up with more improvements later. |
Clarify when to send ACK frames in the transport doc, as discussed in Seattle.