From: Vitaliy Makkoveev Subject: Re: Remove soassertlocked() assertion from soreserve() To: Alexander Bluhm Cc: tech@openbsd.org Date: Tue, 18 Feb 2025 14:26:19 +0300 On Mon, Feb 17, 2025 at 06:46:08PM +0100, Alexander Bluhm wrote: > On Mon, Feb 17, 2025 at 12:43:23PM +0300, Vitaliy Makkoveev wrote: > > We don't need it anymore. Note, the modification of both socket buffers > > is serialized. > > I found some sb_hiwat reads which are not protected by sb_mtx. But > they have socket lock. We could keep socket lock here and not care > about sb_mtx. > I know about them. But they are single loads like in tcp_output(). Do we really need to use READ_ONCE() or lock mutex for such access? 193 tcp_output(struct tcpcb *tp) ... 353 txmaxseg = ulmin(so->so_snd.sb_hiwat / 2, tp->t_maxseg); 354 > Any reason why you want to remove it? Is socket lock not taken > anyway by all callers? > In your "nfs shared socket lock" thread, we decided to move `sb_flags' modifications out of solock() with separate diff. But soreserve() also doesn't need for solock(). 234 nfs_connect(struct nfsmount *nmp, struct nfsreq *rep) ... 370 solock_shared(so); 371 error = soreserve(so, sndreserve, rcvreserve); 372 if (error) 373 goto bad_locked; 374 mtx_enter(&so->so_rcv.sb_mtx); 375 so->so_rcv.sb_flags |= SB_NOINTR; 376 mtx_leave(&so->so_rcv.sb_mtx); 377 mtx_enter(&so->so_snd.sb_mtx); 378 so->so_snd.sb_flags |= SB_NOINTR; 379 mtx_leave(&so->so_snd.sb_mtx); 380 sounlock_shared(so); Note, outside nfs we call soreserve() on attach or sonewconn() paths and the socket is not yet exported or attached to the stack. In the nfs case we reserve space in the middle of socket setup, so does this solock() really matter? > bluhm > > > Index: sys/kern/uipc_socket2.c > > =================================================================== > > RCS file: /cvs/src/sys/kern/uipc_socket2.c,v > > diff -u -p -r1.180 uipc_socket2.c > > --- sys/kern/uipc_socket2.c 17 Feb 2025 08:56:33 -0000 1.180 > > +++ sys/kern/uipc_socket2.c 17 Feb 2025 09:34:24 -0000 > > @@ -609,8 +609,6 @@ sowakeup(struct socket *so, struct sockb > > int > > soreserve(struct socket *so, u_long sndcc, u_long rcvcc) > > { > > - soassertlocked(so); > > - > > mtx_enter(&so->so_rcv.sb_mtx); > > mtx_enter(&so->so_snd.sb_mtx); > > if (sbreserve(&so->so_snd, sndcc)) >