Re: [Qemu-devel] [PATCH v2 1/3] Make thread pool implementation modular

[Kevin Wolf]

Am 04.11.2013 um 11:28 hat Matthias Brugger geschrieben:
> This patch introduces function pointers for the thread pool, so that
> it's implementation can be set at run-time.
> 
> Signed-off-by: Matthias Brugger <address@hidden>

Like Stefan said, this can really only be given a meaningful review in
context with an actual user of the infrastructure, but I'll mention some
minor points in this series anyway.

>  include/block/thread-pool.h | 11 +++++++++++
>  thread-pool.c               | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+)
> 
> diff --git a/include/block/thread-pool.h b/include/block/thread-pool.h
> index 32afcdd..1f73712 100644
> --- a/include/block/thread-pool.h
> +++ b/include/block/thread-pool.h
> @@ -38,4 +38,15 @@ int coroutine_fn thread_pool_submit_co(ThreadPool *pool,
>          ThreadPoolFunc *func, void *arg);
>  void thread_pool_submit(ThreadPool *pool, ThreadPoolFunc *func, void *arg);
>  
> +ThreadPoolFuncArr *thread_pool_probe(void);
> +void thread_pool_delete(ThreadPoolFuncArr *tpf);
> +
> +struct ThreadPoolFuncArr {

If "Arr" is supposed to mean "array", this isn't really one.

Similar structs containing function pointers have names ending in
Info (AIOCBInfo, NetClientInfo) or Driver (BlockDriver).

> +    BlockDriverAIOCB *(*thread_pool_submit_aio)(ThreadPool *pool,
> +        ThreadPoolFunc *func, void *arg, BlockDriverCompletionFunc *cb,
> +        void *opaque);
> +    ThreadPool *(*thread_pool_new)(AioContext *ctx);
> +};
> +
> +
>  #endif
> diff --git a/thread-pool.c b/thread-pool.c
> index 3735fd3..53294a9 100644
> --- a/thread-pool.c
> +++ b/thread-pool.c
> @@ -26,6 +26,7 @@
>  #include "qemu/main-loop.h"
>  
>  static void do_spawn_thread(ThreadPool *pool);
> +static void thread_pool_aio_free(ThreadPool *pool);
>  
>  typedef struct ThreadPoolElement ThreadPoolElement;
>  
> @@ -77,6 +78,7 @@ struct ThreadPool {
>      int pending_threads; /* threads created but not running yet */
>      int pending_cancellations; /* whether we need a cond_broadcast */
>      bool stopping;
> +    void (*thread_pool_free)(ThreadPool *pool);
>  };
>  
>  static void *worker_thread(void *opaque)
> @@ -300,6 +302,7 @@ static void thread_pool_init_one(ThreadPool *pool, 
> AioContext *ctx)
>      qemu_sem_init(&pool->sem, 0);
>      pool->max_threads = 64;
>      pool->new_thread_bh = aio_bh_new(ctx, spawn_thread_bh_fn, pool);
> +    pool->thread_pool_free = &thread_pool_aio_free;
>  
>      QLIST_INIT(&pool->head);
>      QTAILQ_INIT(&pool->request_list);
> @@ -316,6 +319,11 @@ ThreadPool *thread_pool_new(AioContext *ctx)
>  
>  void thread_pool_free(ThreadPool *pool)
>  {
> +    pool->thread_pool_free(pool);
> +}
> +
> +void thread_pool_aio_free(ThreadPool *pool)
> +{
>      if (!pool) {
>          return;
>      }
> @@ -346,3 +354,28 @@ void thread_pool_free(ThreadPool *pool)
>      event_notifier_cleanup(&pool->notifier);
>      g_free(pool);
>  }
> +
> +ThreadPoolFuncArr *thread_pool_probe(void)

What is probed here? Isn't it just a function that creates a thread
pool?

> +{
> +    ThreadPoolFuncArr *tpf_pool = NULL;
> +
> +    if (tpf_pool) {
> +        return tpf_pool;
> +    }
> +
> +    tpf_pool = g_new(ThreadPoolFuncArr, 1);
> +    if (!tpf_pool) {
> +        printf("error allocating thread pool\n");
> +        return NULL;
> +    }

g_new() doesn't fail.

> +
> +    tpf_pool->thread_pool_submit_aio = thread_pool_submit_aio;
> +    tpf_pool->thread_pool_new = thread_pool_new;
> +
> +    return tpf_pool;
> +}
> +
> +void thread_pool_delete(ThreadPoolFuncArr *tpf)
> +{
> +    g_free(tpf);
> +}

Kevin