[libvirt] [PATCH] qemu: bulk stats: implement (cpu) tune group.
John Ferlan
jferlan at redhat.com
Mon Feb 23 18:33:47 UTC 2015
On 02/11/2015 09:22 AM, Francesco Romani wrote:
> Management applications, like oVirt, may need to setup cpu quota
> limits to enforce QoS for VMs.
>
> For this purpose, management applications also need to check how
> VMs are behaving with respect to CPU quota. This data is avaialble
> using the virDomainGetSchedulerParameters API.
>
> This patch adds a new group to bulk stats API to obtain the same
> information.
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1191428
> ---
> include/libvirt/libvirt-domain.h | 1 +
> src/libvirt-domain.c | 16 ++++++++
> src/qemu/qemu_driver.c | 84 ++++++++++++++++++++++++++++++++++++++++
> tools/virsh-domain-monitor.c | 7 ++++
> 4 files changed, 108 insertions(+)
>
In general looks good... There's a few spelling and spacing nits below
which I could fix up before pushing for you...
You are missing 'virsh.pod' - something easily added as well.
The one question I have is around the switch name (looking for any other
thoughts...)
Should the option be "cpu-tune" instead of "tune-cpu", especially since
the name of the function has "*CpuTune"? Or even 'sched-info' to match
the 'virsh schedinfo $dom' command? I suppose some day there'd be
'numa-tune' data desired as well, but that's a different issue...
John
> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> index 4dbd7f5..3d8c6af 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -1700,6 +1700,7 @@ typedef enum {
> VIR_DOMAIN_STATS_VCPU = (1 << 3), /* return domain virtual CPU info */
> VIR_DOMAIN_STATS_INTERFACE = (1 << 4), /* return domain interfaces info */
> VIR_DOMAIN_STATS_BLOCK = (1 << 5), /* return domain block info */
> + VIR_DOMAIN_STATS_TUNE_CPU = (1 << 6), /* return domain CPU tuning info */
> } virDomainStatsTypes;
>
> typedef enum {
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 492e90a..a4effa3 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -10990,6 +10990,22 @@ virConnectGetDomainCapabilities(virConnectPtr conn,
> * "block.<num>.physical" - physical size in bytes of the container of the
> * backing image as unsigned long long.
> *
> + * VIR_DOMAIN_STATS_TUNE_CPU: Return CPU tuning statistics
> + * and usage information.
> + * The typed parameter keys are in this format:
> + * "tune.vcpu.quota" - max allowed bandwith, in microseconds, as
s/bandwith/bandwidth
> + * long long integer. -1 means 'infinite'.
> + * "tune.vcpu.period" - timeframe on which the virtual cpu quota is
> + * enforced, in microseconds, as unsigned long long.
> + * "tune.emu.quota" - max allowd bandwith for emulator threads,
s/bandwith/bandwidth
s/allowd/allowed
> + * in microseconds, as long long integer.
> + * -1 means 'infinite'.
> + * "tune.emu.period" - timeframe on which the emulator quota is
> + * enforced, in microseconds, as unsigned long long.
> + * "tune.cpu.shares" - weight of this VM. This value is meaningful
> + * only if compared with the other values of
> + * the running vms. Expressed as unsigned long long.
s/vms/domains
FWIW: I guess I find it hard to read 'vms' without thinking about the
VMS (or OpenVMS) operating system ;-) [guess where I started my career].
> + *
> * Note that entire stats groups or individual stat fields may be missing from
> * the output in case they are not supported by the given hypervisor, are not
> * applicable for the current state of the guest domain, or their retrieval
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 26fc6a2..5548626 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -18797,6 +18797,89 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
>
> #undef QEMU_ADD_COUNT_PARAM
>
> +
> +#define QEMU_ADD_PARAM_LL(record, maxparams, name, value) \
> +do { \
> + if (virTypedParamsAddLLong(&(record)->params, \
> + &(record)->nparams, \
> + maxparams, \
> + name, \
> + value) < 0) \
> + goto cleanup; \
> +} while (0)
> +
> +#define QEMU_ADD_PARAM_ULL(record, maxparams, name, value) \
> +do { \
> + if (virTypedParamsAddULLong(&(record)->params, \
> + &(record)->nparams, \
> + maxparams, \
> + name, \
> + value) < 0) \
> + goto cleanup; \
> +} while (0)
> +
> +static int
> +qemuDomainGetStatsCpuTune(virQEMUDriverPtr driver,
> + virDomainObjPtr dom,
> + virDomainStatsRecordPtr record,
> + int *maxparams,
> + unsigned int privflags ATTRIBUTE_UNUSED)
> +{
> + int ret = -1;
> + unsigned long long shares = 0;
> + qemuDomainObjPrivatePtr priv = dom->privateData;
> + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> +
> + if (!cfg->privileged ||
> + !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) {
^
Just a minor nit about spacing - needs one more...
> + ret = 0;
> + goto cleanup;
> + }
> +
> + if (virCgroupGetCpuShares(priv->cgroup, &shares) < 0) {
> + ret = 0;
> + goto cleanup;
> + }
> +
> + QEMU_ADD_PARAM_ULL(record, maxparams, "tune.cpu.shares", shares);
> +
> + if (virCgroupSupportsCpuBW(priv->cgroup)) {
> + unsigned long long period = 0;
> + long long quota = 0;
> + unsigned long long emulator_period = 0;
> + long long emulator_quota = 0;
> + int err;
> +
> + err = qemuGetVcpusBWLive(dom, &period, "a);
> + if (!err) {
> + QEMU_ADD_PARAM_ULL(record, maxparams,
> + "tune.vcpu.period", period);
> + QEMU_ADD_PARAM_LL(record, maxparams,
> + "tune.vcpu.quota", quota);
> + }
> +
> + err = qemuGetEmulatorBandwidthLive(dom, priv->cgroup,
> + &emulator_period, &emulator_quota);
> + if (!err) {
> + QEMU_ADD_PARAM_ULL(record, maxparams,
> + "tune.emu.period", emulator_period);
> + QEMU_ADD_PARAM_LL(record, maxparams,
> + "tune.emu.quota", emulator_quota);
> + }
having 'emulator_' specific variables probably isn't necessary, but not
a big deal..
> + }
> +
> + ret = 0;
> +
> + cleanup:
> + virObjectUnref(cfg);
> + return ret;
> +}
> +
> +#undef QEMU_ADD_PARAM_LL
> +
> +#undef QEMU_ADD_PARAM_ULL
> +
> +
> typedef int
> (*qemuDomainGetStatsFunc)(virQEMUDriverPtr driver,
> virDomainObjPtr dom,
> @@ -18817,6 +18900,7 @@ static struct qemuDomainGetStatsWorker qemuDomainGetStatsWorkers[] = {
> { qemuDomainGetStatsVcpu, VIR_DOMAIN_STATS_VCPU, false },
> { qemuDomainGetStatsInterface, VIR_DOMAIN_STATS_INTERFACE, false },
> { qemuDomainGetStatsBlock, VIR_DOMAIN_STATS_BLOCK, true },
> + { qemuDomainGetStatsCpuTune, VIR_DOMAIN_STATS_TUNE_CPU, false },
> { NULL, 0, false }
> };
>
> diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
> index 925eb1b..e425e43 100644
> --- a/tools/virsh-domain-monitor.c
> +++ b/tools/virsh-domain-monitor.c
> @@ -1997,6 +1997,10 @@ static const vshCmdOptDef opts_domstats[] = {
> .type = VSH_OT_BOOL,
> .help = N_("report domain block device statistics"),
> },
> + {.name = "tune-cpu",
> + .type = VSH_OT_BOOL,
> + .help = N_("report domain cpu scheduler parameters"),
> + },
> {.name = "list-active",
> .type = VSH_OT_BOOL,
> .help = N_("list only active domains"),
> @@ -2107,6 +2111,9 @@ cmdDomstats(vshControl *ctl, const vshCmd *cmd)
> if (vshCommandOptBool(cmd, "block"))
> stats |= VIR_DOMAIN_STATS_BLOCK;
>
> + if (vshCommandOptBool(cmd, "tune-cpu"))
> + stats |= VIR_DOMAIN_STATS_TUNE_CPU;
> +
> if (vshCommandOptBool(cmd, "list-active"))
> flags |= VIR_CONNECT_GET_ALL_DOMAINS_STATS_ACTIVE;
>
>
More information about the libvir-list
mailing list