MEDIUM: quic: Send ACK frames asap

Due to a erroneous interpretation of the RFC 9000 (quic-transport), ACKs frames
were always sent only after having received two ack-eliciting packets.
This could trigger useless retransmissions for tail packets on the peer side.
For now on, we send as soon as possible ACK frames as soon as we have ACK to send,
in the same packets as the ack-eliciting frame packets, and we also send ACK
frames after having received 2 ack-eliciting packets since the last time we sent
an ACK frame with other ack-eliciting frames.

diff --git a/include/haproxy/xprt_quic-t.h b/include/haproxy/xprt_quic-t.h
index 2c4827aba871b03c86e28cc9d2bc64bbe755e58c..9cb163ae9ccdae512670fd4ba7303e7ba87f4858 100644 (file)
--- a/include/haproxy/xprt_quic-t.h
+++ b/include/haproxy/xprt_quic-t.h
@@ -420,6 +420,7 @@ struct quic_pktns {
                /* Largest acked sent packet. */
                int64_t largest_acked_pn;
                struct quic_arngs arngs;
+               unsigned int nb_aepkts_since_last_ack;
        } rx;
        unsigned int flags;
 };
@@ -443,6 +444,10 @@ struct quic_dgram {
 /* Default QUIC connection transport parameters */
 extern struct quic_transport_params quic_dflt_transport_params;
 
+/* Maximum number of ack-eliciting received packets since the last
+ * ACK frame was sent
+ */
+#define QUIC_MAX_RX_AEPKTS_SINCE_LAST_ACK       2
 /* Flag a received packet as being an ack-eliciting packet. */
 #define QUIC_FL_RX_PACKET_ACK_ELICITING (1UL << 0)
 
@@ -508,6 +513,8 @@ struct quic_rx_strm_frm {
 #define QUIC_FL_TX_PACKET_PADDING       (1UL << 1)
 /* Flag a sent packet as being in flight. */
 #define QUIC_FL_TX_PACKET_IN_FLIGHT     (QUIC_FL_TX_PACKET_ACK_ELICITING | QUIC_FL_TX_PACKET_PADDING)
+/* Flag a sent packet as containg an ACK frame */
+#define QUIC_FL_TX_PACKET_ACK           (1UL << 2)
 
 /* Structure to store enough information about TX QUIC packets. */
 struct quic_tx_packet {
@@ -716,8 +723,6 @@ struct quic_conn {
        struct {
                /* Number of received bytes. */
                uint64_t bytes;
-               /* Number of ack-eliciting received packets. */
-               size_t nb_ack_eliciting;
                /* Transport parameters the peer will receive */
                struct quic_transport_params params;
                /* RX buffer */

diff --git a/include/haproxy/xprt_quic.h b/include/haproxy/xprt_quic.h
index 567cbefe90fe49cf9b1b289db8bfd9c32b90f75f..09c19ce386ffa92e44d83d3048c37bbd626da673 100644 (file)
--- a/include/haproxy/xprt_quic.h
+++ b/include/haproxy/xprt_quic.h
@@ -987,6 +987,7 @@ static inline void quic_pktns_init(struct quic_pktns *pktns)
        pktns->rx.arngs.root = EB_ROOT_UNIQUE;
        pktns->rx.arngs.sz = 0;
        pktns->rx.arngs.enc_sz = 0;
+       pktns->rx.nb_aepkts_since_last_ack = 0;
 
        pktns->flags = 0;
 }

diff --git a/src/xprt_quic.c b/src/xprt_quic.c
index 8c70e6570e51775da8df99efa1b72d7cd017226b..bac4ab36923c1cc229f49e08620ca5eeb2ee6284 100644 (file)
--- a/src/xprt_quic.c
+++ b/src/xprt_quic.c
@@ -174,7 +174,7 @@ DECLARE_STATIC_POOL(pool_head_quic_conn_stream, "qc_stream_desc", sizeof(struct
 static struct quic_tx_packet *qc_build_pkt(unsigned char **pos, const unsigned char *buf_end,
                                            struct quic_enc_level *qel, struct list *frms,
                                            struct quic_conn *qc, size_t dglen, int pkt_type,
-                                           int padding, int ack, int probe, int cc, int *err);
+                                           int padding, int probe, int cc, int *err);
 static struct task *quic_conn_app_io_cb(struct task *t, void *context, unsigned int state);
 static void qc_idle_timer_rearm(struct quic_conn *qc, int read);
 
@@ -2636,6 +2636,33 @@ static inline void qc_set_dg(struct cbuf *cbuf,
        cb_add(cbuf, dglen + sizeof dglen + sizeof pkt);
 }
 
+/* Returns 1 if a packet may be built for <qc> from <qel> encryption level
+ * with <frms> as ack-eliciting frame list to send, 0 if not.
+ * <cc> must equal to 1 if an immediate close was asked, 0 if not.
+ * <probe> must equalt to 1 if a probing packet is required, 0 if not.
+ */
+static int qc_may_build_pkt(struct quic_conn *qc, struct list *frms,
+                            struct quic_enc_level *qel, int cc, int probe)
+{
+       unsigned int must_ack =
+               qel->pktns->rx.nb_aepkts_since_last_ack >= QUIC_MAX_RX_AEPKTS_SINCE_LAST_ACK;
+
+       /* Do not build any more packet if the TX secrets are not available or
+        * if there is nothing to send, i.e. if no CONNECTION_CLOSE or ACK are required
+        * and if there is no more packets to send upon PTO expiration
+        * and if there is no more ack-eliciting frames to send or in flight
+        * congestion control limit is reached for prepared data
+        */
+       if (!(qel->tls_ctx.flags & QUIC_FL_TLS_SECRETS_SET) ||
+           (!cc && !probe && !must_ack &&
+            (LIST_ISEMPTY(frms) || qc->path->prep_in_flight >= qc->path->cwnd))) {
+               TRACE_DEVEL("nothing more to do", QUIC_EV_CONN_PHPKTS, qc);
+               return 0;
+       }
+
+       return 1;
+}
+
 /* Prepare as much as possible short packets which are also datagrams into <qr>
  * ring buffer for the QUIC connection with <ctx> as I/O handler context from
  * <frms> list of prebuilt frames.
@@ -2670,28 +2697,17 @@ static int qc_prep_app_pkts(struct quic_conn *qc, struct qring *qr,
         */
        end_buf = pos + cb_contig_space(cbuf) - sizeof(uint16_t);
        while (end_buf - pos >= (int)qc->path->mtu + dg_headlen) {
-               int err, probe, ack, cc;
+               int err, probe, cc;
 
                TRACE_POINT(QUIC_EV_CONN_PHPKTS, qc, qel);
-               probe = ack = 0;
+               probe = 0;
                cc =  qc->flags & QUIC_FL_CONN_IMMEDIATE_CLOSE;
-               if (!cc) {
+               /* We do not probe if an immediate close was asked */
+               if (!cc)
                        probe = qel->pktns->tx.pto_probe;
-                       ack = qel->pktns->flags & QUIC_FL_PKTNS_ACK_REQUIRED;
-               }
-               /* Do not build any more packet if the TX secrets are not available or
-                * if there is nothing to send, i.e. if no CONNECTION_CLOSE or ACK are required
-                * and if there is no more packets to send upon PTO expiration
-                * and if there is no more CRYPTO data available or in flight
-                * congestion control limit is reached for prepared data
-                */
-               if (!(qel->tls_ctx.flags & QUIC_FL_TLS_SECRETS_SET) ||
-                   (!cc && !ack && !probe &&
-                    (LIST_ISEMPTY(frms) ||
-                     qc->path->prep_in_flight >= qc->path->cwnd))) {
-                       TRACE_DEVEL("nothing more to do", QUIC_EV_CONN_PHPKTS, qc);
+
+               if (!qc_may_build_pkt(qc, frms, qel, cc, probe))
                        break;
-               }
 
                /* Leave room for the datagram header */
                pos += dg_headlen;
@@ -2703,7 +2719,7 @@ static int qc_prep_app_pkts(struct quic_conn *qc, struct qring *qr,
                }
 
                pkt = qc_build_pkt(&pos, end, qel, frms, qc, 0, 0,
-                                  QUIC_PACKET_TYPE_SHORT, ack, probe, cc, &err);
+                                  QUIC_PACKET_TYPE_SHORT, probe, cc, &err);
                switch (err) {
                case -2:
                        goto err;
@@ -2783,30 +2799,17 @@ static int qc_prep_pkts(struct quic_conn *qc, struct qring *qr,
        end_buf = pos + cb_contig_space(cbuf) - sizeof dglen;
        first_pkt = prv_pkt = NULL;
        while (end_buf - pos >= (int)qc->path->mtu + dg_headlen || prv_pkt) {
-               int err, probe, ack, cc;
+               int err, probe, cc;
                enum quic_pkt_type pkt_type;
 
                TRACE_POINT(QUIC_EV_CONN_PHPKTS, qc, qel);
-               probe = ack = 0;
+               probe = 0;
                cc =  qc->flags & QUIC_FL_CONN_IMMEDIATE_CLOSE;
-               if (!cc) {
+               /* We do not probe if an immediate close was asked */
+               if (!cc)
                        probe = qel->pktns->tx.pto_probe;
-                       ack = qel->pktns->flags & QUIC_FL_PKTNS_ACK_REQUIRED;
-               }
-               /* Do not build any more packet if the TX secrets are not available or
-                * if there is nothing to send, i.e. if no CONNECTION_CLOSE or ACK are required
-                * and if there is no more packets to send upon PTO expiration
-                * and if there is no more CRYPTO data available or in flight
-                * congestion control limit is reached for prepared data
-                */
-               if (!(qel->tls_ctx.flags & QUIC_FL_TLS_SECRETS_SET) ||
-                   (!cc && !ack && !probe &&
-                    (LIST_ISEMPTY(&qel->pktns->tx.frms) ||
-                     qc->path->prep_in_flight >= qc->path->cwnd))) {
-                       TRACE_DEVEL("nothing more to do", QUIC_EV_CONN_PHPKTS, qc);
-                       /* Set the current datagram as prepared into <cbuf> if
-                        * the was already a correct packet which was previously written.
-                        */
+
+               if (!qc_may_build_pkt(qc, &qel->pktns->tx.frms, qel, cc, probe)) {
                        if (prv_pkt)
                                qc_set_dg(cbuf, dglen, first_pkt);
                        /* Let's select the next encryption level */
@@ -2833,7 +2836,7 @@ static int qc_prep_pkts(struct quic_conn *qc, struct qring *qr,
                }
 
                cur_pkt = qc_build_pkt(&pos, end, qel, &qel->pktns->tx.frms,
-                                      qc, dglen, padding, pkt_type, ack, probe, cc, &err);
+                                      qc, dglen, padding, pkt_type, probe, cc, &err);
                switch (err) {
                case -2:
                        goto err;
@@ -3382,9 +3385,9 @@ int qc_treat_rx_pkts(struct quic_enc_level *cur_el, struct quic_enc_level *next_
                        else {
                                struct quic_arng ar = { .first = pkt->pn, .last = pkt->pn };
 
-                               if (pkt->flags & QUIC_FL_RX_PACKET_ACK_ELICITING) {
-                                       if (!(++qc->rx.nb_ack_eliciting & 1) || force_ack)
-                                               qel->pktns->flags |= QUIC_FL_PKTNS_ACK_REQUIRED;
+                               if (pkt->flags & QUIC_FL_RX_PACKET_ACK_ELICITING || force_ack) {
+                                       qel->pktns->flags |= QUIC_FL_PKTNS_ACK_REQUIRED;
+                                       qel->pktns->rx.nb_aepkts_since_last_ack++;
                                        qc_idle_timer_rearm(qc, 1);
                                }
                                if (pkt->pn > largest_pn)
@@ -3920,7 +3923,6 @@ static struct quic_conn *qc_new_conn(unsigned int version, int ipv4,
        qc->tx.bytes = 0;
        /* RX part. */
        qc->rx.bytes = 0;
-       qc->rx.nb_ack_eliciting = 0;
        qc->rx.buf = b_make(buf_area, QUIC_CONN_RX_BUFSZ, 0, 0);
        LIST_INIT(&qc->rx.pkt_list);
        if (!quic_tls_ku_init(qc)) {
@@ -5179,7 +5181,7 @@ static inline int qc_build_frms(struct list *outlist, struct list *inlist,
 static int qc_do_build_pkt(unsigned char *pos, const unsigned char *end,
                            size_t dglen, struct quic_tx_packet *pkt,
                            int64_t pn, size_t *pn_len, unsigned char **buf_pn,
-                           int ack, int padding, int cc, int probe,
+                           int padding, int cc, int probe,
                            struct quic_enc_level *qel, struct quic_conn *qc,
                            struct list *frms)
 {
@@ -5197,14 +5199,11 @@ static int qc_do_build_pkt(unsigned char *pos, const unsigned char *end,
        /* Length field value with CRYPTO frames if present. */
        len_frms = 0;
        beg = pos;
-       /* When not probing and not acking, and no immediate close is required,
-        * reduce the size of this buffer to respect
-        * the congestion controller window. So, we do not limit the size of this
-        * packet if we have an ACK frame to send because an ACK frame is not
-        * ack-eliciting. This size will be limited if we have ack-eliciting
-        * frames to send from <frms>.
+       /* When not probing, and no immediate close is required, reduce the size of this
+        * buffer to respect the congestion controller window.
+        * This size will be limited if we have ack-eliciting frames to send from <frms>.
         */
-       if (!probe && !ack && !cc) {
+       if (!probe && !LIST_ISEMPTY(frms) && !cc) {
                size_t path_room;
 
                path_room = quic_path_prep_data(qc->path);
@@ -5236,7 +5235,8 @@ static int qc_do_build_pkt(unsigned char *pos, const unsigned char *end,
        head_len = pos - beg;
        /* Build an ACK frame if required. */
        ack_frm_len = 0;
-       if (!cc && ack && !eb_is_empty(&qel->pktns->rx.arngs.root)) {
+       if ((qel->pktns->flags & QUIC_FL_PKTNS_ACK_REQUIRED)) {
+           BUG_ON(eb_is_empty(&qel->pktns->rx.arngs.root));
                ack_frm.tx_ack.ack_delay = 0;
                ack_frm.tx_ack.arngs = &qel->pktns->rx.arngs;
                /* XXX BE CAREFUL XXX : here we reserved at least one byte for the
@@ -5326,6 +5326,7 @@ static int qc_do_build_pkt(unsigned char *pos, const unsigned char *end,
                        goto no_room;
 
                pkt->largest_acked_pn = quic_pktns_get_largest_acked_pn(qel->pktns);
+               pkt->flags |= QUIC_FL_TX_PACKET_ACK;
        }
 
        /* Ack-eliciting frames */
@@ -5421,7 +5422,7 @@ static struct quic_tx_packet *qc_build_pkt(unsigned char **pos,
                                            const unsigned char *buf_end,
                                            struct quic_enc_level *qel, struct list *frms,
                                            struct quic_conn *qc, size_t dglen, int padding,
-                                           int pkt_type, int ack, int probe, int cc, int *err)
+                                           int pkt_type, int probe, int cc, int *err)
 {
        /* The pointer to the packet number field. */
        unsigned char *buf_pn;
@@ -5447,7 +5448,7 @@ static struct quic_tx_packet *qc_build_pkt(unsigned char **pos,
 
        pn = qel->pktns->tx.next_pn + 1;
        if (!qc_do_build_pkt(*pos, buf_end, dglen, pkt, pn, &pn_len, &buf_pn,
-                            ack, padding, cc, probe, qel, qc, frms)) {
+                            padding, cc, probe, qel, qc, frms)) {
                *err = -1;
                goto err;
        }
@@ -5486,8 +5487,10 @@ static struct quic_tx_packet *qc_build_pkt(unsigned char **pos,
                pkt->in_flight_len = pkt->len;
                qc->path->prep_in_flight += pkt->len;
        }
-       /* Always reset these flags */
-       qel->pktns->flags &= ~QUIC_FL_PKTNS_ACK_REQUIRED;
+       if (pkt->flags & QUIC_FL_TX_PACKET_ACK) {
+               qel->pktns->flags &= ~QUIC_FL_PKTNS_ACK_REQUIRED;
+               qel->pktns->rx.nb_aepkts_since_last_ack = 0;
+       }
 
        pkt->pktns = qel->pktns;
        TRACE_LEAVE(QUIC_EV_CONN_HPKT, qc, pkt);