Fix time loss detection and early retransmit #394

ianswett · 2017-03-13T16:33:33Z

No description provided.

janaiyengar

Thanks Ian, a few comments

janaiyengar · 2017-03-13T17:19:45Z

draft-ietf-quic-recovery.md

-  threshold in time, rather than in packet number gaps.
+loss_time:
+: The time at which the next packet will be considered lost based on early
+transmit or exceeding the reordering window in time.


early transmit --> early retransmit

janaiyengar · 2017-03-13T17:20:29Z

draft-ietf-quic-recovery.md

-  packet's size.
+: An association of packet numbers to information about them, including a number
+  field indicating the packet number, a time field indicating the time a packet
+  was sent and a bytes field indicating the packet's size.  sent_packets is


oxford comma nit: "... was sent, and a bytes field ..."

janaiyengar · 2017-03-13T17:20:58Z

draft-ietf-quic-recovery.md

+  field indicating the packet number, a time field indicating the time a packet
+  was sent and a bytes field indicating the packet's size.  sent_packets is
+  ordered by packet number, and packets remain in sent_packets until
+  acknowledged or lost.


nit: "until they are acknowledged or lost"

I left it as is, because it's shorter.

janaiyengar · 2017-03-13T17:23:46Z

draft-ietf-quic-recovery.md

+   else:
+     reordering_threshold = kReorderingThreshold
+     time_reordering_fraction = infinite
+   lost_time = 0


loss_time = 0

SGTM, I went with loss_time everywhere.

janaiyengar · 2017-03-13T17:24:45Z

draft-ietf-quic-recovery.md

@@ -406,10 +410,9 @@ Pseudocode for SetLossDetectionAlarm follows:
        alarm_duration = 2 * smoothed_rtt
      alarm_duration = max(alarm_duration, kMinTLPTimeout)
      alarm_duration = alarm_duration << handshake_count
-    else if (largest sent packet is acked):
-      // Early retransmit
-      // with an alarm to reduce spurious retransmits.


I think this comment can fit in a line

It was removed.

janaiyengar · 2017-03-13T17:39:10Z

draft-ietf-quic-recovery.md

-       time_delta = acked.time_sent - unacked.time_sent
-       packet_delta = acked.packet_number - unacked.packet_number
-       if (time_delta > kTimeReorderThreshold * smoothed_rtt):
+     time_when_lost = 0;


This isn't the time but a wait period... suggestion: reordering_period ? or "short_timeout"? you could just call it short_timeout_period... up to you

How about delay_until_lost?

janaiyengar · 2017-03-13T17:46:56Z

draft-ietf-quic-recovery.md

-       packet_delta = acked.packet_number - unacked.packet_number
-       if (time_delta > kTimeReorderThreshold * smoothed_rtt):
+     time_when_lost = 0;
+     if (time_reordering_fraction == infinite):


This should be !=

janaiyengar · 2017-03-13T17:47:40Z

draft-ietf-quic-recovery.md

+     foreach (unacked less than largest_acked.packet_number):
+       time_since_sent = now() - unacked.time_sent
+       packet_delta = largest_acked.packet_number - unacked.packet_number
+       if (time_since_sent > time_when_lost):


if time loss detection is disabled, won't time_when_lost be 0?

Good point, I set it to infinite.

janaiyengar · 2017-03-13T17:50:12Z

draft-ietf-quic-recovery.md

         lost_packets.insert(unacked)
       else if (packet_delta > reordering_threshold)
         lost_packets.insert(unacked)
+       else if (loss_time == 0 && time_when_lost != 0):
+         loss_time = time_when_lost - time_since_sent


add a comment here saying something like "loss_time is used for setting the loss detection alarm later."

janaiyengar · 2017-03-13T17:51:36Z

draft-ietf-quic-recovery.md

     OnPacketsLost(lost_packets)
-     MaybeRetransmit(lost_packets)
+     foreach (packet in lost_packets)
+       sent_packets.remove(packet.packet_number)


does it make sense, for editorializing, to move these packets to a "lost_packets" structure? doesn't need to be done here, but thinking aloud

Possibly, though I still need a way to inform the congestion control of newly lost packets. I was thinking of creating a pending_retransmissions data structure in congestion control to store any lost packets that couldn't be sent immediately?

ianswett

Thanks Jana, updated.

ianswett · 2017-03-13T18:30:16Z

draft-ietf-quic-recovery.md

-  packet's size.
+: An association of packet numbers to information about them, including a number
+  field indicating the packet number, a time field indicating the time a packet
+  was sent and a bytes field indicating the packet's size.  sent_packets is


ianswett · 2017-03-13T18:30:32Z

draft-ietf-quic-recovery.md

+  field indicating the packet number, a time field indicating the time a packet
+  was sent and a bytes field indicating the packet's size.  sent_packets is
+  ordered by packet number, and packets remain in sent_packets until
+  acknowledged or lost.


I left it as is, because it's shorter.

ianswett · 2017-03-13T18:32:14Z

draft-ietf-quic-recovery.md

