[PATCH] qemu-kvm: Switch POSIX compat AIO implementation to upstream

[PATCH] qemu-kvm: Switch POSIX compat AIO implementation to upstream

[Jan Kiszka]

Upstream's version is about to be signal-free and will stop handling
SIGUSR2 specially. So it's time to adopt its implementation, ie. switch
from signalfd to a pipe.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

This should help pulling upstream into qemu-kvm when "block: avoid
SIGUSR2" is merged. And will help merging further cleanups of this code
I'm working on.

 posix-aio-compat.c |   73 ++++++++++++++++++++++-----------------------------
 1 files changed, 32 insertions(+), 41 deletions(-)

diff --git a/posix-aio-compat.c b/posix-aio-compat.c
index d8ad9ef..d375b56 100644
--- a/posix-aio-compat.c
+++ b/posix-aio-compat.c
@@ -27,7 +27,6 @@
 #include "qemu-common.h"
 #include "trace.h"
 #include "block_int.h"
-#include "compatfd.h"
 
 #include "block/raw-posix-aio.h"
 
@@ -43,7 +42,6 @@ struct qemu_paiocb {
     int aio_niov;
     size_t aio_nbytes;
 #define aio_ioctl_cmd   aio_nbytes /* for QEMU_AIO_IOCTL */
-    int ev_signo;
     off_t aio_offset;
 
     QTAILQ_ENTRY(qemu_paiocb) node;
@@ -54,7 +52,7 @@ struct qemu_paiocb {
 };
 
 typedef struct PosixAioState {
-    int fd;
+    int rfd, wfd;
     struct qemu_paiocb *first_aio;
 } PosixAioState;
 
@@ -310,12 +308,10 @@ static ssize_t handle_aiocb_rw(struct qemu_paiocb *aiocb)
     return nbytes;
 }
 
+static void posix_aio_notify_event(void);
+
 static void *aio_thread(void *unused)
 {
-    pid_t pid;
-
-    pid = getpid();
-
     mutex_lock(&lock);
     pending_threads--;
     mutex_unlock(&lock);
@@ -382,7 +378,7 @@ static void *aio_thread(void *unused)
         aiocb->ret = ret;
         mutex_unlock(&lock);
 
-        if (kill(pid, aiocb->ev_signo)) die("kill failed");
+        posix_aio_notify_event();
     }
 
     cur_threads--;
