[libvirt] [PATCH 05/10] util: resctrl: refactoring some functions
Wang, Huaqiang
huaqiang.wang at intel.com
Fri Sep 7 10:52:07 UTC 2018
> -----Original Message-----
> From: John Ferlan [mailto:jferlan at redhat.com]
> Sent: Wednesday, September 5, 2018 10:49 PM
> To: Wang, Huaqiang <huaqiang.wang at intel.com>; libvir-list at redhat.com
> Cc: Feng, Shaohe <shaohe.feng at intel.com>; Niu, Bing <bing.niu at intel.com>;
> Ding, Jian-feng <jian-feng.ding at intel.com>; Zang, Rui <rui.zang at intel.com>
> Subject: Re: [libvirt] [PATCH 05/10] util: resctrl: refactoring some functions
>
>
>
> On 08/27/2018 07:23 AM, Wang Huaqiang wrote:
> > Some code, in virresctrl.c, manupulating the file objects of resctrlfs
> > could be reused for cache monitor interfaces. This patch refactor
> > these functions for purpose of reusing code in later patch:
> >
> > virResctrlAllocDeterminePath
> > virResctrlAllocCreate
> > virResctrlAddPID
> >
> > Signed-off-by: Wang Huaqiang <huaqiang.wang at intel.com>
> > ---
> > src/util/virresctrl.c | 126
> > +++++++++++++++++++++++++++++++++++++-------------
> > 1 file changed, 93 insertions(+), 33 deletions(-)
> >
>
> Yikes, 3 or more patches in one.
>
Will be split according to your suggestions.
> > diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index
> > 2f6923a..b3bae6e 100644
> > --- a/src/util/virresctrl.c
> > +++ b/src/util/virresctrl.c
> > @@ -2082,25 +2082,94 @@ virResctrlAllocAssign(virResctrlInfoPtr
> > resctrl, }
> >
> >
> > -int
> > -virResctrlAllocDeterminePath(virResctrlAllocPtr alloc,
> > - const char *machinename)
> > +static int
> > +virResctrlDeterminePath(const char *id,
> > + const char *root,
>
> Let's use @rootpath instead of @root
>
Will be fixed..
> > + const char *parentpath,
> > + const char *prefix,
> > + char **path)
>
> Take it slowly - round 1, convert virResctrlAllocDeterminePath into using
> virResctrlDeterminePath, but don't add the @parentpath yet since it's not
> "introduced" until later.
>
OK. Split into two steps/patches.
> I'd prefer to see the same argument order as being printed too...
>
OK. Will pay attention to the order.
> > {
> > - if (!alloc->id) {
> > + if (!id) {
> > virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > - _("Resctrl Allocation ID must be set before creation"));
> > + _("Resctrl resource ID must be set before
> > + creation"));
> > return -1;
> > }
> >
> > - if (!alloc->path &&
> > - virAsprintf(&alloc->path, "%s/%s-%s",
> > - SYSFS_RESCTRL_PATH, machinename, alloc->id) < 0)
> > + if (*path) {
> > + virReportError(VIR_ERR_INTERNAL_ERROR,
> > + _("Resctrl group (%s) already created, path=%s."),
> > + id, *path);
>
> The indent is off here w/ @id - need another space. Still is this a programming
> error or something else? Tough to tell since you're adding multiple things at one
> time.
Yes. Will add a space before @id.
Will follow your suggestion mention below, this function will take @path as a return
Value. No parameter of @path then, and no error message.
>
> > return -1;
> > + }
> > +
> > + if (!parentpath && !root) {
> > + if (virAsprintf(path, "%s/%s-%s",
> > + SYSFS_RESCTRL_PATH, prefix, id) < 0)
> > + return -1;
>
> and this is just the initial case...
>
> > + } else if (!parentpath) {
> > + if (virAsprintf(path, "%s/%s/%s-%s",
> > + SYSFS_RESCTRL_PATH, parentpath, prefix, id) < 0)
> > + return -1;
> > + } else {
> > + if (virAsprintf(path, "%s/%s/%s-%s",
> > + root, parentpath, prefix, id) < 0)
> > + return -1;
> > + }
>
> These are additional cases added later on, but used in this patch, so they need to
> "wait" to be added until we see "where" they come from.
Will be fixed.
>
> >
> > return 0;
>
> Seems to me rather than passing &alloc->path, this function could return @path
> and the caller then be able to "handle" that.
OK. Follow the suggestion, take @path as a return value.
>
> For the "first pass" before @root and @parentpath are added, using:
>
> ignore_value(virAsprintf(&path, "%s/%s-%s",
> rootpath, prefix, id));
>
> return path;
>
OK. Take your suggestion and will make change.
> > }
> >
> >
> > +int
> > +virResctrlAllocDeterminePath(virResctrlAllocPtr alloc,
> > + const char *machinename) {
> > + return virResctrlDeterminePath(alloc->id, NULL, NULL,
> > + machinename, &alloc->path);
>
> Thus this becomes:
>
> if (!(alloc->path = virResctrlDeterminePath(SYSFS_RESCTRL_PATH,
> machinename,
> alloc->id)))
> return -1;
>
> return 0;
>
Understand. Will be followed.
> > +}
> > +
>
> should be two blank lines between and this could use a comment describing
> what it's doing and what it's assumptions are.
>
Two blank lines here for coding style consistence, ok.
And add following comments to describe the functionality.
/*
* This helper creates the resctrl group by making the real directory in
* resctrl file system. @path is the directory path.
*/
> > +static int
> > +virResctrlCreateGroup(virResctrlInfoPtr resctrl,
> > + char *path)
>
> s/char/const char/
>
Will be fixed.
> should be:
>
> virResctrlCreateGroupPath
>
I prefer the original name ' virResctrlCreateGroup' than 'virResctrlCreateGroupPath'.
The main role of this function is to make a directory, and the directory is called
'resource group' in kernel's document. See document
https://www.kernel.org/doc/Documentation/x86/intel_rdt_ui.txt
If you still think 'virResctrlCreateGroupPath' is better, OK, let's change it.
> > +{
> > + int ret = -1;
> > + int lockfd = -1;
> > +
> > + if (!path)
> > + return -1;
>
> This would cause some sort of unknown error, but it's a caller bug isn't it? That
> is if @path is empty before calling in here, then we've missed some other
> condition, so in this instance it doesn't quite make sense.
>
OK. I need to pay more attention on these places that could cause 'unknown error'.
> > +
> > + if (virResctrlInfoIsEmpty(resctrl)) {
> > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > + _("Resource control is not supported on this host"));
> > + return -1;
> > + }
>
> Not quite sure what this has to do with creating the GroupPath.
Does 'this' mean the invoking of ' virResctrlInfoIsEmpty'?
virResctrlInfoIsEmpty return true ensures that the 'resctrl fs' is supported here.
>Feels like some
> that should be in the caller, but I guess that depends on future usage.... I see this
> helper is called in the next patch by virResctrlAllocCreateMonitor which isn't
> used until patch9 and only called once/if virResctrlAllocCreate is successful.
>
Awesome, your feeling is right.
My design is, virResctrlAllocCreate creates an resource 'allocation', and
virResctrlAllocCreateMonitor creates a resource 'monitor'. The 'monitor' belongs
to some specific 'allocation'. If you want to create a 'monitor', an 'allocation' must
be created already, and link the 'monitor' to the 'allocation'.
So when virResctrlAllocCreateMonitor is called, the virResctrlAllocCreate must
be called successfully.
And the ' virResctrlInfoIsEmpty' is checked more than one times.
Will move the call of virResctrlInfoIsEmpty into virResctrlAllocCreate.
> So it doesn't seem that calling it once for each time
> virResctrlAllocCreateMonitor is called is really necessary since @resctrl doesn't
> change.
>
> In fact, going back to qemuProcessResctrlCreate it would seem that calling
> virResctrlAllocCreate once for each vm->def->nresctrls would also be somewhat
> inefficient since caps->host.resctrl (a/k/a @resctrl) doesn't change. But moving
> it back there may mean needing to check if
> vm->def->resctrls[i]->alloc is NULL...
>
> I think perhaps some more thought needs to be placed on "efficient" code paths
> before adding the monitor code paths.
Confused. Do you still talking about the mult-call over function virResctrlInfoIsEmpty?
>
> > +
> > + if (STREQ(path, SYSFS_RESCTRL_PATH))
> > + return 0;
>
> This concept doesn't appear until the next patch, so we cannot introduce it yet.
>
OK. Will split the patch and make change accordingly.
> > +
> > + if (virFileExists(path)) {
> > + virReportError(VIR_ERR_INTERNAL_ERROR,
> > + _("Path '%s' for resctrl resource group exists"), path);
> > + goto cleanup;
> > + }
> > +
> > + lockfd = virResctrlLockWrite();
> > + if (lockfd < 0)
> > + goto cleanup;
>
> This Lock/Unlock sequence should be in the caller... and the fact that the lock
> should be taken documented as "expected" in the caller.
>
Will remove locker here and put locker in caller.
> > +
> > + if (virFileMakePath(path) < 0) {
> > + virReportSystemError(errno,
> > + _("Cannot create resctrl directory '%s'"), path);
> > + goto cleanup;
> > + }
> > +
> > + ret = 0;
> > + cleanup:
> > + virResctrlUnlock(lockfd);
> > + return ret;
>
> In the short term, @ret probably isn't needed - return 0 or -1 directly.
>
Will be fixed.
> > +}
> > +
> > +
> > /* This checks if the directory for the alloc exists. If not it tries to create
> > * it and apply appropriate alloc settings. */ int @@ -2116,21
> > +2185,11 @@ virResctrlAllocCreate(virResctrlInfoPtr resctrl,
> > if (!alloc)
> > return 0;
> >
> > - if (virResctrlInfoIsEmpty(resctrl)) {
> > - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > - _("Resource control is not supported on this host"));
> > - return -1;
> > - }
> > -
> > if (virResctrlAllocDeterminePath(alloc, machinename) < 0)
> > return -1;
>
> If we return from this and alloc->path == NULL, there's a coding error, so I see
> no reason in virResctrlCreateGroupPath that we'd need to validate that (at least
> yet). It's a static helper and should be called only when your expected conditions
> are right.
>
Got. Will pay attention to places that will generate 'unkown error's.
> >
> > - if (virFileExists(alloc->path)) {
> > - virReportError(VIR_ERR_INTERNAL_ERROR,
> > - _("Path '%s' for resctrl allocation exists"),
> > - alloc->path);
> > - goto cleanup;
> > - }
> > + if (virResctrlCreateGroup(resctrl, alloc->path) < 0)
> > + return -1;
> >
> > lockfd = virResctrlLockWrite();
> > if (lockfd < 0)
>
> The call to virResctrlCreateGroupPath should come after this rather than
> Lock/Unlock when creating the directory and then Lock/Unlock again when
> writing to the file. I think it's all one autonomous operation.
OK.
>
> > @@ -2146,13 +2205,6 @@ virResctrlAllocCreate(virResctrlInfoPtr resctrl,
> > 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;
> > - }
> > -
> > VIR_DEBUG("Writing resctrl schemata '%s' into '%s'", alloc_str,
> schemata_path);
> > if (virFileWriteStr(schemata_path, alloc_str, 0) < 0) {
> > rmdir(alloc->path);
> > @@ -2171,21 +2223,21 @@ virResctrlAllocCreate(virResctrlInfoPtr
> > resctrl, }
> >
> >
>
> The next hunk is fine as long as it is a single patch.
>
> John
>
Thanks for review.
Huaqiang
> > -int
> > -virResctrlAllocAddPID(virResctrlAllocPtr alloc,
> > - pid_t pid)
> > +static int
> > +virResctrlAddPID(char *path,
> > + pid_t pid)
> > {
> > char *tasks = NULL;
> > char *pidstr = NULL;
> > int ret = 0;
> >
> > - if (!alloc->path) {
> > + if (!path) {
> > virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > - _("Cannot add pid to non-existing resctrl allocation"));
> > + _("Cannot add pid to non-existing resctrl
> > + group"));
> > return -1;
> > }
> >
> > - if (virAsprintf(&tasks, "%s/tasks", alloc->path) < 0)
> > + if (virAsprintf(&tasks, "%s/tasks", path) < 0)
> > return -1;
> >
> > if (virAsprintf(&pidstr, "%lld", (long long int) pid) < 0) @@
> > -2207,6 +2259,14 @@ virResctrlAllocAddPID(virResctrlAllocPtr alloc,
> >
> >
> > int
> > +virResctrlAllocAddPID(virResctrlAllocPtr alloc,
> > + pid_t pid)
> > +{
> > + return virResctrlAddPID(alloc->path, pid); }
> > +
> > +
> > +int
> > virResctrlAllocRemove(virResctrlAllocPtr alloc) {
> > int ret = 0;
> >
More information about the libvir-list
mailing list