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;