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

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&gt; > > --- > > 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 ...