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