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

[Jamie Lokier]

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

There's some mild abuse of the mutex/condvar going on.

As (queue->exit || queue->idle_threads > queue->min_threads) is a
condition for breaking out of the loop, that condition ought to be
checked in the mutex->cond_wait region, but it isn't.

It doesn't matter here because the queue is empty when queue->exit,
and the idle > min_threads condition can't become true.

> >The min/max_threads parameters of the queue are currently immutable, 
> >so it can never happen that a thread has to be expired while it's 
> >waiting.  It may well become true in the future, in which case the 
> >condvar will have to be broadcast when min_threads changes.

Broadcasting when min_threads decreases wouldn't be enough, because
min_threads isn't checked inside the mutex->cond_wait region.

-- Jamie