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

[Paolo Bonzini <pbonzini@redhat.com>]

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.

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