On 07/18/2018 03:57 AM, bing.niu(a)intel.com wrote: I'm assuming you are coordinating with the [RFC PATCHv2 00/10] x86 RDT Cache Monitoring Technology (CMT)​ that's on list as well. I don't have the cycles to do any compare/contrast between the two... One argument per line... I can fix before pushing though. Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John

From: Bing Niu <bing.niu(a)intel.com&gt; Refactor virResctrlAllocFormat so that it is easy to support other resource allocation technologies. Signed-off-by: Bing Niu <bing.niu(a)intel.com&gt; --- src/util/virresctrl.c | 43 ++++++++++++++++++++++++++++--------------- 1 file changed, 28 insertions(+), 15 deletions(-) diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 98e7296..1515de2 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -848,17 +848,13 @@ virResctrlAllocGetID(virResctrlAllocPtr alloc) } -char * -virResctrlAllocFormat(virResctrlAllocPtr alloc) +static int +virResctrlAllocFormatCache(virResctrlAllocPtr alloc, virBufferPtr buf) { - virBuffer buf = VIR_BUFFER_INITIALIZER; unsigned int level = 0; unsigned int type = 0; unsigned int cache = 0; - if (!alloc) - return NULL; - for (level = 0; level < alloc->nlevels; level++) { virResctrlAllocPerLevelPtr a_level = alloc->levels[level]; @@ -871,7 +867,7 @@ virResctrlAllocFormat(virResctrlAllocPtr alloc) if (!a_type) continue; - virBufferAsprintf(&buf, "L%u%s:", level, virResctrlTypeToString(type)); + virBufferAsprintf(buf, "L%u%s:", level, virResctrlTypeToString(type)); for (cache = 0; cache < a_type->nmasks; cache++) { virBitmapPtr mask = a_type->masks[cache]; @@ -881,21 +877,38 @@ virResctrlAllocFormat(virResctrlAllocPtr alloc) continue; mask_str = virBitmapToString(mask, false, true); - if (!mask_str) { - virBufferFreeAndReset(&buf); - return NULL; - } + if (!mask_str) + return -1; - virBufferAsprintf(&buf, "%u=%s;", cache, mask_str); + virBufferAsprintf(buf, "%u=%s;", cache, mask_str); VIR_FREE(mask_str); } - virBufferTrim(&buf, ";", 1); - virBufferAddChar(&buf, '\n'); + virBufferTrim(buf, ";", 1); + virBufferAddChar(buf, '\n'); } } - virBufferCheckError(&buf); + if (virBufferCheckError(buf) < 0) + return -1; + else + return 0; +} + + +char * +virResctrlAllocFormat(virResctrlAllocPtr alloc) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (!alloc) + return NULL; + + if (virResctrlAllocFormatCache(alloc, &buf) < 0) { + virBufferFreeAndReset(&buf); + return NULL; + } + return virBufferContentAndReset(&buf); } -- 2.7.4

On 2018年07月25日 06:03, John Ferlan wrote: Thanks for that. ;)

On 07/18/2018 03:57 AM, bing.niu(a)intel.com wrote: Last sentence is duplicitous. The closing */ can be on the previous line. s/ properly/probably/ ?? Is that what you meant? s/mba/memory bandwidth/ right? s/ Hence/ Hence/ It's a raging debate at times on this list whether to go with 1 space or 2. I think 1 space usually wins, but I use 2 many times as well. I'll adjust this to 1 space though ;-) "kernel" can fit on previous line meaning "wrong." can fit on previous line. Also, no need for blank line between here and virReportError I'd just note /* Similar reasoning as min_bandwidth above */ This can be local to the if... virResctrlInfoMemBWPtr membw_info = resctrl->membw_info; This last hunk should be it's own patch. I can split it out for you. The rest of the patch introduces the concept, does the query, and saves the data in the "right place". This hunk says, hey we have some membw_info data that can change our perception of reality, so we need to adjust. Although nothing yet has set last_level_cache or max_id... I'll assume that's comeing. I'll also make membw_info "local" to the if {}. The only hmm... I have is this last hunk, I've already forgotten what we discussed the previous series. But my hmm is related to why is it here and what impact can it (eventually) have if the values are changed in this method while perhaps also being changed in a different method. I'm sure I'll learn more of that as I move forward. Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John BTW: I'm stopping here for the evening, I'll work through the rest hopefully tomorrow depending on interruptions.

