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

[Arun R Bharadwaj]

* Anthony Liguori <address@hidden> [2010-11-01 08:40:36]:

> On 10/26/2010 09:14 AM, Arun R Bharadwaj wrote:
> >+Q.7: Apart from the global pool of threads, can I have my own private Queue 
> >?
> >+A.7: Yes, the threadlets framework allows subsystems to create their own 
> >private
> >+     queues with associated pools of threads.
> >+
> >+     - Define a PrivateQueue
> >+       ThreadletQueue myQueue;
> >+
> >+     - Initialize it:
> >+       threadlet_queue_init(&myQueue, my_max_threads, my_min_threads);
> >+       where my_max_threads is the maximum number of threads that can be in 
> >the
> >+       thread pool and my_min_threads is the minimum number of active 
> >threads
> >+       that can be in the thread-pool.
> >+
> >+     - Submit work:
> >+       submit_threadletwork_to_queue(&myQueue,&my_work);
> >+
> >+     - Cancel work:
> >+       cancel_threadletwork_on_queue(&myQueue,&my_work);
> >diff --git a/qemu-threadlets.c b/qemu-threadlets.c
> >new file mode 100644
> >index 0000000..8fc3be0
> >--- /dev/null
> >+++ b/qemu-threadlets.c
> >@@ -0,0 +1,167 @@
> >+/*
> >+ * Threadlet support for offloading tasks to be executed asynchronously
> >+ *
> >+ * Copyright IBM, Corp. 2008
> >+ * Copyright IBM, Corp. 2010
> >+ *
> >+ * Authors:
> >+ *  Anthony Liguori<address@hidden>
> >+ *  Aneesh Kumar K.V<address@hidden>
> >+ *  Gautham R Shenoy<address@hidden>
> >+ *
> >+ * This work is licensed under the terms of the GNU GPL, version 2.  See
> >+ * the COPYING file in the top-level directory.
> >+ */
> >+
> >+#include "qemu-threadlets.h"
> >+#include "osdep.h"
> >+
> >+#define MAX_GLOBAL_THREADS  64
> >+#define MIN_GLOBAL_THREADS  64
> >+static ThreadletQueue globalqueue;
> >+static int globalqueue_init;
> >+
> >+static void *threadlet_worker(void *data)
> >+{
> >+    ThreadletQueue *queue = data;
> >+
> >+    qemu_mutex_lock(&(queue->lock));
> 
> The use of parens here is unnecessary and unusual within the code
> base.  I'd prefer that they be dropped.
> 
> >+    while (1) {
> >+        ThreadletWork *work;
> >+        int ret = 0;
> >+
> >+        while (QTAILQ_EMPTY(&(queue->request_list))&&
> >+               (ret != ETIMEDOUT)) {
> >+            /* wait for cond to be signalled or broadcast for 1000s */
> >+            ret = qemu_cond_timedwait(&(queue->cond),
> >+                                    &(queue->lock), 10*100000);
> >+        }
> >+
> >+        assert(queue->idle_threads != 0);
> >+        if (QTAILQ_EMPTY(&(queue->request_list))) {
> >+            if (queue->cur_threads>  queue->min_threads) {
> >+                /* We retain the minimum number of threads */
> >+                break;
> 
> You set MIN_GLOBAL_THREADS to MAX_GLOBAL_THREADS which makes this
> dead code.  Why are you forcing the thread pool to remain a fixed
> size?+/**
> 
> >+ * 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)) {
> 
> Why have this unlikely?
> 
> >+/**
> >+ * deque_threadletwork: Cancel a task queued on the global queue.
> >+ * @work: Contains the information of the task that needs to be cancelled.
> >+ *
> >+ * Returns: 0 if the task is successfully cancelled.
> >+ *          1 otherwise.
> >+ */
> >+int deque_threadletwork(ThreadletWork *work)
> >+{
> >+    return deque_threadletwork_on_queue(&globalqueue, work);
> >+}
> 
> I really struggle with the use of namespaces here.
> 
> Wouldn't threadletwork_deque make more sense?  And I think dequeue
> is the proper spelling.
> 
> threadletwork is too long of a name too.
> 
> >+/**
> >+ * threadlet_queue_init: Initialize a threadlet queue.
> >+ * @queue: The threadlet queue to be initialized.
> >+ * @max_threads: Maximum number of threads processing the queue.
> >+ * @min_threads: Minimum number of threads processing the queue.
> >+ */
> >+void threadlet_queue_init(ThreadletQueue *queue,
> >+                                int max_threads, int min_threads)
> >+{
> >+    queue->cur_threads  = 0;
> >+    queue->idle_threads = 0;
> >+    queue->max_threads  = max_threads;
> >+    queue->min_threads  = min_threads;
> >+    QTAILQ_INIT(&(queue->request_list));
> >+    qemu_mutex_init(&(queue->lock));
> >+    qemu_cond_init(&(queue->cond));
> >+}
> 
> It's unclear to me why we need to have multiple threadlet pools.
> Why not just have a single global threadlet pool?
> 

According to Gautham's original patchset, there is a global threadlet
pool and also a private threadlet pool which any subsystem can
declare. I have just retained the same design. This is described in
the documentation file.

-arun
> Regards,
> 
> Anthony Liguori
>