On 10/19/2010 2:36 PM, Anthony Liguori wrote: > On 10/19/2010 01:36 PM, Balbir Singh wrote: >>> + qemu_mutex_lock(&(queue->lock)); >>> + while (1) { >>> + ThreadletWork *work; >>> + int ret = 0; >>> + >>> + while (QTAILQ_EMPTY(&(queue->request_list))&& >>> + (ret != ETIMEDOUT)) { >>> + ret = qemu_cond_timedwait(&(queue->cond), >>> + &(queue->lock), 10*100000); >>> >> Ewww... what is 10*100000, can we use something more meaningful >> please? >> > > A define is fine but honestly, it's pretty darn obvious what it means... > >>> + } >>> + >>> + assert(queue->idle_threads != 0); >>> >> This assertion holds because we believe one of the idle_threads >> actually did the dequeuing, right? >> > > An idle thread is a thread is one that is not doing work. At this point in > the > code, we are not doing any work (yet) so if idle_threads count is zero, > something is horribly wrong. We're also going to unconditionally decrement in > the future code path which means that if idle_threads is 0, it's going to > become > -1. > > The use of idle_thread is to detect whether it's necessary to spawn an > additional thread. > >>> + if (QTAILQ_EMPTY(&(queue->request_list))) { >>> + if (queue->cur_threads> queue->min_threads) { >>> + /* We retain the minimum number of threads */ >>> + break; >>> + } >>> + } else { >>> + work = QTAILQ_FIRST(&(queue->request_list)); >>> + QTAILQ_REMOVE(&(queue->request_list), work, node); >>> + >>> + queue->idle_threads--; >>> + qemu_mutex_unlock(&(queue->lock)); >>> + >>> + /* execute the work function */ >>> + work->func(work); >>> + >>> + qemu_mutex_lock(&(queue->lock)); >>> + queue->idle_threads++; >>> + } >>> + } >>> + >>> + queue->idle_threads--; >>> + queue->cur_threads--; >>> + qemu_mutex_unlock(&(queue->lock)); >>> + >>> + return NULL; >>> >> Does anybody do a join on the exiting thread from the pool? >> > > No. The thread is created in a detached state. > >>> +} >>> + >>> +static void spawn_threadlet(ThreadletQueue *queue) >>> +{ >>> + QemuThread thread; >>> + >>> + queue->cur_threads++; >>> + queue->idle_threads++; >>> + >>> + qemu_thread_create(&thread, threadlet_worker, queue); >>> >> >>> +} >>> + >>> +/** >>> + * submit_threadletwork_to_queue: Submit a new task to a private queue to >>> be >>> + * executed asynchronously. >>> + * @queue: Per-subsystem private queue to which the new task needs >>> + * to be submitted. >>> + * @work: Contains information about the task that needs to be submitted. >>> + */ >>> +void submit_threadletwork_to_queue(ThreadletQueue *queue, ThreadletWork >>> *work) >>> +{ >>> + qemu_mutex_lock(&(queue->lock)); >>> + if (queue->idle_threads == 0&& queue->cur_threads< >>> queue->max_threads) { >>> + spawn_threadlet(queue); >>> >> So we hold queue->lock, spawn the thread, the spawned thread tries to >> acquire queue->lock >> > > Yup. > >>> + } >>> + QTAILQ_INSERT_TAIL(&(queue->request_list), work, node); >>> + qemu_mutex_unlock(&(queue->lock)); >>> + qemu_cond_signal(&(queue->cond)); >>> >> In the case that we just spawned the threadlet, the cond_signal is >> spurious. If we need predictable scheduling behaviour, >> qemu_cond_signal needs to happen with queue->lock held. >> > > It doesn't really affect predictability.. > >> I'd rewrite the function as >> >> /** >> * submit_threadletwork_to_queue: Submit a new task to a private queue to be >> * executed asynchronously. >> * @queue: Per-subsystem private queue to which the new task needs >> * to be submitted. >> * @work: Contains information about the task that needs to be submitted. >> */ >> void submit_threadletwork_to_queue(ThreadletQueue *queue, ThreadletWork >> *work) >> { >> qemu_mutex_lock(&(queue->lock)); >> if (queue->idle_threads == 0&& (queue->cur_threads< >> queue->max_threads)) { >> spawn_threadlet(queue); >> } else { >> qemu_cond_signal(&(queue->cond)); >> } >> QTAILQ_INSERT_TAIL(&(queue->request_list), work, node); >> qemu_mutex_unlock(&(queue->lock)); >> } >> > > 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.
Given that qemu_cond_timedwait() need to get the queue->lock before returning the singalled thread will wakeup and wait on the queue->lock. - JV > > 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. > >>> +/** >>> + * submit_threadletwork: Submit to the global queue a new task to be >>> executed >>> + * asynchronously. >>> + * @work: Contains information about the task that needs to be submitted. >>> + */ >>> +void submit_threadletwork(ThreadletWork *work) >>> +{ >>> + if (unlikely(!globalqueue_init)) { >>> + threadlet_queue_init(&globalqueue, MAX_GLOBAL_THREADS, >>> + MIN_GLOBAL_THREADS); >>> + globalqueue_init = 1; >>> + } >>> >> What protects globalqueue_init? >> > > qemu_mutex, and that unlikely is almost certainly a premature optimization. > > Regards, > > Anthony Liguori > >