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

On Sun, Feb 11, 2024 at 06:16:03PM +0100, Alexander Bluhm wrote:
> 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.
> 

Will replace it later, when corresponding sbappend*() will be protected
with `sb_mtx'. This time this conversion is useless, but grows the diff.

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

Done, thanks.