Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- src/util/virresctrl.c | 81 ++++++++++++++++++++++++++++++++++----------------- 1 file changed, 54 insertions(+), 27 deletions(-) diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 42876c6e2b9b..df6461a04676 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -1041,6 +1041,55 @@ virResctrlAllocParse(virResctrlInfoPtr resctrl, } +static int +virResctrlAllocGetGroup(virResctrlInfoPtr resctrl, + const char *groupname, + virResctrlAllocPtr *alloc) +{ + char *schemata = NULL; + int rv = virFileReadValueString(&schemata, + SYSFS_RESCTRL_PATH + "/%s/schemata", + groupname); + + *alloc = NULL; + + if (rv < 0) + return rv; + + *alloc = virResctrlAllocNew(); + if (!*alloc) + goto error; + + if (virResctrlAllocParse(resctrl, *alloc, schemata) < 0) + goto error; + + VIR_FREE(schemata); + return 0; + + error: + VIR_FREE(schemata); + virObjectUnref(*alloc); + *alloc = NULL; + return -1; +} + + +static virResctrlAllocPtr +virResctrlAllocGetDefault(virResctrlInfoPtr resctrl) +{ + virResctrlAllocPtr ret = NULL; + int rv = virResctrlAllocGetGroup(resctrl, ".", &ret); + + if (rv == -2) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Could not read schemata file for the default group")); + } + + return ret; +} + + static void virResctrlAllocSubtractPerType(virResctrlAllocPerTypePtr dst, virResctrlAllocPerTypePtr src) @@ -1141,7 +1190,6 @@ virResctrlAllocGetUnused(virResctrlInfoPtr resctrl) virResctrlAllocPtr alloc = NULL; struct dirent *ent = NULL; DIR *dirp = NULL; - char *schemata = NULL; int rv = -1; if (virResctrlInfoIsEmpty(resctrl)) { @@ -1154,22 +1202,12 @@ virResctrlAllocGetUnused(virResctrlInfoPtr resctrl) if (!ret) return NULL; - if (virFileReadValueString(&schemata, - SYSFS_RESCTRL_PATH - "/schemata") < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Could not read schemata file for the default group")); - goto error; - } - - alloc = virResctrlAllocNew(); + alloc = virResctrlAllocGetDefault(resctrl); if (!alloc) goto error; - if (virResctrlAllocParse(resctrl, alloc, schemata) < 0) - goto error; - virResctrlAllocSubtract(ret, alloc); + virObjectUnref(alloc); if (virDirOpen(&dirp, SYSFS_RESCTRL_PATH) < 0) goto error; @@ -1178,11 +1216,7 @@ virResctrlAllocGetUnused(virResctrlInfoPtr resctrl) if (STREQ(ent->d_name, "info")) continue; - VIR_FREE(schemata); - rv = virFileReadValueString(&schemata, - SYSFS_RESCTRL_PATH - "/%s/schemata", - ent->d_name); + rv = virResctrlAllocGetGroup(resctrl, ent->d_name, &alloc); if (rv == -2) continue; @@ -1193,15 +1227,9 @@ virResctrlAllocGetUnused(virResctrlInfoPtr resctrl) goto error; } - virObjectUnref(alloc); - alloc = virResctrlAllocNew(); - if (!alloc) - goto error; - - if (virResctrlAllocParse(resctrl, alloc, schemata) < 0) - goto error; - virResctrlAllocSubtract(ret, alloc); + virObjectUnref(alloc); + alloc = NULL; } if (rv < 0) goto error; @@ -1209,7 +1237,6 @@ virResctrlAllocGetUnused(virResctrlInfoPtr resctrl) cleanup: virObjectUnref(alloc); VIR_DIR_CLOSE(dirp); - VIR_FREE(schemata); return ret; error: -- 2.15.1

