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

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

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