Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Discuss Application-Limited Sending #1637

Merged
merged 13 commits into from Jan 29, 2019
Merged

Discuss Application-Limited Sending #1637

Changes from 6 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
ceab0e2
Discuss Application-Limited Sending
ianswett Aug 7, 2018
a38f3f9
Update draft-ietf-quic-recovery.md
ianswett Aug 7, 2018
b6546ab
Merge branch 'master' into ianswett-app-limited
ianswett Jan 11, 2019
f49e4a4
Rewrite and specifically note pacing
ianswett Jan 11, 2019
10f9ded
Update draft-ietf-quic-recovery.md
ianswett Jan 11, 2019
fb13365
Update draft-ietf-quic-recovery.md
martinthomson Jan 15, 2019
3fb5df1
Update draft-ietf-quic-recovery.md
ianswett Jan 16, 2019
6f6e34c
Merge branch 'master' into ianswett-app-limited
ianswett Jan 23, 2019
f9aaa91
Update draft-ietf-quic-recovery.md
ianswett Jan 23, 2019
bb96c70
Update draft-ietf-quic-recovery.md
janaiyengar Jan 29, 2019
c21ced5
Update draft-ietf-quic-recovery.md
janaiyengar Jan 29, 2019
07ef710
Update draft-ietf-quic-recovery.md
ianswett Jan 29, 2019
06cf5b0
Update draft-ietf-quic-recovery.md
ianswett Jan 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 16 additions & 4 deletions draft-ietf-quic-recovery.md
View file
Open in desktop
Expand Up @@ -923,7 +923,6 @@ congestion window is less than ssthresh, which typically only occurs after an
PTO. While in slow start, QUIC increases the congestion window by the number of
bytes acknowledged when each acknowledgment is processed.


## Congestion Avoidance

Slow start exits to congestion avoidance. Congestion avoidance in NewReno
Expand All @@ -945,7 +944,6 @@ The recovery period limits congestion window reduction to once per round trip.
During recovery, the congestion window remains unchanged irrespective of new
losses or increases in the ECN-CE counter.


## Probe Timeout

Probe packets MUST NOT be blocked by the congestion controller. A sender MUST
Expand All @@ -964,8 +962,7 @@ response of collapsing the congestion window on persistent congestion is
functionally similar to a sender's response on a Retransmission Timeout (RTO) in
TCP {{RFC5681}}.


## Pacing
## Pacing {#pacing}

This document does not specify a pacer, but it is RECOMMENDED that a sender pace
sending of all in-flight packets based on input from the congestion
Expand Down Expand Up @@ -999,6 +996,18 @@ paces the sending of any packets in excess of the initial congestion window.
A sender MAY implement alternate mechanisms to update its congestion window
after idle periods, such as those proposed for TCP in {{?RFC7661}}.

## Application Limited Sending

The congestion window should not be increased in slow start or congestion
avoidance when it is not fully utilized. The congestion window could be
under-utilized due to insufficient application data to send or flow control
ianswett marked this conversation as resolved.
Show resolved Hide resolved
limits.
ianswett marked this conversation as resolved.
Show resolved Hide resolved

When the sender is pacing (see {{pacing}}) packets, the sender may be unable
to use the full congestion window for a period of time after receiving an
ACK, due to pacing. In this case, the sender should not consider themselves
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ACK, due to pacing. In this case, the sender should not consider themselves
acknowledgment. In this case, the sender should not consider themselves

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I mean ACK, as in the frame?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need more than an ACK, you need one that contains fresh acknowledgments, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, changed to acknowledgements.

application limited and should allow the congestion window to increase.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really following the logic here. I think that you need more words.

The reason that the congestion window is limited in this fashion is that without being exercised, there is no assurance that this much data can be sent. If there is a sudden increase in demand from the application such that the inflated limit is now used, that results in using an untested limit, which might result in severe congestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SG, maybe @janaiyengar or @nibanks will have a suggestion?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's just a case of too much detail. I think folks should understand that just because a pacing interval has currently been used up, that's not considered application limited? So, maybe, just don't have this at all?


## Discarding Packet Number Space State

When keys for a packet number space are discarded, any in-flight packets
Expand Down Expand Up @@ -1119,6 +1128,9 @@ acked_packet from sent_packets.
if (InRecovery(acked_packet.time_sent)):
// Do not increase congestion window in recovery period.
return
if (IsAppLimited())
// Do not increase congestion_window if application limited.
return
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This gets into implementation specifics a bit, but I don't think the pseudocode can be as trivial as this.

Naively, if this function is called for two different packets in a row, without a corresponding send operation being performed between them, then only the first OnPacketAckedCC ever actually increases congestion_window because the previous call just decremented bytes_in_flight and incremented congestion_window guaranteeing that IsAppLimited() will always return false for future packets.

In a slightly more intelligent interpretation of this, where you actually preform this logic for all the bytes acknowledged in a whole ACK frame in a single call, you can still have the same problem if you end up processing two ACK frames back to back before you end up doing a send. I have some ideas on how to solve that (at least for my implementation) but I was wondering if this should be mentioned (if not pseudocoded) here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, getting this right is complex. I am going to not define the IsAppLimited() method, but document what it's intent is.

if (congestion_window < ssthresh):
// Slow start.
congestion_window += acked_packet.size
Expand Down