On 02/10/20(Fri) 19:09, Scott Cheloha wrote:
> On Fri, Oct 02, 2020 at 12:19:35PM +0200, Martin Pieuchot wrote:
> > @@ -635,12 +642,39 @@ sys_kevent(struct proc *p, void *v, regi
> > goto done;
> > }
> >
> > +
> > KQREF(kq);
> > FRELE(fp, p);
> > - error = kqueue_scan(kq, SCARG(uap, nevents), SCARG(uap, eventlist),
> > - tsp, kev, p, &n);
> > + /*
> > + * Collect as many events as we can. The timeout on successive
> > + * loops is disabled (kqueue_scan() becomes non-blocking).
> > + */
> > + total = 0;
> > + error = 0;
> > + kqueue_scan_setup(&scan, kq);
> > + while ((n = SCARG(uap, nevents) - total) > 0) {
> > + if (n > nitems(kev))
> > + n = nitems(kev);
> > + ready = kqueue_scan(&scan, n, kev, tsp, p, &error);
> > + if (ready == 0)
> > + break;
> > + error = copyout(kev, SCARG(uap, eventlist) + total,
> > + sizeof(struct kevent) * ready);
> > +#ifdef KTRACE
> > + if (KTRPOINT(p, KTR_STRUCT))
> > + ktrevent(p, kev, ready);
> > +#endif
> > + total += ready;
> > + if (error || ready < n)
> > + break;
> > + tsp = &ts; /* successive loops non-blocking */
> > + timespecclear(tsp);
>
> Here, this. Why do we force a non-blocking loop the second time?
If there's a second time that implies the first time already reported
some events so there's already something to return to userland. In that
case we just want to gather the events that were not collected the first
time and not sleep again.
> > + }
> > + kqueue_scan_finish(&scan);
> > KQRELE(kq);
> > - *retval = n;
> > + if (error == EWOULDBLOCK)
> > + error = 0;
> > + *retval = total;
> > return (error);
> >
> > done:
> > @@ -894,24 +928,22 @@ kqueue_sleep(struct kqueue *kq, struct t
> > return (error);
> > }
> >
> > +/*
> > + * 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 *kq, int maxevents, struct kevent *ulistp,
> > - struct timespec *tsp, struct kevent *kev, struct proc *p, int *retval)
> > +kqueue_scan(struct kqueue_scan_state *scan, int maxevents,
> > + struct kevent *kevp, struct timespec *tsp, struct proc *p, int *errorp)
> > {
> > - struct kevent *kevp;
> > - struct knote mend, mstart, *kn;
> > - int s, count, nkev, error = 0;
> > -
> > - nkev = 0;
> > - kevp = kev;
> > + struct knote *kn;
> > + struct kqueue *kq = scan->kqs_kq;
> > + int s, count, nkev = 0, error = 0;
> >
> > count = maxevents;
> > if (count == 0)
> > goto done;
> > -
> > - memset(&mstart, 0, sizeof(mstart));
> > - memset(&mend, 0, sizeof(mend));
> > -
> > retry:
> > KASSERT(count == maxevents);
> > KASSERT(nkev == 0);
> > @@ -923,7 +955,8 @@ retry:
> >
> > s = splhigh();
> > if (kq->kq_count == 0) {
> > - if (tsp != NULL && !timespecisset(tsp)) {
> > + if ((tsp != NULL && !timespecisset(tsp)) ||
> > + scan->kqs_nevent != 0) {
> > splx(s);
> > error = 0;
> > goto done;
> > @@ -931,7 +964,7 @@ retry:
> > kq->kq_state |= KQ_SLEEP;
> > error = kqueue_sleep(kq, tsp);
> > splx(s);
> > - if (error == 0 || error == EWOULDBLOCK)
> > + if (error == 0)
> > goto retry;
>
> Why wouldn't we want to retry in the EWOULDBLOCK case?
> You have a check for
>
> tsp != NULL && !timespecisset(tsp)
>
> e.g., when you time out.
I don't recall why or even if there was a reason. I'll change it back,
thanks.
> > +
> > + /*
> > + * 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;
> > + }
>
> Aside: can the new logic (below) not handle the case where
> nevents == 0? Like, what happens if we go into kqueue_scan()
> with count == 0?
First like of kqueue_scan() does:
if (count == 0)
goto done
So there's no sleep. I opted for the explicit sleep rather than the
obfuscated one ;)
> > + /* Collect at most `nevents' possibly waiting in kqueue_scan() */
> > + kqueue_scan_setup(&scan, p->p_kq);
> > + while (nevents > 0) {
> > + struct kevent kev[KQ_NEVENTS];
> > + int i, ready, count;
> > +
> > + /* Maxium number of events per iteration */
> > + count = MIN(nitems(kev), nevents);
> > + ready = kqueue_scan(&scan, count, kev, timeout, p, &error);
> > +#ifdef KTRACE
> > + if (KTRPOINT(p, KTR_STRUCT))
> > + ktrevent(p, kev, ready);
> > +#endif
> > + /* Convert back events that are ready. */
> > + for (i = 0; i < ready; i++)
> > + *retval += pselcollect(p, &kev[i], pobits);
> > +
> > + /*
> > + * Stop if there was an error or if we had enough
> > + * place to collect all events that were ready.
> > + */
> > + if (error || ready < count)
> > + break;
> > +
> > + timeout = &ts; /* successive loops non-blocking */
> > + timespecclear(timeout);
>
> ... again, it looks like we now truncate instead of resleeping.
Yes, that's on purpose. Maybe you can suggest a comment to explain this
behavior that seems surprising?
