[libvirt] [PATCH 05/10] util: resctrl: refactoring some functions
John Ferlan
jferlan at redhat.com
Wed Sep 5 14:48:45 UTC 2018
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.
> 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
> + 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.
I'd prefer to see the same argument order as being printed too...
> {
> - 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.
> 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.
>
> return 0;
Seems to me rather than passing &alloc->path, this function could return
@path and the caller then be able to "handle" that.
For the "first pass" before @root and @parentpath are added, using:
ignore_value(virAsprintf(&path, "%s/%s-%s",
rootpath, prefix, id));
return path;
> }
>
>
> +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;
> +}
> +
should be two blank lines between and this could use a comment
describing what it's doing and what it's assumptions are.
> +static int
> +virResctrlCreateGroup(virResctrlInfoPtr resctrl,
> + char *path)
s/char/const char/
should be:
virResctrlCreateGroupPath
> +{
> + 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.
> +
> + 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. 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.
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.
> +
> + if (STREQ(path, SYSFS_RESCTRL_PATH))
> + return 0;
This concept doesn't appear until the next patch, so we cannot introduce
it yet.
> +
> + 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.
> +
> + 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.
> +}
> +
> +
> /* 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.
>
> - 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.
> @@ -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
> -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