@@ -524,29 +520,18 @@ static int posix_aio_process_queue(void *opaque)
 static void posix_aio_read(void *opaque)
 {
     PosixAioState *s = opaque;
-    union {
-        struct qemu_signalfd_siginfo siginfo;
-        char buf[128];
-    } sig;
-    size_t offset;
+    ssize_t len;
 
-    /* try to read from signalfd, don't freak out if we can't read anything */
-    offset = 0;
-    while (offset < 128) {
-        ssize_t len;
+    /* read all bytes from signal pipe */
+    for (;;) {
+        char bytes[16];
 
-        len = read(s->fd, sig.buf + offset, 128 - offset);
+        len = read(s->rfd, bytes, sizeof(bytes));
         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;
+            continue; /* try again */
+        if (len == sizeof(bytes))
+            continue; /* more to read */
+        break;
     }
 
     posix_aio_process_queue(s);
@@ -560,6 +545,16 @@ static int posix_aio_flush(void *opaque)
 
 static PosixAioState *posix_aio_state;
 
+static void posix_aio_notify_event(void)
+{
+    char byte = 0;
+    ssize_t ret;
+
+    ret = write(posix_aio_state->wfd, &byte, sizeof(byte));
+    if (ret < 0 && errno != EAGAIN)
+        die("write()");
+}
+
 static void paio_remove(struct qemu_paiocb *acb)
 {
     struct qemu_paiocb **pacb;
@@ -621,7 +616,6 @@ BlockDriverAIOCB *paio_submit(BlockDriverState *bs, int fd,
         return NULL;
     acb->aio_type = type;
     acb->aio_fildes = fd;
-    acb->ev_signo = SIGUSR2;
 
     if (qiov) {
         acb->aio_iov = qiov->iov;
@@ -649,7 +643,6 @@ BlockDriverAIOCB *paio_ioctl(BlockDriverState *bs, int fd,
         return NULL;
     acb->aio_type = QEMU_AIO_IOCTL;
     acb->aio_fildes = fd;
-    acb->ev_signo = SIGUSR2;
     acb->aio_offset = 0;
     acb->aio_ioctl_buf = buf;
     acb->aio_ioctl_cmd = req;
@@ -663,8 +656,8 @@ BlockDriverAIOCB *paio_ioctl(BlockDriverState *bs, int fd,
 
 int paio_init(void)
 {
-    sigset_t mask;
     PosixAioState *s;
+    int fds[2];
     int ret;
 
     if (posix_aio_state)
@@ -672,21 +665,19 @@ int paio_init(void)
 
     s = g_malloc(sizeof(PosixAioState));
 
-    /* Make sure to block AIO signal */
-    sigemptyset(&mask);
-    sigaddset(&mask, SIGUSR2);
-    sigprocmask(SIG_BLOCK, &mask, 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);
-- 
1.7.3.4

Re: [PATCH] qemu-kvm: Switch POSIX compat AIO implementation to upstream

[Stefan Hajnoczi]

On Tue, Sep 20, 2011 at 06:49:49PM +0200, Jan Kiszka wrote:
> Upstream's version is about to be signal-free and will stop handling
> SIGUSR2 specially. So it's time to adopt its implementation, ie. switch
> from signalfd to a pipe.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
> This should help pulling upstream into qemu-kvm when "block: avoid
> SIGUSR2" is merged. And will help merging further cleanups of this code
> I'm working on.
> 
>  posix-aio-compat.c |   73 ++++++++++++++++++++++-----------------------------
>  1 files changed, 32 insertions(+), 41 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

Perhaps qemu_eventfd() can be used in the future instead of an explicit
pipe.  Then Linux will do eventfd while other OSes will fall back to
pipes.

Stefan

Re: [PATCH] qemu-kvm: Switch POSIX compat AIO implementation to upstream

[Kevin Wolf]

Am 20.09.2011 18:49, schrieb Jan Kiszka:
> Upstream's version is about to be signal-free and will stop handling
> SIGUSR2 specially. So it's time to adopt its implementation, ie. switch
> from signalfd to a pipe.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
> This should help pulling upstream into qemu-kvm when "block: avoid
> SIGUSR2" is merged. And will help merging further cleanups of this code
> I'm working on.

Acked-by: Kevin Wolf <kwolf@redhat.com>

Re: [PATCH] qemu-kvm: Switch POSIX compat AIO implementation to upstream

[Avi Kivity]

On 09/20/2011 07:49 PM, Jan Kiszka wrote:
> Upstream's version is about to be signal-free and will stop handling
> SIGUSR2 specially. So it's time to adopt its implementation, ie. switch
> from signalfd to a pipe.
>

Applied, thanks.

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

Re: [PATCH] qemu-kvm: Switch POSIX compat AIO implementation to upstream

[Jan Kiszka]

On 2011-09-21 10:06, Stefan Hajnoczi wrote:
> On Tue, Sep 20, 2011 at 06:49:49PM +0200, Jan Kiszka wrote:
>> Upstream's version is about to be signal-free and will stop handling
>> SIGUSR2 specially. So it's time to adopt its implementation, ie. switch
>> from signalfd to a pipe.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>
>> This should help pulling upstream into qemu-kvm when "block: avoid
>> SIGUSR2" is merged. And will help merging further cleanups of this code
>> I'm working on.
>>
>>  posix-aio-compat.c |   73 ++++++++++++++++++++++-----------------------------
>>  1 files changed, 32 insertions(+), 41 deletions(-)
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> 
> Perhaps qemu_eventfd() can be used in the future instead of an explicit
> pipe.  Then Linux will do eventfd while other OSes will fall back to
> pipes.

Basically simpler code, or does this also have runtime benefits?

Jan

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

Re: [PATCH] qemu-kvm: Switch POSIX compat AIO implementation to upstream

[Avi Kivity]

On 09/26/2011 08:23 PM, Jan Kiszka wrote:
> >
> >  Perhaps qemu_eventfd() can be used in the future instead of an explicit
> >  pipe.  Then Linux will do eventfd while other OSes will fall back to
> >  pipes.
>
> Basically simpler code, or does this also have runtime benefits?
>

In corner cases, the completion can block on the write() with pipes, but 
not eventfd.

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

Re: [PATCH] qemu-kvm: Switch POSIX compat AIO implementation to upstream

[Anthony Liguori]

On 09/26/2011 12:24 PM, Avi Kivity wrote:
> On 09/26/2011 08:23 PM, Jan Kiszka wrote:
>> >
>> > Perhaps qemu_eventfd() can be used in the future instead of an explicit
>> > pipe. Then Linux will do eventfd while other OSes will fall back to
>> > pipes.
>>
>> Basically simpler code, or does this also have runtime benefits?
>>
>
> In corner cases, the completion can block on the write() with pipes, but not
> eventfd.

The pipe is O_NONBLOCK and the only thing the caller cares about is whether 
there is *some* data in the PIPE so it can happily ignore EAGAIN.

So the pipe implementation never blocks on write() either.  It may require 
multiple reads to drain the queue though whereas eventfd would only require a 
single read to drain the queue.

Regards,

Anthony Liguori

>

Re: [PATCH] qemu-kvm: Switch POSIX compat AIO implementation to upstream

[Avi Kivity]

On 09/26/2011 09:09 PM, Anthony Liguori wrote:
> On 09/26/2011 12:24 PM, Avi Kivity wrote:
>> On 09/26/2011 08:23 PM, Jan Kiszka wrote:
>>> >
>>> > Perhaps qemu_eventfd() can be used in the future instead of an 
>>> explicit
>>> > pipe. Then Linux will do eventfd while other OSes will fall back to
>>> > pipes.
>>>
>>> Basically simpler code, or does this also have runtime benefits?
>>>
>>
>> In corner cases, the completion can block on the write() with pipes, 
>> but not
>> eventfd.
>
> The pipe is O_NONBLOCK and the only thing the caller cares about is 
> whether there is *some* data in the PIPE so it can happily ignore EAGAIN.
>
> So the pipe implementation never blocks on write() either.  It may 
> require multiple reads to drain the queue though whereas eventfd would 
> only require a single read to drain the queue.
>

Oh, so switching to eventfd won't change much.  Still nice though IMO.

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