[libvirt] [PATCHv7 14/18] util: Add function for checking if monitor is running

[John Ferlan]

On 10/22/18 4:01 AM, Wang Huaqiang wrote:
> Check whether monitor is running by checking the monitor's PIDs status.
> 
> Monitor is looked as running normally if the vcpu PID list matches with
> the content of monitor's 'tasks' file.
> 
> Signed-off-by: Wang Huaqiang <huaqiang.wang at intel.com>
> ---
>  src/libvirt_private.syms |   1 +
>  src/util/virresctrl.c    | 102 ++++++++++++++++++++++++++++++++++++++++++++++-
>  src/util/virresctrl.h    |   3 ++
>  3 files changed, 105 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index d59ac86..91801ed 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2684,6 +2684,7 @@ virResctrlMonitorAddPID;
>  virResctrlMonitorCreate;
>  virResctrlMonitorDeterminePath;
>  virResctrlMonitorGetID;
> +virResctrlMonitorIsRunning;
>  virResctrlMonitorNew;
>  virResctrlMonitorRemove;
>  virResctrlMonitorSetAlloc;
> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
> index b0205bc..fa3e6e9 100644
> --- a/src/util/virresctrl.c
> +++ b/src/util/virresctrl.c
> @@ -359,6 +359,9 @@ struct _virResctrlMonitor {
>      /* libvirt-generated path in /sys/fs/resctrl for this particular
>       * monitor */
>      char *path;
> +    /* Tracking the tasks' PID associated with this monitor */
> +    pid_t *pids;
> +    size_t npids;
>  };
>  
>  
> @@ -418,6 +421,7 @@ virResctrlMonitorDispose(void *obj)
>      virObjectUnref(monitor->alloc);
>      VIR_FREE(monitor->id);
>      VIR_FREE(monitor->path);
> +    VIR_FREE(monitor->pids);
>  }
>  
>  
> @@ -2540,7 +2544,20 @@ int
>  virResctrlMonitorAddPID(virResctrlMonitorPtr monitor,
>                          pid_t pid)
>  {
> -    return virResctrlAddPID(monitor->path, pid);
> +    size_t i = 0;
> +
> +    if (virResctrlAddPID(monitor->path, pid) < 0)
> +        return -1;
> +
> +    for (i = 0; i < monitor->npids; i++) {
> +        if (pid == monitor->pids[i])
> +            return 0;
> +    }
> +
> +    if (VIR_APPEND_ELEMENT(monitor->pids, monitor->npids, pid) < 0)
> +        return -1;
> +
> +    return 0;

could just be a return VIR_APPEND_ELEMENT()

So we theoretically have our @pid in the task file (ResctrlAddPID) and
this internal list of pids which makes me wonder why other than
"quicker" lookup than opening the tasks file and looking for our pid, right?

But based on the next hunk for virResctrlMonitorIsRunning, I'm not so
sure. In fact I wonder why are we doing this?

>  }
>  
>  
> @@ -2613,3 +2630,86 @@ virResctrlMonitorRemove(virResctrlMonitorPtr monitor)
>  
>      return ret;
>  }
> +
> +
> +static int
> +virResctrlPIDSorter(const void *pida, const void *pidb)
> +{
> +    return *(pid_t*)pida - *(pid_t*)pidb;
> +}
> +
> +
> +bool
> +virResctrlMonitorIsRunning(virResctrlMonitorPtr monitor)
> +{
> +    char *pidstr = NULL;
> +    char **spids = NULL;
> +    size_t nspids = 0;
> +    pid_t *pids = NULL;
> +    size_t npids = 0;
> +    size_t i = 0;
> +    int rv = -1;
> +    bool ret = false;
> +

So this is a heavyweight type method and seems to me to do far more than
just determine if a monitor is running.  In fact, it seems it's
validating that the internal list of monitor->pids matches whatever is
in the *tasks file. There's multiple failure scenarios that may or may
not "mean" a monitor is or isn't running.

> +    /* path is not determined yet, monitor is not running*/
> +    if (!monitor->path)
> +        return false;
> +
> +    /* No vcpu PID filled, regard monitor as not running */
> +    if (monitor->npids == 0)
> +        return false;
> +
> +    /* If no 'tasks' file found, underlying resctrl directory is not
> +     * ready, regard monitor as not running */
> +    rv = virFileReadValueString(&pidstr, "%s/tasks", monitor->path);
> +    if (rv < 0)
> +        goto cleanup;

So we read the file, but while we're reading someone could have added to
it... There's some locking concerns here.

The *tasks file can have a pid added by a <cache> and the same pid added
by a <monitor> it seems at least from my reading of qemuProcessSetupVcpu
logic where virResctrlAllocAddPID would be called first followed by a
call to virResctrlMonitorAddPID. Neither checks if the pid exists before
writing (so yes more questions/concerns about patch 13 logic).

> +
> +    /* no PID in task file, monitor is not running */
> +    if (!*pidstr)
> +        goto cleanup;
> +
> +    /* The tracking monitor PID list is not identical to the
> +     * list in current resctrl directory. monitor is corrupted,
> +     * report error and un-running state */
> +    spids = virStringSplitCount(pidstr, "\n", 0, &nspids);
> +    if (nspids != monitor->npids) {
> +        VIR_ERROR(_("Monitor %s PID list mismatch in length"), monitor->path);
> +        goto cleanup;
> +    }

See this doesn't make sense - why have a lookaside list then?  Either
you trust what's in your file or you don't.

Having boolean logic return true/false where false can either be an
error generated or a truly false value just doesn't seem right.

> +
> +    for (i = 0; i < nspids; i++) {
> +        unsigned int val = 0;
> +        pid_t pid = 0;
> +
> +        if (virStrToLong_uip(spids[i], NULL, 0, &val) < 0) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Monitor %s failed in parse PID list"),
> +                           monitor->path);
> +            goto cleanup;
> +        }
> +
> +        pid = (pid_t)val;
> +
> +        if (VIR_APPEND_ELEMENT(pids, npids, pid) < 0)
> +            goto cleanup;
> +    }

So we're building this list and could have problems doing so.

I'm not sure this is "good" and why we need to do this.  Either keep an
internal list of pids or keep the list of pids in a file.

John

> +
> +    qsort(pids, npids, sizeof(pid_t), virResctrlPIDSorter);
> +    qsort(monitor->pids, monitor->npids, sizeof(pid_t), virResctrlPIDSorter);
> +
> +    for (i = 0; i < monitor->npids; i++) {
> +        if (monitor->pids[i] != pids[i]) {
> +            VIR_ERROR(_("Monitor %s PID list corrupted"), monitor->path);
> +            goto cleanup;
> +        }
> +    }
> +
> +    ret = true;
> + cleanup:
> +    virStringListFree(spids);
> +    VIR_FREE(pids);
> +    VIR_FREE(pidstr);
> +
> +    return ret;
> +}
> diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
> index 804d6f5..8d8fdc2 100644
> --- a/src/util/virresctrl.h
> +++ b/src/util/virresctrl.h
> @@ -219,4 +219,7 @@ virResctrlMonitorSetAlloc(virResctrlMonitorPtr monitor,
>  
>  int
>  virResctrlMonitorRemove(virResctrlMonitorPtr monitor);
> +
> +bool
> +virResctrlMonitorIsRunning(virResctrlMonitorPtr monitor);
>  #endif /*  __VIR_RESCTRL_H__ */
>