$subj: s/virBitampString/virBitmapString/ On 11/13/2017 03:50 AM, Martin Kletzander wrote:

Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_capabilities.c | 4 ++-- src/util/virbitmap.c | 6 ++++-- src/util/virbitmap.h | 2 +- 3 files changed, 7 insertions(+), 5 deletions(-)

FWIW: For some reason 4/21 hasn't yet made it to the mail server near me. I don't know why, it's just lost or on vacation. The only comment I have on it is that the commit message "util: Rename virBitmapString tovirBitmapToString" needs a slight adjustment "...String to virBit..." I have one adjustment below regarding one arg per line in function... Consider this a: Reviewed-by: John Ferlan <jferlan@redhat.com> For this and 04/21 because I'm way to lazy to figure out how to generate a reply to something I don't have the original email... John

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"

Or "prehex" ;-)

* * Convert @bitmap to printable string. * * Returns pointer to the string or NULL on error. */ -char *virBitmapString(virBitmapPtr bitmap) +char *virBitmapString(virBitmapPtr bitmap, bool prefix)

One line for each argument is the more "recent" style...

{ 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);

On 11/13/2017 03:50 AM, Martin Kletzander wrote:

It is literally only a wrapper around virBitmapNewData() and virBitmapFormat(), only the naming was wrong since it was introduced.

in commit id '7d8afc47'.

And because we have virBitmap*String functions where the meaning of the 'String' is constant, this might confuse someone.

And some would say DataFormatWhat - cannot please everyone though ;-) because "naming is hard" (per abologna). FWIW: When you noted the 'String' is constant, I immediately thought about a "const char *String" meaning it wouldn't need to be free'd.

Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- 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(-)

So like patch 04/21 - patch 06/21 also hasn't shown up yet, so please consider the following for comments and R-b (all the others showed up). 1. The commit message needs massaging - "fromvirBitmapToString" to be "from virBitmapToString" 2. The 3rd argument should be on it's only line (like noted in 03) With the slight adjustment below, consider both 05 and 06 as: Reviewed-by: John Ferlan <jferlan@redhat.com> John [...]

diff --git a/tests/virbitmaptest.c b/tests/virbitmaptest.c index c24d4a72f70a..488796719dd9 100644 --- a/tests/virbitmaptest.c +++ b/tests/virbitmaptest.c

There's a comment above here that needs adjustment ... s/DataToString/DataFormat to be consistent...

@@ -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; [...]

On Mon, Nov 13, 2017 at 03:42:40PM -0500, John Ferlan wrote:

On 11/13/2017 03:50 AM, Martin Kletzander wrote:

...

Sometimes the size of the bitmap matters and it might not be guessed correctly when parsing from some type of input. For example virBitmapNewData() has Byte granularity, virBitmapNewString() has nibble granularity and so on. virBitmapParseUnlimited() can be tricked into creating huge bitmap that's not needed (e.g.: "0-2,^99999999"). This function provides a way to shrink the bitmap. It is not supposed to free any memory.

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...

I don't free it because a) it would cost more time and b) we over-allocate a bit anyway. Also this is mainly used so that the bitmap size is predictable, not much else.

...

Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virbitmap.c | 19 +++++++++++++++++++ src/util/virbitmap.h | 2 ++ 3 files changed, 22 insertions(+)

With a couple of notes below handled,

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

...

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0b78a0681c5e..3986cc523e39 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1369,6 +1369,7 @@ virBitmapParseUnlimited; virBitmapSetAll; virBitmapSetBit; virBitmapSetBitExpand; +virBitmapShrink; virBitmapSize; virBitmapSubtract; virBitmapToData; diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index ac6ff4f6d26d..95b1f8656907 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -176,6 +176,25 @@ int virBitmapSetBit(virBitmapPtr bitmap, size_t b) return 0; }

+ +/** + * virBitmapShrink: + * @map: Pointer to bitmap + * @b: last bit position to be excluded from bitmap + * + * Resizes the bitmap so that no more than @b bits will fit into it. Nothing + * will change if the size is already smaller than @b.

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)

Added

...

+ */ +void virBitmapShrink(virBitmapPtr map, size_t b)

void virBitmapStrink(virBitmapPtr map, size_t b)

done

...

