[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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 <hutao 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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]