-----Original Message----- From: John Ferlan [mailto:jferlan@redhat.com] Sent: Wednesday, September 5, 2018 10:49 PM To: Wang, Huaqiang <huaqiang.wang@intel.com>; libvir-list@redhat.com Cc: Feng, Shaohe <shaohe.feng@intel.com>; Niu, Bing <bing.niu@intel.com>; Ding, Jian-feng <jian-feng.ding@intel.com>; Zang, Rui <rui.zang@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@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;