[PATCH] git daemon: avoid waking up too often

[PATCH] git daemon: avoid waking up too often

[Johannes Schindelin]

To avoid waking up unnecessarily, a pipe is set up that is only ever
written to by child_handler(), when a child disconnects, as suggested
per Junio.

This avoids waking up the main process every second to see if a child
was disconnected.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	I have this in production ever since I posted it the first time,
	without problems.

	Oh, and it removes more lines than it introduces ;-)

 daemon.c |   24 +++++++++++-------------
 1 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/daemon.c b/daemon.c
index 7df41a6..850f6f9 100644
--- a/daemon.c
+++ b/daemon.c
@@ -16,6 +16,7 @@
 static int log_syslog;
 static int verbose;
 static int reuseaddr;
+static int child_handler_pipe[2];
 
 static const char daemon_usage[] =
 "git daemon [--verbose] [--syslog] [--export-all]\n"
@@ -788,6 +789,7 @@ static void child_handler(int signo)
 				pid = -pid;
 			dead_child[reaped % MAX_CHILDREN] = pid;
 			children_reaped = reaped + 1;
+			write(child_handler_pipe[1], &status, 1);
 			continue;
 		}
 		break;
@@ -933,12 +935,17 @@ static int service_loop(int socknum, int *socklist)
 	struct pollfd *pfd;
 	int i;
 
-	pfd = xcalloc(socknum, sizeof(struct pollfd));
+	if (pipe(child_handler_pipe) < 0)
+		die ("Could not set up pipe for child handler");
+
+	pfd = xcalloc(socknum + 1, sizeof(struct pollfd));
 
 	for (i = 0; i < socknum; i++) {
 		pfd[i].fd = socklist[i];
 		pfd[i].events = POLLIN;
 	}
+	pfd[socknum].fd = child_handler_pipe[0];
+	pfd[socknum].events = POLLIN;
 
 	signal(SIGCHLD, child_handler);
 
@@ -946,16 +953,7 @@ static int service_loop(int socknum, int *socklist)
 		int i;
 		int timeout;
 
-		/*
-		 * This 1-sec timeout could lead to idly looping but it is
-		 * here so that children culled in child_handler() are reported
-		 * without too much delay.  We could probably set up a pipe
-		 * to ourselves that we poll, and write to the fd from child_handler()
-		 * to wake us up (and consume it when the poll() returns...
-		 */
-		timeout = (children_spawned != children_deleted) ? 1000 : -1;
-		i = poll(pfd, socknum, timeout);
-		if (i < 0) {
+		if (poll(pfd, socknum + 1, -1) < 0) {
 			if (errno != EINTR) {
 				error("poll failed, resuming: %s",
 				      strerror(errno));
@@ -963,9 +961,9 @@ static int service_loop(int socknum, int *socklist)
 			}
 			continue;
 		}
-		if (i == 0) {
+		if (pfd[socknum].revents & POLLIN) {
+			read(child_handler_pipe[0], &i, 1);
 			check_dead_children();
-			continue;
 		}
 
 		for (i = 0; i < socknum; i++) {
-- 
1.6.0.rc0.22.gf2096d.dirty

[PATCH v2] git daemon: avoid waking up too often

[Johannes Schindelin]

To avoid waking up unnecessarily, a pipe is set up that is only ever
written to by child_handler(), when a child disconnects, as suggested
per Junio.

This avoids waking up the main process every second to see if a child
was disconnected.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	Darn.  Forgot to commit after fixing the compiler warning:
	The now-unused variable timeout is gone.

	That is the only difference to the version I have running in
	production ;-)

 daemon.c |   25 +++++++++++--------------
 1 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/daemon.c b/daemon.c
index 7df41a6..4540e8d 100644
--- a/daemon.c
+++ b/daemon.c
@@ -16,6 +16,7 @@
 static int log_syslog;
 static int verbose;
 static int reuseaddr;
+static int child_handler_pipe[2];
 
 static const char daemon_usage[] =
 "git daemon [--verbose] [--syslog] [--export-all]\n"
@@ -788,6 +789,7 @@ static void child_handler(int signo)
 				pid = -pid;
 			dead_child[reaped % MAX_CHILDREN] = pid;
 			children_reaped = reaped + 1;
+			write(child_handler_pipe[1], &status, 1);
 			continue;
 		}
 		break;
@@ -933,29 +935,24 @@ static int service_loop(int socknum, int *socklist)
 	struct pollfd *pfd;
 	int i;
 
-	pfd = xcalloc(socknum, sizeof(struct pollfd));
+	if (pipe(child_handler_pipe) < 0)
+		die ("Could not set up pipe for child handler");
+
+	pfd = xcalloc(socknum + 1, sizeof(struct pollfd));
 
 	for (i = 0; i < socknum; i++) {
 		pfd[i].fd = socklist[i];
 		pfd[i].events = POLLIN;
 	}
+	pfd[socknum].fd = child_handler_pipe[0];
+	pfd[socknum].events = POLLIN;
 
 	signal(SIGCHLD, child_handler);
 
 	for (;;) {
 		int i;
-		int timeout;
 
-		/*
-		 * This 1-sec timeout could lead to idly looping but it is
-		 * here so that children culled in child_handler() are reported
-		 * without too much delay.  We could probably set up a pipe
-		 * to ourselves that we poll, and write to the fd from child_handler()
-		 * to wake us up (and consume it when the poll() returns...
-		 */
-		timeout = (children_spawned != children_deleted) ? 1000 : -1;
-		i = poll(pfd, socknum, timeout);
-		if (i < 0) {
+		if (poll(pfd, socknum + 1, -1) < 0) {
 			if (errno != EINTR) {
 				error("poll failed, resuming: %s",
 				      strerror(errno));
@@ -963,9 +960,9 @@ static int service_loop(int socknum, int *socklist)
 			}
 			continue;
 		}
-		if (i == 0) {
+		if (pfd[socknum].revents & POLLIN) {
+			read(child_handler_pipe[0], &i, 1);
 			check_dead_children();
-			continue;
 		}
 
 		for (i = 0; i < socknum; i++) {
-- 
1.6.0.rc0.22.gf2096d.dirty

Re: [PATCH v2] git daemon: avoid waking up too often

[Johannes Sixt]

Johannes Schindelin schrieb:
> To avoid waking up unnecessarily, a pipe is set up that is only ever
> written to by child_handler(), when a child disconnects, as suggested
> per Junio.
> 
> This avoids waking up the main process every second to see if a child
> was disconnected.

This makes porting this beast to Windows practically impossible because we
cannot have a poll() implementation that waits both on a listening socket
and a pipe. :-(

-- Hannes

Re: [PATCH v2] git daemon: avoid waking up too often

[Johannes Schindelin]

Hi,

On Wed, 23 Jul 2008, Johannes Sixt wrote:

> Johannes Schindelin schrieb:
> > To avoid waking up unnecessarily, a pipe is set up that is only ever
> > written to by child_handler(), when a child disconnects, as suggested
> > per Junio.
> > 
> > This avoids waking up the main process every second to see if a child
> > was disconnected.
> 
> This makes porting this beast to Windows practically impossible because we
> cannot have a poll() implementation that waits both on a listening socket
> and a pipe. :-(

Umm.  We also do not have signal handlers, do we?  AFAICT you wanted to 
simulate _this_ program's fork() with a start_async(), right?  All you 
have to do is to refactor the code with the pipe to do the update 
(properly guarded by a mutex, which is called "CriticalSection" in Windows 
speak, since it is impossible for Microsoft to accept common conventions).

The biggest part to get this beast running on Windows is to move it to the 
start_async() method, not above-mentioned refactoring.

Ciao,
Dscho

Re: [PATCH v2] git daemon: avoid waking up too often

[Avery Pennarun]

On 7/23/08, Johannes Sixt <j.sixt@viscovery.net> wrote:
> This makes porting this beast to Windows practically impossible because we
>  cannot have a poll() implementation that waits both on a listening socket
>  and a pipe. :-(

I have worked around such problems in the past by having a thread
whose job it is to read from the pipe and simply write it to a socket.
 The trick works for "console" objects, too, which in win32 are even
less agreeable than pipes.

(In case your life wasn't disgusting enough already today :))

Alternatively, you could use something like socketpair() instead of a
pipe for this purpose.  Naturally, Win32 helps you out here by somehow
forgetting to include socketpair() in winsock, although it's sort of
easy to emulate.

Have fun,

Avery

Re: [PATCH v2] git daemon: avoid waking up too often

[Johannes Schindelin]

Hi,

On Wed, 23 Jul 2008, Avery Pennarun wrote:

> On 7/23/08, Johannes Sixt <j.sixt@viscovery.net> wrote:
> > This makes porting this beast to Windows practically impossible 
> > because we cannot have a poll() implementation that waits both on a 
> > listening socket and a pipe. :-(
> 
> I have worked around such problems in the past by having a thread whose 
> job it is to read from the pipe and simply write it to a socket. The 
> trick works for "console" objects, too, which in win32 are even less 
> agreeable than pipes.
> 
> (In case your life wasn't disgusting enough already today :))

Thanks.  I thought the code I looked at today was ugly, but you convinced 
me that there is no limit.

> Alternatively, you could use something like socketpair() instead of a 
> pipe for this purpose.  Naturally, Win32 helps you out here by somehow 
> forgetting to include socketpair() in winsock, although it's sort of 
> easy to emulate.

Or alternatively, you could read my response to Hannes where I explain 
that the pipe() is not even needed with threaded start_async() (as opposed 
to fork()ed one).

Ciao,
Dscho