diff mbox series

[v9,07/20] multi-process: define transmission functions in remote

Message ID 20200827181231.22778-8-elena.ufimtseva@oracle.com (mailing list archive)
State New, archived
Headers show
Series Initial support for multi-process Qemu | expand

Commit Message

Elena Ufimtseva Aug. 27, 2020, 6:12 p.m. UTC 
From: Elena Ufimtseva <elena.ufimtseva@oracle.com>

remote process uses Qemu event loop and cannot block while
communicating with the remote proxy object.
This patch defines the co-routines to receive and send
messages to the proxy from remote process.

TODO: Avoid the aio_poll by entering the co-routines
from the higher level to avoid aio_poll.

Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
---
 include/io/mpqemu-link.h |  12 +++++
 io/mpqemu-link.c         | 108 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 120 insertions(+)

Comments

Stefan Hajnoczi Sept. 23, 2020, 2:02 p.m. UTC | #1 
On Thu, Aug 27, 2020 at 11:12:18AM -0700, elena.ufimtseva@oracle.com wrote:
> TODO: Avoid the aio_poll by entering the co-routines
> from the higher level to avoid aio_poll.

The monitor is unresponsive during the aio_poll() loop. Is this a
blocker for you?

Running all mpqemu communication in a coroutine as mentioned in this
TODO is a cleaner solution. Then this patch will be unnecessary.