+{ + if (!map) + return; + + if (map->max_bit >= b) + map->max_bit = b; +} + + /** * virBitmapExpand: * @map: Pointer to bitmap diff --git a/src/util/virbitmap.h b/src/util/virbitmap.h index 7b2bea8b534c..2464814055de 100644 --- a/src/util/virbitmap.h +++ b/src/util/virbitmap.h @@ -153,4 +153,6 @@ void virBitmapIntersect(virBitmapPtr a, virBitmapPtr b) void virBitmapSubtract(virBitmapPtr a, virBitmapPtr b) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);

+void virBitmapShrink(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

I went the other way and moved it in the .c file, I have no idea why I didn't put it in the end anyway.

...

#endif

On 11/13/2017 03:50 AM, Martin Kletzander wrote:

Because the cache banks are initialized based on the order in which their respective directories exist on the filesystem, they can appear in diferrent

s/diferrent/different/

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@redhat.com> --- src/conf/capabilities.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)

A couple of fingers not typing what the brain told them to type and a couple of other nits, for Reviewed-by: John Ferlan <jferlan@redhat.com> John

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); }

Two blank lines new functions

+static int +virCapsHostCacheBankSorter(const void *a, const void *b)

One line per argument

+{ + 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

predictable (weird, my email browser didn't flag the other spelling)

+ * 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);

On 11/13/2017 03:50 AM, Martin Kletzander wrote:

Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- 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(-)

Reviewed-by: John Ferlan <jferlan@redhat.com> John couple of noisy review thoughts below...

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++) {

You could have "virResctrlInfoPtr controls = bank->controls[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 */

Or "if (STRNEQ(unit, min_unit))" - to be more faithful to the comment! I read this as - if min_short_size is there, then we check the unit by knowing the math to get the value. To some degree if the pretty format function allowed one to "choose" a specific format size that'd perhaps work too, but that's perhaps more work than it's worth.

+ 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); } [...]

On Tue, Nov 14, 2017 at 10:57:26AM -0500, John Ferlan wrote:

On 11/13/2017 03:50 AM, Martin Kletzander wrote:

...

This allows for looking up the cache control information more sensibly from conf/capabilities.c and also provides more data to the virresctrl module that will get more usable later on.

Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- po/POTFILES.in | 1 + src/conf/capabilities.c | 48 +++---- src/conf/capabilities.h | 4 +- src/libvirt_private.syms | 4 +- src/util/virresctrl.c | 335 +++++++++++++++++++++++++++++++++-------------- src/util/virresctrl.h | 24 ++-- 6 files changed, 274 insertions(+), 142 deletions(-)

[...]

...

diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 2a11825a52dc..6c6692e78f42 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -18,6 +18,11 @@

#include <config.h>

+#include <sys/file.h> +#include <sys/types.h> +#include <sys/stat.h> +#include <fcntl.h> + #include "virresctrl.h"

#include "c-ctype.h" @@ -25,12 +30,16 @@ #include "viralloc.h" #include "virfile.h" #include "virlog.h" +#include "virobject.h" #include "virstring.h"

+ #define VIR_FROM_THIS VIR_FROM_RESCTRL

VIR_LOG_INIT("util.virresctrl")

+ +/* Common definitions */ #define SYSFS_RESCTRL_PATH "/sys/fs/resctrl"

