From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:53873) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QvUmu-0003f6-P6 for qemu-devel@nongnu.org; Mon, 22 Aug 2011 09:43:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QvUms-0006SW-EV for qemu-devel@nongnu.org; Mon, 22 Aug 2011 09:43:36 -0400 Received: from mail-yw0-f45.google.com ([209.85.213.45]:50340) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QvUms-0006SP-8u for qemu-devel@nongnu.org; Mon, 22 Aug 2011 09:43:34 -0400 Received: by ywf9 with SMTP id 9so4153018ywf.4 for ; Mon, 22 Aug 2011 06:43:33 -0700 (PDT) Message-ID: <4E525D03.1040602@codemonkey.ws> Date: Mon, 22 Aug 2011 08:43:31 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1314019498-8550-1-git-send-email-aliguori@us.ibm.com> <4E525B32.2010602@siemens.com> In-Reply-To: <4E525B32.2010602@siemens.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] main: force enabling of I/O thread List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Kiszka Cc: Peter Maydell , Anthony Liguori , "qemu-devel@nongnu.org" , Blue Swirl , Paolo Bonzini , Aurelien Jarno On 08/22/2011 08:35 AM, Jan Kiszka wrote: > 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. Can you rebase and resubmit? I think there was confusion about which tree they were supposed to go through. I'll apply them directly. > > 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. I still think we should enable unconditionally. We have a few months to try and improve the situation further. Fundamentally, using a dedicated VCPU thread is the Right Architecture for TCG so I think we are just delaying the inevitable by leaving it disabled. Regards, Anthony Liguori > > 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 >> --- >> 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 >