On 11/12/18 8:31 AM, Wang Huaqiang wrote: [...] [...] /* If the allocation is empty, then it is impossible to add a PID to * allocation due to lacking of its 'tasks' file so just return */ You added this backwards, must go below the following: With those changes, Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; (As with the rest, I can make the changes for you). John

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; Reviewed-by: John Ferlan <jferlan(a)redhat.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 7bd52cd..94689dd 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -2316,6 +2316,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 (STREQ(alloc->path, SYSFS_RESCTRL_PATH)) 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

Refactor virResctrlAllocSetID. In new code the error of id overwriting is reported promptly. Signed-off-by: Wang Huaqiang <huaqiang.wang(a)intel.com&gt; --- src/util/virresctrl.c | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 3216abe..4e4831c 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -1361,17 +1361,32 @@ virResctrlAllocForeachMemory(virResctrlAllocPtr alloc, } -int -virResctrlAllocSetID(virResctrlAllocPtr alloc, - const char *id) +static int +virResctrlSetID(char **resctrlid, + const char *id) { if (!id) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Resctrl allocation 'id' cannot be NULL")); + _("New resctrl 'id' cannot be NULL")); + return -1; + } + + if (*resctrlid) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Attempt to overwrite resctrlid='%s' with id='%s'"), + *resctrlid, id); return -1; } - return VIR_STRDUP(alloc->id, id); + return VIR_STRDUP(*resctrlid, id); +} + + +int +virResctrlAllocSetID(virResctrlAllocPtr alloc, + const char *id) +{ + return virResctrlSetID(&alloc->id, id); } -- 2.7.4

Add virResctrlMonitorSetID by leveraging previous refactored patch. Signed-off-by: Wang Huaqiang <huaqiang.wang(a)intel.com&gt; --- src/util/virresctrl.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 4e4831c..ed682c9 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -2581,3 +2581,12 @@ virResctrlMonitorCreate(virResctrlMonitorPtr monitor, virResctrlUnlock(lockfd); return ret; } + + +int +virResctrlMonitorSetID(virResctrlMonitorPtr monitor, + const char *id) + +{ + return virResctrlSetID(&monitor->id, id); +} -- 2.7.4

Add interfaces monitor group to support operations such as add PID, get ID, remove group ... etc. Signed-off-by: Wang Huaqiang <huaqiang.wang(a)intel.com&gt; --- src/libvirt_private.syms | 5 ++ src/util/virresctrl.c | 167 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virresctrl.h | 26 ++++++++ 3 files changed, 198 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a878083..2938833 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2683,7 +2683,12 @@ virResctrlInfoNew; virResctrlMonitorAddPID; virResctrlMonitorCreate; virResctrlMonitorDeterminePath; +virResctrlMonitorGetCacheOccupancy; +virResctrlMonitorGetID; virResctrlMonitorNew; +virResctrlMonitorRemove; +virResctrlMonitorSetAlloc; +virResctrlMonitorSetID; # util/virrotatingfile.h diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index ed682c9..9e7de62 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -2590,3 +2590,170 @@ virResctrlMonitorSetID(virResctrlMonitorPtr monitor, { return virResctrlSetID(&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; + + if (STREQ(monitor->path, monitor->alloc->path)) + return 0; + + VIR_DEBUG("Removing resctrl monitor path=%s", monitor->path); + if (rmdir(monitor->path) != 0 && errno != ENOENT) { + ret = -errno; + VIR_ERROR(_("Unable to remove %s (%d)"), monitor->path, errno); + } + + 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 */ + if (*nstats) + 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. + * @stats: Array of virResctrlMonitorStatsPtr for receiving cache occupancy + * data. Caller is responsible to free this array. + * @nstats: 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 *stats, + size_t *nstats) +{ + return virResctrlMonitorGetStats(monitor, "llc_occupancy", + stats, nstats); +} diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h index 76e40a2..45ec967 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); @@ -205,4 +212,23 @@ 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); + +int +virResctrlMonitorGetCacheOccupancy(virResctrlMonitorPtr monitor, + virResctrlMonitorStatsPtr *caches, + size_t *ncaches); #endif /* __VIR_RESCTRL_H__ */ -- 2.7.4

