Re: [Qemu-devel] [PATCH for-2.5] virtio-9p: use QEMU thread pool

[Greg Kurz <gkurz@linux.vnet.ibm.com>]

On Fri, 27 Nov 2015 12:43:06 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> The QEMU thread pool already has a mechanism to invoke callbacks in the main
> thread.  It does not need an EventNotifier and it is more efficient too.
> Use it instead of GAsyncQueue + GThreadPool + glue.
> 
> As a side effect, it silences Coverity's complaint about an unchecked
> return value for event_notifier_init.
> 

And it makes the code a lot nicer too !

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---

I have just one minor remark in hw/9pfs/virtio-9p-coth.h but anyway:

Reviewed-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
Tested-by: Greg Kurz <gkurz@linux.vnet.ibm.com>

>  hw/9pfs/virtio-9p-coth.c   | 79 +++++++++++-----------------------------------
>  hw/9pfs/virtio-9p-coth.h   |  9 +-----
>  hw/9pfs/virtio-9p-device.c |  4 ---
>  3 files changed, 20 insertions(+), 72 deletions(-)
> 
> diff --git a/hw/9pfs/virtio-9p-coth.c b/hw/9pfs/virtio-9p-coth.c
> index 5057f8d..fb6e8f8 100644
> --- a/hw/9pfs/virtio-9p-coth.c
> +++ b/hw/9pfs/virtio-9p-coth.c
> @@ -12,71 +12,30 @@
>   *
>   */
> 
> -#include "fsdev/qemu-fsdev.h"
> -#include "qemu/thread.h"
> -#include "qemu/event_notifier.h"
> +#include "qemu-common.h"
> +#include "block/thread-pool.h"
>  #include "qemu/coroutine.h"
> +#include "qemu/main-loop.h"
>  #include "virtio-9p-coth.h"
> 
> -/* v9fs glib thread pool */
> -static V9fsThPool v9fs_pool;
> +/* Called from QEMU I/O thread.  */
> +static void coroutine_enter_cb(void *opaque, int ret)
> +{
> +    Coroutine *co = opaque;
> +    qemu_coroutine_enter(co, NULL);
> +}
> +
> +/* Called from worker thread.  */
> +static int coroutine_enter_func(void *arg)
> +{
> +    Coroutine *co = arg;
> +    qemu_coroutine_enter(co, NULL);
> +    return 0;
> +}
> 
>  void co_run_in_worker_bh(void *opaque)
>  {
>      Coroutine *co = opaque;
> -    g_thread_pool_push(v9fs_pool.pool, co, NULL);
> -}
> -
> -static void v9fs_qemu_process_req_done(EventNotifier *e)
> -{
> -    Coroutine *co;
> -
> -    event_notifier_test_and_clear(e);
> -
> -    while ((co = g_async_queue_try_pop(v9fs_pool.completed)) != NULL) {
> -        qemu_coroutine_enter(co, NULL);
> -    }
> -}
> -
> -static void v9fs_thread_routine(gpointer data, gpointer user_data)
> -{
> -    Coroutine *co = data;
> -
> -    qemu_coroutine_enter(co, NULL);
> -
> -    g_async_queue_push(v9fs_pool.completed, co);
> -
> -    event_notifier_set(&v9fs_pool.e);
> -}
> -
> -int v9fs_init_worker_threads(void)
> -{
> -    int ret = 0;
> -    V9fsThPool *p = &v9fs_pool;
> -    sigset_t set, oldset;
> -
> -    sigfillset(&set);
> -    /* Leave signal handling to the iothread.  */
> -    pthread_sigmask(SIG_SETMASK, &set, &oldset);
> -
> -    p->pool = g_thread_pool_new(v9fs_thread_routine, p, -1, FALSE, NULL);
> -    if (!p->pool) {
> -        ret = -1;
> -        goto err_out;
> -    }
> -    p->completed = g_async_queue_new();
> -    if (!p->completed) {
> -        /*
> -         * We are going to terminate.
> -         * So don't worry about cleanup
> -         */
> -        ret = -1;
> -        goto err_out;
> -    }
> -    event_notifier_init(&p->e, 0);
> -
> -    event_notifier_set_handler(&p->e, v9fs_qemu_process_req_done);
> -err_out:
> -    pthread_sigmask(SIG_SETMASK, &oldset, NULL);
> -    return ret;
> +    thread_pool_submit_aio(qemu_get_aio_context()->thread_pool,
> +                           coroutine_enter_func, co, coroutine_enter_cb, co);
>  }
> diff --git a/hw/9pfs/virtio-9p-coth.h b/hw/9pfs/virtio-9p-coth.h
> index 0fbe49a..535743f 100644
> --- a/hw/9pfs/virtio-9p-coth.h
> +++ b/hw/9pfs/virtio-9p-coth.h
> @@ -20,13 +20,6 @@
>  #include "virtio-9p.h"
>  #include <glib.h>
> 

glib.h is no more needed.

> -typedef struct V9fsThPool {
> -    EventNotifier e;
> -
> -    GThreadPool *pool;
> -    GAsyncQueue *completed;
> -} V9fsThPool;
> -
>  /*
>   * we want to use bottom half because we want to make sure the below
>   * sequence of events.
> @@ -45,7 +38,7 @@ typedef struct V9fsThPool {
>          qemu_bh_schedule(co_bh);                                        \
>          /*                                                              \
>           * yield in qemu thread and re-enter back                       \
> -         * in glib worker thread                                        \
> +         * in worker thread                                             \
>           */                                                             \
>          qemu_coroutine_yield();                                         \
>          qemu_bh_delete(co_bh);                                          \
> diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
> index e3abcfa..944b5f5 100644
> --- a/hw/9pfs/virtio-9p-device.c
> +++ b/hw/9pfs/virtio-9p-device.c
> @@ -116,10 +116,6 @@ static void virtio_9p_device_realize(DeviceState *dev, Error **errp)
>                     " and export path:%s", s->fsconf.fsdev_id, s->ctx.fs_root);
>          goto out;
>      }
> -    if (v9fs_init_worker_threads() < 0) {
> -        error_setg(errp, "worker thread initialization failed");
> -        goto out;
> -    }
> 
>      /*
>       * Check details of export path, We need to use fs driver