From: Amaury Denoyelle <adenoyelle@haproxy.com>
Date: Fri, 22 Nov 2024 14:43:16 +0000 (+0100)
Subject: BUG/MEDIUM: quic: fix sending performance due to qc_prep_pkts() return
X-Git-Tag: v3.1.0~28
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=044452546eaad056187070471efef87452da79d5;p=thirdparty%2Fhaproxy.git

BUG/MEDIUM: quic: fix sending performance due to qc_prep_pkts() return

qc_prep_pkts() is a QUIC transport level function which encodes one or
several datagrams in a buffer before sending them. It returns the number
of encoded datagram. This is especially important when pacing is used to
limit packet bursts.

This datagram accounting was not trivial as qc_prep_pkts() used several
code paths depending on the condition of the current encoded packet.
Thus, there were several places were the local variable dgram_cnt could
have been incremented. This was implemented by the following commit :

  commit 5cb8f8a6224db96f4386277c41ddae4a29a4130d
  MINOR: quic: support a max number of built packet per send iteration

However, there is a bug due to a missing increment when all frames from
the current QEL have been encoded. In this case, the encoding continue
in the same datagram to coalesce a futur packet. However, if this is the
last QEL, encoding loop will then break. As first_pkt is not NULL,
qc_txb_store() is called outside but dgram_cnt is yet not incremented.

In particular, this causes qc_prep_pkts() to return 0 when there is only
small STREAM frames to emit for application QEL. In qc_send(), this is
interpreted as a value which prevents further emission for the current
invokation. Thus, it may hurts performance, both without and with
pacing.

To fix this, removing multiple dgram_cnt increment. Now, it is modified
only in a single place which should cover every case, and render the
code easier to validate.

The most notable case where the bug is visible is when using cubic with
pacing without any burst, with quic-cc-algo cubic(,1). First, transfer
bandwidth in average was suboptimal, with significant variation. Worst,
it could sometimes fall dramatically for a particular stream without
recovering before returning to an expected level on the next one.

No need to backport.
---

diff --git a/src/quic_tx.c b/src/quic_tx.c
index 536350eec4..37f46fe731 100644
--- a/src/quic_tx.c
+++ b/src/quic_tx.c
@@ -696,7 +696,6 @@ static int qc_prep_pkts(struct quic_conn *qc, struct buffer *buf,
 					if (first_pkt && (first_pkt->type != QUIC_PACKET_TYPE_INITIAL ||
 					                  wrlen >= QUIC_INITIAL_PACKET_MINLEN)) {
 						qc_txb_store(buf, wrlen, first_pkt);
-						++dgram_cnt;
 					}
 					TRACE_PROTO("could not prepare anymore packet", QUIC_EV_CONN_PHPKTS, qc, qel);
 					break;
@@ -733,6 +732,12 @@ static int qc_prep_pkts(struct quic_conn *qc, struct buffer *buf,
 					cur_pkt->flags |= QUIC_FL_TX_PACKET_COALESCED;
 			}
 
+			/* If <dglen> is NULL at this stage, it means the built
+			 * packet is the first of a new datagram.
+			 */
+			if (!dglen)
+				++dgram_cnt;
+
 			total += cur_pkt->len;
 			dglen += cur_pkt->len;
 			wrlen += cur_pkt->len;
@@ -766,8 +771,6 @@ static int qc_prep_pkts(struct quic_conn *qc, struct buffer *buf,
 				prv_pkt = cur_pkt;
 				dglen = 0;
 
-				++dgram_cnt;
-
 				/* man 7 udp UDP_SEGMENT
 				 * The segment size must be chosen such that at
 				 * most 64 datagrams are sent in a single call
@@ -781,7 +784,6 @@ static int qc_prep_pkts(struct quic_conn *qc, struct buffer *buf,
 				wrlen = dglen = 0;
 				padding = 0;
 				prv_pkt = NULL;
-				++dgram_cnt;
 				gso_dgram_cnt = 0;
 			}