[libvirt] [PATCH 16/21] resctrl: Add functions to work with resctrl allocations
Pavel Hrdina
phrdina at redhat.com
Tue Nov 21 11:40:06 UTC 2017
On Tue, Nov 21, 2017 at 10:49:29AM +0100, Martin Kletzander wrote:
> 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 at redhat.com>
> > > ---
> > > 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 :)
I opened the patch in vim and configured to display TAB and there are
two TABs, so please double check it in your editor :).
> [...]
>
> > > @@ -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.
So the allocation happens in these public functions:
* virResctrlAllocNewFromInfo()
- There you know based on Info whether you need to allocate new
object or not. This is probably the only case where the automatic
allocation helps a little bit.
* virResctrlAllocSetSize()
- This is indirectly called from virDomainCachetuneDefParse() where
you know if there is any cache element to parse so you can
allocate new object if there is anything to set.
So there shouldn't be any need to have a function that will check the
allocation. Otherwise I wouldn't have suggested this modification.
>
> [...]
>
> > > +
> > > +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.
I think that you can decide to merge the functions or not based on the
resulting code after it is rewritten. It's not requirement, more like
suggestion based on what I was imagine how the new code could look like.
>
> [...]
>
> > > +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).
Right, if it makes sense to use it as standalone function the error
message should probably stay here.
>
> > > +
> > > + 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 ...
I understand that it saves current state so you don't have to parse it
again but from the patch series it looks like it is used only for
tests. So the question about speed was only related to tests where
you use this optimization.
Pavel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20171121/12f3683a/attachment-0001.sig>
More information about the libvir-list
mailing list