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

[Anthony Liguori <anthony@codemonkey.ws>]

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;
>       }