Stefan Beller <sbel...@google.com> writes:
> + while (1) {
> + int i;
> + int output_timeout = 100;
> + int spawn_cap = 4;
> +
> + if (!no_more_task) {
> + for (i = 0; i < spawn_cap; i++) {
> + int code;
> + if (pp->nr_processes == pp->max_processes)
> + break;
> +
> + code = pp_start_one(pp);
> + if (!code)
> + continue;
> + if (code < 0) {
> + pp->shutdown = 1;
> + kill_children(pp, SIGTERM);
> + }
> + no_more_task = 1;
> + break;
> + }
> + }
> + if (no_more_task && !pp->nr_processes)
> + break;
I may have comments on other parts of this patch, but I noticed this
a bit hard to read while reading the end result.
Losing the outer "if (!no_more_task)" and replacing the above with
for (no_more_task = 0, i = 0;
!no_more_task && i < spawn_cap;
i++) {
... do things that may or may not set
... no_more_task
}
if (no_more_task && ...)
break;
would make it clear that regardless of spawn_cap, no_more_task is
honored.
Also I think that having the outer "if (!no_more_task)" and not
having "no_more_task = 0" after each iteration is buggy. Even when
next_task() told start_one() that it does not have more tasks for
now, as long as there are running processes, it is entirely plausible
that next call to next_task() can return "now we have some more task
to do".
Although I think it would make it unsightly, if you want to have the
large indentation that protects the spawn_cap loop from getting
entered, the condition would be
if (!pp->shutdown) {
for (... spawn_cap loop ...) {
...
}
}
That structure could make sense. But even then I would probably
write it more like
...
int spawn_cap = 4;
pp = pp_init(...);
while (1) {
int no_more_task = 0;
for (i = 0;
!no_more_task && !pp->shutdown && i < spawn_cap;
i++) {
...
code = start_one();
... set no_more_task to 1 as needed
... set pp->shutdown to 1 as needed
}
if (no_more_task && !pp->nr_processes)
break;
buffer_stderr(...);
output(...);
collect(...);
}
That is, you need to have two independent conditions that tell you
not to spawn any new task:
(1) You called start_one() repeatedly and next_task() said "nothing
more for now", so you know calling start_one() one more time
without changing other conditions (like draining output from
running processes and culling finished ones) will not help.
Letting other parts of the application that uses this scheduler
loop (i.e. drain output, cull finished process, etc.) may
change the situation and you _do_ need to call start_one() when
the next_task() merely said "nothing more for now".
That is what no_more_task controls.
(2) The application said "I want the system to be gracefully shut
down". next_task() may also have said "nothing more for now"
and you may have set no_more_task in response to it, but unlike
(1) above, draining and culling must be done only to shut the
system down, the application does not want new processes to be
added. You do not want to enter the spawn_cap loop when it
happens.
That is what pp->shutdown controls.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
