Mailing List Archive

Author: phk
Date: 2008-05-26 12:13:52 +0200 (Mon, 26 May 2008)
New Revision: 2639

Modified:
trunk/varnish-cache/bin/varnishd/cache_acceptor.c
trunk/varnish-cache/bin/varnishd/cache_center.c
trunk/varnish-cache/bin/varnishd/mgt_child.c
trunk/varnish-cache/bin/varnishd/tcp.c
Log:
Be more consistent about sockets and blocking/non-blocking mode:

Accept sockets are non-blocking, to avoid races where the client closes
before we get to accept it. (Spotted by: "chen xiaoyong")

Unfortunately, that means that session sockets inherit non-blocking mode,
which is the opposite of what we want in the worker thread but correct
for the acceptor thread.

We prefer to have the extra syscalls in the worker thread, that complicates
things a little bit.

Use ioctl(FIONBIO) instead of fcntl(2) which is surprisingly expensive.


Modified: trunk/varnish-cache/bin/varnishd/cache_acceptor.c
===================================================================
--- trunk/varnish-cache/bin/varnishd/cache_acceptor.c 2008-05-26 09:14:23 UTC (rev 2638)
+++ trunk/varnish-cache/bin/varnishd/cache_acceptor.c 2008-05-26 10:13:52 UTC (rev 2639)
@@ -270,6 +270,11 @@
AZ(sp->obj);
AZ(sp->vcl);
assert(sp->fd >= 0);
+ /*
+ * Set nonblocking in the worker-thread, before passing to the
+ * acceptor thread, to reduce syscall density of the latter.
+ */
+ TCP_nonblocking(sp->fd);
if (vca_act->pass == NULL)
assert(sizeof sp == write(vca_pipes[1], &sp, sizeof sp));
else

Modified: trunk/varnish-cache/bin/varnishd/cache_center.c
===================================================================
--- trunk/varnish-cache/bin/varnishd/cache_center.c 2008-05-26 09:14:23 UTC (rev 2638)
+++ trunk/varnish-cache/bin/varnishd/cache_center.c 2008-05-26 10:13:52 UTC (rev 2639)
@@ -906,6 +906,16 @@
w = sp->wrk;
CHECK_OBJ_NOTNULL(w, WORKER_MAGIC);

+ /*
+ * Whenever we come in from the acceptor we need to set blocking
+ * mode, but there is no point in setting it when we come from
+ * ESI or when a parked sessions returns.
+ * It would be simpler to do this in the acceptor, but we'd rather
+ * do the syscall in the worker thread.
+ */
+ if (sp->step == STP_FIRST || sp->step == STP_START)
+ TCP_blocking(sp->fd);
+
for (done = 0; !done; ) {
assert(sp->wrk == w);
/*

Modified: trunk/varnish-cache/bin/varnishd/mgt_child.c
===================================================================
--- trunk/varnish-cache/bin/varnishd/mgt_child.c 2008-05-26 09:14:23 UTC (rev 2638)
+++ trunk/varnish-cache/bin/varnishd/mgt_child.c 2008-05-26 10:13:52 UTC (rev 2639)
@@ -150,6 +150,12 @@
free(ls);
continue;
}
+ /*
+ * Set nonblocking mode to avoid a race where a client
+ * closes before we call accept(2) and nobody else are in
+ * the listen queue to release us.
+ */
+ TCP_nonblocking(ls->sock);
TCP_filter_http(ls->sock);
good++;
}

Modified: trunk/varnish-cache/bin/varnishd/tcp.c
===================================================================
--- trunk/varnish-cache/bin/varnishd/tcp.c 2008-05-26 09:14:23 UTC (rev 2638)
+++ trunk/varnish-cache/bin/varnishd/tcp.c 2008-05-26 10:13:52 UTC (rev 2639)
@@ -37,7 +37,7 @@
#include <netinet/in.h>

#include <errno.h>
-#include <fcntl.h>
+#include <sys/ioctl.h>
#include <netdb.h>
#include <stdio.h>
#include <stdlib.h>
@@ -112,18 +112,22 @@
#endif
}

-/*--------------------------------------------------------------------*/
+/*--------------------------------------------------------------------
+ * Functions for controlling NONBLOCK mode.
+ *
+ * We use FIONBIO because it is cheaper than fcntl(2), which requires
+ * us to do two syscalls, one to get and one to set, the latter of
+ * which mucks about a bit before it ends up calling ioctl(FIONBIO),
+ * at least on FreeBSD.
+ */

void
TCP_blocking(int sock)
{
int i;

- i = fcntl(sock, F_GETFL);
- assert(i != -1);
- i &= ~O_NONBLOCK;
- i = fcntl(sock, F_SETFL, i);
- assert(i != -1);
+ i = 0;
+ AZ(ioctl(sock, FIONBIO, &i));
}

void
@@ -131,9 +135,6 @@
{
int i;

- i = fcntl(sock, F_GETFL);
- assert(i != -1);
- i |= O_NONBLOCK;
- i = fcntl(sock, F_SETFL, i);
- assert(i != -1);
+ i = 1;
+ AZ(ioctl(sock, FIONBIO, &i));
}