Replace the global pipe_lock with a more fine-grained lock per pipe

pair. One lock per pipe pair is used to guarantee exclusive access to
both ends of a pipe.

Thanks to millert@ and tedu@ for the feedback and ok visa@

1 files changed, 83 insertions, 60 deletions

diff --git a/sys/kern/sys_pipe.c b/sys/kern/sys_pipe.c
index d2ab731d9de..f4e00bfa056 100644
--- a/sys/kern/sys_pipe.c
+++ b/sys/kern/sys_pipe.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: sys_pipe.c,v 1.110 2020/01/08 16:27:41 visa Exp $	*/
+/*	$OpenBSD: sys_pipe.c,v 1.111 2020/01/09 18:54:33 anton Exp $	*/
 
 /*
  * Copyright (c) 1996 John S. Dyson
@@ -104,11 +104,7 @@ unsigned int nbigpipe;
 static unsigned int amountpipekva;
 
 struct pool pipe_pool;
-
-/*
- * Global lock protecting fields of `struct pipe'.
- */
-struct rwlock pipe_lock = RWLOCK_INITIALIZER("pipeglk");
+struct pool pipe_lock_pool;
 
 int	dopipe(struct proc *, int *, int);
 void	pipeselwakeup(struct pipe *);
@@ -158,16 +154,33 @@ dopipe(struct proc *p, int *ufds, int flags)
 	struct filedesc *fdp = p->p_fd;
 	struct file *rf, *wf;
 	struct pipe *rpipe, *wpipe = NULL;
+	struct rwlock *lock;
 	int fds[2], cloexec, error;
 
 	cloexec = (flags & O_CLOEXEC) ? UF_EXCLOSE : 0;
 