Introduce virResctrlAllocCopyMasks() and use that to initially copy the default group schemata to the allocation before reserving any parts of the cache. The reason for this is that when new group is created the schemata will have unknown data in it. If there was previously group with the same CLoS ID, it will have the previous valies, if not it will have all bits set. And we need to set all unspecified (in the XML) allocations to the same one as the default group. Some non-Linux functions now need to be made public due to this change. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1289368 Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- src/util/virresctrl.c | 72 +++++++++++++++++++++++++++++++++++---------------- 1 file changed, 50 insertions(+), 22 deletions(-) diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index df6461a04676..a0ea2748713e 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -667,8 +667,6 @@ virResctrlAllocGetType(virResctrlAllocPtr resctrl, } -#ifdef __linux__ - static int virResctrlAllocUpdateMask(virResctrlAllocPtr resctrl, unsigned int level, @@ -696,8 +694,6 @@ virResctrlAllocUpdateMask(virResctrlAllocPtr resctrl, return virBitmapCopy(a_type->masks[cache], mask); } -#endif - static int virResctrlAllocUpdateSize(virResctrlAllocPtr resctrl, @@ -917,8 +913,6 @@ virResctrlAllocFormat(virResctrlAllocPtr resctrl) } -#ifdef __linux__ - static int virResctrlAllocParseProcessCache(virResctrlInfoPtr resctrl, virResctrlAllocPtr alloc, @@ -1090,6 +1084,8 @@ virResctrlAllocGetDefault(virResctrlInfoPtr resctrl) } +#ifdef __linux__ + static void virResctrlAllocSubtractPerType(virResctrlAllocPerTypePtr dst, virResctrlAllocPerTypePtr src) @@ -1298,23 +1294,8 @@ virResctrlAllocFindUnused(virResctrlAllocPerTypePtr a_type, ssize_t last_bits = 0; ssize_t last_pos = -1; - /* If there is no reservation requested we need to set all bits. That's due - * to weird interface of the resctrl sysfs. It's also the reason why we - * cannot reserve the whole cache in one allocation. */ - if (!size) { - a_mask = virBitmapNew(i_type->bits); - if (!a_mask) - return -1; - - virBitmapSetAll(a_mask); - - if (virResctrlAllocSetMask(a_type, cache, a_mask) < 0) { - virBitmapFree(a_mask); - return -1; - } - + if (!size) return 0; - } if (cache >= f_type->nmasks) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -1417,6 +1398,44 @@ virResctrlAllocFindUnused(virResctrlAllocPerTypePtr a_type, } +static int +virResctrlAllocCopyMasks(virResctrlAllocPtr dst, + virResctrlAllocPtr src) +{ + unsigned int level = 0; + + for (level = 0; level < src->nlevels; level++) { + virResctrlAllocPerLevelPtr s_level = src->levels[level]; + unsigned int type = 0; + + if (!s_level) + continue; + + for (type = 0; type < VIR_CACHE_TYPE_LAST; type++) { + virResctrlAllocPerTypePtr s_type = s_level->types[type]; + virResctrlAllocPerTypePtr d_type = NULL; + unsigned int cache = 0; + + if (!s_type) + continue; + + d_type = virResctrlAllocGetType(dst, level, type); + if (!d_type) + return -1; + + for (cache = 0; cache < s_type->nmasks; cache++) { + virBitmapPtr mask = s_type->masks[cache]; + + if (mask && virResctrlAllocUpdateMask(dst, level, type, cache, mask) < 0) + return -1; + } + } + } + + return 0; +} + + /* * This function is called when creating an allocation in the system. What it * does is that it gets all the unused bits using virResctrlAllocGetUnused() and @@ -1430,11 +1449,19 @@ virResctrlAllocMasksAssign(virResctrlInfoPtr resctrl, int ret = -1; unsigned int level = 0; virResctrlAllocPtr alloc_free = NULL; + virResctrlAllocPtr alloc_default = NULL; alloc_free = virResctrlAllocGetUnused(resctrl); if (!alloc_free) return -1; + alloc_default = virResctrlAllocGetDefault(resctrl); + if (!alloc_default) + return -1; + + if (virResctrlAllocCopyMasks(alloc, alloc_default) < 0) + return -1; + for (level = 0; level < alloc->nlevels; level++) { virResctrlAllocPerLevelPtr a_level = alloc->levels[level]; virResctrlAllocPerLevelPtr f_level = NULL; @@ -1482,6 +1509,7 @@ virResctrlAllocMasksAssign(virResctrlInfoPtr resctrl, ret = 0; cleanup: virObjectUnref(alloc_free); + virObjectUnref(alloc_default); return ret; } -- 2.15.1

