[Qemu-devel] [PATCH 0/5] Spread the use of QEMU threading & locking API

[Qemu-devel] [PATCH 0/5] Spread the use of QEMU threading & locking API

[Jan Kiszka]

Refreshed versions of some cleanups I already sent last year. See
patches for details.

The series also helps using different scheduling policies for QEMU
threads which includes hardening internal locks.

Jan Kiszka (5):
  Introduce qemu_cond_timedwait for POSIX
  Switch POSIX compat AIO to QEMU abstractions
  Use qemu_eventfd for POSIX AIO
  Reorder POSIX compat AIO code
  Switch compatfd to QEMU thread

 compatfd.c          |   16 +----
 posix-aio-compat.c  |  162 +++++++++++++++++----------------------------------
 qemu-thread-posix.c |   23 +++++++
 qemu-thread-posix.h |    5 ++
 4 files changed, 84 insertions(+), 122 deletions(-)

-- 
1.7.3.4

[Qemu-devel] [PATCH 1/5] Introduce qemu_cond_timedwait for POSIX

[Jan Kiszka]

First user will be POSIX compat aio. Windows use cases aren't in sight,
so this remains a POSIX-only service for now.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 qemu-thread-posix.c |   23 +++++++++++++++++++++++
 qemu-thread-posix.h |    5 +++++
 2 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/qemu-thread-posix.c b/qemu-thread-posix.c
index 9e1b5fb..cd65df2 100644
--- a/qemu-thread-posix.c
+++ b/qemu-thread-posix.c
@@ -17,6 +17,7 @@
 #include <signal.h>
 #include <stdint.h>
 #include <string.h>
+#include <sys/time.h>
 #include "qemu-thread.h"
 
 static void error_exit(int err, const char *msg)
@@ -115,6 +116,28 @@ void qemu_cond_wait(QemuCond *cond, QemuMutex *mutex)
         error_exit(err, __func__);
 }
 
+/* Returns true if condition was signals, false if timed out. */
+bool qemu_cond_timedwait(QemuCond *cond, QemuMutex *mutex,
+                         unsigned int timeout_ms)
+{
+    struct timespec ts;
+    struct timeval tv;
+    int err;
+
+    gettimeofday(&tv, NULL);
+    ts.tv_sec = tv.tv_sec + timeout_ms / 1000;
+    ts.tv_nsec = tv.tv_usec * 1000 + timeout_ms % 1000;
+    if (ts.tv_nsec > 1000000000) {
+        ts.tv_sec++;
+        ts.tv_nsec -= 1000000000;
+    }
+    err = pthread_cond_timedwait(&cond->cond, &mutex->lock, &ts);
+    if (err && err != ETIMEDOUT) {
+        error_exit(err, __func__);
+    }
+    return err == 0;
+}
+
 void qemu_thread_create(QemuThread *thread,
                        void *(*start_routine)(void*),
                        void *arg, int mode)
diff --git a/qemu-thread-posix.h b/qemu-thread-posix.h
index ee4618e..9f00524 100644
--- a/qemu-thread-posix.h
+++ b/qemu-thread-posix.h
@@ -1,5 +1,6 @@
 #ifndef __QEMU_THREAD_POSIX_H
 #define __QEMU_THREAD_POSIX_H 1
