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

[Wang, Huaqiang]

This patch is added previously because I though the vcpupid would be 
changed during

libvirt re-connection, and if vcpupid is changed and libvirt is not 
aware of this change

it will make monitor working on an stale *tasks file and monitor will 
get wrong

cache information.

But the vcpuid will not change in process of libvirt re-connection, the 
*tasks file is always

tacking on right PIDs. So this patch is not necessary now, will be 
removed in next update

of patch series.


Thanks for review.

Huaqiang



On 11/6/2018 3:07 AM, John Ferlan wrote:
> 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__ */
>>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20181112/619ce73e/attachment-0001.htm>