Re: [Qemu-devel] [PATCH] posix-aio-compat: fix latency issues

[Frediano Ziglio]

2011/8/8 Avi Kivity <address@hidden>:
> In certain circumstances, posix-aio-compat can incur a lot of latency:
>  - threads are created by vcpu threads, so if vcpu affinity is set,
>   aio threads inherit vcpu affinity.  This can cause many aio threads
>   to compete for one cpu.
>  - we can create up to max_threads (64) aio threads in one go; since a
>   pthread_create can take around 30μs, we have up to 2ms of cpu time
>   under a global lock.
>
> Fix by:
>  - moving thread creation to the main thread, so we inherit the main
>   thread's affinity instead of the vcpu thread's affinity.
>  - if a thread is currently being created, and we need to create yet
>   another thread, let thread being born create the new thread, reducing
>   the amount of time we spend under the main thread.
>  - drop the local lock while creating a thread (we may still hold the
>   global mutex, though)
>
> Note this doesn't eliminate latency completely; scheduler artifacts or
> lack of host cpu resources can still cause it.  We may want pre-allocated
> threads when this cannot be tolerated.
>
> Thanks to Uli Obergfell of Red Hat for his excellent analysis and suggestions.
>
> Signed-off-by: Avi Kivity <address@hidden>

Why not calling pthread_attr_setaffinity_np (where available) before
thread creation or shed_setaffinity at thread start instead of telling
another thread to create a thread for us just to get affinity cleared?

Regards
  Frediano

> ---
>  posix-aio-compat.c |   48 ++++++++++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 46 insertions(+), 2 deletions(-)
>
> diff --git a/posix-aio-compat.c b/posix-aio-compat.c
> index 8dc00cb..aa30673 100644
> --- a/posix-aio-compat.c
> +++ b/posix-aio-compat.c
> @@ -30,6 +30,7 @@
>
>  #include "block/raw-posix-aio.h"
>
> +static void do_spawn_thread(void);
>
>  struct qemu_paiocb {
>     BlockDriverAIOCB common;
> @@ -64,6 +65,9 @@ static pthread_attr_t attr;
>  static int max_threads = 64;
>  static int cur_threads = 0;
>  static int idle_threads = 0;
> +static int new_threads = 0;     /* backlog of threads we need to create */
> +static int pending_threads = 0; /* threads created but not running yet */
> +static QEMUBH *new_thread_bh;
>  static QTAILQ_HEAD(, qemu_paiocb) request_list;
>
>  #ifdef CONFIG_PREADV
> @@ -311,6 +315,13 @@ static void *aio_thread(void *unused)
>
>     pid = getpid();
>
> +    mutex_lock(&lock);
> +    if (new_threads) {
> +        do_spawn_thread();
> +    }
> +    pending_threads--;
> +    mutex_unlock(&lock);
> +
>     while (1) {
>         struct qemu_paiocb *aiocb;
>         ssize_t ret = 0;
> @@ -381,11 +392,18 @@ static void *aio_thread(void *unused)
>     return NULL;
>  }
>
> -static void spawn_thread(void)
> +static void do_spawn_thread(void)
>  {
>     sigset_t set, oldset;
>
> -    cur_threads++;
> +    if (!new_threads) {
> +        return;
> +    }
> +
> +    new_threads--;
> +    pending_threads++;
> +
> +    mutex_unlock(&lock);
>
>     /* block all signals */
>     if (sigfillset(&set)) die("sigfillset");
> @@ -394,6 +412,31 @@ static void spawn_thread(void)
>     thread_create(&thread_id, &attr, aio_thread, NULL);
>
>     if (sigprocmask(SIG_SETMASK, &oldset, NULL)) die("sigprocmask restore");
> +
> +    mutex_lock(&lock);
> +}
> +
> +static void spawn_thread_bh_fn(void *opaque)
> +{
> +    mutex_lock(&lock);
> +    do_spawn_thread();
> +    mutex_unlock(&lock);
> +}
> +
> +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 qemu_paio_submit(struct qemu_paiocb *aiocb)
> @@ -665,6 +708,7 @@ int paio_init(void)
>         die2(ret, "pthread_attr_setdetachstate");
>
>     QTAILQ_INIT(&request_list);
> +    new_thread_bh = qemu_bh_new(spawn_thread_bh_fn, NULL);
>
>     posix_aio_state = s;
>     return 0;
> --
> 1.7.5.3
>
>
>