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 2 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
12 changes: 11 additions & 1 deletion draft-ietf-quic-recovery.md
View file
Open in desktop
Expand Up @@ -966,7 +966,6 @@ congestion window is less than ssthresh, which typically only occurs after an
RTO. While in slow start, QUIC increases the congestion window by the number of
bytes acknowledged when each ack is processed.


## Congestion Avoidance

Slow start exits to congestion avoidance. Congestion avoidance in NewReno
Expand All @@ -989,6 +988,11 @@ 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.

## Application Limited Sending

If the sender is sufficiently application limited that the congestion window is
Copy link
Member

Choose a reason for hiding this comment

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

We don't really get a definition of "application limited" from this. The point is that you won't increase the congestion window if the entire congestion window isn't being actively used.

Is there precedent for this sort of capping of the window?

I assume that this includes flow control as the source of the limitation. Which might be worth mentioning.

not fully utilized, the congestion window should not be increased in slow start
or congestion avoidance.

## Tail Loss Probe

Expand Down Expand Up @@ -1115,13 +1119,19 @@ acked_packet from sent_packets.
~~~
InRecovery(packet_number):
return packet_number <= end_of_recovery

ianswett marked this conversation as resolved.
Show resolved Hide resolved
IsAppLimited():
bytes_in_flight >= congestion_window - kMaximumDatagramSize
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this backward? Bytes in flight can't be greater than the congestion window, but if the bytes in flight are within one datagram of the window, you're clearly not limited.

Copy link
Member

Choose a reason for hiding this comment

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

It reads backwards to me as well. And is it the right test? @nibanks' comments suggest that there might be other ways to detect the condition of not being limited. For instance, as long as data is enqueued for sending, it might be reasonable to say that the application is waiting on the congestion controller (or pacer) to allow sending.

Copy link
Member

Choose a reason for hiding this comment

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

I agree this is backward. The >= should just be < or <=. But as far as tracking IsAppLimited() or whatever you want to call it, I would just suggest a modification to the code:

Instead of using bytes_in_flight here and the helper function IsAppLimited, introduce a new variable window_full that is only set in OnPacketSent. At the end of each call to that function, if bytes_in_flight is close enough to congestion_window set it to true, otherwise false. Then below, you use that variable to decide if the cwnd should be increased..

Copy link
Member

Choose a reason for hiding this comment

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

That seems better, yes. When you say "close enough", I assume that you mean "so close that you couldn't send anything meaningful", something like headerlen+sizeof(cid)+16+sizeof(STREAM frame overhead), so maybe sizeof(cid)+32 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm okay with what's in the pseudocode -- so close you couldn't send another full-size datagram.

Copy link
Member

Choose a reason for hiding this comment

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

I was specifically not defining "close enough". I am fine with a single datagram size or something smaller.


OnPacketAckedCC(acked_packet):
// Remove from bytes_in_flight.
bytes_in_flight -= acked_packet.bytes
if (InRecovery(acked_packet.packet_number)):
// 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.bytes
Expand Down