[1/3] Introduce threadlets

From: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>

This patch creates a generic asynchronous-task-offloading infrastructure named
threadlets. The core idea has been borrowed from the threading framework that
is being used by paio.

The reason for creating this generic infrastructure is so that other subsystems,
such as virtio-9p could make use of it for offloading tasks that could block.

The patch creates a global queue on-to which subsystems can queue their tasks to
be executed asynchronously.

The patch also provides API's that allow a subsystem to create a private queue
with an associated pool of threads.

[ego@in.ibm.com: Facelift of the code, Documentation, cancel_threadlet
and other helpers]

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
Signed-off-by: Gautham R Shenoy <ego@in.ibm.com>
Signed-off-by: Sripathi Kodi <sripathik@in.ibm.com>
Signed-off-by: Arun R Bharadwaj <arun@linux.vnet.ibm.com>
---
 Makefile.objs          |    3 +
 docs/async-support.txt |  141 ++++++++++++++++++++++++++++++++++++++++
 qemu-threadlets.c      |  169 ++++++++++++++++++++++++++++++++++++++++++++++++
 qemu-threadlets.h      |   48 ++++++++++++++
 4 files changed, 360 insertions(+), 1 deletions(-)
 create mode 100644 docs/async-support.txt
 create mode 100644 qemu-threadlets.c
 create mode 100644 qemu-threadlets.h

On Wed, Oct 13, 2010 at 4:31 PM, Arun R Bharadwaj
<arun@linux.vnet.ibm.com> wrote:
> From: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>
> This patch creates a generic asynchronous-task-offloading infrastructure named
> threadlets. The core idea has been borrowed from the threading framework that
> is being used by paio.
>
> The reason for creating this generic infrastructure is so that other subsystems,
> such as virtio-9p could make use of it for offloading tasks that could block.
>
> The patch creates a global queue on-to which subsystems can queue their tasks to
> be executed asynchronously.
>
> The patch also provides API's that allow a subsystem to create a private queue
> with an associated pool of threads.
>
> [ego@in.ibm.com: Facelift of the code, Documentation, cancel_threadlet
> and other helpers]
>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> Signed-off-by: Gautham R Shenoy <ego@in.ibm.com>
> Signed-off-by: Sripathi Kodi <sripathik@in.ibm.com>
> Signed-off-by: Arun R Bharadwaj <arun@linux.vnet.ibm.com>
> ---
>  Makefile.objs          |    3 +
>  docs/async-support.txt |  141 ++++++++++++++++++++++++++++++++++++++++
>  qemu-threadlets.c      |  169 ++++++++++++++++++++++++++++++++++++++++++++++++
>  qemu-threadlets.h      |   48 ++++++++++++++
>  4 files changed, 360 insertions(+), 1 deletions(-)
>  create mode 100644 docs/async-support.txt
>  create mode 100644 qemu-threadlets.c
>  create mode 100644 qemu-threadlets.h
>
> diff --git a/Makefile.objs b/Makefile.objs
> index cd5a24b..2cf8aba 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -9,6 +9,8 @@ qobject-obj-y += qerror.o
>
>  block-obj-y = cutils.o cache-utils.o qemu-malloc.o qemu-option.o module.o
>  block-obj-y += nbd.o block.o aio.o aes.o osdep.o qemu-config.o
> +block-obj-$(CONFIG_POSIX) += qemu-thread.o
> +block-obj-$(CONFIG_POSIX) += qemu-threadlets.o
>  block-obj-$(CONFIG_POSIX) += posix-aio-compat.o
>  block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
>
> @@ -124,7 +126,6 @@ endif
>  common-obj-y += $(addprefix ui/, $(ui-obj-y))
>
>  common-obj-y += iov.o acl.o
> -common-obj-$(CONFIG_THREAD) += qemu-thread.o
>  common-obj-y += notify.o event_notifier.o
>  common-obj-y += qemu-timer.o
>
> diff --git a/docs/async-support.txt b/docs/async-support.txt
> new file mode 100644
> index 0000000..2e8adc9
> --- /dev/null
> +++ b/docs/async-support.txt

Why not call it docs/threadlets.txt?

> @@ -0,0 +1,141 @@
> +== How to use the various asynchronous models supported in Qemu ==

This only describes threadlets.

> +
> +== Threadlets ==
> +
> +Q.1: What are threadlets ?
> +A.1: Threadlets is an infrastructure within QEMU that allows other subsystems
> +     to offload possibly blocking work to a queue to be processed by a pool
> +     of threads asynchrnonously.

asynchronously typo

> +
> +Q.2: When would one want to use threadlets ?
> +A.2: Threadlets are useful when there are operations that can be performed
> +     outside the context of the VCPU/IO threads inorder to free these latter
> +     to service any other guest requests.
> +
> +Q.3: I have some work that can be executed in an asynchronous context. How
> +     should I go about it ?
> +A.3: One could follow the steps listed below:
> +
> +     - Define a function which would do the asynchrnous work.

asynchronous typo

