[Qemu-devel] [patch 0/7] separate thread for io v2

[Qemu-devel] [patch 0/7] separate thread for io v2

[mtosatti]

Move IO event processing to a separate thread, with a global mutex
serializing access to QEMU data structures. This opens the possibility
to execute certain operations in parallel, and is necessary to integrate
KVM SMP support.

Future changes:
- make the cpu_single_env history nicer
- use signalfd and block IO event signals in the io thread
- optimize tcg interaction
- implement kvm smp

-- 

[Qemu-devel] [patch 1/7] qemu: mutex/thread/cond wrappers

[mtosatti]

[-- Attachment #1: iothread-mutex --]
[-- Type: text/plain, Size: 5444 bytes --]


Index: qemu/Makefile.target
===================================================================
--- qemu.orig/Makefile.target
+++ qemu/Makefile.target
@@ -500,7 +500,7 @@ endif #CONFIG_BSD_USER
 # System emulator target
 ifndef CONFIG_USER_ONLY
 
-OBJS=vl.o osdep.o monitor.o pci.o loader.o isa_mmio.o machine.o
+OBJS=vl.o qemu-thread.o osdep.o monitor.o pci.o loader.o isa_mmio.o machine.o
 # virtio has to be here due to weird dependency between PCI and virtio-net.
 # need to fix this properly
 OBJS+=virtio.o virtio-blk.o virtio-balloon.o virtio-net.o virtio-console.o
Index: qemu/vl.c
===================================================================
--- qemu.orig/vl.c
+++ qemu/vl.c
@@ -36,6 +36,7 @@
 #include "gdbstub.h"
 #include "qemu-timer.h"
 #include "qemu-char.h"
+#include "qemu-thread.h"
 #include "cache-utils.h"
 #include "block.h"
 #include "audio/audio.h"
@@ -263,6 +264,8 @@ static QEMUTimer *nographic_timer;
 
 uint8_t qemu_uuid[16];
 
+QemuMutex qemu_global_mutex;
+
 /***********************************************************/
 /* x86 ISA bus support */
 
@@ -3650,7 +3653,14 @@ void main_loop_wait(int timeout)
         slirp_select_fill(&nfds, &rfds, &wfds, &xfds);
     }
 #endif
+
+    /*
+     * main_loop_wait() *must* not assume any global state is consistent across
+     * select() invocations.
+     */
+    qemu_mutex_unlock(&qemu_global_mutex);
     ret = select(nfds + 1, &rfds, &wfds, &xfds, &tv);
