From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:48270) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R8YJa-0001Td-Jn for qemu-devel@nongnu.org; Tue, 27 Sep 2011 10:07:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1R8YJU-0005br-KP for qemu-devel@nongnu.org; Tue, 27 Sep 2011 10:07:18 -0400 Received: from mail-vw0-f45.google.com ([209.85.212.45]:58117) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R8YJU-0005bg-HA for qemu-devel@nongnu.org; Tue, 27 Sep 2011 10:07:12 -0400 Received: by vws17 with SMTP id 17so7369403vws.4 for ; Tue, 27 Sep 2011 07:07:11 -0700 (PDT) Message-ID: <4E81D88B.4020504@codemonkey.ws> Date: Tue, 27 Sep 2011 09:07:07 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <4E78C42D.5030207@siemens.com> <20110921080600.GA9847@stefanha-thinkpad.localdomain> <4E80B50B.9000301@siemens.com> <4E80B55F.5020203@redhat.com> <4E80BFF3.8000907@us.ibm.com> <4E8190BE.3000801@redhat.com> <4E81D609.1060203@siemens.com> In-Reply-To: <4E81D609.1060203@siemens.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] Use qemu_eventfd for POSIX AIO List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Kiszka Cc: Kevin Wolf , Anthony Liguori , Stefan Hajnoczi , Marcelo Tosatti , qemu-devel , Avi Kivity 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 > --- > 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 > #endif > > -#ifdef CONFIG_EVENTFD > -#include > -#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 > +#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; > }