8:11 a.m.
This series adds range checking to cpu scheduler tunables when changed using
the API. V2 contains a new patch that avoids changing all tunables from virsh
when setting just a single one.
Peter Krempa (3):
qemu: clean up qemuSetSchedulerParametersFlags()
qemu: Add range checking for scheduler tunables when changed by API
virsh: Update only changed scheduler tunables
src/qemu/qemu_driver.c | 96 ++++++++++++++++++++++++++++++--------------------
tools/virsh-domain.c | 14 ++++----
2 files changed, 66 insertions(+), 44 deletions(-)
--
1.7.12
8:11 a.m.
New subject: [libvirt] [PATCHv2 1/3] qemu: clean up qemuSetSchedulerParametersFlags()
This patch tries to clean the code up a little bit and shorten very long
lines.
The apparent semantic change from moving the condition before calling
the setter function is a non-issue here as the setter function is a
no-op when called with both arguments zero.
---
Diff to v1:
- enhanced commit message to explain apparent semantic change
---
src/qemu/qemu_driver.c | 69 +++++++++++++++++++++++---------------------------
1 file changed, 31 insertions(+), 38 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b12d9bc..4b8b751 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7801,6 +7801,8 @@ qemuSetSchedulerParametersFlags(virDomainPtr dom,
virCgroupPtr group = NULL;
virDomainObjPtr vm = NULL;
virDomainDefPtr vmdef = NULL;
+ unsigned long long value_ul;
+ long long value_l;
int ret = -1;
int rc;
@@ -7857,74 +7859,65 @@ qemuSetSchedulerParametersFlags(virDomainPtr dom,
for (i = 0; i < nparams; i++) {
virTypedParameterPtr param = ¶ms[i];
+ value_ul = param->value.ul;
+ value_l = param->value.l;
if (STREQ(param->field, VIR_DOMAIN_SCHEDULER_CPU_SHARES)) {
if (flags & VIR_DOMAIN_AFFECT_LIVE) {
- rc = virCgroupSetCpuShares(group, params[i].value.ul);
- if (rc != 0) {
+ if ((rc = virCgroupSetCpuShares(group, value_ul))) {
virReportSystemError(-rc, "%s",
_("unable to set cpu shares
tunable"));
goto cleanup;
}
-
- vm->def->cputune.shares = params[i].value.ul;
+ vm->def->cputune.shares = value_ul;
}
- if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
- vmdef->cputune.shares = params[i].value.ul;
- }
+ if (flags & VIR_DOMAIN_AFFECT_CONFIG)
+ vmdef->cputune.shares = value_ul;
+
} else if (STREQ(param->field, VIR_DOMAIN_SCHEDULER_VCPU_PERIOD)) {
- if (flags & VIR_DOMAIN_AFFECT_LIVE) {
- rc = qemuSetVcpusBWLive(vm, group, params[i].value.ul, 0);
- if (rc != 0)
+ if (flags & VIR_DOMAIN_AFFECT_LIVE && value_ul) {
+ if ((rc = qemuSetVcpusBWLive(vm, group, value_ul, 0)))
goto cleanup;
- if (params[i].value.ul)
- vm->def->cputune.period = params[i].value.ul;
+ vm->def->cputune.period = value_ul;
}
- if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
+ if (flags & VIR_DOMAIN_AFFECT_CONFIG)
vmdef->cputune.period = params[i].value.ul;
- }
+
} else if (STREQ(param->field, VIR_DOMAIN_SCHEDULER_VCPU_QUOTA)) {
- if (flags & VIR_DOMAIN_AFFECT_LIVE) {
- rc = qemuSetVcpusBWLive(vm, group, 0, params[i].value.l);
- if (rc != 0)
+ if (flags & VIR_DOMAIN_AFFECT_LIVE && value_l) {
+ if ((rc = qemuSetVcpusBWLive(vm, group, 0, value_l)))
goto cleanup;
- if (params[i].value.l)
- vm->def->cputune.quota = params[i].value.l;
+ vm->def->cputune.quota = value_l;
}
- if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
- vmdef->cputune.quota = params[i].value.l;
- }
+ if (flags & VIR_DOMAIN_AFFECT_CONFIG)
+ vmdef->cputune.quota = value_l;
+
} else if (STREQ(param->field, VIR_DOMAIN_SCHEDULER_EMULATOR_PERIOD)) {
- if (flags & VIR_DOMAIN_AFFECT_LIVE) {
- rc = qemuSetEmulatorBandwidthLive(vm, group, params[i].value.ul, 0);
- if (rc != 0)
+ if (flags & VIR_DOMAIN_AFFECT_LIVE && value_ul) {
+ if ((rc = qemuSetEmulatorBandwidthLive(vm, group, value_ul, 0)))
goto cleanup;
- if (params[i].value.ul)
- vm->def->cputune.emulator_period = params[i].value.ul;
+ vm->def->cputune.emulator_period = value_ul;
}
- if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
- vmdef->cputune.emulator_period = params[i].value.ul;
- }
+ if (flags & VIR_DOMAIN_AFFECT_CONFIG)
+ vmdef->cputune.emulator_period = value_ul;
+
} else if (STREQ(param->field, VIR_DOMAIN_SCHEDULER_EMULATOR_QUOTA)) {
- if (flags & VIR_DOMAIN_AFFECT_LIVE) {
- rc = qemuSetEmulatorBandwidthLive(vm, group, 0, params[i].value.l);
- if (rc != 0)
+ if (flags & VIR_DOMAIN_AFFECT_LIVE && value_l) {
+ if ((rc = qemuSetEmulatorBandwidthLive(vm, group, 0, value_l)))
goto cleanup;
- if (params[i].value.l)
- vm->def->cputune.emulator_quota = params[i].value.l;
+ vm->def->cputune.emulator_quota = value_l;
}
- if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
- vmdef->cputune.emulator_quota = params[i].value.l;
- }
+ if (flags & VIR_DOMAIN_AFFECT_CONFIG)
+ vmdef->cputune.emulator_quota = value_l;
}
}
--
1.7.12
8:11 a.m.
New subject: [libvirt] [PATCHv2 2/3] qemu: Add range checking for scheduler tunables when changed by API
The quota and period tunables for cpu scheduler accept only a certain
range of values. When changing the live configuration invalid values get
rejected. This check is not performed when changing persistent config.
This patch adds a separate range check, that improves error messages
when changing live config and adds the check for persistent config.
This check is done only when using the API. It is still possible to
specify invalid values in the XML.
---
Diff to v1:
- changed typecast to native long long specification
---
src/qemu/qemu_driver.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4b8b751..8e8e00c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -106,6 +106,11 @@
#define QEMU_NB_TOTAL_CPU_STAT_PARAM 3
#define QEMU_NB_PER_CPU_STAT_PARAM 2
+#define QEMU_SCHED_MIN_PERIOD 1000LL
+#define QEMU_SCHED_MAX_PERIOD 1000000LL
+#define QEMU_SCHED_MIN_QUOTA 1000LL
+#define QEMU_SCHED_MAX_QUOTA 18446744073709551LL
+
#if HAVE_LINUX_KVM_H
# include <linux/kvm.h>
#endif
@@ -7790,6 +7795,15 @@ cleanup:
return -1;
}
+#define SCHED_RANGE_CHECK(VAR, NAME, MIN, MAX) \
+ if (((VAR) > 0 && (VAR) < (MIN)) || (VAR) > (MAX)) {
\
+ virReportError(VIR_ERR_INVALID_ARG, \
+ _("value of '%s' is out of range [%lld, %lld]"),
\
+ NAME, MIN, MAX); \
+ rc = -1; \
+ goto cleanup; \
+ }
+
static int
qemuSetSchedulerParametersFlags(virDomainPtr dom,
virTypedParameterPtr params,
@@ -7876,6 +7890,9 @@ qemuSetSchedulerParametersFlags(virDomainPtr dom,
vmdef->cputune.shares = value_ul;
} else if (STREQ(param->field, VIR_DOMAIN_SCHEDULER_VCPU_PERIOD)) {
+ SCHED_RANGE_CHECK(value_ul, VIR_DOMAIN_SCHEDULER_VCPU_PERIOD,
+ QEMU_SCHED_MIN_PERIOD, QEMU_SCHED_MAX_PERIOD);
+
if (flags & VIR_DOMAIN_AFFECT_LIVE && value_ul) {
if ((rc = qemuSetVcpusBWLive(vm, group, value_ul, 0)))
goto cleanup;
@@ -7887,6 +7904,9 @@ qemuSetSchedulerParametersFlags(virDomainPtr dom,
vmdef->cputune.period = params[i].value.ul;
} else if (STREQ(param->field, VIR_DOMAIN_SCHEDULER_VCPU_QUOTA)) {
+ SCHED_RANGE_CHECK(value_l, VIR_DOMAIN_SCHEDULER_VCPU_QUOTA,
+ QEMU_SCHED_MIN_QUOTA, QEMU_SCHED_MAX_QUOTA);
+
if (flags & VIR_DOMAIN_AFFECT_LIVE && value_l) {
if ((rc = qemuSetVcpusBWLive(vm, group, 0, value_l)))
goto cleanup;
@@ -7898,6 +7918,9 @@ qemuSetSchedulerParametersFlags(virDomainPtr dom,
vmdef->cputune.quota = value_l;
} else if (STREQ(param->field, VIR_DOMAIN_SCHEDULER_EMULATOR_PERIOD)) {
+ SCHED_RANGE_CHECK(value_ul, VIR_DOMAIN_SCHEDULER_EMULATOR_PERIOD,
+ QEMU_SCHED_MIN_PERIOD, QEMU_SCHED_MAX_PERIOD);
+
if (flags & VIR_DOMAIN_AFFECT_LIVE && value_ul) {
if ((rc = qemuSetEmulatorBandwidthLive(vm, group, value_ul, 0)))
goto cleanup;
@@ -7909,6 +7932,9 @@ qemuSetSchedulerParametersFlags(virDomainPtr dom,
vmdef->cputune.emulator_period = value_ul;
} else if (STREQ(param->field, VIR_DOMAIN_SCHEDULER_EMULATOR_QUOTA)) {
+ SCHED_RANGE_CHECK(value_l, VIR_DOMAIN_SCHEDULER_EMULATOR_QUOTA,
+ QEMU_SCHED_MIN_QUOTA, QEMU_SCHED_MAX_QUOTA);
+
if (flags & VIR_DOMAIN_AFFECT_LIVE && value_l) {
if ((rc = qemuSetEmulatorBandwidthLive(vm, group, 0, value_l)))
goto cleanup;
@@ -7944,6 +7970,7 @@ cleanup:
qemuDriverUnlock(driver);
return ret;
}
+#undef SCHED_RANGE_CHECK
static int
qemuSetSchedulerParameters(virDomainPtr dom,
--
1.7.12
8:11 a.m.
New subject: [libvirt] [PATCHv1 3/3] virsh: Update only changed scheduler tunables
When setting the cpu tunables in virsh you are able to update only one
of them. Virsh while doing the update updated all of the tunables with
old values except for the one changed. This is unfortunate as it:
a) might overwrite some other change by a race condition (unprobable)
b) fails with range checking as some of the old values saved might be
out of range
This patch changes the update procedure so that only the changed value
is updated on the host.
---
New in series.
---
tools/virsh-domain.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index dcf2b8c..2abb457 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -3390,7 +3390,7 @@ cmdSchedinfo(vshControl *ctl, const vshCmd *cmd)
virDomainPtr dom;
virTypedParameterPtr params = NULL;
int nparams = 0;
- int update = 0;
+ int update = -1;
int i, ret;
bool ret_val = false;
unsigned int flags = 0;
@@ -3450,16 +3450,18 @@ cmdSchedinfo(vshControl *ctl, const vshCmd *cmd)
goto cleanup;
if (ret == 1)
- update = 1;
+ update = i;
}
/* Update parameters & refresh data */
- if (update) {
+ if (update >= 0) {
if (flags || current)
- ret = virDomainSetSchedulerParametersFlags(dom, params,
- nparams, flags);
+ ret = virDomainSetSchedulerParametersFlags(dom,
+ &(params[update]),
+ 1, flags);
else
- ret = virDomainSetSchedulerParameters(dom, params, nparams);
+ ret = virDomainSetSchedulerParameters(dom,
+ &(params[update]), 1);
if (ret == -1)
goto cleanup;
--
1.7.12
4461
days inactive
4461
days old
3 comments
1 participants
participants (1)
-
Peter Krempa