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

Re: [libvirt] [PATCH 5/6] qemu: Implement hypervisor's period and quota tunable XML configuration and parsing



On Wed, Apr 25, 2012 at 05:46:06PM +0800, Wen Congyang wrote:
> set hypervisor's period and quota when the vm starts. It will affect to set
> vm's period and quota: donot set vm's period and quota if we limit hypevisor
> thread's bandwidth(hypervisor_quota > 0).
> ---
>  src/conf/domain_conf.c |   25 ++++++++++++++-
>  src/conf/domain_conf.h |    2 +
>  src/qemu/qemu_cgroup.c |   37 ++++++++++++++++-------
>  src/qemu/qemu_driver.c |   75 ++++++++++++++++++++++++++++++------------------
>  4 files changed, 98 insertions(+), 41 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 5ab052a..28a6436 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -7931,6 +7931,14 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
>                           &def->cputune.quota) < 0)
>          def->cputune.quota = 0;
>  
> +    if (virXPathULongLong("string(./cputune/hypervisor_period[1])", ctxt,
> +                          &def->cputune.hypervisor_period) < 0)
> +        def->cputune.hypervisor_period = 0;
> +
> +    if (virXPathLongLong("string(./cputune/hypervisor_quota[1])", ctxt,
> +                         &def->cputune.hypervisor_quota) < 0)
> +        def->cputune.hypervisor_quota = 0;
> +
>      if ((n = virXPathNodeSet("./cputune/vcpupin", ctxt, &nodes)) < 0) {
>          goto error;
>      }
> @@ -12406,7 +12414,8 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>      virBufferAsprintf(buf, ">%u</vcpu>\n", def->maxvcpus);
>  
>      if (def->cputune.shares || def->cputune.vcpupin ||
> -        def->cputune.period || def->cputune.quota)
> +        def->cputune.period || def->cputune.quota ||
> +        def->cputune.hypervisor_period || def->cputune.hypervisor_quota)
>          virBufferAddLit(buf, "  <cputune>\n");
>  
>      if (def->cputune.shares)
> @@ -12418,6 +12427,17 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>      if (def->cputune.quota)
>          virBufferAsprintf(buf, "    <quota>%lld</quota>\n",
>                            def->cputune.quota);
> +
> +    if (def->cputune.hypervisor_period)
> +        virBufferAsprintf(buf, "    <hypervisor_period>%llu"
> +                          "</hypervisor_period>\n",
> +                          def->cputune.hypervisor_period);
> +
> +    if (def->cputune.hypervisor_period)
> +        virBufferAsprintf(buf, "    <hypervisor_quota>%lld"
> +                          "</hypervisor_quota>\n",
> +                          def->cputune.hypervisor_quota);
> +
>      if (def->cputune.vcpupin) {
>          for (i = 0; i < def->cputune.nvcpupin; i++) {
>              virBufferAsprintf(buf, "    <vcpupin vcpu='%u' ",
> @@ -12439,7 +12459,8 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>      }
>  
>      if (def->cputune.shares || def->cputune.vcpupin ||
> -        def->cputune.period || def->cputune.quota)
> +        def->cputune.period || def->cputune.quota ||
> +        def->cputune.hypervisor_period || def->cputune.hypervisor_quota)
>          virBufferAddLit(buf, "  </cputune>\n");
>  
>      if (def->numatune.memory.nodemask) {
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 0eed60e..2a37e26 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1558,6 +1558,8 @@ struct _virDomainDef {
>          unsigned long shares;
>          unsigned long long period;
>          long long quota;
> +        unsigned long long hypervisor_period;
> +        long long hypervisor_quota;
>          int nvcpupin;
>          virDomainVcpuPinDefPtr *vcpupin;
>      } cputune;
> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> index 727c0d4..7c6ef33 100644
> --- a/src/qemu/qemu_cgroup.c
> +++ b/src/qemu/qemu_cgroup.c
> @@ -493,17 +493,23 @@ int qemuSetupCgroupForVcpu(struct qemud_driver *driver, virDomainObjPtr vm)
>          goto cleanup;
>      }

Check if cgroup CPU is active here:

       if (!qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_CPU)) {
           virReportSystemError(EINVAL,
	                        _("%s"),
				"Cgroup CPU is not active.");
           goto cleanup;
       }

and remove all following checks in this function.

