[libvirt] [PATCH 16/21] resctrl: Add functions to work with resctrl allocations

[John Ferlan]

[...]

>>  /* 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.
> 

[...]