On Tue, Sep 29, 2015 at 8:12 PM, Junio C Hamano <gits...@pobox.com> wrote: > 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 indeed better to read. Though if we reset `no_more_task` each iteration of the loop by having its declaration inside the loop, the condition for exiting the loop needs to be: if ((no_more_task || pp->shutdown) && !pp->nr_processes) break; for correctness. When looking at that condition, I realized that I implicitly assumed the workflow, where get_next_task returns 0 intermittently, to be a second class citizen. I reworded the documentation as well there. > > 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