Introduced virDomainResctrlNew to do the most part of virDomainResctrlAppend and move the operation of appending resctrl to @def->resctrls out of function. Rather than rely on virDomainResctrlAppend to perform the allocation, move the onus to the caller and make use of virBitmapNewCopy for @vcpus and virObjectRef for @alloc, thus removing the need to set each to NULL after the call. Signed-off-by: Wang Huaqiang <huaqiang.wang(a)intel.com&gt; --- src/conf/domain_conf.c | 56 +++++++++++++++++++++++++++++--------------------- 1 file changed, 33 insertions(+), 23 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 237540b..8433214 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18948,26 +18948,22 @@ virDomainCachetuneDefParseCache(xmlXPathContextPtr ctxt, } -static int -virDomainResctrlAppend(virDomainDefPtr def, - xmlNodePtr node, - virResctrlAllocPtr alloc, - virBitmapPtr vcpus, - unsigned int flags) +static virDomainResctrlDefPtr +virDomainResctrlNew(xmlNodePtr node, + virResctrlAllocPtr alloc, + virBitmapPtr vcpus, + unsigned int flags) { char *vcpus_str = NULL; char *alloc_id = NULL; - virDomainResctrlDefPtr tmp_resctrl = NULL; - int ret = -1; - - if (VIR_ALLOC(tmp_resctrl) < 0) - goto cleanup; + virDomainResctrlDefPtr resctrl = NULL; + virDomainResctrlDefPtr ret = NULL; /* We need to format it back because we need to be consistent in the naming * even when users specify some "sub-optimal" string there. */ vcpus_str = virBitmapFormat(vcpus); if (!vcpus_str) - goto cleanup; + return NULL; if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE)) alloc_id = virXMLPropString(node, "id"); @@ -18985,15 +18981,20 @@ virDomainResctrlAppend(virDomainDefPtr def, if (virResctrlAllocSetID(alloc, alloc_id) < 0) goto cleanup; - tmp_resctrl->vcpus = vcpus; - tmp_resctrl->alloc = alloc; + if (VIR_ALLOC(resctrl) < 0) + goto cleanup; - if (VIR_APPEND_ELEMENT(def->resctrls, def->nresctrls, tmp_resctrl) < 0) + if (!(resctrl->vcpus = virBitmapNewCopy(vcpus))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to copy 'vcpus'")); goto cleanup; + } - ret = 0; + resctrl->alloc = virObjectRef(alloc); + + VIR_STEAL_PTR(ret, resctrl); cleanup: - virDomainResctrlDefFree(tmp_resctrl); + virDomainResctrlDefFree(resctrl); VIR_FREE(alloc_id); VIR_FREE(vcpus_str); return ret; @@ -19010,6 +19011,7 @@ virDomainCachetuneDefParse(virDomainDefPtr def, xmlNodePtr *nodes = NULL; virBitmapPtr vcpus = NULL; virResctrlAllocPtr alloc = NULL; + virDomainResctrlDefPtr resctrl = NULL; ssize_t i = 0; int n; int ret = -1; @@ -19048,19 +19050,22 @@ virDomainCachetuneDefParse(virDomainDefPtr def, goto cleanup; } + resctrl = virDomainResctrlNew(node, alloc, vcpus, flags); + if (!resctrl) + goto cleanup; + if (virResctrlAllocIsEmpty(alloc)) { ret = 0; goto cleanup; } - if (virDomainResctrlAppend(def, node, alloc, vcpus, flags) < 0) + if (VIR_APPEND_ELEMENT(def->resctrls, def->nresctrls, resctrl) < 0) goto cleanup; - vcpus = NULL; - alloc = NULL; ret = 0; cleanup: ctxt->node = oldnode; + virDomainResctrlDefFree(resctrl); virObjectUnref(alloc); virBitmapFree(vcpus); VIR_FREE(nodes); @@ -19218,6 +19223,8 @@ virDomainMemorytuneDefParse(virDomainDefPtr def, xmlNodePtr *nodes = NULL; virBitmapPtr vcpus = NULL; virResctrlAllocPtr alloc = NULL; + virDomainResctrlDefPtr resctrl = NULL; + ssize_t i = 0; int n; int ret = -1; @@ -19262,15 +19269,18 @@ virDomainMemorytuneDefParse(virDomainDefPtr def, * just update the existing alloc information, which is done in above * virDomainMemorytuneDefParseMemory */ if (new_alloc) { - if (virDomainResctrlAppend(def, node, alloc, vcpus, flags) < 0) + resctrl = virDomainResctrlNew(node, alloc, vcpus, flags); + if (!resctrl) + goto cleanup; + + if (VIR_APPEND_ELEMENT(def->resctrls, def->nresctrls, resctrl) < 0) goto cleanup; - vcpus = NULL; - alloc = NULL; } ret = 0; cleanup: ctxt->node = oldnode; + virDomainResctrlDefFree(resctrl); virObjectUnref(alloc); virBitmapFree(vcpus); VIR_FREE(nodes); -- 2.7.4

Introducing <monitor> element under <cachetune> to represent a cache monitor. Signed-off-by: Wang Huaqiang <huaqiang.wang(a)intel.com&gt; Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; --- docs/formatdomain.html.in | 26 +++ docs/schemas/domaincommon.rng | 10 + src/conf/domain_conf.c | 234 ++++++++++++++++++++- src/conf/domain_conf.h | 11 + tests/genericxml2xmlindata/cachetune-cdp.xml | 3 + .../cachetune-colliding-monitor.xml | 30 +++ tests/genericxml2xmlindata/cachetune-small.xml | 7 + tests/genericxml2xmltest.c | 2 + 8 files changed, 322 insertions(+), 1 deletion(-) create mode 100644 tests/genericxml2xmlindata/cachetune-colliding-monitor.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 8850a71..92ad4b2 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -759,6 +759,12 @@ &lt;cachetune vcpus='0-3'&gt; &lt;cache id='0' level='3' type='both' size='3' unit='MiB'/&gt; &lt;cache id='1' level='3' type='both' size='3' unit='MiB'/&gt; + &lt;monitor level='3' vcpus='1'/&gt; + &lt;monitor level='3' vcpus='0-3'/&gt; + &lt;/cachetune&gt; + &lt;cachetune vcpus='4-5'&gt; + &lt;monitor level='3' vcpus='4'/&gt; + &lt;monitor level='3' vcpus='5'/&gt; &lt;/cachetune&gt; &lt;memorytune vcpus='0-3'&gt; &lt;node id='0' bandwidth='60'/&gt; @@ -978,6 +984,26 @@ </dd> </dl> </dd> + <dt><code>monitor</code><span class="since">Since 4.10.0</span></dt> + <dd> + The optional element <code>monitor</code> creates the cache + monitor(s) for current cache allocation and has the following + required attributes: + <dl> + <dt><code>level</code></dt> + <dd> + Host cache level the monitor belongs to. + </dd> + <dt><code>vcpus</code></dt> + <dd> + vCPU list the monitor applies to. A monitor's vCPU list + can only be the member(s) of the vCPU list of the associated + allocation. The default monitor has the same vCPU list as the + associated allocation. For non-default monitors, overlapping + vCPUs are not permitted. + </dd> + </dl> + </dd> </dl> </dd> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 2b465be..1296b7f 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -981,6 +981,16 @@ </optional> </element> </zeroOrMore> + <zeroOrMore> + <element name="monitor"> + <attribute name="level"> + <ref name='unsignedInt'/> + </attribute> + <attribute name="vcpus"> + <ref name='cpuset'/> + </attribute> + </element> + </zeroOrMore> </element> </zeroOrMore> <zeroOrMore> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8433214..7696098 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2955,13 +2955,31 @@ virDomainLoaderDefFree(virDomainLoaderDefPtr loader) static void +virDomainResctrlMonDefFree(virDomainResctrlMonDefPtr domresmon) +{ + if (!domresmon) + return; + + virBitmapFree(domresmon->vcpus); + virObjectUnref(domresmon->instance); + VIR_FREE(domresmon); +} + + +static void virDomainResctrlDefFree(virDomainResctrlDefPtr resctrl) { + size_t i = 0; + if (!resctrl) return; + for (i = 0; i < resctrl->nmonitors; i++) + virDomainResctrlMonDefFree(resctrl->monitors[i]); + virObjectUnref(resctrl->alloc); virBitmapFree(resctrl->vcpus); + VIR_FREE(resctrl->monitors); VIR_FREE(resctrl); } @@ -18948,6 +18966,177 @@ virDomainCachetuneDefParseCache(xmlXPathContextPtr ctxt, } +/* Checking if the monitor's vcpus is conflicted with existing allocation + * and monitors. + * + * Returns 1 if @vcpus equals to @resctrl->vcpus, then the monitor will + * share the underlying resctrl group with @resctrl->alloc. Returns - 1 + * if any conflict found. Returns 0 if no conflict and @vcpus is not equal + * to @resctrl->vcpus. + */ +static int +virDomainResctrlMonValidateVcpus(virDomainResctrlDefPtr resctrl, + virBitmapPtr vcpus) +{ + size_t i = 0; + int vcpu = -1; + size_t mons_same_alloc_vcpus = 0; + + if (virBitmapIsAllClear(vcpus)) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("vcpus is empty")); + return -1; + } + + while ((vcpu = virBitmapNextSetBit(vcpus, vcpu)) >= 0) { + if (!virBitmapIsBitSet(resctrl->vcpus, vcpu)) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("Monitor vcpus conflicts with allocation")); + return -1; + } + } + + if (virBitmapEqual(vcpus, resctrl->vcpus)) + return 1; + + for (i = 0; i < resctrl->nmonitors; i++) { + if (virBitmapEqual(resctrl->vcpus, resctrl->monitors[i]->vcpus)) { + mons_same_alloc_vcpus++; + continue; + } + + if (virBitmapOverlaps(vcpus, resctrl->monitors[i]->vcpus)) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("Monitor vcpus conflicts with monitors")); + + return -1; + } + } + + if (mons_same_alloc_vcpus > 1) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("Too many monitors have the same vcpu as allocation")); + return -1; + } + + return 0; +} + + +#define VIR_DOMAIN_RESCTRL_MONITOR_CACHELEVEL 3 + +static int +virDomainResctrlMonDefParse(virDomainDefPtr def, + xmlXPathContextPtr ctxt, + xmlNodePtr node, + virResctrlMonitorType tag, + virDomainResctrlDefPtr resctrl) +{ + virDomainResctrlMonDefPtr domresmon = NULL; + xmlNodePtr oldnode = ctxt->node; + xmlNodePtr *nodes = NULL; + unsigned int level = 0; + char *tmp = NULL; + char *id = NULL; + size_t i = 0; + int n = 0; + int rv = -1; + int ret = -1; + + ctxt->node = node; + + if ((n = virXPathNodeSet("./monitor", ctxt, &nodes)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot extract monitor nodes")); + goto cleanup; + } + + for (i = 0; i < n; i++) { + if (VIR_ALLOC(domresmon) < 0) + goto cleanup; + + domresmon->tag = tag; + + domresmon->instance = virResctrlMonitorNew(); + if (!domresmon->instance) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Could not create monitor")); + goto cleanup; + } + + if (tag == VIR_RESCTRL_MONITOR_TYPE_CACHE) { + tmp = virXMLPropString(nodes[i], "level"); + if (!tmp) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Missing monitor attribute 'level'")); + goto cleanup; + } + + if (virStrToLong_uip(tmp, NULL, 10, &level) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid monitor attribute 'level' value '%s'"), + tmp); + goto cleanup; + } + + if (level != VIR_DOMAIN_RESCTRL_MONITOR_CACHELEVEL) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid monitor cache level '%d'"), + level); + goto cleanup; + } + + VIR_FREE(tmp); + } + + if (virDomainResctrlParseVcpus(def, nodes[i], &domresmon->vcpus) < 0) + goto cleanup; + + rv = virDomainResctrlMonValidateVcpus(resctrl, domresmon->vcpus); + if (rv < 0) + goto cleanup; + + /* If monitor's vcpu list is identical to the vcpu list of the + * associated allocation, set monitor's id to the same value + * as the allocation. */ + if (rv == 1) { + const char *alloc_id = virResctrlAllocGetID(resctrl->alloc); + + if (VIR_STRDUP(id, alloc_id) < 0) + goto cleanup; + } else { + if (!(tmp = virBitmapFormat(domresmon->vcpus))) + goto cleanup; + + if (virAsprintf(&id, "vcpus_%s", tmp) < 0) + goto cleanup; + } + + virResctrlMonitorSetAlloc(domresmon->instance, resctrl->alloc); + + if (virResctrlMonitorSetID(domresmon->instance, id) < 0) + goto cleanup; + + if (VIR_APPEND_ELEMENT(resctrl->monitors, + resctrl->nmonitors, + domresmon) < 0) + goto cleanup; + + VIR_FREE(id); + VIR_FREE(tmp); + } + + ret = 0; + cleanup: + ctxt->node = oldnode; + VIR_FREE(id); + VIR_FREE(tmp); + VIR_FREE(nodes); + virDomainResctrlMonDefFree(domresmon); + return ret; +} + + static virDomainResctrlDefPtr virDomainResctrlNew(xmlNodePtr node, virResctrlAllocPtr alloc, @@ -19054,7 +19243,14 @@ virDomainCachetuneDefParse(virDomainDefPtr def, if (!resctrl) goto cleanup; - if (virResctrlAllocIsEmpty(alloc)) { + if (virDomainResctrlMonDefParse(def, ctxt, node, + VIR_RESCTRL_MONITOR_TYPE_CACHE, + resctrl) < 0) + goto cleanup; + + /* If no <cache> element or <monitor> element in <cachetune>, do not + * append any resctrl element */ + if (!resctrl->nmonitors && virResctrlAllocIsEmpty(alloc)) { ret = 0; goto cleanup; } @@ -27091,12 +27287,41 @@ virDomainCachetuneDefFormatHelper(unsigned int level, static int +virDomainResctrlMonDefFormatHelper(virDomainResctrlMonDefPtr domresmon, + virResctrlMonitorType tag, + virBufferPtr buf) +{ + char *vcpus = NULL; + + if (domresmon->tag != tag) + return 0; + + virBufferAddLit(buf, "<monitor "); + + if (tag == VIR_RESCTRL_MONITOR_TYPE_CACHE) { + virBufferAsprintf(buf, "level='%u' ", + VIR_DOMAIN_RESCTRL_MONITOR_CACHELEVEL); + } + + vcpus = virBitmapFormat(domresmon->vcpus); + if (!vcpus) + return -1; + + virBufferAsprintf(buf, "vcpus='%s'/>\n", vcpus); + + VIR_FREE(vcpus); + return 0; +} + + +static int virDomainCachetuneDefFormat(virBufferPtr buf, virDomainResctrlDefPtr resctrl, unsigned int flags) { virBuffer childrenBuf = VIR_BUFFER_INITIALIZER; char *vcpus = NULL; + size_t i = 0; int ret = -1; virBufferSetChildIndent(&childrenBuf, buf); @@ -27105,6 +27330,13 @@ virDomainCachetuneDefFormat(virBufferPtr buf, &childrenBuf) < 0) goto cleanup; + for (i = 0; i < resctrl->nmonitors; i ++) { + if (virDomainResctrlMonDefFormatHelper(resctrl->monitors[i], + VIR_RESCTRL_MONITOR_TYPE_CACHE, + &childrenBuf) < 0) + goto cleanup; + } + if (virBufferCheckError(&childrenBuf) < 0) goto cleanup; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index e30a4b2..60f6464 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2236,12 +2236,23 @@ struct _virDomainCputune { }; +typedef struct _virDomainResctrlMonDef virDomainResctrlMonDef; +typedef virDomainResctrlMonDef *virDomainResctrlMonDefPtr; +struct _virDomainResctrlMonDef { + virBitmapPtr vcpus; + virResctrlMonitorType tag; + virResctrlMonitorPtr instance; +}; + typedef struct _virDomainResctrlDef virDomainResctrlDef; typedef virDomainResctrlDef *virDomainResctrlDefPtr; struct _virDomainResctrlDef { virBitmapPtr vcpus; virResctrlAllocPtr alloc; + + virDomainResctrlMonDefPtr *monitors; + size_t nmonitors; }; diff --git a/tests/genericxml2xmlindata/cachetune-cdp.xml b/tests/genericxml2xmlindata/cachetune-cdp.xml index 9718f06..9f4c139 100644 --- a/tests/genericxml2xmlindata/cachetune-cdp.xml +++ b/tests/genericxml2xmlindata/cachetune-cdp.xml @@ -8,9 +8,12 @@ <cachetune vcpus='0-1'> <cache id='0' level='3' type='code' size='7680' unit='KiB'/> <cache id='1' level='3' type='data' size='3840' unit='KiB'/> + <monitor level='3' vcpus='0'/> + <monitor level='3' vcpus='1'/> </cachetune> <cachetune vcpus='2'> <cache id='1' level='3' type='code' size='6' unit='MiB'/> + <monitor level='3' vcpus='2'/> </cachetune> <cachetune vcpus='3'> <cache id='1' level='3' type='data' size='6912' unit='KiB'/> diff --git a/tests/genericxml2xmlindata/cachetune-colliding-monitor.xml b/tests/genericxml2xmlindata/cachetune-colliding-monitor.xml new file mode 100644 index 0000000..d481fb5 --- /dev/null +++ b/tests/genericxml2xmlindata/cachetune-colliding-monitor.xml @@ -0,0 +1,30 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>4</vcpu> + <cputune> + <cachetune vcpus='0-1'> + <cache id='0' level='3' type='both' size='768' unit='KiB'/> + <monitor level='3' vcpus='2'/> + </cachetune> + </cputune> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-i686</emulator> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/genericxml2xmlindata/cachetune-small.xml b/tests/genericxml2xmlindata/cachetune-small.xml index ab2d9cf..748be08 100644 --- a/tests/genericxml2xmlindata/cachetune-small.xml +++ b/tests/genericxml2xmlindata/cachetune-small.xml @@ -7,6 +7,13 @@ <cputune> <cachetune vcpus='0-1'> <cache id='0' level='3' type='both' size='768' unit='KiB'/> + <monitor level='3' vcpus='0'/> + <monitor level='3' vcpus='1'/> + <monitor level='3' vcpus='0-1'/> + </cachetune> + <cachetune vcpus='2-3'> + <monitor level='3' vcpus='2'/> + <monitor level='3' vcpus='3'/> </cachetune> </cputune> <os> diff --git a/tests/genericxml2xmltest.c b/tests/genericxml2xmltest.c index fa941f0..4393d44 100644 --- a/tests/genericxml2xmltest.c +++ b/tests/genericxml2xmltest.c @@ -137,6 +137,8 @@ mymain(void) TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_PARSE); DO_TEST_FULL("cachetune-colliding-types", false, true, TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_PARSE); + DO_TEST_FULL("cachetune-colliding-monitor", false, true, + TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_PARSE); DO_TEST("memorytune"); DO_TEST_FULL("memorytune-colliding-allocs", false, true, TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_PARSE); -- 2.7.4

