Re: [PATCH RESEND v6 12/36] multi-process: add functions to synchronize proxy and remote endpoints

[Stefan Hajnoczi]

On Wed, Apr 22, 2020 at 09:13:47PM -0700, address@hidden wrote:
> From: Jagannathan Raman <address@hidden>
> 
> 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.

Why are temporary eventfds used instead of sending a reply message from
the remote device program back to QEMU?

> Signed-off-by: John G Johnson <address@hidden>
> Signed-off-by: Jagannathan Raman <address@hidden>
> Signed-off-by: Elena Ufimtseva <address@hidden>
> ---
>  include/io/mpqemu-link.h |  7 +++++
>  io/mpqemu-link.c         | 61 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 68 insertions(+)
> 
> diff --git a/include/io/mpqemu-link.h b/include/io/mpqemu-link.h
> index af401e640c..ef95599bca 100644
> --- a/include/io/mpqemu-link.h
> +++ b/include/io/mpqemu-link.h
> @@ -124,4 +124,11 @@ void mpqemu_link_set_callback(MPQemuLinkState *s,
>  void mpqemu_start_coms(MPQemuLinkState *s, MPQemuChannel* chan);
>  bool mpqemu_msg_valid(MPQemuMsg *msg);
>  
> +#define GET_REMOTE_WAIT eventfd(0, EFD_CLOEXEC)
> +#define PUT_REMOTE_WAIT(wait) close(wait)

Hiding this in macros makes the code harder to understand.

Why is an eventfd necessary instead of a reply message? It's simpler and
probably faster to use a reply message instead of creating and passing
temporary eventfds.

> +#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 48f53a8928..cc0a7aecd4 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"
> @@ -204,6 +205,66 @@ int mpqemu_msg_recv(MPQemuMsg *msg, MPQemuChannel *chan)
>      return rc;
>  }
>  
> +/*
> + * wait_for_remote() Synchronizes QEMU and the remote process. The maximum
> + *                   wait time is 1s, after which the wait times out.
> + *                   The function alse returns a 64 bit return value after
> + *                   the wait. The function uses eventfd() to do the wait
> + *                   and pass the return values. eventfd() can't return a
> + *                   value of '0'. Therefore, all return values are offset
> + *                   by '1' at the sending end, and corrected at the
> + *                   receiving end.
> + */
> +
> +uint64_t wait_for_remote(int efd)
> +{
> +    struct pollfd pfd = { .fd = efd, .events = POLLIN };
> +    uint64_t val;
> +    int ret;
> +
> +    ret = poll(&pfd, 1, 1000);

This 1 second blocking operation is not allowed in an event loop since
it will stall any other event loop activity. If locks are held then
other threads may also be stalled.

It's likely that this will need to change as part of the QEMU event loop
integration. Caller code can be kept mostly unchanged if you use
coroutines.

> +
> +    switch (ret) {
> +    case 0:
> +        qemu_log_mask(LOG_REMOTE_DEBUG, "Error wait_for_remote: Timed 
> out\n");
> +        /* TODO: Kick-off error recovery */
> +        return UINT64_MAX;
> +    case -1:
> +        qemu_log_mask(LOG_REMOTE_DEBUG, "Poll error wait_for_remote: %s\n",
> +                      strerror(errno));
> +        return UINT64_MAX;
> +    default:
> +        if (read(efd, &val, sizeof(val)) == -1) {
> +            qemu_log_mask(LOG_REMOTE_DEBUG, "Error wait_for_remote: %s\n",
> +                          strerror(errno));
> +            return UINT64_MAX;
> +        }
> +    }
> +
> +    /*
> +     * The remote process could write a non-zero value
> +     * to the eventfd to wake QEMU up. However, the drawback of using eventfd
> +     * for this purpose is that a return value of zero wouldn't wake QEMU up.
> +     * Therefore, we offset the return value by one at the remote process and
> +     * correct it in the QEMU end.
> +     */
> +    val = (val == UINT64_MAX) ? val : (val - 1);
> +
> +    return val;
> +}
> +
> +void notify_proxy(int efd, uint64_t val)
> +{
> +    val = (val == UINT64_MAX) ? val : (val + 1);
> +    ssize_t len = -1;
> +
> +    len = write(efd, &val, sizeof(val));
> +    if (len == -1 || len != sizeof(val)) {
> +        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);
> -- 
> 2.25.GIT
>

signature.asc
Description: PGP signature