Re: [Qemu-devel] [PATCH v2 2/5] tpm: replace GThreadPool with AIO threadpool

[Stefan Berger <stefanb@linux.vnet.ibm.com>]

On 01/19/2018 09:11 AM, Marc-André Lureau wrote:
> The TPM backend uses a GThreadPool to handle IO in a seperate
> thread. However, GThreadPool isn't integrated with Qemu main loops,
> making it unnecessarily complicated to deal with.
>
> Qemu has a AIO threadpool, that is better integrated with loops and
> various IO functions, provides completion BH by default etc.
>
> Remove the only user of GThreadPool from qemu, use AIO threadpool.
>
> Note that the backend:
> - no longer accepts queing multiple requests (unneeded so far)
> - increase ref to itself when handling a command, for extra safety
> - tpm_backend_thread_end() is renamed tpm_backend_finish_sync() and
> will wait for completion of BH (request_completed), which will help
> migration handling.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

I have been using this for a while:
Tested-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
Reviewed-by: Stefan Berger <stefanb@linux.vnet.ibm.com>


> ---
>   include/sysemu/tpm_backend.h | 12 ++++++++--
>   backends/tpm.c               | 52 +++++++++++++++++++++++---------------------
>   2 files changed, 37 insertions(+), 27 deletions(-)
>
> diff --git a/include/sysemu/tpm_backend.h b/include/sysemu/tpm_backend.h
> index 0d6c994a62..a69593e0cd 100644
> --- a/include/sysemu/tpm_backend.h
> +++ b/include/sysemu/tpm_backend.h
> @@ -45,9 +45,8 @@ struct TPMBackend {
>       /*< protected >*/
>       TPMIf *tpmif;
>       bool opened;
> -    GThreadPool *thread_pool;
>       bool had_startup_error;
> -    QEMUBH *bh;
> +    TPMBackendCmd *cmd;
>
>       /* <public> */
>       char *id;
> @@ -196,6 +195,15 @@ TPMVersion tpm_backend_get_tpm_version(TPMBackend *s);
>    */
>   size_t tpm_backend_get_buffer_size(TPMBackend *s);
>
> +/**
> + * tpm_backend_finish_sync:
> + * @s: the backend to call into
> + *
> + * Finish the pending command synchronously (this will call aio_poll()
> + * on qemu main AIOContext until it ends)
> + */
> +void tpm_backend_finish_sync(TPMBackend *s);
> +
>   /**
>    * tpm_backend_query_tpm:
>    * @s: the backend
> diff --git a/backends/tpm.c b/backends/tpm.c
> index 91222c5164..143807aa37 100644
> --- a/backends/tpm.c
> +++ b/backends/tpm.c
> @@ -19,30 +19,35 @@
>   #include "sysemu/tpm.h"
>   #include "qemu/thread.h"
>   #include "qemu/main-loop.h"
> +#include "block/thread-pool.h"
> +#include "qemu/error-report.h"
>
> -static void tpm_backend_request_completed_bh(void *opaque)
> +static void tpm_backend_request_completed(void *opaque, int ret)
>   {
>       TPMBackend *s = TPM_BACKEND(opaque);
>       TPMIfClass *tic = TPM_IF_GET_CLASS(s->tpmif);
>
>       tic->request_completed(s->tpmif);
> +
> +    /* no need for atomic, as long the BQL is taken */
> +    s->cmd = NULL;
> +    object_unref(OBJECT(s));
>   }
>
> -static void tpm_backend_worker_thread(gpointer data, gpointer user_data)
> +static int tpm_backend_worker_thread(gpointer data)
>   {
> -    TPMBackend *s = TPM_BACKEND(user_data);
> +    TPMBackend *s = TPM_BACKEND(data);
>       TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);
>
> -    k->handle_request(s, (TPMBackendCmd *)data);
> +    k->handle_request(s, s->cmd);
>
> -    qemu_bh_schedule(s->bh);
> +    return 0;
>   }
>
> -static void tpm_backend_thread_end(TPMBackend *s)
> +void tpm_backend_finish_sync(TPMBackend *s)
>   {
> -    if (s->thread_pool) {
> -        g_thread_pool_free(s->thread_pool, FALSE, TRUE);
> -        s->thread_pool = NULL;
> +    while (s->cmd) {
> +        aio_poll(qemu_get_aio_context(), true);
>       }
>   }
>
> @@ -74,10 +79,7 @@ int tpm_backend_startup_tpm(TPMBackend *s, size_t buffersize)
>       TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);
>
>       /* terminate a running TPM */
> -    tpm_backend_thread_end(s);
> -
> -    s->thread_pool = g_thread_pool_new(tpm_backend_worker_thread, s, 1, TRUE,
> -                                       NULL);
> +    tpm_backend_finish_sync(s);
>
>       res = k->startup_tpm ? k->startup_tpm(s, buffersize) : 0;
>
> @@ -93,7 +95,17 @@ bool tpm_backend_had_startup_error(TPMBackend *s)
>
>   void tpm_backend_deliver_request(TPMBackend *s, TPMBackendCmd *cmd)
>   {
> -    g_thread_pool_push(s->thread_pool, cmd, NULL);
> +    ThreadPool *pool = aio_get_thread_pool(qemu_get_aio_context());
> +
> +    if (s->cmd != NULL) {
> +        error_report("There is a TPM request pending");
> +        return;
> +    }
> +
> +    s->cmd = cmd;
> +    object_ref(OBJECT(s));
> +    thread_pool_submit_aio(pool, tpm_backend_worker_thread, s,
> +                           tpm_backend_request_completed, s);
>   }
>
>   void tpm_backend_reset(TPMBackend *s)
> @@ -104,7 +116,7 @@ void tpm_backend_reset(TPMBackend *s)
>           k->reset(s);
>       }
>
> -    tpm_backend_thread_end(s);
> +    tpm_backend_finish_sync(s);
>
>       s->had_startup_error = false;
>   }
> @@ -159,28 +171,18 @@ TPMInfo *tpm_backend_query_tpm(TPMBackend *s)
>       return info;
>   }
>
> -static void tpm_backend_instance_init(Object *obj)
> -{
> -    TPMBackend *s = TPM_BACKEND(obj);
> -
> -    s->bh = qemu_bh_new(tpm_backend_request_completed_bh, s);
> -}
> -
>   static void tpm_backend_instance_finalize(Object *obj)
>   {
>       TPMBackend *s = TPM_BACKEND(obj);
>
>       object_unref(OBJECT(s->tpmif));
>       g_free(s->id);
> -    tpm_backend_thread_end(s);
> -    qemu_bh_delete(s->bh);
>   }
>
>   static const TypeInfo tpm_backend_info = {
>       .name = TYPE_TPM_BACKEND,
>       .parent = TYPE_OBJECT,
>       .instance_size = sizeof(TPMBackend),
> -    .instance_init = tpm_backend_instance_init,
>       .instance_finalize = tpm_backend_instance_finalize,
>       .class_size = sizeof(TPMBackendClass),
>       .abstract = true,