Re: kqueue EV_DISPATCH and EV_EOF interaction
On Thu, Mar 29, 2018 at 15:09 +0200, Lukas Larsson wrote: > Hello, > > I've been re-writing the polling mechanisms in the Erlang VM and stumbled > across > something that might be a bug in the OpenBSD implementation of kqueue. > > When using EV_DISPATCH, the event is never triggered again after the EV_EOF > flag has been delivered, even though there is more data to be read from the > socket. > > I've attached a smallish program that shows the problem. > > The shortened ktrace output looks like this on OpenBSD 6.2: > > 29672 a.out0.012883 CALL kevent(4,0x7f7e8220,1,0,0,0) > 29672 a.out0.012888 STRU struct kevent { ident=5, filter=EVFILT_READ, > flags=0x81, fflags=0<>, data=0, udata=0x0 } > 29672 a.out0.012895 RET kevent 0 > 29672 a.out0.012904 CALL kevent(4,0,0,0x7f7e7cf0,32,0) > 29672 a.out0.013408 STRU struct kevent { ident=5, filter=EVFILT_READ, > flags=0x81, fflags=0<>, data=6, udata=0x0 } > 29672 a.out0.013493 RET kevent 1 > 29672 a.out0.013548 CALL read(5,0x7f7e8286,0x2) > 29672 a.out0.013562 RET read 2 > 29672 a.out0.013590 CALL kevent(4,0x7f7e8220,1,0,0,0) > 29672 a.out0.013594 STRU struct kevent { ident=5, filter=EVFILT_READ, > flags=0x84, fflags=0<>, data=0, udata=0x0 } > 29672 a.out0.013608 RET kevent 0 > 29672 a.out1.08 CALL kevent(4,0,0,0x7f7e7cf0,32,0) > 29672 a.out1.022537 STRU struct kevent { ident=5, filter=EVFILT_READ, > flags=0x8081, fflags=0<>, data=4, udata=0x0 } > 29672 a.out1.022572 RET kevent 1 > 29672 a.out1.022663 CALL read(5,0x7f7e8286,0x2) > 29672 a.out1.022707 RET read 2 > 29672 a.out1.022816 CALL kevent(4,0x7f7e8220,1,0,0,0) > 29672 a.out1.022822 STRU struct kevent { ident=5, filter=EVFILT_READ, > flags=0x84, fflags=0<>, data=0, udata=0x0 } > 29672 a.out1.022835 RET kevent 0 > 29672 a.out2.032238 CALL kevent(4,0,0,0x7f7e7cf0,32,0) > 29672 a.out5.277194 PSIG SIGINT SIG_DFL > > In this example I would have expected the last kevent call to return with > EV_EOF and > data set to 2, but it does not trigger again. If I don't use EV_DISPATCH, > the event is > triggered again and the program terminates. > > Does anyone know if this is the expected behavior or a bug? > > I've worked around this issue by using EV_ONESHOT instead of EV_DISPATCH on > OpenBSD for now, but would like to use EV_DISPATCH in the future as I've > found > that it aligns better with the abstractions that I use, and could possibly > be a little bit > more performant. > > Lukas > > PS. If relevant, it seems like FreeBSD does behave the way that I expected, > i.e. > it triggers again for EV_DISPATCH after EV_EOF has been shown. DS. > #include > #include > #include > #include > #include > #include > > #include > #include > #include > #include > #include > #include > > #define USE_DISPATCH 1 > > int main() { > struct addrinfo *addr; > struct addrinfo hints; > int kq, listen_s, fd = -1; > struct kevent evSet; > struct kevent evList[32]; > > /* open a TCP socket */ > memset(&hints, 0, sizeof hints); > hints.ai_family = PF_UNSPEC; /* any supported protocol */ > hints.ai_flags = AI_PASSIVE; /* result for bind() */ > hints.ai_socktype = SOCK_STREAM; /* TCP */ > int error = getaddrinfo ("127.0.0.1", "8080", &hints, &addr); > if (error) > errx(1, "getaddrinfo failed: %s", gai_strerror(error)); > listen_s = socket(addr->ai_family, addr->ai_socktype, addr->ai_protocol); > if (setsockopt(listen_s, SOL_SOCKET, SO_REUSEADDR, &(int){ 1 }, > sizeof(int)) < 0) > errx(1, "setsockopt(SO_REUSEADDR) failed"); > bind(listen_s, addr->ai_addr, addr->ai_addrlen); > listen(listen_s, 5); > > kq = kqueue(); > > system("echo -n abcdef | nc -v -w 1 127.0.0.1 8080 &"); > > EV_SET(&evSet, listen_s, EVFILT_READ, EV_ADD, 0, 0, NULL); > if (kevent(kq, &evSet, 1, NULL, 0, NULL) == -1) > err(1, "kevent"); > > while(1) { > int i; > int nev = kevent(kq, NULL, 0, evList, 32, NULL); > for (i = 0; i < nev; i++) { > if (evList[i].ident == listen_s) { > struct sockaddr_storage addr; > socklen_t socklen = sizeof(addr); > if (fd != -1) > close(fd); > fd = accept(evList[i].ident, (struct sockaddr *)&addr, > &socklen); > printf("accepted %d\n", fd); > #if USE_DISPATCH > EV_SET(&evSet, fd, EVFILT_READ, EV_ADD|EV_DISPATCH, 0, 0, > NULL); > #else > EV_SET(&evSet, fd, EVFILT_READ, EV_ADD, 0, 0, NULL); > #endif > if (kevent(kq, &evSet, 1, NULL, 0, NULL) == -1) > err(1, "kevent"); > } else { > if (evList[i].flags & EV_EOF && evList[i].data == 0) { > printf("closing %d\n", fd); > close(fd); >
Re: kqueue EV_DISPATCH and EV_EOF interaction
On Fri, Mar 30, 2018 at 01:21 +0200, Mike Belopuhov wrote: > > Hi, > > This appears to be an issue with reactivating disabled event sources > in kqueue_register. Something along the lines of FreeBSD commits: > > https://svnweb.freebsd.org/base?view=revision&revision=274560 and > https://reviews.freebsd.org/rS295786 where parent differential review > https://reviews.freebsd.org/D5307 has some additional comments. > > In any case, by either porting their code (#else branch) or slightly > adjusting our own (I think that should be enough), I can no longer > reproduce the issue you've reported. Please test and report back if > that solves your original issue. Either variants will require > rigorous testing and a thorough review. > > Cheers, > Mike > After a bit of tinkering, I think I can minimize the change even further. Basically we just need to call the filter once and if there's some data available, it'll return true and we'll mark the knote as active. diff --git sys/kern/kern_event.c sys/kern/kern_event.c index fb9cad360b1..4e0949645cb 100644 --- sys/kern/kern_event.c +++ sys/kern/kern_event.c @@ -671,10 +671,12 @@ kqueue_register(struct kqueue *kq, struct kevent *kev, struct proc *p) } if ((kev->flags & EV_ENABLE) && (kn->kn_status & KN_DISABLED)) { s = splhigh(); kn->kn_status &= ~KN_DISABLED; + if (kn->kn_fop->f_event(kn, 0)) + kn->kn_status |= KN_ACTIVE; if ((kn->kn_status & KN_ACTIVE) && ((kn->kn_status & KN_QUEUED) == 0)) knote_enqueue(kn); splx(s); }
Re: kqueue EV_DISPATCH and EV_EOF interaction
On Fri, Mar 30, 2018 at 1:51 AM, Mike Belopuhov wrote: > On Fri, Mar 30, 2018 at 01:21 +0200, Mike Belopuhov wrote: > > > > Hi, > > > > This appears to be an issue with reactivating disabled event sources > > in kqueue_register. Something along the lines of FreeBSD commits: > > > > https://svnweb.freebsd.org/base?view=revision&revision=274560 and > > https://reviews.freebsd.org/rS295786 where parent differential review > > https://reviews.freebsd.org/D5307 has some additional comments. > > > > In any case, by either porting their code (#else branch) or slightly > > adjusting our own (I think that should be enough), I can no longer > > reproduce the issue you've reported. Please test and report back if > > that solves your original issue. Either variants will require > > rigorous testing and a thorough review. > > > > Cheers, > > Mike > > > > After a bit of tinkering, I think I can minimize the change even > further. Basically we just need to call the filter once and if > there's some data available, it'll return true and we'll mark the > knote as active. > > diff --git sys/kern/kern_event.c sys/kern/kern_event.c > index fb9cad360b1..4e0949645cb 100644 > --- sys/kern/kern_event.c > +++ sys/kern/kern_event.c > @@ -671,10 +671,12 @@ kqueue_register(struct kqueue *kq, struct kevent > *kev, struct proc *p) > } > > if ((kev->flags & EV_ENABLE) && (kn->kn_status & KN_DISABLED)) { > s = splhigh(); > kn->kn_status &= ~KN_DISABLED; > + if (kn->kn_fop->f_event(kn, 0)) > + kn->kn_status |= KN_ACTIVE; > if ((kn->kn_status & KN_ACTIVE) && > ((kn->kn_status & KN_QUEUED) == 0)) > knote_enqueue(kn); > splx(s); > } > Hello, Thank you for your help and the patch. I've applied the smaller patch to one of our test machines and the small testcase I sent here on the list has been fixed. I also ran our larger test suites where I first found the issue and those work as well. Lukas
Re: kqueue EV_DISPATCH and EV_EOF interaction
On Tue, Apr 03, 2018 at 17:00 +0200, Lukas Larsson wrote: > On Fri, Mar 30, 2018 at 1:51 AM, Mike Belopuhov wrote: > > > On Fri, Mar 30, 2018 at 01:21 +0200, Mike Belopuhov wrote: > > > > > > Hi, > > > > > > This appears to be an issue with reactivating disabled event sources > > > in kqueue_register. Something along the lines of FreeBSD commits: > > > > > > https://svnweb.freebsd.org/base?view=revision&revision=274560 and > > > https://reviews.freebsd.org/rS295786 where parent differential review > > > https://reviews.freebsd.org/D5307 has some additional comments. > > > > > > In any case, by either porting their code (#else branch) or slightly > > > adjusting our own (I think that should be enough), I can no longer > > > reproduce the issue you've reported. Please test and report back if > > > that solves your original issue. Either variants will require > > > rigorous testing and a thorough review. > > > > > > Cheers, > > > Mike > > > > > > > After a bit of tinkering, I think I can minimize the change even > > further. Basically we just need to call the filter once and if > > there's some data available, it'll return true and we'll mark the > > knote as active. > > > > diff --git sys/kern/kern_event.c sys/kern/kern_event.c > > index fb9cad360b1..4e0949645cb 100644 > > --- sys/kern/kern_event.c > > +++ sys/kern/kern_event.c > > @@ -671,10 +671,12 @@ kqueue_register(struct kqueue *kq, struct kevent > > *kev, struct proc *p) > > } > > > > if ((kev->flags & EV_ENABLE) && (kn->kn_status & KN_DISABLED)) { > > s = splhigh(); > > kn->kn_status &= ~KN_DISABLED; > > + if (kn->kn_fop->f_event(kn, 0)) > > + kn->kn_status |= KN_ACTIVE; > > if ((kn->kn_status & KN_ACTIVE) && > > ((kn->kn_status & KN_QUEUED) == 0)) > > knote_enqueue(kn); > > splx(s); > > } > > > > Hello, > > Thank you for your help and the patch. I've applied the smaller patch to > one of our test machines > and the small testcase I sent here on the list has been fixed. I also ran > our larger test suites where > I first found the issue and those work as well. > > Lukas Thanks a lot for a great bug report and testing, I've checked in the diff. Cheers, Mike