From: Bing Niu <bing.niu(a)intel.com&gt; Add memory bandwidth allocation support to virresctrl class. Introducing virResctrlAllocMemBW which is used for allocating memory bandwidth. Following virResctrlAllocPerType, it also employs a nested sparse array to indicate whether allocation is available for particular last level cache. Signed-off-by: Bing Niu <bing.niu(a)intel.com&gt; --- src/util/virresctrl.c | 346 ++++++++++++++++++++++++++++++++++++++++++++++++-- src/util/virresctrl.h | 13 ++ 2 files changed, 346 insertions(+), 13 deletions(-) diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 06e2702..bec2afd 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -36,9 +36,9 @@ VIR_LOG_INIT("util.virresctrl") /* Resctrl is short for Resource Control. It might be implemented for various - * resources, but at the time of this writing this is only supported for cache - * allocation technology (aka CAT). Hence the reson for leaving 'Cache' out of - * all the structure and function names for now (can be added later if needed. + * resources. Currently this supports cache allocation technology (aka CAT) and + * memory bandwidth allocation (aka MBA). More resources technologies may be + * added in feature. */ @@ -89,6 +89,8 @@ typedef virResctrlAllocPerType *virResctrlAllocPerTypePtr; typedef struct _virResctrlAllocPerLevel virResctrlAllocPerLevel; typedef virResctrlAllocPerLevel *virResctrlAllocPerLevelPtr; +typedef struct _virResctrlAllocMemBW virResctrlAllocMemBW; +typedef virResctrlAllocMemBW *virResctrlAllocMemBWPtr; /* Class definitions and initializations */ static virClassPtr virResctrlInfoClass; @@ -181,7 +183,10 @@ virResctrlInfoDispose(void *obj) * consequently a directory under /sys/fs/resctrl). Since it can have multiple * parts of multiple caches allocated it is represented as bunch of nested * sparse arrays (by sparse I mean array of pointers so that each might be NULL - * in case there is no allocation for that particular one (level, cache, ...)). + * in case there is no allocation for that particular cache allocation (level, + * cache, ...) or memory allocation for particular node). + * + * =====Cache allocation technology (CAT)===== * * Since one allocation can be made for caches on different levels, the first * nested sparse array is of types virResctrlAllocPerLevel. For example if you @@ -206,6 +211,16 @@ virResctrlInfoDispose(void *obj) * all of them. While doing that we store the bitmask in a sparse array of * virBitmaps named `masks` indexed the same way as `sizes`. The upper bounds * of the sparse arrays are stored in nmasks or nsizes, respectively. + * + * =====Memory Bandwidth allocation technology (MBA)===== + * + * The memory bandwidth allocation support in virResctrlAlloc works in the same + * fashion as CAT. However, memory bandwidth controller doesn't have a hierarchy + * organization as cache, each node have one memory bandwidth controller to + * memory bandwidth distribution. The number of memory bandwidth controller is + * identical with number of last level cache. So MBA also employs a sparse array + * to represent whether a memory bandwidth allocation happens on corresponding node. + * The available memory controller number is collected in 'virResctrlInfo'. */ struct _virResctrlAllocPerType { /* There could be bool saying whether this is set or not, but since everything @@ -226,12 +241,24 @@ struct _virResctrlAllocPerLevel { * VIR_CACHE_TYPE_LAST number of items */ }; +/* + * virResctrlAllocMemBW represents one memory bandwidth allocation. Since it can have + * several last level caches in a NUMA system, it is also represented as a nested + * sparse arrays as virRestrlAllocPerLevel. + */ +struct _virResctrlAllocMemBW { + unsigned int **bandwidths; + size_t nbandwidths; +}; + struct _virResctrlAlloc { virObject parent; virResctrlAllocPerLevelPtr *levels; size_t nlevels; + virResctrlAllocMemBWPtr mem_bw; + /* The identifier (any unique string for now) */ char *id; /* libvirt-generated path in /sys/fs/resctrl for this particular @@ -275,6 +302,13 @@ virResctrlAllocDispose(void *obj) VIR_FREE(level); } + if (alloc->mem_bw) { + virResctrlAllocMemBWPtr mem_bw = alloc->mem_bw; + for (i = 0; i < mem_bw->nbandwidths; i++) + VIR_FREE(mem_bw->bandwidths[i]); + } + + VIR_FREE(alloc->mem_bw); VIR_FREE(alloc->id); VIR_FREE(alloc->path); VIR_FREE(alloc->levels); @@ -697,6 +731,9 @@ virResctrlAllocIsEmpty(virResctrlAllocPtr alloc) if (!alloc) return true; + if (alloc->mem_bw) + return false; + for (i = 0; i < alloc->nlevels; i++) { virResctrlAllocPerLevelPtr a_level = alloc->levels[i]; @@ -890,6 +927,27 @@ virResctrlAllocSetCacheSize(virResctrlAllocPtr alloc, int +virResctrlAllocForeachMemory(virResctrlAllocPtr alloc, + virResctrlAllocForeachMemoryCallback cb, + void *opaque) +{ + size_t i = 0; + + if (!alloc) + return 0; + + if (alloc->mem_bw) { + virResctrlAllocMemBWPtr mem_bw = alloc->mem_bw; + for (i = 0; i < mem_bw->nbandwidths; i++) + if (mem_bw->bandwidths[i]) + cb(i, *mem_bw->bandwidths[i], opaque); + } + + return 0; +} + + +int virResctrlAllocForeachCache(virResctrlAllocPtr alloc, virResctrlAllocForeachCacheCallback cb, void *opaque) @@ -952,6 +1010,240 @@ virResctrlAllocGetID(virResctrlAllocPtr alloc) } +static void +virResctrlMemoryBandwidthSubtract(virResctrlAllocPtr free, + virResctrlAllocPtr used) +{ + size_t i; + + if (!used->mem_bw) + return; + + for (i = 0; i < used->mem_bw->nbandwidths; i++) { + if (used->mem_bw->bandwidths[i]) + *(free->mem_bw->bandwidths[i]) -= *(used->mem_bw->bandwidths[i]); + } +} + + +int +virResctrlSetMemoryBandwidth(virResctrlAllocPtr alloc, + unsigned int id, + unsigned int memory_bandwidth) +{ + virResctrlAllocMemBWPtr mem_bw = alloc->mem_bw; + + if (!mem_bw) { + if (VIR_ALLOC(mem_bw) < 0) + return -1; + alloc->mem_bw = mem_bw; + } + + if (mem_bw->nbandwidths <= id && + VIR_EXPAND_N(mem_bw->bandwidths, mem_bw->nbandwidths, + id - mem_bw->nbandwidths + 1) < 0) + return -1; + + if (mem_bw->bandwidths[id]) { + virReportError(VIR_ERR_XML_ERROR, + _("Memory Bandwidth already defined for node %u"), + id); + return -1; + } + + if (VIR_ALLOC(mem_bw->bandwidths[id]) < 0) + return -1; + + *(mem_bw->bandwidths[id]) = memory_bandwidth; + return 0; +} + + +static int +virResctrlAllocMemoryBandwidthFormat(virResctrlAllocPtr alloc, + virBufferPtr buf) +{ + size_t i; + + if (!alloc->mem_bw) + return 0; + + virBufferAddLit(buf, "MB:"); + + for (i = 0; i < alloc->mem_bw->nbandwidths; i++) { + if (alloc->mem_bw->bandwidths[i]) { + virBufferAsprintf(buf, "%zd=%u;", i, + *(alloc->mem_bw->bandwidths[i])); + } + } + + virBufferTrim(buf, ";", 1); + virBufferAddChar(buf, '\n'); + if (virBufferCheckError(buf) < 0) + return -1; + else + return 0; +} + + +static int +virResctrlAllocMemoryBandwidth(virResctrlInfoPtr resctrl, + virResctrlAllocPtr alloc, + virResctrlAllocPtr free) +{ + size_t i; + virResctrlAllocMemBWPtr mem_bw_alloc = alloc->mem_bw; + virResctrlAllocMemBWPtr mem_bw_free = free->mem_bw; + virResctrlInfoMemBWPtr mem_bw_info = resctrl->membw_info; + + if (!mem_bw_alloc) + return 0; + + if (mem_bw_alloc && !mem_bw_info) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("RDT Memory Bandwidth allocation " + "unsupported")); + return -1; + } + + for (i = 0; i < mem_bw_alloc->nbandwidths; i++) { + if (!mem_bw_alloc->bandwidths[i]) + continue; + + if (*(mem_bw_alloc->bandwidths[i]) % mem_bw_info->bandwidth_granularity) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Memory Bandwidth allocation of size " + "%u is not divisible by granularity %u"), + *(mem_bw_alloc->bandwidths[i]), + mem_bw_info->bandwidth_granularity); + return -1; + } + if (*(mem_bw_alloc->bandwidths[i]) < mem_bw_info->min_bandwidth) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Memory Bandwidth allocation of size " + "%u is smaller than the minimum " + "allowed allocation %u"), + *(mem_bw_alloc->bandwidths[i]), + mem_bw_info->min_bandwidth); + return -1; + } + if (i > mem_bw_info->max_id) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("bandwidth controller %zd not exist, " + "max controller id %u"), + i, mem_bw_info->max_id); + return -1; + } + if (*(mem_bw_alloc->bandwidths[i]) > *(mem_bw_free->bandwidths[i])) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Not enough room for allocation of %u%% " + "bandwidth on node %zd, available bandwidth %u%%"), + *(mem_bw_alloc->bandwidths[i]), i, + *(mem_bw_free->bandwidths[i])); + return -1; + } + } + return 0; +} + + +static int +virResctrlAllocParseProcessMemoryBandwidth(virResctrlInfoPtr resctrl, + virResctrlAllocPtr alloc, + char *mem_bw) +{ + unsigned int bandwidth; + unsigned int id; + char *tmp = NULL; + + tmp = strchr(mem_bw, '='); + if (!tmp) + return 0; + *tmp = '\0'; + tmp++; + + if (virStrToLong_uip(mem_bw, NULL, 10, &id) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid node id %u "), id); + return -1; + } + if (virStrToLong_uip(tmp, NULL, 10, &bandwidth) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid bandwidth %u"), bandwidth); + return -1; + } + if (bandwidth < resctrl->membw_info->min_bandwidth || + id > resctrl->membw_info->max_id) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing or inconsistent resctrl info for " + "memory bandwidth node '%u'"), id); + return -1; + } + if (alloc->mem_bw->nbandwidths <= id && + VIR_EXPAND_N(alloc->mem_bw->bandwidths, alloc->mem_bw->nbandwidths, + id - alloc->mem_bw->nbandwidths + 1) < 0) { + return -1; + } + if (!alloc->mem_bw->bandwidths[id]) { + if (VIR_ALLOC(alloc->mem_bw->bandwidths[id]) < 0) + return -1; + } + + *(alloc->mem_bw->bandwidths[id]) = bandwidth; + return 0; +} + + +static int +virResctrlAllocParseMemoryBandwidthLine(virResctrlInfoPtr resctrl, + virResctrlAllocPtr alloc, + char *line) +{ + char **mbs = NULL; + char *tmp = NULL; + size_t nmbs = 0; + size_t i; + int ret = -1; + + /* For no reason there can be spaces */ + virSkipSpaces((const char **) &line); + + if (STRNEQLEN(line, "MB", 2)) + return 0; + + if (!resctrl || !resctrl->membw_info || + !resctrl->membw_info->min_bandwidth || + !resctrl->membw_info->bandwidth_granularity) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing or inconsistent resctrl info for " + "memory bandwidth allocation")); + } + + if (!alloc->mem_bw) { + if (VIR_ALLOC(alloc->mem_bw) < 0) + return -1; + } + + tmp = strchr(line, ':'); + if (!tmp) + return 0; + tmp++; + + mbs = virStringSplitCount(tmp, ";", 0, &nmbs); + if (nmbs == 0) + return 0; + + for (i = 0; i < nmbs; i++) { + if (virResctrlAllocParseProcessMemoryBandwidth(resctrl, alloc, mbs[i]) < 0) + goto cleanup; + } + ret = 0; + cleanup: + virStringListFree(mbs); + return ret; +} + + static int virResctrlAllocFormatCache(virResctrlAllocPtr alloc, virBufferPtr buf) { @@ -1013,6 +1305,11 @@ virResctrlAllocFormat(virResctrlAllocPtr alloc) return NULL; } + if (virResctrlAllocMemoryBandwidthFormat(alloc, &buf) < 0) { + virBufferFreeAndReset(&buf); + return NULL; + } + return virBufferContentAndReset(&buf); } @@ -1139,6 +1436,8 @@ virResctrlAllocParse(virResctrlInfoPtr resctrl, lines = virStringSplitCount(schemata, "\n", 0, &nlines); for (i = 0; i < nlines; i++) { + if (virResctrlAllocParseMemoryBandwidthLine(resctrl, alloc, lines[i]) < 0) + goto cleanup; if (virResctrlAllocParseCacheLine(resctrl, alloc, lines[i]) < 0) goto cleanup; } @@ -1273,6 +1572,22 @@ virResctrlAllocNewFromInfo(virResctrlInfoPtr info) } } + /* set default free memory bandwidth to 100%*/ + if (info->membw_info) { + if (VIR_ALLOC(ret->mem_bw) < 0) + goto error; + + if (VIR_EXPAND_N(ret->mem_bw->bandwidths, ret->mem_bw->nbandwidths, + info->membw_info->max_id + 1) < 0) + goto error; + + for (i = 0; i < ret->mem_bw->nbandwidths; i++) { + if (VIR_ALLOC(ret->mem_bw->bandwidths[i]) < 0) + goto error; + *(ret->mem_bw->bandwidths[i]) = 100; + } + } + cleanup: virBitmapFree(mask); return ret; @@ -1284,13 +1599,14 @@ virResctrlAllocNewFromInfo(virResctrlInfoPtr info) /* * This function creates an allocation that represents all unused parts of all - * caches in the system. It uses virResctrlInfo for creating a new full - * allocation with all bits set (using virResctrlAllocNewFromInfo()) and then - * scans for all allocations under /sys/fs/resctrl and subtracts each one of - * them from it. That way it can then return an allocation with only bit set - * being those that are not mentioned in any other allocation. It is used for - * two things, a) calculating the masks when creating allocations and b) from - * tests. + * caches and memory bandwidth in the system. It uses virResctrlInfo for + * creating a new full allocation with all bits set (using + * virResctrlAllocNewFromInfo()), memory bandwidth 100% and then scans + * for all allocations under /sys/fs/resctrl and subtracts each one of them + * from it. That way it can then return an allocation with only bit set + * being those that are not mentioned in any other allocation for CAT and + * available memory bandwidth for MBA. It is used for two things, a) calculating + * the masks and bandwidth available when creating allocations and b) from tests. */ virResctrlAllocPtr virResctrlAllocGetUnused(virResctrlInfoPtr resctrl) @@ -1336,6 +1652,7 @@ virResctrlAllocGetUnused(virResctrlInfoPtr resctrl) goto error; } + virResctrlMemoryBandwidthSubtract(ret, alloc); virResctrlAllocSubtract(ret, alloc); virObjectUnref(alloc); alloc = NULL; @@ -1526,8 +1843,8 @@ virResctrlAllocCopyMasks(virResctrlAllocPtr dst, /* * This function is called when creating an allocation in the system. What it - * does is that it gets all the unused bits using virResctrlAllocGetUnused() and - * then tries to find a proper space for every requested allocation effectively + * does is that it gets all the unused resources using virResctrlAllocGetUnused() + * and then tries to find a proper space for every requested allocation effectively * transforming `sizes` into `masks`. */ static int @@ -1547,6 +1864,9 @@ virResctrlAllocAssign(virResctrlInfoPtr resctrl, if (!alloc_default) goto cleanup; + if (virResctrlAllocMemoryBandwidth(resctrl, alloc, alloc_free) < 0) + goto cleanup; + if (virResctrlAllocCopyMasks(alloc, alloc_default) < 0) goto cleanup; diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h index d657c06..d43fd31 100644 --- a/src/util/virresctrl.h +++ b/src/util/virresctrl.h @@ -73,6 +73,10 @@ typedef int virResctrlAllocForeachCacheCallback(unsigned int level, unsigned long long size, void *opaque); +typedef int virResctrlAllocForeachMemoryCallback(unsigned int id, + unsigned int size, + void *opaque); + virResctrlAllocPtr virResctrlAllocNew(void); @@ -85,6 +89,15 @@ virResctrlAllocSetCacheSize(virResctrlAllocPtr alloc, virCacheType type, unsigned int cache, unsigned long long size); +int +virResctrlAllocForeachMemory(virResctrlAllocPtr resctrl, + virResctrlAllocForeachMemoryCallback cb, + void *opaque); + +int +virResctrlSetMemoryBandwidth(virResctrlAllocPtr alloc, + unsigned int id, + unsigned int memory_bandwidth); int virResctrlAllocForeachCache(virResctrlAllocPtr alloc, -- 2.7.4

