[libvirt] [PATCHv4 4/4] blkiotune: add qemu support for blkiotune.device_weight
Daniel P. Berrange
berrange at redhat.com
Tue Nov 29 17:14:00 UTC 2011
On Tue, Nov 15, 2011 at 03:22:03PM +0800, Hu Tao wrote:
> On Mon, Nov 14, 2011 at 09:30:02PM -0700, Eric Blake wrote:
> > From: Hu Tao <hutao at cn.fujitsu.com>
> >
> > 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 | 20 ++
> > src/qemu/qemu_driver.c | 218 +++++++++++++++++++-
> > src/util/cgroup.c | 51 +++++
> > src/util/cgroup.h | 4 +
> > .../qemuxml2argv-blkiotune-device.args | 4 +
> > tests/qemuxml2argvtest.c | 1 +
> > tests/qemuxml2xmltest.c | 1 +
> > 8 files changed, 295 insertions(+), 5 deletions(-)
> > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-blkiotune-device.args
> >
> > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> > index d78fd53..3575fe0 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..eda4c66 100644
> > --- a/src/qemu/qemu_cgroup.c
> > +++ b/src/qemu/qemu_cgroup.c
> > @@ -312,6 +312,26 @@ int qemuSetupCgroup(struct qemud_driver *driver,
> > }
> > }
> >
> > + if (vm->def->blkio.ndevices) {
> > + if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_BLKIO)) {
> > + for (i = 0; i < vm->def->blkio.ndevices; i++) {
> > + virBlkioDeviceWeightPtr dw = &vm->def->blkio.devices[i];
> > + rc = virCgroupSetBlkioDeviceWeight(cgroup, dw->path,
> > + dw->weight);
> > + if (rc != 0) {
> > + virReportSystemError(-rc,
> > + _("Unable to set io device weight "
> > + "for domain %s"),
> > + vm->def->name);
> > + goto cleanup;
> > + }
> > + }
> > + } else {
> > + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > + _("Block I/O tuning is not available on this host"));
> > + }
> > + }
> > +
> > 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 b0ce115..43f4041 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);
> >
> > @@ -5885,6 +5885,105 @@ cleanup:
> > return ret;
> > }
> >
> > +/* deviceWeightStr in the form of /device/path,weight,/device/path,weight
> > + * for example, /dev/disk/by-path/pci-0000:00:1f.2-scsi-0:0:0:0,800
> > + */
> > +static int
> > +parseBlkioWeightDeviceStr(char *deviceWeightStr,
> > + virBlkioDeviceWeightPtr *dw, int *size,
> > + virCgroupPtr cgroup)
> > +{
> > + 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, hence 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) {
> > + 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;
> > +
> > + if (cgroup) {
> > + int rc = virCgroupSetBlkioDeviceWeight(cgroup,
> > + result[i].path,
> > + result[i].weight);
>
> Does more than the function name says. Would it be better to just do the
> parsing here, and set the cgroup values after parsing(see my comment to
> patch 3 for filtering out 0-weight when writing to xml):
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 43f4041..275d5c8 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -5890,8 +5890,7 @@ cleanup:
> */
> static int
> parseBlkioWeightDeviceStr(char *deviceWeightStr,
> - virBlkioDeviceWeightPtr *dw, int *size,
> - virCgroupPtr cgroup)
> + virBlkioDeviceWeightPtr *dw, int *size)
> {
> char *temp;
> int ndevices = 0;
> @@ -5942,24 +5941,8 @@ parseBlkioWeightDeviceStr(char *deviceWeightStr,
> if (virStrToLong_ui(temp, &p, 10, &result[i].weight) < 0)
> goto error;
>
> - if (cgroup) {
> - int rc = virCgroupSetBlkioDeviceWeight(cgroup,
> - result[i].path,
> - result[i].weight);
> - if (rc < 0) {
> - virReportSystemError(-rc,
> - _("Unable to set io device weight "
> - "for path %s"),
> - result[i].path);
> - goto cleanup;
> - }
> - }
> + i++;
>
> - /* 0-weight entries only affect cgroups, they don't stick in xml */
> - if (result[i].weight)
> - i++;
> - else
> - VIR_FREE(result[i].path);
> if (*p == '\0')
> break;
> else if (*p != ',')
> @@ -6087,7 +6070,23 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom,
>
> if (parseBlkioWeightDeviceStr(params[i].value.s,
> &devices,
> - &ndevices, group) < 0) {
> + &ndevices) < 0) {
> + ret = -1;
> + continue;
> + }
> + for (i = 0; i < ndevices; i++) {
> + rc = virCgroupSetBlkioDeviceWeight(group,
> + devices[i].path,
> + devices[i].weight);
> + if (rc < 0) {
> + virReportSystemError(-rc,
> + _("Unable to set io device weight "
> + "for path %s"),
> + devices[i].path);
> + break;
> + }
> + }
> + if (i != ndevices) {
> ret = -1;
> continue;
> }
> @@ -6140,7 +6139,7 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom,
> }
> if (parseBlkioWeightDeviceStr(params[i].value.s,
> &devices,
> - &ndevices, NULL) < 0) {
> + &ndevices) < 0) {
> ret = -1;
> continue;
> }
Yes, IMHO it is better to do the setting of values, after parsing
is fully complete.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
More information about the libvir-list
mailing list