At 05/17/2012 12:46 PM, KAMEZAWA Hiroyuki Wrote:
> (2012/05/17 12:54), Wen Congyang wrote:
>
>> At 05/17/2012 11:13 AM, Hu Tao Wrote:
>>> 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.
>>
>> Before this patch, we limit the whole vm when we limit the vcpus
>> because we donot want to the hypervisor threads eat too much cpu
>> time.
>>
>> So, if we donot limit the hypervisor, the behavior shoule not be
>> changed. So we should limit the whole vm. If the hypervisor threads
>> are limited, there is no need to limit the whole vm.
>>
>
>
> need to tune hypervisor quota to limit vcpu-only quota ?
> Sounds strange to me.
No, current implemetion is:
1. limit vcpu and hypervisor: vm is not limited
2. limit vcpu only: vm is limited
3. limit hypervisor: vm is not limited
4. vcpu and hypervisor are not limited: vm is not limited.
Thanks
Wen Congyang
>
> Thanks,
> -Kame
>
>
>
>
>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list