/* Resctrl is short for Resource Control. It might be implemented for various @@ -55,133 +64,257 @@ VIR_ENUM_IMPL(virResctrl, VIR_CACHE_TYPE_LAST, "CODE", "DATA")

-int -virResctrlGetCacheInfo(unsigned int level, - unsigned long long size, - virCacheType scope, - virResctrlInfoPtr **controls, - size_t *ncontrols) -{ - int ret = -1; - char *tmp = NULL; - char *path = NULL; - char *cbm_mask = NULL; - char *type_upper = NULL; - unsigned int bits = 0; - unsigned int min_cbm_bits = 0; - virResctrlInfoPtr control; - - if (VIR_ALLOC(control) < 0) - goto cleanup;

- if (scope != VIR_CACHE_TYPE_BOTH && - virStringToUpper(&type_upper, virCacheTypeToString(scope)) < 0) - goto cleanup; +/* Info-related definitions and InfoClass-related functions */ +typedef struct _virResctrlInfoPerType virResctrlInfoPerType; +typedef virResctrlInfoPerType *virResctrlInfoPerTypePtr; +struct _virResctrlInfoPerType { + /* Kernel-provided information */ + char *cbm_mask; + unsigned int min_cbm_bits;

- if (virFileReadValueUint(&control->max_allocation, - SYSFS_RESCTRL_PATH "/info/L%u%s/num_closids", - level, - type_upper ? type_upper : "") < 0) - goto cleanup; + /* Our computed information from the above */ + unsigned int bits; + unsigned int max_cache_id;

- if (virFileReadValueString(&cbm_mask, - SYSFS_RESCTRL_PATH - "/info/L%u%s/cbm_mask", - level, - type_upper ? type_upper: "") < 0) - goto cleanup; + /* In order to be self-sufficient we need size information per cache. + * Funnily enough, one of the outcomes of the resctrlfs design is that it + * does not account for different sizes per cache on the same level. So + * for the sake of easiness, let's copy that, for now. */ + unsigned long long size;

- if (virFileReadValueUint(&min_cbm_bits, - SYSFS_RESCTRL_PATH "/info/L%u%s/min_cbm_bits", - level, - type_upper ? type_upper : "") < 0) - goto cleanup; + /* Information that we will return upon request (this is public struct) as + * until now all the above is internal to this module */ + virResctrlInfoPerCache control; +};

- virStringTrimOptionalNewline(cbm_mask); +typedef struct _virResctrlInfoPerLevel virResctrlInfoPerLevel; +typedef virResctrlInfoPerLevel *virResctrlInfoPerLevelPtr; +struct _virResctrlInfoPerLevel { + virResctrlInfoPerTypePtr *types; +};

- for (tmp = cbm_mask; *tmp != '\0'; tmp++) { - if (c_isxdigit(*tmp)) - bits += count_one_bits(virHexToBin(*tmp)); - } +struct _virResctrlInfo { + virObject parent;

- control->granularity = size / bits; - if (min_cbm_bits != 1) - control->min = min_cbm_bits * control->granularity; + virResctrlInfoPerLevelPtr *levels; + size_t nlevels; +};

- control->scope = scope; +static virClassPtr virResctrlInfoClass;

- if (VIR_APPEND_ELEMENT(*controls, *ncontrols, control) < 0) - goto cleanup; +static void +virResctrlInfoDispose(void *obj) +{ + size_t i = 0; + size_t j = 0;

- ret = 0; + virResctrlInfoPtr resctrl = obj;

- cleanup: - VIR_FREE(path); - VIR_FREE(cbm_mask); - VIR_FREE(type_upper); - VIR_FREE(control); - return ret; -} + for (i = 0; i < resctrl->nlevels; i++) { + virResctrlInfoPerLevelPtr level = resctrl->levels[--resctrl->nlevels];

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"...

You're right, this should be: virResctrlInfoPerLevelPtr level = resctrl->levels[i]; It's just leftover from previous version that had: while (resctrl->nlevels)

...

+ + if (!level) + continue;

+ if (level->types) { + for (j = 0; j < VIR_CACHE_TYPE_LAST; j++)

level->types[j]->cbm_mask is leaked.

Fixed

...

+ VIR_FREE(level->types[j]); + } + VIR_FREE(level->types); + VIR_FREE(level); + } + + VIR_FREE(resctrl->levels); +}

-static inline int -virResctrlGetCacheDir(char **path, - const char *prefix, - unsigned int level, - virCacheType type) +static int virResctrlInfoOnceInit(void) { - return virAsprintf(path, - SYSFS_RESCTRL_PATH "%s/L%u%s", - prefix ? prefix : "", - level, - virResctrlTypeToString(type)); + if (!(virResctrlInfoClass = virClassNew(virClassForObject(), + "virResctrlInfo", + sizeof(virResctrlInfo), + virResctrlInfoDispose)))

The indent is off by 1 for the above 3 lines.

Good catch, this used to be called virResctrlAllocClass in one of the past versions before "automatic" rename :D Hence the off-by-one indent ;)

...

+ return -1; + + return 0; }

+VIR_ONCE_GLOBAL_INIT(virResctrlInfo)