+    qemu_mutex_lock(&qemu_global_mutex);
     if (ret > 0) {
         IOHandlerRecord **pioh;
 
@@ -3708,6 +3718,9 @@ static int main_loop(void)
 #endif
     CPUState *env;
 
+    qemu_mutex_init(&qemu_global_mutex);
+    qemu_mutex_lock(&qemu_global_mutex);
+
     cur_cpu = first_cpu;
     next_cpu = cur_cpu->next_cpu ?: first_cpu;
     for(;;) {
Index: qemu/qemu-thread.c
===================================================================
--- /dev/null
+++ qemu/qemu-thread.c
@@ -0,0 +1,99 @@
+#include <errno.h>
+#include <time.h>
+#include <signal.h>
+#include "qemu-thread.h"
+
+int qemu_mutex_init(QemuMutex *mutex)
+{
+    return pthread_mutex_init(&mutex->lock, NULL);
+}
+
+void qemu_mutex_lock(QemuMutex *mutex)
+{
+    pthread_mutex_lock(&mutex->lock);
+}
+
+int qemu_mutex_trylock(QemuMutex *mutex)
+{
+    return pthread_mutex_trylock(&mutex->lock);
+}
+
+static void add_to_timespec(struct timespec *ts, unsigned int msecs)
+{
+    ts->tv_sec = ts->tv_sec + (long)(msecs / 1000);
+    ts->tv_nsec = (ts->tv_nsec + ((long)msecs % 1000) * 1000000);
+    if (ts->tv_nsec >= 1000000000) {
+        ts->tv_nsec -= 1000000000;
+        ts->tv_sec++;
+    }
+}
+
+int qemu_mutex_timedlock(QemuMutex *mutex, unsigned int msecs)
+{
+    struct timespec ts;
+
+    clock_gettime(CLOCK_REALTIME, &ts);
+    add_to_timespec(&ts, msecs);
+
+    return pthread_mutex_timedlock(&mutex->lock, &ts);
+}
+
+void qemu_mutex_unlock(QemuMutex *mutex)
+{
+    pthread_mutex_unlock(&mutex->lock);
+}
+
+void qemu_cond_init(QemuCond *cond)
+{
+    pthread_cond_init(&cond->cond, NULL);
+}
+
+void qemu_cond_signal(QemuCond *cond)
+{
+    pthread_cond_signal(&cond->cond);
+}
+
+void qemu_cond_broadcast(QemuCond *cond)
+{
+    pthread_cond_broadcast(&cond->cond);
+}
+
+int qemu_cond_wait(QemuCond *cond, QemuMutex *mutex)
+{
+    return pthread_cond_wait(&cond->cond, &mutex->lock);
+}
+
+int qemu_cond_timedwait(QemuCond *cond, QemuMutex *mutex, unsigned int msecs)
+{
+    struct timespec ts;
+
+    clock_gettime(CLOCK_REALTIME, &ts);
+    add_to_timespec(&ts, msecs);
+
+    return pthread_cond_timedwait(&cond->cond, &mutex->lock, &ts);
+}
+
+int qemu_thread_create(QemuThread *thread,
+                       void *(*start_routine)(void*),
+                       void *arg)
+{
+    return pthread_create(&thread->thread, NULL, start_routine, arg);
+}
+
+int qemu_thread_signal(QemuThread *thread, int sig)
+{
+    if (thread->thread != 0)
+        return pthread_kill(thread->thread, sig);
+    return -1; /* XXX: ESCHR */
+}
+
+void qemu_thread_self(QemuThread *thread)
+{
+    thread->thread = pthread_self();
+}
+
+int qemu_thread_equal(QemuThread *thread1, QemuThread *thread2)
+{
+   return (thread1->thread == thread2->thread);
+}
+
Index: qemu/qemu-thread.h
===================================================================
--- /dev/null
+++ qemu/qemu-thread.h
@@ -0,0 +1,38 @@
+#include "semaphore.h"
+#include "pthread.h"
+
+struct QemuMutex {
+    pthread_mutex_t lock;
+};
+
+struct QemuCond {
+    pthread_cond_t cond;
+};
+
+struct QemuThread {
+    pthread_t thread;
+};
+
+typedef struct QemuMutex QemuMutex;
+typedef struct QemuCond QemuCond;
+typedef struct QemuThread QemuThread;
+
+int qemu_mutex_init(QemuMutex *mutex);
+void qemu_mutex_lock(QemuMutex *mutex);
+int qemu_mutex_trylock(QemuMutex *mutex);
+int qemu_mutex_timedlock(QemuMutex *mutex, unsigned int msecs);
+void qemu_mutex_unlock(QemuMutex *mutex);
+
+void qemu_cond_init(QemuCond *cond);
+void qemu_cond_signal(QemuCond *cond);
+void qemu_cond_broadcast(QemuCond *cond);
+int qemu_cond_wait(QemuCond *cond, QemuMutex *mutex);
+int qemu_cond_timedwait(QemuCond *cond, QemuMutex *mutex, unsigned int msecs);
+
+int qemu_thread_create(QemuThread *thread,
+                       void *(*start_routine)(void*),
+                       void *arg);
+int qemu_thread_signal(QemuThread *thread, int sig);
+void qemu_thread_self(QemuThread *thread);
+int qemu_thread_equal(QemuThread *thread1, QemuThread *thread2);
+

-- 

[Qemu-devel] [patch 2/7] qemu: separate thread for io

[mtosatti]

[-- Attachment #1: introduce-iothread --]
[-- Type: text/plain, Size: 7694 bytes --]


Index: qemu/exec.c
===================================================================
--- qemu.orig/exec.c
+++ qemu/exec.c
@@ -1513,6 +1513,20 @@ void cpu_interrupt(CPUState *env, int ma
     /* FIXME: This is probably not threadsafe.  A different thread could
        be in the middle of a read-modify-write operation.  */
     env->interrupt_request |= mask;
+
+    switch(mask) {
+    case CPU_INTERRUPT_HARD:
+    case CPU_INTERRUPT_SMI:
+    case CPU_INTERRUPT_NMI:
+    case CPU_INTERRUPT_EXIT:
+    /*
+     * only unlink the TB's if we're called from cpu thread context,
+     * otherwise signal cpu thread to do it.
+     */
+        if (qemu_notify_event(env))
+               return;
+    }
+
 #if defined(USE_NPTL)
     /* FIXME: TB unchaining isn't SMP safe.  For now just ignore the
        problem and hope the cpu will stop of its own accord.  For userspace
Index: qemu/qemu-common.h
===================================================================
--- qemu.orig/qemu-common.h
+++ qemu/qemu-common.h
@@ -190,6 +190,8 @@ int cpu_load(QEMUFile *f, void *opaque, 
 
 /* Force QEMU to stop what it's doing and service IO */
 void qemu_service_io(void);
+void main_loop_break(void);
+int qemu_notify_event(void *env);
 
 typedef struct QEMUIOVector {
     struct iovec *iov;
Index: qemu/vl.c
===================================================================
--- qemu.orig/vl.c
+++ qemu/vl.c
@@ -155,6 +155,7 @@
 //#define DEBUG_NET
 //#define DEBUG_SLIRP
 
+#define SIG_IPI (SIGRTMIN+4)
 
 #ifdef DEBUG_IOPORT
 #  define LOG_IOPORT(...) qemu_log_mask(CPU_LOG_IOPORT, ## __VA_ARGS__)
@@ -265,6 +266,10 @@ static QEMUTimer *nographic_timer;
 uint8_t qemu_uuid[16];
 
 QemuMutex qemu_global_mutex;
+QemuMutex qemu_fair_mutex;
+
+QemuThread io_thread;
+QemuThread cpus_thread;
 
 /***********************************************************/
 /* x86 ISA bus support */
@@ -1328,7 +1333,6 @@ static void host_alarm_handler(int host_
                                qemu_get_clock(vm_clock))) ||
         qemu_timer_expired(active_timers[QEMU_TIMER_REALTIME],
                            qemu_get_clock(rt_clock))) {
-        CPUState *env = next_cpu;
 
 #ifdef _WIN32
         struct qemu_alarm_win32 *data = ((struct qemu_alarm_timer*)dwUser)->priv;
@@ -1339,15 +1343,6 @@ static void host_alarm_handler(int host_
 #endif
         alarm_timer->flags |= ALARM_FLAG_EXPIRED;
 
-        if (env) {
-            /* stop the currently executing cpu because a timer occured */
-            cpu_interrupt(env, CPU_INTERRUPT_EXIT);
-#ifdef USE_KQEMU
-            if (env->kqemu_enabled) {
-                kqemu_cpu_interrupt(env);
-            }
-#endif
-        }
         event_pending = 1;
     }
 }
@@ -2878,6 +2873,7 @@ int qemu_set_fd_handler2(int fd,
         ioh->opaque = opaque;
         ioh->deleted = 0;
     }
+    main_loop_break();
     return 0;
 }
 
@@ -3334,6 +3330,7 @@ void qemu_bh_schedule(QEMUBH *bh)
     if (env) {
         cpu_interrupt(env, CPU_INTERRUPT_EXIT);
     }
+    main_loop_break();
 }
 
 void qemu_bh_cancel(QEMUBH *bh)
@@ -3611,6 +3608,155 @@ static void host_main_loop_wait(int *tim
 }
 #endif
 
+static int wait_signal(int timeout)
+{
+    struct timespec ts;
+    sigset_t waitset;
+
+    ts.tv_sec = timeout / 1000;
+    ts.tv_nsec = (timeout % 1000) * 1000000;
+    sigemptyset(&waitset);
+    sigaddset(&waitset, SIGUSR1);
+
+    return sigtimedwait(&waitset, NULL, &ts);
+}
+
+static int has_work(CPUState *env)
+{
+    int r = 0;
+    if (!env->halted)
+        r = 1;
+    if (env->interrupt_request & (CPU_INTERRUPT_HARD | CPU_INTERRUPT_NMI))
+        r = 1;
+    return r;
+}
+
+static void qemu_wait_io_event(CPUState *env, int timeout)
+{
+    qemu_mutex_unlock(&qemu_global_mutex);
+
+    if (timeout && !has_work(env))
+        wait_signal(timeout);
+    /*
+     * Users of qemu_global_mutex can be starved, having no chance
+     * to acquire it since this path will get to it first.
+     * So use another lock to provide fairness.
+     */
+    qemu_mutex_lock(&qemu_fair_mutex);
+    qemu_mutex_unlock(&qemu_fair_mutex);
+
+    qemu_mutex_lock(&qemu_global_mutex);
+}
+
+static void cpu_signal(int sig)
+{
+    QemuThread self;
+    CPUState *env = cpu_single_env;
+
+    event_pending = 1;
+
+    qemu_thread_self(&self);
+    if (!qemu_thread_equal(&self, &cpus_thread))
+        return;
+
+    if (env) {
+            /* stop the currently executing cpu because an event occurred */
+        cpu_interrupt(env, CPU_INTERRUPT_EXIT);
+#ifdef USE_KQEMU
+        if (env->kqemu_enabled) {
+            kqemu_cpu_interrupt(env);
+        }
+#endif
+    }
+}
+
+static void block_io_signals(void)
+{
+    sigset_t set;
+    struct sigaction sigact;
+
+    sigemptyset(&set);
+    sigaddset(&set, SIGUSR2);
+    sigaddset(&set, SIGIO);
+    sigaddset(&set, SIGALRM);
+    sigaddset(&set, SIG_IPI);
+    pthread_sigmask(SIG_BLOCK, &set, NULL);
+
+    sigemptyset(&set);
+    sigaddset(&set, SIGUSR1);
+    pthread_sigmask(SIG_UNBLOCK, &set, NULL);
+
+    memset(&sigact, 0, sizeof(sigact));
+    sigact.sa_handler = cpu_signal;
+    sigaction(SIGUSR1, &sigact, NULL);
+}
+
+/* used to wake up the io thread */
+static void sig_ipi_handler(int sig)
+{
+}
+
+static void unblock_io_signals(void)
+{
+    sigset_t set;
+    struct sigaction sigact;
+
+    sigemptyset(&set);
+    sigaddset(&set, SIGUSR2);
+    sigaddset(&set, SIGIO);
+    sigaddset(&set, SIGALRM);
+    sigaddset(&set, SIG_IPI);
+    pthread_sigmask(SIG_UNBLOCK, &set, NULL);
+
+    sigemptyset(&set);
+    sigaddset(&set, SIGUSR1);
+    pthread_sigmask(SIG_BLOCK, &set, NULL);
+
+    memset(&sigact, 0, sizeof(sigact));
+    sigact.sa_handler = sig_ipi_handler;
+    sigaction(SIG_IPI, &sigact, NULL);
+}
+
+int qemu_notify_event(void *cpu_env)
+{
+    QemuThread me;
+    qemu_thread_self(&me);
+
+    if (qemu_thread_equal(&cpus_thread, &me))
+        return 0;
+    qemu_thread_signal(&cpus_thread, SIGUSR1);
+    return 1;
+}
+
+void main_loop_break(void)
+{
+    QemuThread me;
+    qemu_thread_self(&me);
+    if (qemu_thread_equal(&io_thread, &me))
+        return;
+    qemu_thread_signal(&io_thread, SIG_IPI);
+}
+
+static void *io_thread_fn(void *arg)
+{
+    unblock_io_signals();
+    qemu_mutex_lock(&qemu_global_mutex);
+    while (1)
+        main_loop_wait(1000);
+}
+
+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);
+}
+
 void main_loop_wait(int timeout)
 {
     IOHandlerRecord *ioh;
@@ -3660,7 +3806,7 @@ void main_loop_wait(int timeout)
      */
     qemu_mutex_unlock(&qemu_global_mutex);
     ret = select(nfds + 1, &rfds, &wfds, &xfds, &tv);
-    qemu_mutex_lock(&qemu_global_mutex);
+    qemu_signal_lock(100);
     if (ret > 0) {
         IOHandlerRecord **pioh;
 
@@ -3718,9 +3864,15 @@ static int main_loop(void)
 #endif
     CPUState *env;
 
+    qemu_mutex_init(&qemu_fair_mutex);
     qemu_mutex_init(&qemu_global_mutex);
     qemu_mutex_lock(&qemu_global_mutex);
 
+    qemu_thread_create(&io_thread, io_thread_fn, NULL);
+    block_io_signals();
+
+    qemu_thread_self(&cpus_thread);
+
     cur_cpu = first_cpu;
     next_cpu = cur_cpu->next_cpu ?: first_cpu;
     for(;;) {
@@ -3853,7 +4005,7 @@ static int main_loop(void)
 #ifdef CONFIG_PROFILER
         ti = profile_getclock();
 #endif
-        main_loop_wait(timeout);
+        qemu_wait_io_event(env, timeout);
 #ifdef CONFIG_PROFILER
         dev_time += profile_getclock() - ti;
 #endif

-- 

[Qemu-devel] [patch 3/7] qemu: main thread does io and cpu thread is spawned

[mtosatti]

[-- Attachment #1: vcpus-on-thread --]
[-- Type: text/plain, Size: 2647 bytes --]


Index: qemu/vl.c
===================================================================
--- qemu.orig/vl.c
+++ qemu/vl.c
@@ -3623,12 +3623,13 @@ static int wait_signal(int timeout)
 
 static int has_work(CPUState *env)
 {
-    int r = 0;
+    if (!vm_running)
+        return 0;
     if (!env->halted)
-        r = 1;
+        return 1;
     if (env->interrupt_request & (CPU_INTERRUPT_HARD | CPU_INTERRUPT_NMI))
-        r = 1;
-    return r;
+        return 1;
+    return 0;
 }
 
 static void qemu_wait_io_event(CPUState *env, int timeout)
@@ -3737,14 +3738,6 @@ void main_loop_break(void)
     qemu_thread_signal(&io_thread, SIG_IPI);
 }
 
-static void *io_thread_fn(void *arg)
-{
-    unblock_io_signals();
-    qemu_mutex_lock(&qemu_global_mutex);
-    while (1)
-        main_loop_wait(1000);
-}
-
 static void qemu_signal_lock(unsigned int msecs)
 {
     qemu_mutex_lock(&qemu_fair_mutex);
@@ -3842,9 +3835,11 @@ void main_loop_wait(int timeout)
 #endif
 
     /* vm time timers */
-    if (vm_running && likely(!(cur_cpu->singlestep_enabled & SSTEP_NOTIMER)))
-        qemu_run_timers(&active_timers[QEMU_TIMER_VIRTUAL],
-                        qemu_get_clock(vm_clock));
+    if (vm_running) {
+        if (cur_cpu && likely(!(cur_cpu->singlestep_enabled & SSTEP_NOTIMER)))
+            qemu_run_timers(&active_timers[QEMU_TIMER_VIRTUAL],
+                             qemu_get_clock(vm_clock));
+    }
 
     /* real time timers */
     qemu_run_timers(&active_timers[QEMU_TIMER_REALTIME],
@@ -3856,7 +3851,7 @@ void main_loop_wait(int timeout)
 
 }
 
-static int main_loop(void)
+static void *cpu_main_loop(void *arg)
 {
     int ret, timeout;
 #ifdef CONFIG_PROFILER
@@ -3864,15 +3859,11 @@ static int main_loop(void)
 #endif
     CPUState *env;
 
-    qemu_mutex_init(&qemu_fair_mutex);
-    qemu_mutex_init(&qemu_global_mutex);
-    qemu_mutex_lock(&qemu_global_mutex);
-
-    qemu_thread_create(&io_thread, io_thread_fn, NULL);
     block_io_signals();
-
     qemu_thread_self(&cpus_thread);
 
+    qemu_mutex_lock(&qemu_global_mutex);
+
     cur_cpu = first_cpu;
     next_cpu = cur_cpu->next_cpu ?: first_cpu;
     for(;;) {
@@ -4011,7 +4002,23 @@ static int main_loop(void)
 #endif
     }
     cpu_disable_ticks();
-    return ret;
+    return NULL;
+}
+
+static void main_loop(void)
+{
+    qemu_mutex_init(&qemu_fair_mutex);
+    qemu_mutex_init(&qemu_global_mutex);
+    qemu_mutex_lock(&qemu_global_mutex);
+
+    qemu_thread_self(&io_thread);
+
+    unblock_io_signals();
+
+    qemu_thread_create(&cpus_thread, cpu_main_loop, NULL);
+
+    while (1)
+        main_loop_wait(1000);
 }
 
 static void help(int exitcode)

-- 

[Qemu-devel] [patch 4/7] qemu: handle reset/poweroff/shutdown in iothread

[mtosatti]

[-- Attachment #1: move-machine-events-to-iothread --]
[-- Type: text/plain, Size: 2475 bytes --]

Its simpler to handle these events from only one context.

Index: qemu/vl.c
===================================================================
--- qemu.orig/vl.c
+++ qemu/vl.c
@@ -3540,22 +3540,19 @@ void qemu_system_reset_request(void)
     } else {
         reset_requested = 1;
     }
-    if (cpu_single_env)
-        cpu_interrupt(cpu_single_env, CPU_INTERRUPT_EXIT);
+    main_loop_break();
 }
 
 void qemu_system_shutdown_request(void)
 {
     shutdown_requested = 1;
-    if (cpu_single_env)
-        cpu_interrupt(cpu_single_env, CPU_INTERRUPT_EXIT);
+    main_loop_break();
 }
 
 void qemu_system_powerdown_request(void)
 {
     powerdown_requested = 1;
-    if (cpu_single_env)
-        cpu_interrupt(cpu_single_env, CPU_INTERRUPT_EXIT);
+    main_loop_break();
 }
 
 #ifdef _WIN32
@@ -3921,25 +3918,6 @@ static void *cpu_main_loop(void *arg)
             }
             cur_cpu = env;
 
-            if (shutdown_requested) {
-                ret = EXCP_INTERRUPT;
-                if (no_shutdown) {
-                    vm_stop(0);
-                    no_shutdown = 0;
-                }
-                else
-                    break;
-            }
-            if (reset_requested) {
-                reset_requested = 0;
-                qemu_system_reset();
-                ret = EXCP_INTERRUPT;
-            }
-            if (powerdown_requested) {
-                powerdown_requested = 0;
-		qemu_system_powerdown();
-                ret = EXCP_INTERRUPT;
-            }
             if (unlikely(ret == EXCP_DEBUG)) {
                 gdb_set_stop_cpu(cur_cpu);
                 vm_stop(EXCP_DEBUG);
@@ -3987,10 +3965,6 @@ static void *cpu_main_loop(void *arg)
                 timeout = 0;
             }
         } else {
-            if (shutdown_requested) {
-                ret = EXCP_INTERRUPT;
-                break;
-            }
             timeout = 5000;
         }
 #ifdef CONFIG_PROFILER
@@ -4017,8 +3991,18 @@ static void main_loop(void)
 
     qemu_thread_create(&cpus_thread, cpu_main_loop, NULL);
 
-    while (1)
+    while (1) {
         main_loop_wait(1000);
+        if (qemu_shutdown_requested()) {
+            if (no_shutdown)
+                no_shutdown = 0;
+            else
+                    break;
+        } else if (qemu_powerdown_requested())
+            qemu_system_powerdown();
+        else if (qemu_reset_requested())
+            qemu_system_reset();
+    }
 }
 
 static void help(int exitcode)