On 11/12/18 8:31 AM, Wang Huaqiang wrote: This should just be: return qemuDomainGetStatsCpuCgroup(dom, record, maxparams); yes, causes a merge conflict in next patch, but that one at least has the return 0 this one would have needed... Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John

On 11/12/18 8:31 AM, Wang Huaqiang wrote: I believe virsh.pod needs an update to list this type of data in the domstats --cpu-total output. In any case, I need to drop off for the evening - I'll pick this up in the morning. You can send an update to this patch and I can merge it in. The first 15 patches look good with some minor adjustments... John Data {

On 11/12/18 8:31 AM, Wang Huaqiang wrote: I'll comment on these in your update... Data { Why "l" (BTW: 'k' would be preferred because '1' and 'l' are very difficult to delineate). Something doesn't quite add up with this... Since we're only filling in types with 'cache' types and erroring out otherwise ... see [1] data points below... [1] Perhaps this should be done up front and "avoided" since this helper should only care above getting cache stats... IOW: for (j = 0; j < resctrl->nmonitors; j++) { virDomainResctrlMonDefPtr domresmon = NULL; virResctrlMonitorPtr monitor = resctrl->monitors[j]->instance; domresmon = resctrl->monitors[j]; if (domresmon->tag != VIR_RESCTRL_MONITOR_TYPE_CACHE) continue; This this loop only fetches cache information and then the 'l' (or preferably 'k') makes more sense Might be nice to add comments... I'd place the above two below the following hunk - perf wise and logically since @tag is the important piece here. However, it may not be important to compare the [i].tag == tag as long as you've done your collection of *only* one type. Hope this makes sense! Thus your code is just: if (nmondata == 0) return 0; [1] In the current scheme of qemuDomainGetStatsCpuResMonitor, how could we get this tag? If your goal was to make a "generic" printing API to be used by both cpustats and memstats (eventually), then perhaps the name of the helper should just be qemuDomainGetStatsResMonitor (or *MonitorParams). Since @tag is a parameter and it's fairly easy to see it's use and the formatting of the params is based purely on the tag in the generically output data. The helper should also be appropriately placed in qemu_driver.c such that when memBW stats support is added it can just be used and doesn't need to be moved. Similar 'l' concerns here use 'j' instead [1] To further the above conversation, @nmonitors won't be limited by cache stats only, but the collection of the @mondata is. So I think here nmonitors should only include tags w/ *TYPE_CACHE. You could pass @nmonitors here and use as 'nmondata' in the above function to "ensure" that the nmonitors looping and filling of mondata doesn't go beyond bounds - although that shouldn't happen, so it's not a requirement as long as the algorithm to calculate @nmonitors and allocate @mondata is the same algorithm used to fill in @mondata. Similar comment here - this is only being called from qemuDomainGetStatsCpu thus it should only pass VIR_RESCTRL_MONITOR_TYPE_CACHE and not be run in a loop. If eventually memBW data was filled in - it would be printed next to cpu-stats data and that doesn't necessarily make sense, does it? This obviously would have some differences based on my comments from patch15. Rather than have you post patches 1-15 again just to fix 16, I'll push 1-15 and let you work on 16 and 17. We still have time before the next release. Besides once I push, we'll find out fairly quickly whether some other arch has build/compile problems! John

I have had some discussion with intel engineers about the MBM and CMT, it is known that these are very similar technologies in underlying hardware but tracking difference set of CPU internal counters. For CMT, the concept is very clear, it is monitoring last level cache and report the cache usage. For MBM it is reports memory bandwidth by tracking counters from cache size, it reflects the DRAM bandwidth, so it will be no problem to call it as memory bandwidth. And I'll submit the updated code for patch16 and 17 and addressing your review comments and suggestions. Since cache monitor is the only monitor we support now, just as you pointed out, I'll not involve memBW monitor tag to avoid confusion. For MBM patches, I'll submit RFC patches to raise discussion about its interface. I'll also rename the new added data structure name and function names, please have a review. BR Huaqiang

On 11/12/18 8:31 AM, Wang Huaqiang wrote: s/monitor by/monitoring using the/ s/threads, and added interface/threads. Added interfaces/ s/to get/to get and display/ s/information/statistics/ s/through command/through the command/ s/./ via the virConnectGetAllDomainStats API. This should be in the 'New features' section - this is definitely not a bug fix! John