-/* - * This function tests whether TYPE of cache control is supported or not. - * - * Returns 0 if not, 1 if yes and negative value on error. - */ -static int -virResctrlGetCacheSupport(unsigned int level, virCacheType type) +static virResctrlInfoPtr +virResctrlInfoNew(void) { - int ret = -1; - char *path = NULL; - - if (virResctrlGetCacheDir(&path, "/info", level, type) < 0) - return -1; + if (virResctrlInfoInitialize() < 0) + return NULL;

- ret = virFileExists(path); - VIR_FREE(path); - return ret; + return virObjectNew(virResctrlInfoClass); }

-/* - * This function tests which TYPE of cache control is supported - * Return values are: - * -1: error - * 0: none - * 1: CAT - * 2: CDP - */ +/* Info-related functions */ int -virResctrlGetCacheControlType(unsigned int level) +virResctrlGetInfo(virResctrlInfoPtr *resctrl) { + DIR *dirp = NULL; + char *info_path = NULL; + char *endptr = NULL; + char *tmp_str = NULL; + int ret = 0; int rv = -1; - - rv = virResctrlGetCacheSupport(level, VIR_CACHE_TYPE_BOTH); - if (rv < 0) + int type = 0; + struct dirent *ent = NULL; + unsigned int level = 0; + virResctrlInfoPerLevelPtr i_level = NULL; + virResctrlInfoPerTypePtr i_type = NULL; + virResctrlInfoPtr tmp_info = virResctrlInfoNew(); + + if (!tmp_info) return -1; - if (rv) - return 1;

- rv = virResctrlGetCacheSupport(level, VIR_CACHE_TYPE_CODE); - if (rv < 0) - return -1; - if (rv) - return 2; + rv = virDirOpenIfExists(&dirp, SYSFS_RESCTRL_PATH "/info"); + if (rv <= 0) { + ret = rv; + goto cleanup; + }

- return 0; + while ((rv = virDirRead(dirp, &ent, SYSFS_RESCTRL_PATH "/info")) > 0) { + if (ent->d_type != DT_DIR) + continue; + + if (ent->d_name[0] != 'L') + continue; + + if (virStrToLong_uip(ent->d_name + 1, &endptr, 10, &level) < 0) + goto cleanup; + + type = virResctrlTypeFromString(endptr); + if (type < 0) + goto cleanup; + + if (VIR_ALLOC(i_type) < 0) + goto cleanup; + + i_type->control.scope = type; + + if (virFileReadValueUint(&i_type->control.max_allocation, + SYSFS_RESCTRL_PATH "/info/%s/num_closids", + ent->d_name) < 0) + goto cleanup; + + if (virFileReadValueString(&i_type->cbm_mask, + SYSFS_RESCTRL_PATH + "/info/%s/cbm_mask", + ent->d_name) < 0) + goto cleanup; + + if (virFileReadValueUint(&i_type->min_cbm_bits, + SYSFS_RESCTRL_PATH "/info/%s/min_cbm_bits", + ent->d_name) < 0) + goto cleanup; + + virStringTrimOptionalNewline(i_type->cbm_mask); + + if (tmp_info->nlevels <= level && + VIR_EXPAND_N(tmp_info->levels, tmp_info->nlevels, + level - tmp_info->nlevels + 1) < 0) + goto cleanup;

Also one more indentation is wrong here. Found by selecting the whole file and letting it auto indent, this was the only missing change, so it should be fine as well. Fixed now.

...

+ + if (!tmp_info->levels[level] && + (VIR_ALLOC(tmp_info->levels[level]) < 0 || + VIR_ALLOC_N(tmp_info->levels[level]->types, VIR_CACHE_TYPE_LAST) < 0)) + goto cleanup; + i_level = tmp_info->levels[level]; + + if (i_level->types[type]) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Duplicate cache type in resctrlfs for level %u"), + level); + goto cleanup; + } + + if (VIR_ALLOC(i_level->types[type]) < 0) + goto cleanup; + + for (tmp_str = i_type->cbm_mask; *tmp_str != '\0'; tmp_str++) { + if (!c_isxdigit(*tmp_str)) + goto cleanup; + + i_type->bits += count_one_bits(virHexToBin(*tmp_str)); + } + + i_level->types[type] = i_type; + i_type = NULL; + } + + *resctrl = tmp_info; + tmp_info = NULL;

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).

Some particular ones should just be "resctrl is not supported here, don't fail starting the daemon because of that". However those are handled, this one should have `int ret = -1;` in the top and `ret = 0;` before the `cleanup:` label of course. Thanks for seeing that. This used to return the pointer to the info previously, that's where the weirdness comes from.

...

+ cleanup: + VIR_DIR_CLOSE(dirp); + VIR_FREE(info_path); + virObjectUnref(tmp_info); + VIR_FREE(i_type); + return ret; +} +

