[Qemu-devel] Re: [patch 05/11] qemu: separate thread for io

[Anthony Liguori <aliguori@us.ibm.com>]

mtosatti@redhat.com wrote:
> Introduce a thread to handle host IO events.
>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>
> Index: trunk/qemu-common.h
> ===================================================================
> --- trunk.orig/qemu-common.h
> +++ trunk/qemu-common.h
> @@ -191,6 +191,10 @@ void main_loop_break(void);
>  /* Force QEMU to process pending events */
>  void qemu_notify_event(void);
>
> +/* Unblock cpu */
> +void qemu_cpu_kick(void *env);
> +int qemu_cpu_self(void *env);
> +
>  typedef struct QEMUIOVector {
>      struct iovec *iov;
>      int niov;
> Index: trunk/vl.c
> ===================================================================
> --- trunk.orig/vl.c
> +++ trunk/vl.c
> @@ -146,6 +146,7 @@ int main(int argc, char **argv)
>  #include "gdbstub.h"
>  #include "qemu-timer.h"
>  #include "qemu-char.h"
> +#include "qemu-thread.h"
>  #include "cache-utils.h"
>  #include "block.h"
>  #include "dma.h"
> @@ -278,6 +279,13 @@ uint8_t qemu_uuid[16];
>
>  static int io_thread_fd = -1;
>
> +QemuMutex qemu_global_mutex;
> +QemuMutex qemu_fair_mutex;
> +
> +QemuThread io_thread;
> +QemuThread cpus_thread;
> +QemuCond halt_cond;
> +
>  /***********************************************************/
>  /* x86 ISA bus support */
>
> @@ -1347,8 +1355,6 @@ static void host_alarm_handler(int host_
>          write(alarm_timer_wfd, &byte, sizeof(byte));
>  #endif
>          alarm_timer->flags |= ALARM_FLAG_EXPIRED;
> -
> -        qemu_notify_event();
>      }
>  }
>   

Isn't this unsafe in TCG?  If you have chained TBs in a tight loop, 
removing qemu_notify_event() eliminates the cpu_interrupt call which 
then means that you'll never break out of that tight TB loop.

> @@ -2957,6 +2963,7 @@ int qemu_set_fd_handler2(int fd,
>          ioh->opaque = opaque;
>          ioh->deleted = 0;
>      }
> +    main_loop_break();
>      return 0;
>  }
>
> @@ -3324,7 +3331,6 @@ static int ram_load(QEMUFile *f, void *o
>
>  void qemu_service_io(void)
>  {
> -    qemu_notify_event();
>  }
>   

The same comment from above is applicable here.  qemu_service_io() is 
called from a signal handler.  We should 
s/qemu_service_io/qemu_notify_event/.

>  void main_loop_break(void)
> @@ -3733,6 +3731,105 @@ static void host_main_loop_wait(int *tim
>  }
>  #endif
>   

#ifndef CONFIG_IO_THREAD, doesn't main_loop_break need to do cpu_interrupt?

> +static void qemu_wait_io_event(CPUState *env, int timeout)
> +{
> +    if (timeout)
> +        while (!tcg_has_work(env))
> +            qemu_cond_timedwait(&halt_cond, &qemu_global_mutex, timeout);
>   

What's the point of having a timeout?

> +
> +static void qemu_signal_lock(unsigned int msecs)
> +{
> +    qemu_mutex_lock(&qemu_fair_mutex);
> +
> +    while (qemu_mutex_trylock(&qemu_global_mutex)) {
> +        qemu_thread_signal(&cpus_thread, SIGUSR1);
> +        if (!qemu_mutex_timedlock(&qemu_global_mutex, msecs))
> +            break;
> +    }
> +    qemu_mutex_unlock(&qemu_fair_mutex);
> +}
> +
>   

Now's a good time to attempt to move a lot of the main loop stuff into a 
separate file.  We can do it after the IO thread series or you can make 
it part of the IO thread series.

-- 
Regards,

Anthony Liguori