> +       void my_threadlet_func(ThreadletWork *work)

Usually these functions will be static.

> +       {
> +       }
> +
> +     - Declare an object of type ThreadletWork;
> +       ThreadletWork work;
> +
> +
> +     - Assign a value to the "func" member of ThreadletWork object.
> +       work.func = my_threadlet_func;
> +
> +     - Submit the threadlet to the global queue.
> +       submit_threadletwork(&work);
> +
> +     - Continue servicing some other guest operations.
> +
> +Q.4: I want to my_threadlet_func to access some non-global data. How do I do
> +     that ?
> +A.4: Suppose you want my_threadlet_func to access some non-global data-object
> +     of type myPrivateData. In that case one could follow the following steps.
> +
> +     - Define a member of the type ThreadletWork within myPrivateData.
> +       typdef myPrivateData {

typedef typo, struct missing

Also, the QEMU coding style usually requires CapsNames for struct types...

> +               ...;
> +               ...;
> +               ...;
> +               ThreadletWork work;
> +       } myPrivateData;
> +
> +       myPrivateData myData;

...and myData would be my_data.

> +
> +     - Initialize myData.work as described in A.3
> +       myData.work.func = my_threadlet_func;
> +       submit_threadletwork(&myData.work);
> +
> +     - Access the myData object inside my_threadlet_func() using container_of
> +       primitive
> +       void my_threadlet_func(ThreadletWork *work)
> +       {
> +               myPrivateData *mydata_ptr;
> +               mydata_ptr = container_of(work, myPrivateData, work);
> +
> +               /* mydata_ptr now points to myData object */
> +       }
> +
> +Q.5: Are there any precautions one must take while sharing data with the
> +     Asynchrnous thread-pool ?

asynchronous typo

> +A.5: Yes, make sure that the helper function of the type my_threadlet_func()
> +     does not access/modify data when it can be accessed or modified in the
> +     context of VCPU thread or IO thread. This is because the asynchronous
> +     threads in the pool can run in parallel with the VCPU/IOThreads as shown
> +     in the figure.
> +
> +     A typical workflow is as follows:
> +
> +              VCPU/IOThread
> +                   |
> +                   | (1)
> +                   |
> +                   V
> +                Offload work              (2)
> +      |-------> to threadlets -----------------------------> Helper thread
> +      |            |                                               |
> +      |            |                                               |
> +      |            | (3)                                           | (4)
> +      |            |                                               |
> +      |         Handle other Guest requests                        |
> +      |            |                                               |
> +      |            |                                               V
> +      |            | (3)                                  Signal the I/O Thread
> +      |(6)         |                                               |
> +      |            |                                              /
> +      |            |                                             /
> +      |            V                                            /
> +      |          Do the post <---------------------------------/
> +      |          processing               (5)
> +      |            |
> +      |            | (6)
> +      |            V
> +      |-Yes------ More async work?
> +                   |
> +                   | (7)
> +                  No
> +                   |
> +                   |
> +                   .
> +                   .
> +
> +    Hence one needs to make sure that in the steps (3) and (4) which run in
> +    parallel, any global data is accessed within only one context.
> +
> +Q.6: I have queued a threadlet which I want to cancel. How do I do that ?
> +A.6: Threadlets framework provides the API cancel_threadlet:
> +       - int cancel_threadletwork(ThreadletWork *work)
> +
> +     The API scans the ThreadletQueue to see if (work) is present. If it finds
> +     work, it'll dequeue work and return 0.
> +
> +     On the other hand, if it does not find the (work) in the ThreadletQueue,
> +     then it'll return 1. This can imply two things. Either the work is being
> +     processed by one of the helper threads or it has been processed. The
> +     threadlet infrastructure currently _does_not_ distinguish between these
> +     two and the onus is on the caller to do that.
> +
> +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..1442122
> --- /dev/null
> +++ b/qemu-threadlets.c
> @@ -0,0 +1,169 @@
> +/*
> + * Threadlet support for offloading tasks to be executed asynchronously
> + *
> + * Copyright IBM, Corp. 2008
> + * Copyright IBM, Corp. 2010
> + *
> + * Authors:
> + *  Anthony Liguori     <aliguori@us.ibm.com>
> + *  Aneesh Kumar K.V    <aneesh.kumar@linux.vnet.ibm.com>
> + *  Gautham R Shenoy    <ego@in.ibm.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
> +ThreadletQueue globalqueue;

This should be static.

> +static int globalqueue_init;
> +
> +static void *threadlet_worker(void *data)
> +{
> +    ThreadletQueue *queue = data;
> +
> +    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);
> +        }
> +
> +        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;
> +            }
> +        } 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;
> +}
> +
> +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);
> +    }
> +    QTAILQ_INSERT_TAIL(&(queue->request_list), work, node);
> +    qemu_mutex_unlock(&(queue->lock));
> +    qemu_cond_signal(&(queue->cond));

Worker thread signalling and spawning has race conditions.  See my
previous email:

"There are race conditions here:

1. When a new threadlet is started because there are no idle threads,
qemu_cond_signal() may fire a blank because the threadlet isn't inside
qemu_cond_timedwait() yet.  The result, the work item is deadlocked
until another thread grabs more work off the queue.  If I'm reading
the code correctly this bug is currently present!
2. Moving qemu_cond_signal() outside queue->lock is dangerous for the
same reason: you need to be careful not to qemu_cond_signal() when the
thread isn't inside qemu_cond_timedwait()."

> +}
> +
> +/**
> + * 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;
> +    }
> +
> +    submit_threadletwork_to_queue(&globalqueue, work);
> +}
> +
> +/**
> + * cancel_threadletwork_on_queue: Cancel a task queued on a Queue.
> + * @queue: The queue containing the task to be cancelled.
> + * @work: Contains the information of the task that needs to be cancelled.
> + *
> + * Returns: 0 if the task is successfully cancelled.
> + *          1 otherwise.
> + */
> +int cancel_threadletwork_on_queue(ThreadletQueue *queue, ThreadletWork *work)
> +{
> +    ThreadletWork *ret_work;
> +    int found = 0;
> +
> +    qemu_mutex_lock(&(queue->lock));
> +    QTAILQ_FOREACH(ret_work, &(queue->request_list), node) {
> +        if (ret_work == work) {
> +            QTAILQ_REMOVE(&(queue->request_list), ret_work, node);
> +            found = 1;
> +            break;
> +        }
> +    }
> +    qemu_mutex_unlock(&(queue->lock));
> +
> +    if (found) {
> +        return 0;
> +    }
> +
> +    return 1;

Perhaps invert the logic of "found" and just call it "ret", then you
don't need this if statement that flips its value.

> +}
> +
> +/**
> + * cancel_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 cancel_threadletwork(ThreadletWork *work)
> +{
> +    return cancel_threadletwork_on_queue(&globalqueue, work);
> +}
> +
> +/**
> + * 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));
> +}
> diff --git a/qemu-threadlets.h b/qemu-threadlets.h
> new file mode 100644
> index 0000000..3df9b10
> --- /dev/null
> +++ b/qemu-threadlets.h
> @@ -0,0 +1,48 @@
> +/*
> + * Threadlet support for offloading tasks to be executed asynchronously
> + *
> + * Copyright IBM, Corp. 2008
> + * Copyright IBM, Corp. 2010
> + *
> + * Authors:
> + *  Anthony Liguori     <aliguori@us.ibm.com>
> + *  Aneesh Kumar K.V    <aneesh.kumar@linux.vnet.ibm.com>
> + *  Gautham R Shenoy    <ego@in.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.
> + */
> +
> +#ifndef QEMU_ASYNC_WORK_H
> +#define QEMU_ASYNC_WORK_H

