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<address@hidden>
> > > ---
> > >    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