[libvirt] [PATCHv2 0/2] Fix zero cpu shares handling
Treat 0 as a valid value for cputune shares. v1: https://www.redhat.com/archives/libvir-list/2014-March/msg00215.html v2: squashed the fixup that was sent as a separate patch to the first commit and fixed two more occurences (one of them pointed out by Martin) Ján Tomko (2): Treat zero cpu shares as a valid value Show the real cpu shares value in live xml src/conf/domain_conf.c | 12 ++++---- src/conf/domain_conf.h | 1 + src/lxc/lxc_cgroup.c | 13 ++++++-- src/lxc/lxc_driver.c | 8 ++++- src/lxc/lxc_native.c | 8 +++-- src/parallels/parallels_driver.c | 1 + src/qemu/qemu_cgroup.c | 14 ++++++--- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_driver.c | 13 ++++++-- src/vmx/vmx.c | 3 +- .../qemuxml2argv-cputune-zero-shares.args | 5 ++++ .../qemuxml2argv-cputune-zero-shares.xml | 35 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + tests/qemuxml2xmltest.c | 1 + 14 files changed, 97 insertions(+), 20 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-cputune-zero-shares.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-cputune-zero-shares.xml -- 1.8.3.2
Currently, <cputune><shares>0</shares></cputune> is treated as if it were not specified. Treat is as a valid value if it was explicitly specified and write it to the cgroups. --- src/conf/domain_conf.c | 12 ++++---- src/conf/domain_conf.h | 1 + src/lxc/lxc_cgroup.c | 2 +- src/lxc/lxc_driver.c | 2 ++ src/lxc/lxc_native.c | 8 +++-- src/parallels/parallels_driver.c | 1 + src/qemu/qemu_cgroup.c | 4 +-- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_driver.c | 6 +++- src/vmx/vmx.c | 3 +- .../qemuxml2argv-cputune-zero-shares.args | 5 ++++ .../qemuxml2argv-cputune-zero-shares.xml | 35 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + tests/qemuxml2xmltest.c | 1 + 14 files changed, 69 insertions(+), 14 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-cputune-zero-shares.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-cputune-zero-shares.xml diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 92b1324..72f25f1 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11402,11 +11402,13 @@ virDomainDefParseXML(xmlDocPtr xml, } /* Extract cpu tunables. */ - if (virXPathULong("string(./cputune/shares[1])", ctxt, - &def->cputune.shares) < -1) { + if ((n = virXPathULong("string(./cputune/shares[1])", ctxt, + &def->cputune.shares)) < -1) { virReportError(VIR_ERR_XML_ERROR, "%s", _("can't parse cputune shares value")); goto error; + } else if (n == 0) { + def->cputune.sharesSpecified = true; } if (virXPathULongLong("string(./cputune/period[1])", ctxt, @@ -17133,7 +17135,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, virBufferAsprintf(buf, " current='%u'", def->vcpus); virBufferAsprintf(buf, ">%u</vcpu>\n", def->maxvcpus); - if (def->cputune.shares || + if (def->cputune.sharesSpecified || (def->cputune.nvcpupin && !virDomainIsAllVcpupinInherited(def)) || def->cputune.period || def->cputune.quota || def->cputune.emulatorpin || @@ -17141,7 +17143,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, virBufferAddLit(buf, "<cputune>\n"); virBufferAdjustIndent(buf, 2); - if (def->cputune.shares) + if (def->cputune.sharesSpecified) virBufferAsprintf(buf, "<shares>%lu</shares>\n", def->cputune.shares); if (def->cputune.period) @@ -17196,7 +17198,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, VIR_FREE(cpumask); } virBufferAdjustIndent(buf, -2); - if (def->cputune.shares || + if (def->cputune.sharesSpecified || (def->cputune.nvcpupin && !virDomainIsAllVcpupinInherited(def)) || def->cputune.period || def->cputune.quota || def->cputune.emulatorpin || diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 37191a8..79cf1a4 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2000,6 +2000,7 @@ struct _virDomainDef { struct { unsigned long shares; + bool sharesSpecified; unsigned long long period; long long quota; unsigned long long emulator_period; diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c index 39d955c..876c32e 100644 --- a/src/lxc/lxc_cgroup.c +++ b/src/lxc/lxc_cgroup.c @@ -36,7 +36,7 @@ static int virLXCCgroupSetupCpuTune(virDomainDefPtr def, virCgroupPtr cgroup) { int ret = -1; - if (def->cputune.shares != 0 && + if (def->cputune.sharesSpecified && virCgroupSetCpuShares(cgroup, def->cputune.shares) < 0) goto cleanup; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 0ab1ba2..7c8e7c0 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1892,10 +1892,12 @@ lxcDomainSetSchedulerParametersFlags(virDomainPtr dom, goto cleanup; vm->def->cputune.shares = params[i].value.ul; + vm->def->cputune.sharesSpecified = true; } if (flags & VIR_DOMAIN_AFFECT_CONFIG) { vmdef->cputune.shares = params[i].value.ul; + vmdef->cputune.sharesSpecified = true; } } else if (STREQ(param->field, VIR_DOMAIN_SCHEDULER_VCPU_PERIOD)) { if (flags & VIR_DOMAIN_AFFECT_LIVE) { diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c index 663e29c..fc19378 100644 --- a/src/lxc/lxc_native.c +++ b/src/lxc/lxc_native.c @@ -677,9 +677,11 @@ lxcSetCpuTune(virDomainDefPtr def, virConfPtr properties) virConfValuePtr value; if ((value = virConfGetValue(properties, "lxc.cgroup.cpu.shares")) && - value->str && virStrToLong_ul(value->str, NULL, 10, - &def->cputune.shares) < 0) - goto error; + value->str) { + if (virStrToLong_ul(value->str, NULL, 10, &def->cputune.shares) < 0) + goto error; + def->cputune.sharesSpecified = true; + } if ((value = virConfGetValue(properties, "lxc.cgroup.cpu.cfs_quota_us")) && diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c index 2cba3ca..8cb68ce 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -2046,6 +2046,7 @@ parallelsApplyChanges(virConnectPtr conn, virDomainObjPtr dom, virDomainDefPtr n } if (old->cputune.shares != new->cputune.shares || + old->cputune.sharesSpecified != new->cputune.sharesSpecified || old->cputune.period != new->cputune.period || old->cputune.quota != new->cputune.quota || old->cputune.nvcpupin != new->cputune.nvcpupin) { diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index a97f184..738503a 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -644,7 +644,7 @@ qemuSetupCpuCgroup(virDomainObjPtr vm) qemuDomainObjPrivatePtr priv = vm->privateData; if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) { - if (vm->def->cputune.shares) { + if (vm->def->cputune.sharesSpecified) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("CPU tuning is not available on this host")); return -1; @@ -653,7 +653,7 @@ qemuSetupCpuCgroup(virDomainObjPtr vm) } } - if (vm->def->cputune.shares && + if (vm->def->cputune.sharesSpecified && virCgroupSetCpuShares(priv->cgroup, vm->def->cputune.shares) < 0) return -1; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index aa1a3db..8fc52a0 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7775,7 +7775,7 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; } - if (def->cputune.shares || def->cputune.period || + if (def->cputune.sharesSpecified || def->cputune.period || def->cputune.quota || def->cputune.emulator_period || def->cputune.emulator_quota) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ba470a1..970f698 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9039,10 +9039,14 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, if (virCgroupSetCpuShares(priv->cgroup, value_ul) < 0) goto cleanup; vm->def->cputune.shares = value_ul; + vm->def->cputune.sharesSpecified = true; } - if (flags & VIR_DOMAIN_AFFECT_CONFIG) + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { vmdef->cputune.shares = value_ul; + vmdef->cputune.sharesSpecified = true; + } + } else if (STREQ(param->field, VIR_DOMAIN_SCHEDULER_VCPU_PERIOD)) { SCHED_RANGE_CHECK(value_ul, VIR_DOMAIN_SCHEDULER_VCPU_PERIOD, diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index 8fb2a93..281aae4 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -1504,6 +1504,7 @@ virVMXParseConfig(virVMXContext *ctx, "found '%s'"), sched_cpu_shares); goto cleanup; } + def->cputune.sharesSpecified = true; } /* def:lifecycle */ @@ -3175,7 +3176,7 @@ virVMXFormatConfig(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virDomainDe } /* def:cputune.shares -> vmx:sched.cpu.shares */ - if (def->cputune.shares > 0) { + if (def->cputune.sharesSpecified) { /* See http://www.vmware.com/support/developer/vc-sdk/visdk41pubs/ApiReference/vim.... */ if (def->cputune.shares == def->vcpus * 500) { virBufferAddLit(&buffer, "sched.cpu.shares = \"low\"\n"); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-cputune-zero-shares.args b/tests/qemuxml2argvdata/qemuxml2argv-cputune-zero-shares.args new file mode 100644 index 0000000..bc6d241 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-cputune-zero-shares.args @@ -0,0 +1,5 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ +/usr/bin/qemu \ +-name QEMUGuest1 -S -M pc -m 214 -smp 2 -nographic \ +-monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb \ +-hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-cputune-zero-shares.xml b/tests/qemuxml2argvdata/qemuxml2argv-cputune-zero-shares.xml new file mode 100644 index 0000000..d597054 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-cputune-zero-shares.xml @@ -0,0 +1,35 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>2</vcpu> + <cputune> + <shares>0</shares> + <period>1000000</period> + <quota>-1</quota> + <vcpupin vcpu='0' cpuset='0'/> + <vcpupin vcpu='1' cpuset='1'/> + <emulatorpin cpuset='1'/> + </cputune> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 5d6a64b..bedfbb1 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1163,6 +1163,7 @@ mymain(void) DO_TEST("blkiotune", QEMU_CAPS_NAME); DO_TEST("blkiotune-device", QEMU_CAPS_NAME); DO_TEST("cputune", QEMU_CAPS_NAME); + DO_TEST("cputune-zero-shares", QEMU_CAPS_NAME); DO_TEST("numatune-memory", NONE); DO_TEST("numatune-auto-nodeset-invalid", NONE); DO_TEST("numad", NONE); diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 8e8fe71..9a16289 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -260,6 +260,7 @@ mymain(void) DO_TEST("blkiotune"); DO_TEST("blkiotune-device"); DO_TEST("cputune"); + DO_TEST("cputune-zero-shares"); DO_TEST("smp"); DO_TEST("lease"); -- 1.8.3.2
On 14.03.2014 16:03, Ján Tomko wrote:
Currently, <cputune><shares>0</shares></cputune> is treated as if it were not specified.
Treat is as a valid value if it was explicitly specified and write it to the cgroups. --- src/conf/domain_conf.c | 12 ++++---- src/conf/domain_conf.h | 1 + src/lxc/lxc_cgroup.c | 2 +- src/lxc/lxc_driver.c | 2 ++ src/lxc/lxc_native.c | 8 +++-- src/parallels/parallels_driver.c | 1 + src/qemu/qemu_cgroup.c | 4 +-- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_driver.c | 6 +++- src/vmx/vmx.c | 3 +- .../qemuxml2argv-cputune-zero-shares.args | 5 ++++ .../qemuxml2argv-cputune-zero-shares.xml | 35 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + tests/qemuxml2xmltest.c | 1 + 14 files changed, 69 insertions(+), 14 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-cputune-zero-shares.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-cputune-zero-shares.xml
ACK Michal
--- src/lxc/lxc_cgroup.c | 13 ++++++++++--- src/lxc/lxc_driver.c | 6 +++++- src/qemu/qemu_cgroup.c | 12 +++++++++--- src/qemu/qemu_driver.c | 7 ++++++- 4 files changed, 30 insertions(+), 8 deletions(-) diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c index 876c32e..5887b24 100644 --- a/src/lxc/lxc_cgroup.c +++ b/src/lxc/lxc_cgroup.c @@ -36,9 +36,16 @@ static int virLXCCgroupSetupCpuTune(virDomainDefPtr def, virCgroupPtr cgroup) { int ret = -1; - if (def->cputune.sharesSpecified && - virCgroupSetCpuShares(cgroup, def->cputune.shares) < 0) - goto cleanup; + + if (def->cputune.sharesSpecified) { + unsigned long long val; + if (virCgroupSetCpuShares(cgroup, def->cputune.shares) < 0) + goto cleanup; + + if (virCgroupGetCpuShares(cgroup, &val) < 0) + goto cleanup; + def->cputune.shares = val; + } if (def->cputune.quota != 0 && virCgroupSetCpuCfsQuota(cgroup, def->cputune.quota) < 0) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 7c8e7c0..46b3fc0 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1888,10 +1888,14 @@ lxcDomainSetSchedulerParametersFlags(virDomainPtr dom, if (STREQ(param->field, VIR_DOMAIN_SCHEDULER_CPU_SHARES)) { if (flags & VIR_DOMAIN_AFFECT_LIVE) { + unsigned long long val; if (virCgroupSetCpuShares(priv->cgroup, params[i].value.ul) < 0) goto cleanup; - vm->def->cputune.shares = params[i].value.ul; + if (virCgroupGetCpuShares(priv->cgroup, &val) < 0) + goto cleanup; + + vm->def->cputune.shares = val; vm->def->cputune.sharesSpecified = true; } diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 738503a..7e88aea 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -653,9 +653,15 @@ qemuSetupCpuCgroup(virDomainObjPtr vm) } } - if (vm->def->cputune.sharesSpecified && - virCgroupSetCpuShares(priv->cgroup, vm->def->cputune.shares) < 0) - return -1; + if (vm->def->cputune.sharesSpecified) { + unsigned long long val; + if (virCgroupSetCpuShares(priv->cgroup, vm->def->cputune.shares) < 0) + return -1; + + if (virCgroupGetCpuShares(priv->cgroup, &val) < 0) + return -1; + vm->def->cputune.shares = val; + } return 0; } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 970f698..0cf25cb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9036,9 +9036,14 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, if (STREQ(param->field, VIR_DOMAIN_SCHEDULER_CPU_SHARES)) { if (flags & VIR_DOMAIN_AFFECT_LIVE) { + unsigned long long val; if (virCgroupSetCpuShares(priv->cgroup, value_ul) < 0) goto cleanup; - vm->def->cputune.shares = value_ul; + + if (virCgroupGetCpuShares(priv->cgroup, &val) < 0) + goto cleanup; + + vm->def->cputune.shares = val; vm->def->cputune.sharesSpecified = true; } -- 1.8.3.2
On 14.03.2014 16:03, Ján Tomko wrote:
--- src/lxc/lxc_cgroup.c | 13 ++++++++++--- src/lxc/lxc_driver.c | 6 +++++- src/qemu/qemu_cgroup.c | 12 +++++++++--- src/qemu/qemu_driver.c | 7 ++++++- 4 files changed, 30 insertions(+), 8 deletions(-)
I'd expect some explanation in the commit message why are we bothering with this. ACK with the commit message extended. Michal
On 03/25/2014 05:22 PM, Michal Privoznik wrote:
On 14.03.2014 16:03, Ján Tomko wrote:
--- src/lxc/lxc_cgroup.c | 13 ++++++++++--- src/lxc/lxc_driver.c | 6 +++++- src/qemu/qemu_cgroup.c | 12 +++++++++--- src/qemu/qemu_driver.c | 7 ++++++- 4 files changed, 30 insertions(+), 8 deletions(-)
I'd expect some explanation in the commit message why are we bothering with this.
ACK with the commit message extended.
Thanks, I've added the following to the commit message and pushed the series: Currently, the Linux kernel treats values of '0' and '1' as the minimum of 2. Values larger than the maximum are changed to the maximum. Re-reading the shares value after setting it reflects this in the live domain XML. Jan
participants (2)
-
Ján Tomko -
Michal Privoznik