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)) {
> >
> >
