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

[John Snow <jsnow@redhat.com>]

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