>  
> -    /* Set cpu bandwidth for the vm */
> -    if (period || quota) {
> -        if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_CPU)) {
> -            /* Ensure that we can multiply by vcpus without overflowing. */
> -            if (quota > LLONG_MAX / vm->def->vcpus) {
> -                virReportSystemError(EINVAL,
> -                                     _("%s"),
> -                                     "Unable to set cpu bandwidth quota");
> -                goto cleanup;
> -            }
> +    /*
> +     * Set cpu bandwidth for the vm if any of the following is true:
> +     * 1. we donot know VCPU<->PID mapping or all vcpus run in the same thread
> +     * 2. the hypervisor threads are not limited(quota <= 0)
> +     */
> +    if ((period || quota) &&
> +        qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_CPU)) {
> +        /* Ensure that we can multiply by vcpus without overflowing. */
> +        if (quota > LLONG_MAX / vm->def->vcpus) {
> +            virReportSystemError(EINVAL,
> +                                 _("%s"),
> +                                 "Unable to set cpu bandwidth quota");
> +            goto cleanup;
> +        }
>  
> +        if (priv->nvcpupids == 0 || priv->vcpupids[0] == vm->pid ||
> +            vm->def->cputune.hypervisor_quota <= 0) {
>              if (quota > 0)
>                  vm_quota = quota * vm->def->vcpus;
>              else
> @@ -514,7 +520,7 @@ int qemuSetupCgroupForVcpu(struct qemud_driver *driver, virDomainObjPtr vm)
>      }
>  
>      if (priv->nvcpupids == 0 || priv->vcpupids[0] == vm->pid) {
> -        /* If we does not know VCPU<->PID mapping or all vcpu runs in the same
> +        /* If we donot know VCPU<->PID mapping or all vcpus run in the same
>           * thread, we cannot control each vcpu.
>           */
>          virCgroupFree(&cgroup);
> @@ -570,6 +576,8 @@ int qemuSetupCgroupForHypervisor(struct qemud_driver *driver,
>      virCgroupPtr cgroup = NULL;
>      virCgroupPtr cgroup_hypervisor = NULL;
>      qemuDomainObjPrivatePtr priv = vm->privateData;
> +    unsigned long long period = vm->def->cputune.hypervisor_period;
> +    long long quota = vm->def->cputune.hypervisor_quota;
>      int rc;
>  
>      if (driver->cgroup == NULL)
> @@ -608,6 +616,13 @@ int qemuSetupCgroupForHypervisor(struct qemud_driver *driver,
>          goto cleanup;
>      }
>  
> +    if (period || quota) {
> +        if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_CPU)) {
> +            if (qemuSetupCgroupVcpuBW(cgroup_hypervisor, period, quota) < 0)

Actually, qemuSetupCgroupVcpuBW just changes the cgroup's
settings(period, quota), it doesn't care about what the
cgroup is associated with. So can we change the name to
qemuSetupCgroupBW?

> +                goto cleanup;
> +        }
> +    }
> +
>      virCgroupFree(&cgroup_hypervisor);
>      virCgroupFree(&cgroup);
>      return 0;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index c3555ca..2e40aee 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -7007,6 +7007,7 @@ qemuSetVcpusBWLive(virDomainObjPtr vm, virCgroupPtr cgroup,
>      int rc;
>      long long vm_quota = 0;
>      long long old_quota = 0;
> +    long long hypervisor_quota = vm->def->cputune.hypervisor_quota;
>      unsigned long long old_period = 0;
>  
>      if (period == 0 && quota == 0)
> @@ -7039,6 +7040,16 @@ qemuSetVcpusBWLive(virDomainObjPtr vm, virCgroupPtr cgroup,
>          goto cleanup;
>      }
>  
> +    /* If we donot know VCPU<->PID mapping or all vcpu runs in the same
> +     * thread, we cannot control each vcpu. So we only modify cpu bandwidth
> +     * for the vm.
> +     */
> +    if (priv->nvcpupids == 0 || priv->vcpupids[0] == vm->pid) {
> +        if (qemuSetupCgroupVcpuBW(cgroup, period, vm_quota) < 0)
> +            goto cleanup;
> +        return 0;
> +    }
> +
>      /*
>       * If quota will be changed to a small value, we should modify vcpu's quota
>       * first. Otherwise, we should modify vm's quota first.
> @@ -7048,8 +7059,12 @@ qemuSetVcpusBWLive(virDomainObjPtr vm, virCgroupPtr cgroup,
>       *
>       * If both quota and period will be changed to a big/small value, we cannot
>       * modify period and quota together.
> +     *
> +     * If the hypervisor threads are limited, we can update period for vm first,
> +     * and then we can modify period and quota for the vcpu together even if
> +     * they will be changed to a big/small value.
>       */
> -    if ((quota != 0) && (period != 0)) {
> +    if (hypervisor_quota <= 0 && quota != 0 && period != 0) {
>          if (((quota > old_quota) && (period > old_period)) ||
>              ((quota < old_quota) && (period < old_period))) {
>              /* modify period */
> @@ -7063,40 +7078,44 @@ qemuSetVcpusBWLive(virDomainObjPtr vm, virCgroupPtr cgroup,
>          }
>      }
>  
> -    if (((vm_quota != 0) && (vm_quota > old_quota)) ||
> -        ((period != 0) && (period < old_period)))
> -        /* Set cpu bandwidth for the vm */
> -        if (qemuSetupCgroupVcpuBW(cgroup, period, vm_quota) < 0)
> -            goto cleanup;
> -
> -    /* If we does not know VCPU<->PID mapping or all vcpu runs in the same
> -     * thread, we cannot control each vcpu. So we only modify cpu bandwidth
> -     * when each vcpu has a separated thread.
> +    /*
> +     * If the hypervisor threads are not limited, set cpu bandwidth for the
> +     * vm.
>       */
> -    if (priv->nvcpupids != 0 && priv->vcpupids[0] != vm->pid) {
> -        for (i = 0; i < priv->nvcpupids; i++) {
> -            rc = virCgroupForVcpu(cgroup, i, &cgroup_vcpu, 0);
> -            if (rc < 0) {
> -                virReportSystemError(-rc,
> -                                     _("Unable to find vcpu cgroup for %s(vcpu:"
> -                                       " %d)"),
> -                                     vm->def->name, i);
> -                goto cleanup;
> -            }
> -
> -            if (qemuSetupCgroupVcpuBW(cgroup_vcpu, period, quota) < 0)
> +    if (vm->def->cputune.hypervisor_quota <= 0) {
> +        if (((vm_quota != 0) && (vm_quota > old_quota)) ||
> +            ((period != 0) && (period < old_period)))
> +            if (qemuSetupCgroupVcpuBW(cgroup, period, vm_quota) < 0)
>                  goto cleanup;
> +    }
>  
> -            virCgroupFree(&cgroup_vcpu);
> +    /* each vcpu has a separated thread, so we modify cpu bandwidth for vcpu. */
> +    for (i = 0; i < priv->nvcpupids; i++) {
> +        rc = virCgroupForVcpu(cgroup, i, &cgroup_vcpu, 0);
> +        if (rc < 0) {
> +            virReportSystemError(-rc,
> +                                 _("Unable to find vcpu cgroup for %s(vcpu:"
> +                                   " %d)"),
> +                                 vm->def->name, i);
> +            goto cleanup;
>          }
> -    }
>  
> -    if (((vm_quota != 0) && (vm_quota <= old_quota)) ||
> -        ((period != 0) && (period >= old_period)))
> -        /* Set cpu bandwidth for the vm */
> -        if (qemuSetupCgroupVcpuBW(cgroup, period, vm_quota) < 0)
> +        if (qemuSetupCgroupVcpuBW(cgroup_vcpu, period, quota) < 0)
>              goto cleanup;
>  
> +        virCgroupFree(&cgroup_vcpu);
> +    }
> +
> +    /*
> +     * If the hypervisor threads are not limited, set cpu bandwidth for the vm.
> +     */

This makes me confused. Why do we have to limit the whole vm when the
hypervisor threads are not limited? Users may want to limit the vcpus
only.

We have 3 situations now:

1. limit the vcpus only.
2. limit the hypervisor but not vcpus.
3. limit the whole vm.

I'm not thinking this patch is capable with all of these 3 situations.

> +    if (hypervisor_quota <= 0) {
> +        if (((vm_quota != 0) && (vm_quota <= old_quota)) ||
> +            ((period != 0) && (period >= old_period)))
> +            if (qemuSetupCgroupVcpuBW(cgroup, period, vm_quota) < 0)
> +                goto cleanup;
> +    }
> +
>      return 0;
>  
>  cleanup:
> -- 
> 1.7.1
> 
> --
> libvir-list mailing list
> libvir-list redhat com
> https://www.redhat.com/mailman/listinfo/libvir-list

-- 
Thanks,
Hu Tao


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