From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: Relax sockets splicing locking
To: Vitaliy Makkoveev <mvs@openbsd.org>
Cc: tech@openbsd.org
Date: Tue, 7 Jan 2025 00:31:15 +0100

On Mon, Jan 06, 2025 at 04:26:30PM +0300, Vitaliy Makkoveev wrote:
> On Mon, Jan 06, 2025 at 01:17:44AM +0100, Alexander Bluhm wrote:
> > On Sat, Jan 04, 2025 at 08:07:14PM +0300, Vitaliy Makkoveev wrote:
> > > On Sat, Jan 04, 2025 at 06:39:26PM +0300, Vitaliy Makkoveev wrote:
> > > > > On 4 Jan 2025, at 16:34, Alexander Bluhm <bluhm@openbsd.org> wrote:
> > > > > 
> > > 
> > > > > [skip]
> > > 
> > > > > I have tested it with my regress and netlink setup.  My perform
> > > > > testing machines are currently busy with other tests.
> > > > > 
> > > > > We have no propper regression tests for splicing and unsplicing
> > > > > under heavy load.  I am currently trying to write a test.
> > > > > 
> > > > 
> > > > We need something like multithreaded attempts of simultaneous
> > > > splicing and unsplicing one socket to itself. Or the same but
> > > > for two sockets.
> > > > 
> > > 
> > > The simplest test to concurrently splice/unsplice socket.
> > 
> > I have commited a more exhaustive test in regress/sys/netinet/tcpthread.
> > It does all kind of TCP socket operations in parallel.
> > 
> > But your test also has value as it does splice/unsplice very fast.
> > If you want, I can integrate it somewhere in regress/sys/kern/sosplice
> > with a proper regress Makefile.
> > 
> > Currently the kernel does not complain about unsplicing sockets
> > that are not spliced.  I think it should report an error.  That
> > would help counting success in our tests.
> > 
> 
> Does ENOTCONN look reasonable? We are not interesting in this error in
> the soclose(), soidle() and sotask() paths, only in the unsplicing path
> of sosplice().

I have nearly the same diff.  The ENOTCONN is not unique.  sosplice()
may return that for other conditions.  I find it useful that different
error numbers point to the failure.  Reading errno(2) up and down,
I came to the conclusion that EDESTADDRREQ may be suitable.  The
socket is not spliced, you are supposed to specify a drain file
descriptor.

The new error should be documented in sosplice(9) man page.

I have fixed the failing test in regress/sys/kern/sosplice .

With EDESTADDRREQ and man page changes
OK bluhm@

> Index: sys/kern/uipc_socket.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/uipc_socket.c,v
> diff -u -p -r1.356 uipc_socket.c
> --- sys/kern/uipc_socket.c	4 Jan 2025 15:57:02 -0000	1.356
> +++ sys/kern/uipc_socket.c	6 Jan 2025 13:22:20 -0000
> @@ -1388,9 +1388,10 @@ sosplice(struct socket *so, int fd, off_
>  			sosp = soref(so->so_sp->ssp_socket);
>  			sounsplice(so, so->so_sp->ssp_socket, 0);
>  			sorele(sosp);
> -		}
> +		} else
> +			error = ENOTCONN;
>  		sbunlock(&so->so_rcv);
> -		return (0);
> +		return (error);
>  	}
>  
>  	if (sosplice_taskq == NULL) {