public inbox for passt-dev@passt.top
 help / color / mirror / code / Atom feed
* [PATCH] tcp_splice: avoid delay on certain transfers
@ 2026-10-02 16:39 Bernhard M. Wiedemann
  2026-10-05 19:37 ` Stefano Brivio
  0 siblings, 1 reply; 3+ messages in thread
From: Bernhard M. Wiedemann @ 2026-10-02 16:39 UTC (permalink / raw)
  To: passt-dev; +Cc: Bernhard M. Wiedemann

without this patch, sending buffers of certain sizes
caused a delay of 200ms because tcp_splice_forward()
passes SPLICE_F_MORE to the writer
when a read filled at least 90% of the pipe.

This is easy to hit when pipes are small: once a user exceeds
fs.pipe-user-pages-soft (64 MiB by default, which a few long-running
pasta instances with large pipes reach on their own),
tcp_set_pipe_size() settles on the 8 KiB minimum, and every message
whose length is 7372 to 8192 bytes (90% of the pipe and more) past a
multiple of 8 KiB stalls.

This change leaves bulk throughput unchanged.

Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Bernhard M. Wiedemann <bwiedemann@suse.de>
---
Notes:
  The change was slightly tested and benchmarked. Results look decent.
  Not sure if we actually need the flow_trace for error handling there.
  This issue was accidentally found by running PostgreSQL in a rootless
    podman container for benchmarking.

 tcp_splice.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/tcp_splice.c b/tcp_splice.c
index 4b01f1a..f7a3913 100644
--- a/tcp_splice.c
+++ b/tcp_splice.c
@@ -488,6 +488,7 @@ static int tcp_splice_forward(struct ctx *c,
 {
 	uint8_t lowat_set_flag = RCVLOWAT_SET(fromsidei);
 	uint8_t lowat_act_flag = RCVLOWAT_ACT(fromsidei);
+	bool corked = false;
 
 	while (1) {
 		ssize_t readlen, written;
@@ -517,8 +518,17 @@ static int tcp_splice_forward(struct ctx *c,
 			 * there's nothing in the pipe so there's nothing to do
 			 * write side either.
 			 */
-			if (!conn->pending[fromsidei])
+			if (!conn->pending[fromsidei]) {
+				/* Setting TCP_NODELAY again flushes data held
+				 * back by SPLICE_F_MORE
+				 */
+				if (corked &&
+				    setsockopt(conn->s[!fromsidei], SOL_TCP,
+					       TCP_NODELAY, &((int){ 1 }),
+					       sizeof(int)))
+					flow_trace(conn, "failed to push data");
 				break;
+			}
 		} else {
 			conn->pending[fromsidei] += readlen;
 
@@ -549,6 +559,9 @@ static int tcp_splice_forward(struct ctx *c,
 		if (written < 0)
 			break;
 
+		if (written > 0)
+			corked = more;
+
 		conn->pending[fromsidei] -= written;
 
 		if (!conn->pending[fromsidei] && readlen <= 0) {
-- 
2.55.0


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] tcp_splice: avoid delay on certain transfers
  2026-10-02 16:39 [PATCH] tcp_splice: avoid delay on certain transfers Bernhard M. Wiedemann
@ 2026-10-05 19:37 ` Stefano Brivio
  2026-10-06  9:41   ` Bernhard M. Wiedemann
  0 siblings, 1 reply; 3+ messages in thread
From: Stefano Brivio @ 2026-10-05 19:37 UTC (permalink / raw)
  To: Bernhard M. Wiedemann; +Cc: passt-dev

Bernard, thanks for the investigation and for the patch. Just one doubt:

On Fri,  2 Oct 2026 18:39:25 +0200
"Bernhard M. Wiedemann" <bwiedemann@suse.de> wrote:

> without this patch, sending buffers of certain sizes
> caused a delay of 200ms because tcp_splice_forward()
> passes SPLICE_F_MORE to the writer
> when a read filled at least 90% of the pipe.
> 
> This is easy to hit when pipes are small: once a user exceeds
> fs.pipe-user-pages-soft (64 MiB by default, which a few long-running
> pasta instances with large pipes reach on their own),
> tcp_set_pipe_size() settles on the 8 KiB minimum, and every message
> whose length is 7372 to 8192 bytes (90% of the pipe and more) past a
> multiple of 8 KiB stalls.
> 
> This change leaves bulk throughput unchanged.
> 
> Assisted-by: Claude:claude-opus-5-5
> Signed-off-by: Bernhard M. Wiedemann <bwiedemann@suse.de>
> ---
> Notes:
>   The change was slightly tested and benchmarked. Results look decent.
>   Not sure if we actually need the flow_trace for error handling there.
>   This issue was accidentally found by running PostgreSQL in a rootless
>     podman container for benchmarking.
> 
>  tcp_splice.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/tcp_splice.c b/tcp_splice.c
> index 4b01f1a..f7a3913 100644
> --- a/tcp_splice.c
> +++ b/tcp_splice.c
> @@ -488,6 +488,7 @@ static int tcp_splice_forward(struct ctx *c,
>  {
>  	uint8_t lowat_set_flag = RCVLOWAT_SET(fromsidei);
>  	uint8_t lowat_act_flag = RCVLOWAT_ACT(fromsidei);
> +	bool corked = false;
>  
>  	while (1) {
>  		ssize_t readlen, written;
> @@ -517,8 +518,17 @@ static int tcp_splice_forward(struct ctx *c,
>  			 * there's nothing in the pipe so there's nothing to do
>  			 * write side either.
>  			 */
> -			if (!conn->pending[fromsidei])
> +			if (!conn->pending[fromsidei]) {
> +				/* Setting TCP_NODELAY again flushes data held
> +				 * back by SPLICE_F_MORE

...nice, I didn't know about that trick. But wouldn't it be more
natural to not use SPLICE_F_MORE if the pipe is small enough? Do you
have a stand-alone reproducer that could help figuring this out?

I'm a bit worried we might cause unnecessary setsockopt() calls in some
corner cases if we go this way, even though it's a rather minor concern
(we just called splice(), and that setsockopt() is not _that_ expensive
in comparison).

> +				 */
> +				if (corked &&
> +				    setsockopt(conn->s[!fromsidei], SOL_TCP,
> +					       TCP_NODELAY, &((int){ 1 }),
> +					       sizeof(int)))
> +					flow_trace(conn, "failed to push data");
>  				break;
> +			}
>  		} else {
>  			conn->pending[fromsidei] += readlen;
>  
> @@ -549,6 +559,9 @@ static int tcp_splice_forward(struct ctx *c,
>  		if (written < 0)
>  			break;
>  
> +		if (written > 0)
> +			corked = more;
> +
>  		conn->pending[fromsidei] -= written;
>  
>  		if (!conn->pending[fromsidei] && readlen <= 0) {

-- 
Stefano


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] tcp_splice: avoid delay on certain transfers
  2026-10-05 19:37 ` Stefano Brivio
