Re: [Qemu-devel] [PATCH] main: force enabling of I/O thread

[Jan Kiszka]

On 2011-08-22 15:24, Anthony Liguori wrote:
> Enabling the I/O thread by default seems like an important part of declaring
> 1.0.  Besides allowing true SMP support with KVM, the I/O thread means that 
> the
> TCG VCPU doesn't have to multiplex itself with the I/O dispatch routines which
> currently requires a (racey) signal based alarm system.
> 
> I know there have been concerns about performance.  I think so far the ones 
> that
> have come up (virtio-net) are most likely due to secondary reasons like
> decreased batching.

iothread enabled without [1] gives unacceptable performance for ARM
emulation (Musicpal board) here. With that series applied, the host CPU
load is measurably higher, but no longer excessively.

As that series demonstrates, a major issue for TCG is that the iothread
and the VCPU thread run in lock-step, almost nothing is parallelized. At
least so far. Paolo and Peter (and to some degree /me) discussed some
better TCG treading last Thursday.

Jan

[1] http://thread.gmane.org/gmane.comp.emulators.qemu/99658

> 
> I think we ought to force enabling I/O thread early in 1.0 development and
> commit to resolving any lingering issues.
> 
> Signed-off-by: Anthony Liguori <address@hidden>
> ---
>  configure    |   13 -----
>  cpus.c       |  143 
> ----------------------------------------------------------
>  hw/qxl.c     |    4 --
>  kvm-all.c    |    2 +-
>  qemu-timer.c |   53 ---------------------
>  vl.c         |   17 -------
>  6 files changed, 1 insertions(+), 231 deletions(-)
> 
> diff --git a/configure b/configure
> index 234a4c5..ef8d7ae 100755
> --- a/configure
> +++ b/configure
> @@ -165,7 +165,6 @@ darwin_user="no"
>  bsd_user="no"
>  guest_base=""
>  uname_release=""
> -io_thread="no"
>  mixemu="no"
>  aix="no"
>  blobs="yes"
> @@ -722,8 +721,6 @@ for opt do
>    ;;
>    --enable-attr) attr="yes"
>    ;;
> -  --enable-io-thread) io_thread="yes"
> -  ;;
>    --disable-blobs) blobs="no"
>    ;;
>    --with-pkgversion=*) pkgversion=" ($optarg)"
> @@ -2159,12 +2156,6 @@ EOF
>  
>  if compile_prog "" "" ; then
>    signalfd=yes
> -elif test "$kvm" = "yes" -a "$io_thread" != "yes"; then
> -  echo
> -  echo "ERROR: Host kernel lacks signalfd() support,"
> -  echo "but KVM depends on it when the IO thread is disabled."
> -  echo
> -  exit 1
>  fi
>  
>  # check if eventfd is supported
> @@ -2711,7 +2702,6 @@ echo "NPTL support      $nptl"
>  echo "GUEST_BASE        $guest_base"
>  echo "PIE user targets  $user_pie"
>  echo "vde support       $vde"
> -echo "IO thread         $io_thread"
>  echo "Linux AIO support $linux_aio"
>  echo "ATTR/XATTR support $attr"
>  echo "Install blobs     $blobs"
> @@ -2969,9 +2959,6 @@ if test "$xen" = "yes" ; then
>    echo "CONFIG_XEN_BACKEND=y" >> $config_host_mak
>    echo "CONFIG_XEN_CTRL_INTERFACE_VERSION=$xen_ctrl_version" >> 
> $config_host_mak
>  fi
> -if test "$io_thread" = "yes" ; then
> -  echo "CONFIG_IOTHREAD=y" >> $config_host_mak
> -fi
>  if test "$linux_aio" = "yes" ; then
>    echo "CONFIG_LINUX_AIO=y" >> $config_host_mak
>  fi
> diff --git a/cpus.c b/cpus.c
> index c996ac5..fe18a60 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -173,12 +173,9 @@ static void cpu_handle_guest_debug(CPUState *env)
>  {
>      gdb_set_stop_cpu(env);
>      qemu_system_debug_request();
> -#ifdef CONFIG_IOTHREAD
>      env->stopped = 1;
> -#endif
>  }
>  
> -#ifdef CONFIG_IOTHREAD
>  static void cpu_signal(int sig)
>  {
>      if (cpu_single_env) {
> @@ -186,7 +183,6 @@ static void cpu_signal(int sig)
>      }
>      exit_request = 1;
>  }
> -#endif
>  
>  #ifdef CONFIG_LINUX
>  static void sigbus_reraise(void)
> @@ -262,12 +258,6 @@ static void qemu_kvm_eat_signals(CPUState *env)
>              exit(1);
>          }
>      } while (sigismember(&chkset, SIG_IPI) || sigismember(&chkset, SIGBUS));
> -
> -#ifndef CONFIG_IOTHREAD
> -    if (sigismember(&chkset, SIGIO) || sigismember(&chkset, SIGALRM)) {
> -        qemu_notify_event();
> -    }
> -#endif
>  }
>  
>  #else /* !CONFIG_LINUX */
> @@ -390,7 +380,6 @@ static int qemu_signal_init(void)
>      int sigfd;
>      sigset_t set;
>  
> -#ifdef CONFIG_IOTHREAD
>      /* SIGUSR2 used by posix-aio-compat.c */
>      sigemptyset(&set);
>      sigaddset(&set, SIGUSR2);
> @@ -409,18 +398,6 @@ static int qemu_signal_init(void)
>      sigaddset(&set, SIGIO);
>      sigaddset(&set, SIGALRM);
>      sigaddset(&set, SIGBUS);
> -#else
> -    sigemptyset(&set);
> -    sigaddset(&set, SIGBUS);
> -    if (kvm_enabled()) {
> -        /*
> -         * We need to process timer signals synchronously to avoid a race
> -         * between exit_request check and KVM vcpu entry.
> -         */
> -        sigaddset(&set, SIGIO);
> -        sigaddset(&set, SIGALRM);
> -    }
> -#endif
>      pthread_sigmask(SIG_BLOCK, &set, NULL);
>  
>      sigfd = qemu_signalfd(&set);
> @@ -447,7 +424,6 @@ static void qemu_kvm_init_cpu_signals(CPUState *env)
>      sigact.sa_handler = dummy_signal;
>      sigaction(SIG_IPI, &sigact, NULL);
>  
> -#ifdef CONFIG_IOTHREAD
>      pthread_sigmask(SIG_BLOCK, NULL, &set);
>      sigdelset(&set, SIG_IPI);
>      sigdelset(&set, SIGBUS);
> @@ -456,17 +432,7 @@ static void qemu_kvm_init_cpu_signals(CPUState *env)
>          fprintf(stderr, "kvm_set_signal_mask: %s\n", strerror(-r));
>          exit(1);
>      }
> -#else
> -    sigemptyset(&set);
> -    sigaddset(&set, SIG_IPI);
> -    sigaddset(&set, SIGIO);
> -    sigaddset(&set, SIGALRM);
> -    pthread_sigmask(SIG_BLOCK, &set, NULL);
>  
> -    pthread_sigmask(SIG_BLOCK, NULL, &set);
> -    sigdelset(&set, SIGIO);
> -    sigdelset(&set, SIGALRM);
> -#endif
>      sigdelset(&set, SIG_IPI);
>      sigdelset(&set, SIGBUS);
>      r = kvm_set_signal_mask(env, &set);
> @@ -478,7 +444,6 @@ static void qemu_kvm_init_cpu_signals(CPUState *env)
>  
>  static void qemu_tcg_init_cpu_signals(void)
>  {
> -#ifdef CONFIG_IOTHREAD
>      sigset_t set;
>      struct sigaction sigact;
>  
> @@ -489,7 +454,6 @@ static void qemu_tcg_init_cpu_signals(void)
>      sigemptyset(&set);
>      sigaddset(&set, SIG_IPI);
>      pthread_sigmask(SIG_UNBLOCK, &set, NULL);
> -#endif
>  }
>  
>  #else /* _WIN32 */
> @@ -535,106 +499,6 @@ static void qemu_tcg_init_cpu_signals(void)
>  }
>  #endif /* _WIN32 */
>  
> -#ifndef CONFIG_IOTHREAD
> -int qemu_init_main_loop(void)
> -{
> -    int ret;
> -
> -    ret = qemu_signal_init();
> -    if (ret) {
> -        return ret;
> -    }
> -
> -    qemu_init_sigbus();
> -
> -    return qemu_event_init();
> -}
> -
> -void qemu_main_loop_start(void)
> -{
> -}
> -
> -void qemu_init_vcpu(void *_env)
> -{
> -    CPUState *env = _env;
> -    int r;
> -
> -    env->nr_cores = smp_cores;
> -    env->nr_threads = smp_threads;
> -
> -    if (kvm_enabled()) {
> -        r = kvm_init_vcpu(env);
> -        if (r < 0) {
> -            fprintf(stderr, "kvm_init_vcpu failed: %s\n", strerror(-r));
> -            exit(1);
> -        }
> -        qemu_kvm_init_cpu_signals(env);
> -    } else {
> -        qemu_tcg_init_cpu_signals();
> -    }
> -}
> -
> -int qemu_cpu_is_self(void *env)
> -{
> -    return 1;
> -}
> -
> -void run_on_cpu(CPUState *env, void (*func)(void *data), void *data)
> -{
> -    func(data);
> -}
> -
> -void resume_all_vcpus(void)
> -{
> -}
> -
> -void pause_all_vcpus(void)
> -{
> -}
> -
> -void qemu_cpu_kick(void *env)
> -{
> -}
> -
> -void qemu_cpu_kick_self(void)
> -{
> -#ifndef _WIN32
> -    assert(cpu_single_env);
> -
> -    raise(SIG_IPI);
> -#else
> -    abort();
> -#endif
> -}
> -
> -void qemu_notify_event(void)
> -{
> -    CPUState *env = cpu_single_env;
> -
> -    qemu_event_increment ();
> -    if (env) {
> -        cpu_exit(env);
> -    }
> -    if (next_cpu && env != next_cpu) {
> -        cpu_exit(next_cpu);
> -    }
> -    exit_request = 1;
> -}
> -
> -void qemu_mutex_lock_iothread(void) {}
> -void qemu_mutex_unlock_iothread(void) {}
> -
> -void cpu_stop_current(void)
> -{
> -}
> -
> -void vm_stop(int reason)
> -{
> -    do_vm_stop(reason);
> -}
> -
> -#else /* CONFIG_IOTHREAD */
> -
>  QemuMutex qemu_global_mutex;
>  static QemuCond qemu_io_proceeded_cond;
>  static bool iothread_requesting_mutex;
> @@ -1036,8 +900,6 @@ void vm_stop(int reason)
>      do_vm_stop(reason);
>  }
>  
> -#endif
> -
>  static int tcg_cpu_exec(CPUState *env)
>  {
>      int ret;
> @@ -1092,11 +954,6 @@ bool cpu_exec_all(void)
>          qemu_clock_enable(vm_clock,
>                            (env->singlestep_enabled & SSTEP_NOTIMER) == 0);
>  
> -#ifndef CONFIG_IOTHREAD
> -        if (qemu_alarm_pending()) {
> -            break;
> -        }
> -#endif
>          if (cpu_can_run(env)) {
>              if (kvm_enabled()) {
>                  r = kvm_cpu_exec(env);
> diff --git a/hw/qxl.c b/hw/qxl.c
> index bab60a5..c3a3e97 100644
> --- a/hw/qxl.c
> +++ b/hw/qxl.c
> @@ -1388,11 +1388,7 @@ static void init_pipe_signaling(PCIQXLDevice *d)
>         dprint(d, 1, "%s: pipe creation failed\n", __FUNCTION__);
>         return;
>     }
> -#ifdef CONFIG_IOTHREAD
>     fcntl(d->pipe[0], F_SETFL, O_NONBLOCK);
> -#else
> -   fcntl(d->pipe[0], F_SETFL, O_NONBLOCK /* | O_ASYNC */);
> -#endif
>     fcntl(d->pipe[1], F_SETFL, O_NONBLOCK);
>     fcntl(d->pipe[0], F_SETOWN, getpid());
>  
> diff --git a/kvm-all.c b/kvm-all.c
> index 0ae2e26..fbb9ff3 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -479,7 +479,7 @@ static int kvm_check_many_ioeventfds(void)
>       * Older kernels have a 6 device limit on the KVM io bus.  Find out so we
>       * can avoid creating too many ioeventfds.
>       */
> -#if defined(CONFIG_EVENTFD) && defined(CONFIG_IOTHREAD)
> +#if defined(CONFIG_EVENTFD)
>      int ioeventfds[7];
>      int i, ret = 0;
>      for (i = 0; i < ARRAY_SIZE(ioeventfds); i++) {
> diff --git a/qemu-timer.c b/qemu-timer.c
> index 19313d3..46dd483 100644
> --- a/qemu-timer.c
> +++ b/qemu-timer.c
> @@ -101,22 +101,6 @@ static int64_t cpu_get_clock(void)
>      }
>  }
>  
> -#ifndef CONFIG_IOTHREAD
> -static int64_t qemu_icount_delta(void)
> -{
> -    if (!use_icount) {
> -        return 5000 * (int64_t) 1000000;
> -    } else if (use_icount == 1) {
> -        /* When not using an adaptive execution frequency
> -           we tend to get badly out of sync with real time,
> -           so just delay for a reasonable amount of time.  */
> -        return 0;
> -    } else {
> -        return cpu_get_icount() - cpu_get_clock();
> -    }
> -}
> -#endif
> -
>  /* enable cpu_get_ticks() */
>  void cpu_enable_ticks(void)
>  {
> @@ -688,9 +672,7 @@ void configure_icount(const char *option)
>      if (!option)
>          return;
>  
> -#ifdef CONFIG_IOTHREAD
>      vm_clock->warp_timer = qemu_new_timer_ns(rt_clock, icount_warp_rt, NULL);
> -#endif
>  
>      if (strcmp(option, "auto") != 0) {
>          icount_time_shift = strtol(option, NULL, 0);
> @@ -1178,41 +1160,6 @@ void quit_timers(void)
>  
>  int qemu_calculate_timeout(void)
>  {
> -#ifndef CONFIG_IOTHREAD
> -    int timeout;
> -
> -    if (!vm_running)
> -        timeout = 5000;
> -    else {
> -     /* XXX: use timeout computed from timers */
> -        int64_t add;
> -        int64_t delta;
> -        /* Advance virtual time to the next event.  */
> -     delta = qemu_icount_delta();
> -        if (delta > 0) {
> -            /* If virtual time is ahead of real time then just
> -               wait for IO.  */
> -            timeout = (delta + 999999) / 1000000;
> -        } else {
> -            /* Wait for either IO to occur or the next
> -               timer event.  */
> -            add = qemu_next_icount_deadline();
> -            /* We advance the timer before checking for IO.
> -               Limit the amount we advance so that early IO
> -               activity won't get the guest too far ahead.  */
> -            if (add > 10000000)
> -                add = 10000000;
> -            delta += add;
> -            qemu_icount += qemu_icount_round (add);
> -            timeout = delta / 1000000;
> -            if (timeout < 0)
> -                timeout = 0;
> -        }
> -    }
> -
> -    return timeout;
> -#else /* CONFIG_IOTHREAD */
>      return 1000;
> -#endif
>  }
>  
> diff --git a/vl.c b/vl.c
> index d661e8e..11f5669 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1438,17 +1438,6 @@ void main_loop_wait(int nonblocking)
>  
>  }
>  
> -#ifndef CONFIG_IOTHREAD
> -static int vm_request_pending(void)
> -{
> -    return powerdown_requested ||
> -           reset_requested ||
> -           shutdown_requested ||
> -           debug_requested ||
> -           vmstop_requested;
> -}
> -#endif
> -
>  qemu_irq qemu_system_powerdown;
>  
>  static void main_loop(void)
> @@ -1462,12 +1451,6 @@ static void main_loop(void)
>      qemu_main_loop_start();
>  
>      for (;;) {
> -#ifndef CONFIG_IOTHREAD
> -        nonblocking = cpu_exec_all();
> -        if (vm_request_pending()) {
> -            nonblocking = true;
> -        }
> -#endif
>  #ifdef CONFIG_PROFILER
>          ti = profile_getclock();
>  #endif

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux