Re: [libvirt] [PATCHv4 4/4] blkiotune: add qemu support for blkiotune.device_weight
On 11/29/2011 10:14 AM, Daniel P. Berrange wrote: >> >> 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): >> > > Yes, IMHO it is better to do the setting of values, after parsing > is fully complete. Okay, the series is now pushed with this last round of improvements included. However, I'm still working on a 5/4. Consider: # virsh blkiotune dom --device-weights /dev/sda,502,/dev/sdb,498 # virsh blkiotune dom --device-weights /dev/sda,503 # virsh blkiotune dom weight : 500 device_weight : /dev/sda,503 # cat /cgroup/blkio/libvirt/qemu/dom/blkio.weight_device 8:0 503 8:16498 Oops - /dev/sdb wasn't cleared, even though our new query no longer reports it. Calling virsh blkiotune dome --device-weights should either clear all existing weights before writing in just the specified weights of the new string. Or it should only affect the weights of paths specified in the new string, while preserving all pre-existing weights for all other paths. I'm leaning towards the latter (that is, you _have_ to pass an explicit 0 to set in order to remove /dev/sdb from the XML and from get, and that mentioning just /dev/sda on set leaves /dev/sdb's setting alone). Besides, that's how cgroups does it - write an explicit 0 to remove a device from the cgroup file; all other writes modify a device if it is already listed, otherwise they add a new line to the file. -- Eric Blake ebl...@redhat.com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv4 4/4] blkiotune: add qemu support for blkiotune.device_weight
On 11/15/2011 12:22 AM, Hu Tao wrote:
> On Mon, Nov 14, 2011 at 09:30:02PM -0700, Eric Blake wrote:
>> From: Hu Tao
>>
>> Implement setting/getting per-device blkio weights in qemu,
>> using the cgroups blkio.weight_device tunable.
>
> 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):
ACK to your improvements for setting, but you forgot to do 0-weight
filtering from the getting side:
diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c
index a8fea6c..105bdde 100644
--- i/src/qemu/qemu_driver.c
+++ w/src/qemu/qemu_driver.c
@@ -6270,9 +6270,14 @@ static int
qemuDomainGetBlkioParameters(virDomainPtr dom,
case 1: /* blkiotune.device_weight */
if (vm->def->blkio.ndevices > 0) {
virBuffer buf = VIR_BUFFER_INITIALIZER;
+bool comma = false;
for (j = 0; j < vm->def->blkio.ndevices; j++) {
-if (j)
+if (!vm->def->blkio.devices[j].weight)
+continue;
+if (comma)
virBufferAddChar(&buf, ',');
+else
+comma = true;
virBufferAsprintf(&buf, "%s,%u",
vm->def->blkio.devices[j].path,
vm->def->blkio.devices[j].weight);
@@ -6282,7 +6287,8 @@ static int
qemuDomainGetBlkioParameters(virDomainPtr dom,
goto cleanup;
}
param->value.s = virBufferContentAndReset(&buf);
-} else {
+}
+if (!param->value.s) {
param->value.s = strdup("");
if (!param->value.s) {
virReportOOMError();
--
Eric Blake ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv4 4/4] blkiotune: add qemu support for blkiotune.device_weight
On 11/14/2011 09:30 PM, Eric Blake wrote:
> From: Hu Tao
>
> Implement setting/getting per-device blkio weights in qemu,
> using the cgroups blkio.weight_device tunable.
> +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"));
> +}
Copy and paste, but this should 'goto cleanup' to ensure the error is
propagated.
--
Eric Blake ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv4 4/4] blkiotune: add qemu support for blkiotune.device_weight
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
> >
> > 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-: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,
> > +
Re: [libvirt] [PATCHv4 4/4] blkiotune: add qemu support for blkiotune.device_weight
On Mon, Nov 14, 2011 at 09:30:02PM -0700, Eric Blake wrote: > From: Hu Tao > > 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 ACK 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv4 4/4] blkiotune: add qemu support for blkiotune.device_weight
On 11/15/2011 03:33 PM, Eric Blake wrote: > On 11/15/2011 12:22 AM, Hu Tao wrote: >> On Mon, Nov 14, 2011 at 09:30:02PM -0700, Eric Blake wrote: >>> From: Hu Tao >>> >>> Implement setting/getting per-device blkio weights in qemu, >>> using the cgroups blkio.weight_device tunable. > >>> >>> +/* 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, >>> + virCgroupPtr cgroup) > >> >> 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): > > Either approach works for me (renaming this function more accurately so > that the domain_conf never has 0-weight entries, or going with your > improvements to allow 0-weight entries in the domain_conf def but not > expose them in the XML). I guess whoever reviews this series can cast > the deciding vote on which of the two approaches we commit. Ping - this series seems worth including in 0.9.8; can we get a third-party review? -- Eric Blake ebl...@redhat.com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv4 4/4] blkiotune: add qemu support for blkiotune.device_weight
On 11/15/2011 12:22 AM, Hu Tao wrote: > On Mon, Nov 14, 2011 at 09:30:02PM -0700, Eric Blake wrote: >> From: Hu Tao >> >> Implement setting/getting per-device blkio weights in qemu, >> using the cgroups blkio.weight_device tunable. >> >> +/* 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, >> + virCgroupPtr cgroup) > > 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): Either approach works for me (renaming this function more accurately so that the domain_conf never has 0-weight entries, or going with your improvements to allow 0-weight entries in the domain_conf def but not expose them in the XML). I guess whoever reviews this series can cast the deciding vote on which of the two approaches we commit. -- Eric Blake ebl...@redhat.com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv4 4/4] blkiotune: add qemu support for blkiotune.device_weight
On Mon, Nov 14, 2011 at 09:30:02PM -0700, Eric Blake wrote:
> From: Hu Tao
>
> 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-: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
