Fix the regression with SPARC emulation

PATCH In review Anthony Liguori 3 participants 5 messages Oct 7, 2008 – Oct 8, 2008 Kbuild

Share & tools

Anthony Liguori

author

2008-10-07 22:27 UTC #0 lore raw

PATCH 1/1 Fix the regression with SPARC emulation

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

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 <[email protected]>

compatfd.c deleted +0 ~~-127~~

deleted file mode 100644
index cc5ced3..0000000
@@ -1,127 +0,0 @@
-/*
- * signalfd/eventfd compatibility
- *
- * Copyright IBM, Corp. 2008
- *
- * Authors:
- *  Anthony Liguori   <[email protected]>
- *
- * 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);
-}

compatfd.h deleted +0 ~~-28~~

deleted file mode 100644
index 55a111a..0000000
@@ -1,28 +0,0 @@
-/*
- * signalfd/eventfd compatibility
- *
- * Copyright IBM, Corp. 2008
- *
- * Authors:
- *  Anthony Liguori   <[email protected]>
- *
- * 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

Anthony Liguori

author

2008-10-07 22:42 UTC #1 lore raw

Quoted 3 lines · Anthony Liguori

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

Quoted 3 lines

Regards,
Anthony Liguori

commit d3e13a91224d167652288ba98de15658a233d60e

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

Signed-off-by Anthony Liguori <[email protected]>

compatfd.c deleted +0 ~~-127~~

deleted file mode 100644
index cc5ced3..0000000
@@ -1,127 +0,0 @@
-/*
- * signalfd/eventfd compatibility
- *
- * Copyright IBM, Corp. 2008
- *
- * Authors:
- *  Anthony Liguori   <[email protected]>
- *
- * 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);
-}

compatfd.h deleted +0 ~~-28~~

deleted file mode 100644
index 55a111a..0000000
@@ -1,28 +0,0 @@
-/*
- * signalfd/eventfd compatibility
- *
- * Copyright IBM, Corp. 2008
- *
- * Authors:
- *  Anthony Liguori   <[email protected]>
- *
- * 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

Makefile +0 -4

index 35061a4..36b36cd 100644
@@ -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

Makefile.target +0 -4

index c2016d1..f6ec8ef 100644
@@ -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

block-raw-posix.c +35 ~~-35~~

110 lines not on this page · raw message

configure +0 ~~-35~~

index a224e6a..2a0d180 100755
@@ -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

qemu-common.h +3 -0

index 0731223..cc95fe6 100644
@@ -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

qemu-tool.c +4 -0

index 63e2056..87cc294 100644
@@ -26,6 +26,10 @@ struct QEMUBH
     void *opaque;
 };
 
+void qemu_service_io(void)
+{
+}
+
 void term_printf(const char *fmt, ...)
 {
 }

vl.c +13 -0

index 439d317..5e72199 100644
@@ -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) */

Aurelien Jarno

2008-10-07 23:21 UTC #2 lore raw

Quoted 9 lines · Anthony Liguori

Quoted 7 lines · Anthony Liguori
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.

Signature · 4 lines

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

Blue Swirl

2008-10-08 18:36 UTC #3 lore raw

In reply to Anthony Liguori

Quoted 3 lines · Anthony Liguori

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?

Quoted 2 lines

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.

Anthony Liguori

author

2008-10-08 19:41 UTC #4 lore raw

Quoted 9 lines · Blue Swirl

Quoted 4 lines · Anthony Liguori
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.

Quoted 4 lines

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.

Quoted 6 lines

Quoted 3 lines
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