12:40 a.m.
On 02/03/2014 08:32 PM, Jiri Denemark wrote:
On Mon, Jan 13, 2014 at 14:28:10 +0800, mrhines(a)linux.vnet.ibm.com
wrote:
> From: "Michael R. Hines" <mrhines(a)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(a)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