From: Alexander Bluhm <alexander.bluhm@gmx.net>
Subject: Re: Use `sb_mtx' instead of `inp_mtx' in receive path for inet sockets
To: Vitaliy Makkoveev <mvs@openbsd.org>
Cc: tech@openbsd.org
Date: Sun, 11 Feb 2024 18:16:03 +0100

On Sun, Feb 04, 2024 at 08:40:13PM +0300, Vitaliy Makkoveev wrote:
> In soreceve(), we only touch `so_rcv' socket buffer, which has it's own
> `sb_mtx' mutex(9) for protection. So, we can avoid solock() in this
> path - it's enough to hold `sb_mtx' in soreceive() and around
> corresponding sbappend*(). But not right now :)
> 
> This time we use shared netlock for some inet sockets in the soreceive()
> path. To protect `so_rcv' buffer we use `inp_mtx' mutex(9) and the
> prul_lock() to acquire this mutex(9) at socket layer. But the `inp_mtx'
> mutex belongs to the PCB. We initialize socket before PCB, tcp(4)
> sockets could exist without PCB, so I want to use different locks for
> PCB and socket layers of socket.
> 
> This diff mechanically replaces `inp_mtx' by `sb_mtx' in the receive
> path. Only for sockets which already use `inp_mtx'. All other sockets
> left as is. They will be converted later.
> 
> Since the `sb_mtx' is optional, the new SB_MTXLOCK flag introduced. If
> this flag is set on `sb_flags', the `sb_mtx' mutex(9) should be taken.
> New sb_mtx_lock() and sb_mtx_unlock() was introduced to hide this check.
> They are temporary and will be replaced by mtx_enter() when all this
> area will be converted to `sb_mtx' mutex(9).
> 
> Also, the new sbmtxassertlocked() function introduced to throw
> corresponding assertion for SB_MTXLOCK marked buffers. This time only
> sbappendaddr() calls it. This function is also temporary and will be
> replaced by MTX_ASSERT_LOCKED() later.
> 
> This naming could be strange, bu now we sblock() for different purpose,
> so I want to keep it as is for a while.

OK bluhm@

> @@ -919,7 +929,7 @@ sbappendaddr(struct socket *so, struct s
>  	struct mbuf *m, *n, *nlast;
>  	int space = asa->sa_len;
>  
> -	soassertlocked(so);
> +	sbmtxassertlocked(so, sb);

What about the other sbappend...() functions?  If you replace
soassertlocked() also there, we see if you missed something.

> @@ -220,24 +220,24 @@ rip_input(struct mbuf **mp, int *offp, i
>  		else
>  			n = m_copym(m, 0, M_COPYALL, M_NOWAIT);
>  		if (n != NULL) {
> +			struct socket *so = inp->inp_socket;
>  			int ret;
>  
>  			if (inp->inp_flags & INP_CONTROLOPTS ||
>  			    inp->inp_socket->so_options & SO_TIMESTAMP)

You could also use so instead of inp->inp_socket here.