[Qemu-devel] Re: [PATCH V4 2/3] qemu: Generic task offloading framework: threadlets

[Gautham R Shenoy]

On Wed, Jun 16, 2010 at 02:34:16PM +0200, Paolo Bonzini wrote:
>> +block-obj-y += qemu-thread.o
>> +block-obj-y += async-work.o
>
> These should be (at least for now) block-obj-$(CONFIG_POSIX).

Right. Will fix that.
>
>> +        while (QTAILQ_EMPTY(&(queue->request_list))&&
>> +               (ret != ETIMEDOUT)) {
>> +            ret = qemu_cond_timedwait(&(queue->cond),
>> +                                    &(queue->lock), 10*100000);
>> +        }
>
> Using qemu_cond_timedwait is a hack for not properly broadcasting the 
> condvar in flush_threadlet_queue.

I think Anthony answered this one.
>
>> +        if (QTAILQ_EMPTY(&(queue->request_list)))
>> +            goto check_exit;
>
> What's the reason for the goto?  {...} works just as well.

Yes {...} works.

Besides, this two step condition checking is broken and can
cause the threads to exit even in the presence of unprocessed
queued ThreadletWork items.
Will fix this in the v5 (hopefully there will be one :-))

>
>> +/**
>> + * flush_threadlet_queue: Wait till completion of all the submitted tasks
>> + * @queue: Queue containing the tasks we're waiting on.
>> + */
>> +void flush_threadlet_queue(ThreadletQueue *queue)
>> +{
>> +    qemu_mutex_lock(&queue->lock);
>> +    queue->exit = 1;
>> +
>> +    qemu_barrier_init(&queue->barr, queue->cur_threads + 1);
>> +    qemu_mutex_unlock(&queue->lock);
>> +
>> +    qemu_barrier_wait(&queue->barr);
>
> Can be implemented just as well with queue->cond and a loop waiting for 
> queue->cur_threads == 0.  This would remove the need to implement barriers 
> in qemu-threads (especially for Win32).  Anyway whoever will contribute 
> Win32 qemu-threads can do it, since it's not hard.

That was the other option I had considered before going for barriers,
for no particular reason. Now, considering that barriers are not
welcome, I will implement this method.

>
>> +int cancel_threadlet_common(ThreadletWork *work)
>> +{
>> +    return cancel_threadlet(&globalqueue, work);
>> +}
>
> I would prefer *_threadlet to be the globalqueue function (and 
> flush_threadlets) and queue_*_threadlet to be the special-queue function. I 
> should have spoken earlier probably, but please consider this if there will 
> be a v5.

Sure, will do that.
>
>> + * Generalization based on posix-aio emulation code.
>
> No need to specify these as long as the original authors are attributed 
> properly.

Ok!
>
>> +static inline void threadlet_queue_init(ThreadletQueue *queue,
>> +                                int max_threads, int min_threads)
>> +{
>> +    queue->cur_threads  = 0;
>> +    queue->idle_threads = 0;
>> +    queue->exit = 0;
>> +    queue->max_threads  = max_threads;
>> +    queue->min_threads  = min_threads;
>> +    QTAILQ_INIT(&(queue->request_list));
>> +    QTAILQ_INIT(&(queue->threadlet_work_pool));
>> +    qemu_mutex_init(&(queue->lock));
>> +    qemu_cond_init(&(queue->cond));
>> +}
>
> No need to make this inline.

Will fix this.
>
>> +extern void threadlet_submit(ThreadletQueue *queue,
>> +                              ThreadletWork *work);
>> +
>> +extern void threadlet_submit_common(ThreadletWork *work);
>> +
>> +extern int cancel_threadlet(ThreadletQueue *queue, ThreadletWork *work);
>> +extern int cancel_threadlet_common(ThreadletWork *work);
>> +
>> +
>> +extern void flush_threadlet_queue(ThreadletQueue *queue);
>> +extern void flush_common_threadlet_queue(void);
>
> Please make the position of the verb consistent (e.g. "submit_threadlet").

Overlooked threadlet_submit() in the rename process. It has to be
submit_threadlet(). Will fix.

Thanks for the detailed review.
Regards
gautham.
>
> Paolo