Re: [PATCH 2/5] thread-utils: add a threaded task queue

From: Stefan Beller <sbeller@google.com>
Date: Aug 27, 2015, 17:02 UTC
Message-ID: <CAGZ79kZQ6z8L6Cp-Z56EBq1e-700NsxW9XmFq7fXuMwtUyRSvg@mail.gmail.com>
In-Reply-To: <a35977776a7ea97b0ddd4d1921e3e95f@www.dscho.org>

On Thu, Aug 27, 2015 at 5:59 AM, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:

Show 24 quoted lines

> Hi Stefan,
>
> On 2015-08-27 02:52, Stefan Beller wrote:
>
>> diff --git a/run-command.c b/run-command.c
>> index 28e1d55..cb15cd9 100644
>> --- a/run-command.c
>> +++ b/run-command.c
>> @@ -668,6 +668,22 @@ int git_atexit(void (*handler)(void))
>>
>>  #endif
>>
>> +void setup_main_thread(void)
>> [...]
>>
>> diff --git a/thread-utils.c b/thread-utils.c
>> index a2135e0..b45ab92 100644
>> --- a/thread-utils.c
>> +++ b/thread-utils.c
>> [...]
>> +/* FIXME: deduplicate this code with run-command.c */
>> +static void setup_main_thread(void)
>
> Do you remember off-hand why the code could not be moved to thread-utils.c wholesale? Just curious.

The code in run-command has a few things regarding the struct async handling in there, which we don't need/want. I just realized there is some duplicate code, but I couldn't cut it clearly out.

Show 25 quoted lines

>
>> +#else /* NO_PTHREADS */
>> +
>> +struct task_queue {
>> +     int early_return;
>> +};
>> +
>> +struct task_queue *create_task_queue(unsigned max_threads)
>> +{
>> +     struct task_queue *tq = xmalloc(sizeof(*tq));
>> +
>> +     tq->early_return = 0;
>> +}
>> +
>> +void add_task(struct task_queue *tq,
>> +           int (*fct)(struct task_queue *tq, void *task),
>
> Might make sense to typedef this... Maybe task_t?
>
>> +           void *task)
>> +{
>> +     if (tq->early_return)
>> +             return;
>
> Ah, so "early_return" actually means "interrupted" or "canceled"?

The early_return is meant to return early in case of an error in some thread. In the threaded version, the `dispatcher` is executed in each thread. It gets its new tasks via `next_task`, which takes the early_return value from the thread, ORs it into the early_return of the task queue (which all threads have access to). So by the ORing into the task queues early_return the signal to abort early is propagated to a place all threads have access to. And in case that value is set, the `next_task` will return NULL as an indication to cleanup and pthread_exit.

Show 5 quoted lines

>
> I guess I will have to set aside some time to wrap my head around the way tasks are handled here, in particular how the two `early_return` variables (`dispatcher()`'s local variable and the field in the `task_queue`) interact.
>
> Thanks!
> Dscho