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
