[src/trunk]: src/sys/kern Fix the races of direct select()/poll():

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

details:   https://anonhg.NetBSD.org/src/rev/2471cddd59db
branches:  trunk
changeset: 768000:2471cddd59db
user:      hannken <hannken%NetBSD.org@localhost>
date:      Sat Aug 06 11:04:25 2011 +0000

description:
Fix the races of direct select()/poll():

- When sel_do_scan() restarts do a full initialization with selclear() so
  we start from an empty set without registered events.  Defer the
  evaluation of l_selret after selclear() and add the count of direct events
  to the count of events.

- For selscan()/pollscan() zero the output descriptors before we poll and
  for selscan() take the sc_lock before we change them.

- Change sel_setevents() to not count events already set.

Reviewed by: YAMAMOTO Takashi <yamt%netbsd.org@localhost>

Should fix PR #44763 (select/poll direct-set optimization seems racy)
       and PR #45187 (select(2) sometimes doesn't wakeup)

diffstat:

 sys/kern/sys_select.c |  54 ++++++++++++++++++++++++++++++++++----------------
 1 files changed, 37 insertions(+), 17 deletions(-)

diffs (163 lines):

diff -r 930da3c1d893 -r 2471cddd59db sys/kern/sys_select.c
--- a/sys/kern/sys_select.c     Sat Aug 06 11:02:41 2011 +0000
+++ b/sys/kern/sys_select.c     Sat Aug 06 11:04:25 2011 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: sys_select.c,v 1.33 2011/05/28 15:33:41 christos Exp $ */
+/*     $NetBSD: sys_select.c,v 1.34 2011/08/06 11:04:25 hannken Exp $  */
 
 /*-
  * Copyright (c) 2007, 2008, 2009, 2010 The NetBSD Foundation, Inc.
@@ -84,7 +84,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: sys_select.c,v 1.33 2011/05/28 15:33:41 christos Exp $");
+__KERNEL_RCSID(0, "$NetBSD: sys_select.c,v 1.34 2011/08/06 11:04:25 hannken Exp $");
 
 #include <sys/param.h>
 #include <sys/systm.h>
@@ -235,18 +235,19 @@
        sc = curcpu()->ci_data.cpu_selcluster;
        lock = sc->sc_lock;
        l->l_selcluster = sc;
-       SLIST_INIT(&l->l_selwait);
-
-       l->l_selret = 0;
        if (op == SELOP_SELECT) {
                l->l_selbits = fds;
                l->l_selni = ni;
        } else {
                l->l_selbits = NULL;
        }
+
        for (;;) {
                int ncoll;
 
+               SLIST_INIT(&l->l_selwait);
+               l->l_selret = 0;
+
                /*
                 * No need to lock.  If this is overwritten by another value
                 * while scanning, we will retry below.  We only need to see
@@ -276,18 +277,18 @@
                if (__predict_false(sc->sc_ncoll != ncoll)) {
                        /* Collision: perform re-scan. */
                        mutex_spin_exit(lock);
+                       selclear();
                        continue;
                }
                if (__predict_true(l->l_selflag == SEL_EVENT)) {
                        /* Events occured, they are set directly. */
                        mutex_spin_exit(lock);
-                       KASSERT(l->l_selret != 0);
-                       *retval = l->l_selret;
                        break;
                }
                if (__predict_true(l->l_selflag == SEL_RESET)) {
                        /* Events occured, but re-scan is requested. */
                        mutex_spin_exit(lock);
+                       selclear();
                        continue;
                }
                /* Nothing happen, therefore - sleep. */
@@ -304,6 +305,12 @@
        }
        selclear();
 
+       /* Add direct events if any. */
+       if (l->l_selflag == SEL_EVENT) {
+               KASSERT(l->l_selret != 0);
+               *retval += l->l_selret;
+       }
+
        if (__predict_false(mask))
                sigsuspendteardown(l);
 
@@ -371,11 +378,14 @@
        fd_mask *ibitp, *obitp;
        int msk, i, j, fd, n;
        file_t *fp;
+       kmutex_t *lock;
 
+       lock = curlwp->l_selcluster->sc_lock;
        ibitp = (fd_mask *)(bits + ni * 0);
        obitp = (fd_mask *)(bits + ni * 3);
        n = 0;
 
+       memset(obitp, 0, ni * 3);
        for (msk = 0; msk < 3; msk++) {
                for (i = 0; i < nfd; i += NFDBITS) {
                        fd_mask ibits, obits;
@@ -397,7 +407,12 @@
                                }
                                fd_putfile(fd);
                        }
-                       *obitp++ = obits;
+                       if (obits != 0) {
+                               mutex_spin_enter(lock);
+                               *obitp |= obits;
+                               mutex_spin_exit(lock);
+                       }
+                       obitp++;
                }
        }
        *retval = n;
@@ -505,14 +520,14 @@
 pollscan(struct pollfd *fds, const int nfd, register_t *retval)
 {
        file_t *fp;
-       int i, n = 0;
+       int i, n = 0, revents;
 
        for (i = 0; i < nfd; i++, fds++) {
+               fds->revents = 0;
                if (fds->fd < 0) {
-                       fds->revents = 0;
+                       revents = 0;
                } else if ((fp = fd_getfile(fds->fd)) == NULL) {
-                       fds->revents = POLLNVAL;
-                       n++;
+                       revents = POLLNVAL;
                } else {
                        /*
                         * Perform poll: registers select request or returns
@@ -520,12 +535,14 @@
                         * selrecord(), which is a pointer to struct pollfd.
                         */
                        curlwp->l_selrec = (uintptr_t)fds;
-                       fds->revents = (*fp->f_ops->fo_poll)(fp,
+                       revents = (*fp->f_ops->fo_poll)(fp,
                            fds->events | POLLERR | POLLHUP);
-                       if (fds->revents != 0)
-                               n++;
                        fd_putfile(fds->fd);
                }
+               if (revents) {
+                       fds->revents = revents;
+                       n++;
+               }
        }
        *retval = n;
        return (0);
@@ -626,7 +643,9 @@
                int n;
 
                for (n = 0; n < 3; n++) {
-                       if ((fds[idx] & fbit) != 0 && (sel_flag[n] & events)) {
+                       if ((fds[idx] & fbit) != 0 &&
+                           (ofds[idx] & fbit) == 0 &&
+                           (sel_flag[n] & events)) {
                                ofds[idx] |= fbit;
                                ret++;
                        }
@@ -638,8 +657,9 @@
                int revents = events & (pfd->events | POLLERR | POLLHUP);
 
                if (revents) {
+                       if (pfd->revents == 0)
+                               ret = 1;
                        pfd->revents |= revents;
-                       ret = 1;
                }
        }
        /* Check whether there are any events to return. */