[src/trunk]: src/sys Fix a thundering herd problem in pipes.

[dholland <dholland%NetBSD.org@localhost>]

details:   https://anonhg.NetBSD.org/src/rev/9a31ee1b1d05
branches:  trunk
changeset: 980169:9a31ee1b1d05
user:      dholland <dholland%NetBSD.org@localhost>
date:      Mon Jan 25 19:21:11 2021 +0000

description:
Fix a thundering herd problem in pipes.

Wake only one waiter when data becomes available, not all of them.
Waking them all is not a usual case, but turns up with make's job
token pipes. (Probably make's job signalling scheme should also be
revised, assuming rillig hasn't already done that, but that's a
separate issue.)

This change will not do us much good for the moment because we don't
distinguish cv_signal from cv_broadcast for interruptible sleeps, but
that's also a separate problem.

Seen on FreeBSD; from mjg at freebsd a couple months ago. Patch was
mine (iirc) but the real work in this sort of thing is discovering the
problem.

diffstat:

 sys/kern/sys_pipe.c |  19 ++++++++++++-------
 sys/sys/pipe.h      |   6 +++---
 2 files changed, 15 insertions(+), 10 deletions(-)

diffs (79 lines):

diff -r df9384463dd9 -r 9a31ee1b1d05 sys/kern/sys_pipe.c
--- a/sys/kern/sys_pipe.c       Mon Jan 25 19:10:57 2021 +0000
+++ b/sys/kern/sys_pipe.c       Mon Jan 25 19:21:11 2021 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: sys_pipe.c,v 1.151 2020/12/11 03:00:09 thorpej Exp $   */
+/*     $NetBSD: sys_pipe.c,v 1.152 2021/01/25 19:21:11 dholland Exp $  */
 
 /*-
  * Copyright (c) 2003, 2007, 2008, 2009 The NetBSD Foundation, Inc.
@@ -68,7 +68,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: sys_pipe.c,v 1.151 2020/12/11 03:00:09 thorpej Exp $");
+__KERNEL_RCSID(0, "$NetBSD: sys_pipe.c,v 1.152 2021/01/25 19:21:11 dholland Exp $");
 
 #include <sys/param.h>
 #include <sys/systm.h>
@@ -344,13 +344,19 @@
        KASSERT(mutex_owned(pipe->pipe_lock));
 
        while (pipe->pipe_state & PIPE_LOCKFL) {
-               pipe->pipe_state |= PIPE_LWANT;
+               pipe->pipe_waiters++;
+               KASSERT(pipe->pipe_waiters != 0); /* just in case */
                if (catch_p) {
                        error = cv_wait_sig(&pipe->pipe_lkcv, pipe->pipe_lock);
-                       if (error != 0)
+                       if (error != 0) {
+                               KASSERT(pipe->pipe_waiters > 0);
+                               pipe->pipe_waiters--;
                                return error;
+                       }
                } else
                        cv_wait(&pipe->pipe_lkcv, pipe->pipe_lock);
+               KASSERT(pipe->pipe_waiters > 0);
+               pipe->pipe_waiters--;
        }
 
        pipe->pipe_state |= PIPE_LOCKFL;
@@ -368,9 +374,8 @@
        KASSERT(pipe->pipe_state & PIPE_LOCKFL);
 
        pipe->pipe_state &= ~PIPE_LOCKFL;
-       if (pipe->pipe_state & PIPE_LWANT) {
-               pipe->pipe_state &= ~PIPE_LWANT;
-               cv_broadcast(&pipe->pipe_lkcv);
+       if (pipe->pipe_waiters > 0) {
+               cv_signal(&pipe->pipe_lkcv);
        }
 }
 
diff -r df9384463dd9 -r 9a31ee1b1d05 sys/sys/pipe.h
--- a/sys/sys/pipe.h    Mon Jan 25 19:10:57 2021 +0000
+++ b/sys/sys/pipe.h    Mon Jan 25 19:21:11 2021 +0000
@@ -1,4 +1,4 @@
-/* $NetBSD: pipe.h,v 1.37 2020/06/25 14:22:19 jdolecek Exp $ */
+/* $NetBSD: pipe.h,v 1.38 2021/01/25 19:21:11 dholland Exp $ */
 
 /*
  * Copyright (c) 1996 John S. Dyson
@@ -80,8 +80,7 @@
 #define PIPE_SIGNALR   0x020   /* Do selwakeup() on read(2) */
 #define        PIPE_LOCKFL     0x100   /* Process has exclusive access to
                                   pointers/data. */
-#define        PIPE_LWANT      0x200   /* Process wants exclusive access to
-                                  pointers/data. */
+/*     unused          0x200   */
 #define        PIPE_RESTART    0x400   /* Return ERESTART to blocked syscalls */
 
 /*
@@ -100,6 +99,7 @@
        struct  timespec pipe_mtime;    /* time of last modify */
        struct  timespec pipe_btime;    /* time of creation */
        pid_t   pipe_pgid;              /* process group for sigio */
+       u_int   pipe_waiters;           /* number of waiters pending */
        struct  pipe *pipe_peer;        /* link with other direction */
        u_int   pipe_state;             /* pipe status info */
        int     pipe_busy;              /* busy flag, to handle rundown */