3:49 a.m.
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(a)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.