Re: [Qemu-block] [Qemu-devel] [PATCH v3 3/9] libqos: Add migration helpers

[John Snow]

On 05/05/2015 07:35 AM, Kevin Wolf wrote:

Am 04.05.2015 um 19:52 hat John Snow geschrieben:



On 05/04/2015 08:07 AM, Kevin Wolf wrote:

Am 30.04.2015 um 20:07 hat John Snow geschrieben:

+/* Otherwise, we need to wait: poll until migration is completed. */
+while (1) {
+rsp = qmp_execute(query-migrate);
+g_assert(qdict_haskey(rsp, return));
+sub = qdict_get_qdict(rsp, return);
+g_assert(qdict_haskey(sub, status));
+st = qdict_get_str(sub, status);
+
+/* setup, active, completed, failed, cancelled */
+if (strcmp(st, completed) == 0) {
+QDECREF(rsp);
+break;
+}
+
+if ((strcmp(st, setup) == 0) || (strcmp(st, active) == 0)) {
+QDECREF(rsp);
+continue;


Wouldn't it be nicer to sleep a bit before retrying?



I actually figured that all the string and stream manipulation for
sending and receiving QMP queries was enough sleep because of how
quick a migration without any guest should complete -- in practice
this loop doesn't ever seem to trigger more than once.


This surprised me a bit at first because there's no way that string
operations are _that_ slow. You would definitely spin a while in this
loop (and potentially slow down the migration by that).

I think what saves you is that you wait for the STOP event first, and
when qemu's migration thread sends that event, it happens to have
already taken the global mutex. This means that you get your enough
sleep from the qemu monitor, which won't respond before migration has
completed.


If you still think sleep is necessary, I can add some very small
sleep in a separate patch, or when I merge the tree. Something like:

g_usleep(5000) /* 5 msec */


If I were you, I'd add it just to be nice (just applying it to your tree
instead of sending out a new version would be okay). If you don't want
to, I won't insist, though. I mean, I already gave my R-b...

Kevin



It's worth finding out if my reasoning is sane, and you cared enough to 
comment.


I'll add the sleep when I merge, no problem :)

Thanks!
--js

Re: [Qemu-block] [Qemu-devel] [PATCH v3 3/9] libqos: Add migration helpers

[John Snow]

On 05/04/2015 08:07 AM, Kevin Wolf wrote:

Am 30.04.2015 um 20:07 hat John Snow geschrieben:

libqos.c:
 -set_context for addressing which commands go where
 -migrate performs the actual migration

malloc.c:
 - Structure of the allocator is adjusted slightly with
   a second-tier malloc to make swapping around the allocators
   easy when we migrate the lists from the source to the destination.

Signed-off-by: John Snow js...@redhat.com
---
  tests/libqos/libqos.c | 84 +++
  tests/libqos/libqos.h |  2 ++
  tests/libqos/malloc.c | 74 ++---
  tests/libqos/malloc.h |  1 +
  4 files changed, 144 insertions(+), 17 deletions(-)

diff --git a/tests/libqos/libqos.c b/tests/libqos/libqos.c
index 7e72078..ac1bae1 100644
--- a/tests/libqos/libqos.c
+++ b/tests/libqos/libqos.c
@@ -1,5 +1,6 @@
  #include stdio.h
  #include stdlib.h
+#include string.h
  #include glib.h
  #include unistd.h
  #include fcntl.h
@@ -62,6 +63,89 @@ void qtest_shutdown(QOSState *qs)
  g_free(qs);
  }

+void set_context(QOSState *s)
+{
+global_qtest = s-qts;
+}
+
+static QDict *qmp_execute(const char *command)
+{
+char *fmt;
+QDict *rsp;
+
+fmt = g_strdup_printf({ 'execute': '%s' }, command);
+rsp = qmp(fmt);
+g_free(fmt);
+
+return rsp;
+}
+
+void migrate(QOSState *from, QOSState *to, const char *uri)
+{
+const char *st;
+char *s;
+QDict *rsp, *sub;
+bool running;
+
+set_context(from);
+
+/* Is the machine currently running? */
+rsp = qmp_execute(query-status);
+g_assert(qdict_haskey(rsp, return));
+sub = qdict_get_qdict(rsp, return);
+g_assert(qdict_haskey(sub, running));
+running = qdict_get_bool(sub, running);
+QDECREF(rsp);
+
+/* Issue the migrate command. */
+s = g_strdup_printf({ 'execute': 'migrate',
+'arguments': { 'uri': '%s' } },
+uri);
+rsp = qmp(s);
+g_free(s);
+g_assert(qdict_haskey(rsp, return));
+QDECREF(rsp);
+
+/* Wait for STOP event, but only if we were running: */
+if (running) {
+qmp_eventwait(STOP);
+}
+
+/* If we were running, we can wait for an event. */
+if (running) {
+migrate_allocator(from-alloc, to-alloc);
+set_context(to);
+qmp_eventwait(RESUME);
+return;
+}
+
+/* Otherwise, we need to wait: poll until migration is completed. */
+while (1) {
+rsp = qmp_execute(query-migrate);
+g_assert(qdict_haskey(rsp, return));
+sub = qdict_get_qdict(rsp, return);
+g_assert(qdict_haskey(sub, status));
+st = qdict_get_str(sub, status);
+
+/* setup, active, completed, failed, cancelled */
+if (strcmp(st, completed) == 0) {
+QDECREF(rsp);
+break;
+}
+
+if ((strcmp(st, setup) == 0) || (strcmp(st, active) == 0)) {
+QDECREF(rsp);
+continue;


Wouldn't it be nicer to sleep a bit before retrying?



I actually figured that all the string and stream manipulation for 
sending and receiving QMP queries was enough sleep because of how 
quick a migration without any guest should complete -- in practice this 
loop doesn't ever seem to trigger more than once.


If you still think sleep is necessary, I can add some very small sleep 
in a separate patch, or when I merge the tree. Something like:


g_usleep(5000) /* 5 msec */


+}
+
+fprintf(stderr, Migration did not complete, status: %s\n, st);
+g_assert_not_reached();
+}
+
+migrate_allocator(from-alloc, to-alloc);
+set_context(to);
+}


Kevin