Patchwork [19/25] aio: add generic thread-pool facility

login
register
mail settings
Submitter Paolo Bonzini
Date Oct. 26, 2012, 2:05 p.m.
Message ID <1351260355-19802-20-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/194515/
State New
Headers show

Comments

Paolo Bonzini - Oct. 26, 2012, 2:05 p.m.
Add a generic thread-pool.  The code is roughly based on posix-aio-compat.c,
with some changes, especially the following:

- use QemuSemaphore instead of QemuCond;

- separate the state of the thread from the return code of the worker
function.  The return code is totally opaque for the thread pool;

- do not busy wait when doing cancellation.

A more generic threadpool (but still specific to I/O so that in the future
it can use special scheduling classes or PI mutexes) can have many uses:
it allows more flexibility in raw-posix.c and can more easily be extended
to Win32, and it will also be used to do an msync of the persistent bitmap.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 Makefile.objs |   2 +-
 thread-pool.c | 279 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 thread-pool.h |  34 +++++++
 trace-events  |   5 ++
 4 file modificati, 319 inserzioni(+). 1 rimozione(-)
 create mode 100644 thread-pool.c
 create mode 100644 thread-pool.h
Stefan Hajnoczi - Oct. 30, 2012, 7:13 p.m.
On Fri, Oct 26, 2012 at 04:05:49PM +0200, Paolo Bonzini wrote:
> +static void event_notifier_ready(EventNotifier *notifier)
> +{
> +    ThreadPoolElement *elem, *next;
> +
> +    event_notifier_test_and_clear(notifier);
> +restart:
> +    QLIST_FOREACH_SAFE(elem, &head, all, next) {
> +        if (elem->state != THREAD_CANCELED && elem->state != THREAD_DONE) {
> +            continue;
> +        }
> +        if (elem->state == THREAD_DONE) {
> +            trace_thread_pool_complete(elem, elem->common.opaque, elem->ret);
> +        }
> +        if (elem->state == THREAD_DONE && elem->common.cb) {
> +            QLIST_REMOVE(elem, all);
> +            elem->common.cb(elem->common.opaque, elem->ret);

This function didn't take the lock.  First it accessed elem->state and
how it reads elem->ret.  We need to take the lock to ensure both
elem->state and elem->ret have been set - otherwise we could read
elem->ret before the return value was stored.

> +typedef struct ThreadPoolCo {
> +    Coroutine *co;
> +    int ret;
> +} ThreadPoolCo;
> +
> +static void thread_pool_co_cb(void *opaque, int ret)
> +{
> +    ThreadPoolCo *co = opaque;
> +
> +    co->ret = ret;
> +    qemu_coroutine_enter(co->co, NULL);
> +}
> +
> +int coroutine_fn thread_pool_submit_co(ThreadPoolFunc *func, void *arg)
> +{
> +    ThreadPoolCo tpc = { .co = qemu_coroutine_self(), .ret = -EINPROGRESS };
> +    assert(qemu_in_coroutine());
> +    thread_pool_submit_aio(func, arg, thread_pool_co_cb, &tpc);
> +    qemu_coroutine_yield();
> +    return tpc.ret;

It's important to understand that the submit_aio, yield, return ret
pattern works because we assume this function was called as part of the
main loop.

If thread_pool_submit_co() was called outside the event loop and global
mutex, then there is a race between the submit_aio and yield steps where
thread_pool_co_cb() is called before this coroutine yields!

I see no reason why this is a problem today but I had to think through
this case when reading the code.

Stefan

Patch

diff --git a/Makefile.objs b/Makefile.objs
index da744e1..4a6d47e 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -43,7 +43,7 @@  coroutine-obj-$(CONFIG_WIN32) += coroutine-win32.o
 
 block-obj-y = cutils.o iov.o cache-utils.o qemu-option.o module.o async.o
 block-obj-y += nbd.o block.o blockjob.o aes.o qemu-config.o
-block-obj-y += qemu-progress.o qemu-sockets.o uri.o
+block-obj-y += thread-pool.o qemu-progress.o qemu-sockets.o uri.o
 block-obj-y += $(coroutine-obj-y) $(qobject-obj-y) $(version-obj-y)
 block-obj-$(CONFIG_POSIX) += posix-aio-compat.o
 block-obj-$(CONFIG_POSIX) += event_notifier-posix.o aio-posix.o
diff --git a/thread-pool.c b/thread-pool.c
new file mode 100644
index 0000000..266f12f
--- /dev/null
+++ b/thread-pool.c
@@ -0,0 +1,279 @@ 
+/*
+ * QEMU block layer thread pool
+ *
+ * Copyright IBM, Corp. 2008
+ * Copyright Red Hat, Inc. 2012
+ *
+ * Authors:
+ *  Anthony Liguori   <aliguori@us.ibm.com>
+ *  Paolo Bonzini     <pbonzini@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ * Contributions after 2012-01-13 are licensed under the terms of the
+ * GNU GPL, version 2 or (at your option) any later version.
+ */
+#include "qemu-common.h"
+#include "qemu-queue.h"
+#include "qemu-thread.h"
+#include "osdep.h"
+#include "qemu-coroutine.h"
+#include "trace.h"
+#include "block_int.h"
+#include "event_notifier.h"
+#include "thread-pool.h"
+
+static void do_spawn_thread(void);
+
+typedef struct ThreadPoolElement ThreadPoolElement;
+
+enum ThreadState {
+    THREAD_QUEUED,
+    THREAD_ACTIVE,
+    THREAD_DONE,
+    THREAD_CANCELED,
+};
+
+struct ThreadPoolElement {
+    BlockDriverAIOCB common;
+    ThreadPoolFunc *func;
+    void *arg;
+    enum ThreadState state;
+    int ret;
+
+    QTAILQ_ENTRY(ThreadPoolElement) reqs;
+    QLIST_ENTRY(ThreadPoolElement) all;
+};
+
+static EventNotifier notifier;
+static QemuMutex lock;
+static QemuCond check_cancel;
+static QemuSemaphore sem;
+static int max_threads = 64;
+static int cur_threads;
+static int idle_threads;
+static int new_threads;     /* backlog of threads we need to create */
+static int pending_threads; /* threads created but not running yet */
+static int pending_cancellations; /* whether we need a cond_broadcast */
+static QEMUBH *new_thread_bh;
+static QLIST_HEAD(, ThreadPoolElement) head;
+static QTAILQ_HEAD(, ThreadPoolElement) request_list;
+
+static void *worker_thread(void *unused)
+{
+    qemu_mutex_lock(&lock);
+    pending_threads--;
+    qemu_mutex_unlock(&lock);
+    do_spawn_thread();
+
+    while (1) {
+        ThreadPoolElement *req;
+        int ret;
+
+        qemu_mutex_lock(&lock);
+        idle_threads++;
+        qemu_mutex_unlock(&lock);
+        ret = qemu_sem_timedwait(&sem, 10000);
+        qemu_mutex_lock(&lock);
+        idle_threads--;
+        if (ret == -1) {
+            if (QTAILQ_EMPTY(&request_list)) {
+                break;
+            }
+            qemu_mutex_unlock(&lock);
+            continue;
+        }
+
+        req = QTAILQ_FIRST(&request_list);
+        QTAILQ_REMOVE(&request_list, req, reqs);
+        req->state = THREAD_ACTIVE;
+        qemu_mutex_unlock(&lock);
+
+        ret = req->func(req->arg);
+
+        qemu_mutex_lock(&lock);
+        req->state = THREAD_DONE;
+        req->ret = ret;
+        if (pending_cancellations) {
+            qemu_cond_broadcast(&check_cancel);
+        }
+        qemu_mutex_unlock(&lock);
+
+        event_notifier_set(&notifier);
+    }
+
+    cur_threads--;
+    qemu_mutex_unlock(&lock);
+
+    return NULL;
+}
+
+static void do_spawn_thread(void)
+{
+    QemuThread t;
+
+    qemu_mutex_lock(&lock);
+    if (!new_threads) {
+        qemu_mutex_unlock(&lock);
+        return;
+    }
+
+    new_threads--;
+    pending_threads++;
+
+    qemu_mutex_unlock(&lock);
+
+    qemu_thread_create(&t, worker_thread, NULL, QEMU_THREAD_DETACHED);
+}
+
+static void spawn_thread_bh_fn(void *opaque)
+{
+    do_spawn_thread();
+}
+
+static void spawn_thread(void)
+{
+    cur_threads++;
+    new_threads++;
+    /* If there are threads being created, they will spawn new workers, so
+     * we don't spend time creating many threads in a loop holding a mutex or
+     * starving the current vcpu.
+     *
+     * If there are no idle threads, ask the main thread to create one, so we
+     * inherit the correct affinity instead of the vcpu affinity.
+     */
+    if (!pending_threads) {
+        qemu_bh_schedule(new_thread_bh);
+    }
+}
+
+static void event_notifier_ready(EventNotifier *notifier)
+{
+    ThreadPoolElement *elem, *next;
+
+    event_notifier_test_and_clear(notifier);
+restart:
+    QLIST_FOREACH_SAFE(elem, &head, all, next) {
+        if (elem->state != THREAD_CANCELED && elem->state != THREAD_DONE) {
+            continue;
+        }
+        if (elem->state == THREAD_DONE) {
+            trace_thread_pool_complete(elem, elem->common.opaque, elem->ret);
+        }
+        if (elem->state == THREAD_DONE && elem->common.cb) {
+            QLIST_REMOVE(elem, all);
+            elem->common.cb(elem->common.opaque, elem->ret);
+            qemu_aio_release(elem);
+            goto restart;
+        } else {
+            /* remove the request */
+            QLIST_REMOVE(elem, all);
+            qemu_aio_release(elem);
+        }
+    }
+}
+
+static int thread_pool_active(EventNotifier *notifier)
+{
+    return !QLIST_EMPTY(&head);
+}
+
+static void thread_pool_cancel(BlockDriverAIOCB *acb)
+{
+    ThreadPoolElement *elem = (ThreadPoolElement *)acb;
+
+    trace_thread_pool_cancel(elem, elem->common.opaque);
+
+    qemu_mutex_lock(&lock);
+    if (elem->state == THREAD_QUEUED &&
+        /* No thread has yet started working on elem. we can try to "steal"
+         * the item from the worker if we can get a signal from the
+         * semaphore.  Because this is non-blocking, we can do it with
+         * the lock taken and ensure that elem will remain THREAD_QUEUED.
+         */
+        qemu_sem_timedwait(&sem, 0) == 0) {
+        QTAILQ_REMOVE(&request_list, elem, reqs);
+        elem->state = THREAD_CANCELED;
+        event_notifier_set(&notifier);
+    } else {
+        pending_cancellations++;
+        while (elem->state != THREAD_CANCELED && elem->state != THREAD_DONE) {
+            qemu_cond_wait(&check_cancel, &lock);
+        }
+        pending_cancellations--;
+    }
+    qemu_mutex_unlock(&lock);
+}
+
+static AIOPool thread_pool_cb_pool = {
+    .aiocb_size         = sizeof(ThreadPoolElement),
+    .cancel             = thread_pool_cancel,
+};
+
+BlockDriverAIOCB *thread_pool_submit_aio(ThreadPoolFunc *func, void *arg,
+        BlockDriverCompletionFunc *cb, void *opaque)
+{
+    ThreadPoolElement *req;
+
+    req = qemu_aio_get(&thread_pool_cb_pool, NULL, cb, opaque);
+    req->func = func;
+    req->arg = arg;
+    req->state = THREAD_QUEUED;
+
+    QLIST_INSERT_HEAD(&head, req, all);
+
+    trace_thread_pool_submit(req, arg);
+
+    qemu_mutex_lock(&lock);
+    if (idle_threads == 0 && cur_threads < max_threads) {
+        spawn_thread();
+    }
+    QTAILQ_INSERT_TAIL(&request_list, req, reqs);
+    qemu_mutex_unlock(&lock);
+    qemu_sem_post(&sem);
+    return &req->common;
+}
+
+typedef struct ThreadPoolCo {
+    Coroutine *co;
+    int ret;
+} ThreadPoolCo;
+
+static void thread_pool_co_cb(void *opaque, int ret)
+{
+    ThreadPoolCo *co = opaque;
+
+    co->ret = ret;
+    qemu_coroutine_enter(co->co, NULL);
+}
+
+int coroutine_fn thread_pool_submit_co(ThreadPoolFunc *func, void *arg)
+{
+    ThreadPoolCo tpc = { .co = qemu_coroutine_self(), .ret = -EINPROGRESS };
+    assert(qemu_in_coroutine());
+    thread_pool_submit_aio(func, arg, thread_pool_co_cb, &tpc);
+    qemu_coroutine_yield();
+    return tpc.ret;
+}
+
+void thread_pool_submit(ThreadPoolFunc *func, void *arg)
+{
+    thread_pool_submit_aio(func, arg, NULL, NULL);
+}
+
+static void thread_pool_init(void)
+{
+    QLIST_INIT(&head);
+    event_notifier_init(&notifier, false);
+    qemu_mutex_init(&lock);
+    qemu_cond_init(&check_cancel);
+    qemu_sem_init(&sem, 0);
+    qemu_aio_set_event_notifier(&notifier, event_notifier_ready,
+                                thread_pool_active);
+
+    QTAILQ_INIT(&request_list);
+    new_thread_bh = qemu_bh_new(spawn_thread_bh_fn, NULL);
+}
+
+block_init(thread_pool_init)
diff --git a/thread-pool.h b/thread-pool.h
new file mode 100644
index 0000000..378a4ac
--- /dev/null
+++ b/thread-pool.h
@@ -0,0 +1,34 @@ 
+/*
+ * QEMU block layer thread pool
+ *
+ * Copyright IBM, Corp. 2008
+ * Copyright Red Hat, Inc. 2012
+ *
+ * Authors:
+ *  Anthony Liguori   <aliguori@us.ibm.com>
+ *  Paolo Bonzini     <pbonzini@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ * Contributions after 2012-01-13 are licensed under the terms of the
+ * GNU GPL, version 2 or (at your option) any later version.
+ */
+
+#ifndef QEMU_THREAD_POOL_H
+#define QEMU_THREAD_POOL_H 1
+
+#include "qemu-common.h"
+#include "qemu-queue.h"
+#include "qemu-thread.h"
+#include "qemu-coroutine.h"
+#include "block_int.h"
+
+typedef int ThreadPoolFunc(void *opaque);
+
+BlockDriverAIOCB *thread_pool_submit_aio(ThreadPoolFunc *func, void *arg,
+     BlockDriverCompletionFunc *cb, void *opaque);
+int coroutine_fn thread_pool_submit_co(ThreadPoolFunc *func, void *arg);
+void thread_pool_submit(ThreadPoolFunc *func, void *arg);
+
+#endif
diff --git a/trace-events b/trace-events
index e2d4580..58c18eb 100644
--- a/trace-events
+++ b/trace-events
@@ -90,6 +90,11 @@  virtio_blk_rw_complete(void *req, int ret) "req %p ret %d"
 virtio_blk_handle_write(void *req, uint64_t sector, size_t nsectors) "req %p sector %"PRIu64" nsectors %zu"
 virtio_blk_handle_read(void *req, uint64_t sector, size_t nsectors) "req %p sector %"PRIu64" nsectors %zu"
 
+# thread-pool.c
+thread_pool_submit(void *req, void *opaque) "req %p opaque %p"
+thread_pool_complete(void *req, void *opaque, int ret) "req %p opaque %p ret %d"
+thread_pool_cancel(void *req, void *opaque) "req %p opaque %p"
+
 # posix-aio-compat.c
 paio_submit(void *acb, void *opaque, int64_t sector_num, int nb_sectors, int type) "acb %p opaque %p sector_num %"PRId64" nb_sectors %d type %d"
 paio_complete(void *acb, void *opaque, int ret) "acb %p opaque %p ret %d"