-	if (((rpipe = pipe_create()) == NULL) ||
-	    ((wpipe = pipe_create()) == NULL)) {
+	if ((rpipe = pipe_create()) == NULL) {
 		error = ENOMEM;
 		goto free1;
 	}
 
+	/*
+	 * One lock is used per pipe pair in order to obtain exclusive access to
+	 * the pipe pair.
+	 */
+	lock = pool_get(&pipe_lock_pool, PR_WAITOK);
+	rw_init(lock, "pipelk");
+	rpipe->pipe_lock = lock;
+
+	if ((wpipe = pipe_create()) == NULL) {
+		error = ENOMEM;
+		goto free1;
+	}
+	wpipe->pipe_lock = lock;
+
+	rpipe->pipe_peer = wpipe;
+	wpipe->pipe_peer = rpipe;
+
 	fdplock(fdp);
 
 	error = falloc(p, &rf, &fds[0]);
@@ -186,9 +199,6 @@ dopipe(struct proc *p, int *ufds, int flags)
 	wf->f_data = wpipe;
 	wf->f_ops = &pipeops;
 
-	rpipe->pipe_peer = wpipe;
-	wpipe->pipe_peer = rpipe;
-
 	fdinsert(fdp, fds[0], cloexec, rf);
 	fdinsert(fdp, fds[1], cloexec, wf);
 
@@ -290,7 +300,7 @@ pipe_peer(struct pipe *cpipe)
 {
 	struct pipe *peer;
 
-	rw_assert_anylock(&pipe_lock);
+	rw_assert_anylock(cpipe->pipe_lock);
 
 	peer = cpipe->pipe_peer;
 	if (peer == NULL || (peer->pipe_state & PIPE_EOF))
@@ -306,11 +316,11 @@ pipe_iolock(struct pipe *cpipe)
 {
 	int error;
 
-	rw_assert_wrlock(&pipe_lock);
+	rw_assert_wrlock(cpipe->pipe_lock);
 
 	while (cpipe->pipe_state & PIPE_LOCK) {
 		cpipe->pipe_state |= PIPE_LWANT;
-		error = rwsleep_nsec(cpipe, &pipe_lock, PRIBIO | PCATCH,
+		error = rwsleep_nsec(cpipe, cpipe->pipe_lock, PRIBIO | PCATCH,
 		    "pipeiolk", INFSLP);
 		if (error)
 			return (error);
@@ -325,7 +335,7 @@ pipe_iolock(struct pipe *cpipe)
 void
 pipe_iounlock(struct pipe *cpipe)
 {
-	rw_assert_wrlock(&pipe_lock);
+	rw_assert_wrlock(cpipe->pipe_lock);
 	KASSERT(cpipe->pipe_state & PIPE_LOCK);
 
 	cpipe->pipe_state &= ~PIPE_LOCK;
@@ -350,7 +360,8 @@ pipe_iosleep(struct pipe *cpipe, const char *wmesg)
 	int error;
 
 	pipe_iounlock(cpipe);
-	error = rwsleep_nsec(cpipe, &pipe_lock, PRIBIO | PCATCH, wmesg, INFSLP);
+	error = rwsleep_nsec(cpipe, cpipe->pipe_lock, PRIBIO | PCATCH, wmesg,
+	    INFSLP);
 	if (error)
 		return (error);
 	return (pipe_iolock(cpipe));
@@ -359,7 +370,7 @@ pipe_iosleep(struct pipe *cpipe, const char *wmesg)
 void
 pipeselwakeup(struct pipe *cpipe)
 {
-	rw_assert_wrlock(&pipe_lock);
+	rw_assert_wrlock(cpipe->pipe_lock);
 
 	KERNEL_LOCK();
 
@@ -384,13 +395,13 @@ pipe_read(struct file *fp, struct uio *uio, int fflags)
 	size_t nread = 0, size;
 	int error;
 
-	rw_enter_write(&pipe_lock);
+	rw_enter_write(rpipe->pipe_lock);
 	++rpipe->pipe_busy;
 	error = pipe_iolock(rpipe);
 	if (error) {
 		--rpipe->pipe_busy;
 		pipe_rundown(rpipe);
-		rw_exit_write(&pipe_lock);
+		rw_exit_write(rpipe->pipe_lock);
 		return (error);
 	}
 
@@ -402,10 +413,10 @@ pipe_read(struct file *fp, struct uio *uio, int fflags)
 				size = rpipe->pipe_buffer.cnt;
 			if (size > uio->uio_resid)
 				size = uio->uio_resid;
-			rw_exit_write(&pipe_lock);
+			rw_exit_write(rpipe->pipe_lock);
 			error = uiomove(&rpipe->pipe_buffer.buffer[rpipe->pipe_buffer.out],
 					size, uio);
-			rw_enter_write(&pipe_lock);
+			rw_enter_write(rpipe->pipe_lock);
 			if (error) {
 				break;
 			}
@@ -473,7 +484,7 @@ unlocked_error:
 	if ((rpipe->pipe_buffer.size - rpipe->pipe_buffer.cnt) >= PIPE_BUF)
 		pipeselwakeup(rpipe);
 
-	rw_exit_write(&pipe_lock);
+	rw_exit_write(rpipe->pipe_lock);
 	return (error);
 }
 
@@ -481,15 +492,16 @@ int
 pipe_write(struct file *fp, struct uio *uio, int fflags)
 {
 	struct pipe *rpipe = fp->f_data, *wpipe;
+	struct rwlock *lock = rpipe->pipe_lock;
 	size_t orig_resid;
 	int error;
 
-	rw_enter_write(&pipe_lock);
+	rw_enter_write(lock);
 	wpipe = pipe_peer(rpipe);
 
 	/* Detect loss of pipe read side, issue SIGPIPE if lost. */
 	if (wpipe == NULL) {
-		rw_exit_write(&pipe_lock);
+		rw_exit_write(lock);
 		return (EPIPE);
 	}
 
@@ -498,7 +510,7 @@ pipe_write(struct file *fp, struct uio *uio, int fflags)
 	if (error) {
 		--wpipe->pipe_busy;
 		pipe_rundown(wpipe);
-		rw_exit_write(&pipe_lock);
+		rw_exit_write(lock);
 		return (error);
 	}
 
@@ -560,10 +572,10 @@ pipe_write(struct file *fp, struct uio *uio, int fflags)
 
 			/* Transfer first segment */
 
-			rw_exit_write(&pipe_lock);
+			rw_exit_write(lock);
 			error = uiomove(&wpipe->pipe_buffer.buffer[wpipe->pipe_buffer.in],
 					segsize, uio);
-			rw_enter_write(&pipe_lock);
+			rw_enter_write(lock);
 
 			if (error == 0 && segsize < size) {
 				/*
@@ -577,10 +589,10 @@ pipe_write(struct file *fp, struct uio *uio, int fflags)
 					panic("Expected pipe buffer wraparound disappeared");
 #endif
 
-				rw_exit_write(&pipe_lock);
+				rw_exit_write(lock);
 				error = uiomove(&wpipe->pipe_buffer.buffer[0],
 						size - segsize, uio);
-				rw_enter_write(&pipe_lock);
+				rw_enter_write(lock);
 			}
 			if (error == 0) {
 				wpipe->pipe_buffer.in += size;
@@ -664,7 +676,7 @@ unlocked_error:
 	if (wpipe->pipe_buffer.cnt)
 		pipeselwakeup(wpipe);
 
-	rw_exit_write(&pipe_lock);
+	rw_exit_write(lock);
 	return (error);
 }
 
@@ -677,7 +689,7 @@ pipe_ioctl(struct file *fp, u_long cmd, caddr_t data, struct proc *p)
 	struct pipe *mpipe = fp->f_data;
 	int error = 0;
 
-	rw_enter_write(&pipe_lock);
+	rw_enter_write(mpipe->pipe_lock);
 
 	switch (cmd) {
 
@@ -712,7 +724,7 @@ pipe_ioctl(struct file *fp, u_long cmd, caddr_t data, struct proc *p)
 		error = ENOTTY;
 	}
 
-	rw_exit_write(&pipe_lock);
+	rw_exit_write(mpipe->pipe_lock);
 
 	return (error);
 }
@@ -720,11 +732,11 @@ pipe_ioctl(struct file *fp, u_long cmd, caddr_t data, struct proc *p)
 int
 pipe_poll(struct file *fp, int events, struct proc *p)
 {
-	struct pipe *rpipe = fp->f_data;
-	struct pipe *wpipe;
+	struct pipe *rpipe = fp->f_data, *wpipe;
+	struct rwlock *lock = rpipe->pipe_lock;
 	int revents = 0;
 
-	rw_enter_write(&pipe_lock);
+	rw_enter_write(lock);
 	wpipe = pipe_peer(rpipe);
 
 	if (events & (POLLIN | POLLRDNORM)) {
@@ -752,7 +764,7 @@ pipe_poll(struct file *fp, int events, struct proc *p)
 		}
 	}
 
-	rw_exit_write(&pipe_lock);
+	rw_exit_write(lock);
 
 	return (revents);
 }
@@ -764,7 +776,7 @@ pipe_stat(struct file *fp, struct stat *ub, struct proc *p)
 
 	memset(ub, 0, sizeof(*ub));
 
-	rw_enter_read(&pipe_lock);
+	rw_enter_read(pipe->pipe_lock);
 	ub->st_mode = S_IFIFO;
 	ub->st_blksize = pipe->pipe_buffer.size;
 	ub->st_size = pipe->pipe_buffer.cnt;
@@ -777,7 +789,7 @@ pipe_stat(struct file *fp, struct stat *ub, struct proc *p)
 	ub->st_ctim.tv_nsec = pipe->pipe_ctime.tv_nsec;
 	ub->st_uid = fp->f_cred->cr_uid;
 	ub->st_gid = fp->f_cred->cr_gid;
-	rw_exit_read(&pipe_lock);
+	rw_exit_read(pipe->pipe_lock);
 	/*
 	 * Left as 0: st_dev, st_ino, st_nlink, st_rdev, st_flags, st_gen.
 	 * XXX (st_dev, st_ino) should be unique.
@@ -828,11 +840,12 @@ void
 pipe_destroy(struct pipe *cpipe)
 {
 	struct pipe *ppipe;
+	struct rwlock *lock = NULL;
 
 	if (cpipe == NULL)
 		return;
 
-	rw_enter_write(&pipe_lock);
+	rw_enter_write(cpipe->pipe_lock);
 
 	pipeselwakeup(cpipe);
 	sigio_free(&cpipe->pipe_sigio);
@@ -845,7 +858,7 @@ pipe_destroy(struct pipe *cpipe)
 	while (cpipe->pipe_busy) {
 		wakeup(cpipe);
 		cpipe->pipe_state |= PIPE_WANTD;
-		rwsleep_nsec(cpipe, &pipe_lock, PRIBIO, "pipecl", INFSLP);
+		rwsleep_nsec(cpipe, cpipe->pipe_lock, PRIBIO, "pipecl", INFSLP);
 	}
 
 	/* Disconnect from peer. */
@@ -855,11 +868,19 @@ pipe_destroy(struct pipe *cpipe)
 		ppipe->pipe_state |= PIPE_EOF;
 		wakeup(ppipe);
 		ppipe->pipe_peer = NULL;
+	} else {
+		/*
+		 * Peer already gone. This is last reference to the pipe lock
+		 * and it must therefore be freed below.
+		 */
+		lock = cpipe->pipe_lock;
 	}
 
-	rw_exit_write(&pipe_lock);
+	rw_exit_write(cpipe->pipe_lock);
 
 	pipe_buffer_free(cpipe);
+	if (lock != NULL)
+		pool_put(&pipe_lock_pool, lock);
 	pool_put(&pipe_pool, cpipe);
 }
 
@@ -869,7 +890,7 @@ pipe_destroy(struct pipe *cpipe)
 int
 pipe_rundown(struct pipe *cpipe)
 {
-	rw_assert_wrlock(&pipe_lock);
+	rw_assert_wrlock(cpipe->pipe_lock);
 
 	if (cpipe->pipe_busy > 0 || (cpipe->pipe_state & PIPE_WANTD) == 0)
 		return (0);
@@ -883,11 +904,11 @@ pipe_rundown(struct pipe *cpipe)
 int
 pipe_kqfilter(struct file *fp, struct knote *kn)
 {
-	struct pipe *rpipe = kn->kn_fp->f_data;
-	struct pipe *wpipe;
+	struct pipe *rpipe = kn->kn_fp->f_data, *wpipe;
+	struct rwlock *lock = rpipe->pipe_lock;
 	int error = 0;
 
-	rw_enter_write(&pipe_lock);
+	rw_enter_write(lock);
 	wpipe = pipe_peer(rpipe);
 
 	switch (kn->kn_filter) {
@@ -908,7 +929,7 @@ pipe_kqfilter(struct file *fp, struct knote *kn)
 		error = EINVAL;
 	}
 
-	rw_exit_write(&pipe_lock);
+	rw_exit_write(lock);
 
 	return (error);
 }
@@ -916,10 +937,10 @@ pipe_kqfilter(struct file *fp, struct knote *kn)
 void
 filt_pipedetach(struct knote *kn)
 {
-	struct pipe *rpipe = kn->kn_fp->f_data;
-	struct pipe *wpipe;
+	struct pipe *rpipe = kn->kn_fp->f_data, *wpipe;
+	struct rwlock *lock = rpipe->pipe_lock;
 
-	rw_enter_write(&pipe_lock);
+	rw_enter_write(lock);
 	wpipe = pipe_peer(rpipe);
 
 	switch (kn->kn_filter) {
@@ -933,30 +954,30 @@ filt_pipedetach(struct knote *kn)
 		break;
 	}
 
-	rw_exit_write(&pipe_lock);
+	rw_exit_write(lock);
 }
 
 int
 filt_piperead(struct knote *kn, long hint)
 {
-	struct pipe *rpipe = kn->kn_fp->f_data;
-	struct pipe *wpipe;
+	struct pipe *rpipe = kn->kn_fp->f_data, *wpipe;
+	struct rwlock *lock = rpipe->pipe_lock;
 
 	if ((hint & NOTE_SUBMIT) == 0)
-		rw_enter_read(&pipe_lock);
+		rw_enter_read(lock);
 	wpipe = pipe_peer(rpipe);
 
 	kn->kn_data = rpipe->pipe_buffer.cnt;
 
 	if ((rpipe->pipe_state & PIPE_EOF) || wpipe == NULL) {
 		if ((hint & NOTE_SUBMIT) == 0)
-			rw_exit_read(&pipe_lock);
+			rw_exit_read(lock);
 		kn->kn_flags |= EV_EOF; 
 		return (1);
 	}
 
 	if ((hint & NOTE_SUBMIT) == 0)
-		rw_exit_read(&pipe_lock);
+		rw_exit_read(lock);
 
 	return (kn->kn_data > 0);
 }
@@ -964,16 +985,16 @@ filt_piperead(struct knote *kn, long hint)
 int
 filt_pipewrite(struct knote *kn, long hint)
 {
-	struct pipe *rpipe = kn->kn_fp->f_data;
-	struct pipe *wpipe;
+	struct pipe *rpipe = kn->kn_fp->f_data, *wpipe;
+	struct rwlock *lock = rpipe->pipe_lock;
 
 	if ((hint & NOTE_SUBMIT) == 0)
-		rw_enter_read(&pipe_lock);
+		rw_enter_read(lock);
 	wpipe = pipe_peer(rpipe);
 
 	if (wpipe == NULL) {
 		if ((hint & NOTE_SUBMIT) == 0)
-			rw_exit_read(&pipe_lock);
+			rw_exit_read(lock);
 		kn->kn_data = 0;
 		kn->kn_flags |= EV_EOF; 
 		return (1);
@@ -981,7 +1002,7 @@ filt_pipewrite(struct knote *kn, long hint)
 	kn->kn_data = wpipe->pipe_buffer.size - wpipe->pipe_buffer.cnt;
 
 	if ((hint & NOTE_SUBMIT) == 0)
-		rw_exit_read(&pipe_lock);
+		rw_exit_read(lock);
 
 	return (kn->kn_data >= PIPE_BUF);
 }
@@ -991,4 +1012,6 @@ pipe_init(void)
 {
 	pool_init(&pipe_pool, sizeof(struct pipe), 0, IPL_MPFLOOR, PR_WAITOK,
 	    "pipepl", NULL);
+	pool_init(&pipe_lock_pool, sizeof(struct rwlock), 0, IPL_MPFLOOR,
+	    PR_WAITOK, "pipelkpl", NULL);
 }