Subject: [quicwg/base-drafts] Only Update max_ack_delay for Retransmittable Packets (#1531)

From: Nick Banks <[email-masked]>
Date: Fri, 06 Jul 2018 11:11:55 -0700

I came across this pseudocode while debugging an issue in my implementation related to it:

```
  UpdateRtt(latest_rtt, ack_delay):
    // min_rtt ignores ack delay.
    min_rtt = min(min_rtt, latest_rtt)
    // Adjust for ack delay if it's plausible.
    if (latest_rtt - min_rtt > ack_delay):
      latest_rtt -= ack_delay
      // Only save into max ack delay if it's used
      // for rtt calculation and is not ack only.
      if (!sent_packets[ack.largest_acked].ack_only)
        max_ack_delay = max(max_ack_delay, ack_delay)
```

Should this be ignoring all packets that don't contain any rentrasmittable frames, instead of just `ack_only` packets?

-- 
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/issues/1531

---

From: Martin Thomson <[email-masked]>
Date: Tue, 10 Jul 2018 18:58:58 -0700

This seems like a real issue.  The test is there because acknowledgements aren't generated directly as a result of receiving some packets.  This includes ACK-only packets, but a definition for what that means isn't in the doc.  

I think that there is a simple fix - define "ACK-only" as being any packet that comprises ACK, ACK_ECN, and PADDING frames only.

There is still a bug in determining maximum ACK delay though.  If the ACK is lost and the receiver has their own data to send some time afterwards, they might bundle an ACK with that new packet.  That would force the delay up.  I think that we should discuss explicit signaling of maximum ACK delay.

-- 
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/issues/1531#issuecomment-404020322

---

From: ianswett <[email-masked]>
Date: Tue, 10 Jul 2018 19:06:32 -0700

The intent here is that ack_only == !retransmittable, but you're right that's never defined.

@martinthomson Given that's where TCP is heading, I agree we should add explicit signaling for max ack delay.  Previously Jana convinced me it wasn't strictly necessary, but measuring it is increasingly feeling like a research project we shouldn't take on for v1.

-- 
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/issues/1531#issuecomment-404021439

---

From: ianswett <[email-masked]>
Date: Tue, 10 Jul 2018 19:13:57 -0700

After taking another look, this is a bit larger fix.  I'll try to address it in PR#1540, because I now need an ack_only bool to indicate it's only ACK, ACK_ECN, or PADDING, as well as an in_flight bool, because PADDING counts towards in flight, but do not elicit(is that better?) acknowledgements.

-- 
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/issues/1531#issuecomment-404022515

---

From: janaiyengar <[email-masked]>
Date: Sat, 14 Jul 2018 07:11:53 -0700

Closed #1531 via #1540.

-- 
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/issues/1531#event-1733882737