Two blank lines between functions

OK so I've:

Reviewed-by: John Ferlan <jferlan@redhat.com>

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.

I added virReportError for every situation that did not report the error and silently went for goto cleanup; Also changed the ret value as described above.

John

...

+int +virResctrlInfoGetCache(virResctrlInfoPtr resctrl, + unsigned int level, + unsigned long long size, + size_t *ncontrols, + virResctrlInfoPerCachePtr **controls) +{ + virResctrlInfoPerLevelPtr i_level = NULL; + virResctrlInfoPerTypePtr i_type = NULL; + size_t i = 0; + int ret = -1; + + if (!resctrl) + return 0; + + if (level >= resctrl->nlevels) + return 0; + + i_level = resctrl->levels[level]; + if (!i_level) + return 0; + + for (i = 0; i < VIR_CACHE_TYPE_LAST; i++) { + i_type = i_level->types[i]; + if (!i_type) + continue; + + /* Let's take the opportunity to update our internal information about + * the cache size */ + if (!i_type->size) { + i_type->size = size; + i_type->control.granularity = size / i_type->bits; + if (i_type->min_cbm_bits != 1) + i_type->control.min = i_type->min_cbm_bits * i_type->control.granularity; + } else { + if (i_type->size != size) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Forbidden inconsistency for resctrlfs, " + "level %u caches have different sizes"), + level); + goto error; + } + i_type->max_cache_id++; + } + + if (VIR_EXPAND_N(*controls, *ncontrols, 1) < 0) + goto error; + if (VIR_ALLOC((*controls)[*ncontrols - 1]) < 0) + goto error; + + memcpy((*controls)[*ncontrols - 1], &i_type->control, sizeof(i_type->control)); + } + + ret = 0; + cleanup: + return ret; + error: + while (*ncontrols) + VIR_FREE((*controls)[--*ncontrols]); + VIR_FREE(*controls); + goto cleanup; }

[...]

On 11/13/2017 03:50 AM, Martin Kletzander wrote:

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

functions

only supposed to be used from tests.

Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- 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

This is a *lot* of code! I wasn't able to run through Coverity mainly because I have some stuff in a local branch that conflicts with earlier patches. If you push those, then I can apply these later patches and let Coverity have a peek on memory leaks or other strangeness that I could have missed below. I'll reserve the right to come back here again ;-) I think there's only a few "missed things".

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];

Again the odd (to me at least) looking loop control that's reducing the for loop end condition.

+ + 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);

what about type->masks[k] You could create a Free function for each entry too.

+ VIR_FREE(type); + } + VIR_FREE(level->types); + VIR_FREE(level); + } + + VIR_FREE(resctrl->id);

resctrl->path ?

+ VIR_FREE(resctrl->levels); +} +

Two blank lines (multiple instances in new functions)

+static int virResctrlAllocOnceInit(void)

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) {

So only ever used on a local file system right? Linux file locking functions are confounding... Why not use virFile{Lock|Unlock}?

+ virReportSystemError(errno, "%s", _("Cannot lock resctrlfs")); + VIR_FORCE_CLOSE(fd); + return -1; + } + + return fd; +} + +static inline int +virResctrlLockRead(void)

Not used in this series...

+{ + 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 */

So if close unlocks too, then why the unlock?

+ 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 */

A few notes about the arguments could be beneficial... Or at least the algorithm that allows partial allocations to work for future consumers.

+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]; +} +

This could really use a functional description especially since it's external... Interesting way to code this though - took a few attempts to stare at it before it finally started sinking in ;-)

+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);> +} +

Similarly here too... I think "external" function should be commented. I'll only mention again here though...

+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)

No external consumer yet.

+{ + 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. */

Should we use 'sa_assert' instead of just hoping? it's not the verboten assert, but at least the Coverity or Clang would catch, right?

+ for (j = 0; j < VIR_CACHE_TYPE_LAST; j++) { + virResctrlAllocSubtractPerType(a->levels[i]->types[j], + b->levels[i]->types[j]); + } + } + } +} + +virResctrlAllocPtr +virResctrlAllocNewFromInfo(virResctrlInfoPtr info)

