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@xxxxxxxxxx>
> ---
> 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__ */
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
