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@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 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.
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