-- 

[Qemu-devel] [patch 5/7] qemu: pause and resume cpu thread(s)

[mtosatti]

[-- Attachment #1: pause-stop-threads --]
[-- Type: text/plain, Size: 5198 bytes --]

Since cpu emulation happens on a separate thread, it is necessary to
pause/resume it upon certain events such as reset, debug exception,
live migration, etc.

Index: qemu/vl.c
===================================================================
--- qemu.orig/vl.c
+++ qemu/vl.c
@@ -271,6 +271,11 @@ QemuMutex qemu_fair_mutex;
 QemuThread io_thread;
 QemuThread cpus_thread;
 
+QemuCond qemu_pause_cond;
+
+static void pause_all_vcpus(void);
+static void resume_all_vcpus(void);
+
 /***********************************************************/
 /* x86 ISA bus support */
 
@@ -3461,6 +3466,7 @@ void vm_start(void)
     if (!vm_running) {
         cpu_enable_ticks();
         vm_running = 1;
+        resume_all_vcpus();
         vm_state_notify(1, 0);
         qemu_rearm_alarm_timer(alarm_timer);
     }
@@ -3471,6 +3477,7 @@ void vm_stop(int reason)
     if (vm_running) {
         cpu_disable_ticks();
         vm_running = 0;
+        pause_all_vcpus();
         vm_state_notify(0, reason);
     }
 }
