From: Bing Niu <bing.niu(a)intel.com&gt; Renaming some functions and packing some CAT logic into functions Signed-off-by: Bing Niu <bing.niu(a)intel.com&gt; --- src/conf/domain_conf.c | 2 +- src/libvirt_private.syms | 2 +- src/util/virresctrl.c | 93 +++++++++++++++++++++++++++++++----------------- src/util/virresctrl.h | 16 ++++----- 4 files changed, 70 insertions(+), 43 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 522e0c2..62c412e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -26882,7 +26882,7 @@ virDomainCachetuneDefFormat(virBufferPtr buf, int ret = -1; virBufferSetChildIndent(&childrenBuf, buf); - virResctrlAllocForeachSize(cachetune->alloc, + virResctrlAllocForeachCache(cachetune->alloc, virDomainCachetuneDefFormatHelper, &childrenBuf); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ea24f28..fc17059 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2627,7 +2627,7 @@ virCacheTypeToString; virResctrlAllocAddPID; virResctrlAllocCreate; virResctrlAllocDeterminePath; -virResctrlAllocForeachSize; +virResctrlAllocForeachCache; virResctrlAllocFormat; virResctrlAllocGetID; virResctrlAllocGetUnused; diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index e492a63..e62061f 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -316,11 +316,9 @@ virResctrlUnlock(int fd) } -/* virResctrlInfo-related definitions */ static int -virResctrlGetInfo(virResctrlInfoPtr resctrl) +virResctrlGetCacheInfo(virResctrlInfoPtr resctrl, DIR *dirp) { - DIR *dirp = NULL; char *endptr = NULL; char *tmp_str = NULL; int ret = -1; @@ -332,12 +330,6 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl) virResctrlInfoPerLevelPtr i_level = NULL; virResctrlInfoPerTypePtr i_type = NULL; - rv = virDirOpenIfExists(&dirp, SYSFS_RESCTRL_PATH "/info"); - if (rv <= 0) { - ret = rv; - goto cleanup; - } - while ((rv = virDirRead(dirp, &ent, SYSFS_RESCTRL_PATH "/info")) > 0) { VIR_DEBUG("Parsing info type '%s'", ent->d_name); if (ent->d_name[0] != 'L') @@ -443,12 +435,33 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl) ret = 0; cleanup: - VIR_DIR_CLOSE(dirp); VIR_FREE(i_type); return ret; } +/* virResctrlInfo-related definitions */ +static int +virResctrlGetInfo(virResctrlInfoPtr resctrl) +{ + DIR *dirp = NULL; + int ret = -1; + + ret = virDirOpenIfExists(&dirp, SYSFS_RESCTRL_PATH "/info"); + if (ret <= 0) + goto cleanup; + + ret = virResctrlGetCacheInfo(resctrl, dirp); + if (ret < 0) + goto cleanup; + + ret = 0; + cleanup: + VIR_DIR_CLOSE(dirp); + return ret; +} + + virResctrlInfoPtr virResctrlInfoNew(void) { @@ -773,8 +786,8 @@ virResctrlAllocSetSize(virResctrlAllocPtr alloc, int -virResctrlAllocForeachSize(virResctrlAllocPtr alloc, - virResctrlAllocForeachSizeCallback cb, +virResctrlAllocForeachCache(virResctrlAllocPtr alloc, + virResctrlAllocForeachCacheCallback cb, void *opaque) { int ret = 0; @@ -835,17 +848,14 @@ virResctrlAllocGetID(virResctrlAllocPtr alloc) } -char * -virResctrlAllocFormat(virResctrlAllocPtr alloc) +static int +virResctrlAllocFormatCache(virResctrlAllocPtr alloc, virBufferPtr buf) { - virBuffer buf = VIR_BUFFER_INITIALIZER; + int ret = -1; 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]; @@ -858,7 +868,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]; @@ -868,20 +878,37 @@ virResctrlAllocFormat(virResctrlAllocPtr alloc) continue; mask_str = virBitmapToString(mask, false, true); - if (!mask_str) { - virBufferFreeAndReset(&buf); - return NULL; - } + if (!mask_str) + return ret; - 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'); } } + ret = 0; + virBufferCheckError(buf); + return ret; +} + + +char * +virResctrlAllocFormat(virResctrlAllocPtr alloc) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (!alloc) + return NULL; + + if (virResctrlAllocFormatCache(alloc, &buf) < 0) { + virBufferFreeAndReset(&buf); + return NULL; + } + virBufferCheckError(&buf); return virBufferContentAndReset(&buf); } @@ -939,9 +966,9 @@ virResctrlAllocParseProcessCache(virResctrlInfoPtr resctrl, static int -virResctrlAllocParseProcessLine(virResctrlInfoPtr resctrl, - virResctrlAllocPtr alloc, - char *line) +virResctrlAllocParseCacheLine(virResctrlInfoPtr resctrl, + virResctrlAllocPtr alloc, + char *line) { char **caches = NULL; char *tmp = NULL; @@ -1009,7 +1036,7 @@ virResctrlAllocParse(virResctrlInfoPtr resctrl, lines = virStringSplitCount(schemata, "\n", 0, &nlines); for (i = 0; i < nlines; i++) { - if (virResctrlAllocParseProcessLine(resctrl, alloc, lines[i]) < 0) + if (virResctrlAllocParseCacheLine(resctrl, alloc, lines[i]) < 0) goto cleanup; } @@ -1401,8 +1428,8 @@ virResctrlAllocCopyMasks(virResctrlAllocPtr dst, * transforming `sizes` into `masks`. */ static int -virResctrlAllocMasksAssign(virResctrlInfoPtr resctrl, - virResctrlAllocPtr alloc) +virResctrlAllocAssign(virResctrlInfoPtr resctrl, + virResctrlAllocPtr alloc) { int ret = -1; unsigned int level = 0; @@ -1526,7 +1553,7 @@ virResctrlAllocCreate(virResctrlInfoPtr resctrl, if (lockfd < 0) goto cleanup; - if (virResctrlAllocMasksAssign(resctrl, alloc) < 0) + if (virResctrlAllocAssign(resctrl, alloc) < 0) goto cleanup; alloc_str = virResctrlAllocFormat(alloc); diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h index 9052a2b..26c5f3b 100644 --- a/src/util/virresctrl.h +++ b/src/util/virresctrl.h @@ -67,11 +67,11 @@ virResctrlInfoGetCache(virResctrlInfoPtr resctrl, typedef struct _virResctrlAlloc virResctrlAlloc; typedef virResctrlAlloc *virResctrlAllocPtr; -typedef int virResctrlAllocForeachSizeCallback(unsigned int level, - virCacheType type, - unsigned int cache, - unsigned long long size, - void *opaque); +typedef int virResctrlAllocForeachCacheCallback(unsigned int level, + virCacheType type, + unsigned int cache, + unsigned long long size, + void *opaque); virResctrlAllocPtr virResctrlAllocNew(void); @@ -87,9 +87,9 @@ virResctrlAllocSetSize(virResctrlAllocPtr alloc, unsigned long long size); int -virResctrlAllocForeachSize(virResctrlAllocPtr alloc, - virResctrlAllocForeachSizeCallback cb, - void *opaque); +virResctrlAllocForeachCache(virResctrlAllocPtr alloc, + virResctrlAllocForeachCacheCallback cb, + void *opaque); int virResctrlAllocSetID(virResctrlAllocPtr alloc, -- 2.7.4

On 2018年06月30日 06:36, John Ferlan wrote: Yes. I ran syntax-check, unfortunately it didn't throw a error to me. :( That is due to I refactor virResctrlGetInfo and add a virResctrlGetCacheInfo before virResctrlGetInfo. Actually I didn't touch this line, however the git format diff as that. :( Will do in next version. Will do in next version. Will do in next version. Have a curious question about "one line per argument". Usually we separate arguments into multiple lines only if the line length for putting in one line beyonds 80m character. So in libvert's coding convention, we need put one arguments one line regardless of line length? Because above line doesn't exceed 80 characters. Will fix in next version. OK. Ok. I will remove it from caller and let each callee do virBufferCheckError. And add return value testing parts. OK! Thanks for the clarification here. :) OK! OK! Bing

From: Bing Niu <bing.niu(a)intel.com&gt; Add memory bandwidth allocation support basing on existing virresctrl implementation. Two new structures virResctrlInfoMB and virResctrlAllocMB are introduced. virResctrlInfoMB is used to record host system MBA supporting information, e.g., minimum bandwidth allowed, bandwidth granularity, number of bandwidth controller (same as number of last level cache). virResctrlAllocMB is used for allocating memory bandwidth. Following virResctrlAllocPerType, it also employs a nested sparse array to indicate whether allocation is available for particular llc. Overall, the memory bandwidth allocation follows the same sequence as existing virresctrl cache allocation model. It exposes interfaces for probing host's memory bandwidth capability on initialization time and memory bandwidth allocation on runtime. Signed-off-by: Bing Niu <bing.niu(a)intel.com&gt; --- src/libvirt_private.syms | 2 + src/util/virresctrl.c | 383 ++++++++++++++++++++++++++++++++++++++++++++++- src/util/virresctrl.h | 14 ++ 3 files changed, 397 insertions(+), 2 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fc17059..6925062 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2628,6 +2628,7 @@ virResctrlAllocAddPID; virResctrlAllocCreate; virResctrlAllocDeterminePath; virResctrlAllocForeachCache; +virResctrlAllocForeachMemory; virResctrlAllocFormat; virResctrlAllocGetID; virResctrlAllocGetUnused; @@ -2635,6 +2636,7 @@ virResctrlAllocNew; virResctrlAllocRemove; virResctrlAllocSetID; virResctrlAllocSetSize; +virResctrlSetMemoryBandwidth; virResctrlInfoGetCache; virResctrlInfoNew; diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index e62061f..de736b0 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -80,18 +80,21 @@ typedef virResctrlInfoPerType *virResctrlInfoPerTypePtr; typedef struct _virResctrlInfoPerLevel virResctrlInfoPerLevel; typedef virResctrlInfoPerLevel *virResctrlInfoPerLevelPtr; +typedef struct _virResctrlInfoMB virResctrlInfoMB; +typedef virResctrlInfoMB *virResctrlInfoMBPtr; + typedef struct _virResctrlAllocPerType virResctrlAllocPerType; typedef virResctrlAllocPerType *virResctrlAllocPerTypePtr; typedef struct _virResctrlAllocPerLevel virResctrlAllocPerLevel; typedef virResctrlAllocPerLevel *virResctrlAllocPerLevelPtr; - +typedef struct _virResctrlAllocMB virResctrlAllocMB; +typedef virResctrlAllocMB *virResctrlAllocMBPtr; /* Class definitions and initializations */ static virClassPtr virResctrlInfoClass; static virClassPtr virResctrlAllocClass; - /* virResctrlInfo */ struct _virResctrlInfoPerType { /* Kernel-provided information */ @@ -116,11 +119,29 @@ struct _virResctrlInfoPerLevel { virResctrlInfoPerTypePtr *types; }; +/* Information about memory bandwidth allocation */ +struct _virResctrlInfoMB { + /* minimum memory bandwidth allowed*/ + unsigned int min_bandwidth; + /* bandwidth granularity */ + unsigned int bandwidth_granularity; + /* Maximum number of simultaneous allocations */ + unsigned int max_allocation; + /* level number of last level cache*/ + unsigned int last_level_cache; + /* max id of last level cache, this is used to track + * how many last level cache available in host system + * */ + unsigned int max_id; +}; + struct _virResctrlInfo { virObject parent; virResctrlInfoPerLevelPtr *levels; size_t nlevels; + + virResctrlInfoMBPtr mb_info; }; @@ -146,6 +167,7 @@ virResctrlInfoDispose(void *obj) VIR_FREE(level); } + VIR_FREE(resctrl->mb_info); VIR_FREE(resctrl->levels); } @@ -202,12 +224,23 @@ struct _virResctrlAllocPerLevel { * VIR_CACHE_TYPE_LAST number of items */ }; +/* + * virResctrlAllocMB represents one memory bandwidth allocation. Since it can have + * multiple last level caches in a NUMA system, it is also represented as a nested + * sparse arrays as virRestrlAllocPerLevel + */ +struct _virResctrlAllocMB { + unsigned int **bandwidth; + size_t nsizes; +}; + struct _virResctrlAlloc { virObject parent; virResctrlAllocPerLevelPtr *levels; size_t nlevels; + virResctrlAllocMBPtr mba; /* The identifier (any unique string for now) */ char *id; /* libvirt-generated path in /sys/fs/resctrl for this particular @@ -251,6 +284,13 @@ virResctrlAllocDispose(void *obj) VIR_FREE(level); } + if (alloc->mba) { + virResctrlAllocMBPtr mba = alloc->mba; + for (i = 0; i < mba->nsizes; i++) + VIR_FREE(mba->bandwidth[i]); + } + + VIR_FREE(alloc->mba); VIR_FREE(alloc->id); VIR_FREE(alloc->path); VIR_FREE(alloc->levels); @@ -440,6 +480,61 @@ virResctrlGetCacheInfo(virResctrlInfoPtr resctrl, DIR *dirp) } +static int +virResctrlGetMemoryBandwidthInfo(virResctrlInfoPtr resctrl) +{ + int ret = -1; + int rv = -1; + virResctrlInfoMBPtr i_mb = NULL; + + /* query memory bandwidth allocation info */ + if (VIR_ALLOC(i_mb) < 0) + goto cleanup; + rv = virFileReadValueUint(&i_mb->bandwidth_granularity, + SYSFS_RESCTRL_PATH "/info/MB/bandwidth_gran"); + if (rv == -2) { + /* The file doesn't exist, so it's unusable for us, + * properly mba unsupported */ + VIR_WARN("The path '" SYSFS_RESCTRL_PATH "/info/MB/bandwidth_gran'" + "does not exist"); + ret = 0; + goto cleanup; + } else if (rv < 0) { + /* Other failures are fatal, so just quit */ + goto cleanup; + } + + rv = virFileReadValueUint(&i_mb->min_bandwidth, + SYSFS_RESCTRL_PATH "/info/MB/min_bandwidth"); + if (rv == -2) { + /* If the previous file exists, so should this one. Hence -2 is + * fatal in this case as well (errors out in next condition) - the + * kernel interface might've changed too much or something else is + * wrong. */ + + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot get min bandwidth from resctrl cache info")); + } + if (rv < 0) + goto cleanup; + + rv = virFileReadValueUint(&i_mb->max_allocation, + SYSFS_RESCTRL_PATH "/info/MB/num_closids"); + if (rv == -2) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot get max allocation from resctrl cache info")); + } + if (rv < 0) + goto cleanup; + + VIR_STEAL_PTR(resctrl->mb_info, i_mb); + ret = 0; + cleanup: + VIR_FREE(i_mb); + return ret; +} + + /* virResctrlInfo-related definitions */ static int virResctrlGetInfo(virResctrlInfoPtr resctrl) @@ -451,6 +546,10 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl) if (ret <= 0) goto cleanup; + ret = virResctrlGetMemoryBandwidthInfo(resctrl); + if (ret < 0) + goto cleanup; + ret = virResctrlGetCacheInfo(resctrl, dirp); if (ret < 0) goto cleanup; @@ -492,6 +591,9 @@ virResctrlInfoIsEmpty(virResctrlInfoPtr resctrl) if (!resctrl) return true; + if (resctrl->mb_info) + return false; + for (i = 0; i < resctrl->nlevels; i++) { virResctrlInfoPerLevelPtr i_level = resctrl->levels[i]; @@ -517,12 +619,26 @@ virResctrlInfoGetCache(virResctrlInfoPtr resctrl, { virResctrlInfoPerLevelPtr i_level = NULL; virResctrlInfoPerTypePtr i_type = NULL; + virResctrlInfoMBPtr mb_info = NULL; size_t i = 0; int ret = -1; if (virResctrlInfoIsEmpty(resctrl)) return 0; + /* Let's take the opportunity to update the number of + * last level cache. This number is used to calculate + * free memory bandwidth */ + if (resctrl->mb_info) { + mb_info = resctrl->mb_info; + if (level > mb_info->last_level_cache) { + mb_info->last_level_cache = level; + mb_info->max_id = 0; + } else if (mb_info->last_level_cache == level) { + mb_info->max_id++; + } + } + if (level >= resctrl->nlevels) return 0; @@ -593,6 +709,9 @@ virResctrlAllocIsEmpty(virResctrlAllocPtr alloc) if (!alloc) return true; + if (alloc->mba) + return false; + for (i = 0; i < alloc->nlevels; i++) { virResctrlAllocPerLevelPtr a_level = alloc->levels[i]; @@ -786,6 +905,27 @@ virResctrlAllocSetSize(virResctrlAllocPtr alloc, int +virResctrlAllocForeachMemory(virResctrlAllocPtr alloc, + virResctrlAllocForeachMemoryCallback cb, + void *opaque) +{ + unsigned int id = 0; + + if (!alloc) + return 0; + + if (alloc->mba) { + virResctrlAllocMBPtr mba = alloc->mba; + for (id = 0; id < mba->nsizes; id++) + if (mba->bandwidth[id]) + cb(id, *mba->bandwidth[id], opaque); + } + + return 0; +} + + +int virResctrlAllocForeachCache(virResctrlAllocPtr alloc, virResctrlAllocForeachCacheCallback cb, void *opaque) @@ -848,6 +988,217 @@ virResctrlAllocGetID(virResctrlAllocPtr alloc) } +static void +virResctrlMemoryBandwidthSubstract(virResctrlAllocPtr free, + virResctrlAllocPtr used) +{ + size_t i; + + if (used->mba) { + for (i = 0; i < used->mba->nsizes; i++) { + if (used->mba->bandwidth[i]) + *(free->mba->bandwidth[i]) -= *(used->mba->bandwidth[i]); + } + } +} + + +int +virResctrlSetMemoryBandwidth(virResctrlAllocPtr alloc, + unsigned int id, + unsigned int memory_bandwidth) +{ + virResctrlAllocMBPtr mba = alloc->mba; + + if (!mba) { + if (VIR_ALLOC(mba) < 0) + return -1; + alloc->mba = mba; + } + + if (mba->nsizes <= id && + VIR_EXPAND_N(mba->bandwidth, mba->nsizes, + id - mba->nsizes + 1) < 0) + return -1; + + if (mba->bandwidth[id]) { + virReportError(VIR_ERR_XML_ERROR, + _("Collision Memory Bandwidth on node %d"), + id); + return -1; + } + + if (VIR_ALLOC(mba->bandwidth[id]) < 0) + return -1; + + *(mba->bandwidth[id]) = memory_bandwidth; + return 0; +} + + +static int +virResctrlAllocMemoryBandwidthFormat(virResctrlAllocPtr alloc, virBufferPtr buf) +{ + int id; + + if (!alloc->mba) + return 0; + + virBufferAddLit(buf, "MB:"); + + for (id = 0; id < alloc->mba->nsizes; id++) { + if (alloc->mba->bandwidth[id]) { + virBufferAsprintf(buf, "%u=%u;", id, + *(alloc->mba->bandwidth[id])); + } + } + + virBufferTrim(buf, ";", 1); + virBufferAddChar(buf, '\n'); + virBufferCheckError(buf); + return 0; +} + + +static int +virResctrlAllocMemoryBandwidth(virResctrlInfoPtr resctrl, + virResctrlAllocPtr alloc, virResctrlAllocPtr free) +{ + int id; + int ret = -1; + virResctrlAllocMBPtr mb_alloc = alloc->mba; + virResctrlAllocMBPtr mb_free = free->mba; + virResctrlInfoMBPtr mb_info = resctrl->mb_info; + + if (!mb_alloc) { + ret = 0; + return ret; + } + + if (mb_alloc && !mb_info) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("RDT Memory Bandwidth allocation" + " unsupported")); + return ret; + } + + for (id = 0; id < mb_alloc->nsizes; id ++) { + if (!mb_alloc->bandwidth[id]) + continue; + + if (*(mb_alloc->bandwidth[id]) % mb_info->bandwidth_granularity) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Memory Bandwidth allocation of size " + "%u is not divisible by granularity %u"), + *(mb_alloc->bandwidth[id]), + mb_info->bandwidth_granularity); + return ret; + } + if (*(mb_alloc->bandwidth[id]) < mb_info->min_bandwidth) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Memory Bandwidth allocation of size " + "%u is smaller than the minimum " + "allowed allocation %u"), + *(mb_alloc->bandwidth[id]), + mb_info->min_bandwidth); + return ret; + } + if (id > mb_info->max_id) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("bandwidth controller %u not exist," + " max controller id %u"), + id, mb_info->max_id); + return ret; + } + if (*(mb_alloc->bandwidth[id]) > *(mb_free->bandwidth[id])) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Not enough room for allocation of %u " + "bandwidth for node %u%%, freed %u%%"), + id, *(mb_alloc->bandwidth[id]), + *(mb_free->bandwidth[id])); + return ret; + } + } + ret = 0; + return ret; +} + + +static int +virResctrlAllocParseMemoryBandwidthLine(virResctrlInfoPtr resctrl, + virResctrlAllocPtr alloc, + char *line) +{ + char **mbs = NULL; + char *tmp = NULL; + unsigned int bandwidth; + size_t nmb = 0; + unsigned int id; + 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->mb_info + || !resctrl->mb_info->min_bandwidth + || !resctrl->mb_info->bandwidth_granularity) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing or inconsistent resctrl info for " + "memory bandwidth allocation")); + } + + if (!alloc->mba) { + if (VIR_ALLOC(alloc->mba) < 0) + return -1; + } + + tmp = strchr(line, ':'); + if (!tmp) + return 0; + tmp++; + + mbs = virStringSplitCount(tmp, ";", 0, &nmb); + if (!nmb) + return 0; + + for (i = 0; i < nmb; i++) { + tmp = strchr(mbs[i], '='); + *tmp = '\0'; + tmp++; + + if (virStrToLong_uip(mbs[i], NULL, 10, &id) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid node id %u "), id); + goto cleanup; + } + if (virStrToLong_uip(tmp, NULL, 10, &bandwidth) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid bandwidth %u"), bandwidth); + goto cleanup; + } + if (alloc->mba->nsizes <= id && + VIR_EXPAND_N(alloc->mba->bandwidth, alloc->mba->nsizes, + id - alloc->mba->nsizes + 1) < 0) { + goto cleanup; + } + if (!alloc->mba->bandwidth[id]) { + if (VIR_ALLOC(alloc->mba->bandwidth[id]) < 0) + goto cleanup; + } + + *(alloc->mba->bandwidth[id]) = bandwidth; + } + ret = 0; + cleanup: + virStringListFree(mbs); + return ret; +} + + static int virResctrlAllocFormatCache(virResctrlAllocPtr alloc, virBufferPtr buf) { @@ -909,6 +1260,11 @@ virResctrlAllocFormat(virResctrlAllocPtr alloc) return NULL; } + if (virResctrlAllocMemoryBandwidthFormat(alloc, &buf) < 0) { + virBufferFreeAndReset(&buf); + return NULL; + } + virBufferCheckError(&buf); return virBufferContentAndReset(&buf); } @@ -1036,6 +1392,9 @@ 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; } @@ -1170,6 +1529,22 @@ virResctrlAllocNewFromInfo(virResctrlInfoPtr info) } } + /* set default free memory bandwidth to 100%*/ + if (info->mb_info) { + if (VIR_ALLOC(ret->mba) < 0) + goto error; + + if (VIR_EXPAND_N(ret->mba->bandwidth, ret->mba->nsizes, + info->mb_info->max_id - ret->mba->nsizes + 1) < 0) + goto error; + + for (i = 0; i < ret->mba->nsizes; i++) { + if (VIR_ALLOC(ret->mba->bandwidth[i]) < 0) + goto error; + *(ret->mba->bandwidth[i]) = 100; + } + } + cleanup: virBitmapFree(mask); return ret; @@ -1233,6 +1608,7 @@ virResctrlAllocGetUnused(virResctrlInfoPtr resctrl) goto error; } + virResctrlMemoryBandwidthSubstract(ret, alloc); virResctrlAllocSubtract(ret, alloc); virObjectUnref(alloc); alloc = NULL; @@ -1444,6 +1820,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 26c5f3b..5e78334 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); @@ -92,6 +96,16 @@ virResctrlAllocForeachCache(virResctrlAllocPtr alloc, void *opaque); int +virResctrlAllocForeachMemory(virResctrlAllocPtr resctrl, + virResctrlAllocForeachMemoryCallback cb, + void *opaque); + +int +virResctrlSetMemoryBandwidth(virResctrlAllocPtr alloc, + unsigned int id, + unsigned int memory_bandwidth); + +int virResctrlAllocSetID(virResctrlAllocPtr alloc, const char *id); const char * -- 2.7.4