QEMU_THREADLETS_H?

> +
> +#include "qemu-queue.h"
> +#include "qemu-common.h"
> +#include "qemu-thread.h"
> +
> +typedef struct ThreadletQueue
> +{
> +    QemuMutex lock;
> +    QemuCond cond;
> +    int max_threads;
> +    int min_threads;
> +    int cur_threads;
> +    int idle_threads;
> +    QTAILQ_HEAD(, threadlet_work) request_list;
> +} ThreadletQueue;
> +
> +typedef struct threadlet_work

struct ThreadletWork follows coding style more closely.

Stefan

I forgot to add that the semantics of cancellation make it difficult
to write correct user code.  Every cancellation user needs to add
extra synchronization after the cancel call to handle the case where
the work is currently executing.

This seems tricky to me and I suspect code using this interface will
be buggy.  How about the following?
1. Add a return value indicating that the work is currently executing
(this still requires the caller to add extra synchronization but is at
least explicit) versus work is no longer on the list.
2. Add a flag to block until the work has been cancelled or completed.
 This is useful to callers who are allowed to block.

Stefan

On Thu, Oct 14, 2010 at 10:15:30AM +0100, Stefan Hajnoczi wrote:
> I forgot to add that the semantics of cancellation make it difficult
> to write correct user code.  Every cancellation user needs to add
> extra synchronization after the cancel call to handle the case where
> the work is currently executing.
> 
> This seems tricky to me and I suspect code using this interface will
> be buggy.  How about the following?
> 1. Add a return value indicating that the work is currently executing
> (this still requires the caller to add extra synchronization but is at
> least explicit) versus work is no longer on the list.
> 2. Add a flag to block until the work has been cancelled or completed.
>  This is useful to callers who are allowed to block.
> 
In Linux kernel you usually have two function cancel() and
cancel_sync(). Second variant waits for work completion.

--
			Gleb.

On 10/14/2010 11:15 AM, Stefan Hajnoczi wrote:
> I forgot to add that the semantics of cancellation make it difficult
> to write correct user code.  Every cancellation user needs to add
> extra synchronization after the cancel call to handle the case where
> the work is currently executing.
>
> This seems tricky to me and I suspect code using this interface will
> be buggy.  How about the following?
> 1. Add a return value indicating that the work is currently executing
> (this still requires the caller to add extra synchronization but is at
> least explicit) versus work is no longer on the list.
> 2. Add a flag to block until the work has been cancelled or completed.
>   This is useful to callers who are allowed to block.

