[libvirt] [PATCH 5/9] resctrl: Add functions to work with resctrl allocations
Martin Kletzander
mkletzan at redhat.com
Mon Jan 22 15:44:33 UTC 2018
On Thu, Jan 11, 2018 at 03:58:42PM +0100, Pavel Hrdina wrote:
>On Wed, Jan 03, 2018 at 04:05:38PM -0500, John Ferlan wrote:
>>
>>
>> On 12/13/2017 10:39 AM, 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 functions that are exposed in virresctrlpriv.h as those are
>> > only supposed to be used from tests.
>> >
>> > Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
>> > ---
>> > src/Makefile.am | 2 +-
>> > src/libvirt_private.syms | 11 +
>> > src/util/virresctrl.c | 1041 ++++++++++++++++++++++++++++++++++++++++++++-
>> > src/util/virresctrl.h | 62 +++
>> > src/util/virresctrlpriv.h | 27 ++
>> > 5 files changed, 1141 insertions(+), 2 deletions(-)
>> > create mode 100644 src/util/virresctrlpriv.h
>> >
>>
>> Geez, as referenced by the cover letter, I'm glad this is the non way
>> too complicated parts of the code (tongue firmly planted in my cheek
>> with the obligatory eye roll emoji).
>>
>> So, IIUC virResctrlInfoPtr is the "host" world and virResctrlAllocPtr is
>> the "guest" world, right? If so might be something to add to the commit
>> messages to make things just slightly more clear.
>>
>> Lots of code here - hopefully another set of eyes can look at this too -
>> I'm sure I'll miss something as it's been very difficult to review this
>> in one (long) session...
>>
>> Note to self, no sense in grep-ing for "alloc" or "free" any more after
>> this code is pushed as they're liberally used. Kind of funny to see
>> "alloc_free" in the same line ;-)
>>
Feel free to suggest different naming. My head is so full of this that
I couldn't think of anything more sensible.
>> The other usage that was really confusing is @cache w/ @[n]masks and
>> @[n]sizes. It seems to be used as the array index for both and it's
>> never really crystal clear over the relationship between masks and sizes.
>>
There's a lot info that I could add in the comments. And I will for the
next version.
>> > diff --git a/src/Makefile.am b/src/Makefile.am
>> > index 4c022d1e44b9..9b4fd0c1d597 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 \
>> > util/virrotatingfile.h util/virrotatingfile.c \
>> > util/virscsi.c util/virscsi.h \
>> > util/virscsihost.c util/virscsihost.h \
>> > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> > index 1a4f304f5e1f..a8009d913669 100644
>> > --- a/src/libvirt_private.syms
>> > +++ b/src/libvirt_private.syms
>> > @@ -2548,6 +2548,17 @@ virRandomInt;
>> > # util/virresctrl.h
>> > virCacheTypeFromString;
>> > virCacheTypeToString;
>> > +virResctrlAllocAddPID;
>> > +virResctrlAllocCreate;
>> > +virResctrlAllocForeachSize;
>> > +virResctrlAllocFormat;
>> > +virResctrlAllocGetFree;
>> > +virResctrlAllocNew;
>> > +virResctrlAllocRemove;
>> > +virResctrlAllocSetID;
>> > +virResctrlAllocSetSize;
>> > +virResctrlAllocUpdateMask;
>> > +virResctrlAllocUpdateSize;
>> > virResctrlGetInfo;
>> > virResctrlInfoGetCache;
>> > virResctrlInfoNew;
>> > diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
>> > index a681322905be..32ab84b6f121 100644
>> > --- a/src/util/virresctrl.c
>> > +++ b/src/util/virresctrl.c
>> > @@ -18,8 +18,12 @@
>> >
>> > #include <config.h>
>> >
>> > -#include "virresctrl.h"
>> > +#include <sys/file.h>
>> > +#include <sys/types.h>
>> > +#include <sys/stat.h>
>> > +#include <fcntl.h>
>> >
>> > +#include "virresctrlpriv.h"
>> > #include "c-ctype.h"
>> > #include "count-one-bits.h"
>> > #include "viralloc.h"
>> > @@ -151,6 +155,156 @@ virResctrlInfoNew(void)
>> > }
>> >
>> >
>> > +/* Alloc-related definitions and AllocClass-related functions */
>> > +typedef struct _virResctrlAllocPerType virResctrlAllocPerType;
>> > +typedef virResctrlAllocPerType *virResctrlAllocPerTypePtr;
>> > +struct _virResctrlAllocPerType {
>> > + /* There could be bool saying whether this is set or not, but since everything
>> > + * in virResctrlAlloc (and most of libvirt) goes with pointer arrays we would
>> > + * have to have one more level of allocation anyway, so this stays faithful to
>> > + * the concept */
>> > + unsigned long long **sizes;
>> > + size_t nsizes;
>>
>> Perhaps something I should have noted in patch 2 - these @sizes are what
>> exactly? Is it a page size or just a "max size" in ?bytes for something
>> stored in the cache?
>
>It's an array that stores allocation size per CPU cache, since there
>can be multiple CPUs in the host system.
>
>> > +
>> > + /* Mask for each cache */
>> > + virBitmapPtr *masks;
>> > + size_t nmasks;
>>
>> And of course, what does the mask represent?
>
>The mask represents how the cache allocation is written into the
>schemata file and it is based on the cache allocation size.
>Let's say that you have L3 cache with size 15 MiB where granularity
>of cache allocation is 768 KiB but minimal allocation is 1536 KiB.
>This means that you have 20bit mask to configure the cache allocation.
>
>So for example, if you would like to configure 3072 KiB of L3 cache
>for a guest, the size would be stored in sizes. When the guest is
>started, the following code tries to find a free allocation, that can
>be used by the guest and if the allocation is possible it will store
>the allocation mask for that specified CPU.
>
>This would be the guest XML part:
>
> <cachetune vcpus='0-3'>
> <cache id='0' level='3' type='both' size='3' unit='MiB'/>
> </cachetune>
>
>And the resulting mask can be for example:
>
> 0x00F00
>
>Which would lead into this schemata file:
>
>L3:0=00F00
>
>> Just a quick comment would suffice - for the future readers as well.
>>
>> > +};
>> > +
>> > +typedef struct _virResctrlAllocPerLevel virResctrlAllocPerLevel;
>> > +typedef virResctrlAllocPerLevel *virResctrlAllocPerLevelPtr;
>> > +struct _virResctrlAllocPerLevel {
>> > + virResctrlAllocPerTypePtr *types; /* Indexed with enum virCacheType */
>> > +};
>> > +
>> > +struct _virResctrlAlloc {
>> > + virObject parent;
>> > +
>> > + virResctrlAllocPerLevelPtr *levels;
>> > + size_t nlevels;
>>
>> These represent the "L#" for the directory, right? Examples would be
>> L1both, L2code, l3data, right?
>>
>> Just trying to provide enough information so that some future reader
>> will have a chance to understand the design at it's simplist level.
>>
>> > +
>> > + char *id; /* The identifier (any unique string for now) */
>>
>> we could call this uniqueId too, IDC really though. Just having "id"
>> makes it a bit difficult to search on (cacheUID also works).
>>
>> > + char *path;
>>
>> This is our generated path to the guest cache allocation data.
>>
>> > +};
>> > +
>> > +static virClassPtr virResctrlAllocClass;
>> > +
>> > +static void
>> > +virResctrlAllocDispose(void *obj)
>> > +{
>> > + size_t i = 0;
>> > + size_t j = 0;
>> > + size_t k = 0;
>> > +
>> > + virResctrlAllocPtr resctrl = obj;
>> > +
>> > + for (i = 0; i < resctrl->nlevels; i++) {
>> > + virResctrlAllocPerLevelPtr level = resctrl->levels[i];
>> > +
>> > + if (!level)
>> > + continue;
>>
>> Looking at virResctrlAllocGetType, it's possible that ->levels is
>> allocated, but ->levels[level]->types is not, thus potentially causing
>> problems for the following loop.
>>
>> I think a " || !level->types" above would do the trick.
>
>Nice catch, however I would rather refactor the virResctrlAllocGetType
>function to always allocate it correctly or cleanup after itself it
>the allocation fails.
>
>For example instead of this code:
>
> if (!resctrl->levels[level] &&
> (VIR_ALLOC(resctrl->levels[level]) < 0 ||
> VIR_ALLOC_N(resctrl->levels[level]->types, VIR_CACHE_TYPE_LAST) < 0))
> return NULL;
>
>we can have something like this:
>
> if (!resctrl->levels[level]) {
> virResctrlAllocPerTypePtr *types = NULL;
>
> if (VIR_ALLOC_N(types, VIR_CACHE_TYPE_LAST) < 0)
> return NULL;
>
> if (VIR_ALLOC(resctrl->levels[level]) < 0) {
> VIR_FREE(types);
> return NULL;
> }
> resctrl->levels[level]->types = types;
> }
>
Yeah, it used to be sparsely allocated, but when switching I forgot this
part. It should clean up after itself.
>> > +
>> > + for (j = 0; j < VIR_CACHE_TYPE_LAST; j++) {
>> > + virResctrlAllocPerTypePtr type = level->types[j];
>> > +
>> > + if (!type)
>> > + continue;
>> > +
>> > + for (k = 0; k < type->nsizes; k++)
>> > + VIR_FREE(type->sizes[k]);
>> > +
>> > + for (k = 0; k < type->nmasks; k++)
>> > + virBitmapFree(type->masks[k]);
>> > +
>> > + VIR_FREE(type->sizes);
>> > + VIR_FREE(type->masks);
>> > + VIR_FREE(type);
>> > + }
>> > + VIR_FREE(level->types);
>> > + VIR_FREE(level);
>> > + }
>> > +
>> > + VIR_FREE(resctrl->id);
>> > + VIR_FREE(resctrl->path);
>> > + VIR_FREE(resctrl->levels);
>> > +}
>> > +
>> > +
>> > +static int
>> > +virResctrlAllocOnceInit(void)
>> > +{
>> > + if (!(virResctrlAllocClass = virClassNew(virClassForObject(),
>> > + "virResctrlAlloc",
>> > + sizeof(virResctrlAlloc),
>> > + virResctrlAllocDispose)))
>> > + return -1;
>> > +
>> > + return 0;
>> > +}
>> > +
>> > +
>> > +VIR_ONCE_GLOBAL_INIT(virResctrlAlloc)
>> > +
>> > +
>> > +virResctrlAllocPtr
>> > +virResctrlAllocNew(void)
>> > +{
>> > + if (virResctrlAllocInitialize() < 0)
>> > + return NULL;
>> > +
>> > + return virObjectNew(virResctrlAllocClass);
>> > +}
>> > +
>> > +
>> > +/* Common functions */
>> > +static int
>> > +virResctrlLockInternal(int op)
>> > +{
>> > + int fd = open(SYSFS_RESCTRL_PATH, O_DIRECTORY | O_CLOEXEC);
>>
>> I started reviewing bottom up and wondered later on - do we really want
>> to be writing into /sys/fs/resctrl? See virResctrlAllocCreate comments.
>
>Well, we kind of need to write to /sys/fs/resctrl to configure the
>cache allocation in the host for the guest that is about to start.
>
>> > +
>> > + if (fd < 0) {
>> > + virReportSystemError(errno, "%s", _("Cannot open resctrlfs"));
>> > + return -1;
>> > + }
>> > +
>> > + if (flock(fd, op) < 0) {
>> > + virReportSystemError(errno, "%s", _("Cannot lock resctrlfs"));
>> > + VIR_FORCE_CLOSE(fd);
>> > + return -1;
>> > + }
>> > +
>> > + return fd;
>> > +}
>> > +
>> > +
>> > +static inline int
>> > +virResctrlLockWrite(void)
>> > +{
>> > + return virResctrlLockInternal(LOCK_EX);
>> > +}
>> > +
>> > +
>> > +static int
>> > +virResctrlUnlock(int fd)
>> > +{
>> > + if (fd == -1)
>> > + return 0;
>> > +
>> > + /* The lock gets unlocked by closing the fd, which we need to do anyway in
>> > + * order to clean up properly */
>> > + if (VIR_CLOSE(fd) < 0) {
>> > + virReportSystemError(errno, "%s", _("Cannot close resctrlfs"));
>> > +
>> > + /* Trying to save the already broken */
>> > + if (flock(fd, LOCK_UN) < 0)
>> > + virReportSystemError(errno, "%s", _("Cannot unlock resctrlfs"));
>> > + return -1;
>> > + }
>> > +
>> > + return 0;
>> > +}
>> > +
>> > +
>> > /* Info-related functions */
>> > bool
>> > virResctrlInfoIsEmpty(virResctrlInfoPtr resctrl)
>> > @@ -354,3 +508,888 @@ virResctrlInfoGetCache(virResctrlInfoPtr resctrl,
>> > VIR_FREE(*controls);
>> > goto cleanup;
>> > }
>> > +
>> > +
>> > +/* Alloc-related functions */
>> > +bool
>> > +virResctrlAllocIsEmpty(virResctrlAllocPtr resctrl)
>> > +{
>> > + size_t i = 0;
>> > + size_t j = 0;
>> > + size_t k = 0;
>> > +
>> > + if (!resctrl)
>> > + return true;
>> > +
>>
>> Rather than numerous loops - would it perhaps be better to have a single
>> boolean field that gets set whenever a "sizes" or "masks" entry is set?
>> Of course this would all need to be locked, right?
>>
It wouldn't, no two threads can access the same allocation, it's under a VM.
>> > + for (i = 0; i < resctrl->nlevels; i++) {
>> > + virResctrlAllocPerLevelPtr a_level = resctrl->levels[i];
>> > +
>> > + if (!a_level)
>> > + continue;
>> > +
>> > + for (j = 0; j < VIR_CACHE_TYPE_LAST; j++) {
>> > + virResctrlAllocPerTypePtr a_type = a_level->types[j];
>> > +
>> > + if (!a_type)
>> > + continue;
>> > +
>> > + for (k = 0; k < a_type->nsizes; k++) {
>> > + if (a_type->sizes[k])
>> > + return false;
>> > + }
>> > +
>> > + for (k = 0; k < a_type->nmasks; k++) {
>> > + if (a_type->masks[k])
>> > + return false;
>> > + }
>> > + }
>> > + }
>> > +
>> > + return true;
>> > +}
>> > +
>> > +
>> > +static virResctrlAllocPerTypePtr
>> > +virResctrlAllocGetType(virResctrlAllocPtr resctrl,
>> > + unsigned int level,
>> > + virCacheType type)
>> > +{
>> > + virResctrlAllocPerLevelPtr a_level = NULL;
>> > +
>> > + if (resctrl->nlevels <= level &&
>> > + VIR_EXPAND_N(resctrl->levels, resctrl->nlevels, level - resctrl->nlevels + 1) < 0)
>> > + return NULL;
>> > +
>> > + if (!resctrl->levels[level] &&
>> > + (VIR_ALLOC(resctrl->levels[level]) < 0 ||
>> > + VIR_ALLOC_N(resctrl->levels[level]->types, VIR_CACHE_TYPE_LAST) < 0))
>> > + return NULL;
>>
>> Could have a problem if ->levels[level] ALLOC succeeds, but
>> ->levels[level]->types ALLOC_N fails vis-a-vis the assumption in Dispose.
>>
>> > +
>> > + a_level = resctrl->levels[level];
>> > +
>> > + if (!a_level->types[type] && VIR_ALLOC(a_level->types[type]) < 0)
>> > + return NULL;
>> > +
>> > + return a_level->types[type];
>> > +}
>> > +
>> > +
>> > +int
>> > +virResctrlAllocUpdateMask(virResctrlAllocPtr resctrl,
>> > + unsigned int level,
>> > + virCacheType type,
>> > + unsigned int cache,
>> > + virBitmapPtr mask)
>>
>> So far, only ever called from virresctrl.c, so can this be static? Or
>> are there future plans?
>>
>> > +{
>> > + virResctrlAllocPerTypePtr a_type = virResctrlAllocGetType(resctrl, level, type);
>> > +
>> > + if (!a_type)
>> > + return -1;
>> > +
>> > + if (a_type->nmasks <= cache &&
>> > + VIR_EXPAND_N(a_type->masks, a_type->nmasks,
>> > + cache - a_type->nmasks + 1) < 0)
>> > + return -1;
>> > +
>> > + if (!a_type->masks[cache]) {
>> > + a_type->masks[cache] = virBitmapNew(virBitmapSize(mask));
>> > +
>> > + if (!a_type->masks[cache])
>> > + return -1;
>> > + }
>>
>> Since this is an Update function, is it possible that the incoming mask
>> is a different size than what was already existing? IOW: In an else
>> condition, do we need to compare the size of the source and destination
>> before the Copy? And if so, Free the current and allocate one using the
>> new size...
>
>This shouldn't ever happen and if it does, something is seriously
>wrong since the mask is stored in /sys/fs/resctrl and it should
>already ensure that everything is correctly formatted.
>
>> > +
>> > + return virBitmapCopy(a_type->masks[cache], mask);
>>
>> In order to speed up IsEmpty we could set a boolean indicating the
>> change to resctrl
>>
It wouldn't speed up anything really, instead we would have duplicated
information which would cause mess as it can lead to more errors.
>> > +}
>> > +
>> > +
>> > +int
>> > +virResctrlAllocUpdateSize(virResctrlAllocPtr resctrl,
>> > + unsigned int level,
>> > + virCacheType type,
>> > + unsigned int cache,
>> > + unsigned long long size)
>>
>> Same here, only called from virresctrl.c
>>
>> > +{
>> > + virResctrlAllocPerTypePtr a_type = virResctrlAllocGetType(resctrl, level, type);
>> > +
>> > + if (!a_type)
>> > + return -1;
>> > +
>> > + if (a_type->nsizes <= cache &&
>> > + VIR_EXPAND_N(a_type->sizes, a_type->nsizes,
>> > + cache - a_type->nsizes + 1) < 0)
>> > + return -1;
>> > +
>> > + if (!a_type->sizes[cache] && VIR_ALLOC(a_type->sizes[cache]) < 0)
>> > + return -1;
>> > +
>> > + *(a_type->sizes[cache]) = size;
>> > +
>>
>> Same comment regarding IsEmpty and setting a boolean...
>>
>> > + return 0;
>> > +}
>> > +
>> > +
>> > +static bool
>> > +virResctrlAllocCheckCollision(virResctrlAllocPtr a,
>> > + unsigned int level,
>> > + virCacheType type,
>> > + unsigned int cache)
>> > +{
>> > + virResctrlAllocPerLevelPtr a_level = NULL;
>> > + virResctrlAllocPerTypePtr a_type = NULL;
>> > +
>> > + if (!a)
>> > + return false;
>> > +
>> > + if (a->nlevels <= level)
>> > + return false;
>> > +
>> > + a_level = a->levels[level];
>> > +
>> > + if (!a_level)
>> > + return false;
>> > +
>> > + /* All types should be always allocated */
>> > + a_type = a_level->types[VIR_CACHE_TYPE_BOTH];
>> > +
>> > + /* If there is an allocation for type 'both', there can be no other
>> > + * allocation for the same cache */
>> > + if (a_type && a_type->nsizes > cache && a_type->sizes[cache])
>> > + return true;
>> > +
>> > + if (type == VIR_CACHE_TYPE_BOTH) {
>> > + a_type = a_level->types[VIR_CACHE_TYPE_CODE];
>> > +
>> > + if (a_type && a_type->nsizes > cache && a_type->sizes[cache])
>> > + return true;
>> > +
>> > + a_type = a_level->types[VIR_CACHE_TYPE_DATA];
>> > +
>> > + if (a_type && a_type->nsizes > cache && a_type->sizes[cache])
>> > + return true;
>> > + } else {
>> > + a_type = a_level->types[type];
>> > +
>> > + if (a_type && a_type->nsizes > cache && a_type->sizes[cache])
>> > + return true;
>> > + }
>>
>> This is simple yet confusing all in the same pile of code. I'm sure it
>> helps to have the overall design in mind though.
>>
>> Would it even matter to check any of this if IsEmpty returns 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;
>>
>> Is it possible there are no sizes set?
>>
Yes, then it returns zero without calling the callback.
>> > +
>> > + 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);
>> > +}
>> > +
>> > +
>>
>> Just a few words here about the expected format you'll be formatting and
>> parsing would be helpful. Still not 100% clear how [n]sizes comes into
>> play. When you're saving/re-reading I guess I would have thought (for
>> some reason) that perhaps sizes would be important
>>
>> > +char *
>> > +virResctrlAllocFormat(virResctrlAllocPtr resctrl)
>>
>> So far only ever called from virresctrl.c
>>
>> > +{
>> > + virBuffer buf = VIR_BUFFER_INITIALIZER;
>> > + unsigned int level = 0;
>> > + unsigned int type = 0;
>> > + unsigned int cache = 0;
>> > +
>> > + if (!resctrl)
>> > + return NULL;
>> > +
>> > + 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;
>> > +
>> > + virBufferAsprintf(&buf, "L%u%s:", level, virResctrlTypeToString(type));
>> > +
>> > + for (cache = 0; cache < a_type->nmasks; cache++) {
>> > + virBitmapPtr mask = a_type->masks[cache];
>> > + char *mask_str = NULL;
>> > +
>> > + if (!mask)
>> > + continue;
>> > +
>> > + mask_str = virBitmapToString(mask, false, true);
>> > + if (!mask_str) {
>> > + virBufferFreeAndReset(&buf);
>> > + return NULL;
>> > + }
>> > +
>> > + virBufferAsprintf(&buf, "%u=%s;", cache, mask_str);
>> > + VIR_FREE(mask_str);
>> > + }
>> > +
>> > + virBufferTrim(&buf, ";", 1);
>> > + virBufferAddChar(&buf, '\n');
>> > + }
>> > + }
>> > +
>> > + virBufferCheckError(&buf);
>> > + return virBufferContentAndReset(&buf);
>>
>> Joy - some day it'd be nice to get back to the code that was going to
>> combine the CheckError and ContentAndReset... In the meantime, once this
>> is pushed I'll have another Coverity whine to filter (since CheckError
>> doesn't check status like 217 out of 220 calls do).
>>
Yeah, agree. Maybe write it as a GSoC idea? ;)
>> > +}
>> > +
>> > +
>> > +static int
>> > +virResctrlAllocParseProcessCache(virResctrlAllocPtr resctrl,
>> > + unsigned int level,
>> > + virCacheType type,
>> > + char *cache)
>> > +{
>> > + char *tmp = strchr(cache, '=');
>> > + unsigned int cache_id = 0;
>> > + virBitmapPtr mask = NULL;
>> > + int ret = -1;
>> > +
>> > + if (!tmp)
>> > + return 0;
>> > +
>> > + *tmp = '\0';
>> > + tmp++;
>> > +
>> > + if (virStrToLong_uip(cache, NULL, 10, &cache_id) < 0) {
>> > + virReportError(VIR_ERR_INTERNAL_ERROR,
>> > + _("Invalid cache id '%s'"), cache);
>> > + return -1;
>> > + }
>> > +
>> > + mask = virBitmapNewString(tmp);
>> > + if (!mask)
>> > + return -1;
>> > +
>> > + if (virResctrlAllocUpdateMask(resctrl, level, type, cache_id, mask) < 0)
>> > + goto cleanup;
>> > +
>> > + ret = 0;
>> > + cleanup:
>> > + virBitmapFree(mask);
>> > + return ret;
>> > +}
>> > +
>> > +
>> > +static int
>> > +virResctrlAllocParseProcessLine(virResctrlAllocPtr resctrl,
>> > + char *line)
>> > +{
>> > + char **caches = NULL;
>> > + char *tmp = NULL;
>> > + unsigned int level = 0;
>> > + int type = -1;
>> > + size_t ncaches = 0;
>> > + size_t i = 0;
>> > + int ret = -1;
>> > +
>> > + /* Skip lines that don't concern caches, e.g. MB: etc. */
>> > + if (line[0] != 'L')
>> > + return 0;
>> > +
>> > + /* And lines that we can't parse too */
>> > + tmp = strchr(line, ':');
>> > + if (!tmp)
>> > + return 0;
>> > +
>> > + *tmp = '\0';
>> > + tmp++;
>> > +
>> > + if (virStrToLong_uip(line + 1, &line, 10, &level) < 0) {
>> > + virReportError(VIR_ERR_INTERNAL_ERROR,
>> > + _("Cannot parse resctrl schema level '%s'"),
>> > + line + 1);
>> > + return -1;
>> > + }
>> > +
>> > + type = virResctrlTypeFromString(line);
>> > + if (type < 0) {
>> > + virReportError(VIR_ERR_INTERNAL_ERROR,
>> > + _("Cannot parse resctrl schema level '%s'"),
>> > + line + 1);
>> > + return -1;
>> > + }
>> > +
>> > + caches = virStringSplitCount(tmp, ";", 0, &ncaches);
>> > + if (!caches)
>> > + return 0;
>> > +
>> > + for (i = 0; i < ncaches; i++) {
>> > + if (virResctrlAllocParseProcessCache(resctrl, level, type, caches[i]) < 0)
>> > + goto cleanup;
>> > + }
>> > +
>> > + ret = 0;
>> > + cleanup:
>> > + virStringListFree(caches);
>> > + return ret;
>> > +}
>> > +
>> > +
>> > +static int
>> > +virResctrlAllocParse(virResctrlAllocPtr alloc,
>> > + const char *schemata)
>> > +{
>> > + char **lines = NULL;
>> > + size_t nlines = 0;
>> > + size_t i = 0;
>> > + int ret = -1;
>> > +
>> > + lines = virStringSplitCount(schemata, "\n", 0, &nlines);
>> > + for (i = 0; i < nlines; i++) {
>> > + if (virResctrlAllocParseProcessLine(alloc, lines[i]) < 0)
>> > + goto cleanup;
>> > + }
>> > +
>> > + ret = 0;
>> > + cleanup:
>> > + virStringListFree(lines);
>> > + return ret;
>> > +}
>> > +
>> > +
>> > +static void
>> > +virResctrlAllocSubtractPerType(virResctrlAllocPerTypePtr a,
>> > + virResctrlAllocPerTypePtr b)
>> > +{
>> > + size_t i = 0;
>> > +
>> > + if (!a || !b)
>> > + return;
>> > +
>> > + for (i = 0; i < a->nmasks && i < b->nmasks; ++i) {
>> > + if (a->masks[i] && b->masks[i])
>> > + virBitmapSubtract(a->masks[i], b->masks[i]);
>> > + }
>>
>> Does [n]sizes affect anything? IOW: does it matter.. probably not, but
>> my brain is overloaded right now!
>>
Will add more info.
>> > +}
>> > +
>> > +
>> > +static void
>> > +virResctrlAllocSubtract(virResctrlAllocPtr a,
>> > + virResctrlAllocPtr b)
>>
>> perhaps easier to "think about" if using dst and src... here and above.
>>
>> > +{
>> > + size_t i = 0;
>> > + size_t j = 0;
>> > +
>> > + if (!b)
>> > + return;
>> > +
>> > + for (i = 0; i < a->nlevels && b->nlevels; ++i) {
>>
>> b->nlevels is not a boolean or pointer... should be "i < " too right?
>>
>> Any special reason to use ++i over i++?
>>
>>
>> > + if (a->levels[i] && b->levels[i]) {
>> > + /* Here we rely on all the system allocations to use the same types.
>> > + * We kind of _hope_ it's the case. If this is left here until the
>> > + * review and someone finds it, then suggest only removing this last
>> > + * sentence. */
>>
>> Your golden egg has been discovered and I agree, how do we know? and of
>> course what role does CheckCollision (eventually) play?
>>
>> > + for (j = 0; j < VIR_CACHE_TYPE_LAST; j++) {
>> > + virResctrlAllocSubtractPerType(a->levels[i]->types[j],
>> > + b->levels[i]->types[j]);
>> > + }
>> > + }
>> > + }
>> > +}
>> > +
>> > +
>> > +static virResctrlAllocPtr
>> > +virResctrlAllocNewFromInfo(virResctrlInfoPtr info)
>> > +{
>> > + size_t i = 0;
>> > + size_t j = 0;
>> > + size_t k = 0;
>> > + virResctrlAllocPtr ret = virResctrlAllocNew();
>> > + virBitmapPtr mask = NULL;
>> > +
>> > + if (!ret)
>> > + return NULL;
>> > +
>> > + for (i = 0; i < info->nlevels; i++) {
>> > + virResctrlInfoPerLevelPtr i_level = info->levels[i];
>> > +
>> > + if (!i_level)
>> > + continue;
>> > +
>> > + for (j = 0; j < VIR_CACHE_TYPE_LAST; j++) {
>> > + virResctrlInfoPerTypePtr i_type = i_level->types[j];
>> > +
>> > + if (!i_type)
>> > + continue;
>> > +
>> > + virBitmapFree(mask);
>> > + mask = virBitmapNew(i_type->bits);
>> > + if (!mask)
>> > + goto error;
>> > + virBitmapSetAll(mask);
>> > +
>> > + for (k = 0; k <= i_type->max_cache_id; k++) {
>> > + if (virResctrlAllocUpdateMask(ret, i, j, k, mask) < 0)
>> > + goto error;
>> > + }
>> > + }
>> > + }
>> > +
>> > + cleanup:
>> > + virBitmapFree(mask);
>> > + return ret;
>> > + error:
>> > + virObjectUnref(ret);
>> > + ret = NULL;
>> > + goto cleanup;
>> > +}
>> > +
>> > +
>>
>> This needs a function header - args, what does it do, etc.
>>
>> > +virResctrlAllocPtr
>> > +virResctrlAllocGetFree(virResctrlInfoPtr resctrl)
>>
>> So far only ever called from virresctrl.c and from a static function, so
>> is it really necessary to be external?
>>
>> It's also really "odd" to have Free in the name and this isn't
>> necessarily a Free type function - far from it! Perhaps Unused is closer
>> to reality with the usage of the Subtract calls. Of course that could
>> affect variable names selected already...
>>
>> > +{
>> > + virResctrlAllocPtr ret = NULL;
>> > + virResctrlAllocPtr alloc = NULL;
>> > + virBitmapPtr mask = NULL;
>>
>> @mask is unused in this context except for the virBitmapFree
>>
Oh, good catch.
>> > + struct dirent *ent = NULL;
>> > + DIR *dirp = NULL;
>> > + char *schemata = NULL;
>> > + int rv = -1;
>> > +
>> > + if (virResctrlInfoIsEmpty(resctrl)) {
>> > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> > + _("Resource control is not supported on this host"));
>> > + return NULL;
>> > + }
>> > +
>> > + ret = virResctrlAllocNewFromInfo(resctrl);
>> > + if (!ret)
>> > + return NULL;
>> > +
>> > + if (virFileReadValueString(&schemata,
>> > + SYSFS_RESCTRL_PATH
>> > + "/schemata") < 0) {
>> > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> > + _("Could not read schemata file for the default group"));
>> > + goto error;
>> > + }
>>
>> Shall I assume that schemata is a CAT specific file? So is "default
>> group" the right wording? Just seems strange - consistently used
>> though... If changed you'd have multiple changes. I guess something is
>> my mind is wanting to associate it with a cgroup.
>>
Yes it is. I believe I sent a link to the documentation few times, but
I always forgot to add the link to the commit message, I guess. I'll
make sure I double check I added it next time.
>> > +
>> > + alloc = virResctrlAllocNew();
>> > + if (!alloc)
>> > + goto error;
>> > +
>> > + if (virResctrlAllocParse(alloc, schemata) < 0)
>> > + goto error;
>> > + if (virResctrlAllocIsEmpty(alloc)) {
>> > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> > + _("No schemata for default resctrlfs group"));
>> > + goto error;
>> > + }
>> > + virResctrlAllocSubtract(ret, alloc);
>> > +
>> > + if (virDirOpen(&dirp, SYSFS_RESCTRL_PATH) < 0)
>> > + goto error;
>> > +
>> > + while ((rv = virDirRead(dirp, &ent, SYSFS_RESCTRL_PATH)) > 0) {
>> > + if (ent->d_type != DT_DIR)
>> > + continue;
>> > +
>> > + if (STREQ(ent->d_name, "info"))
>> > + continue;
>> > +
>> > + VIR_FREE(schemata);
>> > + if (virFileReadValueString(&schemata,
>> > + SYSFS_RESCTRL_PATH
>> > + "/%s/schemata",
>> > + ent->d_name) < 0) {
>> > + virReportError(VIR_ERR_INTERNAL_ERROR,
>> > + _("Could not read schemata file for group %s"),
>> > + ent->d_name);
>> > + goto error;
>> > + }
>> > +
>> > + virObjectUnref(alloc);
>> > + alloc = virResctrlAllocNew();
>> > + if (!alloc)
>> > + goto error;
>> > +
>> > + if (virResctrlAllocParse(alloc, schemata) < 0)
>> > + goto error;
>> > +
>> > + virResctrlAllocSubtract(ret, alloc);
>> > +
>> > + VIR_FREE(schemata);
>>
>> I think this one could be duplicitous
>>
>> > + }
>> > + if (rv < 0)
>> > + goto error;
>> > +
>> > + cleanup:
>> > + virObjectUnref(alloc);
>> > + VIR_DIR_CLOSE(dirp);
>> > + VIR_FREE(schemata);
>> > + virBitmapFree(mask);
>> > + return ret;
>> > +
>> > + error:
>> > + virObjectUnref(ret);
>> > + ret = NULL;
>> > + goto cleanup;
>> > +}
>> > +
>> > +
>>
>> Could use a short (haha) description for all the magic that happens here!
>>
>> > +static int
>> > +virResctrlAllocMasksAssign(virResctrlInfoPtr r_info,
>>
>> s/r_info/resctrl/ for consistency?
>>
>> > + virResctrlAllocPtr alloc)
>> > +{
>> > + 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;
>> > + }
>>
>> Caller already checks this... and it's a static function...
>>
>> > +
>> > + alloc_free = virResctrlAllocGetFree(r_info);
>> > + 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;
>> > + }
>> > +
>>
>> large wet piles of brain matter all over my keyboard for this following
>> loop. It is 100% not self documenting - I'm not sure WTF is going on.
>
>I have to agree that following this code is extremely hard,
>perhaps splitting it into several functions would help to review
>this madness :).
>
I'll extract into functions. Even though I was told to open-code some
other functions in previous versions.
>> > + for (cache = 0; cache < a_type->nsizes; cache++) {
>> > + unsigned long long *size = a_type->sizes[cache];
>> > + virBitmapPtr a_mask = NULL;
>> > + virBitmapPtr f_mask = NULL;
>> > + 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);
>>
>> Coverity let me know @a_mask needs to be checked vs. NULL
>>
>> > + for (i = last_pos; i < last_pos + need_bits; i++) {
>> > + ignore_value(virBitmapSetBit(a_mask, i));
>> > + ignore_value(virBitmapClearBit(f_mask, i));
>> > + }
>> > +
>> > + if (a_type->nmasks <= cache) {
>> > + if (VIR_EXPAND_N(a_type->masks, a_type->nmasks,
>> > + cache - a_type->nmasks + 1) < 0) {
>> > + virBitmapFree(a_mask);
>> > + goto cleanup;
>> > + }
>> > + }
>> > + a_type->masks[cache] = a_mask;
>> > + }
>> > + }
>> > + }
>> > +
>> > + ret = 0;
>> > + cleanup:
>> > + virObjectUnref(alloc_free);
>> > + return ret;
>> > +}
>> > +
>> > +
>> > +/* This checks if the directory for the alloc exists. If not it tries to create
>> > + * it and apply appropriate alloc settings. */
>>
>> An overly simplified description for all that happens here.
>>
>> > +int
>> > +virResctrlAllocCreate(virResctrlInfoPtr r_info,
>>
>> s/r_info/resctrl/ for consistency?
>>
>> > + virResctrlAllocPtr alloc,
>> > + const char *drivername,
>> > + const char *machinename)
>> > +{
>> > + char *schemata_path = NULL;
>> > + char *alloc_str = NULL;
>> > + int ret = -1;
>> > + int lockfd = -1;
>> > +
>> > + if (!alloc)
>> > + return 0;
>> > +
>> > + if (!r_info) {
>> > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> > + _("Resource control is not supported on this host"));
>> > + return -1;
>> > + }
>> > +
>> > + if (!alloc->path &&
>> > + virAsprintf(&alloc->path, "%s/%s-%s-%s",
>> > + SYSFS_RESCTRL_PATH, drivername, machinename, alloc->id) < 0)
>> > + return -1;
>>
>> FWIW: From one of the .0 responses, this is where you get that
>> "qemu-qemu..." as @drivername is "qemu". I think the @drivername could
>> be superfluous, but it's not 100% clear what the options from the
>> systemd calls to build @machinename are...
>>
Yeah, already fixed in the next version.
>> In any case, if I'm reading things correctly it seems we would be saving
>> the guest data in the /sys/fs/resctrl path - is that the "right" place?
>> I can see for Info (e.g. host) level data that's read, sure, but for
>> guest (or Alloc) data that's written (and read) it's less clear in my
>> mind. Worried a bit about polluting /sys, some permissions issues
>> there, and a bit of future proofing - similar to cgroup layouts which
>> haven't remained constant through time. It would seem guests using this
>> would need to be sufficiently privileged or essentially not a session
>> guest...
>>
>> Shouldn't things go in the guest libDir? IOW: vm->privateData->libDir
>> that could be passed from qemuProcessResctrlCreate and avoid replicating
>> the build-up logic here?
>>
>> I would think avoiding /sys for guest data would probably be a good
>> idea. If the host dies how do we clean up after ourselves? At least if
>> we use guest directories, we can 'easily' document how to clean up.
>
>As I've wrote, we need to write to /sys/fs/resctrl in order to apply
>the cache allocation for a guest. This is not saving the "guest data"
>anywhere, that's already done by having it configured in the guest XML.
>
>I was able to find some documentation:
>
><https://github.com/intel/intel-cmt-cat/wiki/resctrl>
>
>Pavel
>
>> > +
>> > + /* Check if this allocation was already created */
>> > + if (virFileIsDir(alloc->path))
>> > + return 0;
>> > +
>> > + if (virFileExists(alloc->path)) {
>> > + virReportError(VIR_ERR_INTERNAL_ERROR,
>> > + _("Path '%s' for resctrl allocation exists and is not a "
>> > + "directory"), alloc->path);
>> > + goto cleanup;
>> > + }
>> > +
>> > + lockfd = virResctrlLockWrite();
>> > + if (lockfd < 0)
>> > + goto cleanup;
>> > +
>> > + if (virResctrlAllocMasksAssign(r_info, alloc) < 0)
>> > + goto cleanup;
>> > +
>> > + alloc_str = virResctrlAllocFormat(alloc);
>> > + if (!alloc_str)
>> > + goto cleanup;
>> > +
>>
>> There is a *lot* of magic going on here regarding how the above calls
>> will end up generating alloc_str and then how this schemata file gets
>> populated with data that I really think is not self documenting.
>>
>> > + if (virAsprintf(&schemata_path, "%s/schemata", alloc->path) < 0)
>> > + goto cleanup;
>> > +
>> > + if (virFileMakePath(alloc->path) < 0) {
>> > + virReportSystemError(errno,
>> > + _("Cannot create resctrl directory '%s'"),
>> > + alloc->path);
>> > + goto cleanup;
>> > + }
>> > +
>> > + if (virFileWriteStr(schemata_path, alloc_str, 0) < 0) {
>> > + rmdir(alloc->path);
>>
>> open coded virResctrlAllocRemove... could go with virFileDeleteTree too
>>
a) not really, I need different error message here
b) could not, it's a special filesystem (as explained before)
>> > + virReportSystemError(errno,
>> > + _("Cannot write into schemata file '%s'"),
>> > + schemata_path);
>> > + goto cleanup;
>> > + }
>> > +
>> > + ret = 0;
>> > + cleanup:
>> > + virResctrlUnlock(lockfd);
>> > + VIR_FREE(alloc_str);
>> > + VIR_FREE(schemata_path);
>> > + return ret;
>> > +}
>> > +
>> > +
>>
>> Might be nice to describe what this is for... Why does this need to be
>> generated/saved?
>>
>> > +int
>> > +virResctrlAllocAddPID(virResctrlAllocPtr alloc,
>> > + pid_t pid)
>> > +{
>> > + char *tasks = NULL;
>> > + char *pidstr = NULL;
>> > + int ret = 0;
>> > +
>> > + if (!alloc->path) {
>> > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> > + _("Cannot add pid to non-existing resctrl allocation"));
>> > + return -1;
>> > + }
>> > +
>>
>> Should we do any locking here?
>>
No need to, we're not changing schemata.
>> > + if (virAsprintf(&tasks, "%s/tasks", alloc->path) < 0)
>> > + return -1;
>> > +
>> > + if (virAsprintf(&pidstr, "%lld", (long long int) pid) < 0)
>> > + goto cleanup;
>> > +
>> > + if (virFileWriteStr(tasks, pidstr, 0) < 0) {
>> > + virReportSystemError(errno,
>> > + _("Cannot write pid in tasks file '%s'"),
>> > + tasks);
>> > + goto cleanup;
>> > + }
>>
>> We only ever write to this file a @pidstr, what happens when the vCPU is
>> unplugged? Can the cachetunes be updated in that manner?
>>
Hotplug and unplug needs to be fixed, yes.
>> > +
>> > + ret = 0;
>> > + cleanup:
>> > + VIR_FREE(tasks);
>> > + VIR_FREE(pidstr);
>> > + return ret;
>> > +}
>> > +
>> > +
>> > +int
>> > +virResctrlAllocRemove(virResctrlAllocPtr alloc)
>> > +{
>> > + int ret = 0;
>> > +
>> > + if (!alloc->path)
>> > + return 0;
>> > +
>> > + VIR_DEBUG("Removing resctrl allocation %s", alloc->path);
>> > + if (rmdir(alloc->path) != 0 && errno != ENOENT) {
>>
>> Is it only one level of directory or multiple? e.g. would
>> virFileDeleteTree be better? In fact overall it may be better.
>>
>> I hope I found everything ;-)
>>
>> John
>>
>> > + ret = -errno;
>> > + VIR_ERROR(_("Unable to remove %s (%d)"), alloc->path, errno);
>> > + }
>> > +
>> > + return ret;
>> > +}
>> > diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
>> > index 471c02f28dcd..0fbd9dd3acfb 100644
>> > --- a/src/util/virresctrl.h
>> > +++ b/src/util/virresctrl.h
>> > @@ -68,4 +68,66 @@ virResctrlInfoGetCache(virResctrlInfoPtr resctrl,
>> > size_t *ncontrols,
>> > virResctrlInfoPerCachePtr **controls);
>> >
>> > +/* Alloc-related things */
>> > +typedef struct _virResctrlAlloc virResctrlAlloc;
>> > +typedef virResctrlAlloc *virResctrlAllocPtr;
>> > +
>> > +typedef int virResctrlAllocForeachSizeCallback(unsigned int level,
>> > + virCacheType type,
>> > + unsigned int cache,
>> > + unsigned long long size,
>> > + void *opaque);
>> > +
>> > +virResctrlAllocPtr
>> > +virResctrlAllocNew(void);
>> > +
>> > +bool
>> > +virResctrlAllocIsEmpty(virResctrlAllocPtr resctrl);
>> > +
>> > +int
>> > +virResctrlAllocUpdateSize(virResctrlAllocPtr resctrl,
>> > + unsigned int level,
>> > + virCacheType type,
>> > + unsigned int cache,
>> > + unsigned long long size);
>> > +
>> > +int
>> > +virResctrlAllocUpdateMask(virResctrlAllocPtr resctrl,
>> > + unsigned int level,
>> > + virCacheType type,
>> > + unsigned int cache,
>> > + virBitmapPtr mask);
>> > +
>> > +int
>> > +virResctrlAllocSetSize(virResctrlAllocPtr resctrl,
>> > + unsigned int level,
>> > + virCacheType type,
>> > + unsigned int cache,
>> > + unsigned long long size);
>> > +
>> > +int
>> > +virResctrlAllocForeachSize(virResctrlAllocPtr resctrl,
>> > + virResctrlAllocForeachSizeCallback cb,
>> > + void *opaque);
>> > +
>> > +int
>> > +virResctrlAllocSetID(virResctrlAllocPtr alloc,
>> > + const char *id);
>> > +
>> > +char *
>> > +virResctrlAllocFormat(virResctrlAllocPtr alloc);
>> > +
>> > +int
>> > +virResctrlAllocCreate(virResctrlInfoPtr r_info,
>> > + virResctrlAllocPtr alloc,
>> > + const char *drivername,
>> > + const char *machinename);
>> > +
>> > +int
>> > +virResctrlAllocAddPID(virResctrlAllocPtr alloc,
>> > + pid_t pid);
>> > +
>> > +int
>> > +virResctrlAllocRemove(virResctrlAllocPtr alloc);
>> > +
>> > #endif /* __VIR_RESCTRL_H__ */
>> > diff --git a/src/util/virresctrlpriv.h b/src/util/virresctrlpriv.h
>> > new file mode 100644
>> > index 000000000000..932ff7329c8c
>> > --- /dev/null
>> > +++ b/src/util/virresctrlpriv.h
>> > @@ -0,0 +1,27 @@
>> > +/*
>> > + * virresctrlpriv.h:
>> > + *
>> > + * This library is free software; you can redistribute it and/or
>> > + * modify it under the terms of the GNU Lesser General Public
>> > + * License as published by the Free Software Foundation; either
>> > + * version 2.1 of the License, or (at your option) any later version.
>> > + *
>> > + * This library is distributed in the hope that it will be useful,
>> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>> > + * Lesser General Public License for more details.
>> > + *
>> > + * You should have received a copy of the GNU Lesser General Public
>> > + * License along with this library. If not, see
>> > + * <http://www.gnu.org/licenses/>.
>> > + */
>> > +
>> > +#ifndef __VIR_RESCTRL_PRIV_H__
>> > +# define __VIR_RESCTRL_PRIV_H__
>> > +
>> > +# include "virresctrl.h"
>> > +
>> > +virResctrlAllocPtr
>> > +virResctrlAllocGetFree(virResctrlInfoPtr resctrl);
>> > +
>> > +#endif /* __VIR_RESCTRL_PRIV_H__ */
>> >
>>
>> --
>> libvir-list mailing list
>> libvir-list at redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180122/4d39532c/attachment-0001.sig>
More information about the libvir-list
mailing list