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

[Paolo Bonzini <pbonzini@redhat.com>]

> +block-obj-y += qemu-thread.o
> +block-obj-y += async-work.o

These should be (at least for now) block-obj-$(CONFIG_POSIX).

> +        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.

> +        if (QTAILQ_EMPTY(&(queue->request_list)))
> +            goto check_exit;

What's the reason for the goto?  {...} works just as well.

> +/**
> + * 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.

> +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.

> + * Generalization based on posix-aio emulation code.

No need to specify these as long as the original authors are attributed 
properly.

> +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.

> +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").

Paolo