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

[Jag Raman]

> On May 12, 2020, at 6:21 AM, Stefan Hajnoczi <address@hidden> wrote:
> 
> 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?

Originally, we were envisioning a scenario where the remote process would
interrupt QEMU with a message. We used separate eventfds to distinguish
the two.

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

OK, got it.

> 
>> +#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.

In case the remote process has hung or terminated, the 1 second timeout
ensures that the IO operation does not block for too long.

--
Jag

> 
>> +
>> +    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
>>