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.
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 *);