@@ -406,10 +410,9 @@ Pseudocode for SetLossDetectionAlarm follows:
        alarm_duration = 2 * smoothed_rtt
      alarm_duration = max(alarm_duration, kMinTLPTimeout)
      alarm_duration = alarm_duration << handshake_count
-    else if (largest sent packet is acked):
-      // Early retransmit
-      // with an alarm to reduce spurious retransmits.


It was removed.

ianswett · 2017-03-13T18:32:50Z

draft-ietf-quic-recovery.md

+   else:
+     reordering_threshold = kReorderingThreshold
+     time_reordering_fraction = infinite
+   lost_time = 0


SGTM, I went with loss_time everywhere.

ianswett · 2017-03-13T18:35:30Z

draft-ietf-quic-recovery.md

@@ -442,9 +445,9 @@ Pseudocode for OnLossDetectionAlarm follows:
       RetransmitAllHandshakePackets();
       handshake_count++;
     // TODO: Clarify early retransmit and time loss.
-     else if ():
+     else if (lost_time != 0):


ianswett · 2017-03-13T18:38:35Z

draft-ietf-quic-recovery.md

@@ -481,19 +484,31 @@ DetectLostPackets takes one parameter, acked, which is the largest acked packet.
 Pseudocode for DetectLostPackets follows:

 ~~~
-   DetectLostPackets(acked):
+   DetectLostPackets(largest_acked):


It's a packet, not a number, because I use fields of the packet below, such as the time it was sent.

ianswett · 2017-03-13T18:39:39Z

draft-ietf-quic-recovery.md

-       packet_delta = acked.packet_number - unacked.packet_number
-       if (time_delta > kTimeReorderThreshold * smoothed_rtt):
+     time_when_lost = 0;
+     if (time_reordering_fraction == infinite):


ianswett · 2017-03-13T18:41:39Z

draft-ietf-quic-recovery.md

-       time_delta = acked.time_sent - unacked.time_sent
-       packet_delta = acked.packet_number - unacked.packet_number
-       if (time_delta > kTimeReorderThreshold * smoothed_rtt):
+     time_when_lost = 0;


How about delay_until_lost?

ianswett · 2017-03-13T18:45:22Z

draft-ietf-quic-recovery.md

+     foreach (unacked less than largest_acked.packet_number):
+       time_since_sent = now() - unacked.time_sent
+       packet_delta = largest_acked.packet_number - unacked.packet_number
+       if (time_since_sent > time_when_lost):


Good point, I set it to infinite.

ianswett · 2017-03-13T18:47:58Z

draft-ietf-quic-recovery.md

     OnPacketsLost(lost_packets)
-     MaybeRetransmit(lost_packets)
+     foreach (packet in lost_packets)
+       sent_packets.remove(packet.packet_number)


Possibly, though I still need a way to inform the congestion control of newly lost packets. I was thinking of creating a pending_retransmissions data structure in congestion control to store any lost packets that couldn't be sent immediately?

Fix time loss detection and early retransmit

876df8f

ianswett requested a review from janaiyengar March 13, 2017 16:33

Fix trailing whitespace

26bd685

janaiyengar reviewed Mar 13, 2017

View reviewed changes

Fix inconsistencies and nits

55d791c

ianswett commented Mar 13, 2017

View reviewed changes

janaiyengar approved these changes Mar 13, 2017

View reviewed changes

janaiyengar merged commit 07510d1 into master Mar 13, 2017

ianswett mentioned this pull request Mar 14, 2017

Use packets or packet numbers in algorithms/pseudocode #310

Closed

martinthomson deleted the ianswett-timeloss branch April 21, 2017 05:27

martinthomson added design An issue that affects the design of the protocol; resolution requires consensus. -recovery labels May 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix time loss detection and early retransmit #394

Fix time loss detection and early retransmit #394

ianswett commented Mar 13, 2017

janaiyengar left a comment

janaiyengar Mar 13, 2017

janaiyengar Mar 13, 2017

ianswett Mar 13, 2017

janaiyengar Mar 13, 2017

ianswett Mar 13, 2017

janaiyengar Mar 13, 2017

ianswett Mar 13, 2017

janaiyengar Mar 13, 2017

ianswett Mar 13, 2017

janaiyengar Mar 13, 2017

ianswett Mar 13, 2017

janaiyengar Mar 13, 2017

ianswett Mar 13, 2017

janaiyengar Mar 13, 2017

ianswett Mar 13, 2017

janaiyengar Mar 13, 2017

janaiyengar Mar 13, 2017

ianswett Mar 13, 2017

ianswett left a comment

ianswett Mar 13, 2017

ianswett Mar 13, 2017

ianswett Mar 13, 2017

ianswett Mar 13, 2017

ianswett Mar 13, 2017

ianswett Mar 13, 2017

ianswett Mar 13, 2017

ianswett Mar 13, 2017

ianswett Mar 13, 2017

ianswett Mar 13, 2017

Fix time loss detection and early retransmit #394

Fix time loss detection and early retransmit #394

Conversation

ianswett commented Mar 13, 2017

janaiyengar 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

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

Choose a reason for hiding this comment

ianswett 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment