Stefan Sperling <stsp@stsp.name> wrote:
> Run just one server process per server declared in gotwebd.conf, instead
> of running additional sefver processes based on the "prefork" setting.
> 
> The extra sefver processes weren't actually used since only one process
> would manage to accept a connection. The others would all fail in
> accept4() with EWOULDBLOCK and go back to sleep.
> 
> So only the first server process will kick off work to its dedicated
> gotweb sibling, immediately win the accept race(?) again, and then wait
> for its gotweb worker to process current and the next request. The other
> processes all remain idle, at least as far as I have observed behaviour
> on a single-CPU system.
> 
> Having a single listening process dispatch request across gotweb processes
> in a round-robin fashion actually allows requests to be processed in parallel
> as intended. This can be tested by running a difficult blame request (e.g.
> blame got/got.c in got.git) and browsing other gotwebd pages in other tabs.
> 
> The "prefork" setting now only controls the number of gotweb workers
> which will be started.
> 
> In the future, I plan to improve the round-robin strategy since it can
> still lead to requests waiting for a busy worker while other workers
> are idle. This requires tracking the state of requests in sockets.c to
> know when work has completed. But the current gotwebd code no longer does
> such state tracking. My other pending change to split the parsing of FCGI
> parameters into a separate process reintroduces the state tracking for
> unrelated reasons. We could then use this to improve load-balancing, too.

as discussed on irc the other day, i'm not too convinced about this.
I'm not sure we can do right choices in the listener process, since not
all the requests have the same 'weight' (e.g. blame vs log), while the
current design with one shared socket and leaving the kernel to decide
which process 'wins' the race I think it's preferable, as if some
processes are busy, they won't partecipate on the accept(2) 'race'.

Now, as you describe this is not working as intended, and since I don't
want to stop further from having the authentication code in, I'm okay
with committing this and revisit it later.

(i might bump my '-' stats as well in the process... :p)

> Ok?

ok op@ with a very tiny nitpicking below.

> blob - 03f8e6941798467f7ecdd2e1de64cdc88bf28422
> blob + 1489f7bf4879e67af7d43f2ab53d6bdb7885a34d
> --- gotwebd/gotweb.c
> +++ gotwebd/gotweb.c
> @@ -1527,19 +1532,17 @@ recv_server_pipe(struct gotwebd *env, struct imsg *ims
>  	struct imsgev *iev;
>  	int fd;
>  
> -	if (env->iev_server != NULL) {
> -		log_warn("server pipe already received");
> +	if (env->servers_pending <= 0) {
> +		log_warn("server pipes already received");
>  		return;
>  	}
>  
> +

don't add empty lines please :p

>  	fd = imsg_get_fd(imsg);
>  	if (fd == -1)
>  		fatalx("invalid server pipe fd");
>  
> -	iev = calloc(1, sizeof(*iev));
> -	if (iev == NULL)
> -		fatal("calloc");
> -
> +	iev = &env->iev_server[env->servers_pending - 1];
>  	if (imsgbuf_init(&iev->ibuf, fd) == -1)
>  		fatal("imsgbuf_init");
>  	imsgbuf_allow_fdpass(&iev->ibuf);
> @@ -1549,7 +1552,7 @@ recv_server_pipe(struct gotwebd *env, struct imsg *ims
>  	event_set(&iev->ev, fd, EV_READ, gotweb_dispatch_server, iev);
>  	imsg_event_add(iev);
>  
> -	env->iev_server = iev;
> +	env->servers_pending--;
>  }
>  
>  static void