On 01/14/2016 11:27 AM, Peter Krempa wrote:
> Now with the new struct the data can be stored in a much saner place.
> ---
> src/conf/domain_conf.c | 131 ++++++++++++++++++--------------------------
> src/conf/domain_conf.h | 3 +-
> src/libxl/libxl_domain.c | 17 +++---
> src/libxl/libxl_driver.c | 39 ++++++--------
> src/qemu/qemu_cgroup.c | 15 ++----
> src/qemu/qemu_driver.c | 138 ++++++++++++++++++++++-------------------------
> src/qemu/qemu_process.c | 38 +++++++------
> src/test/test_driver.c | 43 ++++++---------
> 8 files changed, 179 insertions(+), 245 deletions(-)
>
Not sure why patch 19 is interspersed since 18 and 20 are "related"
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 29ef357..ca32466 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1289,6 +1289,8 @@ virDomainVcpuInfoClear(virDomainVcpuInfoPtr info)
> {
> if (!info)
> return;
> +
> + virBitmapFree(info->cpumask);
Will there need to be a vcpuinfo->cpumask = NULL afterwards? Since this
is pass by value and not reference.
This is called by virDomainDefSetVcpusMax so it's not just in a pur
*Free path
> }
>
>
> @@ -1422,7 +1424,14 @@ virDomainDefGetVcpu(virDomainDefPtr def,
> static bool
> virDomainDefHasVcpusPin(const virDomainDef *def)
> {
> - return !!def->cputune.nvcpupin;
> + size_t i;
> +
> + for (i = 0; i < def->maxvcpus; i++) {
> + if (def->vcpus[i].cpumask)
> + return true;
> + }
> +
> + return false;
> }
>
>
> @@ -2593,8 +2602,6 @@ void virDomainDefFree(virDomainDefPtr def)
>
> virDomainIOThreadIDDefArrayFree(def->iothreadids, def->niothreadids);
>
> - virDomainPinDefArrayFree(def->cputune.vcpupin, def->cputune.nvcpupin);
> -
> virBitmapFree(def->cputune.emulatorpin);
>
> for (i = 0; i < def->cputune.nvcpusched; i++)
> @@ -14137,77 +14144,62 @@ virDomainIOThreadIDDefParseXML(xmlNodePtr node,
> }
>
>
> -/* Check if pin with same id already exists. */
> -static bool
> -virDomainPinIsDuplicate(virDomainPinDefPtr *def,
> - int npin,
> - int id)
> -{
> - size_t i;
> -
> - if (!def || !npin)
> - return false;
> -
> - for (i = 0; i < npin; i++) {
> - if (def[i]->id == id)
> - return true;
> - }
> -
> - return false;
> -}
> -
> /* Parse the XML definition for a vcpupin
> *
> * vcpupin has the form of
> * <vcpupin vcpu='0' cpuset='0'/>
> */
> -static virDomainPinDefPtr
> -virDomainVcpuPinDefParseXML(xmlNodePtr node,
> - xmlXPathContextPtr ctxt)
> +static int
> +virDomainVcpuPinDefParseXML(virDomainDefPtr def,
> + xmlNodePtr node)
> {
> - virDomainPinDefPtr def;
> - xmlNodePtr oldnode = ctxt->node;
> + virDomainVcpuInfoPtr vcpu;
> unsigned int vcpuid;
> char *tmp = NULL;
> + int ret = -1;
>
> - if (VIR_ALLOC(def) < 0)
> - return NULL;
> -
> - ctxt->node = node;
> -
> - if (!(tmp = virXPathString("string(./@vcpu)", ctxt))) {
> - virReportError(VIR_ERR_XML_ERROR, "%s",
> - _("missing vcpu id in vcpupin"));
> - goto error;
> + if (!(tmp = virXMLPropString(node, "vcpu"))) {
> + virReportError(VIR_ERR_XML_ERROR, "%s", _("missing vcpu id in vcpupin"));
> + goto cleanup;
> }
>
> if (virStrToLong_uip(tmp, NULL, 10, &vcpuid) < 0) {
> virReportError(VIR_ERR_XML_ERROR,
> _("invalid setting for vcpu '%s'"), tmp);
> - goto error;
> + goto cleanup;
> }
> VIR_FREE(tmp);
>
> - def->id = vcpuid;
> + if (!(vcpu = virDomainDefGetVcpu(def, vcpuid)) ||
> + !vcpu->online) {
> + /* To avoid the regression when daemon loading domain confs, we can't
> + * simply error out if <vcpupin> nodes greater than current vcpus.
> + * Ignore them instead. */
> + VIR_WARN("Ignoring vcpupin for missing vcpus");
Is this message true for !online as well? or just there because of the
maxvcpus check?
BTW: Another place where checking an OnlineVcpuMap could be beneficial.
> + ret = 0;
> + goto cleanup;
> + }
>
> if (!(tmp = virXMLPropString(node, "cpuset"))) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("missing cpuset for vcpupin"));
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("missing cpuset for vcpupin"));
> + goto cleanup;
> + }
>
> - goto error;
> + if (vcpu->cpumask) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("duplicate vcpupin for vcpu '%d'"), vcpuid);
> + goto cleanup;
> }
I would think this check could go prior to parsing "cpuset"
>
> - if (virBitmapParse(tmp, 0, &def->cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0)
> - goto error;
> + if (virBitmapParse(tmp, 0, &vcpu->cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0)
> + goto cleanup;
> +
> + ret = 0;
>
> cleanup:
> VIR_FREE(tmp);
> - ctxt->node = oldnode;
> - return def;
> -
> - error:
> - VIR_FREE(def);
> - goto cleanup;
> + return ret;
> }
>
>
> @@ -15131,34 +15123,9 @@ virDomainDefParseXML(xmlDocPtr xml,
> if ((n = virXPathNodeSet("./cputune/vcpupin", ctxt, &nodes)) < 0)
> goto error;
>
> - if (n && VIR_ALLOC_N(def->cputune.vcpupin, n) < 0)
> - goto error;
> -
> for (i = 0; i < n; i++) {
> - virDomainPinDefPtr vcpupin;
> - if (!(vcpupin = virDomainVcpuPinDefParseXML(nodes[i], ctxt)))
> + if (virDomainVcpuPinDefParseXML(def, nodes[i]))
> goto error;
> -
> - if (virDomainPinIsDuplicate(def->cputune.vcpupin,
> - def->cputune.nvcpupin,
> - vcpupin->id)) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - "%s", _("duplicate vcpupin for same vcpu"));
> - virDomainPinDefFree(vcpupin);
> - goto error;
> - }
> -
> - if (vcpupin->id >= virDomainDefGetVcpus(def)) {
> - /* To avoid the regression when daemon loading
> - * domain confs, we can't simply error out if
> - * <vcpupin> nodes greater than current vcpus,
> - * ignoring them instead.
> - */
> - VIR_WARN("Ignore vcpupin for missing vcpus");
> - virDomainPinDefFree(vcpupin);
> - } else {
> - def->cputune.vcpupin[def->cputune.nvcpupin++] = vcpupin;
> - }
> }
> VIR_FREE(nodes);
>
> @@ -21838,15 +21805,19 @@ virDomainDefFormatInternal(virDomainDefPtr def,
> "</emulator_quota>\n",
> def->cputune.emulator_quota);
>
> - for (i = 0; i < def->cputune.nvcpupin; i++) {
> + for (i = 0; i < def->maxvcpus; i++) {
> char *cpumask;
> - virBufferAsprintf(&childrenBuf, "<vcpupin vcpu='%u' ",
> - def->cputune.vcpupin[i]->id);
> + virDomainVcpuInfoPtr vcpu = def->vcpus + i;
>
> - if (!(cpumask = virBitmapFormat(def->cputune.vcpupin[i]->cpumask)))
> + if (!vcpu->cpumask)
> + continue;
> +
> + if (!(cpumask = virBitmapFormat(vcpu->cpumask)))
> goto error;
>
> - virBufferAsprintf(&childrenBuf, "cpuset='%s'/>\n", cpumask);
> + virBufferAsprintf(&childrenBuf,
> + "<vcpupin vcpu='%zu' cpuset='%s'/>\n", i, cpumask);
> +
> VIR_FREE(cpumask);
> }
>
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index f15b558..4a8c199 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2137,8 +2137,6 @@ struct _virDomainCputune {
> long long quota;
> unsigned long long emulator_period;
> long long emulator_quota;
> - size_t nvcpupin;
> - virDomainPinDefPtr *vcpupin;
> virBitmapPtr emulatorpin;
>
> size_t nvcpusched;
> @@ -2153,6 +2151,7 @@ typedef virDomainVcpuInfo *virDomainVcpuInfoPtr;
>
> struct _virDomainVcpuInfo {
> bool online;
> + virBitmapPtr cpumask;
bikeshed...
why not 'cpupinmask' (or something to make it easier to find)
I understand the desire to keep things named similarly (cpumask), but it
just makes it more difficult to "find" specific instances...
> };
>
> typedef struct _virDomainBlkiotune virDomainBlkiotune;
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index 37c92c6..99ce44a 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -816,7 +816,7 @@ int
> libxlDomainSetVcpuAffinities(libxlDriverPrivatePtr driver, virDomainObjPtr vm)
> {
> libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
> - virDomainPinDefPtr pin;
> + virDomainVcpuInfoPtr vcpu;
> libxl_bitmap map;
> virBitmapPtr cpumask = NULL;
> size_t i;
> @@ -825,13 +825,12 @@ libxlDomainSetVcpuAffinities(libxlDriverPrivatePtr driver, virDomainObjPtr vm)
> libxl_bitmap_init(&map);
>
> for (i = 0; i < virDomainDefGetVcpus(vm->def); ++i) {
Here's another OnlineVcpuMap perusal opportunity...
> - pin = virDomainPinFind(vm->def->cputune.vcpupin,
> - vm->def->cputune.nvcpupin,
> - i);
> + vcpu = virDomainDefGetVcpu(vm->def, i);
>
> - if (pin && pin->cpumask)
> - cpumask = pin->cpumask;
> - else
> + if (!vcpu->online)
> + continue;
> +
> + if (!(cpumask = vcpu->cpumask))
> cpumask = vm->def->cpumask;
>
> if (!cpumask)
> @@ -840,9 +839,9 @@ libxlDomainSetVcpuAffinities(libxlDriverPrivatePtr driver, virDomainObjPtr vm)
> if (virBitmapToData(cpumask, &map.map, (int *)&map.size) < 0)
> goto cleanup;
>
> - if (libxl_set_vcpuaffinity(cfg->ctx, vm->def->id, pin->id, &map) != 0) {
> + if (libxl_set_vcpuaffinity(cfg->ctx, vm->def->id, i, &map) != 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Failed to pin vcpu '%d' with libxenlight"), pin->id);
> + _("Failed to pin vcpu '%zu' with libxenlight"), i);
> goto cleanup;
> }
>
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 26c1a43..b9da190 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -2321,6 +2321,7 @@ libxlDomainPinVcpuFlags(virDomainPtr dom, unsigned int vcpu,
> libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
> virDomainDefPtr targetDef = NULL;
> virBitmapPtr pcpumap = NULL;
> + virDomainVcpuInfoPtr vcpuinfo;
> virDomainObjPtr vm;
> int ret = -1;
>
> @@ -2352,10 +2353,16 @@ libxlDomainPinVcpuFlags(virDomainPtr dom, unsigned int vcpu,
> /* Make sure coverity knows targetDef is valid at this point. */
> sa_assert(targetDef);
>
> - pcpumap = virBitmapNewData(cpumap, maplen);
> - if (!pcpumap)
> + if (!(pcpumap = virBitmapNewData(cpumap, maplen)))
> goto endjob;
>
> + if (!(vcpuinfo = virDomainDefGetVcpu(targetDef, vcpu)) ||
> + !vcpuinfo->online) {
Ditto
> + virReportError(VIR_ERR_INVALID_ARG,
> + _("vcpu '%u' is not active"), vcpu);
> + goto endjob;
> + }
> +
> if (flags & VIR_DOMAIN_AFFECT_LIVE) {
> libxl_bitmap map = { .size = maplen, .map = cpumap };
> if (libxl_set_vcpuaffinity(cfg->ctx, vm->def->id, vcpu, &map) != 0) {
> @@ -2366,20 +2373,9 @@ libxlDomainPinVcpuFlags(virDomainPtr dom, unsigned int vcpu,
> }
> }
>
> - if (!targetDef->cputune.vcpupin) {
> - if (VIR_ALLOC(targetDef->cputune.vcpupin) < 0)
> - goto endjob;
> - targetDef->cputune.nvcpupin = 0;
> - }
> - if (virDomainPinAdd(&targetDef->cputune.vcpupin,
> - &targetDef->cputune.nvcpupin,
> - cpumap,
> - maplen,
> - vcpu) < 0) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - "%s", _("failed to update or add vcpupin xml"));
> - goto endjob;
> - }
> + virBitmapFree(vcpuinfo->cpumask);
> + vcpuinfo->cpumask = pcpumap;
> + pcpumap = NULL;
>
> ret = 0;
>
> @@ -2455,15 +2451,14 @@ libxlDomainGetVcpuPinInfo(virDomainPtr dom, int ncpumaps,
> memset(cpumaps, 0x00, maplen * ncpumaps);
>
> for (vcpu = 0; vcpu < ncpumaps; vcpu++) {
> - virDomainPinDefPtr pininfo;
> + virDomainVcpuInfoPtr vcpuinfo = virDomainDefGetVcpu(targetDef, vcpu);
> virBitmapPtr bitmap = NULL;
>
> - pininfo = virDomainPinFind(targetDef->cputune.vcpupin,
> - targetDef->cputune.nvcpupin,
> - vcpu);
> + if (!vcpuinfo->online)
> + continue;
here too in some way (OnlineVcpuMap)
>
> - if (pininfo && pininfo->cpumask)
> - bitmap = pininfo->cpumask;
> + if (vcpuinfo->cpumask)
> + bitmap = vcpuinfo->cpumask;
> else if (targetDef->cpumask)
> bitmap = targetDef->cpumask;
> else
> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> index 1c406ce..3744b52 100644
> --- a/src/qemu/qemu_cgroup.c
> +++ b/src/qemu/qemu_cgroup.c
> @@ -1007,7 +1007,7 @@ qemuSetupCgroupForVcpu(virDomainObjPtr vm)
> virCgroupPtr cgroup_vcpu = NULL;
> qemuDomainObjPrivatePtr priv = vm->privateData;
> virDomainDefPtr def = vm->def;
> - size_t i, j;
> + size_t i;
> unsigned long long period = vm->def->cputune.period;
> long long quota = vm->def->cputune.quota;
> char *mem_mask = NULL;
> @@ -1065,20 +1065,13 @@ qemuSetupCgroupForVcpu(virDomainObjPtr vm)
> virCgroupSetCpusetMems(cgroup_vcpu, mem_mask) < 0)
> goto cleanup;
>
> - /* try to use the default cpu maps */
> - if (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO)
> + if (vcpu->cpumask)
> + cpumap = vcpu->cpumask;
> + else if (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO)
> cpumap = priv->autoCpuset;
> else
> cpumap = vm->def->cpumask;
>
> - /* lookup a more specific pinning info */
> - for (j = 0; j < def->cputune.nvcpupin; j++) {
> - if (def->cputune.vcpupin[j]->id == i) {
> - cpumap = def->cputune.vcpupin[j]->cpumask;
> - break;
> - }
> - }
> -
> if (!cpumap)
> continue;
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 0a4de1b..f253248 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4769,10 +4769,7 @@ qemuDomainHotplugDelVcpu(virQEMUDriverPtr driver,
> VIR_CGROUP_THREAD_VCPU, vcpu) < 0)
> goto cleanup;
>
> - /* Free vcpupin setting */
> - virDomainPinDel(&vm->def->cputune.vcpupin,
> - &vm->def->cputune.nvcpupin,
> - vcpu);
> + virBitmapFree(vcpuinfo->cpumask);
Will there need to be a vcpuinfo->cpumask = NULL afterwards? Since this
is pass by value and not reference.
>
> ret = 0;
>
> @@ -4937,10 +4934,13 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus,
> if (persistentDef) {
> /* remove vcpupin entries for vcpus that were unplugged */
> if (nvcpus < virDomainDefGetVcpus(persistentDef)) {
> - for (i = virDomainDefGetVcpus(persistentDef) - 1; i >= nvcpus; i--)
> - virDomainPinDel(&persistentDef->cputune.vcpupin,
> - &persistentDef->cputune.nvcpupin,
> - i);
> + for (i = virDomainDefGetVcpus(persistentDef) - 1; i >= nvcpus; i--) {
> + virDomainVcpuInfoPtr vcpu = virDomainDefGetVcpu(persistentDef,
> + i);
> +
> + if (vcpu)
> + virBitmapFree(vcpu->cpumask);
Will there need to be a vcpuinfo->cpumask = NULL afterwards? Since this
is pass by value and not reference.
> + }
> }
>
> if (flags & VIR_DOMAIN_VCPU_MAXIMUM) {
> @@ -4999,9 +4999,11 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
> virCgroupPtr cgroup_vcpu = NULL;
> int ret = -1;
> qemuDomainObjPrivatePtr priv;
> - size_t newVcpuPinNum = 0;
> - virDomainPinDefPtr *newVcpuPin = NULL;
> virBitmapPtr pcpumap = NULL;
> + virBitmapPtr pcpumaplive = NULL;
> + virBitmapPtr pcpumappersist = NULL;
> + virDomainVcpuInfoPtr vcpuinfolive = NULL;
> + virDomainVcpuInfoPtr vcpuinfopersist = NULL;
> virQEMUDriverConfigPtr cfg = NULL;
> virObjectEventPtr event = NULL;
> char paramField[VIR_TYPED_PARAM_FIELD_LENGTH] = "";
> @@ -5029,18 +5031,36 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
>
> priv = vm->privateData;
>
> - if (def && vcpu >= virDomainDefGetVcpus(def)) {
> - virReportError(VIR_ERR_INVALID_ARG,
> - _("vcpu %d is out of range of live cpu count %d"),
> - vcpu, virDomainDefGetVcpus(def));
> - goto endjob;
> + if (def) {
> + if (!(vcpuinfolive = virDomainDefGetVcpu(def, vcpu))) {
> + virReportError(VIR_ERR_INVALID_ARG,
> + _("vcpu %d is out of range of live cpu count %d"),
> + vcpu, virDomainDefGetVcpus(def));
> + goto endjob;
> + }
> +
> + if (!vcpuinfolive->online) {
> + virReportError(VIR_ERR_OPERATION_INVALID,
> + _("setting cpu pinning for inactive vcpu '%d' is not "
> + "supported"), vcpu);
Another place for OnlineVcpuMap (persist path too)
> + goto endjob;
> + }
> }
>
> - if (persistentDef && vcpu >= virDomainDefGetVcpus(persistentDef)) {
> - virReportError(VIR_ERR_INVALID_ARG,
> - _("vcpu %d is out of range of persistent cpu count %d"),
> - vcpu, virDomainDefGetVcpus(persistentDef));
> - goto endjob;
> + if (persistentDef) {
> + if (!(vcpuinfopersist = virDomainDefGetVcpu(persistentDef, vcpu))) {
> + virReportError(VIR_ERR_INVALID_ARG,
> + _("vcpu %d is out of range of persistent cpu count %d"),
> + vcpu, virDomainDefGetVcpus(persistentDef));
> + goto endjob;
> + }
> +
> + if (!vcpuinfopersist->online) {
> + virReportError(VIR_ERR_OPERATION_INVALID,
> + _("setting cpu pinning for inactive vcpu '%d' is not "
> + "supported"), vcpu);
> + goto endjob;
> + }
> }
>
> if (!(pcpumap = virBitmapNewData(cpumap, maplen)))
> @@ -5052,6 +5072,10 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
> goto endjob;
> }
>
> + if ((def && !(pcpumaplive = virBitmapNewCopy(pcpumap))) ||
> + (persistentDef && !(pcpumappersist = virBitmapNewCopy(pcpumap))))
> + goto endjob;
> +
> if (def) {
> if (!qemuDomainHasVcpuPids(vm)) {
> virReportError(VIR_ERR_OPERATION_INVALID,
> @@ -5059,26 +5083,6 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
> goto endjob;
> }
>
> - if (def->cputune.vcpupin) {
> - newVcpuPin = virDomainPinDefCopy(def->cputune.vcpupin,
> - def->cputune.nvcpupin);
> - if (!newVcpuPin)
> - goto endjob;
> -
> - newVcpuPinNum = def->cputune.nvcpupin;
> - } else {
> - if (VIR_ALLOC(newVcpuPin) < 0)
> - goto endjob;
> - newVcpuPinNum = 0;
> - }
> -
> - if (virDomainPinAdd(&newVcpuPin, &newVcpuPinNum,
> - cpumap, maplen, vcpu) < 0) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("failed to update vcpupin"));
> - goto endjob;
> - }
> -
> /* Configure the corresponding cpuset cgroup before set affinity. */
> if (virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) {
> if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_VCPU, vcpu,
> @@ -5100,13 +5104,9 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
> }
> }
>
> - if (def->cputune.vcpupin)
> - virDomainPinDefArrayFree(def->cputune.vcpupin,
> - def->cputune.nvcpupin);
> -
> - def->cputune.vcpupin = newVcpuPin;
> - def->cputune.nvcpupin = newVcpuPinNum;
> - newVcpuPin = NULL;
> + virBitmapFree(vcpuinfolive->cpumask);
> + vcpuinfolive->cpumask = pcpumaplive;
> + pcpumaplive = NULL;
>
> if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0)
> goto endjob;
> @@ -5125,24 +5125,12 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
> }
>
> if (persistentDef) {
> - if (!persistentDef->cputune.vcpupin) {
> - if (VIR_ALLOC(persistentDef->cputune.vcpupin) < 0)
> - goto endjob;
> - persistentDef->cputune.nvcpupin = 0;
> - }
> - if (virDomainPinAdd(&persistentDef->cputune.vcpupin,
> - &persistentDef->cputune.nvcpupin,
> - cpumap,
> - maplen,
> - vcpu) < 0) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("failed to update or add vcpupin xml of "
> - "a persistent domain"));
> - goto endjob;
> - }
> + virBitmapFree(vcpuinfopersist->cpumask);
> + vcpuinfopersist->cpumask = pcpumappersist;
> + pcpumappersist = NULL;
>
> - ret = virDomainSaveConfig(cfg->configDir, persistentDef);
> - goto endjob;
> + if (virDomainSaveConfig(cfg->configDir, persistentDef) < 0)
> + goto endjob;
> }
This looked OK, but the names of the new variables caused a few brain
cramps... Like a long runon sentence.
>
> ret = 0;
> @@ -5151,14 +5139,14 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
> qemuDomainObjEndJob(driver, vm);
>
> cleanup:
> - if (newVcpuPin)
> - virDomainPinDefArrayFree(newVcpuPin, newVcpuPinNum);
> if (cgroup_vcpu)
> virCgroupFree(&cgroup_vcpu);
> virDomainObjEndAPI(&vm);
> qemuDomainEventQueue(driver, event);
> VIR_FREE(str);
> virBitmapFree(pcpumap);
> + virBitmapFree(pcpumaplive);
> + virBitmapFree(pcpumappersist);
> virObjectUnref(cfg);
> return ret;
> }
> @@ -5183,7 +5171,8 @@ qemuDomainGetVcpuPinInfo(virDomainPtr dom,
> virDomainObjPtr vm = NULL;
> virDomainDefPtr def;
> int ret = -1;
> - int hostcpus, vcpu;
> + int hostcpus;
> + size_t i;
> virBitmapPtr allcpumap = NULL;
> qemuDomainObjPrivatePtr priv = NULL;
>
> @@ -5215,16 +5204,15 @@ qemuDomainGetVcpuPinInfo(virDomainPtr dom,
> if (ncpumaps < 1)
> goto cleanup;
>
> - for (vcpu = 0; vcpu < ncpumaps; vcpu++) {
> - virDomainPinDefPtr pininfo;
> + for (i = 0; i < ncpumaps; i++) {
> + virDomainVcpuInfoPtr vcpu = virDomainDefGetVcpu(def, i);
> virBitmapPtr bitmap = NULL;
>
> - pininfo = virDomainPinFind(def->cputune.vcpupin,
> - def->cputune.nvcpupin,
> - vcpu);
> + if (!vcpu->online)
> + continue;
>
> - if (pininfo && pininfo->cpumask)
> - bitmap = pininfo->cpumask;
> + if (vcpu->cpumask)
> + bitmap = vcpu->cpumask;
> else if (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO &&
> priv->autoCpuset)
> bitmap = priv->autoCpuset;
> @@ -5233,7 +5221,7 @@ qemuDomainGetVcpuPinInfo(virDomainPtr dom,
> else
> bitmap = allcpumap;
>
> - virBitmapToDataBuf(bitmap, VIR_GET_CPUMAP(cpumaps, maplen, vcpu), maplen);
> + virBitmapToDataBuf(bitmap, VIR_GET_CPUMAP(cpumaps, maplen, i), maplen);
> }
>
> ret = ncpumaps;
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 64b58be..c0043c9 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -2163,23 +2163,23 @@ static int
> qemuProcessSetVcpuAffinities(virDomainObjPtr vm)
> {
> virDomainDefPtr def = vm->def;
> - virDomainPinDefPtr pininfo;
> - int n;
> + virDomainVcpuInfoPtr vcpu;
> + size_t i;
> int ret = -1;
> - VIR_DEBUG("Setting affinity on CPUs nvcpupin=%zu nvcpus=%d hasVcpupids=%d",
> - def->cputune.nvcpupin, virDomainDefGetVcpus(def),
> - qemuDomainHasVcpuPids(vm));
> - if (!def->cputune.nvcpupin)
> - return 0;
> + VIR_DEBUG("Setting affinity on CPUs");
>
> if (!qemuDomainHasVcpuPids(vm)) {
> /* If any CPU has custom affinity that differs from the
> * VM default affinity, we must reject it
> */
> - for (n = 0; n < def->cputune.nvcpupin; n++) {
> - if (def->cputune.vcpupin[n]->cpumask &&
> - !virBitmapEqual(def->cpumask,
> - def->cputune.vcpupin[n]->cpumask)) {
> + for (i = 0; i < virDomainDefGetVcpusMax(def); i++) {
> + vcpu = virDomainDefGetVcpu(def, i);
> +
> + if (!vcpu->online)
> + continue;
Another OnlineVcpuMap opportunity.
> +
> + if (vcpu->cpumask &&
> + !virBitmapEqual(def->cpumask, vcpu->cpumask)) {
> virReportError(VIR_ERR_OPERATION_INVALID,
> "%s", _("cpu affinity is not supported"));
> return -1;
> @@ -2188,22 +2188,20 @@ qemuProcessSetVcpuAffinities(virDomainObjPtr vm)
> return 0;
> }
>
> - for (n = 0; n < virDomainDefGetVcpus(def); n++) {
> + for (i = 0; i < virDomainDefGetVcpusMax(def); i++) {
> virBitmapPtr bitmap;
> - /* set affinity only for existing vcpus */
> - if (!(pininfo = virDomainPinFind(def->cputune.vcpupin,
> - def->cputune.nvcpupin,
> - n)))
> +
> + vcpu = virDomainDefGetVcpu(def, i);
> +
> + if (!vcpu->online)
> continue;
>
> - if (!(bitmap = pininfo->cpumask) &&
> + if (!(bitmap = vcpu->cpumask) &&
> !(bitmap = def->cpumask))
> continue;
>
> - if (virProcessSetAffinity(qemuDomainGetVcpuPid(vm, n),
> - pininfo->cpumask) < 0) {
> + if (virProcessSetAffinity(qemuDomainGetVcpuPid(vm, i), bitmap) < 0)
> goto cleanup;
> - }
> }
>
> ret = 0;
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index 5986749..ed4de12 100644
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -2455,15 +2455,14 @@ static int testDomainGetVcpus(virDomainPtr domain,
> memset(cpumaps, 0, maxinfo * maplen);
>
> for (i = 0; i < maxinfo; i++) {
> - virDomainPinDefPtr pininfo;
> + virDomainVcpuInfoPtr vcpu = virDomainDefGetVcpu(def, i);
> virBitmapPtr bitmap = NULL;
>
> - pininfo = virDomainPinFind(def->cputune.vcpupin,
> - def->cputune.nvcpupin,
> - i);
> + if (vcpu && !vcpu->online)
> + continue;
Another OnlineVcpuMap opportunity (and the following change too)
>
> - if (pininfo && pininfo->cpumask)
> - bitmap = pininfo->cpumask;
> + if (vcpu->cpumask)
> + bitmap = vcpu->cpumask;
> else if (def->cpumask)
> bitmap = def->cpumask;
> else
> @@ -2492,6 +2491,7 @@ static int testDomainPinVcpu(virDomainPtr domain,
> unsigned char *cpumap,
> int maplen)
> {
> + virDomainVcpuInfoPtr vcpuinfo;
> virDomainObjPtr privdom;
> virDomainDefPtr def;
> int ret = -1;
> @@ -2507,29 +2507,21 @@ static int testDomainPinVcpu(virDomainPtr domain,
> goto cleanup;
> }
>
> - if (vcpu > virDomainDefGetVcpus(privdom->def)) {
> + if (!(vcpuinfo = virDomainDefGetVcpu(def, vcpu)) ||
> + !vcpuinfo->online) {
> virReportError(VIR_ERR_INVALID_ARG,
> _("requested vcpu '%d' is not present in the domain"),
> vcpu);
> goto cleanup;
> }
>
> - if (!def->cputune.vcpupin) {
> - if (VIR_ALLOC(def->cputune.vcpupin) < 0)
> - goto cleanup;
> - def->cputune.nvcpupin = 0;
> - }
> - if (virDomainPinAdd(&def->cputune.vcpupin,
> - &def->cputune.nvcpupin,
> - cpumap,
> - maplen,
> - vcpu) < 0) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("failed to update or add vcpupin"));
> + virBitmapFree(vcpuinfo->cpumask);
> +
> + if (!(vcpuinfo->cpumask = virBitmapNewData(cpumap, maplen)))
> goto cleanup;
> - }
>
> ret = 0;
> +
> cleanup:
> virDomainObjEndAPI(&privdom);
> return ret;
> @@ -2566,15 +2558,14 @@ testDomainGetVcpuPinInfo(virDomainPtr dom,
> ncpumaps = virDomainDefGetVcpus(def);
>
> for (vcpu = 0; vcpu < ncpumaps; vcpu++) {
> - virDomainPinDefPtr pininfo;
> + virDomainVcpuInfoPtr vcpuinfo = virDomainDefGetVcpu(def, vcpu);
> virBitmapPtr bitmap = NULL;
>
> - pininfo = virDomainPinFind(def->cputune.vcpupin,
> - def->cputune.nvcpupin,
> - vcpu);
> + if (vcpuinfo && !vcpuinfo->online)
> + continue;
>
> - if (pininfo && pininfo->cpumask)
> - bitmap = pininfo->cpumask;
> + if (vcpuinfo->cpumask)
> + bitmap = vcpuinfo->cpumask;
> else if (def->cpumask)
> bitmap = def->cpumask;
> else
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list