+#include <stdbool.h>
 #include "pthread.h"
 
 struct QemuMutex {
@@ -14,4 +15,8 @@ struct QemuThread {
     pthread_t thread;
 };
 
+/* only provided for posix so far */
+bool qemu_cond_timedwait(QemuCond *cond, QemuMutex *mutex,
+                         unsigned int timeout_ms);
+
 #endif
-- 
1.7.3.4

[Qemu-devel] [PATCH 2/5] Switch POSIX compat AIO to QEMU abstractions

[Jan Kiszka]

Although there is nothing to wrap for non-POSIX here, redirecting thread
and synchronization services to our core simplifies managements jobs
like scheduling parameter adjustment. It also frees compat AIO from some
duplicate code (/wrt qemu-thread).

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 posix-aio-compat.c |  118 +++++++++++++++------------------------------------
 1 files changed, 35 insertions(+), 83 deletions(-)

diff --git a/posix-aio-compat.c b/posix-aio-compat.c
index d311d13..c9b8ebf 100644
--- a/posix-aio-compat.c
+++ b/posix-aio-compat.c
@@ -15,7 +15,6 @@
 
 #include <sys/ioctl.h>
 #include <sys/types.h>
-#include <pthread.h>
 #include <unistd.h>
 #include <errno.h>
 #include <time.h>
@@ -29,9 +28,12 @@
 #include "qemu-common.h"
 #include "trace.h"
 #include "block_int.h"
+#include "qemu-thread.h"
 
 #include "block/raw-posix-aio.h"
 
+#define AIO_THREAD_IDLE_TIMEOUT 10000 /* 10 s */
+
 static void do_spawn_thread(void);
 
 struct qemu_paiocb {
@@ -59,10 +61,9 @@ typedef struct PosixAioState {
 } PosixAioState;
 
 
-static pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER;
-static pthread_cond_t cond = PTHREAD_COND_INITIALIZER;
-static pthread_t thread_id;
-static pthread_attr_t attr;
+static QemuMutex lock;
+static QemuCond cond;
+static QemuThread thread;
 static int max_threads = 64;
 static int cur_threads = 0;
 static int idle_threads = 0;
@@ -88,39 +89,6 @@ static void die(const char *what)
     die2(errno, what);
 }
 
-static void mutex_lock(pthread_mutex_t *mutex)
-{
-    int ret = pthread_mutex_lock(mutex);
-    if (ret) die2(ret, "pthread_mutex_lock");
-}
-
-static void mutex_unlock(pthread_mutex_t *mutex)
-{
-    int ret = pthread_mutex_unlock(mutex);
-    if (ret) die2(ret, "pthread_mutex_unlock");
-}
-
-static int cond_timedwait(pthread_cond_t *cond, pthread_mutex_t *mutex,
-                           struct timespec *ts)
-{
-    int ret = pthread_cond_timedwait(cond, mutex, ts);
-    if (ret && ret != ETIMEDOUT) die2(ret, "pthread_cond_timedwait");
-    return ret;
-}
-
-static void cond_signal(pthread_cond_t *cond)
-{
-    int ret = pthread_cond_signal(cond);
-    if (ret) die2(ret, "pthread_cond_signal");
-}
-
-static void thread_create(pthread_t *thread, pthread_attr_t *attr,
-                          void *(*start_routine)(void*), void *arg)
-{
-    int ret = pthread_create(thread, attr, start_routine, arg);
-    if (ret) die2(ret, "pthread_create");
-}
-
 static ssize_t handle_aiocb_ioctl(struct qemu_paiocb *aiocb)
 {
     int ret;
@@ -313,28 +281,26 @@ static void posix_aio_notify_event(void);
 
 static void *aio_thread(void *unused)
 {
-    mutex_lock(&lock);
+    qemu_mutex_lock(&lock);
     pending_threads--;
-    mutex_unlock(&lock);
+    qemu_mutex_unlock(&lock);
     do_spawn_thread();
 
     while (1) {
         struct qemu_paiocb *aiocb;
-        ssize_t ret = 0;
-        qemu_timeval tv;
-        struct timespec ts;
-
-        qemu_gettimeofday(&tv);
-        ts.tv_sec = tv.tv_sec + 10;
-        ts.tv_nsec = 0;
+        bool timed_out;
+        ssize_t ret;
 
-        mutex_lock(&lock);
+        qemu_mutex_lock(&lock);
 
-        while (QTAILQ_EMPTY(&request_list) &&
-               !(ret == ETIMEDOUT)) {
+        while (QTAILQ_EMPTY(&request_list)) {
             idle_threads++;
-            ret = cond_timedwait(&cond, &lock, &ts);
+            timed_out = !qemu_cond_timedwait(&cond, &lock,
+                                             AIO_THREAD_IDLE_TIMEOUT);
             idle_threads--;
+            if (timed_out) {
+                break;
+            }
         }
 
         if (QTAILQ_EMPTY(&request_list))
@@ -343,7 +309,7 @@ static void *aio_thread(void *unused)
         aiocb = QTAILQ_FIRST(&request_list);
         QTAILQ_REMOVE(&request_list, aiocb, node);
         aiocb->active = 1;
-        mutex_unlock(&lock);
+        qemu_mutex_unlock(&lock);
 
         switch (aiocb->aio_type & QEMU_AIO_TYPE_MASK) {
         case QEMU_AIO_READ:
@@ -375,41 +341,33 @@ static void *aio_thread(void *unused)
             break;
         }
 
-        mutex_lock(&lock);
+        qemu_mutex_lock(&lock);
         aiocb->ret = ret;
-        mutex_unlock(&lock);
+        qemu_mutex_unlock(&lock);
 
         posix_aio_notify_event();
     }
 
     cur_threads--;
-    mutex_unlock(&lock);
+    qemu_mutex_unlock(&lock);
 
     return NULL;
 }
 
 static void do_spawn_thread(void)
 {
-    sigset_t set, oldset;
-
-    mutex_lock(&lock);
+    qemu_mutex_lock(&lock);
     if (!new_threads) {
-        mutex_unlock(&lock);
+        qemu_mutex_unlock(&lock);
         return;
     }
 
     new_threads--;
     pending_threads++;
 
-    mutex_unlock(&lock);
+    qemu_mutex_unlock(&lock);
 
-    /* block all signals */
-    if (sigfillset(&set)) die("sigfillset");
-    if (sigprocmask(SIG_SETMASK, &set, &oldset)) die("sigprocmask");
-
-    thread_create(&thread_id, &attr, aio_thread, NULL);
-
-    if (sigprocmask(SIG_SETMASK, &oldset, NULL)) die("sigprocmask restore");
+    qemu_thread_create(&thread, aio_thread, NULL, QEMU_THREAD_DETACHED);
 }
 
 static void spawn_thread_bh_fn(void *opaque)
@@ -437,21 +395,21 @@ static void qemu_paio_submit(struct qemu_paiocb *aiocb)
 {
     aiocb->ret = -EINPROGRESS;
     aiocb->active = 0;
-    mutex_lock(&lock);
+    qemu_mutex_lock(&lock);
     if (idle_threads == 0 && cur_threads < max_threads)
         spawn_thread();
     QTAILQ_INSERT_TAIL(&request_list, aiocb, node);
-    mutex_unlock(&lock);
-    cond_signal(&cond);
+    qemu_mutex_unlock(&lock);
+    qemu_cond_signal(&cond);
 }
 
 static ssize_t qemu_paio_return(struct qemu_paiocb *aiocb)
 {
     ssize_t ret;
 
-    mutex_lock(&lock);
+    qemu_mutex_lock(&lock);
     ret = aiocb->ret;
-    mutex_unlock(&lock);
+    qemu_mutex_unlock(&lock);
 
     return ret;
 }
@@ -582,14 +540,14 @@ static void paio_cancel(BlockDriverAIOCB *blockacb)
 
     trace_paio_cancel(acb, acb->common.opaque);
 
-    mutex_lock(&lock);
+    qemu_mutex_lock(&lock);
     if (!acb->active) {
         QTAILQ_REMOVE(&request_list, acb, node);
         acb->ret = -ECANCELED;
     } else if (acb->ret == -EINPROGRESS) {
         active = 1;
     }
-    mutex_unlock(&lock);
+    qemu_mutex_unlock(&lock);
 
     if (active) {
         /* fail safe: if the aio could not be canceled, we wait for
@@ -655,11 +613,13 @@ int paio_init(void)
 {
     PosixAioState *s;
     int fds[2];
-    int ret;
 
     if (posix_aio_state)
         return 0;
 
+    qemu_mutex_init(&lock);
+    qemu_cond_init(&cond);
+
     s = g_malloc(sizeof(PosixAioState));
 
     s->first_aio = NULL;
@@ -678,14 +638,6 @@ int paio_init(void)
     qemu_aio_set_fd_handler(s->rfd, posix_aio_read, NULL, posix_aio_flush,
         posix_aio_process_queue, s);
 
-    ret = pthread_attr_init(&attr);
-    if (ret)
-        die2(ret, "pthread_attr_init");
-
-    ret = pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED);
-    if (ret)
-        die2(ret, "pthread_attr_setdetachstate");
-
     QTAILQ_INIT(&request_list);
     new_thread_bh = qemu_bh_new(spawn_thread_bh_fn, NULL);
 
-- 
1.7.3.4

[Qemu-devel] [PATCH 3/5] Use qemu_eventfd for POSIX AIO

[Jan Kiszka]

Use qemu_eventfd for signaling POSIX AIO completions. If native eventfd
suport is available, this avoids multiple read accesses to drain
multiple pending signals. As before we use a pipe if eventfd is not
supported.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 posix-aio-compat.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/posix-aio-compat.c b/posix-aio-compat.c
index c9b8ebf..7c3ff6e 100644
--- a/posix-aio-compat.c
+++ b/posix-aio-compat.c
@@ -481,7 +481,7 @@ static void posix_aio_read(void *opaque)
     PosixAioState *s = opaque;
     ssize_t len;
 
-    /* read all bytes from signal pipe */
+    /* read all bytes from eventfd or signal pipe */
     for (;;) {
         char bytes[16];
 
@@ -506,10 +506,14 @@ static PosixAioState *posix_aio_state;
 
 static void posix_aio_notify_event(void)
 {
-    char byte = 0;
+    /* Write 8 bytes to be compatible with eventfd.  */
+    static const uint64_t val = 1;
     ssize_t ret;
 
-    ret = write(posix_aio_state->wfd, &byte, sizeof(byte));
+    do {
+        ret = write(posix_aio_state->wfd, &val, sizeof(val));
+    } while (ret < 0 && errno == EINTR);
+
     if (ret < 0 && errno != EAGAIN)
         die("write()");
 }
@@ -623,7 +627,7 @@ int paio_init(void)
     s = g_malloc(sizeof(PosixAioState));
 
     s->first_aio = NULL;
-    if (qemu_pipe(fds) == -1) {
+    if (qemu_eventfd(fds) == -1) {
         fprintf(stderr, "failed to create pipe\n");
         g_free(s);
         return -1;
-- 
1.7.3.4

[Qemu-devel] [PATCH 4/5] Reorder POSIX compat AIO code

[Jan Kiszka]

Effectively no functional changes. This just moves
posix_aio_notify_event up without modifying it and folds die/die2 into
their only remaining user.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 posix-aio-compat.c |   44 ++++++++++++++++----------------------------
 1 files changed, 16 insertions(+), 28 deletions(-)

diff --git a/posix-aio-compat.c b/posix-aio-compat.c
index 7c3ff6e..f093156 100644
--- a/posix-aio-compat.c
+++ b/posix-aio-compat.c
@@ -71,6 +71,7 @@ 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;
+static PosixAioState *posix_aio_state;
 
 #ifdef CONFIG_PREADV
 static int preadv_present = 1;
@@ -78,17 +79,6 @@ static int preadv_present = 1;
 static int preadv_present = 0;
 #endif
 
-static void die2(int err, const char *what)
-{
-    fprintf(stderr, "%s failed: %s\n", what, strerror(err));
-    abort();
-}
-
-static void die(const char *what)
-{
-    die2(errno, what);
-}
-
 static ssize_t handle_aiocb_ioctl(struct qemu_paiocb *aiocb)
 {
     int ret;
@@ -277,7 +267,21 @@ static ssize_t handle_aiocb_rw(struct qemu_paiocb *aiocb)
     return nbytes;
 }
 
-static void posix_aio_notify_event(void);
+static void posix_aio_notify_event(void)
+{
+    /* Write 8 bytes to be compatible with eventfd.  */
+    static const uint64_t val = 1;
+    ssize_t ret;
+
+    do {
+        ret = write(posix_aio_state->wfd, &val, sizeof(val));
+    } while (ret < 0 && errno == EINTR);
+
+    if (ret < 0 && errno != EAGAIN) {
+        fprintf(stderr, "write() failed: %s\n", strerror(errno));
+        abort();
+    }
+}
 
 static void *aio_thread(void *unused)
 {
@@ -502,22 +506,6 @@ static int posix_aio_flush(void *opaque)
     return !!s->first_aio;
 }
 
-static PosixAioState *posix_aio_state;
-
-static void posix_aio_notify_event(void)
-{
-    /* Write 8 bytes to be compatible with eventfd.  */
-    static const uint64_t val = 1;
-    ssize_t ret;
-
-    do {
-        ret = write(posix_aio_state->wfd, &val, sizeof(val));
-    } while (ret < 0 && errno == EINTR);
-
-    if (ret < 0 && errno != EAGAIN)
-        die("write()");
-}
-
 static void paio_remove(struct qemu_paiocb *acb)
 {
     struct qemu_paiocb **pacb;
-- 
1.7.3.4

[Qemu-devel] [PATCH 5/5] Switch compatfd to QEMU thread

[Jan Kiszka]

qemu_thread_create already does signal blocking and detaching for us.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 compatfd.c |   16 +++-------------
 1 files changed, 3 insertions(+), 13 deletions(-)

diff --git a/compatfd.c b/compatfd.c
index 42f81ca..8d5a63f 100644
--- a/compatfd.c
+++ b/compatfd.c
@@ -14,10 +14,10 @@
  */
 
 #include "qemu-common.h"
+#include "qemu-thread.h"
 #include "compatfd.h"
 
 #include <sys/syscall.h>
-#include <pthread.h>
 
 struct sigfd_compat_info
 {
@@ -28,10 +28,6 @@ struct sigfd_compat_info
 static void *sigwait_compat(void *opaque)
 {
     struct sigfd_compat_info *info = opaque;
-    sigset_t all;
-
-    sigfillset(&all);
-    pthread_sigmask(SIG_BLOCK, &all, NULL);
 
     while (1) {
         int sig;
@@ -71,9 +67,8 @@ static void *sigwait_compat(void *opaque)
 
 static int qemu_signalfd_compat(const sigset_t *mask)
 {
-    pthread_attr_t attr;
-    pthread_t tid;
     struct sigfd_compat_info *info;
+    QemuThread thread;
     int fds[2];
 
     info = malloc(sizeof(*info));
@@ -93,12 +88,7 @@ static int qemu_signalfd_compat(const sigset_t *mask)
     memcpy(&info->mask, mask, sizeof(*mask));
     info->fd = fds[1];
 
-    pthread_attr_init(&attr);
-    pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED);
-
-    pthread_create(&tid, &attr, sigwait_compat, info);
-
-    pthread_attr_destroy(&attr);
+    qemu_thread_create(&thread, sigwait_compat, info, QEMU_THREAD_DETACHED);
 
     return fds[0];
 }
-- 
1.7.3.4

Re: [Qemu-devel] [PATCH 0/5] Spread the use of QEMU threading & locking API

[Paolo Bonzini]

Il 04/04/2012 17:08, Jan Kiszka ha scritto:
> Refreshed versions of some cleanups I already sent last year. See
> patches for details.

Patches 1, 2 and 5 are fine.  I had an alternative implementation using
a counting semaphore instead of the condition variable (which would work
on Windows too), but I can rebase later if ever.

For patches 3 and 4, I'd rather use an EventNotifier...

Paolo

> The series also helps using different scheduling policies for QEMU
> threads which includes hardening internal locks.
> 
> Jan Kiszka (5):
>   Introduce qemu_cond_timedwait for POSIX
>   Switch POSIX compat AIO to QEMU abstractions
>   Use qemu_eventfd for POSIX AIO
>   Reorder POSIX compat AIO code
>   Switch compatfd to QEMU thread
> 
>  compatfd.c          |   16 +----
>  posix-aio-compat.c  |  162 +++++++++++++++++----------------------------------
>  qemu-thread-posix.c |   23 +++++++
>  qemu-thread-posix.h |    5 ++
>  4 files changed, 84 insertions(+), 122 deletions(-)
> 

Re: [Qemu-devel] [PATCH 0/5] Spread the use of QEMU threading & locking API

[Jan Kiszka]

On 2012-04-04 17:18, Paolo Bonzini wrote:
> Il 04/04/2012 17:08, Jan Kiszka ha scritto:
>> Refreshed versions of some cleanups I already sent last year. See
>> patches for details.
> 
> Patches 1, 2 and 5 are fine.  I had an alternative implementation using
> a counting semaphore instead of the condition variable (which would work
> on Windows too), but I can rebase later if ever.
> 
> For patches 3 and 4, I'd rather use an EventNotifier...

...which still lacks support for non-eventfd systems. Hmm, I guess it's
time to consolidate both.

Jan

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

Re: [Qemu-devel] [PATCH 0/5] Spread the use of QEMU threading & locking API

[Paolo Bonzini]

Il 04/04/2012 17:24, Jan Kiszka ha scritto:
>> > For patches 3 and 4, I'd rather use an EventNotifier...
> ...which still lacks support for non-eventfd systems. Hmm, I guess it's
> time to consolidate both.

Perhaps you can take the relevant patches out of the thread-blocks
branch at git://github.com/pbonzini/qemu.git?  The iothread eventfd
could also use an EventNotifier.

Paolo

Re: [Qemu-devel] [PATCH 0/5] Spread the use of QEMU threading & locking API

[Jan Kiszka]

On 2012-04-04 17:29, Paolo Bonzini wrote:
> Il 04/04/2012 17:24, Jan Kiszka ha scritto:
>>>> For patches 3 and 4, I'd rather use an EventNotifier...
>> ...which still lacks support for non-eventfd systems. Hmm, I guess it's
>> time to consolidate both.
> 
> Perhaps you can take the relevant patches out of the thread-blocks
> branch at git://github.com/pbonzini/qemu.git?  The iothread eventfd
> could also use an EventNotifier.

Current EventNotifier code applies EFD_SEMAPHORE, which is not what we
want for Posix AIO (otherwise we could just stick with the pipe). I
cannot match the reasoning for EFD_SEM in event_notifier.c with the man
page, need to dig deeper.

Jan

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

Re: [Qemu-devel] [PATCH 0/5] Spread the use of QEMU threading & locking API

[Jan Kiszka]

On 2012-04-04 17:38, Jan Kiszka wrote:
> On 2012-04-04 17:29, Paolo Bonzini wrote:
>> Il 04/04/2012 17:24, Jan Kiszka ha scritto:
>>>>> For patches 3 and 4, I'd rather use an EventNotifier...
>>> ...which still lacks support for non-eventfd systems. Hmm, I guess it's
>>> time to consolidate both.
>>
>> Perhaps you can take the relevant patches out of the thread-blocks
>> branch at git://github.com/pbonzini/qemu.git?  The iothread eventfd
>> could also use an EventNotifier.
> 
> Current EventNotifier code applies EFD_SEMAPHORE, which is not what we
> want for Posix AIO (otherwise we could just stick with the pipe). I
> cannot match the reasoning for EFD_SEM in event_notifier.c with the man
> page, need to dig deeper.

Err, non-sense, comment and code diverged and confused my eyes...

Jan

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

Re: [Qemu-devel] [PATCH 0/5] Spread the use of QEMU threading & locking API

[Jan Kiszka]

On 2012-04-04 17:29, Paolo Bonzini wrote:
> Il 04/04/2012 17:24, Jan Kiszka ha scritto:
>>>> For patches 3 and 4, I'd rather use an EventNotifier...
>> ...which still lacks support for non-eventfd systems. Hmm, I guess it's
>> time to consolidate both.
> 
> Perhaps you can take the relevant patches out of the thread-blocks
> branch at git://github.com/pbonzini/qemu.git?  The iothread eventfd
> could also use an EventNotifier.

Yep, this screams for something like QemuEvent which pleases all users
of current qemu_eventfd and EventNotifier - and fit into the existing
threading/synchronization abstraction layout.

I retract patches 3&4, will be done on top of the event rework.

Jan

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

Re: [Qemu-devel] [PATCH 0/5] Spread the use of QEMU threading & locking API

[Paolo Bonzini]

Il 04/04/2012 18:05, Jan Kiszka ha scritto:
>> > Perhaps you can take the relevant patches out of the thread-blocks
>> > branch at git://github.com/pbonzini/qemu.git?  The iothread eventfd
>> > could also use an EventNotifier.
> Yep, this screams for something like QemuEvent which pleases all users
> of current qemu_eventfd and EventNotifier - and fit into the existing
> threading/synchronization abstraction layout.

Kind of, on Unix you cannot poll synchronization primitives so
EventNotifier has to remain separate from qemu-thread.

> I retract patches 3&4, will be done on top of the event rework.

Cool, thanks!

Paolo

Re: [Qemu-devel] [PATCH 0/5] Spread the use of QEMU threading & locking API

[Jan Kiszka]

On 2012-04-04 18:39, Paolo Bonzini wrote:
> Il 04/04/2012 18:05, Jan Kiszka ha scritto:
>>>> Perhaps you can take the relevant patches out of the thread-blocks
>>>> branch at git://github.com/pbonzini/qemu.git?  The iothread eventfd
>>>> could also use an EventNotifier.
>> Yep, this screams for something like QemuEvent which pleases all users
>> of current qemu_eventfd and EventNotifier - and fit into the existing
>> threading/synchronization abstraction layout.
> 
> Kind of, on Unix you cannot poll synchronization primitives so
> EventNotifier has to remain separate from qemu-thread.

QemuEvent will be pollable as you can ask it for its read fd:

void qemu_event_init(QemuEvent *event, bool signaled);
void qemu_event_destroy(QemuEvent *event);
int qemu_event_get_write_fd(QemuEvent *event);
int qemu_event_get_read_fd(QemuEvent *event);
void qemu_event_signal(QemuEvent *event);
bool qemu_event_check(QemuEvent *event);

I'm not yet convinced a qemu_event_set_handler buys us a lot, so I
prefer the get_read_fd interface for now.

This is just a matter of code organization, and I prefer to consolidate
this under the hood of qemu-thread.

Jan

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

Re: [Qemu-devel] [PATCH 0/5] Spread the use of QEMU threading & locking API

[Jan Kiszka]

On 2012-04-04 18:55, Jan Kiszka wrote:
> On 2012-04-04 18:39, Paolo Bonzini wrote:
>> Il 04/04/2012 18:05, Jan Kiszka ha scritto:
>>>>> Perhaps you can take the relevant patches out of the thread-blocks
>>>>> branch at git://github.com/pbonzini/qemu.git?  The iothread eventfd
>>>>> could also use an EventNotifier.
>>> Yep, this screams for something like QemuEvent which pleases all users
>>> of current qemu_eventfd and EventNotifier - and fit into the existing
>>> threading/synchronization abstraction layout.
>>
>> Kind of, on Unix you cannot poll synchronization primitives so

Ah, you meant on Win32 you cannot poll!

>> EventNotifier has to remain separate from qemu-thread.
> 
> QemuEvent will be pollable as you can ask it for its read fd:
> 
> void qemu_event_init(QemuEvent *event, bool signaled);
> void qemu_event_destroy(QemuEvent *event);
> int qemu_event_get_write_fd(QemuEvent *event);
> int qemu_event_get_read_fd(QemuEvent *event);
> void qemu_event_signal(QemuEvent *event);
> bool qemu_event_check(QemuEvent *event);
> 
> I'm not yet convinced a qemu_event_set_handler buys us a lot, so I
> prefer the get_read_fd interface for now.

I'm starting to understand your abstraction here - makes sense. :)

Jan

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

Re: [Qemu-devel] [PATCH 0/5] Spread the use of QEMU threading & locking API

[Paolo Bonzini]

Il 04/04/2012 19:19, Jan Kiszka ha scritto:
>>>> >>> Yep, this screams for something like QemuEvent which pleases all users
>>>> >>> of current qemu_eventfd and EventNotifier - and fit into the existing
>>>> >>> threading/synchronization abstraction layout.
>>> >>
>>> >> Kind of, on Unix you cannot poll synchronization primitives so
> 
> Ah, you meant on Win32 you cannot poll!

No, I meant poll(2), or select if you prefer. :)

The difference between EventNotifier and Qemu{Cond,Mutex} is that the
former can be used in the main loop, the latter cannot.

Under POSIX you have file descriptors and synchronization primitives.
You can poll a set of fds, but synchronization primitives are distinct
and have separate APIs.

Under Windows you have various kinds of waitable objects.  You can poll
a set of them with WaitForMultipleObjects, and synchronization
primitives are waitable objects.  So they have no userspace-only fast
path and are much more heavyweight than POSIX, but you can poll on
different synchronization primitives at the same time.  For example you
can say "I want to get this mutex, but I also need to know if I have to
do this high-priority task; if so I won't care about getting the mutex
anymore".

Now, Qemu{Cond,Mutex} are POSIX-y (lightweight) synchronization
primitives, EventNotifier (waitable) is a Windows-y synchronization
primitive.  That's why I think it is substantially different from other
primitives in qemu-thread.{h,c}.

(Of course, theis is not entirely accurate.  Windows has always had a
POSIX-like lightweight mutex, called CRITICAL_SECTION, and we use it for
QemuMutex; starting from Vista it also grew POSIX-like lightweight
condvars and rwlocks, but QEMU Win32 code targets XP.  And eventfd could
be used to implement most Windows synchronization primitives on Linux,
possibly using EFD_SEMAPHORE).

BTW you _could_ have a QemuEvent primitive based on Windows manual-reset
events. It can be used in some cases as a replacement for condvars,
especially when you have multiple producers and a single consumer (MPSC
queue is perhaps the easiest lock-free data structure).  It can be made
very lightweight on Linux using futexes, and would also support timed
wait easily on Windows.  The API would be more or less like this:

  void qemu_event_init(QemuEvent *event, bool set);
  void qemu_event_wait(QemuEvent *event);
  void qemu_event_timedwait(QemuEvent *event, int ms);
  void qemu_event_set(QemuEvent *event);
  void qemu_event_reset(QemuEvent *event);

BTW^2 what's the guide for calling something qemu_blah?  So far my idea
was that qemu_blah should satisfy one of the following conditions:

1) it is a wrapper around a system call or ANSI C standard function;

2) it handles portability with a trivial implementation for POSIX (resp.
Win32) and a complex implementation on Win32 (resp. POSIX);

3) it modifies global state.

There are some exceptions (e.g. QemuOpts) but overall it roughly matches
cases when new qemu_* are being added.

Does it make sense?

Paolo

Re: [Qemu-devel] [PATCH 0/5] Spread the use of QEMU threading & locking API

[Jan Kiszka]

On 2012-04-05 09:51, Paolo Bonzini wrote:
> Il 04/04/2012 19:19, Jan Kiszka ha scritto:
>>>>>>>> Yep, this screams for something like QemuEvent which pleases all users
>>>>>>>> of current qemu_eventfd and EventNotifier - and fit into the existing
>>>>>>>> threading/synchronization abstraction layout.
>>>>>>
>>>>>> Kind of, on Unix you cannot poll synchronization primitives so
>>
>> Ah, you meant on Win32 you cannot poll!
> 
> No, I meant poll(2), or select if you prefer. :)
> 
> The difference between EventNotifier and Qemu{Cond,Mutex} is that the
> former can be used in the main loop, the latter cannot.

The latter has no real use case for selection/polling, at least not in
QEMU so far. So that is not the issue here.

What was rather unhandy in the abstraction and build process are the
differences in registering an event with a poll loop.

> 
> Under POSIX you have file descriptors and synchronization primitives.
> You can poll a set of fds, but synchronization primitives are distinct
> and have separate APIs.
> 
> Under Windows you have various kinds of waitable objects.  You can poll
> a set of them with WaitForMultipleObjects, and synchronization
> primitives are waitable objects.  So they have no userspace-only fast
> path and are much more heavyweight than POSIX, but you can poll on
> different synchronization primitives at the same time.  For example you
> can say "I want to get this mutex, but I also need to know if I have to
> do this high-priority task; if so I won't care about getting the mutex
> anymore".
> 
> Now, Qemu{Cond,Mutex} are POSIX-y (lightweight) synchronization
> primitives, EventNotifier (waitable) is a Windows-y synchronization
> primitive.  That's why I think it is substantially different from other
> primitives in qemu-thread.{h,c}.
> 
> (Of course, theis is not entirely accurate.  Windows has always had a
> POSIX-like lightweight mutex, called CRITICAL_SECTION, and we use it for
> QemuMutex; starting from Vista it also grew POSIX-like lightweight
> condvars and rwlocks, but QEMU Win32 code targets XP.  And eventfd could
> be used to implement most Windows synchronization primitives on Linux,
> possibly using EFD_SEMAPHORE).
> 
> BTW you _could_ have a QemuEvent primitive based on Windows manual-reset
> events. It can be used in some cases as a replacement for condvars,
> especially when you have multiple producers and a single consumer (MPSC
> queue is perhaps the easiest lock-free data structure).  It can be made
> very lightweight on Linux using futexes, and would also support timed
> wait easily on Windows.  The API would be more or less like this:
> 
>   void qemu_event_init(QemuEvent *event, bool set);
>   void qemu_event_wait(QemuEvent *event);
>   void qemu_event_timedwait(QemuEvent *event, int ms);
>   void qemu_event_set(QemuEvent *event);
>   void qemu_event_reset(QemuEvent *event);

Yes, qemu_event_wait/timedwait could be added on top later on when we
have a use case in sight. But mapping futexes wouldn't be compatible
with eventfd/pipe based signaling.

> 
> BTW^2 what's the guide for calling something qemu_blah?  So far my idea
> was that qemu_blah should satisfy one of the following conditions:
> 
> 1) it is a wrapper around a system call or ANSI C standard function;
> 
> 2) it handles portability with a trivial implementation for POSIX (resp.
> Win32) and a complex implementation on Win32 (resp. POSIX);
> 
> 3) it modifies global state.
> 
> There are some exceptions (e.g. QemuOpts) but overall it roughly matches
> cases when new qemu_* are being added.

It's hard to solve this in a "trivial" way for both of our platform, but
QemuEvent is a QEMU-styled thread synchronization mechanism, so I think
it deserve to enter the namespace.

> 
> Does it make sense?
> 
> Paolo

Let's see :). I'll post my proposal soon.

Jan

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

Re: [Qemu-devel] [PATCH 0/5] Spread the use of QEMU threading & locking API

[Paolo Bonzini]

Il 05/04/2012 12:55, Jan Kiszka ha scritto:
>> BTW you _could_ have a QemuEvent primitive based on Windows manual-reset
>> events. It can be used in some cases as a replacement for condvars,
>> especially when you have multiple producers and a single consumer (MPSC
>> queue is perhaps the easiest lock-free data structure).  It can be made
>> very lightweight on Linux using futexes, and would also support timed
>> wait easily on Windows.  The API would be more or less like this:
>>
>>   void qemu_event_init(QemuEvent *event, bool set);
>>   void qemu_event_wait(QemuEvent *event);
>>   void qemu_event_timedwait(QemuEvent *event, int ms);
>>   void qemu_event_set(QemuEvent *event);
>>   void qemu_event_reset(QemuEvent *event);
> 
> Yes, qemu_event_wait/timedwait could be added on top later on when we
> have a use case in sight. But mapping futexes wouldn't be compatible
> with eventfd/pipe based signaling.

Note that the thing above would be separate from EventNotifier, which is
why I only mentioned as "by the way".

EventNotifier and anything using eventfd/pipes would _not_ be a
"QEMU-styled thread synchronization mechanism" simply because you can
use it with qemu_aio_set_fd_handler.

That's why I think it should be separate from qemu-threads and stay
outside the QEMU namespace.

Paolo

Re: [Qemu-devel] [PATCH 0/5] Spread the use of QEMU threading & locking API

[Jan Kiszka]

On 2012-04-05 13:07, Paolo Bonzini wrote:
> Il 05/04/2012 12:55, Jan Kiszka ha scritto:
>>> BTW you _could_ have a QemuEvent primitive based on Windows manual-reset
>>> events. It can be used in some cases as a replacement for condvars,
>>> especially when you have multiple producers and a single consumer (MPSC
>>> queue is perhaps the easiest lock-free data structure).  It can be made
>>> very lightweight on Linux using futexes, and would also support timed
>>> wait easily on Windows.  The API would be more or less like this:
>>>
>>>   void qemu_event_init(QemuEvent *event, bool set);
>>>   void qemu_event_wait(QemuEvent *event);
>>>   void qemu_event_timedwait(QemuEvent *event, int ms);
>>>   void qemu_event_set(QemuEvent *event);
>>>   void qemu_event_reset(QemuEvent *event);
>>
>> Yes, qemu_event_wait/timedwait could be added on top later on when we
>> have a use case in sight. But mapping futexes wouldn't be compatible
>> with eventfd/pipe based signaling.
> 
> Note that the thing above would be separate from EventNotifier, which is
> why I only mentioned as "by the way".
> 
> EventNotifier and anything using eventfd/pipes would _not_ be a
> "QEMU-styled thread synchronization mechanism" simply because you can
> use it with qemu_aio_set_fd_handler.
> 
> That's why I think it should be separate from qemu-threads and stay
> outside the QEMU namespace.

Sorry, but that makes no sense to me. It is an abstraction we defined
for QEMU usage in a way that fits precisely our (current) needs. That's
what we did with tons of other abstractions before, so it fits very well
here IMHO.

Jan

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

Re: [Qemu-devel] [PATCH 0/5] Spread the use of QEMU threading & locking API

[Paolo Bonzini]

Il 05/04/2012 13:18, Jan Kiszka ha scritto:
>> > Note that the thing above would be separate from EventNotifier, which is
>> > why I only mentioned as "by the way".
>> > 
>> > EventNotifier and anything using eventfd/pipes would _not_ be a
>> > "QEMU-styled thread synchronization mechanism" simply because you can
>> > use it with qemu_aio_set_fd_handler.
>> > 
>> > That's why I think it should be separate from qemu-threads and stay
>> > outside the QEMU namespace.
> 
> Sorry, but that makes no sense to me. It is an abstraction we defined
> for QEMU usage in a way that fits precisely our (current) needs. That's
> what we did with tons of other abstractions before, so it fits very well
> here IMHO.

EventNotifier _is not_ yet another thread synchronization primitive.  It
can be used across processes, across the user/kernel boundary, and the
main loop can wait on multiple instances.  QemuThread synchronization
primitives are only usable within a process, cannot be passed to the
kernel, and cannot signal the main loop.

Besides, QemuEvent is no different from the existing EventNotifier, I
don't think the churn introduced by the rename is justified.

Paolo

Re: [Qemu-devel] [PATCH 0/5] Spread the use of QEMU threading & locking API

[Jan Kiszka]

On 2012-04-05 13:29, Paolo Bonzini wrote:
> Il 05/04/2012 13:18, Jan Kiszka ha scritto:
>>>> Note that the thing above would be separate from EventNotifier, which is
>>>> why I only mentioned as "by the way".
>>>>
>>>> EventNotifier and anything using eventfd/pipes would _not_ be a
>>>> "QEMU-styled thread synchronization mechanism" simply because you can
>>>> use it with qemu_aio_set_fd_handler.
>>>>
>>>> That's why I think it should be separate from qemu-threads and stay
>>>> outside the QEMU namespace.
>>
>> Sorry, but that makes no sense to me. It is an abstraction we defined
>> for QEMU usage in a way that fits precisely our (current) needs. That's
>> what we did with tons of other abstractions before, so it fits very well
>> here IMHO.
> 
> EventNotifier _is not_ yet another thread synchronization primitive.  It
> can be used across processes, across the user/kernel boundary, and the
> main loop can wait on multiple instances.  QemuThread synchronization
> primitives are only usable within a process, cannot be passed to the
> kernel, and cannot signal the main loop.

Yes, QemuEvent can also be triggered externally - so could at least some
of the other synchronization primitives if we had a use case for that.

> 
> Besides, QemuEvent is no different from the existing EventNotifier, I
> don't think the churn introduced by the rename is justified.

It is as EventNotifiers stood aside our synchronization infrastructure,
and were only designed around vhost-net. This moves the concept in the
center AND applies it broadly, including to the main loop. That "churn"
is adoption to our naming and code organization scheme for
synchronization primitives.

Jan

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

Re: [Qemu-devel] [PATCH 0/5] Spread the use of QEMU threading & locking API

[Paolo Bonzini]

Il 05/04/2012 14:04, Jan Kiszka ha scritto:
>> EventNotifier _is not_ yet another thread synchronization primitive.  It
>> can be used across processes, across the user/kernel boundary, and the
>> main loop can wait on multiple instances.  QemuThread synchronization
>> primitives are only usable within a process, cannot be passed to the
>> kernel, and cannot signal the main loop.
> 
> Yes, QemuEvent can also be triggered externally - so could at least some
> of the other synchronization primitives if we had a use case for that.
> 
>> Besides, QemuEvent is no different from the existing EventNotifier, I
>> don't think the churn introduced by the rename is justified.
> 
> It is as EventNotifiers stood aside our synchronization infrastructure,
> and were only designed around vhost-net. This moves the concept in the
> center AND applies it broadly, including to the main loop. That "churn"
> is adoption to our naming and code organization scheme for
> synchronization primitives.

But QemuEvent takes away the best name for a useful concept (a
cross-platform implementation of Win32 events; you can see that in the
RCU patches which were even posted on the list).  We already have a
perfectly good name for EventNotifiers, and there's no reason to break
the history of event-notifier.c.

Paolo

Re: [Qemu-devel] [PATCH 0/5] Spread the use of QEMU threading & locking API

[Paolo Bonzini]

Il 05/04/2012 15:00, Jan Kiszka ha scritto:
>> > But QemuEvent takes away the best name for a useful concept (a
>> > cross-platform implementation of Win32 events; you can see that in the
> The concept is not lost, it perfectly fit this incarnation. Just the
> special futex version for Linux is not feasible.

It's not just about the futex version.  Can you implement a
userspace-only fast path?  Perhaps with EFD_SEMAPHORE you can:

  x = state of the event
    bit 0 = set/reset
    bit 1..31 = waiters

  set
    y = xchg(&x, 1)
    if y > 1
      write y >> 1 to eventfd

  wait
    do {
      y = x
      if (y & 1) return;
    } while (fail to cmpxchg x from y to y + 2)
    read from eventfd

  reset
    cmpxchg x from 1 to 0

but what if you are falling back to pipes?

2) It's much more heavyweight since (like Windows primitives) you need
to set aside OS resources for each QemuEvent.  With mutexes and condvars
the kernel-side waitqueues come and go as they are used.

>> > RCU patches which were even posted on the list).  We already have a
>> > perfectly good name for EventNotifiers, and there's no reason to break
>> > the history of event-notifier.c.
> Have you measured if the futex optimization is actually worth the
> effort, specifically compared to the fast path of mutex/cond loop?

A futex is 30% faster than the mutex/cond combination.  It's called on
fast paths (call_rcu and, depending on how you implement RCU,
rcu_read_unlock) so it's important.

Paolo

Re: [Qemu-devel] [PATCH 0/5] Spread the use of QEMU threading & locking API

[Jan Kiszka]

On 2012-04-05 15:40, Paolo Bonzini wrote:
> Il 05/04/2012 15:00, Jan Kiszka ha scritto:
>>>> But QemuEvent takes away the best name for a useful concept (a
>>>> cross-platform implementation of Win32 events; you can see that in the
>> The concept is not lost, it perfectly fit this incarnation. Just the
>> special futex version for Linux is not feasible.
> 
> It's not just about the futex version.  Can you implement a
> userspace-only fast path?  Perhaps with EFD_SEMAPHORE you can:
> 
>   x = state of the event
>     bit 0 = set/reset
>     bit 1..31 = waiters
> 
>   set
>     y = xchg(&x, 1)
>     if y > 1
>       write y >> 1 to eventfd
> 
>   wait
>     do {
>       y = x
>       if (y & 1) return;
>     } while (fail to cmpxchg x from y to y + 2)
>     read from eventfd
> 
>   reset
>     cmpxchg x from 1 to 0
> 
> but what if you are falling back to pipes?

Either you signal via the fd or via a variable. Doing both won't work as
the state can only be in the eventfd/pipe (for external triggers). We
could switch the mode of our QemuEvent on init, but that will become
ugly I'm afraid.

> 
> 2) It's much more heavyweight since (like Windows primitives) you need
> to set aside OS resources for each QemuEvent.  With mutexes and condvars
> the kernel-side waitqueues come and go as they are used.
> 
>>>> RCU patches which were even posted on the list).  We already have a
>>>> perfectly good name for EventNotifiers, and there's no reason to break
>>>> the history of event-notifier.c.
>> Have you measured if the futex optimization is actually worth the
>> effort, specifically compared to the fast path of mutex/cond loop?
> 
> A futex is 30% faster than the mutex/cond combination.  It's called on
> fast paths (call_rcu and, depending on how you implement RCU,
> rcu_read_unlock) so it's important.

