[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: John Snow
Subject: Re: [Qemu-devel] [Qemu-block] [PATCH v3 3/9] libqos: Add migration helpers
Date: Tue, 05 May 2015 11:50:47 -0400
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0

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

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


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 :)


reply via email to

[Prev in Thread] Current Thread [Next in Thread]