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

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



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 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__ */



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