On 01/31/2018 08:36 AM, Martin Kletzander wrote: [...] Coverity was fairly grumpy this morning... So does this leak alloc_free possibly? and does this leak both alloc_free and alloc_default possibly? John

Due to confusing naming the pointer to the mask got copied which must not happen, so use UpdateMask instead of SetMask. That also means we can get completely rid of SetMask. Also don't clear the free bits since it is not used again (leftover from previous versions). Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- src/util/virresctrl.c | 41 ++++++++++++++--------------------------- 1 file changed, 14 insertions(+), 27 deletions(-) diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index a0ea2748713e..fdd1ded6f76f 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -1253,22 +1253,6 @@ virResctrlAllocGetUnused(virResctrlInfoPtr resctrl ATTRIBUTE_UNUSED) #endif /* ! __linux__ */ -static int -virResctrlAllocSetMask(virResctrlAllocPerTypePtr a_type, - unsigned int cache, - virBitmapPtr mask) -{ - if (a_type->nmasks <= cache && - VIR_EXPAND_N(a_type->masks, a_type->nmasks, - cache - a_type->nmasks + 1) < 0) - return -1; - - virBitmapFree(a_type->masks[cache]); - a_type->masks[cache] = mask; - - return 0; -} - /* * Given the information about requested allocation type `a_type`, the host @@ -1276,16 +1260,19 @@ virResctrlAllocSetMask(virResctrlAllocPerTypePtr a_type, * this function tries to find the smallest free space in which the allocation * for cache id `cache` would fit. We're looking for the smallest place in * order to minimize fragmentation and maximize the possibility of succeeding. + * + * Per-cache allocation for the @level, @type and @cache must already be + * allocated for @alloc (does not have to exist though). */ static int -virResctrlAllocFindUnused(virResctrlAllocPerTypePtr a_type, +virResctrlAllocFindUnused(virResctrlAllocPtr alloc, virResctrlInfoPerTypePtr i_type, virResctrlAllocPerTypePtr f_type, unsigned int level, unsigned int type, unsigned int cache) { - unsigned long long *size = a_type->sizes[cache]; + unsigned long long *size = alloc->levels[level]->types[type]->sizes[cache]; virBitmapPtr a_mask = NULL; virBitmapPtr f_mask = NULL; unsigned long long need_bits; @@ -1293,6 +1280,7 @@ virResctrlAllocFindUnused(virResctrlAllocPerTypePtr a_type, ssize_t pos = -1; ssize_t last_bits = 0; ssize_t last_pos = -1; + int ret = -1; if (!size) return 0; @@ -1384,17 +1372,16 @@ virResctrlAllocFindUnused(virResctrlAllocPerTypePtr a_type, if (!a_mask) return -1; - for (i = last_pos; i < last_pos + need_bits; i++) { + for (i = last_pos; i < last_pos + need_bits; i++) ignore_value(virBitmapSetBit(a_mask, i)); - ignore_value(virBitmapClearBit(f_mask, i)); - } - if (virResctrlAllocSetMask(a_type, cache, a_mask) < 0) { - virBitmapFree(a_mask); - return -1; - } + if (virResctrlAllocUpdateMask(alloc, level, type, cache, a_mask) < 0) + goto cleanup; - return 0; + ret = 0; + cleanup: + virBitmapFree(a_mask); + return ret; } @@ -1500,7 +1487,7 @@ virResctrlAllocMasksAssign(virResctrlInfoPtr resctrl, virResctrlInfoPerLevelPtr i_level = resctrl->levels[level]; virResctrlInfoPerTypePtr i_type = i_level->types[type]; - if (virResctrlAllocFindUnused(a_type, i_type, f_type, level, type, cache) < 0) + if (virResctrlAllocFindUnused(alloc, i_type, f_type, level, type, cache) < 0) goto cleanup; } } -- 2.15.1

The virresctrl will use this as well and we need to have that info after restart to properly clean up /sys/fs/resctrl. Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- src/qemu/qemu_cgroup.c | 4 ---- src/qemu/qemu_process.c | 4 ++++ 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index fbf79a6d8f33..b604edb31c0d 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -986,10 +986,6 @@ qemuConnectCgroup(virDomainObjPtr vm) if (!virCgroupAvailable()) goto done; - priv->machineName = qemuDomainGetMachineName(vm); - if (!priv->machineName) - goto cleanup; - virCgroupFree(&priv->cgroup); if (virCgroupNewDetectMachine(vm->def->name, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 0577f4c35d08..239798fa5d7c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -7198,6 +7198,10 @@ qemuProcessReconnect(void *opaque) if (qemuHostdevUpdateActiveDomainDevices(driver, obj->def) < 0) goto error; + priv->machineName = qemuDomainGetMachineName(obj); + if (!priv->machineName) + goto error; + if (qemuConnectCgroup(obj) < 0) goto error; -- 2.15.1

We can use this from more places later, so just a future code de-duplication. Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- src/libvirt_private.syms | 1 + src/util/virresctrl.c | 29 ++++++++++++++++++++--------- src/util/virresctrl.h | 4 ++++ 3 files changed, 25 insertions(+), 9 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 792fb60568bd..9339c2c3259d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2560,6 +2560,7 @@ virCacheTypeFromString; virCacheTypeToString; virResctrlAllocAddPID; virResctrlAllocCreate; +virResctrlAllocDeterminePath; virResctrlAllocForeachSize; virResctrlAllocFormat; virResctrlAllocGetID; diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index fdd1ded6f76f..e1c4998f7151 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -1501,6 +1501,25 @@ virResctrlAllocMasksAssign(virResctrlInfoPtr resctrl, } +int +virResctrlAllocDeterminePath(virResctrlAllocPtr alloc, + const char *machinename) +{ + if (!alloc->id) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Resctrl Allocation ID must be set before creation")); + return -1; + } + + if (!alloc->path && + virAsprintf(&alloc->path, "%s/%s-%s", + SYSFS_RESCTRL_PATH, machinename, alloc->id) < 0) + return -1; + + return 0; +} + + /* This checks if the directory for the alloc exists. If not it tries to create * it and apply appropriate alloc settings. */ int @@ -1522,15 +1541,7 @@ virResctrlAllocCreate(virResctrlInfoPtr resctrl, return -1; } - if (!alloc->id) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Resctrl Allocation ID must be set before creation")); - return -1; - } - - if (!alloc->path && - virAsprintf(&alloc->path, "%s/%s-%s", - SYSFS_RESCTRL_PATH, machinename, alloc->id) < 0) + if (virResctrlAllocDeterminePath(alloc, machinename) < 0) return -1; if (virFileExists(alloc->path)) { diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h index 63a954feaad1..5368ba211924 100644 --- a/src/util/virresctrl.h +++ b/src/util/virresctrl.h @@ -102,6 +102,10 @@ virResctrlAllocGetID(virResctrlAllocPtr alloc); char * virResctrlAllocFormat(virResctrlAllocPtr alloc); +int +virResctrlAllocDeterminePath(virResctrlAllocPtr alloc, + const char *machinename); + int virResctrlAllocCreate(virResctrlInfoPtr r_info, virResctrlAllocPtr alloc, -- 2.15.1

During reconnect we need to reconstruct the paths of all cachetunes so that they get cleaned up when the domain is stopped. Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- src/qemu/qemu_process.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 239798fa5d7c..bcd4ac8ad699 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -77,6 +77,7 @@ #include "configmake.h" #include "nwfilter_conf.h" #include "netdev_bandwidth_conf.h" +#include "virresctrl.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -7332,6 +7333,12 @@ qemuProcessReconnect(void *opaque) if (qemuConnectAgent(driver, obj) < 0) goto error; + for (i = 0; i < obj->def->ncachetunes; i++) { + if (virResctrlAllocDeterminePath(obj->def->cachetunes[i]->alloc, + priv->machineName) < 0) + goto error; + } + /* update domain state XML with possibly updated state in virDomainObj */ if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, obj, driver->caps) < 0) goto error; -- 2.15.1