BUG/MINOR: quic: ensure cwnd limits are always enforced

Congestion window is limit by a minimal and maximum values which can
never be exceeded. Min value is hardcoded to 2 datagrams as recommended
by the specification. Max value is specified via haproxy configuration.

These values must be respected each time the congestion window size is
adjusted. However, in some rare occasions, limit were not always
enforced. Fix this by implementing wrappers to set or increment the
congestion window. These functions ensure limits are always applied
after the operation.

Additionnally, wrappers also ensure that if window reached a new maximum
value, it is saved in <cwnd_last_max> field.

This should be backported up to 2.6, after a brief period of
observation.

diff --git a/include/haproxy/quic_cc.h b/include/haproxy/quic_cc.h
index 7c93cfdf1fa4f1a3a4a95b93a34832aed0a7cec9..6569799ff640e4d8606c991384c4e6ee7c005f6c 100644 (file)
--- a/include/haproxy/quic_cc.h
+++ b/include/haproxy/quic_cc.h
@@ -115,7 +115,9 @@ static inline size_t quic_cc_path_prep_data(struct quic_cc_path *path)
        return path->cwnd - path->prep_in_flight;
 }
 
-int quic_cwnd_may_increase(const struct quic_cc_path *path);
+void quic_cc_path_reset(struct quic_cc_path *path);
+void quic_cc_path_set(struct quic_cc_path *path, uint64_t val);
+void quic_cc_path_inc(struct quic_cc_path *path, uint64_t val);
 
 #endif /* USE_QUIC */
 #endif /* _PROTO_QUIC_CC_H */

diff --git a/src/quic_cc.c b/src/quic_cc.c
index d96aa3859f772ad4296b81dde919531bb28cdedf..0d9151ebb89efe80978988cb36c653dba32ce7fc 100644 (file)
--- a/src/quic_cc.c
+++ b/src/quic_cc.c
@@ -58,7 +58,7 @@ uint quic_cc_default_pacing_inter(const struct quic_cc *cc)
 }
 
 /* Returns true if congestion window on path ought to be increased. */
-int quic_cwnd_may_increase(const struct quic_cc_path *path)
+static int quic_cwnd_may_increase(const struct quic_cc_path *path)
 {
        /* RFC 9002 7.8. Underutilizing the Congestion Window
         *
@@ -75,3 +75,32 @@ int quic_cwnd_may_increase(const struct quic_cc_path *path)
         */
        return 2 * path->in_flight >= path->cwnd  || path->cwnd < 16384;
 }
+
+/* Restore congestion window for <path> to its minimal value. */
+void quic_cc_path_reset(struct quic_cc_path *path)
+{
+       path->cwnd = path->limit_min;
+}
+
+/* Set congestion window for <path> to <val>. Min and max limits are enforced. */
+void quic_cc_path_set(struct quic_cc_path *path, uint64_t val)
+{
+       path->cwnd = QUIC_MIN(val, path->limit_max);
+       path->cwnd = QUIC_MAX(path->cwnd, path->limit_min);
+
+       path->cwnd_last_max = QUIC_MAX(path->cwnd, path->cwnd_last_max);
+}
+
+/* Increment congestion window for <path> with <val>. Min and max limits are
+ * enforced. Contrary to quic_cc_path_set(), increase is performed only if a
+ * certain minimal level of the window was already filled.
+ */
+void quic_cc_path_inc(struct quic_cc_path *path, uint64_t val)
+{
+       if (quic_cwnd_may_increase(path)) {
+               path->cwnd = QUIC_MIN(path->cwnd + val, path->limit_max);
+               path->cwnd = QUIC_MAX(path->cwnd, path->limit_min);
+
+               path->cwnd_last_max = QUIC_MAX(path->cwnd, path->cwnd_last_max);
+       }
+}

diff --git a/src/quic_cc_bbr.c b/src/quic_cc_bbr.c
index b5a8ac0c7068eaa3740d5734e3954bc616e77e58..e36b61c97d771171f5325d02e80f398cbadb2c8c 100644 (file)
--- a/src/quic_cc_bbr.c
+++ b/src/quic_cc_bbr.c
@@ -419,7 +419,7 @@ static void bbr_save_cwnd(struct bbr *bbr, struct quic_cc_path *p)
 
 static void bbr_restore_cwnd(struct bbr *bbr, struct quic_cc_path *p)
 {
-       p->cwnd = MAX(p->cwnd, bbr->prior_cwnd);
+       quic_cc_path_set(p, MAX(p->cwnd, bbr->prior_cwnd));
 }
 
 /* <gain> must be provided in percents. */