Blocking is somewhat against the spirit of the thing, no?  While I agree 
that the current cancel API is hard to use correctly, blocking defeats 
the purpose of the API.

On 10/14/2010 2:02 AM, Stefan Hajnoczi wrote:
> On Wed, Oct 13, 2010 at 4:31 PM, Arun R Bharadwaj
> <arun@linux.vnet.ibm.com> wrote:
>> From: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>>
>> This patch creates a generic asynchronous-task-offloading infrastructure named
>> threadlets. The core idea has been borrowed from the threading framework that
>> is being used by paio.
>>
>> The reason for creating this generic infrastructure is so that other subsystems,
>> such as virtio-9p could make use of it for offloading tasks that could block.
>>
>> The patch creates a global queue on-to which subsystems can queue their tasks to
>> be executed asynchronously.
>>
>> The patch also provides API's that allow a subsystem to create a private queue
>> with an associated pool of threads.
>>
>> [ego@in.ibm.com: Facelift of the code, Documentation, cancel_threadlet
>> and other helpers]
>>
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>> Signed-off-by: Gautham R Shenoy <ego@in.ibm.com>
>> Signed-off-by: Sripathi Kodi <sripathik@in.ibm.com>
>> Signed-off-by: Arun R Bharadwaj <arun@linux.vnet.ibm.com>
>> ---
>>  Makefile.objs          |    3 +
>>  docs/async-support.txt |  141 ++++++++++++++++++++++++++++++++++++++++
>>  qemu-threadlets.c      |  169 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  qemu-threadlets.h      |   48 ++++++++++++++
>>  4 files changed, 360 insertions(+), 1 deletions(-)
>>  create mode 100644 docs/async-support.txt
>>  create mode 100644 qemu-threadlets.c
>>  create mode 100644 qemu-threadlets.h
>>
>> diff --git a/Makefile.objs b/Makefile.objs
>> index cd5a24b..2cf8aba 100644
>> --- a/Makefile.objs
>> +++ b/Makefile.objs
>> @@ -9,6 +9,8 @@ qobject-obj-y += qerror.o
>>
>>  block-obj-y = cutils.o cache-utils.o qemu-malloc.o qemu-option.o module.o
>>  block-obj-y += nbd.o block.o aio.o aes.o osdep.o qemu-config.o
>> +block-obj-$(CONFIG_POSIX) += qemu-thread.o
>> +block-obj-$(CONFIG_POSIX) += qemu-threadlets.o
>>  block-obj-$(CONFIG_POSIX) += posix-aio-compat.o
>>  block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
>>
>> @@ -124,7 +126,6 @@ endif
>>  common-obj-y += $(addprefix ui/, $(ui-obj-y))
>>
>>  common-obj-y += iov.o acl.o
>> -common-obj-$(CONFIG_THREAD) += qemu-thread.o
>>  common-obj-y += notify.o event_notifier.o
>>  common-obj-y += qemu-timer.o
>>
>> diff --git a/docs/async-support.txt b/docs/async-support.txt
>> new file mode 100644
>> index 0000000..2e8adc9
>> --- /dev/null
>> +++ b/docs/async-support.txt
> 
> Why not call it docs/threadlets.txt?
> 
>> @@ -0,0 +1,141 @@
>> +== How to use the various asynchronous models supported in Qemu ==
> 
> This only describes threadlets.
> 
>> +
>> +== Threadlets ==
>> +
>> +Q.1: What are threadlets ?
>> +A.1: Threadlets is an infrastructure within QEMU that allows other subsystems
>> +     to offload possibly blocking work to a queue to be processed by a pool
>> +     of threads asynchrnonously.
> 
> asynchronously typo
> 
>> +
>> +Q.2: When would one want to use threadlets ?
>> +A.2: Threadlets are useful when there are operations that can be performed
>> +     outside the context of the VCPU/IO threads inorder to free these latter
>> +     to service any other guest requests.
>> +
>> +Q.3: I have some work that can be executed in an asynchronous context. How
>> +     should I go about it ?
>> +A.3: One could follow the steps listed below:
>> +
>> +     - Define a function which would do the asynchrnous work.
> 
> asynchronous typo
> 
>> +       void my_threadlet_func(ThreadletWork *work)
> 
> Usually these functions will be static.
> 
>> +       {
>> +       }
>> +
>> +     - Declare an object of type ThreadletWork;
>> +       ThreadletWork work;
>> +
>> +
>> +     - Assign a value to the "func" member of ThreadletWork object.
>> +       work.func = my_threadlet_func;
>> +
>> +     - Submit the threadlet to the global queue.
>> +       submit_threadletwork(&work);
>> +
>> +     - Continue servicing some other guest operations.
>> +
>> +Q.4: I want to my_threadlet_func to access some non-global data. How do I do
>> +     that ?
>> +A.4: Suppose you want my_threadlet_func to access some non-global data-object
>> +     of type myPrivateData. In that case one could follow the following steps.
>> +
>> +     - Define a member of the type ThreadletWork within myPrivateData.
>> +       typdef myPrivateData {
> 
> typedef typo, struct missing
> 
> Also, the QEMU coding style usually requires CapsNames for struct types...
> 
>> +               ...;
>> +               ...;
>> +               ...;
>> +               ThreadletWork work;
>> +       } myPrivateData;
>> +
>> +       myPrivateData myData;
> 
> ...and myData would be my_data.
> 
>> +
>> +     - Initialize myData.work as described in A.3
>> +       myData.work.func = my_threadlet_func;
>> +       submit_threadletwork(&myData.work);
>> +
>> +     - Access the myData object inside my_threadlet_func() using container_of
>> +       primitive
>> +       void my_threadlet_func(ThreadletWork *work)
>> +       {
>> +               myPrivateData *mydata_ptr;
>> +               mydata_ptr = container_of(work, myPrivateData, work);
>> +
>> +               /* mydata_ptr now points to myData object */
>> +       }
>> +
>> +Q.5: Are there any precautions one must take while sharing data with the
>> +     Asynchrnous thread-pool ?
> 
> asynchronous typo
> 
>> +A.5: Yes, make sure that the helper function of the type my_threadlet_func()
>> +     does not access/modify data when it can be accessed or modified in the
>> +     context of VCPU thread or IO thread. This is because the asynchronous
>> +     threads in the pool can run in parallel with the VCPU/IOThreads as shown
>> +     in the figure.
>> +
>> +     A typical workflow is as follows:
>> +
>> +              VCPU/IOThread
>> +                   |
>> +                   | (1)
>> +                   |
>> +                   V
>> +                Offload work              (2)
>> +      |-------> to threadlets -----------------------------> Helper thread
>> +      |            |                                               |
>> +      |            |                                               |
>> +      |            | (3)                                           | (4)
>> +      |            |                                               |
>> +      |         Handle other Guest requests                        |
>> +      |            |                                               |
>> +      |            |                                               V
>> +      |            | (3)                                  Signal the I/O Thread
>> +      |(6)         |                                               |
>> +      |            |                                              /
>> +      |            |                                             /
>> +      |            V                                            /
>> +      |          Do the post <---------------------------------/
>> +      |          processing               (5)
>> +      |            |
>> +      |            | (6)
>> +      |            V
>> +      |-Yes------ More async work?
>> +                   |
>> +                   | (7)
>> +                  No
>> +                   |
>> +                   |
>> +                   .
>> +                   .
>> +
>> +    Hence one needs to make sure that in the steps (3) and (4) which run in
>> +    parallel, any global data is accessed within only one context.
>> +
>> +Q.6: I have queued a threadlet which I want to cancel. How do I do that ?
>> +A.6: Threadlets framework provides the API cancel_threadlet:
>> +       - int cancel_threadletwork(ThreadletWork *work)
>> +
>> +     The API scans the ThreadletQueue to see if (work) is present. If it finds
>> +     work, it'll dequeue work and return 0.
>> +
>> +     On the other hand, if it does not find the (work) in the ThreadletQueue,
>> +     then it'll return 1. This can imply two things. Either the work is being
>> +     processed by one of the helper threads or it has been processed. The
>> +     threadlet infrastructure currently _does_not_ distinguish between these
>> +     two and the onus is on the caller to do that.
>> +
>> +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..1442122
>> --- /dev/null
>> +++ b/qemu-threadlets.c
>> @@ -0,0 +1,169 @@
>> +/*
>> + * Threadlet support for offloading tasks to be executed asynchronously
>> + *
>> + * Copyright IBM, Corp. 2008
>> + * Copyright IBM, Corp. 2010
>> + *
>> + * Authors:
>> + *  Anthony Liguori     <aliguori@us.ibm.com>
>> + *  Aneesh Kumar K.V    <aneesh.kumar@linux.vnet.ibm.com>
>> + *  Gautham R Shenoy    <ego@in.ibm.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
>> +ThreadletQueue globalqueue;
> 
> This should be static.
> 
>> +static int globalqueue_init;
>> +
>> +static void *threadlet_worker(void *data)
>> +{
>> +    ThreadletQueue *queue = data;
>> +
>> +    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);
>> +        }
>> +
>> +        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;
>> +            }
>> +        } 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;
>> +}
>> +
>> +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);
>> +    }
>> +    QTAILQ_INSERT_TAIL(&(queue->request_list), work, node);
>> +    qemu_mutex_unlock(&(queue->lock));
>> +    qemu_cond_signal(&(queue->cond));
> 
> Worker thread signalling and spawning has race conditions.  See my
> previous email:
> 
> "There are race conditions here:
> 
> 1. When a new threadlet is started because there are no idle threads,
> qemu_cond_signal() may fire a blank because the threadlet isn't inside
> qemu_cond_timedwait() yet.  The result, the work item is deadlocked
> until another thread grabs more work off the queue.  If I'm reading
> the code correctly this bug is currently present!

Moving QTAILQ_INSERT_TAIL() ahead of spawn_threadlet() should take care of this
issue
right?

If not the new work should be blocked until the new thread wait is timed out
or another threadlet done with its previous job..which ever occurs first.
As the worker_thread go and recheck the queue at the end of each timeout.


> 2. Moving qemu_cond_signal() outside queue->lock is dangerous for the
> same reason: you need to be careful not to qemu_cond_signal() when the
> thread isn't inside qemu_cond_timedwait()."

Again here the worst case scenario is timedwait duration if the work is not flowing
continuously. If we have to employ another synchronization technique, with
another set of locks etc..we need to be sure that the additional cost is justified.

Thanks,
JV

> 
>> +}
>> +
>> +/**
>> + * 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;
>> +    }
>> +
>> +    submit_threadletwork_to_queue(&globalqueue, work);
>> +}
>> +
>> +/**
>> + * cancel_threadletwork_on_queue: Cancel a task queued on a Queue.
>> + * @queue: The queue containing the task to be cancelled.
>> + * @work: Contains the information of the task that needs to be cancelled.
>> + *
>> + * Returns: 0 if the task is successfully cancelled.
>> + *          1 otherwise.
>> + */
>> +int cancel_threadletwork_on_queue(ThreadletQueue *queue, ThreadletWork *work)
>> +{
>> +    ThreadletWork *ret_work;
>> +    int found = 0;
>> +
>> +    qemu_mutex_lock(&(queue->lock));
>> +    QTAILQ_FOREACH(ret_work, &(queue->request_list), node) {
>> +        if (ret_work == work) {
>> +            QTAILQ_REMOVE(&(queue->request_list), ret_work, node);
>> +            found = 1;
>> +            break;
>> +        }
>> +    }
>> +    qemu_mutex_unlock(&(queue->lock));
>> +
>> +    if (found) {
>> +        return 0;
>> +    }
>> +
>> +    return 1;
> 
> Perhaps invert the logic of "found" and just call it "ret", then you
> don't need this if statement that flips its value.
> 
>> +}
>> +
>> +/**
>> + * cancel_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 cancel_threadletwork(ThreadletWork *work)
>> +{
>> +    return cancel_threadletwork_on_queue(&globalqueue, work);
>> +}
>> +
>> +/**
>> + * 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));
>> +}
>> diff --git a/qemu-threadlets.h b/qemu-threadlets.h
>> new file mode 100644
>> index 0000000..3df9b10
>> --- /dev/null
>> +++ b/qemu-threadlets.h
>> @@ -0,0 +1,48 @@
>> +/*
>> + * Threadlet support for offloading tasks to be executed asynchronously
>> + *
>> + * Copyright IBM, Corp. 2008
>> + * Copyright IBM, Corp. 2010
>> + *
>> + * Authors:
>> + *  Anthony Liguori     <aliguori@us.ibm.com>
>> + *  Aneesh Kumar K.V    <aneesh.kumar@linux.vnet.ibm.com>
>> + *  Gautham R Shenoy    <ego@in.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2.  See
>> + * the COPYING file in the top-level directory.
>> + */
>> +
>> +#ifndef QEMU_ASYNC_WORK_H
>> +#define QEMU_ASYNC_WORK_H
> 
> QEMU_THREADLETS_H?
> 
>> +
>> +#include "qemu-queue.h"
>> +#include "qemu-common.h"
>> +#include "qemu-thread.h"
>> +
>> +typedef struct ThreadletQueue
>> +{
>> +    QemuMutex lock;
>> +    QemuCond cond;
>> +    int max_threads;
>> +    int min_threads;
>> +    int cur_threads;
>> +    int idle_threads;
>> +    QTAILQ_HEAD(, threadlet_work) request_list;
>> +} ThreadletQueue;
>> +
>> +typedef struct threadlet_work
> 
> struct ThreadletWork follows coding style more closely.
> 
> Stefan
>

