On 26/10/20(Mon) 11:57, Scott Cheloha wrote: > On Mon, Oct 12, 2020 at 11:11:36AM +0200, Martin Pieuchot wrote: > [...] > > +/* > > + * Scan the kqueue, blocking if necessary until the target time is reached. > > + * If tsp is NULL we block indefinitely. If tsp->ts_secs/nsecs are both > > + * 0 we do not block at all. > > + */ > > int > > kqueue_scan(struct kqueue_scan_state *scan, int maxevents, > > - struct kevent *ulistp, struct timespec *tsp, struct kevent *kev, > > - struct proc *p, int *retval) > > + struct kevent *kevp, struct timespec *tsp, struct proc *p, int *errorp) > > Is there any reason to change these argument names?
The array is no longer a user specified list because the interface is now used by other kernel consumers. > > @@ -618,6 +631,8 @@ dopselect(struct proc *p, int nd, fd_set > > pobits[2] = (fd_set *)&bits[5]; > > } > > > > + kqpoll_init(p); > > + > > #define getbits(name, x) \ > > if (name && (error = copyin(name, pibits[x], ni))) \ > > goto done; > > @@ -636,43 +651,63 @@ dopselect(struct proc *p, int nd, fd_set > > if (sigmask) > > dosigsuspend(p, *sigmask &~ sigcantmask); > > > > -retry: > > - ncoll = nselcoll; > > - atomic_setbits_int(&p->p_flag, P_SELECT); > > - error = selscan(p, pibits[0], pobits[0], nd, ni, retval); > > - if (error || *retval) > > + /* Register kqueue events */ > > + if ((error = pselregister(p, pibits[0], nd, ni, &nevents) != 0)) > > goto done; > > - if (timeout == NULL || timespecisset(timeout)) { > > - if (timeout != NULL) { > > - getnanouptime(&start); > > - nsecs = MIN(TIMESPEC_TO_NSEC(timeout), MAXTSLP); > > - } else > > - nsecs = INFSLP; > > - s = splhigh(); > > - if ((p->p_flag & P_SELECT) == 0 || nselcoll != ncoll) { > > - splx(s); > > - goto retry; > > - } > > - atomic_clearbits_int(&p->p_flag, P_SELECT); > > - error = tsleep_nsec(&selwait, PSOCK | PCATCH, "select", nsecs); > > I need to clarify something. > > My understanding of the current state of poll/select is that all > threads wait on the same channel, selwait. Sometimes, a thread will > wakeup(9) *all* threads waiting on that channel. When this happens, > most of the sleeping threads will recheck their conditions, see that > nothing has changed, and go back to sleep. > > Right? > > Is that spurious wakeup case going away with this diff? That is, when > a thread is sleeping in poll/select it will only be woken up by > another thread if the condition for one of the descriptors of interest > changes. Your understanding matches mine, the removal of spurious wakeup might be beneficial to some programs. I see this as a pro of this implementation. > > > - splx(s); > > - if (timeout != NULL) { > > - getnanouptime(&stop); > > - timespecsub(&stop, &start, &elapsed); > > - timespecsub(timeout, &elapsed, timeout); > > - if (timeout->tv_sec < 0) > > - timespecclear(timeout); > > - } > > - if (error == 0 || error == EWOULDBLOCK) > > - goto retry; > > + > > + /* > > + * The poll/select family of syscalls has been designed to > > + * block when file descriptors are not available, even if > > + * there's nothing to wait for. > > + */ > > + if (nevents == 0) { > > + uint64_t nsecs = INFSLP; > > + > > + if (timeout != NULL) > > + nsecs = MAX(1, MIN(TIMESPEC_TO_NSEC(timeout), MAXTSLP)); > > + > > + error = tsleep_nsec(&p->p_kq, PSOCK | PCATCH, "kqsel", nsecs); > > + /* select is not restarted after signals... */ > > + if (error == ERESTART) > > + error = EINTR; > > + if (error == EWOULDBLOCK) > > + error = 0; > > + goto done; > > + } > > This is still wrong. If you want to isolate this case you can't block > when the timeout is empty: Thanks, fixed. > > Index: sys/proc.h > > =================================================================== > > RCS file: /cvs/src/sys/sys/proc.h,v > > retrieving revision 1.300 > > diff -u -p -r1.300 proc.h > > --- sys/proc.h 16 Sep 2020 08:01:15 -0000 1.300 > > +++ sys/proc.h 12 Oct 2020 08:56:21 -0000 > > @@ -320,6 +320,7 @@ struct process { > > > > struct kcov_dev; > > struct lock_list_entry; > > +struct kqueue; > > > > struct p_inentry { > > u_long ie_serial; > > @@ -382,6 +383,8 @@ struct proc { > > struct plimit *p_limit; /* [l] read ref. of p_p->ps_limit */ > > struct kcov_dev *p_kd; /* kcov device handle */ > > struct lock_list_entry *p_sleeplocks; /* WITNESS lock tracking */ > > + struct kqueue *p_kq; /* for select/poll */ > > + unsigned long p_kq_serial; /* for select/poll */ > > arc4random(9) returns a uint32_t. Why do you want an unsigned long here? To make sure the field has the same size as "void *". See how it is used in EV_SET().