On 18/08/20(Tue) 11:22, Mark Kettenis wrote: > > Date: Tue, 18 Aug 2020 11:04:47 +0200 > > From: Martin Pieuchot <m...@openbsd.org> > > > > Diff below changes the order of operations in kqueue_register() to get > > rid of an unnecessary pool_get(). When an event is already present on > > the list try to acquire it first. Note that knote_acquire() may sleep > > in which case the list might have changed so the lookup has to always > > begin from the start. > > > > This will help with lazy removal of knote in poll/select. In this > > scenario EV_ADD is generally always done with an knote already on the > > list. > > > > ok? > > But pool_get() may sleep as well. In my experience it is better to do > the resource allocation up front and release afterwards if it turned > out you didn't need the resource. That's what the current code does.
There's indeed a race possible when multiple threads try to register the same event on the same kqueue, this will lead to a double insert. Thanks! > > Index: kern/kern_event.c > > =================================================================== > > RCS file: /cvs/src/sys/kern/kern_event.c,v > > retrieving revision 1.142 > > diff -u -p -r1.142 kern_event.c > > --- kern/kern_event.c 12 Aug 2020 13:49:24 -0000 1.142 > > +++ kern/kern_event.c 18 Aug 2020 08:58:27 -0000 > > @@ -696,7 +696,7 @@ kqueue_register(struct kqueue *kq, struc > > struct filedesc *fdp = kq->kq_fdp; > > const struct filterops *fops = NULL; > > struct file *fp = NULL; > > - struct knote *kn = NULL, *newkn = NULL; > > + struct knote *kn, *newkn = NULL; > > struct knlist *list = NULL; > > int s, error = 0; > > > > @@ -721,22 +721,12 @@ kqueue_register(struct kqueue *kq, struc > > return (EBADF); > > } > > > > - if (kev->flags & EV_ADD) > > - newkn = pool_get(&knote_pool, PR_WAITOK | PR_ZERO); > > - > > again: > > + kn = NULL; > > if (fops->f_flags & FILTEROP_ISFD) { > > - if ((fp = fd_getfile(fdp, kev->ident)) == NULL) { > > - error = EBADF; > > - goto done; > > - } > > - if (kev->flags & EV_ADD) > > - kqueue_expand_list(kq, kev->ident); > > if (kev->ident < kq->kq_knlistsize) > > list = &kq->kq_knlist[kev->ident]; > > } else { > > - if (kev->flags & EV_ADD) > > - kqueue_expand_hash(kq); > > if (kq->kq_knhashmask != 0) { > > list = &kq->kq_knhash[ > > KN_HASH((u_long)kev->ident, kq->kq_knhashmask)]; > > @@ -749,10 +739,6 @@ again: > > s = splhigh(); > > if (!knote_acquire(kn)) { > > splx(s); > > - if (fp != NULL) { > > - FRELE(fp, p); > > - fp = NULL; > > - } > > goto again; > > } > > splx(s); > > @@ -760,6 +746,21 @@ again: > > } > > } > > } > > + > > + if (kev->flags & EV_ADD && kn == NULL) { > > + newkn = pool_get(&knote_pool, PR_WAITOK | PR_ZERO); > > + if (fops->f_flags & FILTEROP_ISFD) { > > + if ((fp = fd_getfile(fdp, kev->ident)) == NULL) { > > + error = EBADF; > > + goto done; > > + } > > + kqueue_expand_list(kq, kev->ident); > > + } else { > > + kqueue_expand_hash(kq); > > + } > > + > > + } > > + > > KASSERT(kn == NULL || (kn->kn_status & KN_PROCESSING) != 0); > > > > if (kn == NULL && ((kev->flags & EV_ADD) == 0)) { > > > >