[Qemu-devel] [PATCH][RFT] Fix the regression with SPARC emulation

[Qemu-devel] [PATCH][RFT] Fix the regression with SPARC emulation

[Anthony Liguori]

[-- Attachment #1: Type: text/plain, Size: 372 bytes --]

The attached patch should be the proper fix for the sparc performance 
regression.  Unfortunately, this means abandoning signalfd() in favor of 
the signal handler/pipe trick but I don't see another solution to the 
problem.

Please test and let me know if this works for all of your cases (it did 
for me with -clock unix and -clock dynticks).

Regards,

Anthony Liguori

[-- Attachment #2: sigio-fix.patch --]
[-- Type: text/x-patch, Size: 4038 bytes --]

commit d3e13a91224d167652288ba98de15658a233d60e
Author: Anthony Liguori <anthony@squirrel.(none)>
Date:   Tue Oct 7 17:21:38 2008 -0500

    Replace signalfd with signal handler/pipe.  There is no way to interrupt
    the CPU execution loop when a file descriptor becomes readable.  This
    results in a large performance regression in sparc emulation during
    bootup.
    
    This patch switches us to signal handler/pipe which was originally
    suggested by Ian Jackson.  The signal handler lets us interrupt the
    CPU emulation loop while the write to a pipe lets us avoid the
    select/signal race condition.
    
    Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/compatfd.c b/compatfd.c
deleted file mode 100644
index cc5ced3..0000000
--- a/compatfd.c
+++ /dev/null
@@ -1,127 +0,0 @@
-/*
- * signalfd/eventfd compatibility
- *
- * Copyright IBM, Corp. 2008
- *
- * Authors:
- *  Anthony Liguori   <aliguori@us.ibm.com>
- *
- * This work is licensed under the terms of the GNU GPL, version 2.  See
- * the COPYING file in the top-level directory.
- *
- */
-
-#include "qemu-common.h"
-#include "compatfd.h"
-
-#include <sys/syscall.h>
-#include <pthread.h>
-
-struct sigfd_compat_info
-{
-    sigset_t mask;
-    int fd;
-};
-
-static void *sigwait_compat(void *opaque)
-{
-    struct sigfd_compat_info *info = opaque;
-    int err;
-    sigset_t all;
-
-    sigfillset(&all);
-    sigprocmask(SIG_BLOCK, &all, NULL);
-
-    do {
-        siginfo_t siginfo;
-
-        err = sigwaitinfo(&info->mask, &siginfo);
-        if (err == -1 && errno == EINTR) {
-            err = 0;
-            continue;
-        }
-
-        if (err > 0) {
-            char buffer[128];
-            size_t offset = 0;
-
-            memcpy(buffer, &err, sizeof(err));
-            while (offset < sizeof(buffer)) {
-                ssize_t len;
-
-                len = write(info->fd, buffer + offset,
-                            sizeof(buffer) - offset);
-                if (len == -1 && errno == EINTR)
-                    continue;
-
-                if (len <= 0) {
-                    err = -1;
-                    break;
-                }
-
-                offset += len;
-            }
-        }
-    } while (err >= 0);
-
-    return NULL;
-}
-
-static int qemu_signalfd_compat(const sigset_t *mask)
-{
-    pthread_attr_t attr;
-    pthread_t tid;
-    struct sigfd_compat_info *info;
-    int fds[2];
-
-    info = malloc(sizeof(*info));
-    if (info == NULL) {
-        errno = ENOMEM;
-        return -1;
-    }
-
-    if (pipe(fds) == -1) {
-        free(info);
-        return -1;
-    }
-
-    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);
-
-    return fds[0];
-}
-
-int qemu_signalfd(const sigset_t *mask)
-{
-#if defined(CONFIG_signalfd)
-    int ret;
-
-    ret = syscall(SYS_signalfd, -1, mask, _NSIG / 8);
-    if (ret != -1)
-        return ret;
-#endif
-
-    return qemu_signalfd_compat(mask);
-}
-
-int qemu_eventfd(int *fds)
-{
-#if defined(CONFIG_eventfd)
-    int ret;
-
-    ret = syscall(SYS_eventfd, 0);
-    if (ret >= 0) {
-        fds[0] = fds[1] = ret;
-        return 0;
-    }
-#endif
-
-    return pipe(fds);
-}
diff --git a/compatfd.h b/compatfd.h
deleted file mode 100644
index 55a111a..0000000
--- a/compatfd.h
+++ /dev/null
@@ -1,28 +0,0 @@
-/*
- * signalfd/eventfd compatibility
- *
- * Copyright IBM, Corp. 2008
- *
- * Authors:
- *  Anthony Liguori   <aliguori@us.ibm.com>
- *
- * This work is licensed under the terms of the GNU GPL, version 2.  See
- * the COPYING file in the top-level directory.
- *
- */
-
-#ifndef QEMU_COMPATFD_H
-#define QEMU_COMPATFD_H
-
-#include <signal.h>
-
-struct qemu_signalfd_siginfo {
-    uint32_t ssi_signo;
-    uint8_t pad[124];
-};
-
-int qemu_signalfd(const sigset_t *mask);
-
-int qemu_eventfd(int *fds);
-
-#endif

[Qemu-devel] Re: [PATCH][RFT] Fix the regression with SPARC emulation

[Anthony Liguori]

[-- Attachment #1: Type: text/plain, Size: 523 bytes --]

Anthony Liguori wrote:
> The attached patch should be the proper fix for the sparc performance 
> regression.  Unfortunately, this means abandoning signalfd() in favor 
> of the signal handler/pipe trick but I don't see another solution to 
> the problem.
>
> Please test and let me know if this works for all of your cases (it 
> did for me with -clock unix and -clock dynticks).

Err, sorry, here is the proper patch.  git did not do what I expected it 
to do.

Regards,

Anthony Liguori

> Regards,
>
> Anthony Liguori


[-- Attachment #2: sigio-fix.patch --]
[-- Type: text/x-patch, Size: 10326 bytes --]

commit d3e13a91224d167652288ba98de15658a233d60e
Author: Anthony Liguori <anthony@squirrel.(none)>
Date:   Tue Oct 7 17:21:38 2008 -0500

    Replace signalfd with signal handler/pipe.  There is no way to interrupt
    the CPU execution loop when a file descriptor becomes readable.  This
    results in a large performance regression in sparc emulation during
    bootup.
    
    This patch switches us to signal handler/pipe which was originally
    suggested by Ian Jackson.  The signal handler lets us interrupt the
    CPU emulation loop while the write to a pipe lets us avoid the
    select/signal race condition.
    
    Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/compatfd.c b/compatfd.c
deleted file mode 100644
index cc5ced3..0000000
--- a/compatfd.c
+++ /dev/null
@@ -1,127 +0,0 @@
-/*
- * signalfd/eventfd compatibility
- *
- * Copyright IBM, Corp. 2008
- *
- * Authors:
- *  Anthony Liguori   <aliguori@us.ibm.com>
- *
- * This work is licensed under the terms of the GNU GPL, version 2.  See
- * the COPYING file in the top-level directory.
- *
- */
-
-#include "qemu-common.h"
-#include "compatfd.h"
-
-#include <sys/syscall.h>
-#include <pthread.h>
-
-struct sigfd_compat_info
-{
-    sigset_t mask;
-    int fd;
-};
-
-static void *sigwait_compat(void *opaque)
-{
-    struct sigfd_compat_info *info = opaque;
-    int err;
-    sigset_t all;
-
-    sigfillset(&all);
-    sigprocmask(SIG_BLOCK, &all, NULL);
-
-    do {
-        siginfo_t siginfo;
-
-        err = sigwaitinfo(&info->mask, &siginfo);
-        if (err == -1 && errno == EINTR) {
-            err = 0;
-            continue;
-        }
-
-        if (err > 0) {
-            char buffer[128];
-            size_t offset = 0;
-
-            memcpy(buffer, &err, sizeof(err));
-            while (offset < sizeof(buffer)) {
-                ssize_t len;
-
-                len = write(info->fd, buffer + offset,
-                            sizeof(buffer) - offset);
-                if (len == -1 && errno == EINTR)
-                    continue;
-
-                if (len <= 0) {
-                    err = -1;
-                    break;
-                }
-
-                offset += len;
-            }
-        }
-    } while (err >= 0);
-
-    return NULL;
-}
-
-static int qemu_signalfd_compat(const sigset_t *mask)
-{
-    pthread_attr_t attr;
-    pthread_t tid;
-    struct sigfd_compat_info *info;
-    int fds[2];
-
-    info = malloc(sizeof(*info));
-    if (info == NULL) {
-        errno = ENOMEM;
-        return -1;
-    }
-
-    if (pipe(fds) == -1) {
-        free(info);
-        return -1;
-    }
-
-    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);
-
-    return fds[0];
-}
-
-int qemu_signalfd(const sigset_t *mask)
-{
-#if defined(CONFIG_signalfd)
-    int ret;
-
-    ret = syscall(SYS_signalfd, -1, mask, _NSIG / 8);
-    if (ret != -1)
-        return ret;
-#endif
-
-    return qemu_signalfd_compat(mask);
-}
-
-int qemu_eventfd(int *fds)
-{
-#if defined(CONFIG_eventfd)
-    int ret;
-
-    ret = syscall(SYS_eventfd, 0);
-    if (ret >= 0) {
-        fds[0] = fds[1] = ret;
-        return 0;
-    }
-#endif
-
-    return pipe(fds);
-}
diff --git a/compatfd.h b/compatfd.h
deleted file mode 100644
index 55a111a..0000000
--- a/compatfd.h
+++ /dev/null
@@ -1,28 +0,0 @@
-/*
- * signalfd/eventfd compatibility
- *
- * Copyright IBM, Corp. 2008
- *
- * Authors:
- *  Anthony Liguori   <aliguori@us.ibm.com>
- *
- * This work is licensed under the terms of the GNU GPL, version 2.  See
- * the COPYING file in the top-level directory.
- *
- */
-
-#ifndef QEMU_COMPATFD_H
-#define QEMU_COMPATFD_H
-
-#include <signal.h>
-
-struct qemu_signalfd_siginfo {
-    uint32_t ssi_signo;
-    uint8_t pad[124];
-};
-
-int qemu_signalfd(const sigset_t *mask);
-
-int qemu_eventfd(int *fds);
-
-#endif
diff --git a/Makefile b/Makefile
index 35061a4..36b36cd 100644
--- a/Makefile
+++ b/Makefile
@@ -59,10 +59,6 @@ else
 BLOCK_OBJS += block-raw-posix.o
 endif
 
-ifdef CONFIG_AIO
-BLOCK_OBJS += compatfd.o
-endif
-
 ######################################################################
 # libqemu_common.a: Target independent part of system emulation. The
 # long term path is to suppress *all* target specific code in case of
diff --git a/Makefile.target b/Makefile.target
index c2016d1..f6ec8ef 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -481,10 +481,6 @@ else
 OBJS+=block-raw-posix.o
 endif
 
-ifdef CONFIG_AIO
-OBJS+=compatfd.o
-endif
-
 LIBS+=-lz
 ifdef CONFIG_ALSA
 LIBS += -lasound
diff --git a/block-raw-posix.c b/block-raw-posix.c
index c0404fa..83a358c 100644
--- a/block-raw-posix.c
+++ b/block-raw-posix.c
@@ -25,7 +25,6 @@
 #include "qemu-timer.h"
 #include "qemu-char.h"
 #include "block_int.h"
-#include "compatfd.h"
 #include <assert.h>
 #ifdef CONFIG_AIO
 #include <aio.h>
@@ -453,7 +452,7 @@ typedef struct RawAIOCB {
 
 typedef struct PosixAioState
 {
-    int fd;
+    int rfd, wfd;
     RawAIOCB *first_aio;
 } PosixAioState;
 
@@ -494,30 +493,17 @@ static void posix_aio_read(void *opaque)
     PosixAioState *s = opaque;
     RawAIOCB *acb, **pacb;
     int ret;
-    size_t offset;
-    union {
-        struct qemu_signalfd_siginfo siginfo;
-        char buf[128];
-    } sig;
-
-    /* try to read from signalfd, don't freak out if we can't read anything */
-    offset = 0;
-    while (offset < 128) {
-        ssize_t len;
-
-        len = read(s->fd, sig.buf + offset, 128 - offset);
+    ssize_t len;
+
+    do {
+        char byte;
+
+        len = read(s->rfd, &byte, 1);
         if (len == -1 && errno == EINTR)
             continue;
-        if (len == -1 && errno == EAGAIN) {
-            /* there is no natural reason for this to happen,
-             * so we'll spin hard until we get everything just
-             * to be on the safe side. */
-            if (offset > 0)
-                continue;
-        }
-
-        offset += len;
-    }
+        if (len == -1 && errno == EAGAIN)
+            break;
+    } while (len == -1);
 
     for(;;) {
         pacb = &s->first_aio;
@@ -565,10 +551,22 @@ static int posix_aio_flush(void *opaque)
 
 static PosixAioState *posix_aio_state;
 
+static void aio_signal_handler(int signum)
+{
+    if (posix_aio_state) {
+        char byte = 0;
+
+        write(posix_aio_state->wfd, &byte, sizeof(byte));
+    }
+
+    qemu_service_io();
+}
+
 static int posix_aio_init(void)
 {
-    sigset_t mask;
+    struct sigaction act;
     PosixAioState *s;
+    int fds[2];
   
     if (posix_aio_state)
         return 0;
@@ -577,21 +575,23 @@ static int posix_aio_init(void)
     if (s == NULL)
         return -ENOMEM;
 
-    /* Make sure to block AIO signal */
-    sigemptyset(&mask);
-    sigaddset(&mask, SIGUSR2);
-    sigprocmask(SIG_BLOCK, &mask, NULL);
-    
+    sigfillset(&act.sa_mask);
+    act.sa_flags = 0; /* do not restart syscalls to interrupt select() */
+    act.sa_handler = aio_signal_handler;
+    sigaction(SIGUSR2, &act, NULL);
+
     s->first_aio = NULL;
-    s->fd = qemu_signalfd(&mask);
-    if (s->fd == -1) {
-        fprintf(stderr, "failed to create signalfd\n");
+    if (pipe(fds) == -1) {
+        fprintf(stderr, "failed to create pipe\n");
         return -errno;
     }
 
-    fcntl(s->fd, F_SETFL, O_NONBLOCK);
+    s->rfd = fds[0];
+    s->wfd = fds[1];
+
+    fcntl(s->wfd, F_SETFL, O_NONBLOCK);
 
-    qemu_aio_set_fd_handler(s->fd, posix_aio_read, NULL, posix_aio_flush, s);
+    qemu_aio_set_fd_handler(s->rfd, posix_aio_read, NULL, posix_aio_flush, s);
 
 #if defined(__linux__)
     {
diff --git a/configure b/configure
index a224e6a..2a0d180 100755
--- a/configure
+++ b/configure
@@ -112,8 +112,6 @@ aio="yes"
 nptl="yes"
 mixemu="no"
 bluez="yes"
-signalfd="no"
-eventfd="no"
 
 # OS specific
 targetos=`uname -s`
@@ -919,33 +917,6 @@ EOF
   fi
 fi
 
-##########################################
-# signalfd probe
-cat > $TMPC << EOF
-#define _GNU_SOURCE
-#include <unistd.h>
-#include <sys/syscall.h>
-#include <signal.h>
-int main(void) { return syscall(SYS_signalfd, -1, NULL, _NSIG / 8); }
-EOF
-
-if $cc $ARCH_CFLAGS -o $TMPE $TMPC 2> /dev/null ; then
-  signalfd=yes
-fi
-
-##########################################
-# eventfd probe
-cat > $TMPC << EOF
-#define _GNU_SOURCE
-#include <unistd.h>
-#include <sys/syscall.h>
-int main(void) { return syscall(SYS_eventfd, 0); }
-EOF
-
-if $cc $ARCH_CFLAGS -o $TMPE $TMPC 2> /dev/null ; then
-  eventfd=yes
-fi
-
 # Check if tools are available to build documentation.
 if [ -x "`which texi2html 2>/dev/null`" ] && \
    [ -x "`which pod2man 2>/dev/null`" ]; then
@@ -1280,12 +1251,6 @@ if test "$aio" = "yes" ; then
   echo "#define CONFIG_AIO 1" >> $config_h
   echo "CONFIG_AIO=yes" >> $config_mak
 fi
-if test "$signalfd" = "yes" ; then
-  echo "#define CONFIG_signalfd 1" >> $config_h
-fi
-if test "$eventfd" = "yes" ; then
-  echo "#define CONFIG_eventfd 1" >> $config_h
-fi
 
 # XXX: suppress that
 if [ "$bsd" = "yes" ] ; then
diff --git a/qemu-common.h b/qemu-common.h
index 0731223..cc95fe6 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -139,4 +139,7 @@ struct pcmcia_card_s;
 void cpu_save(QEMUFile *f, void *opaque);
 int cpu_load(QEMUFile *f, void *opaque, int version_id);
 
+/* Force QEMU to stop what it's doing and service IO */
+void qemu_service_io(void);
+
 #endif
diff --git a/qemu-tool.c b/qemu-tool.c
index 63e2056..87cc294 100644
--- a/qemu-tool.c
+++ b/qemu-tool.c
@@ -26,6 +26,10 @@ struct QEMUBH
     void *opaque;
 };
 
+void qemu_service_io(void)
+{
+}
+
 void term_printf(const char *fmt, ...)
 {
 }
diff --git a/vl.c b/vl.c
index 439d317..5e72199 100644
--- a/vl.c
+++ b/vl.c
@@ -7482,6 +7482,19 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
     return 0;
 }
 
+void qemu_service_io(void)
+{
+    CPUState *env = cpu_single_env;
+    if (env) {
+        cpu_interrupt(env, CPU_INTERRUPT_EXIT);
+#ifdef USE_KQEMU
+        if (env->kqemu_enabled) {
+            kqemu_cpu_interrupt(env);
+        }
+#endif
+    }
+}
+
 /***********************************************************/
 /* bottom halves (can be seen as timers which expire ASAP) */
 

[Qemu-devel] Re: [PATCH][RFT] Fix the regression with SPARC emulation

[Aurelien Jarno]

On Tue, Oct 07, 2008 at 05:41:59PM -0500, Anthony Liguori wrote:
> Anthony Liguori wrote:
> >The attached patch should be the proper fix for the sparc performance 
> >regression.  Unfortunately, this means abandoning signalfd() in favor 
> >of the signal handler/pipe trick but I don't see another solution to 
> >the problem.
> >
> >Please test and let me know if this works for all of your cases (it 
> >did for me with -clock unix and -clock dynticks).
> 

Yes, it works for me in both cases. Thanks a lot.


-- 
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net

[Qemu-devel] Re: [PATCH][RFT] Fix the regression with SPARC emulation

[Blue Swirl]

On 10/8/08, Anthony Liguori <aliguori@us.ibm.com> wrote:
> The attached patch should be the proper fix for the sparc performance
> regression.  Unfortunately, this means abandoning signalfd() in favor of the
> signal handler/pipe trick but I don't see another solution to the problem.

That's too bad, what was the problem?

Maybe there should be two commits for future bisections, one that
reverses the signalfd patches and second one that adds the signal/pipe
stuff?

>  Please test and let me know if this works for all of your cases (it did for
> me with -clock unix and -clock dynticks).

Works here too, with AIO and AIO disabled.

[Qemu-devel] Re: [PATCH][RFT] Fix the regression with SPARC emulation

[Anthony Liguori]

Blue Swirl wrote:
> On 10/8/08, Anthony Liguori <aliguori@us.ibm.com> wrote:
>   
>> The attached patch should be the proper fix for the sparc performance
>> regression.  Unfortunately, this means abandoning signalfd() in favor of the
>> signal handler/pipe trick but I don't see another solution to the problem.
>>     
>
> That's too bad, what was the problem?
>   

signalfd() worked fine for KVM because KVM handles the main loop in a 
dedicated thread.  This means select() is always running in that 
thread.  With normal QEMU, something needs to kick the VCPU execution 
out of it's loop to handle the IO request.  You need something like a 
signal to do because you need to be able to execute code to basically do 
cpu_interrupt.

The signal-pipe stuff works before we can write to the pipe to queue up 
the io completion, but also do the cpu_interrupt.  I was hoping to set 
SIGIO on the signalfd() to just have a signal handler that did 
cpu_interrupt but it doesn't appear that signalfd() supports SIGIO so 
that's not an option.

> Maybe there should be two commits for future bisections, one that
> reverses the signalfd patches and second one that adds the signal/pipe
> stuff?
>   

It would be difficult to do that because of the aio refactoring that 
came after the signalfd implementation.

>>  Please test and let me know if this works for all of your cases (it did for
>> me with -clock unix and -clock dynticks).
>>     
>
> Works here too, with AIO and AIO disabled.
>   

Okay, I'll commit.

Regards,

Anthony Liguori