@@ -560,7 +560,7 @@ static void bbr_set_cwnd(struct bbr *bbr, struct quic_cc_path *p, uint32_t acked
        cwnd = bbr_bound_cwnd_for_probe_rtt(bbr, p, cwnd);
        cwnd = bbr_bound_cwnd_for_model(bbr, p, cwnd);
        /* Limitation by configuration (not in BBR RFC). */
-       p->cwnd = MIN(cwnd, p->limit_max);
+       quic_cc_path_set(p, cwnd);
 }
 
 static int bbr_init(struct quic_cc *cc)
@@ -1316,7 +1316,7 @@ static void bbr_handle_recovery(struct bbr *bbr, struct quic_cc_path *p,
        bbr->round_count_at_recovery =
                bbr->round_start ? bbr->round_count : bbr->round_count + 1;
        bbr_save_cwnd(bbr, p);
-       p->cwnd = p->in_flight + MAX(acked, p->mtu);
+       quic_cc_path_set(p, p->in_flight + MAX(acked, p->mtu));
        p->recovery_start_ts = bbr->recovery_start_ts;
        bbr->recovery_start_ts = TICK_ETERNITY;
 }

diff --git a/src/quic_cc_cubic.c b/src/quic_cc_cubic.c
index 2c8cf777342a277362a59bd57002617269fd0609..df81ab97ff0ce2ae2902d2a765d20c3b665db98e 100644 (file)
--- a/src/quic_cc_cubic.c
+++ b/src/quic_cc_cubic.c
@@ -383,11 +383,7 @@ static inline void quic_cubic_update(struct quic_cc *cc, uint32_t acked)
                        inc = W_est_inc;
        }
 
-       if (quic_cwnd_may_increase(path)) {
-               path->cwnd += inc;
-               path->cwnd = QUIC_MIN(path->limit_max, path->cwnd);
-               path->cwnd_last_max = QUIC_MAX(path->cwnd, path->cwnd_last_max);
-       }
+       quic_cc_path_inc(path, inc);
  leave:
        TRACE_LEAVE(QUIC_EV_CONN_CC, cc->qc);
 }
@@ -434,7 +430,7 @@ static void quic_enter_recovery(struct quic_cc *cc)
        }
 
        c->ssthresh = (CUBIC_BETA_SCALED * path->cwnd) >> CUBIC_SCALE_FACTOR_SHIFT;
-       path->cwnd =  QUIC_MAX(c->ssthresh, (uint32_t)path->limit_min);
+       quic_cc_path_set(path, c->ssthresh);
        c->state = QUIC_CC_ST_RP;
        TRACE_LEAVE(QUIC_EV_CONN_CC, cc->qc, NULL, cc);
 }
@@ -456,10 +452,7 @@ static void quic_cc_cubic_ss_cb(struct quic_cc *cc, struct quic_cc_event *ev)
                        if (path->cwnd >= QUIC_CC_INFINITE_SSTHESH - acked)
                                goto out;
 
-                       if (quic_cwnd_may_increase(path)) {
-                               path->cwnd += acked;
-                               path->cwnd_last_max = QUIC_MAX(path->cwnd, path->cwnd_last_max);
-                       }
+                       quic_cc_path_inc(path, acked);
                        quic_cc_hystart_track_min_rtt(cc, h, path->loss.latest_rtt);
                        if (ev->ack.pn >= h->wnd_end)
                                h->wnd_end = UINT64_MAX;
@@ -470,15 +463,11 @@ static void quic_cc_cubic_ss_cb(struct quic_cc *cc, struct quic_cc_event *ev)
                        }
                }
                else if (path->cwnd < QUIC_CC_INFINITE_SSTHESH - ev->ack.acked) {
-                       if (quic_cwnd_may_increase(path)) {
-                               path->cwnd += ev->ack.acked;
-                               path->cwnd = QUIC_MIN(path->limit_max, path->cwnd);
-                       }
+                       quic_cc_path_inc(path, ev->ack.acked);
                }
                /* Exit to congestion avoidance if slow start threshold is reached. */
                if (path->cwnd >= c->ssthresh)
                        c->state = QUIC_CC_ST_CA;
