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

Re: [libvirt] [PATCH 3/8] Add vcpu functions to libxl driver



Markus Groß wrote:
> ---
>  src/libxl/libxl_driver.c |  295 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 290 insertions(+), 5 deletions(-)
>
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 7ee3930..49fc90f 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -27,6 +27,7 @@
>  #include <config.h>
>  
>  #include <sys/utsname.h>
> +#include <math.h>
>  #include <libxl.h>
>  
>  #include "internal.h"
> @@ -1207,6 +1208,290 @@ libxlDomainGetInfo(virDomainPtr dom, virDomainInfoPtr info)
>      return ret;
>  }
>  
> +static int
> +libxlDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus,
> +                         unsigned int flags)
> +{
> +    libxlDriverPrivatePtr driver = dom->conn->privateData;
> +    libxlDomainObjPrivatePtr priv;
> +    virDomainDefPtr def;
> +    virDomainObjPtr vm;
> +    libxl_cpumap map;
> +    uint8_t *bitmask = NULL;
> +    unsigned int maplen;
> +    unsigned int i, pos;
> +    int max;
> +    int ret = -1;
> +
> +    virCheckFlags(VIR_DOMAIN_VCPU_LIVE |
> +                  VIR_DOMAIN_VCPU_CONFIG |
> +                  VIR_DOMAIN_VCPU_MAXIMUM, -1);
> +
> +    /* At least one of LIVE or CONFIG must be set.  MAXIMUM cannot be
> +     * mixed with LIVE.  */
> +    if ((flags & (VIR_DOMAIN_VCPU_LIVE | VIR_DOMAIN_VCPU_CONFIG)) == 0 ||
> +        (flags & (VIR_DOMAIN_VCPU_MAXIMUM | VIR_DOMAIN_VCPU_LIVE)) ==
> +         (VIR_DOMAIN_VCPU_MAXIMUM | VIR_DOMAIN_VCPU_LIVE)) {
> +        libxlError(VIR_ERR_INVALID_ARG,
> +                   _("invalid flag combination: (0x%x)"), flags);
> +        return -1;
> +    }
>   

I think we should ensure nvcpus != 0

> +
> +    libxlDriverLock(driver);
> +    vm = virDomainFindByUUID(&driver->domains, dom->uuid);
> +    libxlDriverUnlock(driver);
> +
> +    if (!vm) {
> +        libxlError(VIR_ERR_NO_DOMAIN, "%s", _("no domain with matching uuid"));
> +        goto cleanup;
> +    }
> +
> +    if (!virDomainObjIsActive(vm) && (flags & VIR_DOMAIN_VCPU_LIVE)) {
> +        libxlError(VIR_ERR_OPERATION_INVALID, "%s",
> +                   _("cannot set vcpus on an inactive domain"));
> +        goto cleanup;
> +    }
> +
> +    if (!vm->persistent && (flags & VIR_DOMAIN_VCPU_CONFIG)) {
> +        libxlError(VIR_ERR_OPERATION_INVALID, "%s",
> +                   _("cannot change persistent config of a transient domain"));
> +        goto cleanup;
> +    }
> +
> +    if ((max = libxlGetMaxVcpus(dom->conn, NULL)) < 0) {
> +        libxlError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                   _("could not determine max vcpus for the domain"));
> +        goto cleanup;
> +    }
> +
> +    if (!(flags & VIR_DOMAIN_VCPU_MAXIMUM) && vm->def->maxvcpus < max) {
> +        max = vm->def->maxvcpus;
> +    }
> +
> +    if (nvcpus > max) {
> +        libxlError(VIR_ERR_INVALID_ARG,
> +                   _("requested vcpus is greater than max allowable"
> +                     " vcpus for the domain: %d > %d"), nvcpus, max);
> +        goto cleanup;
> +    }
> +
> +    priv = vm->privateData;
> +
> +    if (!(def = virDomainObjGetPersistentDef(driver->caps, vm)))
> +        goto cleanup;
> +
>   
[...]
> +    maplen = (unsigned int) ceil((double) nvcpus / 8);
> +    if (VIR_ALLOC_N(bitmask, maplen) < 0) {
> +        virReportOOMError();
> +        goto cleanup;
> +    }
> +
> +    memset(bitmask, 0, maplen);
> +    for (i = 0; i < nvcpus; ++i) {
> +        pos = (unsigned int) floor((double) i / 8);
> +        bitmask[pos] |= 1 << (i % 8);
> +    }
> +
> +    map.size = maplen;
> +    map.map = bitmask;
>   

Hmm, could this be simplified to

    if (libxl_cpumap_alloc(&priv->ctx, &map)) {
        virReportOOMError()
        goto cleanup;
    }
    for (i = 0; i < nvcpus; i++)
        libxl_cpumap_set(&map, i);

