[libvirt] [PATCH RESEND RFC v4 5/6] qemu: Implement cfs_period and cfs_quota's modification

[Wen Congyang]

At 07/21/2011 12:34 PM, Daniel Veillard Write:
> On Thu, Jul 21, 2011 at 10:11:32AM +0800, Wen Congyang wrote:
>> ---
>>  src/qemu/qemu_driver.c |  312 ++++++++++++++++++++++++++++++++++++++++++++----
>>  1 files changed, 287 insertions(+), 25 deletions(-)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index cd65bce..fd80537 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -5139,11 +5139,46 @@ cleanup:
>>  }
>>  
>>  
>> +/*
>> + * check whether the host supports CFS bandwidth
>> + *
>> + * Return 1 when CFS bandwidth is supported, 0 when CFS bandwidth is not
>> + * supported, -1 on error.
>> + */
>> +static int qemuGetCpuBWStatus(virCgroupPtr cgroup)
>> +{
>> +    char *cfs_period_path = NULL;
>> +    int ret = -1;
>> +
>> +    if (!cgroup)
>> +        return 0;
>> +
>> +    if (virCgroupPathOfController(cgroup, VIR_CGROUP_CONTROLLER_CPU,
>> +                                  "cpu.cfs_period_us", &cfs_period_path) < 0) {
>> +        qemuReportError(VIR_ERR_INTERNAL_ERROR,
>> +                        "%s",
>> +                        _("cannot get the path of cgroup CPU controller"));
> 
>   Hum, I'm not sure we should really flag this as an error here
> It should be made an INFO I think.
>   What should get an error is if we try to start using cpu control on a
> guest while the host doesn't support it. In practice we need to check
> proper handling in 3 cases:
>   - at guest startup
>   - on migration when checking the target host
>   - when activated at runtime

Okay, I will change the message level.

> 
>> +        goto cleanup;
>> +    }
>> +
>> +    if (access(cfs_period_path, F_OK) < 0) {
>> +        ret = 0;
>> +    } else {
>> +        ret = 1;
>> +    }
>> +
>> +cleanup:
>> +    VIR_FREE(cfs_period_path);
>> +    return ret;
>> +}
>> +
>> +
>>  static char *qemuGetSchedulerType(virDomainPtr dom,
>>                                    int *nparams)
>>  {
>>      struct qemud_driver *driver = dom->conn->privateData;
>>      char *ret = NULL;
>> +    int rc;
>>  
>>      qemuDriverLock(driver);
>>      if (!qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_CPU)) {
>> @@ -5152,8 +5187,15 @@ static char *qemuGetSchedulerType(virDomainPtr dom,
>>          goto cleanup;
>>      }
>>  
>> -    if (nparams)
>> -        *nparams = 1;
>> +    if (nparams) {
>> +        rc = qemuGetCpuBWStatus(driver->cgroup);
>> +        if (rc < 0)
>> +            goto cleanup;
>> +        else if (rc == 0)
>> +            *nparams = 1;
>> +        else
>> +            *nparams = 3;
>> +    }
>>  
>>      ret = strdup("posix");
>>      if (!ret)
>> @@ -5786,6 +5828,58 @@ cleanup:
>>      return ret;
>>  }
>>  
>> +static int
>> +qemuSetVcpusBWLive(virDomainObjPtr vm, virCgroupPtr cgroup,
>> +                   unsigned long long period, long long quota)
>> +{
>> +    int i;
>> +    qemuDomainObjPrivatePtr priv = vm->privateData;
>> +    virCgroupPtr cgroup_vcpu = NULL;
>> +    int rc;
>> +
>> +    if (period == 0 && quota == 0)
>> +        return 0;
>> +
>> +    if (priv->nvcpupids == 0 || priv->vcpupids[0] == vm->pid) {
>> +        /* If we does not know VCPU<->PID mapping or all vcpu runs in the same
>> +         * thread, we can not control each vcpu.
>> +         */
>> +        /* 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");
> 
>   should probably give an hint of why in the error message

EINVAL means the value is invalid. If you think it is not enough, I will change the
error message.

Thanks
Wen Congyang

> 
>> +            goto cleanup;
>> +        }
>> +

snip

>>      ret = 0;
>>  
>>  cleanup:
> 
>   ACK but please add a commit message
> 
> Daniel
>