Subject: [quicwg/base-drafts] Ignoring ACK delay can result in wrong RTT calculations (#3350)

From: kixelated <[email-masked]>
Date: Wed, 15 Jan 2020 12:00:50 -0800

I'm implementing the RTT estimation code in quic-recovery at the moment and I've run into a potential edge case with ACK delays. This is entirely hypothetical for the moment.

For example, suppose `min_rtt = 100` and our actual round trip time is 99ms, 100ms, or 101ms with equal frequency. This minimum RTT was calculated earlier (ex. handshake) but now the peer is delaying ACKs.

If we receive an ACK with `latest_rtt = 126` and `ack_delay = 25`, then `adjusted_rtt = 101`.
If we receive an ACK with `latest_rtt = 125` and `ack_delay = 25`, then `adjusted_rtt = 125`.
If we receive an ACK with `latest_rtt = 124` and `ack_delay = 25`, then `adjusted_rtt = 124`.

The computed result is `smoothed_rtt=117` and `rttvar=11`.
The "actual" result should be closer to `smoothed_rtt=100` and `rttvar=2/3`.

Any delayed ACKs sent faster than `min_rtt` will skew the results based on the `ack_delay`. This includes ACKs sent over a faster path. 

These RTT estimates can be brought closer to reality with a more accurate `min_rtt`. This can only be accomplished if the peer transmits an ACK with `ack_delay=0` and gets lucky.

However, a peer will only send an immediate ACK if it receives two ack-eliciting packets within `max_ack_delay` of each other (and the peer follows the RFC recommendation). Ironically, this is the situation where estimating the RTT is the most important, as the packet rate is too low for fast recovery.

-- 
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/3350

---

From: ianswett <[email-masked]>
Date: Wed, 15 Jan 2020 12:14:35 -0800

I'll note that persistent delayed acks are bad for performance for a variety of reasons, but let's put that aside for now.

I believe the problematic text is:

> MUST NOT apply the adjustment if the resulting RTT sample is smaller than the min_rtt. This limits the underestimation that a misreporting peer can cause to the smoothed_rtt.

The alternative would be to allow the RTT sample to be as small as min_rtt, but no smaller, rather than ignoring the ack_delay entirely when it created a sample less than min_rtt.  The negative of this is it allows a buggy/malicious peer to always specify an overly large ack_delay, which drives smoothed to min_rtt and results in an RTTVar of 0.  This would make PTO quite aggressive, though given we're also adding max_ack_delay onto the PTO, possibly it's acceptable.

In real-world scenarios, I suspect it doesn't matter that much, which is why we went with the more conservative option.

-- 
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/3350#issuecomment-574836935

---

From: kixelated <[email-masked]>
Date: Wed, 15 Jan 2020 12:16:13 -0800

Using the above example, where we expect `smoothed_rtt=100` and `rttvar=0.6`

Here is the problematic code. It produces `smoothed_rtt=116.6` and `rttvar=10.4`
```
    adjusted_rtt = latest_rtt
     if (latest_rtt > min_rtt + ack_delay):
       adjusted_rtt = latest_rtt - ack_delay
```

Like @ianswett said, an improvement would be to use the `ack_delay` while `adjusted_rtt >= min_rtt`:

``
    adjusted_rtt = min(latest_rtt - ack_delay, min_rtt)
``

It produces `smoothed_rtt=100.3` and `rttvar=0.4`. However, this will still over-estimate the smooth and under-estimate the variance.

-- 
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/3350#issuecomment-574837523

---

From: ianswett <[email-masked]>
Date: Wed, 15 Jan 2020 12:18:31 -0800

I think you want a max(), not a min() but yes, that would be the alternative approach.

-- 
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/3350#issuecomment-574838374

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 15 Jan 2020 14:26:43 -0800

Our principle was that an endpoint should not use an RTT sample that is smaller than any RTT it has _directly_ measured.

What is in the draft, as Ian pointed out, is the conservative approach that protects the endpoint from poor reporting of ack delay. A poor min_rtt will lead you astray but that is because the endpoint does not have direct observation of a smaller RTT. Until it does, it is better to be conservative.

-- 
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/3350#issuecomment-574886270

---

From: ianswett <[email-masked]>
Date: Wed, 15 Jan 2020 14:35:25 -0800

I'm not suggesting we should change the existing text, but I believe the change suggested above does fit with the principle you're stating @janaiyengar because you would still never allow the adjusted_rtt to go below min_rtt.

I will note that I hadn't anticipated the RTTVar inflation caused by sometimes ignoring ack_delay completely.

-- 
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/3350#issuecomment-574889220

---

From: mirjak <[email-masked]>
Date: Thu, 23 Jan 2020 01:42:19 -0800

The other option would be to not update smoothed_rtt at all... but the we need a way to ensure that we update still frequently enough.

Also the code should probably say at least (adding =):

if (latest_rtt => min_rtt + 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/3350#issuecomment-577603355

---

From: Martin Thomson <[email-masked]>
Date: Thu, 23 Jan 2020 14:25:55 -0800

Assuming that you mean `latest_rtt >= min_rtt + ack_delay`, that seems reasonable.

-- 
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/3350#issuecomment-577907055