Discuss Application-Limited Sending #1637

ianswett · 2018-08-07T18:59:04Z

janaiyengar · 2018-08-07T19:12:08Z

Both app-limited sending and also receiver-window limited sending should be incorporated in the text here.

janaiyengar · 2018-08-07T19:13:45Z

draft-ietf-quic-recovery.md

+not fully utilized, the congestion window should not be increased in slow start
+or congestion avoidance.  Senders should consider themselves application limited
+if bytes in flight when receiving an ACK frame are more than a max datgram size
+less than the congestion window.


This last sentence is a run-on and unnecessarily detailed. I would simply say something like "if the congestion window is not fully utilized." The detail of kMaxDatagramSize is a rounding thing, which is probably too much detail in this descriptive text.

In that case, I think I should just remove that sentence?

nibanks · 2018-08-07T19:26:01Z

draft-ietf-quic-recovery.md


   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


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.

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

MikeBishop

Given that it's a compound adjective, wouldn't it be "application-limited" throughout?

MikeBishop · 2018-08-07T21:07:58Z

draft-ietf-quic-recovery.md

@@ -1115,13 +1119,19 @@ acked_packet from sent_packets.
 ~~~
   InRecovery(packet_number):
     return packet_number <= end_of_recovery
+
+   IsAppLimited():
+     bytes_in_flight >= congestion_window - kMaximumDatagramSize


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.

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.

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..

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 ?

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

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

draft-ietf-quic-recovery.md

martinthomson · 2018-08-08T02:19:44Z

draft-ietf-quic-recovery.md

@@ -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


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.

martinthomson · 2018-08-08T02:21:24Z

draft-ietf-quic-recovery.md

@@ -1115,13 +1119,19 @@ acked_packet from sent_packets.
 ~~~
   InRecovery(packet_number):
     return packet_number <= end_of_recovery
+
+   IsAppLimited():
+     bytes_in_flight >= congestion_window - kMaximumDatagramSize


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.

mirjak · 2018-08-09T16:58:58Z

Actually as mentioned in #1619 one would need to also reduce the window if application-limited and the send window goes below the congestion window. See https://datatracker.ietf.org/doc/rfc7661/ for TCP.

draft-ietf-quic-recovery.md

martinthomson · 2019-01-13T22:41:30Z

draft-ietf-quic-recovery.md

+
+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


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

I think I mean ACK, as in the frame?

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

True, changed to acknowledgements.

martinthomson · 2019-01-13T22:46:31Z

draft-ietf-quic-recovery.md

+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
+application limited and should allow the congestion window to increase.


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.

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

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?

Co-Authored-By: ianswett <ianswett@users.noreply.github.com>

nibanks

Looks fine to me.

draft-ietf-quic-recovery.md

janaiyengar · 2019-01-28T05:45:24Z

draft-ietf-quic-recovery.md

+to use the full congestion window for a period of time after receiving
+acknowledgements, due to pacing.  In this case, the sender should not consider
+themselves application limited and should allow the congestion window to
+increase.


Suggested rephrasing: "A sender that uses pacing (see {{pacing}}) might delay sending of packets and might not fully utilize the congestion window due to this delay. A sender should not consider itself application limited if it might have utilized the entire congestion window without pacing delay."

Accepted with minor tweaks.

Co-Authored-By: ianswett <ianswett@users.noreply.github.com>

Discuss Application-Limited Sending

ceab0e2

Fixes #1619

ianswett requested a review from janaiyengar August 7, 2018 18:59

janaiyengar reviewed Aug 7, 2018

View reviewed changes

nibanks reviewed Aug 7, 2018

View reviewed changes

Update draft-ietf-quic-recovery.md

a38f3f9

MikeBishop reviewed Aug 7, 2018

View reviewed changes

draft-ietf-quic-recovery.md Outdated Show resolved Hide resolved

martinthomson reviewed Aug 8, 2018

View reviewed changes

martinthomson added the -recovery label Aug 8, 2018

ianswett added 3 commits January 11, 2019 15:39

Merge branch 'master' into ianswett-app-limited

b6546ab

Rewrite and specifically note pacing

f49e4a4

Update draft-ietf-quic-recovery.md

10f9ded

martinthomson reviewed Jan 13, 2019

View reviewed changes

Update draft-ietf-quic-recovery.md

fb13365

Co-Authored-By: ianswett <ianswett@users.noreply.github.com>

nibanks approved these changes Jan 15, 2019

View reviewed changes

ianswett added 3 commits January 16, 2019 12:26

Update draft-ietf-quic-recovery.md

3fb5df1

Merge branch 'master' into ianswett-app-limited

6f6e34c

Update draft-ietf-quic-recovery.md

f9aaa91

janaiyengar reviewed Jan 28, 2019

View reviewed changes

janaiyengar and others added 4 commits January 28, 2019 21:03

Update draft-ietf-quic-recovery.md

bb96c70

Co-Authored-By: ianswett <ianswett@users.noreply.github.com>

Update draft-ietf-quic-recovery.md

c21ced5

Co-Authored-By: ianswett <ianswett@users.noreply.github.com>

Update draft-ietf-quic-recovery.md

07ef710

Update draft-ietf-quic-recovery.md

06cf5b0

janaiyengar merged commit d3fc302 into master Jan 29, 2019

MikeBishop deleted the ianswett-app-limited branch February 6, 2019 00:10

WesleyRosenblum mentioned this pull request Jan 28, 2021

Congestion window may grow too slowly aws/s2n-quic#458

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Discuss Application-Limited Sending #1637

Discuss Application-Limited Sending #1637

ianswett commented Aug 7, 2018

janaiyengar commented Aug 7, 2018

janaiyengar Aug 7, 2018

ianswett Aug 7, 2018

nibanks Aug 7, 2018

ianswett Jan 11, 2019

MikeBishop left a comment

MikeBishop Aug 7, 2018

martinthomson Aug 8, 2018

nibanks Aug 8, 2018

martinthomson Aug 8, 2018

MikeBishop Aug 8, 2018

nibanks Aug 8, 2018

martinthomson Aug 8, 2018

martinthomson Aug 8, 2018

mirjak commented Aug 9, 2018

martinthomson Jan 13, 2019

ianswett Jan 15, 2019

martinthomson Jan 16, 2019

ianswett Jan 16, 2019

martinthomson Jan 13, 2019

ianswett Jan 15, 2019

nibanks Jan 15, 2019

nibanks left a comment

janaiyengar Jan 28, 2019 •

edited

ianswett Jan 29, 2019

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

Discuss Application-Limited Sending #1637

Discuss Application-Limited Sending #1637

Conversation

ianswett commented Aug 7, 2018

janaiyengar commented Aug 7, 2018

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

MikeBishop 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

mirjak commented Aug 9, 2018

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

nibanks left a comment

Choose a reason for hiding this comment

janaiyengar Jan 28, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janaiyengar Jan 28, 2019 •

edited