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