On 10/14/2010 9:16 AM, Avi Kivity wrote:
>  On 10/14/2010 11:15 AM, Stefan Hajnoczi wrote:
>> I forgot to add that the semantics of cancellation make it difficult
>> to write correct user code.  Every cancellation user needs to add
>> extra synchronization after the cancel call to handle the case where
>> the work is currently executing.
>>
>> This seems tricky to me and I suspect code using this interface will
>> be buggy.  How about the following?
>> 1. Add a return value indicating that the work is currently executing
>> (this still requires the caller to add extra synchronization but is at
>> least explicit) versus work is no longer on the list.

Current semantics is .. if not on the list it is currently executing or we are
done with it.
How do we differentiate between those two? Do we need any other state?
>> 2. Add a flag to block until the work has been cancelled or completed.
>>   This is useful to callers who are allowed to block.
> 
> Blocking is somewhat against the spirit of the thing, no?  While I agree that
> the current cancel API is hard to use correctly, blocking defeats the purpose of
> the API.
> 
Are you proposing to add additional state in the return
(canceled/running/not-canceled)
and leave the synchronization part to the user?
i.e not to provide any additional interface for the user to wait
for the scheduled work to finish? Just trying to understand.

Thanks,
JV

On Thu, Oct 14, 2010 at 5:16 PM, Avi Kivity <avi@redhat.com> wrote:
>  On 10/14/2010 11:15 AM, Stefan Hajnoczi wrote:
>>
>> I forgot to add that the semantics of cancellation make it difficult
>> to write correct user code.  Every cancellation user needs to add
>> extra synchronization after the cancel call to handle the case where
>> the work is currently executing.
>>
>> This seems tricky to me and I suspect code using this interface will
>> be buggy.  How about the following?
>> 1. Add a return value indicating that the work is currently executing
>> (this still requires the caller to add extra synchronization but is at
>> least explicit) versus work is no longer on the list.
>> 2. Add a flag to block until the work has been cancelled or completed.
>>  This is useful to callers who are allowed to block.
>
> Blocking is somewhat against the spirit of the thing, no?  While I agree
> that the current cancel API is hard to use correctly, blocking defeats the
> purpose of the API.

