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

[Jamie Lokier]

Jamie Lokier wrote:
> 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.

Sorry, thinko.  It does matter when queue->exit, precisely because the
queue is empty :-)

Even cond_broadcast after queue->exit is set isn't enough to remove
the need for the timed wait hack.

Putting the whole condition inside the mutex->cond_wait region, not
just empty queue test, will remove the need for timed wait.  Broadcast
is still needed, or alternatively a cond_signal from each exiting
thread will allow them to wake and close without a thundering herd.

-- Jamie