[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

[Qemu-devel] [PATCH] Use qemu_eventfd for POSIX AIO

[Jan Kiszka]

Move qemu_eventfd unmodified to oslib-posix and use it for signaling
POSIX AIO completions. If native eventfd suport is available, this
avoids multiple read accesses to drain multiple pending signals. As
before we use a pipe if eventfd is not supported.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 os-posix.c         |   32 --------------------------------
 oslib-posix.c      |   32 +++++++++++++++++++++++++++++++-
 posix-aio-compat.c |   12 ++++++++----
 3 files changed, 39 insertions(+), 37 deletions(-)

diff --git a/os-posix.c b/os-posix.c
index dbf3b24..a918895 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -45,10 +45,6 @@
 #include <sys/syscall.h>
 #endif
 
-#ifdef CONFIG_EVENTFD
-#include <sys/eventfd.h>
-#endif
-
 static struct passwd *user_pwd;
 static const char *chroot_dir;
 static int daemonize;
@@ -333,34 +329,6 @@ void os_set_line_buffering(void)
     setvbuf(stdout, NULL, _IOLBF, 0);
 }
 
-/*
- * Creates an eventfd that looks like a pipe and has EFD_CLOEXEC set.
- */
-int qemu_eventfd(int fds[2])
-{
-#ifdef CONFIG_EVENTFD
-    int ret;
-
-    ret = eventfd(0, 0);
-    if (ret >= 0) {
-        fds[0] = ret;
-        qemu_set_cloexec(ret);
-        if ((fds[1] = dup(ret)) == -1) {
-            close(ret);
-            return -1;
-        }
-        qemu_set_cloexec(fds[1]);
-        return 0;
-    }
-
-    if (errno != ENOSYS) {
-        return -1;
-    }
-#endif
-
-    return qemu_pipe(fds);
-}
-
 int qemu_create_pidfile(const char *filename)
 {
     char buffer[128];
diff --git a/oslib-posix.c b/oslib-posix.c
index a304fb0..8ef7bd7 100644
--- a/oslib-posix.c
+++ b/oslib-posix.c
@@ -47,7 +47,9 @@ extern int daemon(int, int);
 #include "trace.h"
 #include "qemu_socket.h"
 
-
+#ifdef CONFIG_EVENTFD
+#include <sys/eventfd.h>
+#endif
 
 int qemu_daemon(int nochdir, int noclose)
 {
@@ -139,6 +141,34 @@ int qemu_pipe(int pipefd[2])
     return ret;
 }
 
+/*
+ * Creates an eventfd that looks like a pipe and has EFD_CLOEXEC set.
+ */
+int qemu_eventfd(int fds[2])
+{
+#ifdef CONFIG_EVENTFD
+    int ret;
+
+    ret = eventfd(0, 0);
+    if (ret >= 0) {
+        fds[0] = ret;
+        qemu_set_cloexec(ret);
+        if ((fds[1] = dup(ret)) == -1) {
+            close(ret);
+            return -1;
+        }
+        qemu_set_cloexec(fds[1]);
+        return 0;
+    }
+
+    if (errno != ENOSYS) {
+        return -1;
+    }
+#endif
+
+    return qemu_pipe(fds);
+}
+
 int qemu_utimensat(int dirfd, const char *path, const struct timespec *times,
                    int flags)
 {
diff --git a/posix-aio-compat.c b/posix-aio-compat.c
index d3c1174..2aa5ba3 100644
--- a/posix-aio-compat.c
+++ b/posix-aio-compat.c
@@ -521,7 +521,7 @@ static void posix_aio_read(void *opaque)
     PosixAioState *s = opaque;
     ssize_t len;
 
-    /* read all bytes from signal pipe */
+    /* read all bytes from eventfd or signal pipe */
     for (;;) {
         char bytes[16];
 
@@ -546,10 +546,14 @@ static PosixAioState *posix_aio_state;
 
 static void posix_aio_notify_event(void)
 {
-    char byte = 0;
+    /* Write 8 bytes to be compatible with eventfd.  */
+    static const uint64_t val = 1;
     ssize_t ret;
 
-    ret = write(posix_aio_state->wfd, &byte, sizeof(byte));
+    do {
+        ret = write(posix_aio_state->wfd, &val, sizeof(val));
+    } while (ret < 0 && errno == EINTR);
+
     if (ret < 0 && errno != EAGAIN)
         die("write()");
 }
@@ -665,7 +669,7 @@ int paio_init(void)
     s = g_malloc(sizeof(PosixAioState));
 
     s->first_aio = NULL;
-    if (qemu_pipe(fds) == -1) {
+    if (qemu_eventfd(fds) == -1) {
         fprintf(stderr, "failed to create pipe\n");
         return -1;
     }
-- 
1.7.3.4

Re: [Qemu-devel] [PATCH] Use qemu_eventfd for POSIX AIO

[Anthony Liguori]

On 09/27/2011 08:56 AM, Jan Kiszka wrote:
> Move qemu_eventfd unmodified to oslib-posix and use it for signaling
> POSIX AIO completions. If native eventfd suport is available, this
> avoids multiple read accesses to drain multiple pending signals. As
> before we use a pipe if eventfd is not supported.
>
> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
> ---
>   os-posix.c         |   32 --------------------------------
>   oslib-posix.c      |   32 +++++++++++++++++++++++++++++++-
>   posix-aio-compat.c |   12 ++++++++----
>   3 files changed, 39 insertions(+), 37 deletions(-)
>
> diff --git a/os-posix.c b/os-posix.c
> index dbf3b24..a918895 100644
> --- a/os-posix.c
> +++ b/os-posix.c
> @@ -45,10 +45,6 @@
>   #include<sys/syscall.h>
>   #endif
>
> -#ifdef CONFIG_EVENTFD
> -#include<sys/eventfd.h>
> -#endif
> -
>   static struct passwd *user_pwd;
>   static const char *chroot_dir;
>   static int daemonize;
> @@ -333,34 +329,6 @@ void os_set_line_buffering(void)
>       setvbuf(stdout, NULL, _IOLBF, 0);
>   }
>
> -/*
> - * Creates an eventfd that looks like a pipe and has EFD_CLOEXEC set.
> - */
> -int qemu_eventfd(int fds[2])
> -{
> -#ifdef CONFIG_EVENTFD
> -    int ret;
> -
> -    ret = eventfd(0, 0);
> -    if (ret>= 0) {
> -        fds[0] = ret;
> -        qemu_set_cloexec(ret);
> -        if ((fds[1] = dup(ret)) == -1) {
> -            close(ret);
> -            return -1;
> -        }
> -        qemu_set_cloexec(fds[1]);
> -        return 0;
> -    }
> -
> -    if (errno != ENOSYS) {
> -        return -1;
> -    }
> -#endif
> -
> -    return qemu_pipe(fds);
> -}
> -
>   int qemu_create_pidfile(const char *filename)
>   {
>       char buffer[128];
> diff --git a/oslib-posix.c b/oslib-posix.c
> index a304fb0..8ef7bd7 100644
> --- a/oslib-posix.c
> +++ b/oslib-posix.c
> @@ -47,7 +47,9 @@ extern int daemon(int, int);
>   #include "trace.h"
>   #include "qemu_socket.h"
>
> -
> +#ifdef CONFIG_EVENTFD
> +#include<sys/eventfd.h>
> +#endif
>
>   int qemu_daemon(int nochdir, int noclose)
>   {
> @@ -139,6 +141,34 @@ int qemu_pipe(int pipefd[2])
>       return ret;
>   }
>
> +/*
> + * Creates an eventfd that looks like a pipe and has EFD_CLOEXEC set.
> + */
> +int qemu_eventfd(int fds[2])
> +{
> +#ifdef CONFIG_EVENTFD
> +    int ret;
> +
> +    ret = eventfd(0, 0);
> +    if (ret>= 0) {
> +        fds[0] = ret;
> +        qemu_set_cloexec(ret);
> +        if ((fds[1] = dup(ret)) == -1) {
> +            close(ret);
> +            return -1;
> +        }
> +        qemu_set_cloexec(fds[1]);
> +        return 0;
> +    }
> +
> +    if (errno != ENOSYS) {
> +        return -1;
> +    }
> +#endif
> +
> +    return qemu_pipe(fds);
> +}
> +

I think it's a bit dangerous to implement eventfd() in terms of pipe().

You don't expect to handle EAGAIN with eventfd() whereas you have to handle it 
with pipe().

Moreover, the eventfd() counter is not lossy (practically speaking) whereas if 
you use pipe() as a counter, it will be lossy in practice.

This is why posix aio uses pipe() and not eventfd().

Regards,

Anthony Liguori

>   int qemu_utimensat(int dirfd, const char *path, const struct timespec *times,
>                      int flags)
>   {
> diff --git a/posix-aio-compat.c b/posix-aio-compat.c
> index d3c1174..2aa5ba3 100644
> --- a/posix-aio-compat.c
> +++ b/posix-aio-compat.c
> @@ -521,7 +521,7 @@ static void posix_aio_read(void *opaque)
>       PosixAioState *s = opaque;
>       ssize_t len;
>
> -    /* read all bytes from signal pipe */
> +    /* read all bytes from eventfd or signal pipe */
>       for (;;) {
>           char bytes[16];
>
> @@ -546,10 +546,14 @@ static PosixAioState *posix_aio_state;
>
>   static void posix_aio_notify_event(void)
>   {
> -    char byte = 0;
> +    /* Write 8 bytes to be compatible with eventfd.  */
> +    static const uint64_t val = 1;
>       ssize_t ret;
>
> -    ret = write(posix_aio_state->wfd,&byte, sizeof(byte));
> +    do {
> +        ret = write(posix_aio_state->wfd,&val, sizeof(val));
> +    } while (ret<  0&&  errno == EINTR);
> +
>       if (ret<  0&&  errno != EAGAIN)
>           die("write()");
>   }
> @@ -665,7 +669,7 @@ int paio_init(void)
>       s = g_malloc(sizeof(PosixAioState));
>
>       s->first_aio = NULL;
> -    if (qemu_pipe(fds) == -1) {
> +    if (qemu_eventfd(fds) == -1) {
>           fprintf(stderr, "failed to create pipe\n");
>           return -1;
>       }

Re: [Qemu-devel] [PATCH] Use qemu_eventfd for POSIX AIO

[Avi Kivity]

On 09/27/2011 05:07 PM, Anthony Liguori wrote:
>
> You don't expect to handle EAGAIN with eventfd() whereas you have to 
> handle it with pipe().
>
> Moreover, the eventfd() counter is not lossy (practically speaking) 
> whereas if you use pipe() as a counter, it will be lossy in practice.
>
> This is why posix aio uses pipe() and not eventfd().

We could define a qemu_event mechanism that satisfies the least common 
denominator, and is implemented by eventfd when available.

qemu_event_create()
qemu_event_signal()
qemu_event_wait()
qemu_event_poll_add() // registers in main loop
qemu_event_poll_del()

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

Re: [Qemu-devel] [PATCH] Use qemu_eventfd for POSIX AIO

[Anthony Liguori]

On 09/27/2011 09:11 AM, Avi Kivity wrote:
> On 09/27/2011 05:07 PM, Anthony Liguori wrote:
>>
>> You don't expect to handle EAGAIN with eventfd() whereas you have to handle it
>> with pipe().
>>
>> Moreover, the eventfd() counter is not lossy (practically speaking) whereas if
>> you use pipe() as a counter, it will be lossy in practice.
>>
>> This is why posix aio uses pipe() and not eventfd().
>
> We could define a qemu_event mechanism that satisfies the least common
> denominator, and is implemented by eventfd when available.
>
> qemu_event_create()
> qemu_event_signal()
> qemu_event_wait()
> qemu_event_poll_add() // registers in main loop
> qemu_event_poll_del()

See hw/event_notifier.[ch].

Regards,

Anthony Liguori

>

Re: [Qemu-devel] [PATCH] Use qemu_eventfd for POSIX AIO

[Avi Kivity]

On 09/27/2011 05:19 PM, Anthony Liguori wrote:
>> qemu_event_create()
>> qemu_event_signal()
>> qemu_event_wait()
>> qemu_event_poll_add() // registers in main loop
>> qemu_event_poll_del()
>
>
> See hw/event_notifier.[ch].

Precisely.

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

Re: [Qemu-devel] [PATCH] Use qemu_eventfd for POSIX AIO

[Jan Kiszka]

On 2011-09-27 16:19, Anthony Liguori wrote:
> On 09/27/2011 09:11 AM, Avi Kivity wrote:
>> On 09/27/2011 05:07 PM, Anthony Liguori wrote:
>>>
>>> You don't expect to handle EAGAIN with eventfd() whereas you have to handle it
>>> with pipe().
>>>
>>> Moreover, the eventfd() counter is not lossy (practically speaking) whereas if
>>> you use pipe() as a counter, it will be lossy in practice.
>>>
>>> This is why posix aio uses pipe() and not eventfd().
>>
>> We could define a qemu_event mechanism that satisfies the least common
>> denominator, and is implemented by eventfd when available.
>>
>> qemu_event_create()
>> qemu_event_signal()
>> qemu_event_wait()
>> qemu_event_poll_add() // registers in main loop
>> qemu_event_poll_del()
> 
> See hw/event_notifier.[ch].

That code looks suspicious, btw. It claims things ("we use
EFD_SEMAPHORE") it does not do.

Jan

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

Re: [Qemu-devel] [PATCH] Use qemu_eventfd for POSIX AIO

[Jan Kiszka]

On 2011-09-27 16:07, Anthony Liguori wrote:
> On 09/27/2011 08:56 AM, Jan Kiszka wrote:
>> Move qemu_eventfd unmodified to oslib-posix and use it for signaling
>> POSIX AIO completions. If native eventfd suport is available, this
>> avoids multiple read accesses to drain multiple pending signals. As
>> before we use a pipe if eventfd is not supported.
>>
>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>> ---
>>   os-posix.c         |   32 --------------------------------
>>   oslib-posix.c      |   32 +++++++++++++++++++++++++++++++-
>>   posix-aio-compat.c |   12 ++++++++----
>>   3 files changed, 39 insertions(+), 37 deletions(-)
>>
>> diff --git a/os-posix.c b/os-posix.c
>> index dbf3b24..a918895 100644
>> --- a/os-posix.c
>> +++ b/os-posix.c
>> @@ -45,10 +45,6 @@
>>   #include<sys/syscall.h>
>>   #endif
>>
>> -#ifdef CONFIG_EVENTFD
>> -#include<sys/eventfd.h>
>> -#endif
>> -
>>   static struct passwd *user_pwd;
>>   static const char *chroot_dir;
>>   static int daemonize;
>> @@ -333,34 +329,6 @@ void os_set_line_buffering(void)
>>       setvbuf(stdout, NULL, _IOLBF, 0);
>>   }
>>
>> -/*
>> - * Creates an eventfd that looks like a pipe and has EFD_CLOEXEC set.
>> - */
>> -int qemu_eventfd(int fds[2])
>> -{
>> -#ifdef CONFIG_EVENTFD
>> -    int ret;
>> -
>> -    ret = eventfd(0, 0);
>> -    if (ret>= 0) {
>> -        fds[0] = ret;
>> -        qemu_set_cloexec(ret);
>> -        if ((fds[1] = dup(ret)) == -1) {
>> -            close(ret);
>> -            return -1;
>> -        }
>> -        qemu_set_cloexec(fds[1]);
>> -        return 0;
>> -    }
>> -
>> -    if (errno != ENOSYS) {
>> -        return -1;
>> -    }
>> -#endif
>> -
>> -    return qemu_pipe(fds);
>> -}
>> -
>>   int qemu_create_pidfile(const char *filename)
>>   {
>>       char buffer[128];
>> diff --git a/oslib-posix.c b/oslib-posix.c
>> index a304fb0..8ef7bd7 100644
>> --- a/oslib-posix.c
>> +++ b/oslib-posix.c
>> @@ -47,7 +47,9 @@ extern int daemon(int, int);
>>   #include "trace.h"
>>   #include "qemu_socket.h"
>>
>> -
>> +#ifdef CONFIG_EVENTFD
>> +#include<sys/eventfd.h>
>> +#endif
>>
>>   int qemu_daemon(int nochdir, int noclose)
>>   {
>> @@ -139,6 +141,34 @@ int qemu_pipe(int pipefd[2])
>>       return ret;
>>   }
>>
>> +/*
>> + * Creates an eventfd that looks like a pipe and has EFD_CLOEXEC set.
>> + */
>> +int qemu_eventfd(int fds[2])
>> +{
>> +#ifdef CONFIG_EVENTFD
>> +    int ret;
>> +
>> +    ret = eventfd(0, 0);
>> +    if (ret>= 0) {
>> +        fds[0] = ret;
>> +        qemu_set_cloexec(ret);
>> +        if ((fds[1] = dup(ret)) == -1) {
>> +            close(ret);
>> +            return -1;
>> +        }
>> +        qemu_set_cloexec(fds[1]);
>> +        return 0;
>> +    }
>> +
>> +    if (errno != ENOSYS) {
>> +        return -1;
>> +    }
>> +#endif
>> +
>> +    return qemu_pipe(fds);
>> +}
>> +
> 
> I think it's a bit dangerous to implement eventfd() in terms of pipe().
> 
> You don't expect to handle EAGAIN with eventfd() whereas you have to handle it 
> with pipe().

EAGAIN is returned on eventfd read if no event is pending and the fd is
non-blocking - just as we configure it.

> 
> Moreover, the eventfd() counter is not lossy (practically speaking) whereas if 
> you use pipe() as a counter, it will be lossy in practice.
> 
> This is why posix aio uses pipe() and not eventfd().

I don't get this yet. eventfd is lossy by default. It only decreases the
counter on read if you specify EFD_SEMAPHORE - which we do not do.

Jan

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

Re: [Qemu-devel] [PATCH] Use qemu_eventfd for POSIX AIO

[Avi Kivity]

On 09/27/2011 05:29 PM, Jan Kiszka wrote:
> >
> >  Moreover, the eventfd() counter is not lossy (practically speaking) whereas if
> >  you use pipe() as a counter, it will be lossy in practice.
> >
> >  This is why posix aio uses pipe() and not eventfd().
>
> I don't get this yet. eventfd is lossy by default. It only decreases the
> counter on read if you specify EFD_SEMAPHORE - which we do not do.
>

It's not lossy - a read returns the number of events written since the 
last read.

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

Re: [Qemu-devel] [PATCH] Use qemu_eventfd for POSIX AIO

[Anthony Liguori]

On 09/27/2011 09:29 AM, Jan Kiszka wrote:
> On 2011-09-27 16:07, Anthony Liguori wrote:
>> On 09/27/2011 08:56 AM, Jan Kiszka wrote:
>>> Move qemu_eventfd unmodified to oslib-posix and use it for signaling
>>> POSIX AIO completions. If native eventfd suport is available, this
>>> avoids multiple read accesses to drain multiple pending signals. As
>>> before we use a pipe if eventfd is not supported.
>>>
>>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>>> ---
>>>    os-posix.c         |   32 --------------------------------
>>>    oslib-posix.c      |   32 +++++++++++++++++++++++++++++++-
>>>    posix-aio-compat.c |   12 ++++++++----
>>>    3 files changed, 39 insertions(+), 37 deletions(-)
>>>
>>> diff --git a/os-posix.c b/os-posix.c
>>> index dbf3b24..a918895 100644
>>> --- a/os-posix.c
>>> +++ b/os-posix.c
>>> @@ -45,10 +45,6 @@
>>>    #include<sys/syscall.h>
>>>    #endif
>>>
>>> -#ifdef CONFIG_EVENTFD
>>> -#include<sys/eventfd.h>
>>> -#endif
>>> -
>>>    static struct passwd *user_pwd;
>>>    static const char *chroot_dir;
>>>    static int daemonize;
>>> @@ -333,34 +329,6 @@ void os_set_line_buffering(void)
>>>        setvbuf(stdout, NULL, _IOLBF, 0);
>>>    }
>>>
>>> -/*
>>> - * Creates an eventfd that looks like a pipe and has EFD_CLOEXEC set.
>>> - */
>>> -int qemu_eventfd(int fds[2])
>>> -{
>>> -#ifdef CONFIG_EVENTFD
>>> -    int ret;
>>> -
>>> -    ret = eventfd(0, 0);
>>> -    if (ret>= 0) {
>>> -        fds[0] = ret;
>>> -        qemu_set_cloexec(ret);
>>> -        if ((fds[1] = dup(ret)) == -1) {
>>> -            close(ret);
>>> -            return -1;
>>> -        }
>>> -        qemu_set_cloexec(fds[1]);
>>> -        return 0;
>>> -    }
>>> -
>>> -    if (errno != ENOSYS) {
>>> -        return -1;
>>> -    }
>>> -#endif
>>> -
>>> -    return qemu_pipe(fds);
>>> -}
>>> -
>>>    int qemu_create_pidfile(const char *filename)
>>>    {
>>>        char buffer[128];
>>> diff --git a/oslib-posix.c b/oslib-posix.c
>>> index a304fb0..8ef7bd7 100644
>>> --- a/oslib-posix.c
>>> +++ b/oslib-posix.c
>>> @@ -47,7 +47,9 @@ extern int daemon(int, int);
>>>    #include "trace.h"
>>>    #include "qemu_socket.h"
>>>
>>> -
>>> +#ifdef CONFIG_EVENTFD
>>> +#include<sys/eventfd.h>
>>> +#endif
>>>
>>>    int qemu_daemon(int nochdir, int noclose)
>>>    {
>>> @@ -139,6 +141,34 @@ int qemu_pipe(int pipefd[2])
>>>        return ret;
>>>    }
>>>
>>> +/*
>>> + * Creates an eventfd that looks like a pipe and has EFD_CLOEXEC set.
>>> + */
>>> +int qemu_eventfd(int fds[2])
>>> +{
>>> +#ifdef CONFIG_EVENTFD
>>> +    int ret;
>>> +
>>> +    ret = eventfd(0, 0);
>>> +    if (ret>= 0) {
>>> +        fds[0] = ret;
>>> +        qemu_set_cloexec(ret);
>>> +        if ((fds[1] = dup(ret)) == -1) {
>>> +            close(ret);
>>> +            return -1;
>>> +        }
>>> +        qemu_set_cloexec(fds[1]);
>>> +        return 0;
>>> +    }
>>> +
>>> +    if (errno != ENOSYS) {
>>> +        return -1;
>>> +    }
>>> +#endif
>>> +
>>> +    return qemu_pipe(fds);
>>> +}
>>> +
>>
>> I think it's a bit dangerous to implement eventfd() in terms of pipe().
>>
>> You don't expect to handle EAGAIN with eventfd() whereas you have to handle it
>> with pipe().
>
> EAGAIN is returned on eventfd read if no event is pending and the fd is
> non-blocking - just as we configure it.
>
>>
>> Moreover, the eventfd() counter is not lossy (practically speaking) whereas if
>> you use pipe() as a counter, it will be lossy in practice.
>>
>> This is why posix aio uses pipe() and not eventfd().
>
> I don't get this yet. eventfd is lossy by default. It only decreases the
> counter on read if you specify EFD_SEMAPHORE - which we do not do.

uint64_t value;

for (i = 0; i < 1 << 32; i++) {
    value = 1;
    write(fd, &value, sizeof(value));
}

uint64_t count = 0;

do {
    len = read(fd, &value, sizeof(value));
    count += value;
} while (len != -1);

With eventfd, count == 2^32.  With pipe, count == 8192.

That's each '1' is stored in the pipe buffer whereas with eventfd, an index is 
just incremented.

Not sure what you mean re: EFD_SEMAPHORE.  EFD_SEMAPHORE basically means any 
non-zero value is returned as 1 and the counter is decremented by 1.  Without 
EFD_SEMAPHORE, the count is returned and the counter is reset to 0.

Regards,

Anthony Liguori

>
> Jan
>

Re: [Qemu-devel] [PATCH] Use qemu_eventfd for POSIX AIO

[Jan Kiszka]

On 2011-09-27 16:34, Avi Kivity wrote:
> On 09/27/2011 05:29 PM, Jan Kiszka wrote:
>>>
>>>  Moreover, the eventfd() counter is not lossy (practically speaking) whereas if
>>>  you use pipe() as a counter, it will be lossy in practice.
>>>
>>>  This is why posix aio uses pipe() and not eventfd().
>>
>> I don't get this yet. eventfd is lossy by default. It only decreases the
>> counter on read if you specify EFD_SEMAPHORE - which we do not do.
>>
> 
> It's not lossy - a read returns the number of events written since the 
> last read.

Yeah, but what's the point? We don't evaluate this.

Jan

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

Re: [Qemu-devel] [PATCH] Use qemu_eventfd for POSIX AIO

[Anthony Liguori]

On 09/27/2011 09:22 AM, Jan Kiszka wrote:
> On 2011-09-27 16:19, Anthony Liguori wrote:
>> On 09/27/2011 09:11 AM, Avi Kivity wrote:
>>> On 09/27/2011 05:07 PM, Anthony Liguori wrote:
>>>>
>>>> You don't expect to handle EAGAIN with eventfd() whereas you have to handle it
>>>> with pipe().
>>>>
>>>> Moreover, the eventfd() counter is not lossy (practically speaking) whereas if
>>>> you use pipe() as a counter, it will be lossy in practice.
>>>>
>>>> This is why posix aio uses pipe() and not eventfd().
>>>
>>> We could define a qemu_event mechanism that satisfies the least common
>>> denominator, and is implemented by eventfd when available.
>>>
>>> qemu_event_create()
>>> qemu_event_signal()
>>> qemu_event_wait()
>>> qemu_event_poll_add() // registers in main loop
>>> qemu_event_poll_del()
>>
>> See hw/event_notifier.[ch].
>
> That code looks suspicious, btw. It claims things ("we use
> EFD_SEMAPHORE") it does not do.

Indeed.

But the interface appears to be the right one IMHO.

Regards,

Anthony Liguori

>
> Jan
>

Re: [Qemu-devel] [PATCH] Use qemu_eventfd for POSIX AIO

[Paolo Bonzini]

On 09/27/2011 04:07 PM, Anthony Liguori wrote:
>
> I think it's a bit dangerous to implement eventfd() in terms of pipe().
>
> You don't expect to handle EAGAIN with eventfd() whereas you have to
> handle it with pipe().
>
> Moreover, the eventfd() counter is not lossy (practically speaking)
> whereas if you use pipe() as a counter, it will be lossy in practice.
>
> This is why posix aio uses pipe() and not eventfd().

But this is the same idiom we use for the iothread signaling.  We're not 
using the eventfd's counter.  Perhaps it would be nice to complete 
EventNotifier with "notify event" methods and use it, but Jan's patch is 
safe, I think.

Paolo

Re: [Qemu-devel] [PATCH] Use qemu_eventfd for POSIX AIO

[Avi Kivity]

On 09/27/2011 05:36 PM, Jan Kiszka wrote:
> On 2011-09-27 16:34, Avi Kivity wrote:
> >  On 09/27/2011 05:29 PM, Jan Kiszka wrote:
> >>>
> >>>   Moreover, the eventfd() counter is not lossy (practically speaking) whereas if
> >>>   you use pipe() as a counter, it will be lossy in practice.
> >>>
> >>>   This is why posix aio uses pipe() and not eventfd().
> >>
> >>  I don't get this yet. eventfd is lossy by default. It only decreases the
> >>  counter on read if you specify EFD_SEMAPHORE - which we do not do.
> >>
> >
> >  It's not lossy - a read returns the number of events written since the
> >  last read.
>
> Yeah, but what's the point? We don't evaluate this.
>
>

If we write an interface that looks like eventfd but subtly differs, 
someone will get bitten.  If we write a new interface and implement it 
via eventfd (or a pipe), no one gets bitten.

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

Re: [Qemu-devel] [PATCH] Use qemu_eventfd for POSIX AIO

[Jan Kiszka]

On 2011-09-27 16:42, Avi Kivity wrote:
> On 09/27/2011 05:36 PM, Jan Kiszka wrote:
>> On 2011-09-27 16:34, Avi Kivity wrote:
>>>  On 09/27/2011 05:29 PM, Jan Kiszka wrote:
>>>>>
>>>>>   Moreover, the eventfd() counter is not lossy (practically speaking) whereas if
>>>>>   you use pipe() as a counter, it will be lossy in practice.
>>>>>
>>>>>   This is why posix aio uses pipe() and not eventfd().
>>>>
>>>>  I don't get this yet. eventfd is lossy by default. It only decreases the
>>>>  counter on read if you specify EFD_SEMAPHORE - which we do not do.
>>>>
>>>
>>>  It's not lossy - a read returns the number of events written since the
>>>  last read.
>>
>> Yeah, but what's the point? We don't evaluate this.
>>
>>
> 
> If we write an interface that looks like eventfd but subtly differs, 
> someone will get bitten.  If we write a new interface and implement it 
> via eventfd (or a pipe), no one gets bitten.

I don't disagree that there is still room for improving the existing
interface, generalizing to qemu_event. But that's not in the scope of
this patch.

Jan

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

Re: [Qemu-devel] [PATCH] Use qemu_eventfd for POSIX AIO

[Avi Kivity]

On 09/27/2011 05:45 PM, Jan Kiszka wrote:
> I don't disagree that there is still room for improving the existing
> interface, generalizing to qemu_event. But that's not in the scope of
> this patch.
>

Why not use event_notify.c?

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

Re: [Qemu-devel] [PATCH] Use qemu_eventfd for POSIX AIO

[Jan Kiszka]

On 2011-09-27 16:48, Avi Kivity wrote:
> On 09/27/2011 05:45 PM, Jan Kiszka wrote:
>> I don't disagree that there is still room for improving the existing
>> interface, generalizing to qemu_event. But that's not in the scope of
>> this patch.
>>
> 
> Why not use event_notify.c?

It doesn't solve the wrapping issue, it mandates eventfd support.

Jan

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

Re: [Qemu-devel] [PATCH] Use qemu_eventfd for POSIX AIO

[Avi Kivity]

On 09/27/2011 05:50 PM, Jan Kiszka wrote:
> On 2011-09-27 16:48, Avi Kivity wrote:
> >  On 09/27/2011 05:45 PM, Jan Kiszka wrote:
> >>  I don't disagree that there is still room for improving the existing
> >>  interface, generalizing to qemu_event. But that's not in the scope of
> >>  this patch.
> >>
> >
> >  Why not use event_notify.c?
>
> It doesn't solve the wrapping issue, it mandates eventfd support.
>

We can add pipe fallbacks too (though if it was meant to use with vhost, 
that's not what we want).

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

Re: [Qemu-devel] [PATCH] Use qemu_eventfd for POSIX AIO

[Anthony Liguori]

On 09/27/2011 09:54 AM, Avi Kivity wrote:
> On 09/27/2011 05:50 PM, Jan Kiszka wrote:
>> On 2011-09-27 16:48, Avi Kivity wrote:
>> > On 09/27/2011 05:45 PM, Jan Kiszka wrote:
>> >> I don't disagree that there is still room for improving the existing
>> >> interface, generalizing to qemu_event. But that's not in the scope of
>> >> this patch.
>> >>
>> >
>> > Why not use event_notify.c?
>>
>> It doesn't solve the wrapping issue, it mandates eventfd support.
>>
>
> We can add pipe fallbacks too (though if it was meant to use with vhost, that's
> not what we want).

vhost cannot exist on a kernel that doesn't support eventfd so I don't think we 
need to worry about it.

Regards,

Anthony Liguori

>

Re: [Qemu-devel] [PATCH] Use qemu_eventfd for POSIX AIO

[Jan Kiszka]

On 2011-09-27 16:54, Avi Kivity wrote:
> On 09/27/2011 05:50 PM, Jan Kiszka wrote:
>> On 2011-09-27 16:48, Avi Kivity wrote:
>>>  On 09/27/2011 05:45 PM, Jan Kiszka wrote:
>>>>  I don't disagree that there is still room for improving the existing
>>>>  interface, generalizing to qemu_event. But that's not in the scope of
>>>>  this patch.
>>>>
>>>
>>>  Why not use event_notify.c?
>>
>> It doesn't solve the wrapping issue, it mandates eventfd support.
>>
> 
> We can add pipe fallbacks too (though if it was meant to use with vhost, 
> that's not what we want).

Not a practical issue due to the dependency on much more recent vhost.

So EventNotifier will have to be migrated over a generic solution as
well. Again, that's food for additional patches.

Jan

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