Yes, that's why it would have to be an optional flag or a separate
function.  The idea is that callers who are able to block on
cancellation can use common code instead of implementing it from
scratch each time.

Stefan

On Thu, Oct 14, 2010 at 10:17 PM, Venkateswararao Jujjuri (JV)
<jvrao@linux.vnet.ibm.com> wrote:
> On 10/14/2010 2:02 AM, Stefan Hajnoczi wrote:
>> On Wed, Oct 13, 2010 at 4:31 PM, Arun R Bharadwaj
>>> +static void *threadlet_worker(void *data)
>>> +{
>>> +    ThreadletQueue *queue = data;
>>> +
>>> +    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);
>>> +        }
>>> +
>>> +        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;
>>> +            }
>>> +        } 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;
>>> +}
>>> +
>>> +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);
>>> +    }
>>> +    QTAILQ_INSERT_TAIL(&(queue->request_list), work, node);
>>> +    qemu_mutex_unlock(&(queue->lock));
>>> +    qemu_cond_signal(&(queue->cond));
>>
>> Worker thread signalling and spawning has race conditions.  See my
>> previous email:
>>
>> "There are race conditions here:
>>
>> 1. When a new threadlet is started because there are no idle threads,
>> qemu_cond_signal() may fire a blank because the threadlet isn't inside
>> qemu_cond_timedwait() yet.  The result, the work item is deadlocked
>> until another thread grabs more work off the queue.  If I'm reading
>> the code correctly this bug is currently present!
>
> Moving QTAILQ_INSERT_TAIL() ahead of spawn_threadlet() should take care of this
> issue
> right?

