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

[Balbir Singh]

* Anthony Liguori <address@hidden> [2010-10-19 16:36:31]:

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

We can hit this assert if pthread_cond_signal() is called outside of
the mutex, let me try and explain below
 
> >>+        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.
>

That makes sense, thanks for clarifying
 
> >>+}
> >>+
> >>+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.
> 
> 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


The man page also states

"however, if predictable scheduling behavior is required, then that
mutex shall be locked by the thread calling pthread_cond_broadcast()
or pthread_cond_signal()"

> >>+/**
> >>+ * 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
> 

-- 
        Three Cheers,
        Balbir