> +static void coroutine_fn mpqemu_msg_send_co(void *data)
> +{
> +    MPQemuRequest *req = (MPQemuRequest *)data;
> +    Error *local_err = NULL;
> +
> +    mpqemu_msg_send(req->msg, req->ioc, &local_err);
> +    if (local_err) {
> +        error_report("ERROR: failed to send command to remote %d, ",
> +                     req->msg->cmd);
> +        req->finished = true;
> +        req->error = -EINVAL;
> +        return;

local_err is leaked.

> +    }
> +
> +    req->finished = true;
> +}
> +
> +void mpqemu_msg_send_in_co(MPQemuRequest *req, QIOChannel *ioc,
> +                                  Error **errp)
> +{
> +    Coroutine *co;
> +
> +    if (!req->ioc) {
> +        if (errp) {
> +            error_setg(errp, "Channel is set to NULL");
> +        } else {
> +            error_report("Channel is set to NULL");
> +        }

The caller should provide an errp if they are interested in the error
message. Duplicating error messages is messy.

> +static void coroutine_fn mpqemu_msg_recv_co(void *data)
> +{
> +    MPQemuRequest *req = (MPQemuRequest *)data;
> +    Error *local_err = NULL;
> +
> +    mpqemu_msg_recv(req->msg, req->ioc, &local_err);
> +    if (local_err) {
> +        error_report("ERROR: failed to send command to remote %d, ",
> +                     req->msg->cmd);
> +        req->finished = true;
> +        req->error = -EINVAL;
> +        return;

local_err is leaked.
Elena Ufimtseva Sept. 24, 2020, 5:18 p.m. UTC | #2 
On Wed, Sep 23, 2020 at 03:02:46PM +0100, Stefan Hajnoczi wrote:
> On Thu, Aug 27, 2020 at 11:12:18AM -0700, elena.ufimtseva@oracle.com wrote:
> > TODO: Avoid the aio_poll by entering the co-routines
> > from the higher level to avoid aio_poll.
> 
> The monitor is unresponsive during the aio_poll() loop. Is this a
> blocker for you?
>
Hi Stefan
No, not a blocker, had to leave out removal of aio_poll for the next round.
 
> Running all mpqemu communication in a coroutine as mentioned in this
> TODO is a cleaner solution. Then this patch will be unnecessary.
>
Yes, thank you, it will go away in v10.

> > +static void coroutine_fn mpqemu_msg_send_co(void *data)
> > +{
> > +    MPQemuRequest *req = (MPQemuRequest *)data;
> > +    Error *local_err = NULL;
> > +
> > +    mpqemu_msg_send(req->msg, req->ioc, &local_err);
> > +    if (local_err) {
> > +        error_report("ERROR: failed to send command to remote %d, ",
> > +                     req->msg->cmd);
> > +        req->finished = true;
> > +        req->error = -EINVAL;
> > +        return;
> 
> local_err is leaked.
> 
> > +    }
> > +
> > +    req->finished = true;
> > +}
> > +
> > +void mpqemu_msg_send_in_co(MPQemuRequest *req, QIOChannel *ioc,
> > +                                  Error **errp)
> > +{
> > +    Coroutine *co;
> > +
> > +    if (!req->ioc) {
> > +        if (errp) {
> > +            error_setg(errp, "Channel is set to NULL");
> > +        } else {
> > +            error_report("Channel is set to NULL");
> > +        }
> 
> The caller should provide an errp if they are interested in the error
> message. Duplicating error messages is messy.
> 
> > +static void coroutine_fn mpqemu_msg_recv_co(void *data)
> > +{
> > +    MPQemuRequest *req = (MPQemuRequest *)data;
> > +    Error *local_err = NULL;
> > +
> > +    mpqemu_msg_recv(req->msg, req->ioc, &local_err);
> > +    if (local_err) {
> > +        error_report("ERROR: failed to send command to remote %d, ",
> > +                     req->msg->cmd);
> > +        req->finished = true;
> > +        req->error = -EINVAL;
> > +        return;
> 
> local_err is leaked.

Thank you for reviewing these, will fix If the code above will be re-used.

Elena
diff mbox series

Patch

diff --git a/include/io/mpqemu-link.h b/include/io/mpqemu-link.h
index c7de8648bc..e02b5ce663 100644
--- a/include/io/mpqemu-link.h
+++ b/include/io/mpqemu-link.h
@@ -52,6 +52,18 @@  typedef struct {
     int num_fds;
 } MPQemuMsg;
 
+struct MPQemuRequest {
+    MPQemuMsg *msg;
+    QIOChannel *ioc;
+    bool finished;
+    int error;
+};
+
+typedef struct MPQemuRequest MPQemuRequest;
+
+void mpqemu_msg_send_in_co(MPQemuRequest *req, QIOChannel *ioc, Error **errp);
+void mpqemu_msg_recv_in_co(MPQemuRequest *req, QIOChannel *ioc, Error **errp);
+
 void mpqemu_msg_send(MPQemuMsg *msg, QIOChannel *ioc, Error **errp);
 void mpqemu_msg_recv(MPQemuMsg *msg, QIOChannel *ioc, Error **errp);
 
diff --git a/io/mpqemu-link.c b/io/mpqemu-link.c
index d9be2bdeab..1dd776f81b 100644
--- a/io/mpqemu-link.c
+++ b/io/mpqemu-link.c
@@ -150,6 +150,114 @@  fail:
     }
 }
 
+static void coroutine_fn mpqemu_msg_send_co(void *data)
+{
+    MPQemuRequest *req = (MPQemuRequest *)data;
+    Error *local_err = NULL;
+
+    mpqemu_msg_send(req->msg, req->ioc, &local_err);
+    if (local_err) {
+        error_report("ERROR: failed to send command to remote %d, ",
+                     req->msg->cmd);
+        req->finished = true;
+        req->error = -EINVAL;
+        return;
+    }
+
+    req->finished = true;
+}
+
+void mpqemu_msg_send_in_co(MPQemuRequest *req, QIOChannel *ioc,
+                                  Error **errp)
+{
+    Coroutine *co;
+
+    if (!req->ioc) {
+        if (errp) {
+            error_setg(errp, "Channel is set to NULL");
+        } else {
+            error_report("Channel is set to NULL");
+        }
+        return;
+    }
+
+    req->error = 0;
+    req->finished = false;
+
+    co = qemu_coroutine_create(mpqemu_msg_send_co, req);
+    qemu_coroutine_enter(co);
+
+    while (!req->finished) {
+        aio_poll(qemu_get_aio_context(), true);
+    }
+
+    if (req->error) {
+        if (errp) {
+            error_setg(errp, "Error sending message to proxy, "
+                             "error %d", req->error);
+        } else {
+            error_report("Error sending message to proxy, "
+                         "error %d", req->error);
+        }
+    }
+
+    return;
+}
+
+static void coroutine_fn mpqemu_msg_recv_co(void *data)
+{
+    MPQemuRequest *req = (MPQemuRequest *)data;
+    Error *local_err = NULL;
+
+    mpqemu_msg_recv(req->msg, req->ioc, &local_err);
+    if (local_err) {
+        error_report("ERROR: failed to send command to remote %d, ",
+                     req->msg->cmd);
+        req->finished = true;
+        req->error = -EINVAL;
+        return;
+    }
+
+    req->finished = true;
+}
+
+void mpqemu_msg_recv_in_co(MPQemuRequest *req, QIOChannel *ioc,
+                               Error **errp)
+{
+    Coroutine *co;
+
+    if (!req->ioc) {
+        if (errp) {
+            error_setg(errp, "Channel is set to NULL");
+        } else {
+            error_report("Channel is set to NULL");
+        }
+        return;
+    }
+
+    req->error = 0;
+    req->finished = false;
+
+    co = qemu_coroutine_create(mpqemu_msg_recv_co, req);
+    qemu_coroutine_enter(co);
+
+    while (!req->finished) {
+        aio_poll(qemu_get_aio_context(), true);
+    }
+
+    if (req->error) {
+        if (errp) {
+            error_setg(errp, "Error sending message to proxy, "
+                             "error %d", req->error);
+        } else {
+            error_report("Error sending message to proxy, "
+                         "error %d", req->error);
+        }
+    }
+
+    return;
+}
+
 bool mpqemu_msg_valid(MPQemuMsg *msg)
 {
     if (msg->cmd >= MAX && msg->cmd < 0) {