On 2018年06月30日 06:39, John Ferlan wrote: Will fix. OK, will change to "Add memory bandwidth allocation support based on existing virresctrl, so that virresctrl can fit cache allocation and memory bandwidth allocation concurrently" Good idea. Will split virResctrlInfoMB and virResctrlAllocMB in next round. agree and will change to MemBW. Since we already discussed in patch3. I will not reply here. :) Unfortunately syntax-check didn't throw a error to me. And will fix to keep alphabetically. Will fix. Will fix. Will fix by remove that space. OK. will change in next version. Yes. I just notice that, I will add memory bandwidth part in the description of virResctrlAlloc. Will fix above two. I prefer to MemBW since there is no *Alloc* in levels. :) Will fix those two. OK！ Okay, Will change above two. OK. Will fix in next version. again. I am not sure whether this is a "max 80 character in one line" rule in libvirt. :). Anyway I will use one new line for new argument. Will fix above three. OK! Will fix above two. My bad. and will fix Here id is the node of the current memory bandwidth happen. It should be not larger than the max node number which is populated during traverse host cache hierarchy. OK! This error message is to tell the current requested memory bandwidth and current free memory bandwidth. If requesting 60% bandwidth, however there is a domain already allocating 50% bandwidth. Above error will fire. OK No, this is to parse the memory bandwidth information of existing resctrl groups. Above "MB" is the format return by RDT kernel module. Not somthing define by us. OK! OK I will change to "if (nmb != 0)" OK I will add a NULL pointer testing here Well, this function is used to collect the exsiting resctrl groups allocating information to calculate the free memory bandwidth. This virResctrlAllocPtr will be disposed as soon as we finish calculating or error happen. And virResctrlSetMemoryBandwidth is used by domain_conf set memory bandwidth from XML. Those two virResctrlAllocPtrs are unrelative. So I think it's safe. I should change to if (VIR_EXPAND_N(ret->mba->bandwidth, ret->mba->nsizes, info->mb_info->max_id + 1) < 0) This is used to initialize virResctrlAllocPtr basing on virResctrlInfoPtr. OK Yep! I will update function description in next version. Bing