@@ -3620,7 +3627,9 @@ static int wait_signal(int timeout)
 
 static int has_work(CPUState *env)
 {
-    if (!vm_running)
+    if (env->stop)
+        return 1;
+    if (!vm_running || env->stopped)
         return 0;
     if (!env->halted)
         return 1;
@@ -3644,6 +3653,11 @@ static void qemu_wait_io_event(CPUState 
     qemu_mutex_unlock(&qemu_fair_mutex);
 
     qemu_mutex_lock(&qemu_global_mutex);
+    if (env->stop) {
+        env->stop = 0;
+        env->stopped = 1;
+        qemu_cond_signal(&qemu_pause_cond);
+    }
 }
 
 static void cpu_signal(int sig)
@@ -3848,6 +3862,21 @@ void main_loop_wait(int timeout)
 
 }
 
+static int vm_can_run(CPUState *env)
+{
+    if (env->stop)
+        return 0;
+    if (env->stopped)
+        return 0;
+    if (shutdown_requested)
+        return 0;
+    if (powerdown_requested)
+        return 0;
+    if (reset_requested)
+        return 0;
+    return 1;
+}
+
 static void *cpu_main_loop(void *arg)
 {
     int ret, timeout;
@@ -3887,7 +3916,9 @@ static void *cpu_main_loop(void *arg)
                     env->icount_decr.u16.low = decr;
                     env->icount_extra = count;
                 }
-                ret = cpu_exec(env);
+                ret = EXCP_HALTED;
+                if (vm_can_run(env))
+                    ret = cpu_exec(env);
 #ifdef CONFIG_PROFILER
                 qemu_time += profile_getclock() - ti;
 #endif
@@ -3965,6 +3996,7 @@ static void *cpu_main_loop(void *arg)
                 timeout = 0;
             }
         } else {
+            env = env->next_cpu ?: first_cpu;
             timeout = 5000;
         }
 #ifdef CONFIG_PROFILER
@@ -3979,11 +4011,53 @@ static void *cpu_main_loop(void *arg)
     return NULL;
 }
 
+static int all_vcpus_paused(void)
+{
+    CPUState *penv = first_cpu;
+
+    while (penv) {
+        if (penv->stop)
+            return 0;
+        penv = (CPUState *)penv->next_cpu;
+    }
+
+    return 1;
+}
+
+static void pause_all_vcpus(void)
+{
+    CPUState *penv = first_cpu;
+
+    while (penv) {
+        penv->stop = 1;
+        qemu_thread_signal(&cpus_thread, SIGUSR1);
+        penv = (CPUState *)penv->next_cpu;
+    }
+
+    while (!all_vcpus_paused()) {
+        qemu_cond_timedwait(&qemu_pause_cond, &qemu_global_mutex, 100);
+        qemu_thread_signal(&cpus_thread, SIGUSR1);
+    }
+}
+
+static void resume_all_vcpus(void)
+{
+    CPUState *penv = first_cpu;
+
+    while (penv) {
+        penv->stop = 0;
+        penv->stopped = 0;
+        qemu_thread_signal(&cpus_thread, SIGUSR1);
+        penv = (CPUState *)penv->next_cpu;
+    }
+}
+
 static void main_loop(void)
 {
     qemu_mutex_init(&qemu_fair_mutex);
     qemu_mutex_init(&qemu_global_mutex);
     qemu_mutex_lock(&qemu_global_mutex);
+    qemu_cond_init(&qemu_pause_cond);
 
     qemu_thread_self(&io_thread);
 
@@ -3994,14 +4068,18 @@ static void main_loop(void)
     while (1) {
         main_loop_wait(1000);
         if (qemu_shutdown_requested()) {
+            pause_all_vcpus();
             if (no_shutdown)
                 no_shutdown = 0;
             else
                     break;
         } else if (qemu_powerdown_requested())
             qemu_system_powerdown();
-        else if (qemu_reset_requested())
+        else if (qemu_reset_requested()) {
+            pause_all_vcpus();
             qemu_system_reset();
+            resume_all_vcpus();
+        }
     }
 }
 
Index: qemu/cpu-defs.h
===================================================================
--- qemu.orig/cpu-defs.h
+++ qemu/cpu-defs.h
@@ -169,6 +169,8 @@ typedef struct CPUWatchpoint {
     target_ulong mem_io_vaddr; /* target virtual addr at which the      \
                                      memory was accessed */             \
     uint32_t halted; /* Nonzero if the CPU is in suspend state */       \
+    uint32_t stop;   /* Stop request */                                 \
+    uint32_t stopped; /* Artificially stopped */                        \
     uint32_t interrupt_request;                                         \
     /* The meaning of the MMU modes is defined in the target code. */   \
     CPUTLBEntry tlb_table[NB_MMU_MODES][CPU_TLB_SIZE];                  \

-- 

[Qemu-devel] [patch 6/7] qemu: handle vmstop from cpu context

[mtosatti]

[-- Attachment #1: handle-vmstop-vcpu --]
[-- Type: text/plain, Size: 2412 bytes --]

There are certain cases where cpu context requests a vm stop, such as
-ENOSPC handling.

IMO its simpler to handle vmstop only through the iothread.

Note there is change in behaviour: now the cpu thread which requested vm_stop
will actually only stop when it exits back to cpu_main_loop. It might cause
further damage in between vmstop and cpu_main_loop.

CC: Gleb Natapov <gleb@redhat.com>

Index: qemu/vl.c
===================================================================
--- qemu.orig/vl.c
+++ qemu/vl.c
@@ -3472,7 +3472,22 @@ void vm_start(void)
     }
 }
 
