> Am 10.10.2016 um 18:47 schrieb Rafael Zalamena <[email protected]>:
>
>> On Mon, Oct 10, 2016 at 12:32:49PM +0200, Reyk Floeter wrote:
>>> 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
>>
>
> It was aborting because msgbuf_write() was being called and since the
> data had already been imsg_flush()ed it return 0 (like a socket close
> would do) and fatal()ed. The fix is simply call imsg_event_add() to
> remove the EV_WRITE and now msgbuf_write() won't be called anymore
> without any data pending.
>
> The diff also got some readability improvements to make the proc_open()
> smaller and with less conditionals.
>
> ok?
>
Adding imsg_event_add() is the proposed way to do it and is OK. Please keep in
mind that imsg_flush() is a hammer for rare conditions like this that
intentionally break the async nature of the event-based imsg API variant; it
should be used with care.
OK reyk
>
> Index: proc.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/httpd/proc.c,v
> retrieving revision 1.32
> diff -u -p -r1.32 proc.c
> --- proc.c 10 Oct 2016 16:31:35 -0000 1.32
> +++ proc.c 10 Oct 2016 16:40:09 -0000
> @@ -37,8 +37,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,
> @@ -157,72 +155,38 @@ 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;
> + unsigned int src, dst, inst;
>
> - /* 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;
> + /* 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++) {
> - if (pp->pp_pipes[i][j] == -1)
> - continue;
> + for (dst = 0; dst < PROC_MAX; dst++) {
> + /* We don't communicate with ourselves. */
> + if (dst == PROC_PARENT)
> + continue;
>
> - iev = &ps->ps_ievs[i][j];
> - imsg_init(&iev->ibuf, pp->pp_pipes[i][j]);
> + for (inst = 0; inst < ps->ps_instances[dst]; inst++) {
> + iev = &ps->ps_ievs[dst][inst];
> + imsg_init(&iev->ibuf, ps->ps_pp->pp_pipes[dst][inst]);
> event_set(&iev->ev, iev->ibuf.fd, iev->events,
> iev->handler, iev->data);
> event_add(&iev->ev, NULL);
> }
> }
>
> - /* Exchange pipes between process. */
> - for (i = 0; i < PROC_MAX; i++) {
> - /* Parent is already connected with everyone. */
> - if (i == PROC_PARENT)
> - continue;
> + /* Distribute the socketpair()s for everyone. */
> + for (src = 0; src < PROC_MAX; src++)
> + for (dst = src; dst < PROC_MAX; dst++) {
> + /* Parent already distributed its fds. */
> + if (src == PROC_PARENT || dst == PROC_PARENT)
> + continue;
>
> - for (j = 0; j < ps->ps_instances[i]; j++)
> - proc_connectpeer(ps, i, j, &ps->ps_pipes[i][j]);
> - }
> + proc_open(ps, src, dst);
> + }
> }
>
> void
> @@ -230,17 +194,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_PARENT][0];
> + pb = &ps->ps_pipes[dst][proc];
> + if (socketpair(AF_UNIX,
> + SOCK_STREAM | SOCK_NONBLOCK | SOCK_CLOEXEC,
> + PF_UNSPEC, fds) == -1)
> + fatal("%s: socketpair", __func__);
> +
> + pa->pp_pipes[dst][proc] = fds[0];
> + pb->pp_pipes[PROC_PARENT][0] = fds[1];
> + }
> + }
>
> /* Engage! */
> proc_exec(ps, procs, nproc, argc, argv);
> @@ -409,9 +397,11 @@ void
> proc_open(struct privsep *ps, int src, int dst)
> {
> struct privsep_pipes *pa, *pb;
> + struct privsep_fd pf;
> int fds[2];
> unsigned int i, j;
>
> + /* 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. */
> @@ -420,9 +410,6 @@ 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)
> - continue;
> -
> if (socketpair(AF_UNIX,
> SOCK_STREAM | SOCK_NONBLOCK | SOCK_CLOEXEC,
> PF_UNSPEC, fds) == -1)
> @@ -430,6 +417,29 @@ proc_open(struct privsep *ps, int src, i
>
> pa->pp_pipes[dst][j] = fds[0];
> pb->pp_pipes[src][i] = fds[1];
> +
> + 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("%s: proc_compose_imsg", __func__);
> +
> + 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("%s: proc_compose_imsg", __func__);
> +
> + /*
> + * We have to flush to send the descriptors and close
> + * them to avoid the fd ramp on startup.
> + */
> + if (imsg_flush(&ps->ps_ievs[src][i].ibuf) == -1 ||
> + imsg_flush(&ps->ps_ievs[dst][j].ibuf) == -1)
> + fatal("%s: imsg_flush", __func__);
> +
> + imsg_event_add(&ps->ps_ievs[src][i]);
> + imsg_event_add(&ps->ps_ievs[dst][j]);
> }
> }
> }
> @@ -511,9 +521,6 @@ proc_run(struct privsep *ps, struct priv
> struct passwd *pw;
> const char *root;
> struct control_sock *rcs;
> -
> - if (ps->ps_noaction)
> - exit(0);
>
> log_procinit(p->p_title);
>
