On Thu, Mar 28, 2024 at 01:21:17AM +0300, Vitaliy Makkoveev wrote:
> On Wed, Mar 27, 2024 at 10:51:09PM +0100, Alexander Bluhm wrote:
> > On Wed, Mar 27, 2024 at 10:26:27PM +0300, Vitaliy Makkoveev wrote:
> > > On Wed, Mar 27, 2024 at 08:17:33AM +0100, Anton Lindqvist wrote:
> > > >
> > > > Observing two regress hangs in the kernel on netio. Both seems make use
> > > > of unix sockets. Could this be the culprit?
> > > >
> > > > regress/lib/libc/fread
> > > > regress/usr.bin/ssh (scp.sh)
> > >
> > > Sorry for delay.
> > >
> > > It was exposed that `sb_mtx' should not be released between `so_rcv'
> > > usage check and corresponding sbwait() sleep. Otherwise wakeup() could
> > > be lost sometimes.
> > >
> > > This diff fixed regress tests. It introduces sbunlock_locked() and
> > > sbwait_locked() to perform with `sb_mtx' held.
> >
> > Do we really need a restart_unlocked label?
> > Instead of goto restart_unlocked you could call solock_shared(so)
> > and fall through goto restart;
> >
>
> This was the first version, but later I decided 'restart_unlocked' is
> better. No problems to return it back.
>
> > I don't like pr_protocol == AF_UNIX check in generic soreceive()
> > function. Could we check SB_MTXLOCK instead?
> >
>
> Me too. I used "pr_protocol == AF_UNIX" check because sosend() does the
> same. The existing SB_MTXLOCK used by some inet sockets and I strictly
> want to move them separately of this fix.
>
> I propose to introduce SB_OWNLOCK flag and check it. I wanted to use
> this flag for socket with standalone sblock() ability to modify
> sblock()/sbwait() behaviour. I need this flag to convert all
> SB_MTXLOCK'ed sockets separately. After conversion, both SB_MTXLOCK and
> SB_OWNLOCK will be removed.
>
> I propose to introduce SB_OWNLOCK right now and use it instead of
> "pr_protocol == AF_UNIX" check to keep inet sockets as is. I have no
> objections to convert them too, but separately.
sb_flags is short, can you use 4 digits 0x0000 for the flags?
And please use a space after #define, then the diff is less ugly.
OK bluhm@
> Index: sys/kern/uipc_socket.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/uipc_socket.c,v
> retrieving revision 1.322
> diff -u -p -r1.322 uipc_socket.c
> --- sys/kern/uipc_socket.c 26 Mar 2024 09:46:47 -0000 1.322
> +++ sys/kern/uipc_socket.c 27 Mar 2024 22:12:55 -0000
> @@ -161,7 +161,7 @@ soalloc(const struct protosw *prp, int w
> }
> break;
> case AF_UNIX:
> - so->so_rcv.sb_flags |= SB_MTXLOCK;
> + so->so_rcv.sb_flags |= SB_MTXLOCK | SB_OWNLOCK;
> break;
> }
>
> @@ -903,12 +903,23 @@ restart:
> }
> SBLASTRECORDCHK(&so->so_rcv, "soreceive sbwait 1");
> SBLASTMBUFCHK(&so->so_rcv, "soreceive sbwait 1");
> - sb_mtx_unlock(&so->so_rcv);
> - sbunlock(so, &so->so_rcv);
> - error = sbwait(so, &so->so_rcv);
> - if (error) {
> +
> + if (so->so_rcv.sb_flags & SB_OWNLOCK) {
> + sbunlock_locked(so, &so->so_rcv);
> sounlock_shared(so);
> - return (error);
> + error = sbwait_locked(so, &so->so_rcv);
> + sb_mtx_unlock(&so->so_rcv);
> + if (error)
> + return (error);
> + solock_shared(so);
> + } else {
> + sb_mtx_unlock(&so->so_rcv);
> + sbunlock(so, &so->so_rcv);
> + error = sbwait(so, &so->so_rcv);
> + if (error) {
> + sounlock_shared(so);
> + return (error);
> + }
> }
> goto restart;
> }
> Index: sys/kern/uipc_socket2.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/uipc_socket2.c,v
> retrieving revision 1.145
> diff -u -p -r1.145 uipc_socket2.c
> --- sys/kern/uipc_socket2.c 26 Mar 2024 09:46:47 -0000 1.145
> +++ sys/kern/uipc_socket2.c 27 Mar 2024 22:12:55 -0000
> @@ -523,6 +523,18 @@ sbmtxassertlocked(struct socket *so, str
> * Wait for data to arrive at/drain from a socket buffer.
> */
> int
> +sbwait_locked(struct socket *so, struct sockbuf *sb)
> +{
> + int prio = (sb->sb_flags & SB_NOINTR) ? PSOCK : PSOCK | PCATCH;
> +
> + MUTEX_ASSERT_LOCKED(&sb->sb_mtx);
> +
> + sb->sb_flags |= SB_WAIT;
> + return msleep_nsec(&sb->sb_cc, &sb->sb_mtx, prio, "sbwait",
> + sb->sb_timeo_nsecs);
> +}
> +
> +int
> sbwait(struct socket *so, struct sockbuf *sb)
> {
> uint64_t timeo_nsecs;
> @@ -573,20 +585,23 @@ out:
> }
>
> void
> -sbunlock(struct socket *so, struct sockbuf *sb)
> +sbunlock_locked(struct socket *so, struct sockbuf *sb)
> {
> - int dowakeup = 0;
> + MUTEX_ASSERT_LOCKED(&sb->sb_mtx);
>
> - mtx_enter(&sb->sb_mtx);
> sb->sb_flags &= ~SB_LOCK;
> if (sb->sb_flags & SB_WANT) {
> sb->sb_flags &= ~SB_WANT;
> - dowakeup = 1;
> + wakeup(&sb->sb_flags);
> }
> - mtx_leave(&sb->sb_mtx);
> +}
>
> - if (dowakeup)
> - wakeup(&sb->sb_flags);
> +void
> +sbunlock(struct socket *so, struct sockbuf *sb)
> +{
> + mtx_enter(&sb->sb_mtx);
> + sbunlock_locked(so, sb);
> + mtx_leave(&sb->sb_mtx);
> }
>
> /*
> Index: sys/sys/socketvar.h
> ===================================================================
> RCS file: /cvs/src/sys/sys/socketvar.h,v
> retrieving revision 1.126
> diff -u -p -r1.126 socketvar.h
> --- sys/sys/socketvar.h 26 Mar 2024 09:46:47 -0000 1.126
> +++ sys/sys/socketvar.h 27 Mar 2024 22:12:55 -0000
> @@ -128,13 +128,14 @@ struct socket {
> struct klist sb_klist; /* process selecting read/write */
> } so_rcv, so_snd;
> #define SB_MAX (2*1024*1024) /* default for max chars in
> sockbuf */
> -#define SB_LOCK 0x01 /* lock on data queue */
> -#define SB_WANT 0x02 /* someone is waiting to lock */
> -#define SB_WAIT 0x04 /* someone is waiting for
> data/space */
> -#define SB_ASYNC 0x10 /* ASYNC I/O, need signals */
> -#define SB_SPLICE 0x20 /* buffer is splice source or
> drain */
> -#define SB_NOINTR 0x40 /* operations not interruptible
> */
> -#define SB_MTXLOCK 0x80 /* use sb_mtx for sockbuf protection */
> +#define SB_LOCK 0x001 /* lock on data queue */
> +#define SB_WANT 0x002 /* someone is waiting to lock */
> +#define SB_WAIT 0x004 /* someone is waiting for
> data/space */
> +#define SB_ASYNC 0x010 /* ASYNC I/O, need signals */
> +#define SB_SPLICE 0x020 /* buffer is splice source or
> drain */
> +#define SB_NOINTR 0x040 /* operations not interruptible
> */
> +#define SB_MTXLOCK 0x080 /* use sb_mtx for sockbuf protection */
> +#define SB_OWNLOCK 0x100 /* sb_mtx used standalone */
>
> void (*so_upcall)(struct socket *so, caddr_t arg, int waitf);
> caddr_t so_upcallarg; /* Arg for above */
> @@ -320,6 +321,7 @@ int sblock(struct socket *, struct sockb
>
> /* release lock on sockbuf sb */
> void sbunlock(struct socket *, struct sockbuf *);
> +void sbunlock_locked(struct socket *, struct sockbuf *);
>
> #define SB_EMPTY_FIXUP(sb) do {
> \
> if ((sb)->sb_mb == NULL) { \
> @@ -367,6 +369,7 @@ int sbcheckreserve(u_long, u_long);
> int sbchecklowmem(void);
> int sbreserve(struct socket *, struct sockbuf *, u_long);
> int sbwait(struct socket *, struct sockbuf *);
> +int sbwait_locked(struct socket *, struct sockbuf *);
> void soinit(void);
> void soabort(struct socket *);
> int soaccept(struct socket *, struct mbuf *);