If RCU is the only user for this optimized signaling, then I would vote
for doing it in the RCU layer directly. If there are also other users in
sight that could benefit (because of mostly-set-rarely-reset patterns),
I agree that a QemuEvent is the better home. Can you name more use cases
in QEMU?

Happy vacations,
Jan (off for Easter now)

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

Re: [Qemu-devel] [PATCH 0/5] Spread the use of QEMU threading & locking API

[Paolo Bonzini]

Il 05/04/2012 16:01, Jan Kiszka ha scritto:
> Either you signal via the fd or via a variable. Doing both won't work as
> the state can only be in the eventfd/pipe (for external triggers). We
> could switch the mode of our QemuEvent on init, but that will become
> ugly I'm afraid.

Yeah...

>>>>> RCU patches which were even posted on the list).  We already have a
>>>>> perfectly good name for EventNotifiers, and there's no reason to break
>>>>> the history of event-notifier.c.
>>> Have you measured if the futex optimization is actually worth the
>>> effort, specifically compared to the fast path of mutex/cond loop?
>>
>> A futex is 30% faster than the mutex/cond combination.  It's called on
>> fast paths (call_rcu and, depending on how you implement RCU,
>> rcu_read_unlock) so it's important.
> 
> If RCU is the only user for this optimized signaling, then I would vote
> for doing it in the RCU layer directly. If there are also other users in
> sight that could benefit (because of mostly-set-rarely-reset patterns),
> I agree that a QemuEvent is the better home. Can you name more use cases
> in QEMU?

No idea, but the general use case is when you have something that is
multi-producer and single-consumer.

Paolo