From 714009b7bcf921836c2df7fc0d26b2ad257c8307 Mon Sep 17 00:00:00 2001 From: Amaury Denoyelle Date: Tue, 6 Aug 2024 17:36:56 +0200 Subject: [PATCH] MINOR: quic: implement function to check if STREAM is fully acked When a STREAM frame is retransmitted, a check is performed to remove range of data already acked from it. This is useful when STREAM frames are duplicated and splitted to cover different data ranges. The newly retransmitted frame contains only unacked data. This process is performed similarly in qc_dup_pkt_frms() and qc_build_frms(). Refactor the code into a new function named qc_stream_frm_is_acked(). It returns true if frame data are already fully acked and retransmission can be avoided. If only a partial range of data is acknowledged, frame content is updated to only cover the unacked data. This patch does not have any functional change. However, it simplifies retransmission for STREAM frames. Also, it will be reused to fix retransmission for empty STREAM frames with FIN set from the following patch : BUG/MEDIUM: quic: handle retransmit for standalone FIN STREAM As such, it must be backported prior to it. --- include/haproxy/quic_retransmit.h | 4 +++ src/quic_retransmit.c | 59 +++++++++++++++++++------------ src/quic_rx.c | 26 +------------- src/quic_tx.c | 27 +------------- 4 files changed, 42 insertions(+), 74 deletions(-) diff --git a/include/haproxy/quic_retransmit.h b/include/haproxy/quic_retransmit.h index 403a53c06..1cd404421 100644 --- a/include/haproxy/quic_retransmit.h +++ b/include/haproxy/quic_retransmit.h @@ -10,6 +10,10 @@ #include #include +struct quic_frame; + +int qc_stream_frm_is_acked(struct quic_conn *qc, struct quic_frame *f); + void qc_prep_fast_retrans(struct quic_conn *qc, struct quic_pktns *pktns, struct list *frms1, struct list *frms2); diff --git a/src/quic_retransmit.c b/src/quic_retransmit.c index d06293f5d..0ac57cbd5 100644 --- a/src/quic_retransmit.c +++ b/src/quic_retransmit.c @@ -3,12 +3,46 @@ #include #include #include +#include #include #include #include #define TRACE_SOURCE &trace_quic +/* Check if STREAM frame content has already been acknowledged before + * retransmitting it. If only a subset of content is acknowledged, frame is + * updated to only cover the unacked data. + * + * Returns true if frame content is fully acknowledged, false if partially or + * not at all. + */ +int qc_stream_frm_is_acked(struct quic_conn *qc, struct quic_frame *f) +{ + const struct qf_stream *frm = &f->stream; + const struct qc_stream_desc *s = frm->stream; + + if (!eb64_lookup(&qc->streams_by_id, frm->id)) { + TRACE_DEVEL("STREAM frame already acked : stream released", QUIC_EV_CONN_PRSAFRM, qc, f); + return 1; + } + + if (frm->offset.key + frm->len <= s->ack_offset) { + TRACE_DEVEL("STREAM frame already acked : fully acked range", QUIC_EV_CONN_PRSAFRM, qc, f); + return 1; + } + + if (frm->offset.key < s->ack_offset && + frm->offset.key + frm->len > s->ack_offset) { + /* Data range partially acked, remove it from STREAM frame. */ + const uint64_t diff = s->ack_offset - frm->offset.key; + TRACE_DEVEL("updated partially acked frame", QUIC_EV_CONN_PRSAFRM, qc, f); + qc_stream_frm_mv_fwd(f, diff); + } + + return 0; +} + /* Duplicate all frames from list into list * for QUIC connection. * This is a best effort function which never fails even if no memory could be @@ -33,32 +67,11 @@ static void qc_dup_pkt_frms(struct quic_conn *qc, switch (frm->type) { case QUIC_FT_STREAM_8 ... QUIC_FT_STREAM_F: { - struct qf_stream *strm_frm = &frm->stream; - struct eb64_node *node = NULL; - struct qc_stream_desc *stream_desc; - - node = eb64_lookup(&qc->streams_by_id, strm_frm->id); - if (!node) { - TRACE_DEVEL("ignored frame for a released stream", QUIC_EV_CONN_PRSAFRM, qc, frm); - continue; - } - - stream_desc = eb64_entry(node, struct qc_stream_desc, by_id); /* Do not resend this frame if in the "already acked range" */ - if (strm_frm->offset.key + strm_frm->len <= stream_desc->ack_offset) { - TRACE_DEVEL("ignored frame in already acked range", - QUIC_EV_CONN_PRSAFRM, qc, frm); + if (qc_stream_frm_is_acked(qc, frm)) continue; - } - else if (strm_frm->offset.key < stream_desc->ack_offset) { - uint64_t diff = stream_desc->ack_offset - strm_frm->offset.key; - qc_stream_frm_mv_fwd(frm, diff); - TRACE_DEVEL("updated partially acked frame", - QUIC_EV_CONN_PRSAFRM, qc, frm); - } - - strm_frm->dup = 1; + frm->stream.dup = 1; break; } diff --git a/src/quic_rx.c b/src/quic_rx.c index fd01c8f8b..38faafb23 100644 --- a/src/quic_rx.c +++ b/src/quic_rx.c @@ -412,34 +412,10 @@ int qc_handle_frms_of_lost_pkt(struct quic_conn *qc, switch (frm->type) { case QUIC_FT_STREAM_8 ... QUIC_FT_STREAM_F: { - struct qf_stream *strm_frm = &frm->stream; - struct eb64_node *node = NULL; - struct qc_stream_desc *stream_desc; - - node = eb64_lookup(&qc->streams_by_id, strm_frm->id); - if (!node) { - TRACE_DEVEL("released stream", QUIC_EV_CONN_PRSAFRM, qc, frm); - TRACE_DEVEL("freeing frame from packet", QUIC_EV_CONN_PRSAFRM, - qc, frm, &pn); + if (qc_stream_frm_is_acked(qc, frm)) { qc_frm_free(qc, &frm); continue; } - - stream_desc = eb64_entry(node, struct qc_stream_desc, by_id); - /* Do not resend this frame if in the "already acked range" */ - if (strm_frm->offset.key + strm_frm->len <= stream_desc->ack_offset) { - TRACE_DEVEL("ignored frame in already acked range", - QUIC_EV_CONN_PRSAFRM, qc, frm); - qc_frm_free(qc, &frm); - continue; - } - else if (strm_frm->offset.key < stream_desc->ack_offset) { - uint64_t diff = stream_desc->ack_offset - strm_frm->offset.key; - - qc_stream_frm_mv_fwd(frm, diff); - TRACE_DEVEL("updated partially acked frame", - QUIC_EV_CONN_PRSAFRM, qc, frm); - } break; } diff --git a/src/quic_tx.c b/src/quic_tx.c index 4126d6423..8ad3d515e 100644 --- a/src/quic_tx.c +++ b/src/quic_tx.c @@ -1605,35 +1605,10 @@ static int qc_build_frms(struct list *outlist, struct list *inlist, case QUIC_FT_STREAM_8 ... QUIC_FT_STREAM_F: if (cf->stream.dup) { - struct eb64_node *node = NULL; - struct qc_stream_desc *stream_desc = NULL; - struct qf_stream *strm_frm = &cf->stream; - - /* As this frame has been already lost, ensure the stream is always - * available or the range of this frame is not consumed before - * resending it. - */ - node = eb64_lookup(&qc->streams_by_id, strm_frm->id); - if (!node) { - TRACE_DEVEL("released stream", QUIC_EV_CONN_PRSAFRM, qc, cf); + if (qc_stream_frm_is_acked(qc, cf)) { qc_frm_free(qc, &cf); continue; } - - stream_desc = eb64_entry(node, struct qc_stream_desc, by_id); - if (strm_frm->offset.key + strm_frm->len <= stream_desc->ack_offset) { - TRACE_DEVEL("ignored frame frame in already acked range", - QUIC_EV_CONN_PRSAFRM, qc, cf); - qc_frm_free(qc, &cf); - continue; - } - else if (strm_frm->offset.key < stream_desc->ack_offset) { - uint64_t diff = stream_desc->ack_offset - strm_frm->offset.key; - - qc_stream_frm_mv_fwd(cf, diff); - TRACE_DEVEL("updated partially acked frame", - QUIC_EV_CONN_PRSAFRM, qc, cf); - } } /* Note that these frames are accepted in short packets only without * "Length" packet field. Here, <*len> is used only to compute the