* Anthony Liguori <anth...@codemonkey.ws> [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<aligu...@us.ibm.com>
> >+ * Aneesh Kumar K.V<aneesh.ku...@linux.vnet.ibm.com>
> >+ * Gautham R Shenoy<gautham.she...@gmail.com>
> >+ *
> >+ * 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
>