-void vm_stop(int reason)
+static int vmstop_requested;
+
+static int qemu_vmstop_requested(void)
+{
+    int r = vmstop_requested;
+    vmstop_requested = 0;
+    return r;
+}
+
+static void qemu_system_vmstop_request(int reason)
+{
+    vmstop_requested = reason;
+    main_loop_break();
+}
+
+static void __vm_stop(int reason)
 {
     if (vm_running) {
         cpu_disable_ticks();
@@ -3482,6 +3497,21 @@ void vm_stop(int reason)
     }
 }
 
+void vm_stop(int reason)
+{
+    QemuThread me;
+    qemu_thread_self(&me);
+
+    if (!qemu_thread_equal(&me, &io_thread)) {
+        qemu_system_vmstop_request(reason);
+        /* make sure we ourselves can't return to cpu_exec */
+        if (cpu_single_env)
+            cpu_single_env->stop = 1;
+        return;
+    }
+    __vm_stop(reason);
+}
+
 /* reset/shutdown handler */
 
 typedef struct QEMUResetEntry {
@@ -3874,6 +3904,8 @@ static int vm_can_run(CPUState *env)
         return 0;
     if (reset_requested)
         return 0;
+    if (vmstop_requested)
+        return 0;
     return 1;
 }
 
@@ -4054,6 +4086,8 @@ static void resume_all_vcpus(void)
 
 static void main_loop(void)
 {
+    int r;
+
     qemu_mutex_init(&qemu_fair_mutex);
     qemu_mutex_init(&qemu_global_mutex);
     qemu_mutex_lock(&qemu_global_mutex);
@@ -4073,12 +4107,14 @@ static void main_loop(void)
                 no_shutdown = 0;
             else
                     break;
-        } else if (qemu_powerdown_requested())
+        } else if (qemu_powerdown_requested()) {
             qemu_system_powerdown();
-        else if (qemu_reset_requested()) {
+        } else if (qemu_reset_requested()) {
             pause_all_vcpus();
             qemu_system_reset();
             resume_all_vcpus();
+        } else if ((r = qemu_vmstop_requested())) {
+            vm_stop(r);
         }
     }
 }

-- 

[Qemu-devel] [patch 7/7] qemu: use pipe to wakeup io thread

[mtosatti]

[-- Attachment #1: iothread-fd --]
[-- Type: text/plain, Size: 3485 bytes --]

This avoids a wakeup being lost, in case the iothread handles the
signal before select().

Copied and possibly damaged from kvm-userspace.

Index: qemu/vl.c
===================================================================
--- qemu.orig/vl.c
+++ qemu/vl.c
@@ -155,8 +155,6 @@
 //#define DEBUG_NET
 //#define DEBUG_SLIRP
 
-#define SIG_IPI (SIGRTMIN+4)
-
 #ifdef DEBUG_IOPORT
 #  define LOG_IOPORT(...) qemu_log_mask(CPU_LOG_IOPORT, ## __VA_ARGS__)
 #else
@@ -273,6 +271,8 @@ QemuThread cpus_thread;
 
 QemuCond qemu_pause_cond;
 
+static int io_thread_fd = -1;
+
 static void pause_all_vcpus(void);
 static void resume_all_vcpus(void);
 
@@ -3723,7 +3723,6 @@ static void block_io_signals(void)
     sigaddset(&set, SIGUSR2);
     sigaddset(&set, SIGIO);
     sigaddset(&set, SIGALRM);
-    sigaddset(&set, SIG_IPI);
     pthread_sigmask(SIG_BLOCK, &set, NULL);
 
     sigemptyset(&set);
@@ -3735,30 +3734,19 @@ static void block_io_signals(void)
     sigaction(SIGUSR1, &sigact, NULL);
 }
 
-/* used to wake up the io thread */
-static void sig_ipi_handler(int sig)
-{
-}
-
 static void unblock_io_signals(void)
 {
     sigset_t set;
-    struct sigaction sigact;
 
     sigemptyset(&set);
     sigaddset(&set, SIGUSR2);
     sigaddset(&set, SIGIO);
     sigaddset(&set, SIGALRM);
-    sigaddset(&set, SIG_IPI);
     pthread_sigmask(SIG_UNBLOCK, &set, NULL);
 
     sigemptyset(&set);
     sigaddset(&set, SIGUSR1);
     pthread_sigmask(SIG_BLOCK, &set, NULL);
-
-    memset(&sigact, 0, sizeof(sigact));
-    sigact.sa_handler = sig_ipi_handler;
-    sigaction(SIG_IPI, &sigact, NULL);
 }
 
 int qemu_notify_event(void *cpu_env)
@@ -3774,11 +3762,51 @@ int qemu_notify_event(void *cpu_env)
 
 void main_loop_break(void)
 {
-    QemuThread me;
-    qemu_thread_self(&me);
-    if (qemu_thread_equal(&io_thread, &me))
+    uint64_t value = 1;
+    char buffer[8];
+    size_t offset = 0;
+
+    if (io_thread_fd == -1)
         return;
-    qemu_thread_signal(&io_thread, SIG_IPI);
+
+    memcpy(buffer, &value, sizeof(value));
+
+    while (offset < 8) {
+        ssize_t len;
+
+        len = write(io_thread_fd, buffer + offset, 8 - offset);
+        if (len == -1 && errno == EINTR)
+            continue;
+
+        if (len <= 0)
+            break;
+
+        offset += len;
+    }
+
+    if (offset != 8)
+        fprintf(stderr, "failed to notify io thread\n");
+}
+
+/* Used to break IO thread out of select */
+static void io_thread_wakeup(void *opaque)
+{
+    int fd = (unsigned long)opaque;
+    char buffer[8];
+    size_t offset = 0;
+
+    while (offset < 8) {
+        ssize_t len;
+
+        len = read(fd, buffer + offset, 8 - offset);
+        if (len == -1 && errno == EINTR)
+            continue;
+
+        if (len <= 0)
+            break;
+
+        offset += len;
+    }
 }
 
 static void qemu_signal_lock(unsigned int msecs)
@@ -4086,6 +4114,20 @@ static void resume_all_vcpus(void)
     }
 }
 