On 2018年07月26日 06:37, John Ferlan wrote: Will change. [....] OK~ Let's add a return value testing in patch8. I think we should change cachetune virDomainCachetuneDefFormatHelper part also. Since it also doesn't check return value of virResctrlAllocForeachCache. IMO, Giving a return value of function is always a good practice. ;) It's also give some spaces for the future extension. If logic inside @cb became complex. :) Yes. I just realize put everything relating with ResctrlAlloc into one patch will give difficulties to the reviewer. I should split this patch. So how about we split this patch into: 1. introduce virResctrlAllocMemBW and its alloc and free part 2. introduce schemata file parsing part for MBA. 3. introduce functions used to calculate how much memory bandwidth is used (that is find all groups already create) 4. introduce interface function used to set memory bandwidth (virResctrlSetMemoryBandwidth). 5. introduce logic create resctrl group with memory bandwidth. I will add some comments here. And put into a separated patch. Since patch8 is used to parse XML and create resctrl with MBA information purely. So it's better we don't introduce this interface function together with XML parse. We can introduce interface first and wait our consumer patch8. :) How do you think? Will add comments and put this into a dedicated patch with other schemata file parse and formating functions. OK. Yes~ Yes. If host doesn't have MBA feature, however someone try to allocate memory bandwidth for a VM by mistake. In such a case, we will have resctrl->membw_info == NULL && ->mem_bw != NULL Then below check will throw error. Yes! you are right. Maybe someone try to allocate memory bandwidth on a host without MBA support. Will update. er.... from code, it seems just a validate function to grantee enough memory bandwidth available. We can think this function is allocating memory bandwidth from free_memory_bandwidth. But the free_memory_bandwidth is calculated by 100 - sum(memory bandwidth of existing resctrl group). So there is no any management structure used to track free memory bandwidth as other "allocate" or "free" functions, eg memory manange code. Maybe function name need to be adjust. I am not good at naming :(. Do you have a suggestion? Yes. Such thing should not happen. Above check means find a resctrl group with memory bandwidth information on a host without MBA support. Although it is harmless, we can remove this if you like. min_bandwidth and bandwidth_granularity checks should be part of this patch I think. Those parameters are used for sanity check for memory bandwidth allocation, though they are read in patch 4. Do you agree? :) Thanks for your review and help! :) I just want to echo again. We will split this patch as: 1. introduce virResctrlAllocMemBW and its alloc and free part 2. introduce schemata file parsing part for MBA. 3. introduce functions used to calculate how much memory bandwidth is used (that is find all groups already create) 4. introduce interface function used to set memory bandwidth (virResctrlSetMemoryBandwidth). 5. introduce logic create resctrl group with memory bandwidth. I may do some adjustment(merge or split above 5 patch) when try to split this patch. Anyway, above 5 sub-patches are what we try to achive. How do you think? Bing

