qemu-kvm: Switch POSIX compat AIO implementation to upstream

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 <[email protected]>

---

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(-)

Quoted 11 lines · 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 <[email protected]>

---

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.

Quoted 2 lines

 posix-aio-compat.c |   73 ++++++++++++++++++++++-----------------------------
 1 files changed, 32 insertions(+), 41 deletions(-)

Reviewed-by Stefan Hajnoczi <[email protected]>

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.

Quoted 4 lines

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?

Quoted 7 lines · Jan Kiszka

Quoted 4 lines

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.

Quoted 10 lines · Avi Kivity

Quoted 8 lines · Jan Kiszka

Quoted 2 lines

Perhaps qemu_eventfd() can be used in the future instead of an

explicit

Quoted 2 lines

pipe. Then Linux will do eventfd while other OSes will fall back to pipes.

Basically simpler code, or does this also have runtime benefits?

Quoted 2 lines

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.

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<[email protected]>

---
  os-posix.c         |   32 --------------------------------
  oslib-posix.c      |   32 +++++++++++++++++++++++++++++++-
  posix-aio-compat.c |   12 ++++++++----
  3 files changed, 39 insertions(+), 37 deletions(-)

os-posix.c +0 -32

index dbf3b24..a918895 100644
@@ -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];

oslib-posix.c +21 -1

index a304fb0..8ef7bd7 100644
@@ -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;

oslib-posix.c +0 -0

  int qemu_utimensat(int dirfd, const char *path, const struct timespec *times,
                     int flags)
  {

posix-aio-compat.c +8 -4

index d3c1174..2aa5ba3 100644
@@ -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;
      }

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().

Quoted 8 lines · Anthony Liguori

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()

Quoted 5 lines

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].

Quoted 13 lines · Avi Kivity

Quoted 8 lines · Anthony Liguori

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.

Quoted 5 lines

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].

Quoted 20 lines · Anthony Liguori

Quoted 18 lines · Avi Kivity

Quoted 8 lines · Anthony Liguori

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()

Quoted 1 line

See hw/event_notifier.[ch].

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

Jan

Quoted 99 lines · 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<[email protected]>

---
  os-posix.c         |   32 --------------------------------
  oslib-posix.c      |   32 +++++++++++++++++++++++++++++++-
  posix-aio-compat.c |   12 ++++++++----
  3 files changed, 39 insertions(+), 37 deletions(-)

os-posix.c +0 -32

index dbf3b24..a918895 100644
@@ -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];

oslib-posix.c +21 -1

index a304fb0..8ef7bd7 100644
@@ -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;

oslib-posix.c int qemu_pipe(int pipefd[2]) · line 171

Quoted 10 lines

oslib-posix.c +10 -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().

Quoted 5 lines

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.

Quoted 4 lines · Jan Kiszka

Quoted 4 lines

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

Quoted 5 lines

Quoted 1 line

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.

Quoted 7 lines · Avi Kivity

Quoted 6 lines · Jan Kiszka

Quoted 5 lines

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().

Quoted 6 lines

Quoted 3 lines

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.

Quoted 15 lines · Jan Kiszka

Quoted 13 lines · Avi Kivity

Quoted 9 lines · Jan Kiszka

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 4 lines · Jan Kiszka

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?

Quoted 7 lines · Avi Kivity

Quoted 4 lines · Jan Kiszka

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.

Quoted 7 lines · Jan Kiszka

Quoted 6 lines · Avi Kivity

Quoted 4 lines · Jan Kiszka

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.

Quoted 4 lines

Quoted 1 line

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).

Quoted 7 lines · Jan Kiszka

Quoted 6 lines · Avi Kivity

Quoted 4 lines · Jan Kiszka

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.

Quoted 4 lines

Quoted 1 line

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).

Quoted 113 lines · Anthony Liguori

Quoted 99 lines · 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<[email protected]>

---
   os-posix.c         |   32 --------------------------------
   oslib-posix.c      |   32 +++++++++++++++++++++++++++++++-
   posix-aio-compat.c |   12 ++++++++----
   3 files changed, 39 insertions(+), 37 deletions(-)

os-posix.c +0 -32

index dbf3b24..a918895 100644
@@ -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];

oslib-posix.c +21 -1

index a304fb0..8ef7bd7 100644
@@ -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;

oslib-posix.c int qemu_pipe(int pipefd[2]) · line 171

Quoted 10 lines

oslib-posix.c +10 -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().

Quoted 2 lines

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.

Quoted 1 line

Quoted 4 lines

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

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().

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 <[email protected]>

---

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.

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.