From 906b0589546b700b532472ede019e5c5a8ac1f38 Mon Sep 17 00:00:00 2001 From: Amaury Denoyelle Date: Fri, 5 Aug 2022 15:22:28 +0200 Subject: [PATCH] MINOR: quic: explicitely ignore sendto error qc_snd_buf() returns an error if sendto has failed. On standard conditions, we should check for EAGAIN/EWOULDBLOCK errno and if so, register the file-descriptor in the poller to retry the operation later. However, quic_conn uses directly the listener fd which is shared for all QUIC connections of this listener on several threads. Thus, it's complicated to implement fd supversion via the poller : there is no mechanism to easily wakeup quic_conn or MUX after a sendto failure. A quick and simple solution for the moment is to considered a datagram as properly emitted even on sendto error. In the end, this will trigger the quic_conn retransmission timer as data will be considered lost on the network and the send operation will be retried. This solution will be replaced when fd management for quic_conn is reworked. In fact, this quick hack was already in use in the current code, albeit not voluntarily. This is due to a bug caused by an API mismatch on the return type of qc_snd_buf() which never emits a negative error code despite its documentation. Thus, all its invocation were considered as a success. If this bug was fixed, the sending would would have been interrupted by a break which could cause the transfer to freeze. qc_snd_buf() invocation is clean up : the break statement is removed. Send operation is now always explicitely conducted entirely even on error and buffer data is purged. A simple optimization has been added to skip over sendto when looping over several datagrams at the first sendto error. However, to properly function, it requires a fix on the return type of qc_snd_buf() which is provided in another patch. As the behavior before and after this patch seems identical, it is not labelled as a BUG. However, it should be backported for cleaning purpose. It may also have an impact on github issue #1808. --- src/xprt_quic.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/src/xprt_quic.c b/src/xprt_quic.c index e754d9e92..367a9a533 100644 --- a/src/xprt_quic.c +++ b/src/xprt_quic.c @@ -3030,6 +3030,7 @@ int qc_send_ppkts(struct qring *qr, struct ssl_sock_ctx *ctx) { struct quic_conn *qc; struct cbuf *cbuf; + char skip_sendto = 0; qc = ctx->qc; cbuf = qr->cbuf; @@ -3063,9 +3064,23 @@ int qc_send_ppkts(struct qring *qr, struct ssl_sock_ctx *ctx) tmpbuf.area = (char *)pos; tmpbuf.size = tmpbuf.data = dglen; + /* If sendto is on error just skip the call to it for the rest + * of the loop but continue to purge the buffer. Data will be + * transmitted when QUIC packets are detected as lost on our + * side. + * + * TODO use fd-monitoring to detect when send operation can be + * retry. This should improve the bandwidth without relying on + * retransmission timer. However, it requires a major rework on + * quic-conn fd management. + */ TRACE_PROTO("to send", QUIC_EV_CONN_SPPKTS, qc); - if(qc_snd_buf(qc, &tmpbuf, tmpbuf.data, 0) <= 0) - break; + if (!skip_sendto) { + if (qc_snd_buf(qc, &tmpbuf, tmpbuf.data, 0)) { + skip_sendto = 1; + TRACE_PROTO("sendto error, simulate sending for the rest of data", QUIC_EV_CONN_SPPKTS, qc); + } + } cb_del(cbuf, dglen + headlen); qc->tx.bytes += tmpbuf.data;