Re: kqueue EV_DISPATCH and EV_EOF interaction

[Mike Belopuhov]

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=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

Re: kqueue EV_DISPATCH and EV_EOF interaction

[Lukas Larsson]

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=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

[Mike Belopuhov]

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=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

[Mike Belopuhov]

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(, 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", , );
> 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(, listen_s, EVFILT_READ, EV_ADD, 0, 0, NULL);
> if (kevent(kq, , 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 *), 
> );
> printf("accepted %d\n", fd);
> #if USE_DISPATCH
> EV_SET(, fd, EVFILT_READ, EV_ADD|EV_DISPATCH, 0, 0, 
> NULL);
> #else
> EV_SET(, fd, EVFILT_READ, EV_ADD, 0, 0, NULL);
> #endif
> if (kevent(kq, , 1, NULL, 0, NULL) == -1)
> err(1, "kevent");
> } else {
> if (evList[i].flags & EV_EOF && evList[i].data == 0) {
> printf("closing