-               path->cwnd_last_max = QUIC_MAX(path->cwnd, path->cwnd_last_max);
                break;
 
        case QUIC_CC_EVT_LOSS:
@@ -551,10 +540,7 @@ static void quic_cc_cubic_cs_cb(struct quic_cc *cc, struct quic_cc_event *ev)
                if (path->cwnd >= QUIC_CC_INFINITE_SSTHESH - acked)
                        goto out;
 
-               if (quic_cwnd_may_increase(path)) {
-                       path->cwnd += acked;
-                       path->cwnd_last_max = QUIC_MAX(path->cwnd, path->cwnd_last_max);
-               }
+               quic_cc_path_inc(path, acked);
                quic_cc_hystart_track_min_rtt(cc, h, path->loss.latest_rtt);
                if (quic_cc_hystart_may_reenter_ss(h)) {
                        /* Exit to slow start */

diff --git a/src/quic_cc_newreno.c b/src/quic_cc_newreno.c
index 9a52febd86477a370deca57289c7553827c21538..c6a5a360a56e224c7855da1e8f6e165efdb8d5c3 100644 (file)
--- a/src/quic_cc_newreno.c
+++ b/src/quic_cc_newreno.c
@@ -55,7 +55,7 @@ static void quic_cc_nr_slow_start(struct quic_cc *cc)
        struct nr *nr = quic_cc_priv(cc);
 
        path = container_of(cc, struct quic_cc_path, cc);
-       path->cwnd = path->limit_min;
+       quic_cc_path_reset(path);
        /* Re-entering slow start state. */
        nr->state = QUIC_CC_ST_SS;
        /* Recovery start time reset */
@@ -71,7 +71,7 @@ static void quic_cc_nr_enter_recovery(struct quic_cc *cc)
        path = container_of(cc, struct quic_cc_path, cc);
        nr->recovery_start_time = now_ms;
        nr->ssthresh = path->cwnd >> 1;
-       path->cwnd = QUIC_MAX(nr->ssthresh, (uint32_t)path->limit_min);
+       quic_cc_path_set(path, nr->ssthresh);
        nr->state = QUIC_CC_ST_RP;
 }
 
@@ -86,11 +86,7 @@ static void quic_cc_nr_ss_cb(struct quic_cc *cc, struct quic_cc_event *ev)
        path = container_of(cc, struct quic_cc_path, cc);
        switch (ev->type) {
        case QUIC_CC_EVT_ACK:
-               if (quic_cwnd_may_increase(path)) {
-                       path->cwnd += ev->ack.acked;
-                       path->cwnd = QUIC_MIN(path->limit_max, path->cwnd);
-                       path->cwnd_last_max = QUIC_MAX(path->cwnd, path->cwnd_last_max);
-               }
+               quic_cc_path_inc(path, ev->ack.acked);
                /* Exit to congestion avoidance if slow start threshold is reached. */
                if (path->cwnd > nr->ssthresh)
                        nr->state = QUIC_CC_ST_CA;
@@ -126,11 +122,7 @@ static void quic_cc_nr_ca_cb(struct quic_cc *cc, struct quic_cc_event *ev)
                 */
                acked = ev->ack.acked * path->mtu + nr->remain_acked;
                nr->remain_acked = acked % path->cwnd;
-               if (quic_cwnd_may_increase(path)) {
-                       path->cwnd += acked / path->cwnd;
-                       path->cwnd = QUIC_MIN(path->limit_max, path->cwnd);
-                       path->cwnd_last_max = QUIC_MAX(path->cwnd, path->cwnd_last_max);
-               }
+               quic_cc_path_inc(path, acked / path->cwnd);
                break;
        }
 
@@ -170,7 +162,7 @@ static void quic_cc_nr_rp_cb(struct quic_cc *cc, struct quic_cc_event *ev)
 
                nr->state = QUIC_CC_ST_CA;
                nr->recovery_start_time = TICK_ETERNITY;
-               path->cwnd = nr->ssthresh;
+               quic_cc_path_set(path, nr->ssthresh);
                break;
        case QUIC_CC_EVT_LOSS:
                /* Do nothing */