Re: [PATCH v5 08/50] multi-process: add functions to synchronize proxy and remote endpoints

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Jagannathan Raman (jag.raman@oracle.com) wrote:
> In some cases, for example MMIO read, QEMU has to wait for the remote to
> complete a command before proceeding. An eventfd based mechanism is
> added to synchronize QEMU & remote process.
> 
> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> ---
>  include/io/mpqemu-link.h |  7 +++++++
>  io/mpqemu-link.c         | 41 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 48 insertions(+)
> 
> diff --git a/include/io/mpqemu-link.h b/include/io/mpqemu-link.h
> index 2f2dd83..ae04fca 100644
> --- a/include/io/mpqemu-link.h
> +++ b/include/io/mpqemu-link.h
> @@ -135,4 +135,11 @@ void mpqemu_link_set_callback(MPQemuLinkState *s,
>                                mpqemu_link_callback callback);
>  void mpqemu_start_coms(MPQemuLinkState *s);
>  
> +#define GET_REMOTE_WAIT eventfd(0, EFD_CLOEXEC)
> +#define PUT_REMOTE_WAIT(wait) close(wait)
> +#define PROXY_LINK_WAIT_DONE 1
> +
> +uint64_t wait_for_remote(int efd);
> +void notify_proxy(int fd, uint64_t val);
> +
>  #endif
> diff --git a/io/mpqemu-link.c b/io/mpqemu-link.c
> index bac120b..73b7032 100644
> --- a/io/mpqemu-link.c
> +++ b/io/mpqemu-link.c
> @@ -10,6 +10,7 @@
>  
>  #include "qemu/osdep.h"
>  #include "qemu-common.h"
> +#include <poll.h>
>  
>  #include "qemu/module.h"
>  #include "io/mpqemu-link.h"
> @@ -216,6 +217,46 @@ int mpqemu_msg_recv(MPQemuMsg *msg, MPQemuChannel *chan)
>      return rc;
>  }
>  
> +uint64_t wait_for_remote(int efd)
> +{
> +    struct pollfd pfd = { .fd = efd, .events = POLLIN };
> +    uint64_t val;
> +    int ret;
> +
> +    ret = poll(&pfd, 1, 1000);
> +
> +    switch (ret) {
> +    case 0:
> +        qemu_log_mask(LOG_REMOTE_DEBUG, "Error wait_for_remote: Timed out\n");
> +        /* TODO: Kick-off error recovery */
> +        return ULLONG_MAX;

Shouldn't these be UINT64_MAX?

> +    case -1:
> +        qemu_log_mask(LOG_REMOTE_DEBUG, "Poll error wait_for_remote: %s\n",
> +                      strerror(errno));
> +        return ULLONG_MAX;
> +    default:
> +        if (read(efd, &val, sizeof(val)) == -1) {
> +            qemu_log_mask(LOG_REMOTE_DEBUG, "Error wait_for_remote: %s\n",
> +                          strerror(errno));
> +            return ULLONG_MAX;
> +        }
> +    }
> +
> +    val = (val == ULLONG_MAX) ? val : (val - 1);

Can you explain what's going on there??

> +    return val;
> +}
> +
> +void notify_proxy(int efd, uint64_t val)
> +{
> +    val = (val == ULLONG_MAX) ? val : (val + 1);
> +
> +    if (write(efd, &val, sizeof(val)) == -1) {

I'd actually check the write/read's are returning sizeof(val) - they
can on a bad day return 0 or send only a few bytes; in theory?

Dave

> +        qemu_log_mask(LOG_REMOTE_DEBUG, "Error notify_proxy: %s\n",
> +                      strerror(errno));
> +    }
> +}
> +
>  static gboolean mpqemu_link_handler_prepare(GSource *gsrc, gint *timeout)
>  {
>      g_assert(timeout);
> -- 
> 1.8.3.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK