Refactor schemas and virresctrl to support optional <cache> element in <cachetune>. Later, the monitor entry will be introduced and to be placed under <cachetune>. Either cache entry or monitor entry is an optional element of <cachetune>. An cachetune has no <cache> element is taking the default resource allocating policy defined in '/sys/fs/resctrl/schemata'. Signed-off-by: Wang Huaqiang <huaqiang.wang(a)intel.com&gt; --- docs/formatdomain.html.in | 4 ++-- docs/schemas/domaincommon.rng | 4 ++-- src/util/virresctrl.c | 28 ++++++++++++++++++++++++++++ 3 files changed, 32 insertions(+), 4 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 8189959..b1651e3 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -943,8 +943,8 @@ <dl> <dt><code>cache</code></dt> <dd> - This element controls the allocation of CPU cache and has the - following attributes: + This optional element controls the allocation of CPU cache and has + the following attributes: <dl> <dt><code>level</code></dt> <dd> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 099a949..5c533d6 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -956,7 +956,7 @@ <attribute name="vcpus"> <ref name='cpuset'/> </attribute> - <oneOrMore> + <zeroOrMore> <element name="cache"> <attribute name="id"> <ref name='unsignedInt'/> @@ -980,7 +980,7 @@ </attribute> </optional> </element> - </oneOrMore> + </zeroOrMore> </element> </zeroOrMore> <zeroOrMore> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 5d811a2..91bd341 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -234,6 +234,11 @@ virResctrlInfoMonFree(virResctrlInfoMonPtr mon) * in case there is no allocation for that particular cache allocation (level, * cache, ...) or memory allocation for particular node). * + * Allocation corresponding to root directory, /sys/fs/sysctrl/, defines the + * default resource allocating policy, which is created immediately after + * mounting, and owns all the tasks and cpus in the system. Cache or memory + * bandwidth resource will be shared for tasks in this allocation. + * * =====Cache allocation technology (CAT)===== * * Since one allocation can be made for caches on different levels, the first @@ -2215,6 +2220,15 @@ virResctrlAllocDeterminePath(virResctrlAllocPtr alloc, return -1; } + /* If the allocation is empty, then the path will be SYSFS_RESCTRL_PATH */ + if (virResctrlAllocIsEmpty(alloc)) { + if (!alloc->path && + VIR_STRDUP(alloc->path, SYSFS_RESCTRL_PATH) < 0) + return -1; + + return 0; + } + if (!alloc->path && virAsprintf(&alloc->path, "%s/%s-%s", SYSFS_RESCTRL_PATH, machinename, alloc->id) < 0) @@ -2248,6 +2262,11 @@ virResctrlAllocCreate(virResctrlInfoPtr resctrl, if (virResctrlAllocDeterminePath(alloc, machinename) < 0) return -1; + /* If the allocation is empty, then do not create directory in underlying + * resctrl file system */ + if (virResctrlAllocIsEmpty(alloc)) + return 0; + if (virFileExists(alloc->path)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Path '%s' for resctrl allocation exists"), @@ -2302,6 +2321,11 @@ virResctrlAllocAddPID(virResctrlAllocPtr alloc, char *pidstr = NULL; int ret = 0; + /* If the allocation is empty, then it is impossible to add a PID to + * allocation due to lacking of its 'tasks' file. Just return */ + if (virResctrlAllocIsEmpty(alloc)) + return 0; + if (!alloc->path) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Cannot add pid to non-existing resctrl allocation")); @@ -2334,6 +2358,10 @@ virResctrlAllocRemove(virResctrlAllocPtr alloc) { int ret = 0; + /* No directory have ever been created. Just return */ + if (virResctrlAllocIsEmpty(alloc)) + return 0; + if (!alloc->path) return 0; -- 2.7.4

On 2018年11月05日 23:01, John Ferlan wrote: Agree with the changes. "No need to create a directory that has already been created by system." (SYSFS_RESCTRL_PATH is the created directory). Thanks for the review. Huaqiang

On 11/6/18 3:30 AM, Huaqiang,Wang wrote: I'll add the comment: + /* Do not destroy if path is the system/default path for the allocation */ True - I'll adjust the check there too to be: + /* If using the system/default path for the allocation, then we're done */ + if (STREQ(alloc->path, SYSFS_RESCTRL_PATH)) + return 0; + Tks - John

On 10/22/18 4:01 AM, Wang Huaqiang wrote: [...] *resctrl Each ResctrlMonitor is associated... either the root directory allocation /sys/fs/resctrl or a specific allocation defined under the root directory. With these simple changes that I can make for you, Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John [...]

The code for determining resctrl allocation path could be reused for monitor. Refactor it for reuse. Signed-off-by: Wang Huaqiang <huaqiang.wang(a)intel.com&gt; --- src/util/virresctrl.c | 38 ++++++++++++++++++++++++++++++-------- 1 file changed, 30 insertions(+), 8 deletions(-) diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 6530801..956aca8 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -2265,28 +2265,50 @@ virResctrlAllocAssign(virResctrlInfoPtr resctrl, } +static char * +virResctrlDeterminePath(const char *parentpath, + const char *prefix, + const char *id) +{ + char *path = NULL; + + if (!id) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Resctrl ID must be set before determining resctrl " + "parentpath='%s'"), parentpath); + return NULL; + } + + if (virAsprintf(&path, "%s/%s-%s", parentpath, prefix, id) < 0) + return NULL; + + return path; +} + + int virResctrlAllocDeterminePath(virResctrlAllocPtr alloc, const char *machinename) { - if (!alloc->id) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Resctrl Allocation ID must be set before creation")); + if (alloc->path) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Resctrl allocation path is already set to '%s'"), + alloc->path); return -1; } /* If the allocation is empty, then the path will be SYSFS_RESCTRL_PATH */ if (virResctrlAllocIsEmpty(alloc)) { - if (!alloc->path && - VIR_STRDUP(alloc->path, SYSFS_RESCTRL_PATH) < 0) + if (VIR_STRDUP(alloc->path, SYSFS_RESCTRL_PATH) < 0) return -1; return 0; } - if (!alloc->path && - virAsprintf(&alloc->path, "%s/%s-%s", - SYSFS_RESCTRL_PATH, machinename, alloc->id) < 0) + alloc->path = virResctrlDeterminePath(SYSFS_RESCTRL_PATH, + machinename, alloc->id); + + if (!alloc->path) return -1; return 0; -- 2.7.4

