Skip to content

Commit 602dbe5

Browse files
committed
BUG/MEDIUM: quic: correct pacing perf by fixing 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 ensure 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 5cb8f8a 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. The most notable case where it is visible is when using cubic with pacing without any burst with quic-cc-algo cubic(,1). In this case, transfert bandwidth could sometimes fall dramatically for a particular stream before returning to the normal on the next one. No need to backport.
1 parent 2ffea50 commit 602dbe5

File tree

1 file changed

+2
-4
lines changed

1 file changed

+2
-4
lines changed

src/quic_tx.c

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -644,6 +644,8 @@ static int qc_prep_pkts(struct quic_conn *qc, struct buffer *buf,
644644
/* Ensure end does not go beyond buffer */
645645
if (end > (unsigned char *)b_wrap(buf))
646646
end = (unsigned char *)b_wrap(buf);
647+
648+
++dgram_cnt;
647649
}
648650

649651
/* RFC 9000 14.1 Initial datagram size
@@ -696,7 +698,6 @@ static int qc_prep_pkts(struct quic_conn *qc, struct buffer *buf,
696698
if (first_pkt && (first_pkt->type != QUIC_PACKET_TYPE_INITIAL ||
697699
wrlen >= QUIC_INITIAL_PACKET_MINLEN)) {
698700
qc_txb_store(buf, wrlen, first_pkt);
699-
++dgram_cnt;
700701
}
701702
TRACE_PROTO("could not prepare anymore packet", QUIC_EV_CONN_PHPKTS, qc, qel);
702703
break;
@@ -766,8 +767,6 @@ static int qc_prep_pkts(struct quic_conn *qc, struct buffer *buf,
766767
prv_pkt = cur_pkt;
767768
dglen = 0;
768769

769-
++dgram_cnt;
770-
771770
/* man 7 udp UDP_SEGMENT
772771
* The segment size must be chosen such that at
773772
* most 64 datagrams are sent in a single call
@@ -781,7 +780,6 @@ static int qc_prep_pkts(struct quic_conn *qc, struct buffer *buf,
781780
wrlen = dglen = 0;
782781
padding = 0;
783782
prv_pkt = NULL;
784-
++dgram_cnt;
785783
gso_dgram_cnt = 0;
786784
}
787785

0 commit comments

Comments
 (0)