@ 2026-10-06  9:41   ` Bernhard M. Wiedemann
  0 siblings, 0 replies; 3+ messages in thread
From: Bernhard M. Wiedemann @ 2026-10-06  9:41 UTC (permalink / raw)
  To: Stefano Brivio; +Cc: passt-dev


[-- Attachment #1.1.1: Type: text/plain, Size: 3265 bytes --]



On 05/10/2026 21.37, Stefano Brivio wrote:
> Bernard, thanks for the investigation and for the patch. Just one doubt:
[...]
> I'm a bit worried we might cause unnecessary setsockopt() calls in some
> corner cases if we go this way, even though it's a rather minor concern
> (we just called splice(), and that setsockopt() is not _that_ expensive
> in comparison).
The 'corked' variable should ensure that we only add the setsockopt when 
it would otherwise add the unneccesary 200ms delay.

I attached the reproducers and benchmarking tools.
The finding from running it was that SPLICE_F_MORE gives plenty extra 
performance even for 8K pipes and setsockopt hardly costs anything.

To test:
# maybe with adjustment to /proc/sys/fs/pipe-user-pages-soft
pasta --config-net -t 127.0.0.1/25432:15432 -- python3 rr.py serve 15432
python3 rr.py 25432


   ┌─────────┬─────────┬───────────────┬─────────────────┬────────────────┐
│  Build  │  Pipes  │ Worst latency │ Upload (Gbit/s) │ Download/Gbit  │
├─────────┼─────────┼───────────────┼─────────────────┼────────────────┤
│ base    │ 8 KiB   │ 208 ms        │ 89–91           │ 87–89          │
├─────────┼─────────┼───────────────┼─────────────────┼────────────────┤
│ patched │ 8 KiB   │ 0.1 ms        │ 88–89           │ 86–89          │
├─────────┼─────────┼───────────────┼─────────────────┼────────────────┤
│ nomore  │ 8 KiB   │ 0.1 ms        │ 42–43           │ 40             │
├─────────┼─────────┼───────────────┼─────────────────┼────────────────┤
│ base    │ default │ 0.3 ms        │ 203–204         │ 189–198        │
├─────────┼─────────┼───────────────┼─────────────────┼────────────────┤
│ patched │ default │ 0.1 ms        │ 203–205         │ 185–190        │
├─────────┼─────────┼───────────────┼─────────────────┼────────────────┤
│ nomore  │ default │ 0.1 ms        │ 193–203         │ 177–190        │
└─────────┴─────────┴───────────────┴─────────────────┴────────────────┘


Ciao
Bernhard M.

[-- Attachment #1.1.2: bulk.py --]
[-- Type: text/x-python, Size: 1891 bytes --]

"""Bulk throughput through a TCP port. server: bulk.py serve PORT; client: bulk.py PORT MiB RUNS"""
import socket, statistics, sys, time
CH = 1 << 20
def sink(s):
    buf = bytearray(CH); n = 0
    while (r := s.recv_into(buf)): n += r
    return n
def source(s, total):
    mv = memoryview(bytes(CH)); sent = 0
    while sent < total: s.sendall(mv); sent += CH
    s.shutdown(socket.SHUT_WR)
if sys.argv[1] == "serve":
    l = socket.socket(); l.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
    l.bind(("0.0.0.0", int(sys.argv[2]))); l.listen()
    while True:
        s, _ = l.accept(); f = s.makefile("rb"); cmd = f.readline().split()
        if not cmd:              # readiness probe
            f.close(); s.close(); continue
        if cmd[0] == b"up":      # client sends, we count and reply with byte count
            buf = bytearray(CH); n = 0
            while (r := f.readinto(buf)): n += r
            s.sendall(str(n).encode()); f.close(); s.close()
        else:                    # down: we send N bytes
            source(s, int(cmd[1])); sink(s); f.close(); s.close()
else:
    port, mib, runs = int(sys.argv[1]), int(sys.argv[2]), int(sys.argv[3]); total = mib * CH
    for d in (sys.argv[4:] or ["up", "down"]):
        rates = []
        for _ in range(runs):
            s = socket.create_connection(("127.0.0.1", port)); t0 = time.perf_counter()
            s.sendall(f"{d} {total}\n".encode())
            if d == "up":
                source(s, total)
                got = b""
                while (c := s.recv(64)): got += c
                assert int(got) == total, got
            else:
                assert sink(s) == total
            rates.append(total / (time.perf_counter() - t0) / 1e9 * 8)
            s.close()
        print(f"{d:4}: median {statistics.median(rates):6.2f} Gbit/s  (" + " ".join(f"{r:.2f}" for r in rates) + ")", flush=True)

[-- Attachment #1.1.3: rr.py --]
[-- Type: text/x-python, Size: 1410 bytes --]

"""Request/response latency through a TCP port. server: rr.py serve PORT; client: rr.py PORT"""
import socket, struct, sys, time
def recvn(s, n):
    b = bytearray()
    while len(b) < n:
        c = s.recv(n - len(b))
        if not c: raise EOFError
        b += c
    return bytes(b)
if sys.argv[1] == "serve":
    l = socket.socket(); l.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
    l.bind(("0.0.0.0", int(sys.argv[2]))); l.listen()
    while True:
        s, _ = l.accept(); s.setsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY, 1)
        try:
            while True:
                req, resp = struct.unpack("!II", recvn(s, 8)); recvn(s, req); s.sendall(b"r" * resp)
        except EOFError:
            s.close()
else:
    s = socket.create_connection(("127.0.0.1", int(sys.argv[1]))); s.setsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY, 1)
    for label, req, resp in [("request", n, 16) for n in (4000, 7372, 7380, 8000, 9000, 16000, 20000)] + [("response", 16, n) for n in (4000, 7380, 8000, 16000, 64000)]:
        ts = []
        for _ in range(3):
            t0 = time.perf_counter(); s.sendall(struct.pack("!II", req, resp) + b"q" * req); recvn(s, resp); ts.append((time.perf_counter() - t0) * 1000)
        print(f"{label:8} {max(req, resp) if label == 'request' and False else (req if label=='request' else resp):6d} B: " + " ".join(f"{t:6.1f}" for t in ts) + " ms", flush=True)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2026-10-06  9:41 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-10-02 16:39 [PATCH] tcp_splice: avoid delay on certain transfers Bernhard M. Wiedemann
2026-10-05 19:37 ` Stefano Brivio
2026-10-06  9:41   ` Bernhard M. Wiedemann

Code repositories for project(s) associated with this public inbox

	https://passt.top/passt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).