On 2018年11月05日 23:02, John Ferlan wrote: Your change made the message be more clear.  Agree with your change. Thanks for your review. Huaqiang

On 10/22/18 4:01 AM, Wang Huaqiang wrote: Just ran the changes through Coverity - because of the above "monitor->alloc" check, Coverity notes right here that monitor->alloc could be NULL, so I think a check prior to here would be in order, such as either before or after the monitor->path check: if (!monitor->alloc) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing resctrl monitor allocation")); return -1; } Then the monitor->alloc check wouldn't be necessary. Thus the above STRDUP becomes: if (STREQ_NULLABLE(monitor->id, monitor->alloc->id)) { if (VIR_STRDUP(monitor->path, monitor->alloc->path) < 0) return -1; return 0; } Let me know where you want it. John

The code of adding PID to the allocation could be reused, refactor it for later reuse. Signed-off-by: Wang Huaqiang <huaqiang.wang(a)intel.com&gt; --- src/util/virresctrl.c | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 1d0eb01..5fc8772 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -2390,26 +2390,21 @@ virResctrlAllocCreate(virResctrlInfoPtr resctrl, } -int -virResctrlAllocAddPID(virResctrlAllocPtr alloc, - pid_t pid) +static int +virResctrlAddPID(const char *path, + pid_t pid) { char *tasks = NULL; char *pidstr = NULL; int ret = 0; - /* If the allocation is empty, then it is impossible to add a PID to - * allocation due to lacking of its 'tasks' file. Just return */ - if (virResctrlAllocIsEmpty(alloc)) - return 0; - - if (!alloc->path) { + if (!path) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Cannot add pid to non-existing resctrl allocation")); + _("Cannot add pid to non-existing resctrl group")); return -1; } - if (virAsprintf(&tasks, "%s/tasks", alloc->path) < 0) + if (virAsprintf(&tasks, "%s/tasks", path) < 0) return -1; if (virAsprintf(&pidstr, "%lld", (long long int) pid) < 0) @@ -2431,6 +2426,19 @@ virResctrlAllocAddPID(virResctrlAllocPtr alloc, int +virResctrlAllocAddPID(virResctrlAllocPtr alloc, + pid_t pid) +{ + /* If allocation is empty, then no resctrl directory and the 'tasks' file + * exists, just return */ + if (virResctrlAllocIsEmpty(alloc)) + return 0; + + return virResctrlAddPID(alloc->path, pid); +} + + +int virResctrlAllocRemove(virResctrlAllocPtr alloc) { int ret = 0; -- 2.7.4

The code for creating resctrl allocation group could be reused for monitoring group, refactor it for reuse in the later patch. Signed-off-by: Wang Huaqiang <huaqiang.wang(a)intel.com&gt; --- src/util/virresctrl.c | 37 +++++++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 93b5e70..5b984d9 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -2315,6 +2315,26 @@ virResctrlAllocDeterminePath(virResctrlAllocPtr alloc, } +/* This function creates a resctrl directory in resource control file system, + * and the directory path is specified by @path. */ +static int +virResctrlCreateGroupPath(const char *path) +{ + /* Directory exists, return */ + if (virFileExists(path)) + return 0; + + if (virFileMakePath(path) < 0) { + virReportSystemError(errno, + _("Cannot create resctrl directory '%s'"), + path); + return -1; + } + + return 0; +} + + /* This checks if the directory for the alloc exists. If not it tries to create * it and apply appropriate alloc settings. */ int @@ -2344,13 +2364,6 @@ virResctrlAllocCreate(virResctrlInfoPtr resctrl, if (virResctrlAllocIsEmpty(alloc)) return 0; - if (virFileExists(alloc->path)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Path '%s' for resctrl allocation exists"), - alloc->path); - goto cleanup; - } - lockfd = virResctrlLockWrite(); if (lockfd < 0) goto cleanup; @@ -2358,6 +2371,9 @@ virResctrlAllocCreate(virResctrlInfoPtr resctrl, if (virResctrlAllocAssign(resctrl, alloc) < 0) goto cleanup; + if (virResctrlCreateGroupPath(alloc->path) < 0) + goto cleanup; + alloc_str = virResctrlAllocFormat(alloc); if (!alloc_str) goto cleanup; @@ -2365,13 +2381,6 @@ virResctrlAllocCreate(virResctrlInfoPtr resctrl, if (virAsprintf(&schemata_path, "%s/schemata", alloc->path) < 0) goto cleanup; - if (virFileMakePath(alloc->path) < 0) { - virReportSystemError(errno, - _("Cannot create resctrl directory '%s'"), - alloc->path); - goto cleanup; - } - VIR_DEBUG("Writing resctrl schemata '%s' into '%s'", alloc_str, schemata_path); if (virFileWriteStr(schemata_path, alloc_str, 0) < 0) { rmdir(alloc->path); -- 2.7.4

Add interfaces monitor group to support operations such as add PID, set ID, remove group ... etc. Signed-off-by: Wang Huaqiang <huaqiang.wang(a)intel.com&gt; --- src/libvirt_private.syms | 5 +++++ src/util/virresctrl.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virresctrl.h | 14 ++++++++++++++ 3 files changed, 66 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a878083..d59ac86 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2683,7 +2683,12 @@ virResctrlInfoNew; virResctrlMonitorAddPID; virResctrlMonitorCreate; virResctrlMonitorDeterminePath; +virResctrlMonitorGetID; virResctrlMonitorNew; +virResctrlMonitorRemove; +virResctrlMonitorSetAlloc; +virResctrlMonitorSetID; + # util/virrotatingfile.h diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 9f42065..b0205bc 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -2566,3 +2566,50 @@ virResctrlMonitorCreate(virResctrlMonitorPtr monitor, virResctrlUnlock(lockfd); return ret; } + + +int +virResctrlMonitorSetID(virResctrlMonitorPtr monitor, + const char *id) +{ + if (!id) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Resctrl monitor 'id' cannot be NULL")); + return -1; + } + + return VIR_STRDUP(monitor->id, id); +} + + +const char * +virResctrlMonitorGetID(virResctrlMonitorPtr monitor) +{ + return monitor->id; +} + + +void +virResctrlMonitorSetAlloc(virResctrlMonitorPtr monitor, + virResctrlAllocPtr alloc) +{ + monitor->alloc = virObjectRef(alloc); +} + + +int +virResctrlMonitorRemove(virResctrlMonitorPtr monitor) +{ + int ret = 0; + + if (!monitor->path) + return 0; + + VIR_DEBUG("Removing resctrl monitor%s", monitor->path); + if (rmdir(monitor->path) != 0 && errno != ENOENT) { + ret = -errno; + VIR_ERROR(_("Unable to remove %s (%d)"), monitor->path, errno); + } + + return ret; +} diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h index 76e40a2..804d6f5 100644 --- a/src/util/virresctrl.h +++ b/src/util/virresctrl.h @@ -205,4 +205,18 @@ virResctrlMonitorAddPID(virResctrlMonitorPtr monitor, int virResctrlMonitorCreate(virResctrlMonitorPtr monitor, const char *machinename); + +int +virResctrlMonitorSetID(virResctrlMonitorPtr monitor, + const char *id); + +const char * +virResctrlMonitorGetID(virResctrlMonitorPtr monitor); + +void +virResctrlMonitorSetAlloc(virResctrlMonitorPtr monitor, + virResctrlAllocPtr alloc); + +int +virResctrlMonitorRemove(virResctrlMonitorPtr monitor); #endif /* __VIR_RESCTRL_H__ */ -- 2.7.4

On 10/22/18 4:01 AM, Wang Huaqiang wrote: Because we're not "stealing" @*alloc and/or @*vcpus, they do not need to be passed by reference. I can change these. There's some minor merge impact in later patches too, but no big deal. /* NB: Callers assume new @alloc, need to fill in ID now */ Not that it would prevent someone in the future from passing an @alloc w/ ->id already filled in and overwriting, but at least for now it's not the case. With the changes (that I can make), Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John [...]

On 11/6/18 4:51 AM, Huaqiang,Wang wrote: Let's see - virDomainResctrlNew has two callers: 1. virDomainCachetuneDefParse In this case, we "know" we have a new/empty @alloc because if virDomainResctrlVcpuMatch found one, then there'd be a failure. The virDomainCachetuneDefParseCache calls don't seem to fill in alloc->id, but virDomainResctrlNew will for the first time. 2. virDomainMemorytuneDefParse The virDomainResctrlVcpuMatch may find a preexisting @alloc, but @new_alloc is set to true. The virDomainMemorytuneDefParseMemory won't fill alloc->id. Then only if @new_alloc do we call virDomainResctrlNew So I think both are safe "for now". If you want I could modify the virResctrlAllocSetID code to : if (alloc->id) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Attempt to overwrite alloc->id='%s' with id='%s'"), alloc->id, id); return -1; } Let me know. John

This refactor allows to add some code between virDomainResctrlNew and virResctrlAllocIsEmpty to extend the scope of resctrl. Signed-off-by: Wang Huaqiang <huaqiang.wang(a)intel.com&gt; --- src/conf/domain_conf.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 39bd396..a068d4d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19022,15 +19022,15 @@ virDomainCachetuneDefParse(virDomainDefPtr def, goto cleanup; } + resctrl = virDomainResctrlNew(node, &alloc, &vcpus, flags); + if (!resctrl) + goto cleanup; + if (virResctrlAllocIsEmpty(alloc)) { ret = 0; goto cleanup; } - resctrl = virDomainResctrlNew(node, &alloc, &vcpus, flags); - if (!resctrl) - goto cleanup; - if (VIR_APPEND_ELEMENT(def->resctrls, def->nresctrls, resctrl) < 0) goto cleanup; -- 2.7.4

On 10/22/18 4:01 AM, Wang Huaqiang wrote: the associated associated For non-default monitors, overlapping vCPUs are not permitted. [...] [...] Swap order since *IsEmpty is more compute intensive, also change to: if (resctrl->nmonitors == 0 && So is this because when <memtune> is introduced it won't use a cache level, right? Just trying to recall (and perhaps add a comment). [...] I can fix the minor nits, just ack them... Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John

Add functions for creating, destroying, reconnecting resctrl monitor in qemu according to the configuration in domain XML. Signed-off-by: Wang Huaqiang <huaqiang.wang(a)intel.com&gt; --- src/qemu/qemu_process.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 65 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index e9c7618..fba4fb4 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2611,10 +2611,21 @@ qemuProcessResctrlCreate(virQEMUDriverPtr driver, return -1; for (i = 0; i < vm->def->nresctrls; i++) { + size_t j = 0; if (virResctrlAllocCreate(caps->host.resctrl, vm->def->resctrls[i]->alloc, priv->machineName) < 0) goto cleanup; + + for (j = 0; j < vm->def->resctrls[i]->nmonitors; j++) { + virDomainResctrlMonDefPtr mon = NULL; + + mon = vm->def->resctrls[i]->monitors[j]; + if (virResctrlMonitorCreate(mon->instance, + priv->machineName) < 0) + goto cleanup; + + } } ret = 0; @@ -5430,6 +5441,7 @@ qemuProcessSetupVcpu(virDomainObjPtr vm, { pid_t vcpupid = qemuDomainGetVcpuPid(vm, vcpuid); virDomainVcpuDefPtr vcpu = virDomainDefGetVcpu(vm->def, vcpuid); + virDomainResctrlMonDefPtr mon = NULL; size_t i = 0; if (qemuProcessSetupPid(vm, vcpupid, VIR_CGROUP_THREAD_VCPU, @@ -5440,11 +5452,42 @@ qemuProcessSetupVcpu(virDomainObjPtr vm, return -1; for (i = 0; i < vm->def->nresctrls; i++) { + size_t j = 0; virDomainResctrlDefPtr ct = vm->def->resctrls[i]; if (virBitmapIsBitSet(ct->vcpus, vcpuid)) { if (virResctrlAllocAddPID(ct->alloc, vcpupid) < 0) return -1; + + /* The order of invoking virResctrlMonitorAddPID matters, it is + * required to invoke this function first for monitor that has + * the same vcpus setting as the allocation in same def->resctrl. + * Otherwise, some other monitor's pid may be removed from its + * resource group's 'tasks' file.*/ + for (j = 0; j < vm->def->resctrls[i]->nmonitors; j++) { + mon = vm->def->resctrls[i]->monitors[j]; + + if (!virBitmapEqual(ct->vcpus, mon->vcpus)) + continue; + + if (virBitmapIsBitSet(mon->vcpus, vcpuid)) { + if (virResctrlMonitorAddPID(mon->instance, vcpupid) < 0) + return -1; + } + break; + } + + for (j = 0; j < vm->def->resctrls[i]->nmonitors; j++) { + mon = vm->def->resctrls[i]->monitors[j]; + + if (virBitmapEqual(ct->vcpus, mon->vcpus)) + continue; + + if (virBitmapIsBitSet(mon->vcpus, vcpuid)) { + if (virResctrlMonitorAddPID(mon->instance, vcpupid) < 0) + return -1; + } + } break; } } @@ -7207,8 +7250,18 @@ void qemuProcessStop(virQEMUDriverPtr driver, /* Remove resctrl allocation after cgroups are cleaned up which makes it * kind of safer (although removing the allocation should work even with * pids in tasks file */ - for (i = 0; i < vm->def->nresctrls; i++) + for (i = 0; i < vm->def->nresctrls; i++) { + size_t j = 0; + + for (j = 0; j < vm->def->resctrls[i]->nmonitors; j++) { + virDomainResctrlMonDefPtr mon = NULL; + + mon = vm->def->resctrls[i]->monitors[j]; + virResctrlMonitorRemove(mon->instance); + } + virResctrlAllocRemove(vm->def->resctrls[i]->alloc); + } qemuProcessRemoveDomainStatus(driver, vm); @@ -7939,9 +7992,20 @@ qemuProcessReconnect(void *opaque) goto error; for (i = 0; i < obj->def->nresctrls; i++) { + size_t j = 0; + if (virResctrlAllocDeterminePath(obj->def->resctrls[i]->alloc, priv->machineName) < 0) goto error; + + for (j = 0; j < obj->def->resctrls[i]->nmonitors; j++) { + virDomainResctrlMonDefPtr mon = NULL; + + mon = obj->def->resctrls[i]->monitors[j]; + if (virResctrlMonitorDeterminePath(mon->instance, + priv->machineName) < 0) + goto error; + } } /* update domain state XML with possibly updated state in virDomainObj */ -- 2.7.4

On 11/6/2018 2:01 AM, John Ferlan wrote: Yes. Will make such kind of substitution for all cases. Yes. You are right. In my original code logic, which is I need a @monitor->pids array to track all pids belonging to @monitor, these two loops are necessary. The first loop ignores all non-default monitors and adds the default monitor's PIDs to its 'tasks' file if default monitor exists. The second loop adds non-default monitors' PIDs. This order is intentionally designed because file writing for default monitor's 'tasks' file will remove PIDs that existing in other monitor's 'tasks' file. But I have taken your suggestion that the @monitor->pids is meaningless and removed this array, then, these two loops are not needed any more, only the second loop is kept. Along with the review comments you made the code would be: @@ -5440,11 +5451,26 @@ qemuProcessSetupVcpu(virDomainObjPtr vm,          return -1;      for (i = 0; i < vm->def->nresctrls; i++) { +        size_t j = 0;          virDomainResctrlDefPtr ct = vm->def->resctrls[i];          if (virBitmapIsBitSet(ct->vcpus, vcpuid)) {              if (virResctrlAllocAddPID(ct->alloc, vcpupid) < 0)                  return -1; + +            for (j = 0; j < ct->nmonitors; j++) { +                mon = ct->monitors[j]; + +                if (virBitmapEqual(ct->vcpus, mon->vcpus)) +                    continue; + +                if (virBitmapIsBitSet(mon->vcpus, vcpuid)) { +                    if (virResctrlMonitorAddPID(mon->instance, vcpupid) < 0) +                        return -1; +                    break; +                } +            } +              break;          }      } Thanks for review. Huaqiang

On 11/6/2018 2:44 AM, John Ferlan wrote: Then I need to merge the next patch, patch 14, with some previous patch, at least for the 'MonitorAddPID' function, to avoid a second changing of a same function. Got, thanks. But, in my next series of patches, patch 14 is removed with all logic in it. So the patch order seems rational then. The *tasks file is not a file kept in a block device, its content, the PIDS, could be removed if the process/task is terminated or PIDs have been added to some other resctrl group. It seems the  *tasks file will not be stale in scenario of vcpu hot-plug. For vcpu unplugging case, qemuDomainHotplugDelVcpu removes and terminates the vcpu process, right? Then the *tasks file will automatically be updated by removing the process's PID. The removal of PID from *tasks is done by resctrl file system. So there is no hot-plug issue for resctrl in terms of tracking PID in *tasks. In my understanding reconnect means the restart of  libvirtd and the information re-building for VMs. Originally I believed that vcpupid would be changed during a reconnect, that is the reason I add the function for validating monitor in patch14. But the vcpupid has not been changed in process of reconnect. So I decide to remove patch14. Do you still believe "vcpus could be added/deleted and we didn't get notified' in reconnect"? If so, can you list more details? And any operation for adding/deleting VM vcpus out of libvirt is not permitted right? *tasks file is still there with the PIDs in it. Seems it is not needed to recreate it. So I removed my last patch, in that patch I invoked qemuProcessSetupVcpus in process of re-connection. Thanks for review. Huaqiang

On 10/22/18 4:01 AM, Wang Huaqiang wrote: 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? 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. 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). 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. 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

Adding element 'id' to virDomainResctrlDef tracking resource group id, it reflects the attribute 'id' of of element <cachetune> in XML. virResctrlAlloc.id is a copy from virDomanResctrlDef.id. Signed-off-by: Wang Huaqiang <huaqiang.wang(a)intel.com&gt; --- src/conf/domain_conf.c | 20 ++++++++------------ src/conf/domain_conf.h | 1 + 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 01f795f..1aecd8c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2980,6 +2980,7 @@ virDomainResctrlDefFree(virDomainResctrlDefPtr resctrl) virObjectUnref(resctrl->alloc); virBitmapFree(resctrl->vcpus); VIR_FREE(resctrl->monitors); + VIR_FREE(resctrl->id); VIR_FREE(resctrl); } @@ -19145,6 +19146,9 @@ virDomainResctrlNew(xmlNodePtr node, if (VIR_ALLOC(resctrl) < 0) goto cleanup; + if (VIR_STRDUP(resctrl->id, alloc_id) < 0) + goto cleanup; + if (!(resctrl->vcpus = virBitmapNewCopy(*vcpus))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to copy 'vcpus'")); @@ -27323,13 +27327,9 @@ virDomainCachetuneDefFormat(virBufferPtr buf, virBufferAsprintf(buf, "<cachetune vcpus='%s'", vcpus); - if (!(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE)) { - const char *alloc_id = virResctrlAllocGetID(resctrl->alloc); - if (!alloc_id) - goto cleanup; + if (!(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE)) + virBufferAsprintf(buf, " id='%s'", resctrl->id); - virBufferAsprintf(buf, " id='%s'", alloc_id); - } virBufferAddLit(buf, ">\n"); virBufferAddBuffer(buf, &childrenBuf); @@ -27386,13 +27386,9 @@ virDomainMemorytuneDefFormat(virBufferPtr buf, virBufferAsprintf(buf, "<memorytune vcpus='%s'", vcpus); - if (!(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE)) { - const char *alloc_id = virResctrlAllocGetID(resctrl->alloc); - if (!alloc_id) - goto cleanup; + if (!(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE)) + virBufferAsprintf(buf, " id='%s'", resctrl->id); - virBufferAsprintf(buf, " id='%s'", alloc_id); - } virBufferAddLit(buf, ">\n"); virBufferAddBuffer(buf, &childrenBuf); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 60f6464..e190aa2 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2248,6 +2248,7 @@ typedef struct _virDomainResctrlDef virDomainResctrlDef; typedef virDomainResctrlDef *virDomainResctrlDefPtr; struct _virDomainResctrlDef { + char *id; virBitmapPtr vcpus; virResctrlAllocPtr alloc; -- 2.7.4

On 11/6/2018 3:15 AM, John Ferlan wrote: This patch should be removed and will be removed. It is introduced in series v4, in that series the 'resctrl->alloc' is allowed to be NULL representing a default monitor. Now we changed our design and resctrl->alloc is always assigned with an virResctrlAlloc data structure, then 'resctrl->id' is not necessary. We can keep the resctrl ID in alloc->id. Thanks for review. Huaqiang

On 10/22/18 4:01 AM, Wang Huaqiang wrote: Maybe instead of inverting the logic, let's create a cgroup helper... qemuDomainGetStatsCpuCgroup(dom, record, *maxparams) { logic as is now } static int qemuDomainGetStatsCpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, virDomainObjPtr dom, virDomainStatsRecordPtr record, int *maxparams, unsigned int privflags ATTRIBUTE_UNUSED) { return qemuDomainGetStatsCpuCgroup(dom, record, maxparams); } Then the subsequent patch would alter the above check and add the new call. John

Adding the interface in qemu to report CMT statistic information through command 'virsh domstats --cpu-total'. Below is a typical output: # virsh domstats 1 --cpu-total Domain: 'ubuntu16.04-base' ... cpu.cache.monitor.count=2 cpu.cache.monitor.0.name=vcpus_1 cpu.cache.monitor.0.vcpus=1 cpu.cache.monitor.0.bank.count=2 cpu.cache.monitor.0.bank.0.id=0 cpu.cache.monitor.0.bank.0.bytes=4505600 cpu.cache.monitor.0.bank.1.id=1 cpu.cache.monitor.0.bank.1.bytes=5586944 cpu.cache.monitor.1.name=vcpus_4-6 cpu.cache.monitor.1.vcpus=4,5,6 cpu.cache.monitor.1.bank.count=2 cpu.cache.monitor.1.bank.0.id=0 cpu.cache.monitor.1.bank.0.bytes=17571840 cpu.cache.monitor.1.bank.1.id=1 cpu.cache.monitor.1.bank.1.bytes=29106176 Signed-off-by: Wang Huaqiang <huaqiang.wang(a)intel.com&gt; --- src/libvirt-domain.c | 9 ++ src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 229 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virresctrl.c | 130 +++++++++++++++++++++++++++ src/util/virresctrl.h | 12 +++ 5 files changed, 381 insertions(+) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 7690339..4895f9f 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -11345,6 +11345,15 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * "cpu.user" - user cpu time spent in nanoseconds as unsigned long long. * "cpu.system" - system cpu time spent in nanoseconds as unsigned long * long. + * "cpu.cache.monitor.count" - tocal cache monitoring groups + * "cpu.cache.monitor.M.name" - name of cache monitoring group 'M' + * "cpu.cache.monitor.M.vcpus" - vcpus for cache monitoring group 'M' + * "cpu.cache.monitor.M.bank.count" - total bank number of cache monitoring + * group 'M' + * "cpu.cache.monitor.M.bank.N.id" - OS assigned cache bank id for cache + * 'N' in cache monitoring group 'M' + * "cpu.cache.monitor.M.bank.N.bytes" - monitor's cache occupancy of cache + * bank 'N' in cache monitoring group 'M' * * VIR_DOMAIN_STATS_BALLOON: * Return memory balloon device information. diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 91801ed..4551767 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2683,6 +2683,7 @@ virResctrlInfoNew; virResctrlMonitorAddPID; virResctrlMonitorCreate; virResctrlMonitorDeterminePath; +virResctrlMonitorGetCacheOccupancy; virResctrlMonitorGetID; virResctrlMonitorIsRunning; virResctrlMonitorNew; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 12a5f8f..9828118 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -102,6 +102,7 @@ #include "virnuma.h" #include "dirname.h" #include "netdev_bandwidth_conf.h" +#include "c-ctype.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -19698,6 +19699,230 @@ typedef enum { #define HAVE_JOB(flags) ((flags) & QEMU_DOMAIN_STATS_HAVE_JOB) +/* In terms of the output of virBitmapFormat, both '1-3' and '1,3' are valid + * outputs and represent different vcpu set. But it is not easy to + * differentiate these two vcpu set formats at first glance. + * + * This function could be used to clear this ambiguity, it substitutes all '-' + * with ',' while generates semantically correct vcpu set. + * e.g. vcpu set string '1-3' will be replaced by string '1,2,3'. */ +static char * +qemuDomainVcpuFormatHelper(const char *vcpus) +{ + size_t i = 0; + int last = 0; + int start = 0; + char * tmp = NULL; + bool firstnum = true; + const char *cur = vcpus; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *ret = NULL; + + if (virStringIsEmpty(cur)) + return NULL; + + while (*cur != '\0') { + if (!c_isdigit(*cur)) + goto cleanup; + + if (virStrToLong_i(cur, &tmp, 10, &start) < 0) + goto cleanup; + if (start < 0) + goto cleanup; + + cur = tmp; + + virSkipSpaces(&cur); + + if (*cur == ',' || *cur == 0) { + if (!firstnum) + virBufferAddChar(&buf, ','); + virBufferAsprintf(&buf, "%d", start); + firstnum = false; + } else if (*cur == '-') { + cur++; + virSkipSpaces(&cur); + + if (virStrToLong_i(cur, &tmp, 10, &last) < 0) + goto cleanup; + + if (last < start) + goto cleanup; + cur = tmp; + + for (i = start; i <= last; i++) { + if (!firstnum) + + virBufferAddChar(&buf, ','); + virBufferAsprintf(&buf, "%ld", i); + firstnum = 0; + } + + virSkipSpaces(&cur); + } + + if (*cur == ',') { + cur++; + virSkipSpaces(&cur); + } else if (*cur == 0) { + break; + } else { + goto cleanup; + } + } + + ret = virBufferContentAndReset(&buf); + cleanup: + virBufferFreeAndReset(&buf); + return ret; +} + + +static int +qemuDomainGetStatsCpuResource(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, + virDomainObjPtr dom, + virDomainStatsRecordPtr record, + int *maxparams, + unsigned int privflags ATTRIBUTE_UNUSED, + virResctrlMonitorType restag) +{ + char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; + virDomainResctrlMonDefPtr domresmon = NULL; + virResctrlMonitorStatsPtr stats = NULL; + size_t nstats = 0; + virDomainResctrlDefPtr resctrl = NULL; + unsigned int nmonitors = 0; + unsigned int imonitor = 0; + const char *restype = NULL; + char *rawvcpus = NULL; + char *vcpus = NULL; + size_t i = 0; + size_t j = 0; + int ret = -1; + + if (!virDomainObjIsActive(dom)) + return 0; + + if (restag == VIR_RESCTRL_MONITOR_TYPE_CACHE) { + restype = "cache"; + } else { + VIR_DEBUG("Invalid CPU resource type"); + return -1; + } + + for (i = 0; i < dom->def->nresctrls; i++) { + resctrl = dom->def->resctrls[i]; + + for (j = 0; j < resctrl->nmonitors; j++) { + domresmon = resctrl->monitors[j]; + if (virResctrlMonitorIsRunning(domresmon->instance) && + domresmon->tag == restag) + nmonitors++; + } + } + + if (nmonitors) { + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, + "cpu.%s.monitor.count", restype); + if (virTypedParamsAddUInt(&record->params, + &record->nparams, + maxparams, + param_name, + nmonitors) < 0) + goto cleanup; + } + + for (i = 0; i < dom->def->nresctrls; i++) { + resctrl = dom->def->resctrls[i]; + + for (j = 0; j < resctrl->nmonitors; j++) { + size_t l = 0; + virResctrlMonitorPtr monitor = resctrl->monitors[j]->instance; + const char *id = virResctrlMonitorGetID(monitor); + + if (!id) + goto cleanup; + + domresmon = resctrl->monitors[j]; + + if (!virResctrlMonitorIsRunning(domresmon->instance)) + continue; + + if (!(rawvcpus = virBitmapFormat(domresmon->vcpus))) + goto cleanup; + + vcpus = qemuDomainVcpuFormatHelper(rawvcpus); + if (!vcpus) + goto cleanup; + + if (virResctrlMonitorGetCacheOccupancy(monitor, &stats, + &nstats) < 0) + goto cleanup; + + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, + "cpu.%s.monitor.%d.name", restype, imonitor); + if (virTypedParamsAddString(&record->params, + &record->nparams, + maxparams, + param_name, + id) < 0) + goto cleanup; + + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, + "cpu.%s.monitor.%d.vcpus", restype, imonitor); + + if (virTypedParamsAddString(&record->params, + &record->nparams, + maxparams, + param_name, + vcpus) < 0) + goto cleanup; + + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, + "cpu.%s.monitor.%d.bank.count", restype, imonitor); + if (virTypedParamsAddUInt(&record->params, + &record->nparams, + maxparams, + param_name, + nstats) < 0) + goto cleanup; + + for (l = 0; l < nstats; l++) { + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, + "cpu.%s.monitor.%d.bank.%ld.id", + restype, imonitor, l); + if (virTypedParamsAddUInt(&record->params, + &record->nparams, + maxparams, + param_name, + stats[l].id) < 0) + goto cleanup; + + + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, + "cpu.%s.monitor.%d.bank.%ld.bytes", + restype, imonitor, l); + if (virTypedParamsAddUInt(&record->params, + &record->nparams, + maxparams, + param_name, + stats[l].val) < 0) + goto cleanup; + } + + VIR_FREE(stats); + VIR_FREE(vcpus); + imonitor++; + } + } + + ret = 0; + cleanup: + VIR_FREE(vcpus); + return ret; +} + + static int qemuDomainGetStatsCpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, virDomainObjPtr dom, @@ -19735,6 +19960,10 @@ qemuDomainGetStatsCpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, return -1; } + if (qemuDomainGetStatsCpuResource(driver, dom, record, maxparams, privflags, + VIR_RESCTRL_MONITOR_TYPE_CACHE) < 0) + return -1; + return 0; } diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index fa3e6e9..02e7095 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -2713,3 +2713,133 @@ virResctrlMonitorIsRunning(virResctrlMonitorPtr monitor) return ret; } + + +static int +virResctrlMonitorStatsSorter(const void *a, + const void *b) +{ + return ((virResctrlMonitorStatsPtr)a)->id + - ((virResctrlMonitorStatsPtr)b)->id; +} + + +/* + * virResctrlMonitorGetStats + * + * @monitor: The monitor that the statistic data will be retrieved from. + * @resource: The name for resource name. 'llc_occupancy' for cache resource. + * "mbm_total_bytes" and "mbm_local_bytes" for memory bandwidth resource. + * @stats: Array of virResctrlMonitorStatsPtr for holding cache or memory + * bandwidth usage data. + * @nstats: A size_t pointer to hold the returned array length of @stats + * + * Get cache or memory bandwidth utilization information. + * + * Returns 0 on success, -1 on error. + */ +static int +virResctrlMonitorGetStats(virResctrlMonitorPtr monitor, + const char *resource, + virResctrlMonitorStatsPtr *stats, + size_t *nstats) +{ + int rv = -1; + int ret = -1; + DIR *dirp = NULL; + char *datapath = NULL; + struct dirent *ent = NULL; + virResctrlMonitorStatsPtr stat = NULL; + + if (!monitor) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Invalid resctrl monitor")); + return -1; + } + + if (virAsprintf(&datapath, "%s/mon_data", monitor->path) < 0) + return -1; + + if (virDirOpen(&dirp, datapath) < 0) + goto cleanup; + + *nstats = 0; + while (virDirRead(dirp, &ent, datapath) > 0) { + char *node_id = NULL; + + if (VIR_ALLOC(stat) < 0) + goto cleanup; + + /* Looking for directory that contains resource utilization + * information file. The directory name is arranged in format + * "mon_<node_name>_<node_id>". For example, "mon_L3_00" and + * "mon_L3_01" are two target directories for a two nodes system + * with resource utilization data file for each node respectively. + */ + if (ent->d_type != DT_DIR) + continue; + + /* Looking for directory has a prefix 'mon_L' */ + if (!(node_id = STRSKIP(ent->d_name, "mon_L"))) + continue; + + /* Looking for directory has another '_' */ + node_id = strchr(node_id, '_'); + if (!node_id) + continue; + + /* Skip the character '_' */ + if (!(node_id = STRSKIP(node_id, "_"))) + continue; + + /* The node ID number should be here, parsing it. */ + if (virStrToLong_uip(node_id, NULL, 0, &stat->id) < 0) + goto cleanup; + + rv = virFileReadValueUint(&stat->val, "%s/%s/%s", datapath, + ent->d_name, resource); + if (rv == -2) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("File '%s/%s/%s' does not exist."), + datapath, ent->d_name, resource); + } + if (rv < 0) + goto cleanup; + + if (VIR_APPEND_ELEMENT(*stats, *nstats, *stat) < 0) + goto cleanup; + } + + /* Sort in id's ascending order */ + qsort(*stats, *nstats, sizeof(*stat), virResctrlMonitorStatsSorter); + + ret = 0; + cleanup: + VIR_FREE(datapath); + VIR_FREE(stat); + VIR_DIR_CLOSE(dirp); + return ret; +} + + +/* + * virResctrlMonitorGetCacheOccupancy + * + * @monitor: The monitor that the statistic data will be retrieved from. + * @caches: Array of virResctrlMonitorStatsPtr for receiving cache occupancy + * data. Caller is responsible to free this array. + * @ncaches: A size_t pointer to hold the returned array length of @caches + * + * Get cache or memory bandwidth utilization information. + * + * Returns 0 on success, -1 on error. + */ + +int +virResctrlMonitorGetCacheOccupancy(virResctrlMonitorPtr monitor, + virResctrlMonitorStatsPtr *caches, + size_t *ncaches) +{ + return virResctrlMonitorGetStats(monitor, "llc_occupancy", + caches, ncaches); +} diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h index 8d8fdc2..004c40e 100644 --- a/src/util/virresctrl.h +++ b/src/util/virresctrl.h @@ -191,6 +191,13 @@ virResctrlInfoGetMonitorPrefix(virResctrlInfoPtr resctrl, typedef struct _virResctrlMonitor virResctrlMonitor; typedef virResctrlMonitor *virResctrlMonitorPtr; +typedef struct _virResctrlMonitorStats virResctrlMonitorStats; +typedef virResctrlMonitorStats *virResctrlMonitorStatsPtr; +struct _virResctrlMonitorStats { + unsigned int id; + unsigned int val; +}; + virResctrlMonitorPtr virResctrlMonitorNew(void); @@ -222,4 +229,9 @@ virResctrlMonitorRemove(virResctrlMonitorPtr monitor); bool virResctrlMonitorIsRunning(virResctrlMonitorPtr monitor); + +int +virResctrlMonitorGetCacheOccupancy(virResctrlMonitorPtr monitor, + virResctrlMonitorStatsPtr *caches, + size_t *ncaches); #endif /* __VIR_RESCTRL_H__ */ -- 2.7.4

On 11/6/2018 4:04 AM, John Ferlan wrote: Got.  Header file will be removed （actually I planned to remove all code using it.) Yes, this heavy function is just re-formatting the output of vcpus. I also think it not worth to do this kind of reformat. Let's remove it. I just copy-pasted the function header from some other function. Will be removed since it is an internal function. If the lines with virResctrlMonitorIsRunning could be removed, then this loop is not necessary any more. virResctrlMonitorIsRunning is validating monitor through verifying PID lists, but it seems there is no need to do such kind of validation and the PIDs in *tasks file will not change without control: -. The PID in *tasks file will be removed by kernel if vcpu process is terminated, but the vcpu process will not exit except performing vcpu hot-plug. -. In process of libvirt re-connection, VM will not  make any changes in terms of resctrl groups and vcpus, so the content of *tasks will not change and the layout of resctrl groups will not change. -. Any changes out of libvirt are not allowed, for example, manually creating or deleting any resctrl groups by bash commands are not allowed. I decided to remove the lines invoking virResctrlMonitorIsRunning, and the loop will be removed. Actually I will split this function into three functions: qemuDomainGetCpuResMonitorData qemuDomainGetStatsCpuResMonitorPerTag and qemuDomainGetStatsCPUResMonitor qemuDomainGetCpuResMonitorData will fetch data such as the monitor group ID, vcpus, and cache information for all monitors, any error happened in this function will reported to user through virReportError. So the error will be reported to user before dumping the final result. qemuDomainGetStatsCpuResMonitorPerTag is responsible for showing the data retrieved by a successful call of qemuDomainGetCpuResMonitorData. In this function it does not dump any error message created by virReportError which will interrupt the showing of cache monitor information. qemuDomainGetStatsCPUResMonitor is the interface called by qemuDomainGetStatsCPU. we could invoke qemuDomainGetStatsCpuResMonitorPerTag  with tag VIR_RESCTRL_MONITOR_TYPE_CACHE for cache monitor. memBW monitor result will be processed by invoking this function through specify tag VIR_RESCTRL_MONITOR_TYPE_MEMBW. Originally, the purpose of using VIR_DEBUG,not virReportError, to report error is to avoid showing partial monitor information with an error message on screen. I will split this function into three functions, qemuDomainGetCpuResMonitorData qemuDomainGetStatsCpuResMonitorPerTag and qemuDomainGetStatsCPUResMonitor qemuDomainGetCpuResMonitorData will get necessary data and reported any error occurred to user promptly. qemuDomainGetStatsCpuResMonitorPerTag and qemuDomainGetStatsCPUResMonitor will report the cache monitor result. This will avoid the mix of cache monitor result and error message if error occurs. Please review my new code. virResctrlMonitorIsRunning will not be called any more as I stated. and the code will be changed, my new code will be more efficient. This part will be removed. Here the purpose of code is to get the total 'valid' (after a successful check of virResctrlMonitorIsRunning) monitor groups. It actually performing the validation of  nmonitors == resctrl->nmonitors, just as you said. Now I think there is no need to validate the monitor any more. Error message should be reported. And will be reported. Please review the new code. virResctrlMonitorIsRunning disappears in code and patch for this file will be redesigned. Please renew the new code. If virResctrlMonitorGetCacheOccupancy returns without error, nstats will be assigned with the proper bank group number. If virResctrlMonitorGetCacheOccupancy returns an error, then code will go through cleanup path and returns. Seems no need to clear nstats to 0. Any way in my new code, this nstats will not be used any more, please review the logic of new code later. Will be changed to if (*nstats)     qsort(*stats, *nstats, sizeof(*stat), virResctrlMonitorStatsSorter); Will use @stats and @nstats for virResctrlMonitorGetCacheOccupancy and later API (for memBW). Will use @stats and @nstats for parameters. Will use @stats and @nstats for parameters. The virresctrl.* part could be separated, and how about  merging this part with previous patch that adding more monitor interfaces in virresctrl.*. I'll do this in my next update for your review. Thanks for review. Huaqiang

On 10/22/18 4:01 AM, Wang Huaqiang wrote: IIRC the reason for not needing this is that the assumption is that the initial startup (ProcessLaunch) and any hotplug thereafter would end up calling qemuProcessSetupVcpu. Not sure this is necessary for everything except perhaps the resctrl monitor *pids list which I'm not even sure is necessary. John