No external consumer

+{ + 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)

No external consumer...

+{ + 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)

No external consumer...

+{ + 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++) {

OMG, my eyes need a beer!

+ 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)

This is being created in /sys/fs... and theoretically nothing will change for @drivername and @machinename

+ return -1; + + /* Check if this allocation was already created */ + if (virFileIsDir(alloc->path)) { + VIR_FREE(alloc_path);

dead code ;-) "alloc_path" is never allocated... Any concern over the guest being killed without running through virResctrlAllocRemove and the rmdir?

+ 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;

Leaking... and leaving 'em locked on the way out.

+ + 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__ */

On Mon, Nov 13, 2017 at 09:50:31AM +0100, Martin Kletzander wrote:

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@redhat.com> --- 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 \

Use only single space instead of tab after "util/virresctrl.c" and "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) +{

I don't like this implementation, it's too complex and it does two different things based on a bool attribute. I see the benefit that it's convenient but IMHO it's ugly. The only call path that doesn't need allocation is from virResctrlAllocCheckCollision(). The remaining two calls virResctrlAllocUpdateMask() and virResctrlAllocUpdateSize() needs to allocate the internal structures of *virResctrlAllocPtr* object. Another point is that there is no need to have this function create new *virResctrlAllocPtr* object on demand, I would prefer creating that object in advance before we even start filling all the data.

+ 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) +{

The code of this function can be merged into virResctrlAllocUpdateMask() and again only allocate the structures and set the mask. Currently there is no need for "Find" function if we will need it in the future it should definitely only find the mask, not allocate it.

+ 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);

Same as for virResctrlAllocFindMask(). With the exception that we actually need lookup function so create a one, that will only check whether there is some size set or not.

+ + 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); +}

This is how I would expect all the other public functions to look like. Simple, does one thing and there is no magic.

+ +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) +{

The virResctrlAllocPtr object should already exists and this function should only parse the data into existing object.

+ 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; + }

I'm wondering whether this error message can be moved to virResctrlAllocCreate() or somewhere else to hit it as soon as possible.

+ + if (!save_ptr) { + alloc_free = virResctrlAllocGetFree(r_info); + } else { + if (!*save_ptr) + *save_ptr = virResctrlAllocGetFree(r_info); + + alloc_free = *save_ptr; + }

This code and the *save_ptr* is here only for tests purposes. Would it be possible to get rid of it? How much time it takes to calculate free allocation?

+ + 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

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On Wed, Nov 15, 2017 at 07:14:28PM +0100, Pavel Hrdina wrote:

On Mon, Nov 13, 2017 at 09:50:31AM +0100, Martin Kletzander wrote:

...

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@redhat.com> --- 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 \

Use only single space instead of tab after "util/virresctrl.c" and "util/virresctrlpriv.h".

That is actualy a single blank. It was a TAB that I didn't see in the code, but here, since it is one more character to the right, it shows. Again I don't see it in this reply as it again aligned differently :) [...]

...

@@ -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) +{

I don't like this implementation, it's too complex and it does two different things based on a bool attribute. I see the benefit that it's convenient but IMHO it's ugly.

The only call path that doesn't need allocation is from virResctrlAllocCheckCollision(). The remaining two calls virResctrlAllocUpdateMask() and virResctrlAllocUpdateSize() needs to allocate the internal structures of *virResctrlAllocPtr* object.

I'll duplicate the code if that's what's desired. I guess it will not look as gross as this then.

Another point is that there is no need to have this function create new *virResctrlAllocPtr* object on demand, I would prefer creating that object in advance before we even start filling all the data.

Just to make sure we are on the same page benefit-wise. There actually is. It will only be created if anyone adds size or mask to the allocation, otherwise it is NULL. It is easy to check that the allocation is empty. I'll redo it your way, but you need to have new object created, then update some stuff for it and then have a function that checks if the allocation is empty. And that needs three nested loops which there are too many already in this. [...]

...

+ +static virBitmapPtr * +virResctrlAllocFindMask(virResctrlAllocPtr *resctrl, + unsigned int level, + virCacheType type, + unsigned int cache, + bool alloc) +{

The code of this function can be merged into virResctrlAllocUpdateMask() and again only allocate the structures and set the mask. Currently there is no need for "Find" function if we will need it in the future it should definitely only find the mask, not allocate it.

This is here just for separation. I can just cut-n-paste it into the other function. The same with other ones. It sill just create bigger functions that are IMNSHO less readable. Sure I can do that. [...]

...

+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); +}

