Re: [PATCH 05/10] util: resctrl: refactoring some functions

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@xxxxxxxxx>
> ---
>  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;
> 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux