threads / patch / 14602

patchgit daemon: avoid waking up too often

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

## tl;dr

6 messages between Jul 22, 2008 and Jul 23, 2008. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Johannes Schindelin· Jul 22, 2008, 21:52 UTC · lore

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(-)
Show changes to daemon.c +11 −13
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
Johannes Schindelin· Jul 22, 2008, 22:03 UTC · re: Johannes Schindelin · lore

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

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(-)
Show changes to daemon.c +11 −14
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
Johannes Sixt· Jul 23, 2008, 06:51 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin schrieb:
Show 6 quoted lines
> 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
Johannes Schindelin· Jul 23, 2008, 09:17 UTC · re: Johannes Sixt · lore

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

Hi,
On Wed, 23 Jul 2008, Johannes Sixt wrote:
Show 11 quoted lines
> 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

Avery Pennarun· Jul 23, 2008, 19:26 UTC · re: Johannes Sixt · lore

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

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
Johannes Schindelin· Jul 23, 2008, 19:39 UTC · re: Avery Pennarun · lore

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

Hi,
On Wed, 23 Jul 2008, Avery Pennarun wrote:
Show 11 quoted lines
> 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

← back to recent threads