[libvirt] [RFC PATCH v2 1/3] qemu: Expose additional timing metrics for 'setup' and 'mbps'

[Michael R. Hines]

On 02/03/2014 08:32 PM, Jiri Denemark wrote:
> On Mon, Jan 13, 2014 at 14:28:10 +0800, mrhines at linux.vnet.ibm.com wrote:
>> From: "Michael R. Hines" <mrhines at us.ibm.com>
>>
>> RDMA migration uses the 'setup' state in QEMU to optionally lock
>> all memory before the migration starts. The total time spent in
>> this state is already exposed by QEMU, so expose it in libvirt.
>>
>> Additionally, QEMU also now exports migration throughput (mbps),
>> so let's see that one too...
>>
>> Signed-off-by: Michael R. Hines <mrhines at us.ibm.com>
>> ---
>>   include/libvirt/libvirt.h.in | 15 +++++++++++++++
>>   src/qemu/qemu_driver.c       | 14 ++++++++++++++
>>   src/qemu/qemu_monitor.h      | 12 ++++++++++++
>>   src/qemu/qemu_monitor_json.c |  8 ++++++++
>>   4 files changed, 49 insertions(+)
>>
>> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
>> index 5aad75c..5ac2694 100644
>> --- a/include/libvirt/libvirt.h.in
>> +++ b/include/libvirt/libvirt.h.in
>> @@ -4153,6 +4153,21 @@ int virDomainAbortJob(virDomainPtr dom);
>>   #define VIR_DOMAIN_JOB_DOWNTIME                 "downtime"
>>   
>>   /**
>> + * VIR_DOMAIN_JOB_MBPS:
>> + *
>> + * virDomainGetJobStats field: network throughput used while migrating.
>> + */
>> +#define VIR_DOMAIN_JOB_MBPS "mbps"
> I think this would be better as
>
>      #define VIR_DOMAIN_JOB_THROUGHPUT "throughput"
>
> And you need to explicitly mention the type of the value returned in
> this parameter.

Acknowledged.

>
>> +
>> +/**
>> + * VIR_DOMAIN_JOB_SETUP_TIME:
>> + *
>> + * virDomainGetJobStats field: total time in milliseconds spent preparing
>> + * the migration in the 'setup' phase before the iterations begin.
>> + */
>> +#define VIR_DOMAIN_JOB_SETUP_TIME "setup_time"
> Again, the documentation does not mention what type is returned.

Acknowledged.

>> +
>> +/**
>>    * VIR_DOMAIN_JOB_DATA_TOTAL:
>>    *
>>    * virDomainGetJobStats field: total number of bytes supposed to be
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 47d8a09..6a7dcc7 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -11078,6 +11078,20 @@ qemuDomainGetJobStats(virDomainPtr dom,
>>               goto cleanup;
>>       }
>>   
>> +    if (priv->job.status.mbps_set) {
>> +        if (virTypedParamsAddDouble(&par, &npar, &maxpar,
>> +                                    VIR_DOMAIN_JOB_MBPS,
>> +                                    priv->job.status.mbps) < 0)
>> +            goto cleanup;
>> +    }
> I think we should take the Mbps value from qemu and turn it into Bps (or
> even bps), which would make this parameter consistent with other
> migration statistics parameters that return values in bytes. And then we
> can change the type to ULLong.
>

Acknowledged.

>> +
>> +    if (priv->job.status.setup_time_set) {
>> +        if (virTypedParamsAddULLong(&par, &npar, &maxpar,
>> +                                    VIR_DOMAIN_JOB_SETUP_TIME,
>> +                                    priv->job.status.setup_time) < 0)
>> +            goto cleanup;
>> +    }
>> +
>>       if (virTypedParamsAddULLong(&par, &npar, &maxpar,
>>                                   VIR_DOMAIN_JOB_DISK_TOTAL,
>>                                   priv->job.info.fileTotal) < 0 ||
>> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
>> index eabf000..27f9cb4 100644
>> --- a/src/qemu/qemu_monitor.h
>> +++ b/src/qemu/qemu_monitor.h
>> @@ -412,6 +412,18 @@ struct _qemuMonitorMigrationStatus {
>>       /* total or expected depending on status */
>>       bool downtime_set;
>>       unsigned long long downtime;
>> +    /*
>> +     * Duration of the QEMU 'setup' state.
>> +     * for RDMA, this may be on the order of several seconds
>> +     * if pinning support is requested before the migration begins.
>> +     */
>> +    bool setup_time_set;
>> +    unsigned long long setup_time;
>> +    /*
>> +     * Migration throughput in Mbps.
>> +     */
>> +    bool mbps_set;
>> +    double mbps;
>>   
>>       unsigned long long ram_transferred;
>>       unsigned long long ram_remaining;
>> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
>> index ec3b958..214e140 100644
>> --- a/src/qemu/qemu_monitor_json.c
>> +++ b/src/qemu/qemu_monitor_json.c
>> @@ -2426,6 +2426,14 @@ qemuMonitorJSONGetMigrationStatusReply(virJSONValuePtr reply,
>>           virJSONValueObjectGetNumberUlong(ram, "normal-bytes",
>>                                            &status->ram_normal_bytes);
>>   
>> +        if (virJSONValueObjectGetNumberDouble(ram, "mbps",
>> +                                              &status->mbps) == 0)
>> +            status->mbps_set = true;
>> +
>> +        if (virJSONValueObjectGetNumberUlong(ret, "setup-time",
>> +                                              &status->setup_time) == 0)
>> +            status->setup_time_set = true;
>> +
>>           virJSONValuePtr disk = virJSONValueObjectGet(ret, "disk");
>>           if (disk) {
>>               rc = virJSONValueObjectGetNumberUlong(disk, "transferred",
> Overall the patch looks good and could even be pushed separately.
>
> Jirka
>

Will submit a v3 for everything soon....

- Michae