Re: sblock() & solock() ordering
On Mon, Jul 03, 2017 at 11:42:19AM +0200, Martin Pieuchot wrote:
> Updated diff that fixes some issues reported by visa@:
>
> - prevents relocking the netlock in the 'restart' case.
> - always call solock() after sbunlock() in sosplice().
>
> Alexander is there an easy way to trigger the 'restart' case in the
> regression tests? I ran the relayd regress but I'm not sure which
> kernel code it exercises.
See /usr/src/regress/sys/kern/sosplice, it runs all tests with and
without socket splicing. Goal was to see that behavior is identical.
It tries a bunch of timings and dataflow variants with user land
sleep, send and receive syscalls. I triggered many goto restart
in soreceive() and a few in sosend().
> Anyway, ok?
OK bluhm@
>
> Index: sys/socketvar.h
> ===
> RCS file: /cvs/src/sys/sys/socketvar.h,v
> retrieving revision 1.70
> diff -u -p -r1.70 socketvar.h
> --- sys/socketvar.h 26 Jun 2017 09:32:32 - 1.70
> +++ sys/socketvar.h 3 Jul 2017 08:38:14 -
> @@ -239,7 +239,7 @@ struct rwlock;
> * Unless SB_NOINTR is set on sockbuf, sleep is interruptible.
> * Returns error without lock if sleep is interrupted.
> */
> -int sblock(struct sockbuf *, int, struct rwlock *);
> +int sblock(struct socket *, struct sockbuf *, int);
>
> /* release lock on sockbuf sb */
> void sbunlock(struct sockbuf *);
> Index: kern/uipc_socket.c
> ===
> RCS file: /cvs/src/sys/kern/uipc_socket.c,v
> retrieving revision 1.191
> diff -u -p -r1.191 uipc_socket.c
> --- kern/uipc_socket.c3 Jul 2017 08:29:24 - 1.191
> +++ kern/uipc_socket.c3 Jul 2017 08:41:24 -
> @@ -418,14 +418,14 @@ sosend(struct socket *so, struct mbuf *a
> (sizeof(struct fdpass) / sizeof(int)));
> }
>
> -#define snderr(errno) { error = errno; sounlock(s); goto release; }
> +#define snderr(errno) { error = errno; goto release; }
>
> + s = solock(so);
> restart:
> - if ((error = sblock(>so_snd, SBLOCKWAIT(flags), NULL)) != 0)
> + if ((error = sblock(so, >so_snd, SBLOCKWAIT(flags))) != 0)
> goto out;
> so->so_state |= SS_ISSENDING;
> do {
> - s = solock(so);
> if (so->so_state & SS_CANTSENDMORE)
> snderr(EPIPE);
> if (so->so_error) {
> @@ -455,12 +455,10 @@ restart:
> sbunlock(>so_snd);
> error = sbwait(so, >so_snd);
> so->so_state &= ~SS_ISSENDING;
> - sounlock(s);
> if (error)
> goto out;
> goto restart;
> }
> - sounlock(s);
> space -= clen;
> do {
> if (uio == NULL) {
> @@ -471,8 +469,9 @@ restart:
> if (flags & MSG_EOR)
> top->m_flags |= M_EOR;
> } else {
> - error = m_getuio(, atomic,
> - space, uio);
> + sounlock(s);
> + error = m_getuio(, atomic, space, uio);
> + s = solock(so);
> if (error)
> goto release;
> space -= top->m_pkthdr.len;
> @@ -480,7 +479,6 @@ restart:
> if (flags & MSG_EOR)
> top->m_flags |= M_EOR;
> }
> - s = solock(so);
> if (resid == 0)
> so->so_state &= ~SS_ISSENDING;
> if (top && so->so_options & SO_ZEROIZE)
> @@ -488,7 +486,6 @@ restart:
> error = (*so->so_proto->pr_usrreq)(so,
> (flags & MSG_OOB) ? PRU_SENDOOB : PRU_SEND,
> top, addr, control, curproc);
> - sounlock(s);
> clen = 0;
> control = NULL;
> top = NULL;
> @@ -501,6 +498,7 @@ release:
> so->so_state &= ~SS_ISSENDING;
> sbunlock(>so_snd);
> out:
> + sounlock(s);
> m_freem(top);
> m_freem(control);
> return (error);
> @@ -670,9 +668,11 @@ bad:
> *mp = NULL;
>
> restart:
> - if ((error = sblock(>so_rcv, SBLOCKWAIT(flags), NULL)) != 0)
> - return (error);
> s = solock(so);
> + if ((error = sblock(so, >so_rcv, SBLOCKWAIT(flags))) != 0) {
> + sounlock(s);
> + return (error);
> + }
>
> m = so->so_rcv.sb_mb;
> #ifdef SOCKET_SPLICE
> @@ -1040,13 +1040,10 @@ sorflush(struct socket *so)
> {
> struct sockbuf *sb = >so_rcv;
> struct
Re: sblock() & solock() ordering
On 26/06/17(Mon) 16:15, Martin Pieuchot wrote:
> I'd like to enforce the following "lock" ordering: always hold the
> socket lock when calling sblock().
>
> This would allow me to protect `so_state' in sosend() when setting the
> SS_ISSENDING bit.
>
> Diff below implements that. It also gets rid of sbsleep() and uses
> sosleep() instead.
Updated diff that fixes some issues reported by visa@:
- prevents relocking the netlock in the 'restart' case.
- always call solock() after sbunlock() in sosplice().
Alexander is there an easy way to trigger the 'restart' case in the
regression tests? I ran the relayd regress but I'm not sure which
kernel code it exercises.
Anyway, ok?
Index: sys/socketvar.h
===
RCS file: /cvs/src/sys/sys/socketvar.h,v
retrieving revision 1.70
diff -u -p -r1.70 socketvar.h
--- sys/socketvar.h 26 Jun 2017 09:32:32 - 1.70
+++ sys/socketvar.h 3 Jul 2017 08:38:14 -
@@ -239,7 +239,7 @@ struct rwlock;
* Unless SB_NOINTR is set on sockbuf, sleep is interruptible.
* Returns error without lock if sleep is interrupted.
*/
-int sblock(struct sockbuf *, int, struct rwlock *);
+int sblock(struct socket *, struct sockbuf *, int);
/* release lock on sockbuf sb */
void sbunlock(struct sockbuf *);
Index: kern/uipc_socket.c
===
RCS file: /cvs/src/sys/kern/uipc_socket.c,v
retrieving revision 1.191
diff -u -p -r1.191 uipc_socket.c
--- kern/uipc_socket.c 3 Jul 2017 08:29:24 - 1.191
+++ kern/uipc_socket.c 3 Jul 2017 08:41:24 -
@@ -418,14 +418,14 @@ sosend(struct socket *so, struct mbuf *a
(sizeof(struct fdpass) / sizeof(int)));
}
-#definesnderr(errno) { error = errno; sounlock(s); goto release; }
+#definesnderr(errno) { error = errno; goto release; }
+ s = solock(so);
restart:
- if ((error = sblock(>so_snd, SBLOCKWAIT(flags), NULL)) != 0)
+ if ((error = sblock(so, >so_snd, SBLOCKWAIT(flags))) != 0)
goto out;
so->so_state |= SS_ISSENDING;
do {
- s = solock(so);
if (so->so_state & SS_CANTSENDMORE)
snderr(EPIPE);
if (so->so_error) {
@@ -455,12 +455,10 @@ restart:
sbunlock(>so_snd);
error = sbwait(so, >so_snd);
so->so_state &= ~SS_ISSENDING;
- sounlock(s);
if (error)
goto out;
goto restart;
}
- sounlock(s);
space -= clen;
do {
if (uio == NULL) {
@@ -471,8 +469,9 @@ restart:
if (flags & MSG_EOR)
top->m_flags |= M_EOR;
} else {
- error = m_getuio(, atomic,
- space, uio);
+ sounlock(s);
+ error = m_getuio(, atomic, space, uio);
+ s = solock(so);
if (error)
goto release;
space -= top->m_pkthdr.len;
@@ -480,7 +479,6 @@ restart:
if (flags & MSG_EOR)
top->m_flags |= M_EOR;
}
- s = solock(so);
if (resid == 0)
so->so_state &= ~SS_ISSENDING;
if (top && so->so_options & SO_ZEROIZE)
@@ -488,7 +486,6 @@ restart:
error = (*so->so_proto->pr_usrreq)(so,
(flags & MSG_OOB) ? PRU_SENDOOB : PRU_SEND,
top, addr, control, curproc);
- sounlock(s);
clen = 0;
control = NULL;
top = NULL;
@@ -501,6 +498,7 @@ release:
so->so_state &= ~SS_ISSENDING;
sbunlock(>so_snd);
out:
+ sounlock(s);
m_freem(top);
m_freem(control);
return (error);
@@ -670,9 +668,11 @@ bad:
*mp = NULL;
restart:
- if ((error = sblock(>so_rcv, SBLOCKWAIT(flags), NULL)) != 0)
- return (error);
s = solock(so);
+ if ((error = sblock(so, >so_rcv, SBLOCKWAIT(flags))) != 0) {
+ sounlock(s);
+ return (error);
+ }
m = so->so_rcv.sb_mb;
#ifdef SOCKET_SPLICE
@@ -1040,13 +1040,10 @@ sorflush(struct socket *so)
{
struct sockbuf *sb = >so_rcv;
struct protosw *pr = so->so_proto;
- sa_family_t af = pr->pr_domain->dom_family;
struct socket aso;
sb->sb_flags |= SB_NOINTR;
-
Re: sblock() & solock() ordering
On Mon, Jun 26, 2017 at 04:15:50PM +0200, Martin Pieuchot wrote:
> I'd like to enforce the following "lock" ordering: always hold the
> socket lock when calling sblock().
I was already wondering wether the "panic: receive 1" seen by stsp@
may be caused by an additional sleeping point in soreceive(). Now
we sleep less for the socket lock in the critical section.
> It also gets rid of sbsleep() and uses sosleep() instead.
Yes, much nicer.
> ok?
OK bluhm@
>
> Index: sys/socketvar.h
> ===
> RCS file: /cvs/src/sys/sys/socketvar.h,v
> retrieving revision 1.70
> diff -u -p -r1.70 socketvar.h
> --- sys/socketvar.h 26 Jun 2017 09:32:32 - 1.70
> +++ sys/socketvar.h 26 Jun 2017 14:01:31 -
> @@ -239,7 +239,7 @@ struct rwlock;
> * Unless SB_NOINTR is set on sockbuf, sleep is interruptible.
> * Returns error without lock if sleep is interrupted.
> */
> -int sblock(struct sockbuf *, int, struct rwlock *);
> +int sblock(struct socket *, struct sockbuf *, int);
>
> /* release lock on sockbuf sb */
> void sbunlock(struct sockbuf *);
> Index: kern/uipc_socket.c
> ===
> RCS file: /cvs/src/sys/kern/uipc_socket.c,v
> retrieving revision 1.189
> diff -u -p -r1.189 uipc_socket.c
> --- kern/uipc_socket.c26 Jun 2017 09:32:31 - 1.189
> +++ kern/uipc_socket.c26 Jun 2017 14:10:03 -
> @@ -418,14 +418,14 @@ sosend(struct socket *so, struct mbuf *a
> (sizeof(struct fdpass) / sizeof(int)));
> }
>
> -#define snderr(errno) { error = errno; sounlock(s); goto release; }
> +#define snderr(errno) { error = errno; goto release; }
>
> restart:
> - if ((error = sblock(>so_snd, SBLOCKWAIT(flags), NULL)) != 0)
> + s = solock(so);
> + if ((error = sblock(so, >so_snd, SBLOCKWAIT(flags))) != 0)
> goto out;
> so->so_state |= SS_ISSENDING;
> do {
> - s = solock(so);
> if (so->so_state & SS_CANTSENDMORE)
> snderr(EPIPE);
> if (so->so_error) {
> @@ -455,12 +455,10 @@ restart:
> sbunlock(>so_snd);
> error = sbwait(so, >so_snd);
> so->so_state &= ~SS_ISSENDING;
> - sounlock(s);
> if (error)
> goto out;
> goto restart;
> }
> - sounlock(s);
> space -= clen;
> do {
> if (uio == NULL) {
> @@ -471,8 +469,9 @@ restart:
> if (flags & MSG_EOR)
> top->m_flags |= M_EOR;
> } else {
> - error = m_getuio(, atomic,
> - space, uio);
> + sounlock(s);
> + error = m_getuio(, atomic, space, uio);
> + s = solock(so);
> if (error)
> goto release;
> space -= top->m_pkthdr.len;
> @@ -480,7 +479,6 @@ restart:
> if (flags & MSG_EOR)
> top->m_flags |= M_EOR;
> }
> - s = solock(so);
> if (resid == 0)
> so->so_state &= ~SS_ISSENDING;
> if (top && so->so_options & SO_ZEROIZE)
> @@ -488,7 +486,6 @@ restart:
> error = (*so->so_proto->pr_usrreq)(so,
> (flags & MSG_OOB) ? PRU_SENDOOB : PRU_SEND,
> top, addr, control, curproc);
> - sounlock(s);
> clen = 0;
> control = NULL;
> top = NULL;
> @@ -501,6 +498,7 @@ release:
> so->so_state &= ~SS_ISSENDING;
> sbunlock(>so_snd);
> out:
> + sounlock(s);
> m_freem(top);
> m_freem(control);
> return (error);
> @@ -670,9 +668,11 @@ bad:
> *mp = NULL;
>
> restart:
> - if ((error = sblock(>so_rcv, SBLOCKWAIT(flags), NULL)) != 0)
> - return (error);
> s = solock(so);
> + if ((error = sblock(so, >so_rcv, SBLOCKWAIT(flags))) != 0) {
> + sounlock(s);
> + return (error);
> + }
>
> m = so->so_rcv.sb_mb;
> #ifdef SOCKET_SPLICE
> @@ -1040,13 +1040,10 @@ sorflush(struct socket *so)
> {
> struct sockbuf *sb = >so_rcv;
> struct protosw *pr = so->so_proto;
> - sa_family_t af = pr->pr_domain->dom_family;
> struct socket aso;
>
> sb->sb_flags |= SB_NOINTR;
> - sblock(sb, M_WAITOK,
> - (af != PF_LOCAL && af != PF_ROUTE && af != PF_KEY) ?
> - : NULL);
> + sblock(so, sb,
sblock() & solock() ordering
I'd like to enforce the following "lock" ordering: always hold the
socket lock when calling sblock().
This would allow me to protect `so_state' in sosend() when setting the
SS_ISSENDING bit.
Diff below implements that. It also gets rid of sbsleep() and uses
sosleep() instead.
ok?
Index: sys/socketvar.h
===
RCS file: /cvs/src/sys/sys/socketvar.h,v
retrieving revision 1.70
diff -u -p -r1.70 socketvar.h
--- sys/socketvar.h 26 Jun 2017 09:32:32 - 1.70
+++ sys/socketvar.h 26 Jun 2017 14:01:31 -
@@ -239,7 +239,7 @@ struct rwlock;
* Unless SB_NOINTR is set on sockbuf, sleep is interruptible.
* Returns error without lock if sleep is interrupted.
*/
-int sblock(struct sockbuf *, int, struct rwlock *);
+int sblock(struct socket *, struct sockbuf *, int);
/* release lock on sockbuf sb */
void sbunlock(struct sockbuf *);
Index: kern/uipc_socket.c
===
RCS file: /cvs/src/sys/kern/uipc_socket.c,v
retrieving revision 1.189
diff -u -p -r1.189 uipc_socket.c
--- kern/uipc_socket.c 26 Jun 2017 09:32:31 - 1.189
+++ kern/uipc_socket.c 26 Jun 2017 14:10:03 -
@@ -418,14 +418,14 @@ sosend(struct socket *so, struct mbuf *a
(sizeof(struct fdpass) / sizeof(int)));
}
-#definesnderr(errno) { error = errno; sounlock(s); goto release; }
+#definesnderr(errno) { error = errno; goto release; }
restart:
- if ((error = sblock(>so_snd, SBLOCKWAIT(flags), NULL)) != 0)
+ s = solock(so);
+ if ((error = sblock(so, >so_snd, SBLOCKWAIT(flags))) != 0)
goto out;
so->so_state |= SS_ISSENDING;
do {
- s = solock(so);
if (so->so_state & SS_CANTSENDMORE)
snderr(EPIPE);
if (so->so_error) {
@@ -455,12 +455,10 @@ restart:
sbunlock(>so_snd);
error = sbwait(so, >so_snd);
so->so_state &= ~SS_ISSENDING;
- sounlock(s);
if (error)
goto out;
goto restart;
}
- sounlock(s);
space -= clen;
do {
if (uio == NULL) {
@@ -471,8 +469,9 @@ restart:
if (flags & MSG_EOR)
top->m_flags |= M_EOR;
} else {
- error = m_getuio(, atomic,
- space, uio);
+ sounlock(s);
+ error = m_getuio(, atomic, space, uio);
+ s = solock(so);
if (error)
goto release;
space -= top->m_pkthdr.len;
@@ -480,7 +479,6 @@ restart:
if (flags & MSG_EOR)
top->m_flags |= M_EOR;
}
- s = solock(so);
if (resid == 0)
so->so_state &= ~SS_ISSENDING;
if (top && so->so_options & SO_ZEROIZE)
@@ -488,7 +486,6 @@ restart:
error = (*so->so_proto->pr_usrreq)(so,
(flags & MSG_OOB) ? PRU_SENDOOB : PRU_SEND,
top, addr, control, curproc);
- sounlock(s);
clen = 0;
control = NULL;
top = NULL;
@@ -501,6 +498,7 @@ release:
so->so_state &= ~SS_ISSENDING;
sbunlock(>so_snd);
out:
+ sounlock(s);
m_freem(top);
m_freem(control);
return (error);
@@ -670,9 +668,11 @@ bad:
*mp = NULL;
restart:
- if ((error = sblock(>so_rcv, SBLOCKWAIT(flags), NULL)) != 0)
- return (error);
s = solock(so);
+ if ((error = sblock(so, >so_rcv, SBLOCKWAIT(flags))) != 0) {
+ sounlock(s);
+ return (error);
+ }
m = so->so_rcv.sb_mb;
#ifdef SOCKET_SPLICE
@@ -1040,13 +1040,10 @@ sorflush(struct socket *so)
{
struct sockbuf *sb = >so_rcv;
struct protosw *pr = so->so_proto;
- sa_family_t af = pr->pr_domain->dom_family;
struct socket aso;
sb->sb_flags |= SB_NOINTR;
- sblock(sb, M_WAITOK,
- (af != PF_LOCAL && af != PF_ROUTE && af != PF_KEY) ?
-: NULL);
+ sblock(so, sb, M_WAITOK);
socantrcvmore(so);
sbunlock(sb);
aso.so_proto = pr;
@@ -1094,11 +1091,13 @@ sosplice(struct socket *so, int fd, off_
/* If no fd is given, unsplice by removing existing link. */
if (fd < 0) {
+ s =
