On Tue, Feb 18, 2025 at 02:26:19PM +0300, Vitaliy Makkoveev wrote:
> 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?

No.  I don't want to restart the READ_ONCE() discussion at this
point.  As we have socket lock now, it is already protected.  And
if we don't have it, it is just a read.

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

I am not happy how this works now, and where this is heading.
nfs_connect() is a good example of what I mean.

First it does socreate() which locks and releases internally.
sosetopt() locks and releases internally depending on the options.
Then solock_shared(); sobind(); sounlock_shared();
And once again sosetopt().
Next solock_shared(); soconnect(); sosleep_nsec(); sounlock_shared();
Finally solock_shared(); soreserve(); set sb_flags; sounlock_shared();

And we are discussing about the final step.  We have this pattern
more often in our code.

Would it be better to 

socreate() which returns a locked socket
sobind()
soconnect()
while { sosleep_nsec() }
soreserve()
set sb_flags
sounlock_shared()

This would allow us to hold the lock while we are creating the
socket that we need.  Of course one can argue, that this is not
necessary in this case, the socket cannot used by anyone else.  But
for me holding the socket lock for a bunch of operations that belong
together seems more natural.

What do you think?

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

Probably this locking is not necessary.  But I think it would be
better to hold the socket lock over larger code sections than arguing
about each section individualy and making multiple short locks.
Grabbing is expensive, holding it for a while is not.

Could you hold back this diff until we have a clear view in which
direction we want to go?  I guess soreserve() soassertlocked() diff
does not block any work you are doing.

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