BUG/MINOR: quic: Wrong RTT computation (srtt and rrt_var)

Due to the fact that several variable values (rtt_var, srtt) were stored as multiple of their real values, some calculations were less accurate as expected. Stop storing 4*rtt_var values, and 8*srtt values. Adjust all the impacted statements. Must be backported as far as 2.6. (cherry picked from commit fb4294b) |cf: some changes were applied in quic_conn.c because quic_cli.c and quic_trace.c don't exist] Signed-off-by: Christopher Faulet <[email protected]> (cherry picked from commit 35fb593) Signed-off-by: Christopher Faulet <[email protected]> (cherry picked from commit ba2cb69) Signed-off-by: Christopher Faulet <[email protected]>
FireBurn · Sep 7, 2023 · dfa9730 · dfa9730
1 parent 5958011
commit dfa9730
Show file tree

Hide file tree

Showing 4 changed files with 19 additions and 21 deletions.
diff --git a/include/haproxy/quic_loss-t.h b/include/haproxy/quic_loss-t.h
@@ -44,13 +44,13 @@
  */
 
 struct quic_loss {
-	/* The most recent RTT measurement. */
+	/* The most recent RTT measurement (ms) */
 	unsigned int latest_rtt;
-	/* Smoothed RTT << 3 */
+	/* Smoothed RTT (ms) */
 	unsigned int srtt;
-	/* RTT variation << 2 */
+	/* RTT variation (ms) */
 	unsigned int rtt_var;
-	/* Minimum RTT. */
+	/* Minimum RTT (ms) */
 	unsigned int rtt_min;
 	/* Number of NACKed sent PTO. */
 	unsigned int pto_count;

diff --git a/include/haproxy/quic_loss.h b/include/haproxy/quic_loss.h
@@ -34,8 +34,8 @@
 
 static inline void quic_loss_init(struct quic_loss *ql)
 {
-	ql->srtt = QUIC_LOSS_INITIAL_RTT << 3;
-	ql->rtt_var = (QUIC_LOSS_INITIAL_RTT >> 1) << 2;
+	ql->srtt = QUIC_LOSS_INITIAL_RTT;
+	ql->rtt_var = QUIC_LOSS_INITIAL_RTT / 2;
 	ql->rtt_min = 0;
 	ql->pto_count = 0;
 }
@@ -55,8 +55,8 @@ static inline int quic_loss_persistent_congestion(struct quic_loss *ql,
 	if (!period)
 		return 0;
 
-	congestion_period = (ql->srtt >> 3) +
-		QUIC_MAX(ql->rtt_var, QUIC_TIMER_GRANULARITY) + max_ack_delay;
+	congestion_period = ql->srtt +
+		QUIC_MAX(4 * ql->rtt_var, QUIC_TIMER_GRANULARITY) + max_ack_delay;
 	congestion_period *= QUIC_LOSS_PACKET_THRESHOLD;
 
 	return period >= congestion_period;
@@ -67,7 +67,7 @@ static inline unsigned int quic_pto(struct quic_conn *qc)
 {
 	struct quic_loss *ql = &qc->path->loss;
 
-	return (ql->srtt >> 3) + QUIC_MAX(ql->rtt_var, QUIC_TIMER_GRANULARITY) +
+	return ql->srtt + QUIC_MAX(4 * ql->rtt_var, QUIC_TIMER_GRANULARITY) +
 		(HA_ATOMIC_LOAD(&qc->state) >= QUIC_HS_ST_COMPLETE ? qc->max_ack_delay : 0);
 }
 

diff --git a/src/quic_conn.c b/src/quic_conn.c
@@ -549,7 +549,8 @@ static void quic_trace(enum trace_level level, uint64_t mask, const struct trace
 			if (ql)
 				chunk_appendf(&trace_buf,
 				              " srtt=%ums rttvar=%ums min_rtt=%ums",
-				              ql->srtt >> 3, ql->rtt_var >> 2, ql->rtt_min);
+				              ql->srtt, ql->rtt_var, ql->rtt_min);
+
 		}
 		if (mask & QUIC_EV_CONN_CC) {
 			const struct quic_cc_event *ev = a2;

diff --git a/src/quic_loss.c b/src/quic_loss.c
@@ -25,9 +25,8 @@ void quic_loss_srtt_update(struct quic_loss *ql,
 	ql->latest_rtt = rtt;
 	if (!ql->rtt_min) {
 		/* No previous measurement. */
-		ql->srtt = rtt << 3;
-		/* rttval <- rtt / 2 or 4*rttval <- 2*rtt. */
-		ql->rtt_var = rtt << 1;
+		ql->srtt = rtt;
+		ql->rtt_var = rtt / 2;
 		ql->rtt_min = rtt;
 	}
 	else {
@@ -37,13 +36,11 @@ void quic_loss_srtt_update(struct quic_loss *ql,
 		/* Specific to QUIC (RTT adjustment). */
 		if (ack_delay && rtt >= ql->rtt_min + ack_delay)
 			rtt -= ack_delay;
-		diff = (ql->srtt >> 3) - rtt;
+		diff = ql->srtt - rtt;
 		if (diff < 0)
 			diff = -diff;
-		/* 4*rttvar = 3*rttvar + |diff| */
-		ql->rtt_var += diff - (ql->rtt_var >> 2);
-		/* 8*srtt = 7*srtt + rtt */
-		ql->srtt += rtt - (ql->srtt >> 3);
+		ql->rtt_var = (3 * ql->rtt_var + diff) / 4;
+		ql->srtt = (7 * ql->srtt + rtt) / 8;
 	}
 
 	TRACE_DEVEL("Loss info update", QUIC_EV_CONN_RTTUPDT, qc,,, ql);
@@ -90,8 +87,8 @@ struct quic_pktns *quic_pto_pktns(struct quic_conn *qc,
 
 	TRACE_ENTER(QUIC_EV_CONN_SPTO, qc);
 	duration =
-		(ql->srtt >> 3) +
-		(QUIC_MAX(ql->rtt_var, QUIC_TIMER_GRANULARITY) << ql->pto_count);
+		ql->srtt +
+		(QUIC_MAX(4 * ql->rtt_var, QUIC_TIMER_GRANULARITY) << ql->pto_count);
 
 	/* RFC 9002 6.2.2.1. Before Address Validation
 	 *
@@ -165,7 +162,7 @@ void qc_packet_loss_lookup(struct quic_pktns *pktns, struct quic_conn *qc,
 		goto out;
 
 	ql = &qc->path->loss;
-	loss_delay = QUIC_MAX(ql->latest_rtt, ql->srtt >> 3);
+	loss_delay = QUIC_MAX(ql->latest_rtt, ql->srtt);
 	loss_delay = QUIC_MAX(loss_delay, MS_TO_TICKS(QUIC_TIMER_GRANULARITY)) *
 		QUIC_LOSS_TIME_THRESHOLD_MULTIPLICAND / QUIC_LOSS_TIME_THRESHOLD_DIVISOR;