I didn't read the code correctly.  queue->lock is already held by
submit_threadletwork_to_queue() until after QTAILQ_INSERT_TAIL().
threadlet_worker() will only enter its main loop once it acquires
queue->lock.  Therefore the queue definitely has the work before the
spawned thread begins processing.

The work is on the queue when threadlet_worker() enters its main loop,
so it will not need to wait on queue->cond but can process work
immediately.  There is no spawn race condition.

Stefan

On 10/14/2010 11:02 AM, Stefan Hajnoczi wrote:
> 2. Moving qemu_cond_signal() outside queue->lock is dangerous for the
> same reason: you need to be careful not to qemu_cond_signal() when the
> thread isn't inside qemu_cond_timedwait()."

Yes, please do so.

I personally consider it bad programming practice to put the condvar 
signal/broadcast outside the mutex.  While I understand that my ideas 
may not matter much, some Mr. Hoare actually invented them without 
signal-outside-lock, and he probably knows better than anyone reading.

Paolo

On 10/15/2010 2:52 AM, Stefan Hajnoczi wrote:
> On Thu, Oct 14, 2010 at 10:17 PM, Venkateswararao Jujjuri (JV)
> <jvrao@linux.vnet.ibm.com> wrote:
>> On 10/14/2010 2:02 AM, Stefan Hajnoczi wrote:
>>> On Wed, Oct 13, 2010 at 4:31 PM, Arun R Bharadwaj
>>>> +static void *threadlet_worker(void *data)
>>>> +{
>>>> +    ThreadletQueue *queue = data;
>>>> +
>>>> +    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);
>>>> +        }
>>>> +
>>>> +        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;
>>>> +            }
>>>> +        } 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;
>>>> +}
>>>> +
>>>> +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);
>>>> +    }
>>>> +    QTAILQ_INSERT_TAIL(&(queue->request_list), work, node);
>>>> +    qemu_mutex_unlock(&(queue->lock));
>>>> +    qemu_cond_signal(&(queue->cond));
>>>
>>> Worker thread signalling and spawning has race conditions.  See my
>>> previous email:
>>>
>>> "There are race conditions here:
>>>
>>> 1. When a new threadlet is started because there are no idle threads,
>>> qemu_cond_signal() may fire a blank because the threadlet isn't inside
>>> qemu_cond_timedwait() yet.  The result, the work item is deadlocked
>>> until another thread grabs more work off the queue.  If I'm reading
>>> the code correctly this bug is currently present!
>>
>> Moving QTAILQ_INSERT_TAIL() ahead of spawn_threadlet() should take care of this
>> issue
>> right?
> 
> I didn't read the code correctly.  queue->lock is already held by
> submit_threadletwork_to_queue() until after QTAILQ_INSERT_TAIL().
> threadlet_worker() will only enter its main loop once it acquires
> queue->lock.  Therefore the queue definitely has the work before the
> spawned thread begins processing.
> 
> The work is on the queue when threadlet_worker() enters its main loop,
> so it will not need to wait on queue->cond but can process work
> immediately.  There is no spawn race condition.

