Re: KVM call minutes for Feb 1

[Jan Kiszka <jan.kiszka@siemens.com>]

On 2011-02-01 17:53, Anthony Liguori wrote:
> On 02/01/2011 10:36 AM, Jan Kiszka wrote:
>> On 2011-02-01 16:54, Chris Wright wrote:
>>    
>>> KVM upstream merge: status, plans, coordination
>>> - Jan has a git tree, consolidating
>>> - qemu-kvm io threading is still an issue
>>> - Anthony wants to just merge
>>>    - concerns with non-x86 arch and merge
>>>    - concerns with big-bang patch merge and following stability
>>> - post 0.14 conversion to glib mainloop, non-upstreamed qemu-kvm will be
>>>    a problem if it's not there by then
>>> - testing and nuances are still an issue (e.g. stefan berger's mmio read issue)
>>> - qemu-kvm still evolving, needs to get sync'd or it will keep diverging
>>> - 2 implementations of main init, cpu init, Jan has merged them into one
>>>    - qemu-kvm-x86.c file that's only a few hundred lines
>>> - review as one patch to see the fundamental difference
>>>      
>> More precisely, my current work flow is to pick some function(s), e.g.
>> kvm_cpu_exec/kvm_run, and start wondering "What needs to be done to
>> upstream so that qemu-kvm could use that implementation?". If they
>> differ, the reasons need to be understood and patched away, either by
>> fixing/enhancing upstream or simplifying qemu-kvm. Once the upstream
>> changes are merged back, a qemu-kvm patch is posted to switch to that
>> version.
>>
>> Any help will be welcome, either via review of my subtle regressions or
>> on resolving concrete differences.
>>
>> E.g. posix-aio-compat.c: Why does qemu-kvm differ here? If it's because
>> of its own iothread code, can we wrap that away or do we need to
>> consolidate the threading code first? Or do we need to fix something in
>> upstream?
>>    
> 
> I bet it's the eventfd thing.  It's arbitrary.  If you've got a small 
> diff post your series, I'd be happy to take a look at it and see what I 
> can explain.
> 

Looks like it's around signalfd and its emulation:

[git diff qemu/master..master posix-aio-compat.c]

diff --git a/posix-aio-compat.c b/posix-aio-compat.c
index fa5494d..0704064 100644
--- a/posix-aio-compat.c
+++ b/posix-aio-compat.c
@@ -28,6 +28,7 @@
 #include "qemu-common.h"
 #include "trace.h"
 #include "block_int.h"
+#include "compatfd.h"
 
 #include "block/raw-posix-aio.h"
 
@@ -55,7 +56,7 @@ struct qemu_paiocb {
 };
 
 typedef struct PosixAioState {
-    int rfd, wfd;
+    int fd;
     struct qemu_paiocb *first_aio;
 } PosixAioState;
 
@@ -474,18 +475,29 @@ static int posix_aio_process_queue(void *opaque)
 static void posix_aio_read(void *opaque)
 {
     PosixAioState *s = opaque;
-    ssize_t len;
+    union {
+        struct qemu_signalfd_siginfo siginfo;
+        char buf[128];
+    } sig;
+    size_t offset;
 
-    /* read all bytes from signal pipe */
-    for (;;) {
-        char bytes[16];
+    /* 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->rfd, bytes, sizeof(bytes));
+        len = read(s->fd, sig.buf + offset, 128 - offset);
         if (len == -1 && errno == EINTR)
-            continue; /* try again */
-        if (len == sizeof(bytes))
-            continue; /* more to read */
-        break;
+            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;
     }
 
     posix_aio_process_queue(s);
@@ -499,20 +511,6 @@ 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;
@@ -616,9 +614,8 @@ BlockDriverAIOCB *paio_ioctl(BlockDriverState *bs, int fd,
 
 int paio_init(void)
 {
-    struct sigaction act;
+    sigset_t mask;
     PosixAioState *s;
-    int fds[2];
     int ret;
 
     if (posix_aio_state)
@@ -626,24 +623,21 @@ int paio_init(void)
 
     s = qemu_malloc(sizeof(PosixAioState));
 
-    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);
+    /* Make sure to block AIO signal */
+    sigemptyset(&mask);
+    sigaddset(&mask, SIGUSR2);
+    sigprocmask(SIG_BLOCK, &mask, NULL);
 
     s->first_aio = NULL;
-    if (qemu_pipe(fds) == -1) {
-        fprintf(stderr, "failed to create pipe\n");
+    s->fd = qemu_signalfd(&mask);
+    if (s->fd == -1) {
+        fprintf(stderr, "failed to create signalfd\n");
         return -1;
     }
 
-    s->rfd = fds[0];
-    s->wfd = fds[1];
-
-    fcntl(s->rfd, F_SETFL, O_NONBLOCK);
-    fcntl(s->wfd, F_SETFL, O_NONBLOCK);
+    fcntl(s->fd, F_SETFL, O_NONBLOCK);
 
-    qemu_aio_set_fd_handler(s->rfd, posix_aio_read, NULL, posix_aio_flush,
+    qemu_aio_set_fd_handler(s->fd, posix_aio_read, NULL, posix_aio_flush,
         posix_aio_process_queue, s);
 
     ret = pthread_attr_init(&attr);


Jan

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