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; > > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list