From: Bing Niu <bing.niu(a)intel.com&gt; Resctrl not only supports cache tuning, but also memory bandwidth tuning. Renaming cachetune to restune(resource tuning) to reflect that. With restune, all allocation for different resources (cache, memory bandwidth) are aggregated and represented by a virResctrlAllocPtr inside virDomainRestuneDef. Signed-off-by: Bing Niu <bing.niu(a)intel.com&gt; --- src/conf/domain_conf.c | 44 ++++++++++++++++++++++---------------------- src/conf/domain_conf.h | 10 +++++----- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_process.c | 18 +++++++++--------- 4 files changed, 37 insertions(+), 37 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f259b4c..24fefd1 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2963,14 +2963,14 @@ virDomainLoaderDefFree(virDomainLoaderDefPtr loader) static void -virDomainCachetuneDefFree(virDomainCachetuneDefPtr cachetune) +virDomainRestuneDefFree(virDomainRestuneDefPtr restune) { - if (!cachetune) + if (!restune) return; - virObjectUnref(cachetune->alloc); - virBitmapFree(cachetune->vcpus); - VIR_FREE(cachetune); + virObjectUnref(restune->alloc); + virBitmapFree(restune->vcpus); + VIR_FREE(restune); } @@ -3160,9 +3160,9 @@ void virDomainDefFree(virDomainDefPtr def) virDomainShmemDefFree(def->shmems[i]); VIR_FREE(def->shmems); - for (i = 0; i < def->ncachetunes; i++) - virDomainCachetuneDefFree(def->cachetunes[i]); - VIR_FREE(def->cachetunes); + for (i = 0; i < def->nrestunes; i++) + virDomainRestuneDefFree(def->restunes[i]); + VIR_FREE(def->restunes); VIR_FREE(def->keywrap); @@ -18967,7 +18967,7 @@ virDomainCachetuneDefParse(virDomainDefPtr def, xmlNodePtr *nodes = NULL; virBitmapPtr vcpus = NULL; virResctrlAllocPtr alloc = virResctrlAllocNew(); - virDomainCachetuneDefPtr tmp_cachetune = NULL; + virDomainRestuneDefPtr tmp_restune = NULL; char *tmp = NULL; char *vcpus_str = NULL; char *alloc_id = NULL; @@ -18980,7 +18980,7 @@ virDomainCachetuneDefParse(virDomainDefPtr def, if (!alloc) goto cleanup; - if (VIR_ALLOC(tmp_cachetune) < 0) + if (VIR_ALLOC(tmp_restune) < 0) goto cleanup; vcpus_str = virXMLPropString(node, "vcpus"); @@ -19021,8 +19021,8 @@ virDomainCachetuneDefParse(virDomainDefPtr def, goto cleanup; } - for (i = 0; i < def->ncachetunes; i++) { - if (virBitmapOverlaps(def->cachetunes[i]->vcpus, vcpus)) { + for (i = 0; i < def->nrestunes; i++) { + if (virBitmapOverlaps(def->restunes[i]->vcpus, vcpus)) { virReportError(VIR_ERR_XML_ERROR, "%s", _("Overlapping vcpus in cachetunes")); goto cleanup; @@ -19052,16 +19052,16 @@ virDomainCachetuneDefParse(virDomainDefPtr def, if (virResctrlAllocSetID(alloc, alloc_id) < 0) goto cleanup; - VIR_STEAL_PTR(tmp_cachetune->vcpus, vcpus); - VIR_STEAL_PTR(tmp_cachetune->alloc, alloc); + VIR_STEAL_PTR(tmp_restune->vcpus, vcpus); + VIR_STEAL_PTR(tmp_restune->alloc, alloc); - if (VIR_APPEND_ELEMENT(def->cachetunes, def->ncachetunes, tmp_cachetune) < 0) + if (VIR_APPEND_ELEMENT(def->restunes, def->nrestunes, tmp_restune) < 0) goto cleanup; ret = 0; cleanup: ctxt->node = oldnode; - virDomainCachetuneDefFree(tmp_cachetune); + virDomainRestuneDefFree(tmp_restune); virObjectUnref(alloc); virBitmapFree(vcpus); VIR_FREE(alloc_id); @@ -26913,7 +26913,7 @@ virDomainCachetuneDefFormatHelper(unsigned int level, static int virDomainCachetuneDefFormat(virBufferPtr buf, - virDomainCachetuneDefPtr cachetune, + virDomainRestuneDefPtr restune, unsigned int flags) { virBuffer childrenBuf = VIR_BUFFER_INITIALIZER; @@ -26921,7 +26921,7 @@ virDomainCachetuneDefFormat(virBufferPtr buf, int ret = -1; virBufferSetChildIndent(&childrenBuf, buf); - virResctrlAllocForeachCache(cachetune->alloc, + virResctrlAllocForeachCache(restune->alloc, virDomainCachetuneDefFormatHelper, &childrenBuf); @@ -26934,14 +26934,14 @@ virDomainCachetuneDefFormat(virBufferPtr buf, goto cleanup; } - vcpus = virBitmapFormat(cachetune->vcpus); + vcpus = virBitmapFormat(restune->vcpus); if (!vcpus) goto cleanup; virBufferAsprintf(buf, "<cachetune vcpus='%s'", vcpus); if (!(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE)) { - const char *alloc_id = virResctrlAllocGetID(cachetune->alloc); + const char *alloc_id = virResctrlAllocGetID(restune->alloc); if (!alloc_id) goto cleanup; @@ -27062,8 +27062,8 @@ virDomainCputuneDefFormat(virBufferPtr buf, def->iothreadids[i]->iothread_id); } - for (i = 0; i < def->ncachetunes; i++) - virDomainCachetuneDefFormat(&childrenBuf, def->cachetunes[i], flags); + for (i = 0; i < def->nrestunes; i++) + virDomainCachetuneDefFormat(&childrenBuf, def->restunes[i], flags); if (virBufferCheckError(&childrenBuf) < 0) return -1; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 0f10e24..64920fa 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2229,10 +2229,10 @@ struct _virDomainCputune { }; -typedef struct _virDomainCachetuneDef virDomainCachetuneDef; -typedef virDomainCachetuneDef *virDomainCachetuneDefPtr; +typedef struct _virDomainRestuneDef virDomainRestuneDef; +typedef virDomainRestuneDef *virDomainRestuneDefPtr; -struct _virDomainCachetuneDef { +struct _virDomainRestuneDef { virBitmapPtr vcpus; virResctrlAllocPtr alloc; }; @@ -2410,8 +2410,8 @@ struct _virDomainDef { virDomainCputune cputune; - virDomainCachetuneDefPtr *cachetunes; - size_t ncachetunes; + virDomainRestuneDefPtr *restunes; + size_t nrestunes; virDomainNumaPtr numa; virDomainResourceDefPtr resource; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ed76495..a9979ba 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3999,7 +3999,7 @@ qemuDomainDefValidate(const virDomainDef *def, } } - if (def->ncachetunes && + if (def->nrestunes && def->virtType != VIR_DOMAIN_VIRT_KVM) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("cachetune is only supported for KVM domains")); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index c903a8e..233ae94 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2559,7 +2559,7 @@ qemuProcessResctrlCreate(virQEMUDriverPtr driver, virCapsPtr caps = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; - if (!vm->def->ncachetunes) + if (!vm->def->nrestunes) return 0; /* Force capability refresh since resctrl info can change @@ -2568,9 +2568,9 @@ qemuProcessResctrlCreate(virQEMUDriverPtr driver, if (!caps) return -1; - for (i = 0; i < vm->def->ncachetunes; i++) { + for (i = 0; i < vm->def->nrestunes; i++) { if (virResctrlAllocCreate(caps->host.resctrl, - vm->def->cachetunes[i]->alloc, + vm->def->restunes[i]->alloc, priv->machineName) < 0) goto cleanup; } @@ -5376,8 +5376,8 @@ qemuProcessSetupVcpu(virDomainObjPtr vm, &vcpu->sched) < 0) return -1; - for (i = 0; i < vm->def->ncachetunes; i++) { - virDomainCachetuneDefPtr ct = vm->def->cachetunes[i]; + for (i = 0; i < vm->def->nrestunes; i++) { + virDomainRestuneDefPtr ct = vm->def->restunes[i]; if (virBitmapIsBitSet(ct->vcpus, vcpuid)) { if (virResctrlAllocAddPID(ct->alloc, vcpupid) < 0) @@ -7077,8 +7077,8 @@ 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->ncachetunes; i++) - virResctrlAllocRemove(vm->def->cachetunes[i]->alloc); + for (i = 0; i < vm->def->nrestunes; i++) + virResctrlAllocRemove(vm->def->restunes[i]->alloc); qemuProcessRemoveDomainStatus(driver, vm); @@ -7801,8 +7801,8 @@ qemuProcessReconnect(void *opaque) if (qemuConnectAgent(driver, obj) < 0) goto error; - for (i = 0; i < obj->def->ncachetunes; i++) { - if (virResctrlAllocDeterminePath(obj->def->cachetunes[i]->alloc, + for (i = 0; i < obj->def->nrestunes; i++) { + if (virResctrlAllocDeterminePath(obj->def->restunes[i]->alloc, priv->machineName) < 0) goto error; } -- 2.7.4

On 07/18/2018 03:57 AM, bing.niu(a)intel.com wrote: You could make 3 patches out of this - one for each reduction of virDomainCachetuneDefParse... The virDomainFindResctrlAlloc should have used Restune instead of ResctrlAlloc, right? Of course that changes based on how the naming patch shakes out. Actually I think perhaps virDomainRestuneVcpuMatch would be even better (where Restune ends up being whatever shakes out of the previous patch naming decision). In this area, I'm wondering what purpose does @new_alloc serve? If @alloc was already defined, then you added an error message. So the reality is - @new_alloc is always true. Secondary to that you've added a new error related to identical vcpus in the parsing logic. Unfortunately that could make a domain disappear on a libvirtd restart if for some reason it was already defined in that manner. If there's a parsing error because two entries had identical cpus, then there will be an error. Errors would cause a previously OK/found domain to not be defined. So that type of check (now that the code has been around for more than a release) would need to go in some sort of Validation API. This is one of those libvirt define and libvirtd restart quirks. Finally, virDomainUpdateRestune should be virDomainRestuneUpdate since you started with virDomainRestuneParseVcpus and it should go after virDomainCachetuneDefParseCache Hope this all makes sense! John

From: Bing Niu <bing.niu(a)intel.com&gt; Introduce a new section memorytune to support memory bandwidth allocation. This is consistent with existing cachetune. As the example: below: <cputune> ...... <memorytune vcpus='0'> <node id='0' bandwidth='30'/> </memorytune> </cputune> vpus --- vpus subjected to this memory bandwidth. id --- on which node memory bandwidth to be set. bandwidth --- the memory bandwidth percent to set. Signed-off-by: Bing Niu <bing.niu(a)intel.com&gt; --- docs/formatdomain.html.in | 35 ++++ docs/schemas/domaincommon.rng | 17 ++ src/conf/domain_conf.c | 199 +++++++++++++++++++++ .../memorytune-colliding-allocs.xml | 30 ++++ .../memorytune-colliding-cachetune.xml | 32 ++++ tests/genericxml2xmlindata/memorytune.xml | 33 ++++ tests/genericxml2xmltest.c | 5 + 7 files changed, 351 insertions(+) create mode 100644 tests/genericxml2xmlindata/memorytune-colliding-allocs.xml create mode 100644 tests/genericxml2xmlindata/memorytune-colliding-cachetune.xml create mode 100644 tests/genericxml2xmlindata/memorytune.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index a3afe13..4e38446 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -757,6 +757,10 @@ &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;/cachetune&gt; + &lt;memorytune vcpus='0-3'&gt; + &lt;node id='0' bandwidth='60'/&gt; + &lt;/memorytune&gt; + &lt;/cputune&gt; ... &lt;/domain&gt; @@ -950,7 +954,38 @@ </dl> </dd> </dl> + </dd> + <dt><code>memorytune</code></dt> + <dd> + Optional <code>memorytune</code> element can control allocations for + memory bandwidth using the resctrl on the host. Whether or not is this + supported can be gathered from capabilities where some limitations like + minimum bandwidth and required granularity are reported as well. The + required attribute <code>vcpus</code> specifies to which vCPUs this + allocation applies. A vCPU can only be member of one + <code>memorytune</code> element allocations. <code>vcpus</code> specified + by <code>memorytune</code> can be identical to those specified by + <code>cachetune</code>. However they are not allowed to overlap each other. + Supported subelements are: + <dl> + <dt><code>node</code></dt> + <dd> + This element controls the allocation of CPU memory bandwidth and has the + following attributes: + <dl> + <dt><code>id</code></dt> + <dd> + Host node id from which to allocate memory bandwidth. + </dd> + <dt><code>bandwidth</code></dt> + <dd> + The memory bandwidth to allocate from this node. The value by default + is in percentage. + </dd> + </dl> + </dd> + </dl> </dd> </dl> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index f24a563..b4cd96b 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -983,6 +983,23 @@ </oneOrMore> </element> </zeroOrMore> + <zeroOrMore> + <element name="memorytune"> + <attribute name="vcpus"> + <ref name='cpuset'/> + </attribute> + <oneOrMore> + <element name="node"> + <attribute name="id"> + <ref name='unsignedInt'/> + </attribute> + <attribute name="bandwidth"> + <ref name='unsignedInt'/> + </attribute> + </element> + </oneOrMore> + </element> + </zeroOrMore> </interleave> </element> </define> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 695981c..ea9276f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19139,6 +19139,128 @@ virDomainCachetuneDefParse(virDomainDefPtr def, } +static int +virDomainMemorytuneDefParseMemory(xmlXPathContextPtr ctxt, + xmlNodePtr node, + virResctrlAllocPtr alloc) +{ + xmlNodePtr oldnode = ctxt->node; + unsigned int id; + unsigned int bandwidth; + char *tmp = NULL; + int ret = -1; + + ctxt->node = node; + + tmp = virXMLPropString(node, "id"); + if (!tmp) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Missing memorytune attribute 'id'")); + goto cleanup; + } + if (virStrToLong_uip(tmp, NULL, 10, &id) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid memorytune attribute 'id' value '%s'"), + tmp); + goto cleanup; + } + VIR_FREE(tmp); + + tmp = virXMLPropString(node, "bandwidth"); + if (!tmp) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Missing memorytune attribute 'bandwidth'")); + goto cleanup; + } + if (virStrToLong_uip(tmp, NULL, 10, &bandwidth) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid memorytune attribute 'bandwidth' value '%s'"), + tmp); + goto cleanup; + } + VIR_FREE(tmp); + if (virResctrlSetMemoryBandwidth(alloc, id, bandwidth) < 0) + goto cleanup; + + ret = 0; + cleanup: + ctxt->node = oldnode; + VIR_FREE(tmp); + return ret; +} + + +static int +virDomainMemorytuneDefParse(virDomainDefPtr def, + xmlXPathContextPtr ctxt, + xmlNodePtr node, + unsigned int flags) +{ + xmlNodePtr oldnode = ctxt->node; + xmlNodePtr *nodes = NULL; + virBitmapPtr vcpus = NULL; + virResctrlAllocPtr alloc = NULL; + ssize_t i = 0; + int n; + int ret = -1; + bool new_alloc = false; + + ctxt->node = node; + + if (virDomainRestuneParseVcpus(def, node, &vcpus) < 0) + goto cleanup; + + if (virBitmapIsAllClear(vcpus)) { + ret = 0; + goto cleanup; + } + + if ((n = virXPathNodeSet("./node", ctxt, &nodes)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot extract memory nodes under memorytune")); + goto cleanup; + } + + if (virDomainFindResctrlAlloc(def, vcpus, &alloc) < 0) + goto cleanup; + + if (!alloc) { + alloc = virResctrlAllocNew(); + if (!alloc) + goto cleanup; + new_alloc = true; + } else { + alloc = virObjectRef(alloc); + } + + for (i = 0; i < n; i++) { + if (virDomainMemorytuneDefParseMemory(ctxt, nodes[i], alloc) < 0) + goto cleanup; + } + if (virResctrlAllocIsEmpty(alloc)) { + ret = 0; + goto cleanup; + } + /* + * If this is a new allocation, format ID and append to restune, otherwise + * just update the existing alloc information */ + if (new_alloc) { + if (virDomainUpdateRestune(def, node, alloc, vcpus, flags) < 0) + goto cleanup; + vcpus = NULL; + alloc = NULL; + } + + ret = 0; + cleanup: + ctxt->node = oldnode; + virObjectUnref(alloc); + virBitmapFree(vcpus); + VIR_FREE(nodes); + return ret; +} + + static virDomainDefPtr virDomainDefParseXML(xmlDocPtr xml, xmlNodePtr root, @@ -19734,6 +19856,18 @@ virDomainDefParseXML(xmlDocPtr xml, } VIR_FREE(nodes); + if ((n = virXPathNodeSet("./cputune/memorytune", ctxt, &nodes)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot extract memorytune nodes")); + goto error; + } + + for (i = 0; i < n; i++) { + if (virDomainMemorytuneDefParse(def, ctxt, nodes[i], flags) < 0) + goto error; + } + VIR_FREE(nodes); + if (virCPUDefParseXML(ctxt, "./cpu[1]", VIR_CPU_TYPE_GUEST, &def->cpu) < 0) goto error; @@ -27028,6 +27162,68 @@ virDomainCachetuneDefFormat(virBufferPtr buf, static int +virDomainMemorytuneDefFormatHelper(unsigned int id, + unsigned int bandwidth, + void *opaque) +{ + virBufferPtr buf = opaque; + + virBufferAsprintf(buf, + "<node id='%u' bandwidth='%u'/>\n", + id, bandwidth); + return 0; +} + + +static int +virDomainMemorytuneDefFormat(virBufferPtr buf, + virDomainRestuneDefPtr restune, + unsigned int flags) +{ + virBuffer childrenBuf = VIR_BUFFER_INITIALIZER; + char *vcpus = NULL; + int ret = -1; + + virBufferSetChildIndent(&childrenBuf, buf); + virResctrlAllocForeachMemory(restune->alloc, + virDomainMemorytuneDefFormatHelper, + &childrenBuf); + + + if (virBufferCheckError(&childrenBuf) < 0) + goto cleanup; + + if (!virBufferUse(&childrenBuf)) { + ret = 0; + goto cleanup; + } + + vcpus = virBitmapFormat(restune->vcpus); + if (!vcpus) + goto cleanup; + + virBufferAsprintf(buf, "<memorytune vcpus='%s'", vcpus); + + if (!(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE)) { + const char *alloc_id = virResctrlAllocGetID(restune->alloc); + if (!alloc_id) + goto cleanup; + + virBufferAsprintf(buf, " id='%s'", alloc_id); + } + virBufferAddLit(buf, ">\n"); + + virBufferAddBuffer(buf, &childrenBuf); + virBufferAddLit(buf, "</memorytune>\n"); + + ret = 0; + cleanup: + virBufferFreeAndReset(&childrenBuf); + VIR_FREE(vcpus); + return ret; +} + +static int virDomainCputuneDefFormat(virBufferPtr buf, virDomainDefPtr def, unsigned int flags) @@ -27132,6 +27328,9 @@ virDomainCputuneDefFormat(virBufferPtr buf, for (i = 0; i < def->nrestunes; i++) virDomainCachetuneDefFormat(&childrenBuf, def->restunes[i], flags); + for (i = 0; i < def->nrestunes; i++) + virDomainMemorytuneDefFormat(&childrenBuf, def->restunes[i], flags); + if (virBufferCheckError(&childrenBuf) < 0) return -1; diff --git a/tests/genericxml2xmlindata/memorytune-colliding-allocs.xml b/tests/genericxml2xmlindata/memorytune-colliding-allocs.xml new file mode 100644 index 0000000..9b8ebaa --- /dev/null +++ b/tests/genericxml2xmlindata/memorytune-colliding-allocs.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> + <memorytune vcpus='0'> + <node id='0' bandwidth='50'/> + <node id='0' bandwidth='50'/> + </memorytune> + </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/memorytune-colliding-cachetune.xml b/tests/genericxml2xmlindata/memorytune-colliding-cachetune.xml new file mode 100644 index 0000000..5416870 --- /dev/null +++ b/tests/genericxml2xmlindata/memorytune-colliding-cachetune.xml @@ -0,0 +1,32 @@ +<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='code' size='12' unit='KiB'/> + </cachetune> + <memorytune vcpus='0'> + <node id='0' bandwidth='50'/> + </memorytune> + </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/memorytune.xml b/tests/genericxml2xmlindata/memorytune.xml new file mode 100644 index 0000000..ea03e22 --- /dev/null +++ b/tests/genericxml2xmlindata/memorytune.xml @@ -0,0 +1,33 @@ +<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> + <memorytune vcpus='0-1'> + <node id='0' bandwidth='20'/> + <node id='1' bandwidth='30'/> + </memorytune> + <memorytune vcpus='3'> + <node id='0' bandwidth='50'/> + </memorytune> + </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/genericxml2xmltest.c b/tests/genericxml2xmltest.c index 7a4fc1e..e6d4ef2 100644 --- a/tests/genericxml2xmltest.c +++ b/tests/genericxml2xmltest.c @@ -140,6 +140,11 @@ 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("memorytune"); + DO_TEST_FULL("memorytune-colliding-allocs", false, true, + TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_PARSE); + DO_TEST_FULL("memorytune-colliding-cachetune", false, true, + TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_PARSE); DO_TEST("tseg"); -- 2.7.4

On 2018年07月27日 04:36, John Ferlan wrote: OK! Lucky~ OK! I added that before, but deleted since I am not sure which release this series can be accepted OK! Yes! Let's update cachetune part also. Yes. However the minimum allowed memory bandwidth is defined by min_bandwidth of RDT. This is reported in host capability XML. Let's do it in SetMemoryBandwidth. Make XML parse parameters only. :) Above checking is actually for XML define a vcpus that is not exist. If domain define vcpu number as 2. However XML describe cachetune/ memtune <cachetune vcpus='10'> ...... <cachetune> or <memtune vcpus='10'> ...... <memtune> Domain can still be created, but cachetune or memtune is ignored. Since vcpus element is controlling which vcpu thread will be added to corresponding resctrl group. This is existing cachetune behavior. memtune just follow. This is valid in virResctrlSetMemoryBandwidth and throw an error. Since you already set memory bandwidth at node 0. This is good, since you are setting memory bandwidth on different node. If alloc is existing already. Memory bandwidth information is updated in virDomainMemorytuneDefParseMemory and alloc structure is already stored in domain->restunes(will be resallocs in next version :) ). virDomainFindResctrlAlloc is used to find a existing alloc and return its pointer. If alloc is new, we need append new alloc structure to domain->restunes. Let's add a return value check, and also for cachetune part. ;) OK Yes, illegal Yes, you are right. And thanks a lot~ [....]

On 07/18/2018 03:57 AM, bing.niu(a)intel.com wrote: What about virsh available views? And similar to the RDT series what about domstats? I think you can get some good ideas from the RDT CMT RFC that's posted. Not even sure if it's already done internally - but pointing it out... It doesn't have to be done as part of the series, but eventually it may be nice. I'll give the following a cursory look as I have other tasks needing some attention. I'll leave it in the back of my mind that I have to be more thorough on the next pass once I get here. Remove one of the blank lines. This was the only issue I saw in my quick glance - rest seemed OK. John