Subject: [quicwg/base-drafts] Discuss Application-Limited Sending (#1637)

From: ianswett <[email-masked]>
Date: Tue, 07 Aug 2018 18:59:10 +0000 (UTC)

Fixes #1619
You can view, comment on, or merge this pull request online at:

  https://github.com/quicwg/base-drafts/pull/1637

-- Commit Summary --

  * Discuss Application-Limited Sending

-- File Changes --

    M draft-ietf-quic-recovery.md (14)

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/1637.patch
https://github.com/quicwg/base-drafts/pull/1637.diff

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1637

---

From: janaiyengar <[email-masked]>
Date: Tue, 07 Aug 2018 12:14:14 -0700

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

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1637#issuecomment-411169079

---

From: janaiyengar <[email-masked]>
Date: Tue, 07 Aug 2018 19:15:15 +0000 (UTC)

janaiyengar commented on this pull request.

> @@ -989,6 +988,13 @@ 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
+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.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1637#pullrequestreview-144144527

---

From: Nick Banks <[email-masked]>
Date: Tue, 07 Aug 2018 12:26:04 -0700

nibanks commented on this pull request.

    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.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1637#pullrequestreview-144148863

---

From: ianswett <[email-masked]>
Date: Tue, 07 Aug 2018 13:05:07 -0700

ianswett commented on this pull request.

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

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1637#discussion_r208367314

---

From: ianswett <[email-masked]>
Date: Tue, 07 Aug 2018 13:05:36 -0700

@ianswett pushed 1 commit.

a38f3f9  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1637/files/ceab0e21afc6c181d40ca07ca34a8d86027da7e7..a38f3f9e44b95bed81592394ecbeb5f6eb0fa97a

---

From: Mike Bishop <[email-masked]>
Date: Tue, 07 Aug 2018 14:08:33 -0700

MikeBishop commented on this pull request.

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

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

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1637#pullrequestreview-144185455

---

From: Mike Bishop <[email-masked]>
Date: Tue, 07 Aug 2018 14:09:13 -0700

~~~

Build failure is trailing whitespace on this line.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1637#pullrequestreview-144185887

---

From: Martin Thomson <[email-masked]>
Date: Tue, 07 Aug 2018 19:22:01 -0700

martinthomson commented on this pull request.

> @@ -989,6 +988,11 @@ The recovery period limits congestion window reduction to once per round trip.

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.

 ~~~

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.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1637#pullrequestreview-144247689

---

From: Nick Banks <[email-masked]>
Date: Wed, 08 Aug 2018 16:00:02 +0000 (UTC)

~~~

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

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1637#discussion_r208639187

---

From: Martin Thomson <[email-masked]>
Date: Wed, 08 Aug 2018 14:38:57 -0700

~~~

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 ?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1637#discussion_r208744828

---

From: Mike Bishop <[email-masked]>
Date: Wed, 08 Aug 2018 15:21:32 -0700

~~~

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

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1637#discussion_r208754731

---

From: Nick Banks <[email-masked]>
Date: Wed, 08 Aug 2018 16:25:00 -0700

~~~

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

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1637#discussion_r208766163

---

From: mirjak <[email-masked]>
Date: Thu, 09 Aug 2018 09:59:01 -0700

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.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1637#issuecomment-411826667