Skip to content

Commit

Permalink
MEDIUM: mux-quic: do not account release buf in alloc window
Browse files Browse the repository at this point in the history
Since a recent rework, a new tx buffer can only be allocated if the size
of all allocated buffers in used does not exceed the underlying
connection congestion window.

This model caused a severe throughput degradation when a single stream
is used and the congestion window is smaller than bufsize, due a poor
network conditions. In this case, the stream can allocate a single
buffer only. Once it is full, stream emission is interrupted until every
ACKs are received to free the buffer. This is an extremely unefficient
transfer model. Such issue can be reproduced using the following ngtcp2
with 10% rate loss :

ngtcp2-client -q --no-quic-dump --no-http-dump --exit-on-all-streams-close \
  -r 0.1 127.0.0.1 20443 "https://[::]:20443/?s=100m"

To solve this, buffer allocation limit must be relaxed in part. This is
the purpose of this two-part commits serie. This first patch updates
QUIC MUX buffer accounting : as soon as a Tx buffer is fully sent and
released, it is removed from connection buffer window.

This single change fixes completely the degraded case described above.
However, it is not sufficient as it virtually removes any limit on
stream buffer allocation. Indeed, each time a new buffer is released, a
QCS can reallocate a new buffer, which could increase heavily memory
consumption. As such, another limitation must be implemented to prevent
this. This will be the purpose of the next commit.
  • Loading branch information
a-denoyelle authored and haproxyFred committed Sep 2, 2024
1 parent db13df3 commit 5eb97fa
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 23 deletions.
6 changes: 5 additions & 1 deletion src/mux_quic.c
Original file line number Diff line number Diff line change
Expand Up @@ -1158,6 +1158,7 @@ int qcc_realign_stream_txbuf(const struct qcs *qcs, struct buffer *out)
*/
int qcc_release_stream_txbuf(struct qcs *qcs)
{
struct buffer *buf = qc_stream_buf_get(qcs->stream);
const uint64_t bytes = qcs_prep_bytes(qcs);

/* Cannot release buffer if prepared data is not fully sent. */
Expand All @@ -1166,7 +1167,10 @@ int qcc_release_stream_txbuf(struct qcs *qcs)
return 1;
}

/* Free released buf size from buf window. */
qc_stream_buf_release(qcs->stream);
qcc_notify_buf(qcs->qcc, b_size(buf));

return 0;
}

Expand Down Expand Up @@ -1975,7 +1979,7 @@ void qcc_streams_sent_done(struct qcs *qcs, uint64_t data, uint64_t offset)
/* Release buffer if everything sent and buf is full or stream is waiting for room. */
if (!qcs_prep_bytes(qcs) &&
(b_full(&qcs->stream->buf->buf) || qcs->flags & QC_SF_BLK_MROOM)) {
qc_stream_buf_release(qcs->stream);
qcc_release_stream_txbuf(qcs);
qcs->flags &= ~QC_SF_BLK_MROOM;
qcs_notify_send(qcs);
}
Expand Down
36 changes: 14 additions & 22 deletions src/quic_stream.c
Original file line number Diff line number Diff line change
Expand Up @@ -30,15 +30,22 @@ static void qc_stream_buf_free(struct qc_stream_desc *stream,
{
struct quic_conn *qc = stream->qc;
struct buffer *buf = &(*stream_buf)->buf;
uint64_t free_size;

LIST_DEL_INIT(&(*stream_buf)->list);

/* Reset current buf ptr if deleted instance is the same one. */
if (*stream_buf == stream->buf)
if (*stream_buf == stream->buf) {
stream->buf = NULL;

free_size = b_size(buf);
/* Only non-released buffer are notified to MUX layer. */
if (qc->mux_state == QC_MUX_READY) {
if (!(stream->flags & QC_SD_FL_OOB_BUF)) {
/* notify MUX about available buffers. */
qcc_notify_buf(qc->qcc, b_size(buf));
}
}
}

if ((*stream_buf)->sbuf) {
pool_free(pool_head_sbuf, buf->area);
}
Expand All @@ -48,14 +55,6 @@ static void qc_stream_buf_free(struct qc_stream_desc *stream,
}
pool_free(pool_head_quic_stream_buf, *stream_buf);
*stream_buf = NULL;

/* notify MUX about available buffers. */
if (qc->mux_state == QC_MUX_READY) {
if (!(stream->flags & QC_SD_FL_OOB_BUF)) {
/* notify MUX about available buffers. */
qcc_notify_buf(qc->qcc, free_size);
}
}
}

/* Allocate a new stream descriptor with id <id>. The caller is responsible to
Expand Down Expand Up @@ -124,6 +123,9 @@ void qc_stream_desc_release(struct qc_stream_desc *stream,
/* final_size cannot be greater than all currently stored data. */
BUG_ON(final_size > tail_offset);

/* release buffer and notify MUX. */
qcc_notify_buf(stream->qc->qcc, b_size(buf));

/* Remove unsent data from current buffer. */
if (final_size < tail_offset)
b_sub(buf, tail_offset - final_size);
Expand Down Expand Up @@ -209,15 +211,13 @@ void qc_stream_desc_free(struct qc_stream_desc *stream, int closing)
struct quic_conn *qc = stream->qc;
struct eb64_node *frm_node;
unsigned int free_count = 0;
uint64_t free_size = 0;

/* This function only deals with released streams. */
BUG_ON(!(stream->flags & QC_SD_FL_RELEASE));

/* free remaining stream buffers */
list_for_each_entry_safe(buf, buf_back, &stream->buf_list, list) {
if (!(b_data(&buf->buf)) || closing) {
free_size += b_size(&buf->buf);
if (buf->sbuf)
pool_free(pool_head_sbuf, buf->buf.area);
else
Expand All @@ -228,17 +228,9 @@ void qc_stream_desc_free(struct qc_stream_desc *stream, int closing)
}
}

if (free_count) {
if (free_count)
offer_buffers(NULL, free_count);

if (qc->mux_state == QC_MUX_READY) {
if (!(stream->flags & QC_SD_FL_OOB_BUF)) {
/* notify MUX about available buffers. */
qcc_notify_buf(qc->qcc, free_size);
}
}
}

/* qc_stream_desc might be freed before having received all its ACKs.
* This is the case if some frames were retransmitted.
*/
Expand Down

0 comments on commit 5eb97fa

Please sign in to comment.