Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- src/qemu/qemu_capabilities.c | 4 ++-- src/util/virbitmap.c | 6 ++++-- src/util/virbitmap.h | 2 +- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 7cb091056b48..c87feefb3be3 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -1509,7 +1509,7 @@ int virQEMUCapsParseHelpStr(const char *qemu, qemuCaps, check_yajl) < 0) goto cleanup; - strflags = virBitmapString(qemuCaps->flags); + strflags = virBitmapString(qemuCaps->flags, true); VIR_DEBUG("Version %u.%u.%u, cooked version %u, flags %s", major, minor, micro, *version, NULLSTR(strflags)); VIR_FREE(strflags); @@ -2376,7 +2376,7 @@ virQEMUCapsClear(virQEMUCapsPtr qemuCaps, char *virQEMUCapsFlagsString(virQEMUCapsPtr qemuCaps) { - return virBitmapString(qemuCaps->flags); + return virBitmapString(qemuCaps->flags, true); } diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index 57a8fbf82c78..cb6600074781 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -312,17 +312,19 @@ int virBitmapGetBit(virBitmapPtr bitmap, size_t b, bool *result) /** * virBitmapString: * @bitmap: Pointer to bitmap + * @prefix: Whether to prepend "0x" * * Convert @bitmap to printable string. * * Returns pointer to the string or NULL on error. */ -char *virBitmapString(virBitmapPtr bitmap) +char *virBitmapString(virBitmapPtr bitmap, bool prefix) { virBuffer buf = VIR_BUFFER_INITIALIZER; size_t sz; - virBufferAddLit(&buf, "0x"); + if (prefix) + virBufferAddLit(&buf, "0x"); sz = bitmap->map_len; diff --git a/src/util/virbitmap.h b/src/util/virbitmap.h index ffa3c42d792b..dc8fb71a07b8 100644 --- a/src/util/virbitmap.h +++ b/src/util/virbitmap.h @@ -80,7 +80,7 @@ bool virBitmapIsBitSet(virBitmapPtr bitmap, size_t b) int virBitmapGetBit(virBitmapPtr bitmap, size_t b, bool *result) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK; -char *virBitmapString(virBitmapPtr bitmap) +char *virBitmapString(virBitmapPtr bitmap, bool prefix) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; char *virBitmapFormat(virBitmapPtr bitmap); -- 2.15.0

On Mon, Nov 13, 2017 at 02:22:20PM -0500, John Ferlan wrote: hehe, good catch You mean add a space between "to" and virBitmapToString? It is there. Sure, mentioning here is enough. Sure, but a) mainly because otherwise it's a long line and b) it's along the other functions that do the same so the reading style is the same, but here it would just stand out as the only function, I think. Anyway, I fixed that as well.

It is literally only a wrapper around virBitmapNewData() and virBitmapFormat(), only the naming was wrong since it was introduced. And because we have virBitmap*String functions where the meaning of the 'String' is constant, this might confuse someone. Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- src/libvirt_private.syms | 2 +- src/util/virbitmap.c | 6 +++--- src/util/virbitmap.h | 4 ++-- tests/virbitmaptest.c | 4 ++-- tools/virsh-domain.c | 4 ++-- tools/virsh-host.c | 2 +- 6 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c11dc9ab5672..e535167e28fc 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1344,7 +1344,7 @@ virBitmapClearBit; virBitmapClearBitExpand; virBitmapCopy; virBitmapCountBits; -virBitmapDataToString; +virBitmapDataFormat; virBitmapEqual; virBitmapFormat; virBitmapFree; diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index 58ef1a2e299b..3d160aa4d50a 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -1044,7 +1044,7 @@ virBitmapCountBits(virBitmapPtr bitmap) } /** - * virBitmapDataToString: + * virBitmapDataFormat: * @data: the data * @len: length of @data in bytes * @@ -1054,8 +1054,8 @@ virBitmapCountBits(virBitmapPtr bitmap) * Returns: a string representation of the data, or NULL on error */ char * -virBitmapDataToString(const void *data, - int len) +virBitmapDataFormat(const void *data, + int len) { virBitmapPtr map = NULL; char *ret = NULL; diff --git a/src/util/virbitmap.h b/src/util/virbitmap.h index 99eb779d7188..720b389cfe52 100644 --- a/src/util/virbitmap.h +++ b/src/util/virbitmap.h @@ -136,8 +136,8 @@ ssize_t virBitmapNextClearBit(virBitmapPtr bitmap, ssize_t pos) size_t virBitmapCountBits(virBitmapPtr bitmap) ATTRIBUTE_NONNULL(1); -char *virBitmapDataToString(const void *data, - int len) +char *virBitmapDataFormat(const void *data, + int len) ATTRIBUTE_NONNULL(1); bool virBitmapOverlaps(virBitmapPtr b1, virBitmapPtr b2) diff --git a/tests/virbitmaptest.c b/tests/virbitmaptest.c index c24d4a72f70a..488796719dd9 100644 --- a/tests/virbitmaptest.c +++ b/tests/virbitmaptest.c @@ -336,12 +336,12 @@ test5(const void *v ATTRIBUTE_UNUSED) data2[4] != 0x04) goto error; - if (!(str = virBitmapDataToString(data, sizeof(data)))) + if (!(str = virBitmapDataFormat(data, sizeof(data)))) goto error; if (STRNEQ(str, "0,9,34")) goto error; VIR_FREE(str); - if (!(str = virBitmapDataToString(data2, len2))) + if (!(str = virBitmapDataFormat(data2, len2))) goto error; if (STRNEQ(str, "0,2,9,15,34")) goto error; diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 42d5526374fc..adfde692993a 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6527,7 +6527,7 @@ virshVcpuinfoPrintAffinity(vshControl *ctl, vshPrint(ctl, "%-15s ", _("CPU Affinity:")); if (pretty) { - if (!(str = virBitmapDataToString(cpumap, VIR_CPU_MAPLEN(maxcpu)))) + if (!(str = virBitmapDataFormat(cpumap, VIR_CPU_MAPLEN(maxcpu)))) goto cleanup; vshPrint(ctl, _("%s (out of %d)"), str, maxcpu); } else { @@ -6781,7 +6781,7 @@ virshPrintPinInfo(vshControl *ctl, { char *str = NULL; - if (!(str = virBitmapDataToString(cpumap, cpumaplen))) + if (!(str = virBitmapDataFormat(cpumap, cpumaplen))) return false; vshPrint(ctl, "%s", str); diff --git a/tools/virsh-host.c b/tools/virsh-host.c index 5509065fd606..ecaf830e350d 100644 --- a/tools/virsh-host.c +++ b/tools/virsh-host.c @@ -717,7 +717,7 @@ cmdNodeCpuMap(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) vshPrint(ctl, "%-15s ", _("CPU map:")); if (pretty) { - char *str = virBitmapDataToString(cpumap, VIR_CPU_MAPLEN(cpunum)); + char *str = virBitmapDataFormat(cpumap, VIR_CPU_MAPLEN(cpunum)); if (!str) goto cleanup; -- 2.15.0

Truncate the output so that it is only as big as is needed to fit all the bits, not all the units from the map. This will be needed in the future in order to properly format bitmaps for kernel's sysfs files. Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- src/qemu/qemu_capabilities.c | 4 ++-- src/util/virbitmap.c | 29 +++++++++++++++++++++++++++-- src/util/virbitmap.h | 2 +- 3 files changed, 30 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 60ca8ee9ef84..232f88af0fb8 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -1509,7 +1509,7 @@ int virQEMUCapsParseHelpStr(const char *qemu, qemuCaps, check_yajl) < 0) goto cleanup; - strflags = virBitmapToString(qemuCaps->flags, true); + strflags = virBitmapToString(qemuCaps->flags, true, false); VIR_DEBUG("Version %u.%u.%u, cooked version %u, flags %s", major, minor, micro, *version, NULLSTR(strflags)); VIR_FREE(strflags); @@ -2376,7 +2376,7 @@ virQEMUCapsClear(virQEMUCapsPtr qemuCaps, char *virQEMUCapsFlagsString(virQEMUCapsPtr qemuCaps) { - return virBitmapToString(qemuCaps->flags, true); + return virBitmapToString(qemuCaps->flags, true, false); } diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index 3d160aa4d50a..0a7d3452b90c 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -313,15 +313,19 @@ int virBitmapGetBit(virBitmapPtr bitmap, size_t b, bool *result) * virBitmapToString: * @bitmap: Pointer to bitmap * @prefix: Whether to prepend "0x" + * @trim: Whether to output only the minimum required characters * * Convert @bitmap to printable string. * * Returns pointer to the string or NULL on error. */ -char *virBitmapToString(virBitmapPtr bitmap, bool prefix) +char *virBitmapToString(virBitmapPtr bitmap, bool prefix, bool trim) { virBuffer buf = VIR_BUFFER_INITIALIZER; size_t sz; + size_t len; + size_t diff; + char *ret = NULL; if (prefix) virBufferAddLit(&buf, "0x"); @@ -335,7 +339,28 @@ char *virBitmapToString(virBitmapPtr bitmap, bool prefix) } virBufferCheckError(&buf); - return virBufferContentAndReset(&buf); + ret = virBufferContentAndReset(&buf); + if (!ret) + return NULL; + + if (!trim) + return ret; + + if (bitmap->max_bit != bitmap->map_len * VIR_BITMAP_BITS_PER_UNIT) { + char *tmp = ret; + + if (prefix) + tmp += 2; + + len = strlen(tmp); + sz = VIR_DIV_UP(bitmap->max_bit, 4); + diff = len - sz; + + if (diff) + memmove(tmp, tmp + diff, sz + 1); + } + + return ret; } /** diff --git a/src/util/virbitmap.h b/src/util/virbitmap.h index 720b389cfe52..02acb7519d37 100644 --- a/src/util/virbitmap.h +++ b/src/util/virbitmap.h @@ -80,7 +80,7 @@ bool virBitmapIsBitSet(virBitmapPtr bitmap, size_t b) int virBitmapGetBit(virBitmapPtr bitmap, size_t b, bool *result) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK; -char *virBitmapToString(virBitmapPtr bitmap, bool prefix) +char *virBitmapToString(virBitmapPtr bitmap, bool prefix, bool trim) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; char *virBitmapFormat(virBitmapPtr bitmap); -- 2.15.0

On 11/13/2017 03:50 AM, Martin Kletzander wrote: With a couple of minor/noted adjustments below, Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John Two blank lines for new functions to be converted into a bitmap Two blank lines hahahaha, but you're missing the 'v' (covfefe) Don't believe either needs NULLSTR.... If they did STRNEQ would probably need NULLABLE too ;-)... Still @str cannot be NULL if we get here and if strings[i] is NULL, then something very strange happened.

On 11/13/2017 03:50 AM, Martin Kletzander wrote: Is there a specific reason why you don't free memory? Consider that the corollary virBitmapExpand can always be used to regrow the bitmap. I'm fine with not free'ing, but maybe someone would want to... OK, sure they can supply the patch some day, I know... With a couple of notes below handled, Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John Considering adding, "NB: Does not adjust the map->map_len so that a subsequent virBitmapExpand doesn't necessarily need to reallocate." (not required, just a suggestion) void virBitmapStrink(virBitmapPtr map, size_t b) Not that it matters but it's always nice to keep the .h file in the same relative order as the .c file... So this would move below virBitmapSetBit

Because the cache banks are initialized based on the order in which their respective directories exist on the filesystem, they can appear in diferrent order. This is here mainly for tests because the cache directory might have different order of children nodes and tests would fail otherwise. It should not be the case with sysfs, but one can never be sure. And this does not take almost any extra time, mainly because it gets initialized once per driver. Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- src/conf/capabilities.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 9920a675aca3..095ef51c424a 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -1561,6 +1561,20 @@ virCapsHostCacheBankFree(virCapsHostCacheBankPtr ptr) VIR_FREE(ptr); } +static int +virCapsHostCacheBankSorter(const void *a, const void *b) +{ + virCapsHostCacheBankPtr ca = *(virCapsHostCacheBankPtr *)a; + virCapsHostCacheBankPtr cb = *(virCapsHostCacheBankPtr *)b; + + if (ca->level < cb->level) + return -1; + if (ca->level > cb->level) + return 1; + + return ca->id - cb->id; +} + int virCapabilitiesInitCaches(virCapsPtr caps) { @@ -1700,6 +1714,12 @@ virCapabilitiesInitCaches(virCapsPtr caps) goto cleanup; } + /* Sort the array in order for the tests to be predicable. This way we can + * still traverse the directory instead of guessing names (in case there is + * 'index1' and 'index3' but no 'index2'). */ + qsort(caps->host.caches, caps->host.ncaches, + sizeof(*caps->host.caches), virCapsHostCacheBankSorter); + ret = 0; cleanup: VIR_FREE(type); -- 2.15.0

Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- src/conf/capabilities.c | 45 ++++++++++++++-------- tests/vircaps2xmldata/vircaps-x86_64-caches.xml | 2 +- .../vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml | 4 +- .../vircaps2xmldata/vircaps-x86_64-resctrl-skx.xml | 4 +- tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml | 4 +- 5 files changed, 36 insertions(+), 23 deletions(-) diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 095ef51c424a..5bf8ac2019f9 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -883,7 +883,8 @@ virCapabilitiesFormatCaches(virBufferPtr buf, for (i = 0; i < ncaches; i++) { virCapsHostCacheBankPtr bank = caches[i]; char *cpus_str = virBitmapFormat(bank->cpus); - bool kilos = !(bank->size % 1024); + const char *unit = NULL; + unsigned long long short_size = virPrettySize(bank->size, &unit); if (!cpus_str) return -1; @@ -897,32 +898,44 @@ virCapabilitiesFormatCaches(virBufferPtr buf, "size='%llu' unit='%s' cpus='%s'", bank->id, bank->level, virCacheTypeToString(bank->type), - bank->size >> (kilos * 10), - kilos ? "KiB" : "B", - cpus_str); + short_size, unit, cpus_str); VIR_FREE(cpus_str); virBufferSetChildIndent(&controlBuf, buf); for (j = 0; j < bank->ncontrols; j++) { - bool min_kilos = !(bank->controls[j]->granularity % 1024); - - /* Only use KiB if both values are divisible */ - if (bank->controls[j]->min) - min_kilos = min_kilos && !(bank->controls[j]->min % 1024); + const char *min_unit; + unsigned long long gran_short_size = bank->controls[j]->granularity; + unsigned long long min_short_size = bank->controls[j]->min; + + gran_short_size = virPrettySize(gran_short_size, &unit); + min_short_size = virPrettySize(min_short_size, &min_unit); + + /* Only use the smaller unit if they are different */ + if (min_short_size) { + unsigned long long gran_div; + unsigned long long min_div; + + gran_div = bank->controls[j]->granularity / gran_short_size; + min_div = bank->controls[j]->min / min_short_size; + + if (min_div > gran_div) { + min_short_size *= min_div / gran_div; + } else if (min_div < gran_div) { + unit = min_unit; + gran_short_size *= gran_div / min_div; + } + } virBufferAsprintf(&controlBuf, "<control granularity='%llu'", - bank->controls[j]->granularity >> (min_kilos * 10)); + gran_short_size); - if (bank->controls[j]->min) { - virBufferAsprintf(&controlBuf, - " min='%llu'", - bank->controls[j]->min >> (min_kilos * 10)); - } + if (min_short_size) + virBufferAsprintf(&controlBuf, " min='%llu'", min_short_size); virBufferAsprintf(&controlBuf, " unit='%s' type='%s' maxAllocs='%u'/>\n", - min_kilos ? "KiB" : "B", + unit, virCacheTypeToString(bank->controls[j]->scope), bank->controls[j]->max_allocation); } diff --git a/tests/vircaps2xmldata/vircaps-x86_64-caches.xml b/tests/vircaps2xmldata/vircaps-x86_64-caches.xml index fe0be6d08fa7..0c6f3769a2a7 100644 --- a/tests/vircaps2xmldata/vircaps-x86_64-caches.xml +++ b/tests/vircaps2xmldata/vircaps-x86_64-caches.xml @@ -29,7 +29,7 @@ </cells> </topology> <cache> - <bank id='0' level='3' type='both' size='8192' unit='KiB' cpus='0-7'/> + <bank id='0' level='3' type='both' size='8' unit='MiB' cpus='0-7'/> </cache> </host> diff --git a/tests/vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml b/tests/vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml index 7361537bfb56..443917c62d69 100644 --- a/tests/vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml +++ b/tests/vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml @@ -41,11 +41,11 @@ </cells> </topology> <cache> - <bank id='0' level='3' type='both' size='15360' unit='KiB' cpus='0-5'> + <bank id='0' level='3' type='both' size='15' unit='MiB' cpus='0-5'> <control granularity='768' unit='KiB' type='code' maxAllocs='8'/> <control granularity='768' unit='KiB' type='data' maxAllocs='8'/> </bank> - <bank id='1' level='3' type='both' size='15360' unit='KiB' cpus='6-11'> + <bank id='1' level='3' type='both' size='15' unit='MiB' cpus='6-11'> <control granularity='768' unit='KiB' type='code' maxAllocs='8'/> <control granularity='768' unit='KiB' type='data' maxAllocs='8'/> </bank> diff --git a/tests/vircaps2xmldata/vircaps-x86_64-resctrl-skx.xml b/tests/vircaps2xmldata/vircaps-x86_64-resctrl-skx.xml index 4e91c87de3b1..0cd25e59a9e0 100644 --- a/tests/vircaps2xmldata/vircaps-x86_64-resctrl-skx.xml +++ b/tests/vircaps2xmldata/vircaps-x86_64-resctrl-skx.xml @@ -22,8 +22,8 @@ </cells> </topology> <cache> - <bank id='0' level='3' type='both' size='33792' unit='KiB' cpus='0'> - <control granularity='3072' unit='KiB' type='both' maxAllocs='16'/> + <bank id='0' level='3' type='both' size='33' unit='MiB' cpus='0'> + <control granularity='3' unit='MiB' type='both' maxAllocs='16'/> </bank> </cache> </host> diff --git a/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml b/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml index eb02ad3322a2..7629259294d6 100644 --- a/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml +++ b/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml @@ -41,10 +41,10 @@ </cells> </topology> <cache> - <bank id='0' level='3' type='both' size='15360' unit='KiB' cpus='0-5'> + <bank id='0' level='3' type='both' size='15' unit='MiB' cpus='0-5'> <control granularity='768' min='1536' unit='KiB' type='both' maxAllocs='4'/> </bank> - <bank id='1' level='3' type='both' size='15360' unit='KiB' cpus='6-11'> + <bank id='1' level='3' type='both' size='15' unit='MiB' cpus='6-11'> <control granularity='768' min='1536' unit='KiB' type='both' maxAllocs='4'/> </bank> </cache> -- 2.15.0

On Tue, Nov 14, 2017 at 07:38:50AM -0500, John Ferlan wrote: done if I add STRNEQ there it doesn't allow me to remove any code, it would just add a line. I need the math after that anyway.

On 11/13/2017 03:50 AM, Martin Kletzander wrote: [...] This decrements the for end loop condition... So this code is going backwards through the levels... I guess it works, but it's one of those "things" about altering the ending of a for loop within the for loop that just "looks strange"... Alternatively: while ((level = resctrl->levels[--resctrl->nlevels])) right? Nothing that I'd ask to change - it just "looks strange"... level->types[j]->cbm_mask is leaked. The indent is off by 1 for the above 3 lines. Is it "by design" to have a number of failures just return 0? My read of the previous code is that it would return some sort of failure if the "dynamic" reading failed. This while loop would ignore failures and "act as if" there was no cache without letting the consumer know. Is that the intent? If so, then should the code also clear the Last error message? Probably should call that out either in the commit message or the beginning of the while loop that if we have a problem, then we're going to "quietly" continue on as if there was no cache. I'm not opposed to continuing on; however, it is different (albeit difficult to see with the diffs enabled - so I've been going back and forth between old/new code). Two blank lines between functions OK so I've: Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; But obviously there are still questions, still if the design is as I've noted, then carry on... Otherwise, handle/return the error I guess which isn't rocket science. John [...]

With this commit we finally have a way to read and manipulate basic resctrl settings. Locking is done only on exposed functions that read/write from/to resctrlfs. Not in fuctions that are exposed in virresctrlpriv.h as those are only supposed to be used from tests. Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- src/Makefile.am | 2 +- src/libvirt_private.syms | 12 + src/util/virresctrl.c | 1012 ++++++++++++++++++++++++++++++++++++++++++++- src/util/virresctrl.h | 59 +++ src/util/virresctrlpriv.h | 32 ++ 5 files changed, 1115 insertions(+), 2 deletions(-) create mode 100644 src/util/virresctrlpriv.h diff --git a/src/Makefile.am b/src/Makefile.am index 1d24231249de..ad113262fbb0 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -167,7 +167,7 @@ UTIL_SOURCES = \ util/virprocess.c util/virprocess.h \ util/virqemu.c util/virqemu.h \ util/virrandom.h util/virrandom.c \ - util/virresctrl.h util/virresctrl.c \ + util/virresctrl.h util/virresctrl.c util/virresctrlpriv.h \ util/virrotatingfile.h util/virrotatingfile.c \ util/virscsi.c util/virscsi.h \ util/virscsihost.c util/virscsihost.h \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b24728ce4a1d..37bac41e618b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2532,6 +2532,18 @@ virRandomInt; # util/virresctrl.h virCacheTypeFromString; virCacheTypeToString; +virResctrlAllocAddPID; +virResctrlAllocCreate; +virResctrlAllocForeachSize; +virResctrlAllocFormat; +virResctrlAllocGetFree; +virResctrlAllocMasksAssign; +virResctrlAllocNewFromInfo; +virResctrlAllocRemove; +virResctrlAllocSetID; +virResctrlAllocSetSize; +virResctrlAllocUpdateMask; +virResctrlAllocUpdateSize; virResctrlGetInfo; virResctrlInfoGetCache; diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 6c6692e78f42..ac1b38436bb2 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -23,7 +23,7 @@ #include <sys/stat.h> #include <fcntl.h> -#include "virresctrl.h" +#include "virresctrlpriv.h" #include "c-ctype.h" #include "count-one-bits.h" @@ -151,6 +151,153 @@ virResctrlInfoNew(void) } +/* Alloc-related definitions and AllocClass-related functions */ +typedef struct _virResctrlAllocPerType virResctrlAllocPerType; +typedef virResctrlAllocPerType *virResctrlAllocPerTypePtr; +struct _virResctrlAllocPerType { + /* There could be bool saying whether this is set or not, but since everything + * in virResctrlAlloc (and most of libvirt) goes with pointer arrays we would + * have to have one more level of allocation anyway, so this stays faithful to + * the concept */ + unsigned long long **sizes; + size_t nsizes; + + /* Mask for each cache */ + virBitmapPtr *masks; + size_t nmasks; +}; + +typedef struct _virResctrlAllocPerLevel virResctrlAllocPerLevel; +typedef virResctrlAllocPerLevel *virResctrlAllocPerLevelPtr; +struct _virResctrlAllocPerLevel { + virResctrlAllocPerTypePtr *types; /* Indexed with enum virCacheType */ +}; + +struct _virResctrlAlloc { + virObject parent; + + virResctrlAllocPerLevelPtr *levels; + size_t nlevels; + + char *id; /* The identifier (any unique string for now) */ + char *path; +}; + +static virClassPtr virResctrlAllocClass; + +static void +virResctrlAllocDispose(void *obj) +{ + size_t i = 0; + size_t j = 0; + size_t k = 0; + + virResctrlAllocPtr resctrl = obj; + + for (i = 0; i < resctrl->nlevels; i++) { + virResctrlAllocPerLevelPtr level = resctrl->levels[--resctrl->nlevels]; + + if (!level) + continue; + + for (j = 0; j < VIR_CACHE_TYPE_LAST; j++) { + virResctrlAllocPerTypePtr type = level->types[j]; + + if (!type) + continue; + + for (k = 0; k < type->nsizes; k++) + VIR_FREE(type->sizes[k]); + + VIR_FREE(type->sizes); + VIR_FREE(type); + } + VIR_FREE(level->types); + VIR_FREE(level); + } + + VIR_FREE(resctrl->id); + VIR_FREE(resctrl->levels); +} + +static int virResctrlAllocOnceInit(void) +{ + if (!(virResctrlAllocClass = virClassNew(virClassForObject(), + "virResctrlAlloc", + sizeof(virResctrlAlloc), + virResctrlAllocDispose))) + return -1; + + return 0; +} + +VIR_ONCE_GLOBAL_INIT(virResctrlAlloc) + +static virResctrlAllocPtr +virResctrlAllocNew(void) +{ + if (virResctrlAllocInitialize() < 0) + return NULL; + + return virObjectNew(virResctrlAllocClass); +} + + +/* Common functions */ +static int +virResctrlLockInternal(int op) +{ + int fd = open(SYSFS_RESCTRL_PATH, O_DIRECTORY | O_CLOEXEC); + + if (fd < 0) { + virReportSystemError(errno, "%s", _("Cannot open resctrlfs")); + return -1; + } + + if (flock(fd, op) < 0) { + virReportSystemError(errno, "%s", _("Cannot lock resctrlfs")); + VIR_FORCE_CLOSE(fd); + return -1; + } + + return fd; +} + +static inline int +virResctrlLockRead(void) +{ + return virResctrlLockInternal(LOCK_SH); +} + +static inline int +virResctrlLockWrite(void) +{ + return virResctrlLockInternal(LOCK_EX); +} + +static int +virResctrlUnlock(int fd) +{ + int ret = -1; + + if (fd == -1) + return 0; + + /* The lock gets unlocked by closing the fd, which we need to do anyway in + * order to clean up properly */ + if (VIR_CLOSE(fd) < 0) { + virReportSystemError(errno, "%s", _("Cannot close resctrlfs")); + + /* Trying to save the already broken */ + if (flock(fd, LOCK_UN) < 0) + virReportSystemError(errno, "%s", _("Cannot unlock resctrlfs")); + return -1; + } + + return ret; +} + + /* Info-related functions */ int virResctrlGetInfo(virResctrlInfoPtr *resctrl) @@ -318,3 +465,866 @@ virResctrlInfoGetCache(virResctrlInfoPtr resctrl, VIR_FREE(*controls); goto cleanup; } + + +/* Alloc-related functions */ +static virResctrlAllocPerTypePtr +virResctrlAllocFindType(virResctrlAllocPtr *resctrl, + unsigned int level, + virCacheType type, + bool alloc) +{ + virResctrlAllocPerLevelPtr a_level = NULL; + virResctrlAllocPtr tmp = NULL; + + if (!*resctrl) { + if (!alloc || !(*resctrl = virResctrlAllocNew())) + return NULL; + } + + tmp = *resctrl; + + /* Per-level array */ + if (tmp->nlevels <= level) { + if (!alloc || VIR_EXPAND_N(tmp->levels, tmp->nlevels, + level - tmp->nlevels + 1) < 0) + return NULL; + } + + if (!tmp->levels[level]) { + if (!alloc || + VIR_ALLOC(tmp->levels[level]) < 0 || + VIR_ALLOC_N(tmp->levels[level]->types, VIR_CACHE_TYPE_LAST) < 0) + return NULL; + } + a_level = tmp->levels[level]; + + if (!a_level->types[type]) { + if (!alloc || VIR_ALLOC(a_level->types[type]) < 0) + return NULL; + } + + return a_level->types[type]; +} + +static virBitmapPtr * +virResctrlAllocFindMask(virResctrlAllocPtr *resctrl, + unsigned int level, + virCacheType type, + unsigned int cache, + bool alloc) +{ + virResctrlAllocPerTypePtr a_type = virResctrlAllocFindType(resctrl, level, + type, alloc); + + if (!a_type) + return NULL; + + if (!a_type->masks) { + if (!alloc || VIR_ALLOC_N(a_type->masks, cache + 1) < 0) + return NULL; + a_type->nmasks = cache + 1; + } else if (a_type->nmasks <= cache) { + if (!alloc || VIR_EXPAND_N(a_type->masks, a_type->nmasks, + cache - a_type->nmasks + 1) < 0) + return NULL; + } + + return a_type->masks + cache; +} + +static unsigned long long * +virResctrlAllocFindSize(virResctrlAllocPtr *resctrl, + unsigned int level, + virCacheType type, + unsigned int cache, + bool alloc) +{ + virResctrlAllocPerTypePtr a_type = virResctrlAllocFindType(resctrl, level, + type, alloc); + + if (!a_type) + return NULL; + + if (!a_type->sizes) { + if (!alloc || VIR_ALLOC_N(a_type->sizes, cache + 1) < 0) + return NULL; + a_type->nsizes = cache + 1; + } else if (a_type->nsizes <= cache) { + if (!alloc || VIR_EXPAND_N(a_type->sizes, a_type->nsizes, + cache - a_type->nsizes + 1) < 0) + return NULL; + } + + if (!a_type->sizes[cache]) { + if (!alloc || VIR_ALLOC(a_type->sizes[cache]) < 0) + return NULL; + } + + return a_type->sizes[cache]; +} + +int +virResctrlAllocUpdateMask(virResctrlAllocPtr *resctrl, + unsigned int level, + virCacheType type, + unsigned int cache, + virBitmapPtr mask) +{ + virBitmapPtr *found = virResctrlAllocFindMask(resctrl, level, type, cache, + true); + + if (!found) + return -1; + + virBitmapFree(*found); + + *found = virBitmapNew(virBitmapSize(mask)); + if (!*found) + return -1; + + return virBitmapCopy(*found, mask); +} + +int +virResctrlAllocUpdateSize(virResctrlAllocPtr *resctrl, + unsigned int level, + virCacheType type, + unsigned int cache, + unsigned long long size) +{ + unsigned long long *found = virResctrlAllocFindSize(resctrl, level, type, + cache, true); + + if (!found) + return -1; + + *found = size; + return 0; +} + +static bool +virResctrlAllocCheckCollision(virResctrlAllocPtr a, + unsigned int level, + virCacheType type, + unsigned int cache) +{ + /* If there is an allocation for type 'both', there can be no other + * allocation for the same cache */ + if (virResctrlAllocFindSize(&a, level, VIR_CACHE_TYPE_BOTH, cache, false)) + return true; + + if (type == VIR_CACHE_TYPE_BOTH) { + if (virResctrlAllocFindSize(&a, level, VIR_CACHE_TYPE_CODE, cache, false)) + return true; + if (virResctrlAllocFindSize(&a, level, VIR_CACHE_TYPE_DATA, cache, false)) + return true; + } + + /* And never two allocations for the same type */ + if (virResctrlAllocFindSize(&a, level, type, cache, false)) + return true; + + return false; +} + +int +virResctrlAllocSetSize(virResctrlAllocPtr *resctrl, + unsigned int level, + virCacheType type, + unsigned int cache, + unsigned long long size) +{ + if (virResctrlAllocCheckCollision(*resctrl, level, type, cache)) { + virReportError(VIR_ERR_XML_ERROR, + _("Colliding cache allocations for cache " + "level '%u' id '%u', type '%s'"), + level, cache, virCacheTypeToString(type)); + return -1; + } + + return virResctrlAllocUpdateSize(resctrl, level, type, cache, size); +} + +int +virResctrlAllocForeachSize(virResctrlAllocPtr resctrl, + virResctrlAllocForeachSizeCallback cb, + void *opaque) +{ + int ret = 0; + unsigned int level = 0; + unsigned int type = 0; + unsigned int cache = 0; + + if (!resctrl) + return 0; + + for (level = 0; level < resctrl->nlevels; level++) { + virResctrlAllocPerLevelPtr a_level = resctrl->levels[level]; + + if (!a_level) + continue; + + for (type = 0; type < VIR_CACHE_TYPE_LAST; type++) { + virResctrlAllocPerTypePtr a_type = a_level->types[type]; + + if (!a_type) + continue; + + for (cache = 0; cache < a_type->nsizes; cache++) { + unsigned long long *size = a_type->sizes[cache]; + + if (!size) + continue; + + ret = cb(level, type, cache, *size, opaque); + if (ret < 0) + return ret; + } + } + } + + return 0; +} + +int +virResctrlAllocSetID(virResctrlAllocPtr alloc, + const char *id) +{ + if (!id) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Resctrl allocation id cannot be NULL")); + return -1; + } + + return VIR_STRDUP(alloc->id, id); +} + +char * +virResctrlAllocFormat(virResctrlAllocPtr resctrl) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + unsigned int level = 0; + unsigned int type = 0; + unsigned int cache = 0; + + if (!resctrl) + return NULL; + + for (level = 0; level < resctrl->nlevels; level++) { + virResctrlAllocPerLevelPtr a_level = resctrl->levels[level]; + + if (!a_level) + continue; + + for (type = 0; type < VIR_CACHE_TYPE_LAST; type++) { + virResctrlAllocPerTypePtr a_type = a_level->types[type]; + + if (!a_type) + continue; + + virBufferAsprintf(&buf, "L%u%s:", level, virResctrlTypeToString(type)); + + for (cache = 0; cache < a_type->nmasks; cache++) { + virBitmapPtr mask = a_type->masks[cache]; + char *mask_str = NULL; + + if (!mask) + continue; + + mask_str = virBitmapToString(mask, false, true); + if (!mask_str) { + virBufferFreeAndReset(&buf); + return NULL; + } + + virBufferAsprintf(&buf, "%u=%s;", cache, mask_str); + } + + virBufferTrim(&buf, ";", 1); + virBufferAddChar(&buf, '\n'); + } + } + + virBufferCheckError(&buf); + return virBufferContentAndReset(&buf); +} + +static int +virResctrlAllocParseProcessCache(virResctrlAllocPtr *resctrl, + unsigned int level, + virCacheType type, + char *cache) +{ + char *tmp = strchr(cache, '='); + unsigned int cache_id = 0; + virBitmapPtr mask = NULL; + int ret = -1; + + if (!tmp) + return 0; + + *tmp = '\0'; + tmp++; + + if (virStrToLong_uip(cache, NULL, 10, &cache_id) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid cache id '%s'"), cache); + return -1; + } + + mask = virBitmapNewString(tmp); + if (!mask) + return -1; + + if (virResctrlAllocUpdateMask(resctrl, level, type, cache_id, mask) < 0) + goto cleanup; + + ret = 0; + cleanup: + virBitmapFree(mask); + return ret; +} + +static int +virResctrlAllocParseProcessLine(virResctrlAllocPtr *resctrl, + char *line) +{ + char **caches = NULL; + char *tmp = NULL; + unsigned int level = 0; + int type = -1; + size_t ncaches = 0; + size_t i = 0; + int ret = -1; + + /* Skip lines that don't concern caches, e.g. MB: etc. */ + if (line[0] != 'L') + return 0; + + /* And lines that we can't parse too */ + tmp = strchr(line, ':'); + if (!tmp) + return 0; + + *tmp = '\0'; + tmp++; + + if (virStrToLong_uip(line + 1, &line, 10, &level) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse resctrl schema level '%s'"), + line + 1); + return -1; + } + + type = virResctrlTypeFromString(line); + if (type < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse resctrl schema level '%s'"), + line + 1); + return -1; + } + + caches = virStringSplitCount(tmp, ";", 0, &ncaches); + if (!caches) + return 0; + + for (i = 0; i < ncaches; i++) { + if (virResctrlAllocParseProcessCache(resctrl, level, type, caches[i]) < 0) + goto cleanup; + } + + ret = 0; + cleanup: + virStringListFree(caches); + return ret; +} + +static int +virResctrlAllocParse(virResctrlAllocPtr *alloc, + const char *schemata) +{ + virResctrlAllocPtr tmp = NULL; + char **lines = NULL; + size_t nlines = 0; + size_t i = 0; + int ret = -1; + + lines = virStringSplitCount(schemata, "\n", 0, &nlines); + for (i = 0; i < nlines; i++) { + if (virResctrlAllocParseProcessLine(&tmp, lines[i]) < 0) + goto cleanup; + } + + *alloc = tmp; + tmp = NULL; + ret = 0; + cleanup: + virStringListFree(lines); + virObjectUnref(tmp); + return ret; +} + +static void +virResctrlAllocSubtractPerType(virResctrlAllocPerTypePtr a, + virResctrlAllocPerTypePtr b) +{ + size_t i = 0; + + if (!a || !b) + return; + + for (i = 0; i < a->nmasks && i < b->nmasks; ++i) { + if (a->masks[i] && b->masks[i]) + virBitmapSubtract(a->masks[i], b->masks[i]); + } +} + +static void +virResctrlAllocSubtract(virResctrlAllocPtr a, + virResctrlAllocPtr b) +{ + size_t i = 0; + size_t j = 0; + + if (!b) + return; + + for (i = 0; i < a->nlevels && b->nlevels; ++i) { + if (a->levels[i] && b->levels[i]) { + /* Here we rely on all the system allocations to use the same types. + * We kind of _hope_ it's the case. If this is left here until the + * review and someone finds it, then suggest only removing this last + * sentence. */ + for (j = 0; j < VIR_CACHE_TYPE_LAST; j++) { + virResctrlAllocSubtractPerType(a->levels[i]->types[j], + b->levels[i]->types[j]); + } + } + } +} + +virResctrlAllocPtr +virResctrlAllocNewFromInfo(virResctrlInfoPtr info) +{ + size_t i = 0; + size_t j = 0; + size_t k = 0; + virResctrlAllocPtr ret = NULL; + virBitmapPtr mask = NULL; + + for (i = 0; i < info->nlevels; i++) { + virResctrlInfoPerLevelPtr i_level = info->levels[i]; + + if (!i_level) + continue; + + for (j = 0; j < VIR_CACHE_TYPE_LAST; j++) { + virResctrlInfoPerTypePtr i_type = i_level->types[j]; + + if (!i_type) + continue; + + virBitmapFree(mask); + mask = virBitmapNew(i_type->bits); + if (!mask) + goto error; + virBitmapSetAll(mask); + + for (k = 0; k <= i_type->max_cache_id; k++) { + if (virResctrlAllocUpdateMask(&ret, i, j, k, mask) < 0) + goto error; + } + } + } + + cleanup: + virBitmapFree(mask); + return ret; + error: + virObjectUnref(ret); + ret = NULL; + goto cleanup; +} + +virResctrlAllocPtr +virResctrlAllocGetFree(virResctrlInfoPtr resctrl) +{ + virResctrlAllocPtr ret = NULL; + virResctrlAllocPtr alloc = NULL; + virBitmapPtr mask = NULL; + struct dirent *ent = NULL; + DIR *dirp = NULL; + char *schemata = NULL; + int rv = -1; + + ret = virResctrlAllocNewFromInfo(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; + } + + if (virResctrlAllocParse(&alloc, schemata) < 0) + goto error; + if (!alloc) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("No schemata for default resctrlfs group")); + goto error; + } + virResctrlAllocSubtract(ret, alloc); + + if (virDirOpen(&dirp, SYSFS_RESCTRL_PATH) < 0) + goto error; + + while ((rv = virDirRead(dirp, &ent, SYSFS_RESCTRL_PATH)) > 0) { + if (ent->d_type != DT_DIR) + continue; + + if (STREQ(ent->d_name, "info")) + continue; + + VIR_FREE(schemata); + if (virFileReadValueString(&schemata, + SYSFS_RESCTRL_PATH + "/%s/schemata", + ent->d_name) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not read schemata file for group %s"), + ent->d_name); + goto error; + } + + virObjectUnref(alloc); + alloc = NULL; + if (virResctrlAllocParse(&alloc, schemata) < 0) + goto error; + + virResctrlAllocSubtract(ret, alloc); + + VIR_FREE(schemata); + } + if (rv < 0) + goto error; + + cleanup: + virObjectUnref(alloc); + VIR_DIR_CLOSE(dirp); + VIR_FREE(schemata); + virBitmapFree(mask); + return ret; + + error: + virObjectUnref(ret); + ret = NULL; + goto cleanup; +} + +int +virResctrlAllocMasksAssign(virResctrlInfoPtr r_info, + virResctrlAllocPtr alloc, + void **save_ptr) +{ + int ret = -1; + unsigned int level = 0; + virResctrlAllocPtr alloc_free = NULL; + + if (!r_info) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Resource control is not supported on this host")); + return -1; + } + + if (!save_ptr) { + alloc_free = virResctrlAllocGetFree(r_info); + } else { + if (!*save_ptr) + *save_ptr = virResctrlAllocGetFree(r_info); + + alloc_free = *save_ptr; + } + + if (!alloc_free) + return -1; + + for (level = 0; level < alloc->nlevels; level++) { + virResctrlAllocPerLevelPtr a_level = alloc->levels[level]; + virResctrlAllocPerLevelPtr f_level = NULL; + unsigned int type = 0; + + if (!a_level) + continue; + + if (level < alloc_free->nlevels) + f_level = alloc_free->levels[level]; + + if (!f_level) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Cache level %d does not support tuning"), + level); + goto cleanup; + } + + for (type = 0; type < VIR_CACHE_TYPE_LAST; type++) { + virResctrlAllocPerTypePtr a_type = a_level->types[type]; + virResctrlAllocPerTypePtr f_type = f_level->types[type]; + unsigned int cache = 0; + + if (!a_type) + continue; + + if (!f_type) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Cache level %d does not support tuning for " + "scope type '%s'"), + level, virCacheTypeToString(type)); + goto cleanup; + } + + for (cache = 0; cache < a_type->nsizes; cache++) { + unsigned long long *size = a_type->sizes[cache]; + virBitmapPtr a_mask = NULL; + virBitmapPtr f_mask = f_type->masks[cache]; + virResctrlInfoPerLevelPtr i_level = r_info->levels[level]; + virResctrlInfoPerTypePtr i_type = i_level->types[type]; + unsigned long long granularity; + unsigned long long need_bits; + size_t i = 0; + ssize_t pos = -1; + ssize_t last_bits = 0; + ssize_t last_pos = -1; + + if (!size) + continue; + + if (cache >= f_type->nmasks) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Cache with id %u does not exists for level %d"), + cache, level); + goto cleanup; + } + + f_mask = f_type->masks[cache]; + if (!f_mask) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Cache level %d id %u does not support tuning for " + "scope type '%s'"), + level, cache, virCacheTypeToString(type)); + goto cleanup; + } + + granularity = i_type->size / i_type->bits; + need_bits = *size / granularity; + + if (*size % granularity) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Cache allocation of size %llu is not " + "divisible by granularity %llu"), + *size, granularity); + goto cleanup; + } + + if (need_bits < i_type->min_cbm_bits) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Cache allocation of size %llu is smaller " + "than the minimum allowed allocation %llu"), + *size, granularity * i_type->min_cbm_bits); + goto cleanup; + } + + while ((pos = virBitmapNextSetBit(f_mask, pos)) >= 0) { + ssize_t pos_clear = virBitmapNextClearBit(f_mask, pos); + ssize_t bits; + + if (pos_clear < 0) + pos_clear = virBitmapSize(f_mask); + + bits = pos_clear - pos; + + /* Not enough bits, move on and skip all of them */ + if (bits < need_bits) { + pos = pos_clear; + continue; + } + + /* This fits perfectly */ + if (bits == need_bits) { + last_pos = pos; + break; + } + + /* Remember the smaller region if we already found on before */ + if (last_pos < 0 || (last_bits && bits < last_bits)) { + last_bits = bits; + last_pos = pos; + } + + pos = pos_clear; + } + + if (last_pos < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Not enough room for allocation of " + "%llu bytes for level %u cache %u " + "scope type '%s'"), + *size, level, cache, + virCacheTypeToString(type)); + goto cleanup; + } + + a_mask = virBitmapNew(i_type->bits); + for (i = last_pos; i < last_pos + need_bits; i++) { + ignore_value(virBitmapSetBit(a_mask, i)); + ignore_value(virBitmapClearBit(f_mask, i)); + } + + if (a_type->nmasks <= cache) { + if (VIR_EXPAND_N(a_type->masks, a_type->nmasks, + cache - a_type->nmasks + 1) < 0) { + virBitmapFree(a_mask); + goto cleanup; + } + } + a_type->masks[cache] = a_mask; + } + } + } + + ret = 0; + cleanup: + if (!save_ptr) + virObjectUnref(alloc_free); + return ret; +} + +/* This checks if the directory for the alloc exists. If not it tries to create + * it and apply appropriate alloc settings. */ +int +virResctrlAllocCreate(virResctrlInfoPtr r_info, + virResctrlAllocPtr alloc, + const char *drivername, + const char *machinename) +{ + char *alloc_path = NULL; + char *schemata_path = NULL; + bool dir_created = false; + char *alloc_str = NULL; + int ret = -1; + int lockfd = -1; + + if (!alloc) + return 0; + + if (!alloc->path && + virAsprintf(&alloc->path, "%s/%s-%s-%s", + SYSFS_RESCTRL_PATH, drivername, machinename, alloc->id) < 0) + return -1; + + /* Check if this allocation was already created */ + if (virFileIsDir(alloc->path)) { + VIR_FREE(alloc_path); + return 0; + } + + if (virFileExists(alloc->path)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Path '%s' for resctrl allocation exists and is not a " + "directory"), alloc->path); + goto cleanup; + } + + lockfd = virResctrlLockWrite(); + if (lockfd < 0) + goto cleanup; + + if (virResctrlAllocMasksAssign(r_info, alloc, NULL) < 0) + goto cleanup; + + alloc_str = virResctrlAllocFormat(alloc); + if (!alloc_str) + return -1; + + if (virAsprintf(&schemata_path, "%s/schemata", alloc->path) < 0) + goto cleanup; + + if (virFileMakePath(alloc->path) < 0) { + virReportSystemError(errno, + _("Cannot create resctrl directory '%s'"), + alloc->path); + goto cleanup; + } + dir_created = true; + + if (virFileWriteStr(schemata_path, alloc_str, 0) < 0) { + virReportSystemError(errno, + _("Cannot write into schemata file '%s'"), + schemata_path); + goto cleanup; + } + + ret = 0; + cleanup: + if (ret < 0 && dir_created) + rmdir(alloc->path); + virResctrlUnlock(lockfd); + VIR_FREE(alloc_str); + VIR_FREE(alloc_path); + VIR_FREE(schemata_path); + return ret; +} + +int +virResctrlAllocAddPID(virResctrlAllocPtr alloc, + pid_t pid) +{ + char *tasks = NULL; + char *pidstr = NULL; + int ret = 0; + + if (!alloc->path) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot add pid to non-existing resctrl allocation")); + return -1; + } + + if (virAsprintf(&tasks, "%s/tasks", alloc->path) < 0) + return -1; + + if (virAsprintf(&pidstr, "%lld", (long long int) pid) < 0) + goto cleanup; + + if (virFileWriteStr(tasks, pidstr, 0) < 0) { + virReportSystemError(errno, + _("Cannot write pid in tasks file '%s'"), + tasks); + goto cleanup; + } + + ret = 0; + cleanup: + VIR_FREE(tasks); + VIR_FREE(pidstr); + return ret; +} + +int +virResctrlAllocRemove(virResctrlAllocPtr alloc) +{ + int ret = 0; + + if (!alloc->path) + return 0; + + VIR_DEBUG("Removing resctrl allocation %s", alloc->path); + if (rmdir(alloc->path) != 0 && errno != ENOENT) { + ret = -errno; + VIR_ERROR(_("Unable to remove %s (%d)"), alloc->path, errno); + } + + return ret; +} diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h index c4df88f23c3a..b233eca41c03 100644 --- a/src/util/virresctrl.h +++ b/src/util/virresctrl.h @@ -62,4 +62,63 @@ virResctrlInfoGetCache(virResctrlInfoPtr resctrl, size_t *ncontrols, virResctrlInfoPerCachePtr **controls); +/* Alloc-related things */ +typedef struct _virResctrlAlloc virResctrlAlloc; +typedef virResctrlAlloc *virResctrlAllocPtr; + +typedef int virResctrlAllocForeachSizeCallback(unsigned int level, + virCacheType type, + unsigned int cache, + unsigned long long size, + void *opaque); + +virResctrlAllocPtr +virResctrlAllocNewFromInfo(virResctrlInfoPtr info); + +int +virResctrlAllocUpdateSize(virResctrlAllocPtr *resctrl, + unsigned int level, + virCacheType type, + unsigned int cache, + unsigned long long size); + +int +virResctrlAllocUpdateMask(virResctrlAllocPtr *resctrl, + unsigned int level, + virCacheType type, + unsigned int cache, + virBitmapPtr mask); + +int +virResctrlAllocSetSize(virResctrlAllocPtr *resctrl, + unsigned int level, + virCacheType type, + unsigned int cache, + unsigned long long size); + +int +virResctrlAllocForeachSize(virResctrlAllocPtr resctrl, + virResctrlAllocForeachSizeCallback cb, + void *opaque); + +int +virResctrlAllocSetID(virResctrlAllocPtr alloc, + const char *id); + +char * +virResctrlAllocFormat(virResctrlAllocPtr alloc); + +int +virResctrlAllocCreate(virResctrlInfoPtr r_info, + virResctrlAllocPtr alloc, + const char *drivername, + const char *machinename); + +int +virResctrlAllocAddPID(virResctrlAllocPtr alloc, + pid_t pid); + +int +virResctrlAllocRemove(virResctrlAllocPtr alloc); + #endif /* __VIR_RESCTRL_H__ */ diff --git a/src/util/virresctrlpriv.h b/src/util/virresctrlpriv.h new file mode 100644 index 000000000000..4255ad496302 --- /dev/null +++ b/src/util/virresctrlpriv.h @@ -0,0 +1,32 @@ +/* + * virresctrlpriv.h: + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>;. + */ + +#ifndef __VIR_RESCTRL_PRIV_H__ +# define __VIR_RESCTRL_PRIV_H__ + +# include "virresctrl.h" + +virResctrlAllocPtr +virResctrlAllocGetFree(virResctrlInfoPtr resctrl); + +int +virResctrlAllocMasksAssign(virResctrlInfoPtr r_info, + virResctrlAllocPtr alloc, + void **save_ptr); + +#endif /* __VIR_RESCTRL_PRIV_H__ */ -- 2.15.0

On Tue, Nov 14, 2017 at 06:52:48PM -0500, John Ferlan wrote: Fixed I had this as one big gross ball of pus that I had to split into at least bit smaller chunks. I'm not sure I could've split this one even more. Maybe I could, but it didn't occur to me, mainly after renaming, rebasing and splitting if for non-trivial amount of time already. I'll push parts of it later, but I'm keeping this as a branch 'catwip' [1] on my github [2] where you can always get the code from in case it has conflicts with the newest master. [1] https://github.com/nertpinx/libvirt/tree/catwip [2] https://github.com/nertpinx/libvirt.git Yeah, fixed now good point could, maybe I will, but it should not be freed separately anyway... Yes! =) I tried keeping this one more organized, two lines between groups of functions, one line between functions in the same group. But I can do two everywhere, the fact that I don't fully agree is irrelevant (unfortunately), but I'd rather get this in instead of arguing over the amount of whitespace =D Oh, missed this one. Yes, only local filesystem. Because that uses fcnlt(2) which is different lock which might not interactl with flock(2), so all programs working with resctrlfs must use the same type of lock. And resctrlfs should specifically use flock(2) according to the docs: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/D... Yeah, this can be removed. Actually clang will complain about it. I'll remove it, it's not that difficult to add it later on :D ret is pointless here. Only if the close failed, I figured we might as well try to safe the already broken, right? I can remove it if you want. It would not make any sense for it not to be true. I guess I was just angry with the Linux kernel interface for CAT when writing this. That was the case for most of the time I spent on this series. And some of the time I was not working at all. And sometimes when sleeping, too. Anyway, nothing bad happens if it's not true, maybe an allocation that is not purely exclusive will be created. Good point, making it static. Actually, there will be one, in tests, but there is circular dependency with the other function (AllocMasksAssign). The tests need both XML support and this file, XML support needs this file as well, but if we say this file needs tests then it's circular. And I wanted to split it a little bit so we don't get that huge files. see above did someone say "beer"? s/alloc_path/alloc->path/ =) Yes, where do you see that? The remove will be done in qemuProcessStop(). I can remove this one puny directory here, but proper cleanup needs to be done for all anyway. good point. Peter-style bug, I'll fix this. I'll rebase all of these with the fixes and push it on the github so that it's easier to work with. Thanks for the review.

[...] Well I stared at it a *long* time before coming to the conclusion that as odd looking as it is - it does what it's supposed to do in a unique way compared to other libvirt functions. While yes a bit ugly, it didn't feel too complex once I got the basic premise. I also kept thinking this whole sequence relies on the "original caller" (e.g. where &resctrl originally gets passed) to be sure on failure to Unref - tracing back to that was a challenge. Thinking about these functions being called in the middle of some other code - I dunno. Still Like I pointed out - it would help *a lot* to document WTF is going on! Returning the "address of" some location based on array entry/offset does cause some concern - I was able to eventually convince myself it works... Although I must say I studied virResctrlAllocUpdateMask for quite a bit trying to determine whether what it claimed to do was actually being done. John [...]

On Tue, Nov 21, 2017 at 10:49:29AM +0100, Martin Kletzander wrote: I opened the patch in vim and configured to display TAB and there are two TABs, so please double check it in your editor :). So the allocation happens in these public functions: * virResctrlAllocNewFromInfo() - There you know based on Info whether you need to allocate new object or not. This is probably the only case where the automatic allocation helps a little bit. * virResctrlAllocSetSize() - This is indirectly called from virDomainCachetuneDefParse() where you know if there is any cache element to parse so you can allocate new object if there is anything to set. So there shouldn't be any need to have a function that will check the allocation. Otherwise I wouldn't have suggested this modification. I think that you can decide to merge the functions or not based on the resulting code after it is rewritten. It's not requirement, more like suggestion based on what I was imagine how the new code could look like. Right, if it makes sense to use it as standalone function the error message should probably stay here. I understand that it saves current state so you don't have to parse it again but from the patch series it looks like it is used only for tests. So the question about speed was only related to tests where you use this optimization. Pavel

Short, sweet, and to the point. Looks like this patch does nothing, eh? :-)... Of course you realized that based on my patch 15 review. After reading the cover letter I was expecting a least a few sentences here, but nothing, sigh. On 11/13/2017 03:50 AM, Martin Kletzander wrote: Perhaps this goes against norms, but I think that <span> should be placed on the "cachetune" above to signify it's for all of these. If some day later a new element or attribute is created it then gets it's one <span> for the since of that specific version. The required attribute... s/what/to which/ A vCPU can only be a member of one <code>cachetune</code> element allocation. Each <code>vcputune</code> element must have at least one <code>cache</code> subelement. The <code>cache</code> subelement must have Also I like it better when the subelements are in some sort of list format as it's easier to read than a long sentence. So picking up from where I left off before: the following attributes: <dl> <dt><code>level</code></dt> <dd>Host cache level from which to allocate.</dd> <dt><code>id</code></dt> <dd>Host cache id from which to allocate.</dd> <dt><code>type</code></dt> <dd>Whether to allocate <code>code</code>, <code>data</code>, or <code>both</code> regions.</dd> <dt><code>size</code></dt> <dd>The size of the region to allocate. The value by default is in bytes, but the <code>unit</code> attribute can be used to scale the value.</dd> <dt><code>unit</code></dt> <dd>Scaled memory value such as KiB, MiB, GiB, or TiB described in the <code>memory</code> element for <a href="#elementsMemoryAllocation">Memory Allocation</a>.</dd> </dl> BTW: Whether you expound upon what data, code, and both are is up to you. I have a good idea what that means, but it's not necessarily well described as to the usefulness regarding why to select one or the other. 'id' (and only because later on you added '' around id in a different message ;-) Just cachetune?? (consistency w/ other messages - IDC which way, but similar point in this function and the folowing one) Maybe it's just me, but the message just doesn't flow well, consider: "Invalid cachetune attribute 'id' value '%s'" It's not all that important though, but could be used in subsequent messages too. 'level' 'type' Two blank lines. Here again w/ the cachetune/cache The brevity of the format is something all formatting functions should aspire to become! Well that's lengthy! Still that last sentence makes me wonder if that "limited number" is known or is it random and thus if enough were created there would be random failures... This probably should go in some earlier patch (if it's not already pushed ;-)). I think the fixup's are essentially simple. I'm fine with the naming scheme - although maybe someone else who has been thinking about this longer, but didn't jump in to review would have a different opinion ;-) Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John [...]

This patch modifies some not yet used test data so that the adding a test using this data is a clean patch and not an addition of huge file with some adjustments in small files that will be hidden in the middle of that commit. These changes include: - Add system dir in vircaps2xmldata/linux-caches Back when data for systems with resctrl support were added they had the /sys/fs/system directory put into a system/ subdir of the test and /sys/fs/resctrl in a resctrl/ subdir of that test. However, if we also want a negative test for the resctrl (requesting allocation on a system that does not support resctrl), we need one a test case with any sensible (with cache info) system/ subdir and no resctrl/ one. Easiest way is to add a system -> . symlink into existing test case. - Change linux-resctrl's schemata for default group That way we can fit some allocation in. - Remove one cache from resctrl-skx's schemata and make some room for allocations That system already has only one cache, so that file was wrong anyway. We have a version with 2 caches already (linux-resctrl-skx-twocaches), so this will also add variety to future tests. - Add some empty allocation for resctrl-skx Just to have slightly more coverage and variety. We can be sure nothing bad happens if such allocation exists in case we have that in the tests. Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- tests/vircaps2xmldata/linux-caches/system | 1 + tests/vircaps2xmldata/linux-resctrl-skx/resctrl/empty/schemata | 0 tests/vircaps2xmldata/linux-resctrl-skx/resctrl/schemata | 2 +- tests/vircaps2xmldata/linux-resctrl/resctrl/schemata | 2 +- 4 files changed, 3 insertions(+), 2 deletions(-) create mode 120000 tests/vircaps2xmldata/linux-caches/system create mode 100644 tests/vircaps2xmldata/linux-resctrl-skx/resctrl/empty/schemata diff --git a/tests/vircaps2xmldata/linux-caches/system b/tests/vircaps2xmldata/linux-caches/system new file mode 120000 index 000000000000..945c9b46d684 --- /dev/null +++ b/tests/vircaps2xmldata/linux-caches/system @@ -0,0 +1 @@ +. \ No newline at end of file diff --git a/tests/vircaps2xmldata/linux-resctrl-skx/resctrl/empty/schemata b/tests/vircaps2xmldata/linux-resctrl-skx/resctrl/empty/schemata new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/tests/vircaps2xmldata/linux-resctrl-skx/resctrl/schemata b/tests/vircaps2xmldata/linux-resctrl-skx/resctrl/schemata index 4a4913735386..191081f1db47 100644 --- a/tests/vircaps2xmldata/linux-resctrl-skx/resctrl/schemata +++ b/tests/vircaps2xmldata/linux-resctrl-skx/resctrl/schemata @@ -1 +1 @@ -L3:0=7ff;1=7ff +L3:0=0f0 diff --git a/tests/vircaps2xmldata/linux-resctrl/resctrl/schemata b/tests/vircaps2xmldata/linux-resctrl/resctrl/schemata index 9b47d25fc72e..c1a765f1e0a7 100644 --- a/tests/vircaps2xmldata/linux-resctrl/resctrl/schemata +++ b/tests/vircaps2xmldata/linux-resctrl/resctrl/schemata @@ -1 +1 @@ -L3:0=1ffff;1=1ffff +L3:0=1ff00;1=1ff0f -- 2.15.0

This test has 2 different uses: 1) DO_TEST_FREE initializes capabilities from vircaps2xmldata (since it exists there already) and then requests list of free bitmaps (all unallocated space) from virresctrl.c 2) DO_TEST_ALLOC takes capabilities from vircaps2xmldata, and uses resctrl info to request an allocation from virresctrl.c for a VM from genericxml2xmlindata. Desirable outputs are saved in virresctrldata. Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- tests/Makefile.am | 8 +- .../resctrl--cachetune/vcpus-0-1.alloc | 1 + .../resctrl--cachetune/vcpus-3.alloc | 1 + .../resctrl-cdp--cachetune-cdp/vcpus-0-1.alloc | 2 + .../resctrl-cdp--cachetune-cdp/vcpus-2.alloc | 1 + .../resctrl-cdp--cachetune-cdp/vcpus-3.alloc | 1 + tests/virresctrldata/resctrl-cdp.schemata | 2 + .../virresctrldata/resctrl-skx-twocaches.schemata | 1 + tests/virresctrldata/resctrl-skx.schemata | 1 + tests/virresctrldata/resctrl.schemata | 1 + tests/virresctrltest.c | 277 +++++++++++++++++++++ 11 files changed, 295 insertions(+), 1 deletion(-) create mode 100644 tests/virresctrldata/resctrl--cachetune/vcpus-0-1.alloc create mode 100644 tests/virresctrldata/resctrl--cachetune/vcpus-3.alloc create mode 100644 tests/virresctrldata/resctrl-cdp--cachetune-cdp/vcpus-0-1.alloc create mode 100644 tests/virresctrldata/resctrl-cdp--cachetune-cdp/vcpus-2.alloc create mode 100644 tests/virresctrldata/resctrl-cdp--cachetune-cdp/vcpus-3.alloc create mode 100644 tests/virresctrldata/resctrl-cdp.schemata create mode 100644 tests/virresctrldata/resctrl-skx-twocaches.schemata create mode 100644 tests/virresctrldata/resctrl-skx.schemata create mode 100644 tests/virresctrldata/resctrl.schemata create mode 100644 tests/virresctrltest.c diff --git a/tests/Makefile.am b/tests/Makefile.am index effa259e3a45..04695c0ad250 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -231,6 +231,7 @@ if WITH_LINUX test_programs += fchosttest test_programs += scsihosttest test_programs += vircaps2xmltest +test_programs += virresctrltest test_libraries += virusbmock.la \ virnetdevbandwidthmock.la \ virnumamock.la \ @@ -1172,8 +1173,13 @@ virnumamock_la_CFLAGS = $(AM_CFLAGS) virnumamock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS) virnumamock_la_LIBADD = $(MOCKLIBS_LIBS) +virresctrltest_SOURCES = \ + virresctrltest.c testutils.h testutils.c virfilewrapper.h virfilewrapper.c +virresctrltest_LDADD = $(LDADDS) + else ! WITH_LINUX -EXTRA_DIST += vircaps2xmltest.c virnumamock.c virfilewrapper.c virfilewrapper.h +EXTRA_DIST += vircaps2xmltest.c virnumamock.c virfilewrapper.c \ + virfilewrapper.h virresctrltest.c endif ! WITH_LINUX if WITH_NSS diff --git a/tests/virresctrldata/resctrl--cachetune/vcpus-0-1.alloc b/tests/virresctrldata/resctrl--cachetune/vcpus-0-1.alloc new file mode 100644 index 000000000000..6f48dd99d5a1 --- /dev/null +++ b/tests/virresctrldata/resctrl--cachetune/vcpus-0-1.alloc @@ -0,0 +1 @@ +L3:0=0000f;1=000f0 diff --git a/tests/virresctrldata/resctrl--cachetune/vcpus-3.alloc b/tests/virresctrldata/resctrl--cachetune/vcpus-3.alloc new file mode 100644 index 000000000000..b47a36fecc8a --- /dev/null +++ b/tests/virresctrldata/resctrl--cachetune/vcpus-3.alloc @@ -0,0 +1 @@ +L3:0=000f0 diff --git a/tests/virresctrldata/resctrl-cdp--cachetune-cdp/vcpus-0-1.alloc b/tests/virresctrldata/resctrl-cdp--cachetune-cdp/vcpus-0-1.alloc new file mode 100644 index 000000000000..44886a2cfe38 --- /dev/null +++ b/tests/virresctrldata/resctrl-cdp--cachetune-cdp/vcpus-0-1.alloc @@ -0,0 +1,2 @@ +L3CODE:0=00ffc +L3DATA:1=0001f diff --git a/tests/virresctrldata/resctrl-cdp--cachetune-cdp/vcpus-2.alloc b/tests/virresctrldata/resctrl-cdp--cachetune-cdp/vcpus-2.alloc new file mode 100644 index 000000000000..4225b0a36167 --- /dev/null +++ b/tests/virresctrldata/resctrl-cdp--cachetune-cdp/vcpus-2.alloc @@ -0,0 +1 @@ +L3CODE:1=0ff00 diff --git a/tests/virresctrldata/resctrl-cdp--cachetune-cdp/vcpus-3.alloc b/tests/virresctrldata/resctrl-cdp--cachetune-cdp/vcpus-3.alloc new file mode 100644 index 000000000000..10be6844bd38 --- /dev/null +++ b/tests/virresctrldata/resctrl-cdp--cachetune-cdp/vcpus-3.alloc @@ -0,0 +1 @@ +L3DATA:1=03fe0 diff --git a/tests/virresctrldata/resctrl-cdp.schemata b/tests/virresctrldata/resctrl-cdp.schemata new file mode 100644 index 000000000000..2897e2afa603 --- /dev/null +++ b/tests/virresctrldata/resctrl-cdp.schemata @@ -0,0 +1,2 @@ +L3CODE:0=00ffc;1=0ff00 +L3DATA:0=00000;1=03fff diff --git a/tests/virresctrldata/resctrl-skx-twocaches.schemata b/tests/virresctrldata/resctrl-skx-twocaches.schemata new file mode 100644 index 000000000000..86b3801a04c2 --- /dev/null +++ b/tests/virresctrldata/resctrl-skx-twocaches.schemata @@ -0,0 +1 @@ +L3:0=001;1=400 diff --git a/tests/virresctrldata/resctrl-skx.schemata b/tests/virresctrldata/resctrl-skx.schemata new file mode 100644 index 000000000000..5e8b0d636277 --- /dev/null +++ b/tests/virresctrldata/resctrl-skx.schemata @@ -0,0 +1 @@ +L3:0=70f diff --git a/tests/virresctrldata/resctrl.schemata b/tests/virresctrldata/resctrl.schemata new file mode 100644 index 000000000000..fa980e58c9dd --- /dev/null +++ b/tests/virresctrldata/resctrl.schemata @@ -0,0 +1 @@ +L3:0=000ff;1=000f0 diff --git a/tests/virresctrltest.c b/tests/virresctrltest.c new file mode 100644 index 000000000000..e84d9ff94299 --- /dev/null +++ b/tests/virresctrltest.c @@ -0,0 +1,277 @@ +#include <config.h> +#include <stdlib.h> + +#include "testutils.h" +#include "virfilewrapper.h" +#include "virresctrlpriv.h" + + +#define VIR_FROM_THIS VIR_FROM_NONE + +struct virResctrlData { + const char *filename; + bool fail; + const char *test_name; +}; + + +static int +test_virResctrlGetFree(const void *opaque) +{ + struct virResctrlData *data = (struct virResctrlData *) opaque; + char *system_dir = NULL; + char *resctrl_dir = NULL; + int ret = -1; + virResctrlAllocPtr alloc = NULL; + char *schemata_str = NULL; + char *schemata_file; + virCapsPtr caps = NULL; + + if (virAsprintf(&system_dir, "%s/vircaps2xmldata/linux-%s/system", + abs_srcdir, data->filename) < 0) + goto cleanup; + + if (virAsprintf(&resctrl_dir, "%s/vircaps2xmldata/linux-%s/resctrl", + abs_srcdir, data->filename) < 0) + goto cleanup; + + if (virAsprintf(&schemata_file, "%s/virresctrldata/%s.schemata", + abs_srcdir, data->filename) < 0) + goto cleanup; + + virFileWrapperAddPrefix("/sys/devices/system", system_dir); + virFileWrapperAddPrefix("/sys/fs/resctrl", resctrl_dir); + + caps = virCapabilitiesNew(VIR_ARCH_X86_64, false, false); + if (!caps || virCapabilitiesInitCaches(caps) < 0) { + fprintf(stderr, "Could not initialize capabilities"); + goto cleanup; + } + + alloc = virResctrlAllocGetFree(caps->host.resctrl); + + virFileWrapperClearPrefixes(); + + if (!alloc) { + if (data->fail) + ret = 0; + goto cleanup; + } else if (data->fail) { + VIR_TEST_DEBUG("Error expected but there wasn't any.\n"); + ret = -1; + goto cleanup; + } + + schemata_str = virResctrlAllocFormat(alloc); + + if (virTestCompareToFile(schemata_str, schemata_file) < 0) + goto cleanup; + + ret = 0; + cleanup: + virObjectUnref(caps); + virObjectUnref(alloc); + VIR_FREE(system_dir); + VIR_FREE(resctrl_dir); + VIR_FREE(schemata_str); + VIR_FREE(schemata_file); + return ret; +} + +static int +test_virResctrlGetAlloc(const void *opaque) +{ + struct virResctrlData *data = (struct virResctrlData *) opaque; + char *system_dir = NULL; + char *resctrl_dir = NULL; + int ret = -1; + int rv = 0; + virResctrlAllocPtr alloc = NULL; + char *schemata_str = NULL; + char *schemata_file = NULL; + char *domain_file = NULL; + char *vcpus = NULL; + virCapsPtr caps = NULL; + virDomainDefPtr def = NULL; + size_t i = 0; + virDomainXMLOptionPtr xmlopt = NULL; + void *save_ptr = NULL; + + if (virAsprintf(&system_dir, "%s/vircaps2xmldata/linux-%s/system", + abs_srcdir, data->filename) < 0) + goto cleanup; + + if (virAsprintf(&resctrl_dir, "%s/vircaps2xmldata/linux-%s/resctrl", + abs_srcdir, data->filename) < 0) + goto cleanup; + + if (virAsprintf(&domain_file, "%s/genericxml2xmlindata/generic-%s.xml", + abs_srcdir, data->test_name) < 0) + goto cleanup; + + virFileWrapperAddPrefix("/sys/devices/system", system_dir); + virFileWrapperAddPrefix("/sys/fs/resctrl", resctrl_dir); + + caps = virTestGenericCapsInit(); + if (!caps || virCapabilitiesInitCaches(caps) < 0) { + fprintf(stderr, "Could not initialize capabilities"); + goto cleanup; + } + + xmlopt = virTestGenericDomainXMLConfInit(); + if (!xmlopt) + goto cleanup; + + def = virDomainDefParseFile(domain_file, caps, xmlopt, NULL, + VIR_DOMAIN_DEF_PARSE_INACTIVE); + if (!def) + goto cleanup; + + for (i = 0; i < def->ncachetunes; i++) { + if (virResctrlAllocMasksAssign(caps->host.resctrl, + def->cachetunes[i]->alloc, + &save_ptr) < 0) { + rv = -1; + break; + } + } + + virFileWrapperClearPrefixes(); + + if (rv < 0) { + if (data->fail) + ret = 0; + goto cleanup; + } else if (data->fail) { + VIR_TEST_DEBUG("Error expected but there wasn't any.\n"); + goto cleanup; + } + + for (i = 0; i < def->ncachetunes; i++) { + VIR_FREE(vcpus); + vcpus = virBitmapFormat(def->cachetunes[i]->vcpus); + + if (!vcpus) + goto cleanup; + + VIR_FREE(schemata_file); + VIR_FREE(schemata_str); + + if (virAsprintf(&schemata_file, "%s/virresctrldata/%s--%s/vcpus-%s.alloc", + abs_srcdir, data->filename, data->test_name, vcpus) < 0) + goto cleanup; + + schemata_str = virResctrlAllocFormat(def->cachetunes[i]->alloc); + if (!schemata_str) + goto cleanup; + + if (virTestCompareToFile(schemata_str, schemata_file) < 0) + rv = -1; + } + + ret = rv; + cleanup: + virObjectUnref(save_ptr); + virObjectUnref(caps); + virObjectUnref(alloc); + virObjectUnref(xmlopt); + VIR_FREE(system_dir); + VIR_FREE(resctrl_dir); + VIR_FREE(schemata_str); + VIR_FREE(schemata_file); + VIR_FREE(domain_file); + VIR_FREE(vcpus); + virDomainDefFree(def); + return ret; +} + +static int +mymain(void) +{ + struct virResctrlData data = {0}; + int ret = 0; + +#define DO_TEST_FREE(_filename) \ + do { \ + data = (struct virResctrlData) { .filename = _filename }; \ + if (virTestRun("Free: " _filename, test_virResctrlGetFree, &data) < 0) \ + ret = -1; \ + } while (0) + +#define DO_TEST_ALLOC_FULL(_filename, _test_name, _fail) \ + do { \ + data = (struct virResctrlData) { .filename = _filename, \ + .test_name = _test_name, \ + .fail = _fail }; \ + if (virTestRun("Alloc: " _filename "+" _test_name, \ + test_virResctrlGetAlloc, &data) < 0) \ + ret = -1; \ + } while (0) + +#define DO_TEST_ALLOC(_filename, _test_name) \ + DO_TEST_ALLOC_FULL(_filename, _test_name, false) + +#define DO_TEST_ALLOC_FAIL(_filename, _test_name) \ + DO_TEST_ALLOC_FULL(_filename, _test_name, true) + + DO_TEST_FREE("resctrl"); + DO_TEST_FREE("resctrl-cdp"); + DO_TEST_FREE("resctrl-skx"); + DO_TEST_FREE("resctrl-skx-twocaches"); + + + /************* + * Tests with generic-cachetune.xml (decently sized allocations for 2 caches) + ***/ + + /* Simplest one, this one should fail because there is not resctrl support + * on the host */ + DO_TEST_ALLOC_FAIL("caches", "cachetune"); + + DO_TEST_ALLOC("resctrl", "cachetune"); + + /* This one should fail because there are allocations for two caches + * requested, but only one exists */ + DO_TEST_ALLOC_FAIL("resctrl-skx", "cachetune"); + + /* This one should fail because there is not enough room available */ + DO_TEST_ALLOC_FAIL("resctrl-skx-twocaches", "cachetune"); + + /* This one should fail because non-CDP allocation is requested on + * CDP-enabled system, support for this can be added in the future */ + DO_TEST_ALLOC_FAIL("resctrl-cdp", "cachetune"); + + /************* + * Tests with generic-cachetune-small.xml (small allocation for 1 cache) + ***/ + + /* This one should fail because the allocation size is smaller than minimum + * (although with the right granularity) */ + DO_TEST_ALLOC_FAIL("resctrl", "cachetune-small"); + + /* These two should fail because the allocations size is smaller than + * granularity */ + DO_TEST_ALLOC_FAIL("resctrl-skx", "cachetune-small"); + DO_TEST_ALLOC_FAIL("resctrl-skx-twocaches", "cachetune-small"); + + /* This one should fail because non-CDP allocation is requested on + * CDP-enabled system, support for this can be added in the future */ + DO_TEST_ALLOC_FAIL("resctrl-cdp", "cachetune-small"); + + /************* + * Tests with generic-cachetune-cdp.xml (decently sized allocations, + * separate for code and data) + ***/ + + /* These should fail because CDP allocation is requested on non-CDP + * system */ + DO_TEST_ALLOC_FAIL("resctrl", "cachetune-cdp"); + DO_TEST_ALLOC_FAIL("resctrl-skx", "cachetune-cdp"); + DO_TEST_ALLOC_FAIL("resctrl-skx-twocaches", "cachetune-cdp"); + + DO_TEST_ALLOC("resctrl-cdp", "cachetune-cdp"); + + return ret; +} + +VIR_TEST_MAIN(mymain) -- 2.15.0

Commit messages have either gone on a diet or the writer is on strike On 11/13/2017 03:50 AM, Martin Kletzander wrote: This seems reasonable too, Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John [...] Oh and here is Stop doing the Remove... Perhaps why earlier since I hadn't read something like this so perhaps why I asked about it before checking consumers of AllocRemove

On 11/13/2017 03:50 AM, Martin Kletzander wrote: Actually Cache Allocation Technology, right? using the Instead of "in some ways, but..." - let's just describe the basics that got implemented. The ability to define for vCPU(s) how CAT will be configured for the domain for the cache id, level, and size. I trust you can come up with something more complete without writing too much. Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John