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