Re: [Qemu-devel] [PATCH 1/3] Introduce threadlets

[Balbir Singh <balbir@linux.vnet.ibm.com>]

* Venkateswararao Jujjuri (JV) <jvrao@linux.vnet.ibm.com> [2010-10-19 20:46:35]:

> >> I think this is a lot more fragile.  You're relying on the fact that
> >> signal will not cause the signalled thread to actually awaken until
> >> we release the lock and doing work after signalling that the
> >> signalled thread needs to be completed before it wakes up.
> >>
> >> I think you're a lot more robust in the long term if you treat
> >> condition signalling as a hand off point because it makes the code a
> >> lot more explicit about what's happening.
> >>
> > 
> > OK, here is a situation that can happen
> > 
> > T1                              T2
> > ---                             ---
> > threadlet                       submit_threadletwork_to_queue
> > (sees condition as no work)     mutex_lock
> > qemu_cond_timedwait             add_work
> > ...                             mutex_unlock
> > 
> > T3
> > --
> > cancel_threadlet_work_on_queue
> > mutex_lock (grabs it) before T1 can
> > cancels the work
> > 
> > 
> >                                 qemu_cond_signal
> > 
> > T1
> > --
> > Grabs mutex_lock (from within cond_timedwait)
> > Now there is no work to do, the condition
> > has changed before the thread wakes up
> 
> So what? It won't find any work and goes back to sleep or exits.
>

Spurious wakeups are not good - they waste CPU cycles, consume energy.
Beyond that if we look at generic design

a. We want the thread condition to not change before it wakes up
(reduce that window at-least)
b. Although we don't care about thread priorities today in threadlet,
if we ever did and by good design you'd want the thread your waking up
to be contending for the mutex as soon the notifier releases the lock,
otherwise a lower priority thread can starve the original sleeper.

The code as posted today, does not have functional issues except for
opening up the window for spurious wakeups.
 
> idle_threads is decremented only in threadlet_worker(). Given that
> we have a threadlet that is not doing anywork the assert should never hit unless
> something horribly wrong .
>
 

-- 
	Three Cheers,
	Balbir