Re: [libvirt] [PATCH 5/5] blkiotune: add qemu support for blkiotune.device_weight
On 11/08/2011 06:05 AM, Stefan Berger wrote:
On 11/08/2011 06:00 AM, Hu Tao wrote:
Implement setting/getting per-device blkio weights in qemu,
using the cgroups blkio.weight_device tunable.
---
src/libvirt_private.syms |1 +
src/qemu/qemu_cgroup.c | 22 +
src/qemu/qemu_driver.c | 216
-
src/util/cgroup.c| 20
src/util/cgroup.h|3 +
5 files changed, 257 insertions(+), 5 deletions(-)
In addition to Stefan's comments, which I fixed as recommended, I had
some additional comments. In particular, qemu's set blkio parameters
had a pre-existing bug, where using 'virsh blkiotune --live
--persistent' failed to update persistent state, which I noticed in
testing this (a simple matter of removing a bogus 'else').
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -312,6 +312,28 @@ int qemuSetupCgroup(struct qemud_driver *driver,
}
}
+if (qemuCgroupControllerActive(driver,
VIR_CGROUP_CONTROLLER_BLKIO)) {
+char *tmp;
+if (virBlkioDeviceWeightToStr(vm-def-blkio.devices,
+ vm-def-blkio.ndevices,
+tmp) 0) {
+qemuReportError(VIR_ERR_INTERNAL_ERROR,
+_(Unable to set io device weight for
domain %s),
+vm-def-name);
The ToStr function doesn't report an OOM error. So this is ok. I wonder
whether that function should report an OOM error, though? Eric?
I fixed that in my review of 4/5 to have ToStr always report error, so
no need to report error here.
+goto cleanup;
+}
+if (tmp) {
I think you can remove the if branch.
Not entirely true. virBlkioDeviceWeightToStr sets tmp to NULL and
returns 0 if vm-def-blkio.ndevices is 0. But in that case, why even
bother going into this entire block of code?
@@ -5978,6 +6058,53 @@ static int
qemuDomainSetBlkioParameters(virDomainPtr dom,
_(unable to set blkio
weight tunable));
ret = -1;
}
+} else if (STREQ(param-field,
VIR_DOMAIN_BLKIO_DEVICE_WEIGHT)) {
+int ndevices;
+virBlkioDeviceWeightPtr devices = NULL;
+char *tmp;
+if (param-type != VIR_TYPED_PARAM_STRING) {
+qemuReportError(VIR_ERR_INVALID_ARG, %s,
+_(invalid type for device_weight
tunable, expected a 'char *'));
+ret = -1;
+continue;
+}
+
+if (parseBlkioWeightDeviceStr(params[i].value.s,
+devices,
+ndevices) 0) {
+ret = -1;
+continue;
+}
+if (virBlkioDeviceWeightToStr(devices,
+ ndevices,
+tmp) 0) {
+qemuReportError(VIR_ERR_INTERNAL_ERROR,
+_(Unable to set blkio
weight_device tunable));
+virBlkioDeviceWeightArrayClear(devices, ndevices);
+VIR_FREE(devices);
+ret = -1;
+continue;
+}
+if (tmp) {
Useless conditional; here we know tmp is non-NULL because
virBlkioDeviceWeightToStr succeeded.
ACK with those nits fixed.
In my testing, I ran into a weird issue. From the shell:
printf '8:0 500\n8:16 500\n'
/cgroup/blkio/libvirt/qemu/dom/blkio.weight_device
succeeded at altering the cgroup contents. But:
/usr/bin/printf '8:0 500\n8:16 500\n'
/cgroup/blkio/libvirt/qemu/dom/blkio.weight_device
fails, as did the C code, with EINVAL. It took me longer than I care to
admit to realize the issue: bash's printf uses line-buffering mode on
ALL files, and results in two separate write() calls of one line each,
while coreutils' printf and our C code was trying to do two lines at
once and getting rejected by the kernel.
Therefore, we need to split up the code to use cgroups.c for only one
device at a time. Also, when thinking about it more, domain_conf.c is
the wrong place to worry about major() and minor(); util/cgroup.c
already worries about it. So I'm going to propose a v8 of the last two
patches, moving a hunk out of 4/5 into 5/5 so that we only use major()
and minor() in cgroup-specific code, rather than globally in domain_conf.c.
Here's where I got prior to hitting the EINVAL issue with multiple
blocks; I'll be posting further edits in the form of v8 soon.
diff --git i/src/qemu/qemu_cgroup.c w/src/qemu/qemu_cgroup.c
index d397578..99b8105 100644
--- i/src/qemu/qemu_cgroup.c
+++ w/src/qemu/qemu_cgroup.c
@@ -312,7 +312,8 @@ int qemuSetupCgroup(struct qemud_driver *driver,
}
}
-if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_BLKIO)) {
+if (vm-def-blkio.ndevices
+
Re: [libvirt] [PATCH 5/5] blkiotune: add qemu support for blkiotune.device_weight
On 11/08/2011 06:00 AM, Hu Tao wrote:
Implement setting/getting per-device blkio weights in qemu,
using the cgroups blkio.weight_device tunable.
---
src/libvirt_private.syms |1 +
src/qemu/qemu_cgroup.c | 22 +
src/qemu/qemu_driver.c | 216 -
src/util/cgroup.c| 20
src/util/cgroup.h|3 +
5 files changed, 257 insertions(+), 5 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index f7d0fb2..b32d5de 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -89,6 +89,7 @@ virCgroupKillRecursive;
virCgroupMounted;
virCgroupPathOfController;
virCgroupRemove;
+virCgroupSetBlkioDeviceWeight;
virCgroupSetBlkioWeight;
virCgroupSetCpuShares;
virCgroupSetCpuCfsPeriod;
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 2a10bd2..d397578 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -312,6 +312,28 @@ int qemuSetupCgroup(struct qemud_driver *driver,
}
}
+if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_BLKIO)) {
+char *tmp;
+if (virBlkioDeviceWeightToStr(vm-def-blkio.devices,
+ vm-def-blkio.ndevices,
+tmp) 0) {
+qemuReportError(VIR_ERR_INTERNAL_ERROR,
+_(Unable to set io device weight for domain %s),
+vm-def-name);
The ToStr function doesn't report an OOM error. So this is ok. I wonder
whether that function should report an OOM error, though? Eric?
+goto cleanup;
+}
+if (tmp) {
I think you can remove the if branch.
+rc = virCgroupSetBlkioDeviceWeight(cgroup, tmp);
+VIR_FREE(tmp);
+if (rc != 0) {
+virReportSystemError(-rc,
+ _(Unable to set io device weight for domain
%s),
+ vm-def-name);
+goto cleanup;
+}
+}
+}
+
if (vm-def-mem.hard_limit != 0 ||
vm-def-mem.soft_limit != 0 ||
vm-def-mem.swap_hard_limit != 0) {
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index c8a858e..40568d0 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -112,7 +112,7 @@
# define KVM_CAP_NR_VCPUS 9 /* returns max vcpus per vm */
#endif
-#define QEMU_NB_BLKIO_PARAM 1
+#define QEMU_NB_BLKIO_PARAM 2
static void processWatchdogEvent(void *data, void *opaque);
@@ -5888,6 +5888,86 @@ cleanup:
return ret;
}
+/* deviceWeightStr in the form of /device/path,weight,/device/path,weight
+ * for example, /dev/disk/by-path/pci-:00:1f.2-scsi-0:0:0:0,800
+ */
+static int
+parseBlkioWeightDeviceStr(char *deviceWeightStr,
+ virBlkioDeviceWeightPtr *dw, int *size)
+{
+char *temp;
+int ndevices = 0;
+int nsep = 0;
+int i;
+virBlkioDeviceWeightPtr result = NULL;
+
+temp = deviceWeightStr;
+while (temp) {
+temp = strchr(temp, ',');
+if (temp) {
+temp++;
+nsep++;
+}
+}
+
+/* a valid string must have even number of fields */
...and an odd number of commas.
+if (!(nsep 1))
+goto error;
+
+ndevices = (nsep + 1) / 2;
+
+if (VIR_ALLOC_N(result, ndevices) 0) {
+virReportOOMError();
+return -1;
+}
+
+i = 0;
+temp = deviceWeightStr;
+while (temp i ndevices) {
+char *p = temp;
+
+/* device path */
+
+p = strchr(p, ',');
+if (!p)
+goto error;
+
+result[i].path = strndup(temp, p - temp);
+if (!result[i].path) {
+virReportOOMError();
+goto cleanup;
+}
+
+/* weight */
+temp = p + 1;
+
+if (virStrToLong_ui(temp,p, 10,result[i].weight) 0)
+goto error;
+
+i++;
+if (*p == '\0')
+break;
+else if (*p != ',')
+goto error;
+temp = p + 1;
+}
+if (i != ndevices)
+goto error;
+
+*dw = result;
+*size = i;
+
+return 0;
+
+error:
+qemuReportError(VIR_ERR_INVALID_ARG,
+_(unable to parse %s), deviceWeightStr);
+cleanup:
+virBlkioDeviceWeightArrayClear(result, ndevices);
+VIR_FREE(result);
+return -1;
+}
+
static int qemuDomainSetBlkioParameters(virDomainPtr dom,
virTypedParameterPtr params,
int nparams,
@@ -5954,10 +6034,10 @@ static int qemuDomainSetBlkioParameters(virDomainPtr
dom,
ret = 0;
if (flags VIR_DOMAIN_AFFECT_LIVE) {
for (i = 0; i nparams; i++) {
+int rc;
virTypedParameterPtr param =params[i];
if (STREQ(param-field, VIR_DOMAIN_BLKIO_WEIGHT)) {
-int rc;
if (param-type !=
