From: Vitaliy Makkoveev <mvs@openbsd.org>
Subject: Re: Relax sockets splicing locking
To: Alexander Bluhm <bluhm@openbsd.org>
Cc: tech@openbsd.org
Date: Sat, 4 Jan 2025 18:39:26 +0300

> On 4 Jan 2025, at 16:34, Alexander Bluhm <bluhm@openbsd.org> wrote:
> 
> On Sat, Jan 04, 2025 at 02:06:14PM +0300, Vitaliy Makkoveev wrote:
>> On Fri, Jan 03, 2025 at 05:22:51PM +0300, Vitaliy Makkoveev wrote:
>>> Sockets splicing works around sockets buffers which have their own locks
>>> for all socket types, especially sblock() on `so_snd' which keeps
>>> sockets being spliced. 
>>> 
>>> - sosplice() does read-only sockets options and state checks, the only
>>>   modification is `so_sp' assignment. The SB_SPLICE bit modification,
>>>   `ssp_socket' and `ssp_soback' assignment protected with `sb_mtx'
>>>   mutex(9). PCB layer does corresponding checks with `sb_mtx' held, so
>>>   shared solock() is pretty enough in sosplice() path. Introduce
>>>   special sosplice_solock_pair() for that purpose.
>>> 
>>> - sounsplice() requires shared socket lock only around so{r,w}wakeup
>>>   calls.
>>> 
>>> - Push exclusive solock() down to tcp(4) case of somove(). Such sockets
>>>   are not ready do unlocked somove() yet.
>>> 
>> 
>> A little bit updated. The "Make WITNESS happy..." comment is not MP
>> specific, so rewrite it as XXX:
>> 
>> +	/*
>> +	 * XXX: Make WITNESS happy. AF_INET and AF_INET6 sockets could be
>> +	 * spliced together.
>> +	 */
>> 
>> Also whitespace fixed.
> 
> You have this construct several times:
> 
>> +		sosp = soref(so->so_sp->ssp_socket);
>> 		sounsplice(so, so->so_sp->ssp_socket, 0);
>> +		sorele(sosp);
> 
> Would it be nicer to use the refed socket in sounsplice()?
>                sounsplice(so, sosp, 0);
> I practice it should make no difference.
> 

We need the extra reference to to the first socket while dying
socket is spliced back, meanwhile the extra reference for
second socket is not required:

                if (issplicedback(so)) {
                        int freeing = SOSP_FREEING_WRITE;

                        if (so->so_sp->ssp_soback == so)
                                freeing |= SOSP_FREEING_READ;
                        sounsplice(so->so_sp->ssp_soback, so, freeing);
                }
                sbunlock(&soback->so_rcv);
                sorele(soback);

Anyway, this is not the last touch to sounsplice(). I suspect
sowwakeup() could be done lockless.

> 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.