+static void setup_iothread_fd(void)
+{
+    int fds[2];
+
+    if (pipe(fds) == -1) {
+        fprintf(stderr, "failed to create iothread pipe");
+        exit(0);
+    }
+
+    qemu_set_fd_handler2(fds[0], NULL, io_thread_wakeup, NULL,
+                         (void *)(unsigned long)fds[0]);
+    io_thread_fd = fds[1];
+}
+
 static void main_loop(void)
 {
     int r;
@@ -4096,6 +4138,7 @@ static void main_loop(void)
     qemu_cond_init(&qemu_pause_cond);
 
     qemu_thread_self(&io_thread);
+    setup_iothread_fd();
 
     unblock_io_signals();
 

-- 

Re: [Qemu-devel] [patch 1/7] qemu: mutex/thread/cond wrappers

[Avi Kivity]

mtosatti@redhat.com wrote:
> --- qemu.orig/vl.c
> +++ qemu/vl.c
> @@ -36,6 +36,7 @@
>  #include "gdbstub.h"
>  #include "qemu-timer.h"
>  #include "qemu-char.h"
> +#include "qemu-thread.h"
>  #include "cache-utils.h"
>  #include "block.h"
>  #include "audio/audio.h"
> @@ -263,6 +264,8 @@ static QEMUTimer *nographic_timer;
>  
>  uint8_t qemu_uuid[16];
>  
> +QemuMutex qemu_global_mutex;
> +
>  /***********************************************************/
>  /* x86 ISA bus support */
>  
> @@ -3650,7 +3653,14 @@ void main_loop_wait(int timeout)
>          slirp_select_fill(&nfds, &rfds, &wfds, &xfds);
>      }
>  #endif
> +
> +    /*
> +     * main_loop_wait() *must* not assume any global state is consistent across
> +     * select() invocations.
> +     */
> +    qemu_mutex_unlock(&qemu_global_mutex);
>      ret = select(nfds + 1, &rfds, &wfds, &xfds, &tv);
> +    qemu_mutex_lock(&qemu_global_mutex);
>      if (ret > 0) {
>          IOHandlerRecord **pioh;
>  
> @@ -3708,6 +3718,9 @@ static int main_loop(void)
>  #endif
>      CPUState *env;
>  
> +    qemu_mutex_init(&qemu_global_mutex);
> +    qemu_mutex_lock(&qemu_global_mutex);
> +
>      cur_cpu = first_cpu;
>      next_cpu = cur_cpu->next_cpu ?: first_cpu;
>      for(;;) {
>   

All the bits above seem unnecessary for the this patch.


> +
> +int qemu_mutex_init(QemuMutex *mutex)
> +{
> +    return pthread_mutex_init(&mutex->lock, NULL);
> +}
>   

No one will check the return code, so better to check it here and 
abort() if it's bad.

> +static void add_to_timespec(struct timespec *ts, unsigned int msecs)
> +{
> +    ts->tv_sec = ts->tv_sec + (long)(msecs / 1000);
> +    ts->tv_nsec = (ts->tv_nsec + ((long)msecs % 1000) * 1000000);
> +    if (ts->tv_nsec >= 1000000000) {
> +        ts->tv_nsec -= 1000000000;
> +        ts->tv_sec++;
> +    }
> +}
>   

timespec_add_msec()?  and why milliseconds?  Also, maybe uint64_t is a 
better type.

> +
> +int qemu_mutex_timedlock(QemuMutex *mutex, unsigned int msecs)
> +{
> +    struct timespec ts;
> +
> +    clock_gettime(CLOCK_REALTIME, &ts);
> +    add_to_timespec(&ts, msecs);
> +
> +    return pthread_mutex_timedlock(&mutex->lock, &ts);
> +}
>   

I would have preferred a deadline instead of a timeout, but we'll see on 
the next patches.

> +
> +int qemu_thread_create(QemuThread *thread,
> +                       void *(*start_routine)(void*),
> +                       void *arg)
> +{
> +    return pthread_create(&thread->thread, NULL, start_routine, arg);
> +}
>   

Don't return an error that no one will check.

> +
> +int qemu_thread_signal(QemuThread *thread, int sig)
> +{
> +    if (thread->thread != 0)
> +        return pthread_kill(thread->thread, sig);
> +    return -1; /* XXX: ESCHR */
> +}
>   

Ditto.  If the thread dies, qemu dies.

> +int qemu_thread_equal(QemuThread *thread1, QemuThread *thread2)
> +{
> +   return (thread1->thread == thread2->thread);
> +}
>   

Can compare thing1 to thing2 instead of ->thread.  Not that it matters.


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [patch 1/7] qemu: mutex/thread/cond wrappers

[Jamie Lokier]

Avi Kivity wrote:
> >+int qemu_mutex_init(QemuMutex *mutex)
> >+{
> >+    return pthread_mutex_init(&mutex->lock, NULL);
> >+}
> 
> No one will check the return code, so better to check it here and 
> abort() if it's bad.

I agree.

Same applies to pthread_mutex_lock, etc., all of which return an error
code, can fail if misused, and nobody checks.

-- Jamie

[Qemu-devel] Re: [patch 2/7] qemu: separate thread for io

[Anthony Liguori]

mtosatti@redhat.com wrote:
> Index: qemu/exec.c
> ===================================================================
> --- qemu.orig/exec.c
> +++ qemu/exec.c
> @@ -1513,6 +1513,20 @@ void cpu_interrupt(CPUState *env, int ma
>      /* FIXME: This is probably not threadsafe.  A different thread could
>         be in the middle of a read-modify-write operation.  */
>      env->interrupt_request |= mask;
> +
> +    switch(mask) {
> +    case CPU_INTERRUPT_HARD:
> +    case CPU_INTERRUPT_SMI:
> +    case CPU_INTERRUPT_NMI:
> +    case CPU_INTERRUPT_EXIT:
> +    /*
> +     * only unlink the TB's if we're called from cpu thread context,
> +     * otherwise signal cpu thread to do it.
> +     */
> +        if (qemu_notify_event(env))
> +               return;
> +    }
> +
>  #if defined(USE_NPTL)
>      /* FIXME: TB unchaining isn't SMP safe.  For now just ignore the
>         problem and hope the cpu will stop of its own accord.  For userspace
> Index: qemu/qemu-common.h
> ===================================================================
> --- qemu.orig/qemu-common.h
> +++ qemu/qemu-common.h
> @@ -190,6 +190,8 @@ int cpu_load(QEMUFile *f, void *opaque, 
>  
>  /* Force QEMU to stop what it's doing and service IO */
>  void qemu_service_io(void);
> +void main_loop_break(void);
> +int qemu_notify_event(void *env);
>  
>  typedef struct QEMUIOVector {
>      struct iovec *iov;
> Index: qemu/vl.c
> ===================================================================
> --- qemu.orig/vl.c
> +++ qemu/vl.c
> @@ -155,6 +155,7 @@
>  //#define DEBUG_NET
>  //#define DEBUG_SLIRP
>  
> +#define SIG_IPI (SIGRTMIN+4)
>  
>  #ifdef DEBUG_IOPORT
>  #  define LOG_IOPORT(...) qemu_log_mask(CPU_LOG_IOPORT, ## __VA_ARGS__)
> @@ -265,6 +266,10 @@ static QEMUTimer *nographic_timer;
>  uint8_t qemu_uuid[16];
>  
>  QemuMutex qemu_global_mutex;
> +QemuMutex qemu_fair_mutex;
> +
> +QemuThread io_thread;
> +QemuThread cpus_thread;
>  
>  /***********************************************************/
>  /* x86 ISA bus support */
> @@ -1328,7 +1333,6 @@ static void host_alarm_handler(int host_
>                                 qemu_get_clock(vm_clock))) ||
>          qemu_timer_expired(active_timers[QEMU_TIMER_REALTIME],
>                             qemu_get_clock(rt_clock))) {
> -        CPUState *env = next_cpu;
>  
>  #ifdef _WIN32
>          struct qemu_alarm_win32 *data = ((struct qemu_alarm_timer*)dwUser)->priv;
> @@ -1339,15 +1343,6 @@ static void host_alarm_handler(int host_
>  #endif
>          alarm_timer->flags |= ALARM_FLAG_EXPIRED;
>  
> -        if (env) {
> -            /* stop the currently executing cpu because a timer occured */
> -            cpu_interrupt(env, CPU_INTERRUPT_EXIT);
> -#ifdef USE_KQEMU
> -            if (env->kqemu_enabled) {
> -                kqemu_cpu_interrupt(env);
> -            }
> -#endif
> -        }
>          event_pending = 1;
>      }
>  }
> @@ -2878,6 +2873,7 @@ int qemu_set_fd_handler2(int fd,
>          ioh->opaque = opaque;
>          ioh->deleted = 0;
>      }
> +    main_loop_break();
>      return 0;
>  }
>  
> @@ -3334,6 +3330,7 @@ void qemu_bh_schedule(QEMUBH *bh)
>      if (env) {
>          cpu_interrupt(env, CPU_INTERRUPT_EXIT);
>      }
> +    main_loop_break();
>  }
>  
>  void qemu_bh_cancel(QEMUBH *bh)
> @@ -3611,6 +3608,155 @@ static void host_main_loop_wait(int *tim
>  }
>  #endif
>  
> +static int wait_signal(int timeout)
> +{
> +    struct timespec ts;
> +    sigset_t waitset;
> +
> +    ts.tv_sec = timeout / 1000;
> +    ts.tv_nsec = (timeout % 1000) * 1000000;
> +    sigemptyset(&waitset);
> +    sigaddset(&waitset, SIGUSR1);
> +
> +    return sigtimedwait(&waitset, NULL, &ts);
> +}
> +
> +static int has_work(CPUState *env)
> +{
> +    int r = 0;
> +    if (!env->halted)
> +        r = 1;
> +    if (env->interrupt_request & (CPU_INTERRUPT_HARD | CPU_INTERRUPT_NMI))
> +        r = 1;
> +    return r;
> +}
> +
> +static void qemu_wait_io_event(CPUState *env, int timeout)
> +{
> +    qemu_mutex_unlock(&qemu_global_mutex);
> +
> +    if (timeout && !has_work(env))
> +        wait_signal(timeout);
> +    /*
> +     * Users of qemu_global_mutex can be starved, having no chance
> +     * to acquire it since this path will get to it first.
> +     * So use another lock to provide fairness.
> +     */
> +    qemu_mutex_lock(&qemu_fair_mutex);
> +    qemu_mutex_unlock(&qemu_fair_mutex);
>   

This is extremely subtle.  I think it can be made a bit more explicit 
via something like:

int generation = qemu_io_generation() + 1;

qemu_mutex_unlock(&qemu_global_mutex);

if (timeout && !has_work(env))
     wait_signal(timeout);

qemu_mutex_lock(&qemu_global_mutex)

while (qemu_io_generation() < generation)
    qemu_cond_wait(&qemu_io_generation_cond, &qemu_global_mutex);

Then, in main_loop_wait():

>  void main_loop_wait(int timeout)
>  {
>      IOHandlerRecord *ioh;
> @@ -3660,7 +3806,7 @@ void main_loop_wait(int timeout)
>       */
>      qemu_mutex_unlock(&qemu_global_mutex);
>      ret = select(nfds + 1, &rfds, &wfds, &xfds, &tv);
> -    qemu_mutex_lock(&qemu_global_mutex);
>   

qemu_mutex_lock(&qemu_global_mutex);
qemu_io_generation_add();
qemu_cond_signal(&qemu_io_generation_cond);

This will ensure that the TCG loop backs off of qemu_global_mutex for at 
least one main_loop_wait() iteration.

Regards,

Anthony Liguori

[Qemu-devel] Re: [patch 2/7] qemu: separate thread for io

[Marcelo Tosatti]

On Fri, Mar 20, 2009 at 12:43:49PM -0500, Anthony Liguori wrote:
>> +    qemu_mutex_lock(&qemu_fair_mutex);
>> +    qemu_mutex_unlock(&qemu_fair_mutex);
>>   
>
> This is extremely subtle.  I think it can be made a bit more explicit  
> via something like:
>
> int generation = qemu_io_generation() + 1;
>
> qemu_mutex_unlock(&qemu_global_mutex);
>
> if (timeout && !has_work(env))
>     wait_signal(timeout);
>
> qemu_mutex_lock(&qemu_global_mutex)
>
> while (qemu_io_generation() < generation)
>    qemu_cond_wait(&qemu_io_generation_cond, &qemu_global_mutex);
>
> Then, in main_loop_wait():
>
>>  void main_loop_wait(int timeout)
>>  {
>>      IOHandlerRecord *ioh;
>> @@ -3660,7 +3806,7 @@ void main_loop_wait(int timeout)
>>       */
>>      qemu_mutex_unlock(&qemu_global_mutex);
>>      ret = select(nfds + 1, &rfds, &wfds, &xfds, &tv);
>> -    qemu_mutex_lock(&qemu_global_mutex);
>>   
>
> qemu_mutex_lock(&qemu_global_mutex);
> qemu_io_generation_add();
> qemu_cond_signal(&qemu_io_generation_cond);
>
> This will ensure that the TCG loop backs off of qemu_global_mutex for at  
> least one main_loop_wait() iteration.

I don't see much benefit. It has the disadvantage of introducing more
state (the generation counter, the conditional), and the potential
problems associated with this state such as:

- If there is no event for the iothread to process, TCG will throttle
  unnecessarily (i can't see how that could happen, but is a
  possibility) until some event breaks it out of select() thus
  increasing the generation counter.

- Its possible to miss signals (again, I can't see in happening 
  in the scheme you suggest), but..

Also note there is no need to be completly fair. It is fine to
eventually be unfair.

Do you worry about interaction between the two locks? Can make the lock
ordering documented.

Will spend some time thinking about this, need to take multiple
"starved" threads into account.

Thanks.

[Qemu-devel] Re: [patch 2/7] qemu: separate thread for io

[Anthony Liguori]

Marcelo Tosatti wrote:
> On Fri, Mar 20, 2009 at 12:43:49PM -0500, Anthony Liguori wrote:
>   
>>> +    qemu_mutex_lock(&qemu_fair_mutex);
>>> +    qemu_mutex_unlock(&qemu_fair_mutex);
>>>   
>>>       
>> This is extremely subtle.  I think it can be made a bit more explicit  
>> via something like:
>>
>> int generation = qemu_io_generation() + 1;
>>
>> qemu_mutex_unlock(&qemu_global_mutex);
>>
>> if (timeout && !has_work(env))
>>     wait_signal(timeout);
>>
>> qemu_mutex_lock(&qemu_global_mutex)
>>
>> while (qemu_io_generation() < generation)
>>    qemu_cond_wait(&qemu_io_generation_cond, &qemu_global_mutex);
>>
>> Then, in main_loop_wait():
>>
>>     
>>>  void main_loop_wait(int timeout)
>>>  {
>>>      IOHandlerRecord *ioh;
>>> @@ -3660,7 +3806,7 @@ void main_loop_wait(int timeout)
>>>       */
>>>      qemu_mutex_unlock(&qemu_global_mutex);
>>>      ret = select(nfds + 1, &rfds, &wfds, &xfds, &tv);
>>> -    qemu_mutex_lock(&qemu_global_mutex);
>>>   
>>>       
>> qemu_mutex_lock(&qemu_global_mutex);
>> qemu_io_generation_add();
>> qemu_cond_signal(&qemu_io_generation_cond);
>>
>> This will ensure that the TCG loop backs off of qemu_global_mutex for at  
>> least one main_loop_wait() iteration.
>>     
>
> I don't see much benefit. It has the disadvantage of introducing more
> state (the generation counter, the conditional), and the potential
> problems associated with this state such as:
>
> - If there is no event for the iothread to process, TCG will throttle
>   unnecessarily (i can't see how that could happen, but is a
>   possibility) until some event breaks it out of select() thus
>   increasing the generation counter.
>   

Right, you have to couple this with a signal sent from the TCG thread to 
the io thread.  And yeah, signals are not 100% reliable.

> - Its possible to miss signals (again, I can't see in happening 
>   in the scheme you suggest), but..
>
> Also note there is no need to be completly fair. It is fine to
> eventually be unfair.
>
> Do you worry about interaction between the two locks? Can make the lock
> ordering documented.
>   

I need to think about this a bit more.  I understand the idea behind 
qemu_signal_lock() a little better now.

Since we know that the IO thread is trying to run in TCG (because we 
sent a signal to it), I wonder if we can use that as an indicator that 
we have to let the IO thread run for a bit.

BTW, your patches lack commit messages and Signed-off-bys.  Are you not 
ready for having them committed?  I know that we still have to figure 
out Windows support but do you know of any other show stoppers?

Have you thought about how this is going to affect kvm-userspace?  Do 
you think we can eliminate the io threading code in kvm-userspace after 
this goes in?

Regards,

Anthony Liguori

> Will spend some time thinking about this, need to take multiple
> "starved" threads into account.
>
> Thanks.
>
>   

[Qemu-devel] Re: [patch 2/7] qemu: separate thread for io

[Marcelo Tosatti]

On Fri, Mar 20, 2009 at 08:04:41PM -0500, Anthony Liguori wrote:
>> - If there is no event for the iothread to process, TCG will throttle
>>   unnecessarily (i can't see how that could happen, but is a
>>   possibility) until some event breaks it out of select() thus
>>   increasing the generation counter.
>>   
>
> Right, you have to couple this with a signal sent from the TCG thread to  
> the io thread.  And yeah, signals are not 100% reliable.
>
>> - Its possible to miss signals (again, I can't see in happening   in 
>> the scheme you suggest), but..
>>
>> Also note there is no need to be completly fair. It is fine to
>> eventually be unfair.
>>
>> Do you worry about interaction between the two locks? Can make the lock
>> ordering documented.
>>   
>
> I need to think about this a bit more.  I understand the idea behind  
> qemu_signal_lock() a little better now.
>
> Since we know that the IO thread is trying to run in TCG (because we  
> sent a signal to it), I wonder if we can use that as an indicator that  
> we have to let the IO thread run for a bit.
>
> BTW, your patches lack commit messages and Signed-off-bys.  Are you not  
> ready for having them committed?  I know that we still have to figure  
> out Windows support but do you know of any other show stoppers?

There was a significant (25% IIRC) reduction in iperf performance. This
is sort of expected, since there are no optimizations at all (should
collapse the signals sent to TCG context, for one). But my thinking is
to merge the iothread (along the lines of this patchset), stabilize and
then optimize.

How about that?

> Have you thought about how this is going to affect kvm-userspace?  

Oops, no. But I can be held accountable for kvm-userspace iothread until
it can be fully replaced by upstream.

> Do you think we can eliminate the io threading code in kvm-userspace
> after this goes in?

Not immediately, need to generalize some of the changes introduced
by the patchset and merge the remaining logic of kvm-userspace's
qemu-kvm.c.

[Qemu-devel] Re: [patch 2/7] qemu: separate thread for io

[Anthony Liguori]

Marcelo Tosatti wrote:
> There was a significant (25% IIRC) reduction in iperf performance. This
> is sort of expected, since there are no optimizations at all (should
> collapse the signals sent to TCG context, for one). But my thinking is
> to merge the iothread (along the lines of this patchset), stabilize and
> then optimize.
>
> How about that?
>   

I don't mind that but I'll need to check out how things behave with TCG 
and with other guest architectures.  I'm willing to accept a temporary 
performance regression in something like iperf because I don't think 
people are relying on QEMU network performance that much today (outside 
of KVM/Xen) but if we have significant performance regressions with 
non-x86 boards with things like basic boot time, we'll have to fix those 
first.

We also need to not completely break the Windows build before merging.

>> Have you thought about how this is going to affect kvm-userspace?  
>>     
>
> Oops, no. But I can be held accountable for kvm-userspace iothread until
> it can be fully replaced by upstream.
>   

Before merging into QEMU, we should at least make sure that it's not 
going to create a nightmare for Avi :-)

>> Do you think we can eliminate the io threading code in kvm-userspace
>> after this goes in?
>>     
>
> Not immediately, need to generalize some of the changes introduced
> by the patchset and merge the remaining logic of kvm-userspace's
> qemu-kvm.c.
>   

Yeah, I thought it would take some work.  Great work Marcelo!

Regards,

Anthony Liguori

[Qemu-devel] Re: [patch 2/7] qemu: separate thread for io

[Avi Kivity]

Anthony Liguori wrote:
> Before merging into QEMU, we should at least make sure that it's not 
> going to create a nightmare for Avi :-)

While it's likely going to be bad, we shouldn't make too much effort in 
making the merge easy.  We should aim to get the cleanest implementation 
in qemu, and then rework kvm-userspace to match that.  That's the whole 
point of the merge, no?

Of course, if it happens to be easy I won't complain too much.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [patch 2/7] qemu: separate thread for io

[Anthony Liguori]

Avi Kivity wrote:
> Anthony Liguori wrote:
>> Before merging into QEMU, we should at least make sure that it's not 
>> going to create a nightmare for Avi :-)
>
> While it's likely going to be bad, we shouldn't make too much effort 
> in making the merge easy.  We should aim to get the cleanest 
> implementation in qemu, and then rework kvm-userspace to match that.  
> That's the whole point of the merge, no?

The effort to make merging easy should be in kvm-userspace, not in 
qemu.  Thinking through how it would integrate with kvm-userspace is a 
good exercise in determining whether all of the requirements are being 
met for KVM too.

Regards,

Anthony Liguori

> Of course, if it happens to be easy I won't complain too much.
>

Re: [Qemu-devel] [patch 1/7] qemu: mutex/thread/cond wrappers

[Marcelo Tosatti]

On Thu, Mar 19, 2009 at 05:59:37PM +0200, Avi Kivity wrote:
>> +static void add_to_timespec(struct timespec *ts, unsigned int msecs)
>> +{
>> +    ts->tv_sec = ts->tv_sec + (long)(msecs / 1000);
>> +    ts->tv_nsec = (ts->tv_nsec + ((long)msecs % 1000) * 1000000);
>> +    if (ts->tv_nsec >= 1000000000) {
>> +        ts->tv_nsec -= 1000000000;
>> +        ts->tv_sec++;
>> +    }
>> +}
>>   
>
> timespec_add_msec()?  

> and why milliseconds?  

Well, its suitable for the current purposes. Any other suggestion?

> Also, maybe uint64_t is a  better type.

>> +
>> +int qemu_mutex_timedlock(QemuMutex *mutex, unsigned int msecs)
>> +{
>> +    struct timespec ts;
>> +
>> +    clock_gettime(CLOCK_REALTIME, &ts);
>> +    add_to_timespec(&ts, msecs);
>> +
>> +    return pthread_mutex_timedlock(&mutex->lock, &ts);
>> +}
>>   
>
> I would have preferred a deadline instead of a timeout, but we'll see on  
> the next patches.

Please expand?

Fixed the other comments, thanks.

Re: [Qemu-devel] [patch 1/7] qemu: mutex/thread/cond wrappers

[Avi Kivity]

Marcelo Tosatti wrote:
>> and why milliseconds?  
>>     
>
> Well, its suitable for the current purposes. Any other suggestion?
>
>   

uint64_t nanoseconds.  It's standard timekeeping in qemu:

   #define QEMU_TIMER_BASE 1000000000LL

and it's of course best to limit the number of standards.


>> Also, maybe uint64_t is a  better type.
>>     
>
>   
>>> +
>>> +int qemu_mutex_timedlock(QemuMutex *mutex, unsigned int msecs)
>>> +{
>>> +    struct timespec ts;
>>> +
>>> +    clock_gettime(CLOCK_REALTIME, &ts);
>>> +    add_to_timespec(&ts, msecs);
>>> +
>>> +    return pthread_mutex_timedlock(&mutex->lock, &ts);
>>> +}
>>>   
>>>       
>> I would have preferred a deadline instead of a timeout, but we'll see on  
>> the next patches.
>>     
>
> Please expand?
>   

Instead of 'sleep until n nanoseconds have elapsed', 'sleep until 
get_clock() >= deadline'.  I think it simplifies things somewhat.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.