On 06/15/2018 05:29 AM, bing.niu(a)intel.com wrote: Not so sure I'm liking [R|r]estune[s]... Still @cachetunes is an array of a structure that contains a vCPU bitmap and a virResctrlAllocPtr for the /cputune/cachetunes data. The virResctrlAllocPtr was changed to add a the bandwidth allocation, so does this really have to change? I think the question is - if both cachetunes and memtunes exist in domain XML, then for which does the @vcpus relate to. That is, from the cover there's: <memorytune vcpus='0-1'> <node id='0' bandwidth='20'/> </memorytune> and then there's a <cachetune vcpus='0-3'> <cache id='0' level='3' type='both' size='3' unit='MiB'/> <cache id='1' level='3' type='both' size='3' unit='MiB'/> </cachetune> Now what? I haven't looked at patch 4 yet to even know how this "works". I think what you want to do is create a virDomainMemtuneDef that mimics virDomainCachetuneDef, but I'm not 100% sure. Of course that still leaves me wondering how much of virResctrlAllocPtr then becomes wasted. You chose an integration point, but essentially have separate technologies. I'm trying to make sense of it all and am starting to get lost in the process of doing that. If @bandwidth can not be > 100... Can we duplicate the @id somehow? So how does that free buffer that got 100% and then gets subtracted from really work in this model? I probably need to study the code some more, but it's not clear it requires using the "same" sort of logic that cachetune uses where there could be different types (instructs, data, or both) that would be drawing from the size, right? I think that's how that worked. So what's the corollary for memtunes? You set a range from 0 to 100, right and that's it. OK, too many questions and it's late so I'll stop here... Hopefully I didn't leave any stray thoughts in the review of patch 1 or 2. John