Correct...I too missed that. :)

JV

> 
> Stefan

On 10/14/2010 11:32 PM, Venkateswararao Jujjuri (JV) wrote:
> >
> >  Blocking is somewhat against the spirit of the thing, no?  While I agree that
> >  the current cancel API is hard to use correctly, blocking defeats the purpose of
> >  the API.
> >
> Are you proposing to add additional state in the return
> (canceled/running/not-canceled)
> and leave the synchronization part to the user?
> i.e not to provide any additional interface for the user to wait
> for the scheduled work to finish? Just trying to understand.

I wasn't proposing anything since I don't have a good proposal.  Adding 
a callback makes the whole thing an asynchronous design which threads 
are trying to avoid.  Blocking is bad.  Leaving it to the caller is hard 
to use correctly.

Perhaps we can have a threadlet with barrier semantics.  You queue a 
piece of work which is guaranteed to execute after all previously 
submitted work (against the same queue) and before any consequently 
submitted work.

* Avi Kivity <avi@redhat.com> [2010-10-17 10:57:23]:

>  On 10/14/2010 11:32 PM, Venkateswararao Jujjuri (JV) wrote:
> >>
> >>  Blocking is somewhat against the spirit of the thing, no?  While I agree that
> >>  the current cancel API is hard to use correctly, blocking defeats the purpose of
> >>  the API.
> >>
> >Are you proposing to add additional state in the return
> >(canceled/running/not-canceled)
> >and leave the synchronization part to the user?
> >i.e not to provide any additional interface for the user to wait
> >for the scheduled work to finish? Just trying to understand.
> 
> I wasn't proposing anything since I don't have a good proposal.
> Adding a callback makes the whole thing an asynchronous design which
> threads are trying to avoid.  Blocking is bad.  Leaving it to the
> caller is hard to use correctly.
> 
> Perhaps we can have a threadlet with barrier semantics.  You queue a
> piece of work which is guaranteed to execute after all previously
> submitted work (against the same queue) and before any consequently
> submitted work.
> 
> -- 
> error compiling committee.c: too many arguments to function
> 
> 

I would suggest that we have 2 APIs - cancel_threadletwork (current
cancel implementation) and cancel_threadletwork_sync (waits for work
to complete). As of now there is no known user for
cancel_threadletwork_sync. So we can keep this as a TODO for later. I
can provide the APIs for both these so that when we have a user for
cancel_threadletwork_sync, we can go ahead and implement it.

-arun

On 10/18/2010 12:47 PM, Arun R Bharadwaj wrote:
> * Avi Kivity<avi@redhat.com>  [2010-10-17 10:57:23]:
>
> >   On 10/14/2010 11:32 PM, Venkateswararao Jujjuri (JV) wrote:
> >  >>
> >  >>   Blocking is somewhat against the spirit of the thing, no?  While I agree that
> >  >>   the current cancel API is hard to use correctly, blocking defeats the purpose of
> >  >>   the API.
> >  >>
> >  >Are you proposing to add additional state in the return
> >  >(canceled/running/not-canceled)
> >  >and leave the synchronization part to the user?
> >  >i.e not to provide any additional interface for the user to wait
> >  >for the scheduled work to finish? Just trying to understand.
> >
> >  I wasn't proposing anything since I don't have a good proposal.
> >  Adding a callback makes the whole thing an asynchronous design which
> >  threads are trying to avoid.  Blocking is bad.  Leaving it to the
> >  caller is hard to use correctly.
> >
> >  Perhaps we can have a threadlet with barrier semantics.  You queue a
> >  piece of work which is guaranteed to execute after all previously
> >  submitted work (against the same queue) and before any consequently
> >  submitted work.
> >
> >  -- 
> >  error compiling committee.c: too many arguments to function
> >
> >
>
> I would suggest that we have 2 APIs - cancel_threadletwork (current
> cancel implementation) and cancel_threadletwork_sync (waits for work
> to complete). As of now there is no known user for
> cancel_threadletwork_sync. So we can keep this as a TODO for later. I
> can provide the APIs for both these so that when we have a user for
> cancel_threadletwork_sync, we can go ahead and implement it.

I agree it's best not to implement c_t_s() now.  Using it implies a 
stall so we should discourage it.