From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: passt.top; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=eiMg/uFa; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTPS id 55DF65A0262 for ; Wed, 20 May 2026 22:28:57 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1779308936; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=wBZxuusXyIiz/mCvNtn0Immh9EiG/g5ZHVAnRW1FKvY=; b=eiMg/uFar2iv7ewmtsKiyJENLQ9FvwfWCR+XLwGf5+7eIL4bPurOmNJyYhEUrt5HrmEmeM JncrZYU9wxr3DaUF+7CgPwnb7uitepFoPhnALHX4TosEVMUY3EkBhJzJma7kp4cGR+mlNI mFPaKF89Gwsp5qB5YeH3oyFzffaIBSI= Received: from mail-wr1-f69.google.com (mail-wr1-f69.google.com [209.85.221.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-416-PkZ0FPHaMcmyzfHMem3sbw-1; Wed, 20 May 2026 16:28:55 -0400 X-MC-Unique: PkZ0FPHaMcmyzfHMem3sbw-1 X-Mimecast-MFC-AGG-ID: PkZ0FPHaMcmyzfHMem3sbw_1779308934 Received: by mail-wr1-f69.google.com with SMTP id ffacd0b85a97d-449b2a183d3so3484510f8f.0 for ; Wed, 20 May 2026 13:28:54 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1779308934; x=1779913734; h=date:content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=wBZxuusXyIiz/mCvNtn0Immh9EiG/g5ZHVAnRW1FKvY=; b=YZIK7ZZCQejGy+IsDZFnWABfCIky0LKG/fgcXTmGFHSi7Ol3q5Bj51wJPRiJwbLNYY APLgEWFsSCKwma72NUL85022GsXVLr9WT+yzTswuBygjIo8aFOFXJsbc7WlVOvy+FBX1 TwdLsh2uNgli4NKGwedQPKQ60kmEjRaRysFZNrUDYicc08ceUZwWHRNZ9u0eXqpTZqlX dJK5BtrpmkAzh+qReyr7dqtNHHsWpCG1b2qkLTUNr6XPHsKIF7FB2VEVaKARlKcPrFId pkbppBGNP3oD3PgaijbQl0xSbYzzR9ypzTDvdvaOL2qUsIfOE0Bs48TJMyuuQlkNL3cR 2f8A== X-Gm-Message-State: AOJu0YxN+IdDxyEbv7mX39hEgN2V16QIunaGOrfkRflPM5Zs5cE3dWCK D/XY7tJeP767RMgJhRP0bxyR2pjTrnxrZbVak74LPJdaCP9va4hRu2HXB9AAtiUVAU73LMbtxcl ekKb/GZNbjDIZ95+YHCr+ZbaGGvNuYi3+QtmRos9BkSvlOqVJH2ok/g== X-Gm-Gg: Acq92OGMWUyDo6+u119H+k8l47SuHs2u1a38zblYz+ED8LIra9GZYmIOO5ODbj68nBn gTqI/MWBkkaYhfIjuX8rkPQyjhZg8W43ZcImeEtD3TfT1KB50LZDN3Ow1o2zwn4J/PSmWGr4dOR AMJ81ka3hzhF2y2JywlFxxgL0QEMpBymoEiiWeSMqbBLq2QV/0OVN3in7AK6zdoJidgf5v9be8s Hjxs5VCEivejvSdhJF8vBTHxxIFsrjwIaHH7spRFpfxoMFxgOrTgVc1/mYBlOmaxbROMTiiRn4p P77YSlwBsoY0xDraRQ8RtRFrkervjXvfFOEPuyKWOSTt3T/KlJqMXiToRJmPG0Gj9+OoTpWKNqh lQmJlRVIUdC7Hvb27+SP0FPKXwWXMP2K5d5NHVlviSJFmpiHjBVDIdUA8ZG9I X-Received: by 2002:a5d:59ab:0:b0:45d:7fdc:2e99 with SMTP id ffacd0b85a97d-45ea0d604e7mr1328687f8f.12.1779308933572; Wed, 20 May 2026 13:28:53 -0700 (PDT) X-Received: by 2002:a5d:59ab:0:b0:45d:7fdc:2e99 with SMTP id ffacd0b85a97d-45ea0d604e7mr1328663f8f.12.1779308933046; Wed, 20 May 2026 13:28:53 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [176.103.220.4]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-45d9ec39ff1sm61660867f8f.10.2026.05.20.13.28.52 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 20 May 2026 13:28:52 -0700 (PDT) From: Stefano Brivio To: David Gibson Subject: Re: [PATCH 3/6] tcp_splice: Clean up flow control path for splice forwarding Message-ID: <20260520222851.19e5f430@elisabeth> In-Reply-To: <20260520130851.436931-4-david@gibson.dropbear.id.au> References: <20260520130851.436931-1-david@gibson.dropbear.id.au> <20260520130851.436931-4-david@gibson.dropbear.id.au> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.49; x86_64-pc-linux-gnu) MIME-Version: 1.0 Date: Wed, 20 May 2026 22:28:52 +0200 (CEST) X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: XYm961FVZU2dF8S-7FKlALh2ABx7sqdxKpcs5_z2tho_1779308934 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: TLKWJRGQX5FUQPT5UK2IQ5A3HU4EWLIV X-Message-ID-Hash: TLKWJRGQX5FUQPT5UK2IQ5A3HU4EWLIV X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top, Paul Holzinger X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Ah, yes, it looks better now. Three remarks: On Wed, 20 May 2026 23:08:48 +1000 David Gibson wrote: > Splice forwarding can be blocked either waiting for data from one side > or waiting for space on the other. For that reason, > tcp_splice_sock_handler() on either socket can forward data in either or > both directions, depending on whether we have EPOLLIN, EPOLLOUT or both > events. > > The flow control for this is quite hard to follow though, since we forward > in one direction, then sometimes loop back with a goto to do it in the > other direction. Simplify this by adding a tcp_splice_forward() function > with the logic to forward in one direction and calling it either once or > twice from tcp_splice_sock_handler(). > > Signed-off-by: David Gibson > --- > tcp_splice.c | 137 ++++++++++++++++++++++++++------------------------- > 1 file changed, 71 insertions(+), 66 deletions(-) > > diff --git a/tcp_splice.c b/tcp_splice.c > index 34ffea73..18e8b303 100644 > --- a/tcp_splice.c > +++ b/tcp_splice.c > @@ -474,67 +474,20 @@ void tcp_splice_conn_from_sock(const struct ctx *c, union flow *flow, int s0) > } > > /** > - * tcp_splice_sock_handler() - Handler for socket mapped to spliced connection > + * tcp_splice_forward() - Forward data in one direction using splice() > * @c: Execution context > - * @ref: epoll reference > - * @events: epoll events bitmap > + * @conn: Connection to forward data for > + * @fromsidei: Side to forward data from > * > * #syscalls:pasta splice > */ > -void tcp_splice_sock_handler(struct ctx *c, union epoll_ref ref, > - uint32_t events) > +static int tcp_splice_forward(struct ctx *c, struct > + tcp_splice_conn *conn, unsigned fromsidei) I think the struct argument should all be on the same line. > { > - struct tcp_splice_conn *conn = conn_at_sidx(ref.flowside); > - unsigned evsidei = ref.flowside.sidei, fromsidei; > - uint8_t lowat_set_flag, lowat_act_flag; > - int eof, never_read; > - > - assert(conn->f.type == FLOW_TCP_SPLICE); > - > - if (conn->events == SPLICE_CLOSED) > - return; > - > - if (events & EPOLLERR) { > - int err, rc; > - socklen_t sl = sizeof(err); > - > - rc = getsockopt(ref.fd, SOL_SOCKET, SO_ERROR, &err, &sl); > - if (rc) > - flow_perror(conn, "Error retrieving SO_ERROR"); > - else > - flow_dbg(conn, "Error event on %s socket: %s", > - pif_name(conn->f.pif[evsidei]), > - strerror_(err)); > - goto reset; > - } > - > - if (conn->events == SPLICE_CONNECT) { > - if (!(events & EPOLLOUT)) { > - flow_err(conn, "Unexpected events 0x%x during connect", > - events); > - goto reset; > - } > - if (tcp_splice_connect_finish(c, conn)) > - goto reset; > - } > - > - if (events & EPOLLOUT) { > - fromsidei = !evsidei; > - conn_event(conn, ~OUT_WAIT(evsidei)); > - } else { > - fromsidei = evsidei; > - } > - > - if (events & EPOLLRDHUP) > - /* For side 0 this is fake, but implied */ > - conn_event(conn, FIN_RCVD(evsidei)); > - > -swap: > - eof = 0; > - never_read = 1; > - > - lowat_set_flag = RCVLOWAT_SET(fromsidei); > - lowat_act_flag = RCVLOWAT_ACT(fromsidei); > + uint8_t lowat_set_flag = RCVLOWAT_SET(fromsidei); > + uint8_t lowat_act_flag = RCVLOWAT_ACT(fromsidei); > + int never_read = 1; > + int eof = 0; > > while (1) { > ssize_t readlen, written, pending; > @@ -551,7 +504,7 @@ retry: > if (readlen < 0 && errno != EAGAIN) { > flow_perror(conn, "Splicing from %s socket", > pif_name(conn->f.pif[fromsidei])); > - goto reset; > + return -1; > } > > flow_trace(conn, "%zi from read-side call", readlen); > @@ -578,7 +531,7 @@ retry: > if (written < 0 && errno != EAGAIN) { > flow_perror(conn, "Splicing to %s socket", > pif_name(conn->f.pif[!fromsidei])); > - goto reset; > + return -1; > } > > flow_trace(conn, "%zi from write-side call (passed %zi)", > @@ -639,24 +592,76 @@ retry: > if (shutdown(conn->s[!sidei], SHUT_WR) < 0) { > flow_perror(conn, "shutdown() on %s", > pif_name(conn->f.pif[!sidei])); > - goto reset; > + return -1; > } > conn_event(conn, FIN_SENT(!sidei)); > } > } > } > > - if (CONN_HAS(conn, FIN_SENT(0) | FIN_SENT(1))) { > - /* Clean close, no reset */ > - conn_flag(conn, CLOSING); > + return 0; > +} > + > +/** > + * tcp_splice_sock_handler() - Handler for socket mapped to spliced connection > + * @c: Execution context > + * @ref: epoll reference > + * @events: epoll events bitmap > + */ > +void tcp_splice_sock_handler(struct ctx *c, union epoll_ref ref, > + uint32_t events) > +{ > + struct tcp_splice_conn *conn = conn_at_sidx(ref.flowside); > + unsigned evsidei = ref.flowside.sidei; > + > + assert(conn->f.type == FLOW_TCP_SPLICE); > + > + if (conn->events == SPLICE_CLOSED) > return; > + > + if (events & EPOLLERR) { > + int err, rc; > + socklen_t sl = sizeof(err); > + > + rc = getsockopt(ref.fd, SOL_SOCKET, SO_ERROR, &err, &sl); > + if (rc) > + flow_perror(conn, "Error retrieving SO_ERROR"); > + else > + flow_dbg(conn, "Error event on %s socket: %s", > + pif_name(conn->f.pif[evsidei]), > + strerror_(err)); > + goto reset; > + } > + > + if (conn->events == SPLICE_CONNECT) { > + if (!(events & EPOLLOUT)) { > + flow_err(conn, "Unexpected events 0x%x during connect", > + events); > + goto reset; > + } > + if (tcp_splice_connect_finish(c, conn)) > + goto reset; > + } > + > + if (events & EPOLLRDHUP) > + /* For side 0 this is fake, but implied */ > + conn_event(conn, FIN_RCVD(evsidei)); I saw this all goes away in 5/6, so it wouldn't be relevant. But in case we decide to drop 5/6, here are my remarks on the this. EPOLLRDHUP is now handled before checking the other direction of the connection in case of EPOLLOUT. I think it actually makes more sense this way because we update flags with everything we know until that point, and it shouldn't have a functional effect (the check at the end of the new tcp_splice_forward() is on FIN_RCVD(fromsidei)), but I'm raising that in case the change wasn't intended. > + > + if (events & EPOLLOUT) { > + if (tcp_splice_forward(c, conn, !evsidei)) > + goto reset; > + conn_event(conn, ~OUT_WAIT(evsidei)); > } > > - if ((events & (EPOLLIN | EPOLLOUT)) == (EPOLLIN | EPOLLOUT)) { > - events = EPOLLIN; > + if (events & EPOLLIN) { > + if (tcp_splice_forward(c, conn, evsidei)) > + goto reset; This should be: goto reset; instead of: goto reset; > + } > > - fromsidei = !fromsidei; > - goto swap; > + if (CONN_HAS(conn, FIN_SENT(0) | FIN_SENT(1))) { > + /* Clean close, no reset */ > + conn_flag(conn, CLOSING); > + return; > } > > if (events & EPOLLHUP) { -- Stefano