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
---
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 and the same pid added
by a 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)
Re: [libvirt] [PATCHv7 14/18] util: Add function for checking if monitor is running
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
> ---
> 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 and the same pid added
by a 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 r
[libvirt] [PATCHv7 14/18] util: Add function for checking if monitor is running
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
---
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;
}
@@ -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;
+
+/* 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;
+
+/* 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;
+}
+
+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;
+}
+
+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__ */
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
