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