<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body text="#000000" bgcolor="#FFFFFF"> 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 <div class="moz-cite-prefix">On 11/6/2018 3:07 AM, John Ferlan wrote: </div> <blockquote type="cite" cite="mid:d5b0d620-049e-ab5d-7769-85f26acb5181@redhat.com"> <pre wrap=""> On 10/22/18 4:01 AM, Wang Huaqiang wrote: </pre> <blockquote type="cite"> <pre wrap="">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 <a class="moz-txt-link-rfc2396E" href="mailto:huaqiang.wang@intel.com"><huaqiang.wang@intel.com></a> --- 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; </pre> </blockquote> <pre wrap="">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? </pre> <blockquote type="cite"> <pre wrap=""> } @@ -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; + </pre> </blockquote> <pre wrap="">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. </pre> <blockquote type="cite"> <pre wrap="">+ /* 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; </pre> </blockquote> <pre wrap="">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). </pre> <blockquote type="cite"> <pre wrap="">+ + /* 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; + } </pre> </blockquote> <pre wrap="">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. </pre> <blockquote type="cite"> <pre wrap="">+ + 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; + } </pre> </blockquote> <pre wrap="">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 </pre> <blockquote type="cite"> <pre wrap="">+ + 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__ */ </pre> </blockquote> </blockquote> </body> </html>