This is how I would expect all the other public functions to look like. Simple, does one thing and there is no magic.

Well, because it does totally nothing. If all "public" functions would do this then there would be no functionality =D [...]

...

+static int +virResctrlAllocParse(virResctrlAllocPtr *alloc, + const char *schemata) +{

The virResctrlAllocPtr object should already exists and this function should only parse the data into existing object.

Same as above, but OK, I want to get rid of this patchset, finally. [...]

...

+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; + }

I'm wondering whether this error message can be moved to virResctrlAllocCreate() or somewhere else to hit it as soon as possible.

The resctrl-related decision should not be moved out of virresctrl, so it can be moved to AllocCreate(), but not more up. Even then I would rather duplicate it instead of moving it there so that this function can be used on its own (for tests).

...

+ + if (!save_ptr) { + alloc_free = virResctrlAllocGetFree(r_info); + } else { + if (!*save_ptr) + *save_ptr = virResctrlAllocGetFree(r_info); + + alloc_free = *save_ptr; + }

This code and the *save_ptr* is here only for tests purposes. Would it be possible to get rid of it? How much time it takes to calculate free allocation?

Not test purposes, it keeps the current state. Think of it as char **saveptr in strtok_r() and similar. How much time it takes? Depends on how much allocations you have. Crawling through the sysfs tree, doing bunch of allocations here and there. You need to run virResctrlAllocGetFree() on every entry. It's relatively fast, but pointless. But if I do that, not only I can remove these 8 lines of code, but also ...

...

+ + 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));

... this one.

On Mon, Nov 13, 2017 at 09:50:32AM +0100, Martin Kletzander wrote:

Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- docs/formatdomain.html.in | 24 ++ docs/schemas/domaincommon.rng | 32 +++ src/conf/domain_conf.c | 249 +++++++++++++++++++++ src/conf/domain_conf.h | 21 ++ src/util/virresctrl.c | 2 +- .../genericxml2xmlindata/generic-cachetune-cdp.xml | 36 +++ .../generic-cachetune-colliding-allocs.xml | 30 +++ .../generic-cachetune-colliding-tunes.xml | 32 +++ .../generic-cachetune-colliding-types.xml | 30 +++ .../generic-cachetune-small.xml | 29 +++ tests/genericxml2xmlindata/generic-cachetune.xml | 33 +++ tests/genericxml2xmltest.c | 10 + 12 files changed, 527 insertions(+), 1 deletion(-) create mode 100644 tests/genericxml2xmlindata/generic-cachetune-cdp.xml create mode 100644 tests/genericxml2xmlindata/generic-cachetune-colliding-allocs.xml create mode 100644 tests/genericxml2xmlindata/generic-cachetune-colliding-tunes.xml create mode 100644 tests/genericxml2xmlindata/generic-cachetune-colliding-types.xml create mode 100644 tests/genericxml2xmlindata/generic-cachetune-small.xml create mode 100644 tests/genericxml2xmlindata/generic-cachetune.xml

[...]

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 400e900325f2..bf9e61efc8d2 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c

[...]

+static int +virDomainCachetuneDefParse(virDomainDefPtr def, + xmlXPathContextPtr ctxt, + xmlNodePtr node) +{ + xmlNodePtr oldnode = ctxt->node; + xmlNodePtr *nodes = NULL; + virBitmapPtr vcpus = NULL; + virResctrlAllocPtr alloc = NULL; + virDomainCachetuneDefPtr tmp_cachetune = NULL; + char *tmp = NULL; + char *vcpus_str = NULL; + ssize_t i = 0; + int n; + int ret = -1; + + ctxt->node = node; + + if (VIR_ALLOC(tmp_cachetune) < 0) + goto cleanup; + + vcpus_str = virXMLPropString(node, "vcpus"); + if (!vcpus_str) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Missing attribute vcpus for cachetune")); + goto cleanup; + } + if (virBitmapParse(vcpus_str, &vcpus, VIR_DOMAIN_CPUMASK_LEN) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid attribute vcpus '%s' for cachetune"), + vcpus_str); + goto cleanup; + } + + if ((n = virXPathNodeSet("./cache", ctxt, &nodes)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot extract cachetune/cache nodes")); + goto cleanup; + } + + for (i = 0; i < n; i++) { + if (virDomainCachetuneDefParseCache(ctxt, nodes[i], &alloc) < 0) + goto cleanup; + } + + if (!alloc) { + ret = 0; + goto cleanup; + } + + virBitmapShrink(vcpus, def->maxvcpus); + + for (i = 0; i < def->ncachetunes; i++) { + if (virBitmapOverlaps(def->cachetunes[i]->vcpus, vcpus)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Overlapping vcpus in cachetunes")); + goto cleanup; + } + } + + if (virResctrlAllocSetID(alloc, vcpus_str) < 0) + goto cleanup; + + tmp_cachetune->vcpus = vcpus; + tmp_cachetune->alloc = alloc; + vcpus = NULL; + alloc = NULL;

