On Tue, Oct 04, 2016 at 11:54:37PM +0200, Rafael Zalamena wrote:
> On Tue, Oct 04, 2016 at 07:46:52PM +0200, Rafael Zalamena wrote:
> > This diff makes proc.c daemons to use less file descriptors on startup,
> > this way we increase the number of child we can have considerably. This
> > also improves the solution on a bug reported in bugs@
> > "httpd errors out with 'too many open files'".
> >
> > To achieve that I delayed the socket distribution and made a minimal
> > socket allocation in proc_init(), so only the necessary children socket
> > are allocated and passed with proc_exec(). After the event_init() is called
> > we call proc_connect() which creates the socketpair() and immediatly after
> > each call we already sends them without accumulating.
> >
> > Note: We still have to calculate how many fds we will want to have and
> > then limit the daemon prefork configuration.
> >
> > ok?
> >
>
> Paul de Weerd still found problems with the diff, because the httpd(8)
> would not exit successfully with '-n' flag. It happened because the
> new proc_connect() code tried to write fds to children process that
> did not start caused by the ps_noaction flag. (thanks Paul!)
>
> This new diff just adds a check for ps_noaction in proc_init() and
> proc_connect() so it doesn't try to do anything if we are not really
> going to start the daemon. Also I removed the ps_noaction from proc_run()
> since we are not going to get to this point anymore.
>
> ok?
>
The diff looks fine and works on httpd, but it doesn't seem work on
vmd and switchd (i didn't test relayd). So there might be something
wrong, please test applying it to them first.
Reyk
>
> Index: proc.c
> ===================================================================
> RCS file: /home/obsdcvs/src/usr.sbin/httpd/proc.c,v
> retrieving revision 1.27
> diff -u -p -r1.27 proc.c
> --- proc.c 28 Sep 2016 12:01:04 -0000 1.27
> +++ proc.c 4 Oct 2016 21:50:43 -0000
> @@ -36,8 +36,6 @@
>
> void proc_exec(struct privsep *, struct privsep_proc *, unsigned int,
> int, char **);
> -void proc_connectpeer(struct privsep *, enum privsep_procid, int,
> - struct privsep_pipes *);
> void proc_setup(struct privsep *, struct privsep_proc *, unsigned int);
> void proc_open(struct privsep *, int, int);
> void proc_accept(struct privsep *, int, enum privsep_procid,
> @@ -147,72 +145,18 @@ proc_exec(struct privsep *ps, struct pri
> }
>
> void
> -proc_connectpeer(struct privsep *ps, enum privsep_procid id, int inst,
> - struct privsep_pipes *pp)
> -{
> - unsigned int i, j;
> - struct privsep_fd pf;
> -
> - for (i = 0; i < PROC_MAX; i++) {
> - /* Parent is already connected with everyone. */
> - if (i == PROC_PARENT)
> - continue;
> -
> - for (j = 0; j < ps->ps_instances[i]; j++) {
> - /* Don't send socket to child itself. */
> - if (i == (unsigned int)id &&
> - j == (unsigned int)inst)
> - continue;
> - if (pp->pp_pipes[i][j] == -1)
> - continue;
> -
> - pf.pf_procid = i;
> - pf.pf_instance = j;
> - proc_compose_imsg(ps, id, inst, IMSG_CTL_PROCFD,
> - -1, pp->pp_pipes[i][j], &pf, sizeof(pf));
> - pp->pp_pipes[i][j] = -1;
> - }
> - }
> -}
> -
> -/* Inter-connect all process except with ourself. */
> -void
> proc_connect(struct privsep *ps)
> {
> - unsigned int src, i, j;
> - struct privsep_pipes *pp;
> - struct imsgev *iev;
> -
> - /* Listen on appropriate pipes. */
> - src = privsep_process;
> - pp = &ps->ps_pipes[src][ps->ps_instance];
> -
> - for (i = 0; i < PROC_MAX; i++) {
> - /* Don't listen to ourself. */
> - if (i == src)
> - continue;
> -
> - for (j = 0; j < ps->ps_instances[i]; j++) {
> - if (pp->pp_pipes[i][j] == -1)
> - continue;
> -
> - iev = &ps->ps_ievs[i][j];
> - imsg_init(&iev->ibuf, pp->pp_pipes[i][j]);
> - event_set(&iev->ev, iev->ibuf.fd, iev->events,
> - iev->handler, iev->data);
> - event_add(&iev->ev, NULL);
> - }
> - }
> + unsigned int src, dst;
>
> - /* Exchange pipes between process. */
> - for (i = 0; i < PROC_MAX; i++) {
> - /* Parent is already connected with everyone. */
> - if (i == PROC_PARENT)
> - continue;
> + /* Don't distribute any sockets if we are not really going to run. */
> + if (ps->ps_noaction)
> + return;
>
> - for (j = 0; j < ps->ps_instances[i]; j++)
> - proc_connectpeer(ps, i, j, &ps->ps_pipes[i][j]);
> - }
> + /* Distribute the socketpair()s for everyone. */
> + for (src = 0; src < PROC_MAX; src++)
> + for (dst = src; dst < PROC_MAX; dst++)
> + proc_open(ps, src, dst);
> }
>
> void
> @@ -220,17 +164,41 @@ proc_init(struct privsep *ps, struct pri
> int argc, char **argv, enum privsep_procid proc_id)
> {
> struct privsep_proc *p = NULL;
> + struct privsep_pipes *pa, *pb;
> unsigned int proc;
> - unsigned int src, dst;
> + unsigned int dst;
> + int fds[2];
> +
> + /* Don't initiate anything if we are not really going to run. */
> + if (ps->ps_noaction)
> + return;
>
> if (proc_id == PROC_PARENT) {
> privsep_process = PROC_PARENT;
> proc_setup(ps, procs, nproc);
>
> - /* Open socketpair()s for everyone. */
> - for (src = 0; src < PROC_MAX; src++)
> - for (dst = 0; dst < PROC_MAX; dst++)
> - proc_open(ps, src, dst);
> + /*
> + * Create the children sockets so we can use them
> + * to distribute the rest of the socketpair()s using
> + * proc_connect() later.
> + */
> + for (dst = 0; dst < PROC_MAX; dst++) {
> + /* Don't create socket for ourselves. */
> + if (dst == PROC_PARENT)
> + continue;
> +
> + for (proc = 0; proc < ps->ps_instances[dst]; proc++) {
> + pa = &ps->ps_pipes[proc_id][ps->ps_instance];
> + pb = &ps->ps_pipes[dst][proc];
> + if (socketpair(AF_UNIX,
> + SOCK_STREAM | SOCK_NONBLOCK | SOCK_CLOEXEC,
> + PF_UNSPEC, fds) == -1)
> + fatal("socketpair");
> +
> + pa->pp_pipes[dst][proc] = fds[0];
> + pb->pp_pipes[proc_id][ps->ps_instance] = fds[1];
> + }
> + }
>
> /* Engage! */
> proc_exec(ps, procs, nproc, argc, argv);
> @@ -399,9 +367,16 @@ void
> proc_open(struct privsep *ps, int src, int dst)
> {
> struct privsep_pipes *pa, *pb;
> + struct imsgev *iev;
> + struct privsep_fd pf;
> int fds[2];
> unsigned int i, j;
>
> + /* We don't have connections to the target process. */
> + if (ps->ps_ievs[dst] == NULL)
> + return;
> +
> + /* Exchange pipes between process. */
> for (i = 0; i < ps->ps_instances[src]; i++) {
> for (j = 0; j < ps->ps_instances[dst]; j++) {
> /* Don't create sockets for ourself. */
> @@ -410,16 +385,63 @@ proc_open(struct privsep *ps, int src, i
>
> pa = &ps->ps_pipes[src][i];
> pb = &ps->ps_pipes[dst][j];
> - if (pb->pp_pipes[dst][j] != -1)
> + if (pa->pp_pipes[dst][j] == -1 &&
> + pb->pp_pipes[src][i] == -1) {
> + if (socketpair(AF_UNIX,
> + SOCK_STREAM | SOCK_NONBLOCK | SOCK_CLOEXEC,
> + PF_UNSPEC, fds) == -1)
> + fatal("socketpair");
> +
> + pa->pp_pipes[dst][j] = fds[0];
> + pb->pp_pipes[src][i] = fds[1];
> + }
> +
> + iev = &ps->ps_ievs[dst][j];
> + if (iev->ibuf.fd == 0) {
> + imsg_init(&iev->ibuf, pa->pp_pipes[dst][j]);
> + event_set(&iev->ev, iev->ibuf.fd, iev->events,
> + iev->handler, iev->data);
> + event_add(&iev->ev, NULL);
> + }
> +
> + /*
> + * The parent/child pipe was handed by proc_exec()
> + * and ps_ievs[PROC_PARENT] is not allocated here,
> + * so skip imsg_init() and fd send.
> + */
> + if (src == PROC_PARENT)
> continue;
>
> - if (socketpair(AF_UNIX,
> - SOCK_STREAM | SOCK_NONBLOCK | SOCK_CLOEXEC,
> - PF_UNSPEC, fds) == -1)
> - fatal(__func__);
> + iev = &ps->ps_ievs[src][i];
> + if (iev->ibuf.fd == 0) {
> + imsg_init(&iev->ibuf, pa->pp_pipes[src][i]);
> + event_set(&iev->ev, iev->ibuf.fd, iev->events,
> + iev->handler, iev->data);
> + event_add(&iev->ev, NULL);
> + }
> +
> + pf.pf_procid = src;
> + pf.pf_instance = i;
> + if (proc_compose_imsg(ps, dst, j, IMSG_CTL_PROCFD,
> + -1, pb->pp_pipes[src][i], &pf, sizeof(pf)) == -1)
> + fatal("proc_compose_imsg");
>
> - pa->pp_pipes[dst][j] = fds[0];
> - pb->pp_pipes[src][i] = fds[1];
> + pf.pf_procid = dst;
> + pf.pf_instance = j;
> + if (proc_compose_imsg(ps, src, i, IMSG_CTL_PROCFD,
> + -1, pa->pp_pipes[dst][j], &pf, sizeof(pf)) == -1)
> + fatal("proc_compose_imsg");
> +
> + /*
> + * We have to flush to send the descriptors and close
> + * them to avoid the fd ramp on startup.
> + */
> + if (imsg_flush(&ps->ps_ievs[dst][j].ibuf) == -1 ||
> + imsg_flush(&ps->ps_ievs[src][i].ibuf) == -1)
> + fatal("imsg_flush");
> +
> + pa->pp_pipes[dst][j] = -1;
> + pb->pp_pipes[src][i] = -1;
> }
> }
> }
> @@ -501,9 +523,6 @@ proc_run(struct privsep *ps, struct priv
> struct passwd *pw = ps->ps_pw;
> const char *root;
> struct control_sock *rcs;
> -
> - if (ps->ps_noaction)
> - exit(0);
>
> log_procinit(p->p_title);
>
>
--
