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&lt;huaqiang.wang(a)intel.com&gt; > --- > 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__ */ >