VIR_STEAL_PTR()

+ + if (VIR_APPEND_ELEMENT(def->cachetunes, def->ncachetunes, tmp_cachetune) < 0) + goto cleanup; + + ret = 0; + cleanup: + ctxt->node = oldnode; + virDomainCachetuneDefFree(tmp_cachetune); + virObjectUnref(alloc); + virBitmapFree(vcpus); + VIR_FREE(vcpus_str); + VIR_FREE(nodes); + VIR_FREE(tmp); + return ret; +} + + static virDomainDefPtr virDomainDefParseXML(xmlDocPtr xml, xmlNodePtr root, @@ -18632,6 +18804,18 @@ virDomainDefParseXML(xmlDocPtr xml, } VIR_FREE(nodes);

+ if ((n = virXPathNodeSet("./cputune/cachetune", ctxt, &nodes)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot extract cachetune nodes")); + goto error; + } + + for (i = 0; i < n; i++) { + if (virDomainCachetuneDefParse(def, ctxt, nodes[i]) < 0) + goto error; + } + VIR_FREE(nodes); + if (virCPUDefParseXML(ctxt, "./cpu[1]", VIR_CPU_TYPE_GUEST, &def->cpu) < 0) goto error;

@@ -25444,6 +25628,68 @@ virDomainSchedulerFormat(virBufferPtr buf, }

+struct virCachetuneHelperData { + virBufferPtr buf; + size_t vcpu_id; +}; + +static int +virDomainCachetuneDefFormatHelper(unsigned int level, + virCacheType type, + unsigned int cache, + unsigned long long size, + void *opaque)

Wrong indentation.

+{ + const char *unit; + virBufferPtr buf = opaque; + unsigned long long short_size = virPrettySize(size, &unit); + + virBufferAsprintf(buf, + "<cache id='%u' level='%u' type='%s' " + "size='%llu' unit='%s'/>\n", + cache, level, virCacheTypeToString(type), + short_size, unit); + + return 0; +}

Pavel

On Wed, Nov 15, 2017 at 05:22:24PM -0500, John Ferlan wrote:
>
>
>On 11/13/2017 03:50 AM, Martin Kletzander wrote:
>> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>> ---
>>  docs/news.xml | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>> > diff --git a/docs/news.xml b/docs/news.xml
>> index ef855d895886..e06c981e36da 100644
>> --- a/docs/news.xml
>> +++ b/docs/news.xml
>> @@ -35,6 +35,17 @@
>>  <libvirt>
>>    <release version="v3.10.0" date="unreleased">
>>      <section title="New features">
>> +      <change>
>> +        <summary>
>> +          Added support for CAT (resctrl tuning)
>
>Actually Cache Allocation Technology, right?
>

I don't know why I wanted to push resctrl in somehow.  Fixed.

>> +        </summary>
>> +        <description>
>> +          Domain vCPU threads can now have allocated some parts of host cache
>> +          using <code>cachetune</code> element in <code>cputune</code>.  The
>
>using the
>
>> +          support is limited in some ways, but in the future it could be
>> +          extended.
>
>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 left that part out and documented what's supported in, well, the
documentation.  There is no need to have this part in news.xml, I
guess.  Anyway, there'll be everything in v2 later.

>I trust you can come up with something more complete without writing too
>much.
>
>Reviewed-by: John Ferlan <jferlan@redhat.com>
>
>
>John
>> +        </description>
>> +      </change>
>>      </section>
>>      <section title="Improvements">
>>      </section>
>>