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

[Anthony Liguori <aliguori@linux.vnet.ibm.com>]

On 06/16/2010 09:52 AM, Paolo Bonzini wrote:
> On 06/16/2010 04:38 PM, Anthony Liguori wrote:
>> On 06/16/2010 09:29 AM, Paolo Bonzini wrote:
>>> On 06/16/2010 04:22 PM, Jamie Lokier wrote:
>>>> Paolo Bonzini wrote:
>>>>> 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.
>>>>
>>>> Are you sure? It looks like it also expires idle threads after a
>>>> fixed amount of idle time.
>>>
>>> Unnecessary idle threads are immediately expired as soon as the
>>> threadlet exits if ncecessary, since here
>>
>> If a threadlet is waiting to consume more work, unless we do a
>> pthread_cancel (I dislike cancellation) it will keep waiting until it
>> gets more work (which would mean it's not actually idle)...
>
> Agreed---no cancellation, please.
>
> BTW it's obviously okay with signaling the condition when a threadlet 
> is submitted.  But when something affects all queue's workers 
> (flush_threadlet_queue) you want a broadcast and using expiration as a 
> substitute is fishy.

IMHO, there shouldn't be a need for flush_threadlet_queue.  It doesn't 
look used in the aio conversion and if virtio-9p needs it, I suspect 
something is wrong.

Regards,

Anthony Liguori

>>> + queue->idle_threads++;
>>> +
>>> +check_exit:
>>> + if (queue->exit || ((queue->idle_threads > 0) &&
>>> + (queue->cur_threads > queue->min_threads))) {
>>> + /* We exit the queue or we retain minimum number of threads */
>>> + break;
>>> + }
>>>
>>> queue->idle_threads > 0 will always be true (so maybe that should be
>>> changed into an assertion: "this thread is idle, so there must be idle
>>> threads").
>>
>> queue->exit could be true though so it's necessary to at least check
>> that condition.
>
> Yes, of course.  The correct test should be:
>
>   if (queue->exit || queue->cur_threads > queue->min_threads)
>
> But queue->idle_threads will be > 0 even if coming via the goto (which 
> should be eliminated).
>
> Or maybe no.  After flushing you still want min_threads threads to 
> run.  The correct thing then would be:
>
>     do {
>         ...
>         assert (queue->idle_threads > 0);
>         if (queue->exit) {
>             /* Threads waiting on the barrier cannot do work.  */
>             queue->idle_threads--;
>             qemu_mutex_unlock(&(queue->lock));
>             qemu_barrier_wait(&queue->barr);
>             qemu_mutex_lock(&(queue->lock));
>             queue->idle_threads++;
>         }
>     } while (queue->cur_threads <= queue->min_threads);
>
>     queue->idle_threads--;
>     queue->cur_threads--;
>     qemu_mutex_unlock(&queue->lock);
>     return NULL;
>
> So, if min_threads were changed, broadcasting the condition would be 
> enough to exit unwanted threads one at a time, as soon as it grabs the 
> lock.
>
> Paolo