Hi again,

On Tue, Apr 17, 2018 at 01:07:49PM +0200, Olivier Houchard wrote:
[...]
> We only need one to prevent kevent() from trying to scanning the kqueue, so
> only setting kev[0] should be enough. It's inside an #ifdef because
> EV_RECEIPT was only implemented recently in OpenBSD, so all users may not have
> it.
> My hope is, if EV_RECEIPT is not set, we will indeed read even needlessly,
> but we will read them again at the next kevent() call. If it does not, and
> events are lost, then we'll have to add an extra entry that generates an 
> error.

After talking with Willy, here is an updated patch that does that.
That way, the day we'll want to use EV_ONESHOT, we'll be ready, and won't
miss any event.

Regards,

Olivier

>From 56701f85b3311657ea6915df94ec75ac7a83eb54 Mon Sep 17 00:00:00 2001
From: Olivier Houchard <ohouch...@haproxy.com>
Date: Mon, 16 Apr 2018 13:24:48 +0200
Subject: [PATCH] BUG/MEDIUM: kqueue: When adding new events, provide an output
 to get errors.

When adding new events using kevent(), if there's an error, because we're
trying to delete an event that wasn't there, or because the fd has already
been closed, kevent() will either add an event in the eventlist array if
there's enough room for it, and keep on handling other events, or stop and
return -1.
We want it to process all the events, so give it a large-enough array to
store any error.

This should be backported to 1.8.
---
 src/ev_kqueue.c | 43 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 37 insertions(+), 6 deletions(-)

diff --git a/src/ev_kqueue.c b/src/ev_kqueue.c
index a103ece9d..ebfd5d210 100644
--- a/src/ev_kqueue.c
+++ b/src/ev_kqueue.c
@@ -31,6 +31,7 @@
 /* private data */
 static int kqueue_fd[MAX_THREADS]; // per-thread kqueue_fd
 static THREAD_LOCAL struct kevent *kev = NULL;
+static struct kevent *kev_out = NULL; // Trash buffer for kevent() to write 
the eventlist in
 
 /*
  * kqueue() poller
@@ -43,6 +44,8 @@ REGPRM2 static void _do_poll(struct poller *p, int exp)
        int updt_idx, en;
        int changes = 0;
 
+       timeout.tv_sec  = 0;
+       timeout.tv_nsec = 0;
        /* first, scan the update list to find changes */
        for (updt_idx = 0; updt_idx < fd_nbupdt; updt_idx++) {
                fd = fd_updt[updt_idx];
@@ -81,13 +84,21 @@ REGPRM2 static void _do_poll(struct poller *p, int exp)
                        HA_ATOMIC_OR(&fdtab[fd].polled_mask, tid_bit);
                }
        }
-       if (changes)
-               kevent(kqueue_fd[tid], kev, changes, NULL, 0, NULL);
+       if (changes) {
+#ifdef EV_RECEIPT
+               kev[0].flags |= EV_RECEIPT;
+#else
+               /* If EV_RECEIPT isn't defined, just add an invalid entry,
+                * so that we get an error and kevent() stops before scanning
+                * the kqueue.
+                */
+               EV_SET(&kev[changes++], -1, EVFILT_WRITE, EV_DELETE, 0, 0, 
NULL);
+#endif
+               kevent(kqueue_fd[tid], kev, changes, kev_out, changes, 
&timeout);
+       }
        fd_nbupdt = 0;
 
        delta_ms        = 0;
-       timeout.tv_sec  = 0;
-       timeout.tv_nsec = 0;
 
        if (!exp) {
                delta_ms        = MAX_DELAY_MS;
@@ -150,8 +161,12 @@ static int init_kqueue_per_thread()
 {
        int fd;
 
-       /* we can have up to two events per fd (*/
-       kev = calloc(1, sizeof(struct kevent) * 2 * global.maxsock);
+       /* we can have up to two events per fd, so allocate enough to store
+        * 2*fd event, and an extra one, in case EV_RECEIPT isn't defined,
+        * so that we can add an invalid entry and get an error, to avoid
+        * scanning the kqueue uselessly.
+        */
+       kev = calloc(1, sizeof(struct kevent) * (2 * global.maxsock + 1));
        if (kev == NULL)
                goto fail_alloc;
 
@@ -194,6 +209,15 @@ REGPRM1 static int _do_init(struct poller *p)
 {
        p->private = NULL;
 
+       /* we can have up to two events per fd, so allocate enough to store
+        * 2*fd event, and an extra one, in case EV_RECEIPT isn't defined,
+        * so that we can add an invalid entry and get an error, to avoid
+        * scanning the kqueue uselessly.
+        */
+       kev_out = calloc(1, sizeof(struct kevent) * (2 * global.maxsock + 1));
+       if (!kev_out)
+               goto fail_alloc;
+
        kqueue_fd[tid] = kqueue();
        if (kqueue_fd[tid] < 0)
                goto fail_fd;
@@ -203,6 +227,9 @@ REGPRM1 static int _do_init(struct poller *p)
        return 1;
 
  fail_fd:
+       free(kev_out);
+       kev_out = NULL;
+fail_alloc:
        p->pref = 0;
        return 0;
 }
@@ -220,6 +247,10 @@ REGPRM1 static void _do_term(struct poller *p)
 
        p->private = NULL;
        p->pref = 0;
+       if (kev_out) {
+               free(kev_out);
+               kev_out = NULL;
+       }
 }
 
 /*
-- 
2.14.3

Reply via email to