On 07/03/2018 12:10 AM, bing.niu wrote: tl;dr :-) So some day soon it'll be more complicated and have even more rules?!?! IOW: A vCPU or the vCPUs being used by CAT and MBA would need to be the same because in "this world" the group *is* the single vCPU or the range of vCPU's. More lengthy things that I didn't read... Thanks for the pointers. I think keeping track of all the rules and relationships is going to be a real "challenge". Yay, pictures! Thanks for taking the time. Yes and I think further has me believe that the vCPUs in this case are the groups and perhaps becomes the internal "key" for all this. I didn't go look, but have to assume multiple cachetune entries create multiple groups into which the vCPU's for the cachetune entries are placed. So far it's been easy because there's no way to overlap cachetune vCPU values (my assumption, I didn't look at the code). Now we add memtune and dictate that if it uses a vCPU or set of vCPUs that it either matches the existing cachetune entries or is itself unique and doesn't overlap any cachetune. Assume the following: <cachetune vcpus='0-1'> <cachetune vcpus='2-4'> <cachetune vcpus='5-7'> A valid memtune must also be '0-1', '2-4', 5-7', or any new grouping from 8->maxvcpus, such as '8-11'. Conversely, if <memtune vcpus='0-4'> was used, that's invalid. Similarly <memtune vcpus='3'> would be invalid. For the former, it's crossing two boundaries and for the latter it's "part of" some other boundary. Hopefully that's right... I think I was right above based on this part... Unfortunately forward thinking about memorytune didn't happen, so what you propose above is what I think "internally" once we've read the <cachetune> and <memorytune> elements we end up with. And yes, it makes sense why you renamed things now and why the vcpus are essentially reused between the two. Not a fan of restunes, RsrcDir is closer, but not a perfect shorter name the ResourceDirectory. Also rather than essentially cut-copy-paste for the vcpus logic into memtunes from cachetunes, that should be extracted into it's own helper. I didn't go back to the RFC when I reviewed. I tried to have a fresh look. After scanning Pavel's comments now though, I do agree having a <node> inside a <cachetune> to go along with a <cache> element is a bit confusing. But I do now see where the "math" is for the total bandwidth that can be used - it's the per node value. Something that I didn't see during my first pass. Of course I may have realized that had the example in the commit had more than 1 vCPU and 1 node. There is no deprecation possible. You will always have to support the cachetune as a direct child of cputune. If you "fix" things up such that you introduce some other means to have a cputune/resourcetune/cachetune working before you introduce memtune as a child of resourcetune, then that works. But you'll have to know whether you read cputune/cachetune and then format out in that manner. See how the <auth> element has been handled as a "child" of the disk element originally, but now possible as a child of the source element of the disk. It's possible, but messy. Now that the picture is becoming clearer, I can see your point. Right - as noted. I didn't read that originally. I jumped into a review for this series mainly because no one else had started reviewing it. I think I've explained what I missed above... Essentially your example had 1 vCPU and thus 1 node and so it didn't make sense about the 100%. Of course if there were 3 vCPU's and 3 nodes each having some percentage, yeah then it would have been more obvious. My suggestion, let's try to get through reworking how the cachetunes data is stored. Some sort of common API to "add" a new vCPU's value that can be used by cachetune now and eventually memtune later. The vCPUs being the obvious key rather than cachetune. Fair warning - I have some days out of the office coming up over the next week or so. 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: <memorytune vcpus='0'> <node id='0' bandwidth='30'/> </memorytune> 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; --- src/conf/domain_conf.c | 260 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 260 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b3543f3..dbfd69f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19076,6 +19076,189 @@ 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; + virDomainRestuneDefPtr tmp_cachetune = NULL; + char *tmp = NULL; + char *vcpus_str = NULL; + char *alloc_id = NULL; + ssize_t i = 0; + int n; + int ret = -1; + bool new_alloc = false; + + ctxt->node = node; + + if (VIR_ALLOC(tmp_cachetune) < 0) + goto cleanup; + + vcpus_str = virXMLPropString(node, "vcpus"); + if (!vcpus_str) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Missing memorytune attribute 'vcpus'")); + goto cleanup; + } + if (virBitmapParse(vcpus_str, &vcpus, VIR_DOMAIN_CPUMASK_LEN) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid memorytune attribute 'vcpus' value '%s'"), + vcpus_str); + goto cleanup; + } + + /* We need to limit the bitmap to number of vCPUs. If there's nothing left, + * then we can just clean up and return 0 immediately */ + virBitmapShrink(vcpus, def->maxvcpus); + + 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; + } + for (i = 0; i < def->nrestunes; i++) { + /* vcpus group has been created, directly use the existing one. + * Just updating memory allocation information of that group + * */ + if (virBitmapEqual(def->restunes[i]->vcpus, vcpus)) { + alloc = def->restunes[i]->alloc; + break; + } + + /* vcpus can not overlapping */ + if (virBitmapOverlaps(def->restunes[i]->vcpus, vcpus)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Overlapping vcpus in memorytunes")); + goto cleanup; + } + } + + if (!alloc) { + alloc = virResctrlAllocNew(); + new_alloc = true; + } + + for (i = 0; i < n; i++) { + if (virDomainMemorytuneDefParseMemory(ctxt, nodes[i], alloc) < 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 (virResctrlAllocIsEmpty(alloc)) { + ret = 0; + goto cleanup; + } + + + /* We need to format it back because we need to be consistent in the naming + * even when users specify some "sub-optimal" string there. */ + VIR_FREE(vcpus_str); + vcpus_str = virBitmapFormat(vcpus); + if (!vcpus_str) + goto cleanup; + + if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE)) + alloc_id = virXMLPropString(node, "id"); + + if (!alloc_id) { + /* The number of allocations is limited and the directory structure is flat, + * not hierarchical, so we need to have all same allocations in one + * directory, so it's nice to have it named appropriately. For now it's + * 'vcpus_...' but it's designed in order for it to be changeable in the + * future (it's part of the status XML). */ + if (virAsprintf(&alloc_id, "vcpus_%s", vcpus_str) < 0) + goto cleanup; + } + + if (virResctrlAllocSetID(alloc, alloc_id) < 0) + goto cleanup; + + VIR_STEAL_PTR(tmp_cachetune->vcpus, vcpus); + VIR_STEAL_PTR(tmp_cachetune->alloc, alloc); + + if (VIR_APPEND_ELEMENT(def->restunes, def->nrestunes, tmp_cachetune) < 0) + goto cleanup; + } + ret = 0; + cleanup: + ctxt->node = oldnode; + virDomainRestuneDefFree(tmp_cachetune); + if (new_alloc) + virObjectUnref(alloc); + virBitmapFree(vcpus); + VIR_FREE(alloc_id); + VIR_FREE(vcpus_str); + VIR_FREE(nodes); + VIR_FREE(tmp); + return ret; +} + + static virDomainDefPtr virDomainDefParseXML(xmlDocPtr xml, xmlNodePtr root, @@ -19671,6 +19854,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; @@ -26922,6 +27117,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) @@ -27026,6 +27283,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; -- 2.7.4

On 2018年06月30日 06:47, John Ferlan wrote: OK! I will add docs/schemas/*.rng for patch4 and patch5. Bing