Re: [Qemu-devel] [RFC v3 PATCH 08/45] multi-process: add functions to synchronize proxy and remote endpoints

[Stefan Hajnoczi]

On Tue, Sep 03, 2019 at 04:37:34PM -0400, Jagannathan Raman 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 <address@hidden>
> Signed-off-by: Jagannathan Raman <address@hidden>
> Signed-off-by: Elena Ufimtseva <address@hidden>
> ---
>  v1 -> v2:
>    - Added timeout to synchronization functions
> 
>  include/io/proxy-link.h |  8 ++++++++
>  io/proxy-link.c         | 42 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+)
> 
> diff --git a/include/io/proxy-link.h b/include/io/proxy-link.h
> index ee78cdd..b76c574 100644
> --- a/include/io/proxy-link.h
> +++ b/include/io/proxy-link.h
> @@ -28,7 +28,9 @@
>  #include <stddef.h>
>  #include <stdint.h>
>  #include <glib.h>
> +#include <unistd.h>
>  #include <pthread.h>
> +#include <sys/eventfd.h>
>  
>  #include "qemu/osdep.h"
>  #include "qom/object.h"
> @@ -133,11 +135,17 @@ struct ProxyLinkState {
>      proxy_link_callback callback;
>  };
>  
> +#define GET_REMOTE_WAIT eventfd(0, 0)
> +#define PUT_REMOTE_WAIT(wait) close(wait)

Can you use functions instead of macros?  eventfd() is Linux-specific so
this code is not portable.  QEMU has an EventNotifier abstraction but
I'm not sure if it can be used since this patch doesn't include any code
that calls GET_REMOTE_WAIT/PUT_REMOTE_WAIT and there are no comments.  I
don't know what the expected semantics are.

> +#define PROXY_LINK_WAIT_DONE 1
> +
>  ProxyLinkState *proxy_link_create(void);
>  void proxy_link_finalize(ProxyLinkState *s);
>  
>  void proxy_proc_send(ProxyLinkState *s, ProcMsg *msg, ProcChannel *chan);
>  int proxy_proc_recv(ProxyLinkState *s, ProcMsg *msg, ProcChannel *chan);
> +uint64_t wait_for_remote(int efd);
> +void notify_proxy(int fd, uint64_t val);
>  
>  void proxy_link_init_channel(ProxyLinkState *s, ProcChannel **chan, int fd);
>  void proxy_link_destroy_channel(ProcChannel *chan);
> diff --git a/io/proxy-link.c b/io/proxy-link.c
> index 5eb9718..381a38e 100644
> --- a/io/proxy-link.c
> +++ b/io/proxy-link.c
> @@ -31,6 +31,8 @@
>  #include <sys/socket.h>
>  #include <sys/un.h>
>  #include <unistd.h>
> +#include <limits.h>
> +#include <poll.h>
>  
>  #include "qemu/module.h"
>  #include "io/proxy-link.h"
> @@ -216,6 +218,46 @@ int proxy_proc_recv(ProxyLinkState *s, ProcMsg *msg, 
> ProcChannel *chan)
>      return rc;
>  }
>  
> +uint64_t wait_for_remote(int efd)

Hard to tell if this makes sense without any context.  I notice that
EFD_CLOEXEC and EFD_NONBLOCK were not used.  It's likely that
EFD_CLOEXEC should be used.  Since the eventfd is used with poll(2)
EFD_NONBLOCK should probably also be used so it's certain that read()
will not block (which could exceed the timeout).

signature.asc
Description: PGP signature