> +
> +    switch (flags) {
> +    case VIR_DOMAIN_VCPU_MAXIMUM | VIR_DOMAIN_VCPU_CONFIG:
> +        def->maxvcpus = nvcpus;
> +        if (nvcpus < def->vcpus)
> +            def->vcpus = nvcpus;
> +        ret = 0;
> +        break;
> +
> +    case VIR_DOMAIN_VCPU_CONFIG:
> +        def->vcpus = nvcpus;
> +        break;
> +
> +    case VIR_DOMAIN_VCPU_LIVE:
> +        if (libxl_set_vcpuonline(&priv->ctx, dom->id, &map) != 0) {
> +            libxlError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Failed to set vcpus for domain '%d'"
> +                         " with libxenlight"), dom->id);
> +            goto cleanup;
> +        }
> +        break;
> +
> +    case VIR_DOMAIN_VCPU_LIVE | VIR_DOMAIN_VCPU_CONFIG:
> +        if (libxl_set_vcpuonline(&priv->ctx, dom->id, &map) != 0) {
> +            libxlError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Failed to set vcpus for domain '%d'"
> +                         " with libxenlight"), dom->id);
> +            goto cleanup;
> +        }
> +        def->vcpus = nvcpus;
> +        break;
> +    }
> +
> +    ret = 0;
> +
> +    if (flags & VIR_DOMAIN_VCPU_CONFIG)
> +        ret = virDomainSaveConfig(driver->configDir, def);
> +
> +cleanup:
> +    VIR_FREE(bitmask);
> +     if (vm)
> +        virDomainObjUnlock(vm);
> +    return ret;
> +}
> +
> +static int
> +libxlDomainSetVcpus(virDomainPtr dom, unsigned int nvcpus)
> +{
> +    return libxlDomainSetVcpusFlags(dom, nvcpus, VIR_DOMAIN_VCPU_LIVE);
> +}
> +
> +static int
> +libxlDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags)
> +{
> +    libxlDriverPrivatePtr driver = dom->conn->privateData;
> +    virDomainObjPtr vm;
> +    virDomainDefPtr def;
> +    int ret = -1;
> +
> +    virCheckFlags(VIR_DOMAIN_VCPU_LIVE |
> +                  VIR_DOMAIN_VCPU_CONFIG |
> +                  VIR_DOMAIN_VCPU_MAXIMUM, -1);
>   

Documentation for this function says that it is an error to set both
VIR_DOMAIN_VCPU_LIVE and VIR_DOMAIN_VCPU_CONFIG, so we should check for it.

> +
> +    libxlDriverLock(driver);
> +    vm = virDomainFindByUUID(&driver->domains, dom->uuid);
> +    libxlDriverUnlock(driver);
> +
> +    if (!vm) {
> +        libxlError(VIR_ERR_NO_DOMAIN, "%s", _("no domain with matching uuid"));
> +        goto cleanup;
> +    }
> +
> +    if (flags & VIR_DOMAIN_VCPU_LIVE) {
> +        if (!virDomainObjIsActive(vm)) {
> +            libxlError(VIR_ERR_OPERATION_INVALID,
> +                       "%s", _("Domain is not running"));
> +            goto cleanup;
> +        }
> +        def = vm->def;
> +    } else {
> +        def = vm->newDef ? vm->newDef : vm->def;
> +    }
> +
> +    ret = (flags & VIR_DOMAIN_VCPU_MAXIMUM) ? def->maxvcpus : def->vcpus;
> +
> +cleanup:
> +    if (vm)
> +        virDomainObjUnlock(vm);
> +    return ret;
> +}
> +
> +static int
> +libxlDomainPinVcpu(virDomainPtr dom, unsigned int vcpu, unsigned char *cpumap,
> +                   int maplen)
> +{
> +    libxlDriverPrivatePtr driver = dom->conn->privateData;
> +    libxlDomainObjPrivatePtr priv;
> +    virDomainObjPtr vm;
> +    int ret = -1;
> +    libxl_cpumap map;
> +
> +    libxlDriverLock(driver);
> +    vm = virDomainFindByUUID(&driver->domains, dom->uuid);
> +    libxlDriverUnlock(driver);
> +
> +    if (!vm) {
> +        libxlError(VIR_ERR_NO_DOMAIN, "%s", _("no domain with matching uuid"));
> +        goto cleanup;
> +    }
> +
> +    if (!virDomainObjIsActive(vm)) {
> +        libxlError(VIR_ERR_OPERATION_INVALID, "%s",
> +                   _("cannot pin vcpus on an inactive domain"));
> +        goto cleanup;
> +    }
> +
> +    priv = vm->privateData;
> +
> +    map.size = maplen;
> +    map.map = cpumap;
> +    if (libxl_set_vcpuaffinity(&priv->ctx, dom->id, vcpu, &map) != 0) {
> +        libxlError(VIR_ERR_INTERNAL_ERROR,
> +                   _("Failed to pin vcpu '%d' with libxenlight"), vcpu);
> +        goto cleanup;
> +    }
> +    ret = 0;
> +
> +cleanup:
> +    if (vm)
> +        virDomainObjUnlock(vm);
> +    return ret;
> +}
> +
> +
> +static int
> +libxlDomainGetVcpus(virDomainPtr dom, virVcpuInfoPtr info, int maxinfo,
> +                    unsigned char *cpumaps, int maplen)
> +{
> +    libxlDriverPrivatePtr driver = dom->conn->privateData;
> +    libxlDomainObjPrivatePtr priv;
> +    virDomainObjPtr vm;
> +    int ret = -1;
> +
>   

Extra newline.

> +    libxl_vcpuinfo *vcpuinfo;
> +    int maxcpu, hostcpus;
> +
> +    libxlDriverLock(driver);
> +    vm = virDomainFindByUUID(&driver->domains, dom->uuid);
> +    libxlDriverUnlock(driver);
> +
> +    if (!vm) {
> +        libxlError(VIR_ERR_NO_DOMAIN, "%s", _("no domain with matching uuid"));
> +        goto cleanup;
> +    }
> +
> +    if (!virDomainObjIsActive(vm)) {
> +        libxlError(VIR_ERR_OPERATION_INVALID, "%s", _("Domain is not running"));
> +        goto cleanup;
> +    }
> +
> +    priv = vm->privateData;
> +    if ((vcpuinfo = libxl_list_vcpu(&priv->ctx, dom->id, &maxcpu,
> +                                    &hostcpus)) == NULL) {
> +        libxlError(VIR_ERR_INTERNAL_ERROR,
> +                   _("Failed to list vcpus for domain '%d' with libxenlight"),
> +                   dom->id);
> +        goto cleanup;
> +    }
> +
> +    memset(cpumaps, 0, maplen * maxinfo);
>   

Documentation for this function says cpumaps can be NULL.

> +    unsigned int i = 0;
>   

I thought I recalled variable declaration only at start of a block but
don't see anything about it in HACKING.  Perhaps someone else can clarify.

Regards,
Jim

> +    for (i = 0; i < maxcpu && i < maxinfo; ++i) {
> +        info[i].number = vcpuinfo[i].vcpuid;
> +        info[i].cpu = vcpuinfo[i].cpu;
> +        info[i].cpuTime = vcpuinfo[i].vcpu_time;
> +        if (vcpuinfo[i].running)
> +            info[i].state = VIR_VCPU_RUNNING;
> +        else if (vcpuinfo[i].blocked)
> +            info[i].state = VIR_VCPU_BLOCKED;
> +        else
> +            info[i].state = VIR_VCPU_OFFLINE;
> +
> +        unsigned char *cpumap = VIR_GET_CPUMAP(cpumaps, maplen, i);
> +        memcpy(cpumap, vcpuinfo[i].cpumap.map,
> +               MIN(maplen, vcpuinfo[i].cpumap.size));
> +
> +        libxl_vcpuinfo_destroy(&vcpuinfo[i]);
> +    }
> +    VIR_FREE(vcpuinfo);
> +
> +    ret = maxinfo;
> +
> +cleanup:
> +    if (vm)
> +        virDomainObjUnlock(vm);
> +    return ret;
> +}
> +
>  static char *
>  libxlDomainDumpXML(virDomainPtr dom, int flags)
>  {
> @@ -1561,11 +1846,11 @@ static virDriver libxlDriver = {
>      NULL,                       /* domainSave */
>      NULL,                       /* domainRestore */
>      NULL,                       /* domainCoreDump */
> -    NULL,                       /* domainSetVcpus */
> -    NULL,                       /* domainSetVcpusFlags */
> -    NULL,                       /* domainGetVcpusFlags */
> -    NULL,                       /* domainPinVcpu */
> -    NULL,                       /* domainGetVcpus */
> +    libxlDomainSetVcpus,        /* domainSetVcpus */
> +    libxlDomainSetVcpusFlags,   /* domainSetVcpusFlags */
> +    libxlDomainGetVcpusFlags,   /* domainGetVcpusFlags */
> +    libxlDomainPinVcpu,         /* domainPinVcpu */
> +    libxlDomainGetVcpus,        /* domainGetVcpus */
>      NULL,                       /* domainGetMaxVcpus */
>      NULL,                       /* domainGetSecurityLabel */
>      NULL,                       /* nodeGetSecurityModel */
>   


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