[patch 0/2] qemu-kvm cleanups

[patch 0/2] qemu-kvm cleanups

[Marcelo Tosatti]

Two minor signal related cleanups.

[patch 1/2] qemu-kvm: use usptream eventfd code

[Marcelo Tosatti]

[-- Attachment #1: qemu-notify-event --]
[-- Type: text/plain, Size: 3123 bytes --]

Upstream code is equivalent.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: qemu-kvm/cpus.c
===================================================================
--- qemu-kvm.orig/cpus.c
+++ qemu-kvm/cpus.c
@@ -290,11 +290,6 @@ void qemu_notify_event(void)
 {
     CPUState *env = cpu_single_env;
 
-    if (kvm_enabled()) {
-        qemu_kvm_notify_work();
-        return;
-    }
-
     qemu_event_increment ();
     if (env) {
         cpu_exit(env);
Index: qemu-kvm/qemu-kvm.c
===================================================================
--- qemu-kvm.orig/qemu-kvm.c
+++ qemu-kvm/qemu-kvm.c
@@ -71,7 +71,6 @@ static int qemu_system_ready;
 #define SIG_IPI (SIGRTMIN+4)
 
 pthread_t io_thread;
-static int io_thread_fd = -1;
 static int io_thread_sigfd = -1;
 
 static CPUState *kvm_debug_cpu_requested;
@@ -1634,28 +1633,6 @@ int kvm_init_ap(void)
     return 0;
 }
 
-void qemu_kvm_notify_work(void)
-{
-    /* Write 8 bytes to be compatible with eventfd.  */
-    static uint64_t val = 1;
-    ssize_t ret;
-
-    if (io_thread_fd == -1) {
-        return;
-    }
-
-    do {
-        ret = write(io_thread_fd, &val, sizeof(val));
-    } while (ret < 0 && errno == EINTR);
-
-    /* EAGAIN is fine in case we have a pipe.  */
-    if (ret < 0 && errno != EAGAIN) {
-         fprintf(stderr, "qemu_kvm_notify_work: write() filed: %s\n",
-                 strerror(errno));
-         exit (1);
-    }
-}
-
 /* If we have signalfd, we mask out the signals we want to handle and then
  * use signalfd to listen for them.  We rely on whatever the current signal
  * handler is to dispatch the signals when we receive them.
@@ -1692,41 +1669,14 @@ static void sigfd_handler(void *opaque)
     }
 }
 
-/* Used to break IO thread out of select */
-static void io_thread_wakeup(void *opaque)
-{
-    int fd = (unsigned long) opaque;
-    ssize_t len;
-    char buffer[512];
-
-    /* Drain the notify pipe.  For eventfd, only 8 bytes will be read.  */
-    do {
-        len = read(fd, buffer, sizeof(buffer));
-    } while ((len == -1 && errno == EINTR) || len == sizeof(buffer));
-}
-
 int kvm_main_loop(void)
 {
-    int fds[2];
     sigset_t mask;
     int sigfd;
 
     io_thread = pthread_self();
     qemu_system_ready = 1;
 
-    if (qemu_eventfd(fds) == -1) {
-        fprintf(stderr, "failed to create eventfd\n");
-        return -errno;
-    }
-
-    fcntl(fds[0], F_SETFL, O_NONBLOCK);
-    fcntl(fds[1], F_SETFL, O_NONBLOCK);
-
-    qemu_set_fd_handler2(fds[0], NULL, io_thread_wakeup, NULL,
-                         (void *)(unsigned long) fds[0]);
-
-    io_thread_fd = fds[1];
-
     sigemptyset(&mask);
     sigaddset(&mask, SIGIO);
     sigaddset(&mask, SIGALRM);
Index: qemu-kvm/qemu-kvm.h
===================================================================
--- qemu-kvm.orig/qemu-kvm.h
+++ qemu-kvm/qemu-kvm.h
@@ -863,8 +863,6 @@ void qemu_kvm_aio_wait_start(void);
 void qemu_kvm_aio_wait(void);
 void qemu_kvm_aio_wait_end(void);
 
-void qemu_kvm_notify_work(void);
-
 void kvm_tpr_access_report(CPUState *env, uint64_t rip, int is_write);
 
 int kvm_arch_init_irq_routing(void);

[patch 2/2] qemu-kvm: drop posix-aio-compat.cs signalfd usage

[Marcelo Tosatti]

[-- Attachment #1: posix-aio-compat-use-signal --]
[-- Type: text/plain, Size: 3996 bytes --]

Block SIGUSR2, which makes the signal be handled through qemu-kvm.c's
signalfd.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: qemu-kvm/posix-aio-compat.c
===================================================================
--- qemu-kvm.orig/posix-aio-compat.c
+++ qemu-kvm/posix-aio-compat.c
@@ -26,7 +26,6 @@
 #include "osdep.h"
 #include "qemu-common.h"
 #include "block_int.h"
-#include "compatfd.h"
 
 #include "block/raw-posix-aio.h"
 
@@ -54,7 +53,7 @@ struct qemu_paiocb {
 };
 
 typedef struct PosixAioState {
-    int fd;
+    int rfd, wfd;
     struct qemu_paiocb *first_aio;
 } PosixAioState;
 
@@ -473,29 +472,18 @@ static int posix_aio_process_queue(void 
 static void posix_aio_read(void *opaque)
 {
     PosixAioState *s = opaque;
-    union {
-        struct qemu_signalfd_siginfo siginfo;
-        char buf[128];
-    } sig;
-    size_t offset;
-
-    /* try to read from signalfd, don't freak out if we can't read anything */
-    offset = 0;
-    while (offset < 128) {
-        ssize_t len;
+    ssize_t len;
 
-        len = read(s->fd, sig.buf + offset, 128 - offset);
-        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;
-        }
+    /* read all bytes from signal pipe */
+    for (;;) {
+        char bytes[16];
 
-        offset += len;
+        len = read(s->rfd, bytes, sizeof(bytes));
+        if (len == -1 && errno == EINTR)
+            continue; /* try again */
+        if (len == sizeof(bytes))
+            continue; /* more to read */
+        break;
     }
 
     posix_aio_process_queue(s);
@@ -509,6 +497,20 @@ 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;
+        ssize_t ret;
+
+        ret = write(posix_aio_state->wfd, &byte, sizeof(byte));
+        if (ret < 0 && errno != EAGAIN)
+            die("write()");
+    }
+
+    qemu_service_io();
+}
+
 static void paio_remove(struct qemu_paiocb *acb)
 {
     struct qemu_paiocb **pacb;
@@ -610,8 +612,9 @@ BlockDriverAIOCB *paio_ioctl(BlockDriver
 
 int paio_init(void)
 {
-    sigset_t mask;
+    struct sigaction act;
     PosixAioState *s;
+    int fds[2];
     int ret;
 
     if (posix_aio_state)
@@ -619,21 +622,24 @@ int paio_init(void)
 
     s = qemu_malloc(sizeof(PosixAioState));
 
-    /* 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 (qemu_pipe(fds) == -1) {
+        fprintf(stderr, "failed to create pipe\n");
         return -1;
     }
 
-    fcntl(s->fd, F_SETFL, O_NONBLOCK);
+    s->rfd = fds[0];
+    s->wfd = fds[1];
+
+    fcntl(s->rfd, F_SETFL, O_NONBLOCK);
+    fcntl(s->wfd, F_SETFL, O_NONBLOCK);
 
-    qemu_aio_set_fd_handler(s->fd, posix_aio_read, NULL, posix_aio_flush,
+    qemu_aio_set_fd_handler(s->rfd, posix_aio_read, NULL, posix_aio_flush,
         posix_aio_process_queue, s);
 
     ret = pthread_attr_init(&attr);
Index: qemu-kvm/qemu-kvm.c
===================================================================
--- qemu-kvm.orig/qemu-kvm.c
+++ qemu-kvm/qemu-kvm.c
@@ -1680,6 +1680,7 @@ int kvm_main_loop(void)
     sigemptyset(&mask);
     sigaddset(&mask, SIGIO);
     sigaddset(&mask, SIGALRM);
+    sigaddset(&mask, SIGUSR2);
     sigaddset(&mask, SIGBUS);
     sigprocmask(SIG_BLOCK, &mask, NULL);
 

Re: [patch 1/2] qemu-kvm: use usptream eventfd code

[Avi Kivity]

  On 09/06/2010 11:20 PM, Marcelo Tosatti wrote:
> Upstream code is equivalent.
>
> Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
>
> Index: qemu-kvm/cpus.c
> ===================================================================
> --- qemu-kvm.orig/cpus.c
> +++ qemu-kvm/cpus.c
> @@ -290,11 +290,6 @@ void qemu_notify_event(void)
>   {
>       CPUState *env = cpu_single_env;
>
> -    if (kvm_enabled()) {
> -        qemu_kvm_notify_work();
> -        return;
> -    }
> -
>       qemu_event_increment ();
>       if (env) {
>           cpu_exit(env);

qemu_event_increment() is indeed equivalent, but what about the rest?  
Are we guaranteed that cpu_single_env == NULL?

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

Re: [patch 1/2] qemu-kvm: use usptream eventfd code

[Marcelo Tosatti]

On Tue, Sep 07, 2010 at 11:21:32AM +0300, Avi Kivity wrote:
>  On 09/06/2010 11:20 PM, Marcelo Tosatti wrote:
> >Upstream code is equivalent.
> >
> >Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
> >
> >Index: qemu-kvm/cpus.c
> >===================================================================
> >--- qemu-kvm.orig/cpus.c
> >+++ qemu-kvm/cpus.c
> >@@ -290,11 +290,6 @@ void qemu_notify_event(void)
> >  {
> >      CPUState *env = cpu_single_env;
> >
> >-    if (kvm_enabled()) {
> >-        qemu_kvm_notify_work();
> >-        return;
> >-    }
> >-
> >      qemu_event_increment ();
> >      if (env) {
> >          cpu_exit(env);
> 
> qemu_event_increment() is indeed equivalent, but what about the
> rest?  Are we guaranteed that cpu_single_env == NULL?

No, its not NULL. But env->current is, so its fine.

Re: [patch 1/2] qemu-kvm: use usptream eventfd code

[Avi Kivity]

  On 09/07/2010 08:25 PM, Marcelo Tosatti wrote:
> On Tue, Sep 07, 2010 at 11:21:32AM +0300, Avi Kivity wrote:
>>   On 09/06/2010 11:20 PM, Marcelo Tosatti wrote:
>>> Upstream code is equivalent.
>>>
>>> Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
>>>
>>> Index: qemu-kvm/cpus.c
>>> ===================================================================
>>> --- qemu-kvm.orig/cpus.c
>>> +++ qemu-kvm/cpus.c
>>> @@ -290,11 +290,6 @@ void qemu_notify_event(void)
>>>   {
>>>       CPUState *env = cpu_single_env;
>>>
>>> -    if (kvm_enabled()) {
>>> -        qemu_kvm_notify_work();
>>> -        return;
>>> -    }
>>> -
>>>       qemu_event_increment ();
>>>       if (env) {
>>>           cpu_exit(env);
>> qemu_event_increment() is indeed equivalent, but what about the
>> rest?  Are we guaranteed that cpu_single_env == NULL?
> No, its not NULL. But env->current is, so its fine.

Ok, thanks.

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