On 2018年07月25日 06:02, John Ferlan wrote:

On 07/18/2018 03:57 AM, bing.niu@intel.com wrote:

...

From: Bing Niu <bing.niu@intel.com>

Separate resctrl common information parts from CAT specific parts, so that common information parts can be reused among different resource allocation technologies.

Signed-off-by: Bing Niu <bing.niu@intel.com> --- src/util/virresctrl.c | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-)

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...

Yup~ Huaqiang in cc list is working on CMT part. We already sync off line. Since monitor is more complicated and a brand new implementing to replace old perf based monitor functions. Huaqiang will absorb Martin's comment and update his RFC patch. :)

...

diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 6d69c8d..98e7296 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -318,9 +318,8 @@ virResctrlUnlock(int fd)

/* virResctrlInfo-related definitions */ static int -virResctrlGetInfo(virResctrlInfoPtr resctrl) +virResctrlGetCacheInfo(virResctrlInfoPtr resctrl, DIR *dirp)

One argument per line... I can fix before pushing though.

My bad! I should remember one argument per line for function, instead of just splitting line only beyond 80 characters. :(

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

On 07/18/2018 03:57 AM, bing.niu@intel.com wrote:

From: Bing Niu <bing.niu@intel.com>

Refactor virResctrlAllocFormat so that it is easy to support other resource allocation technologies.

Signed-off-by: Bing Niu <bing.niu@intel.com> --- 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)

One argument per line. I'll adjust.

{ - virBuffer buf = VIR_BUFFER_INITIALIZER; unsigned int level = 0; unsigned int type = 0; unsigned int cache = 0;

[...]

- virBufferCheckError(&buf); + if (virBufferCheckError(buf) < 0) + return -1; + else + return 0;

Just return virBufferCheckError(buf); directly I'll adjust before pushing. Reviewed-by: John Ferlan <jferlan@redhat.com> John

On 2018年07月25日 06:08, John Ferlan wrote:

On 07/18/2018 03:57 AM, bing.niu@intel.com wrote:

...

From: Bing Niu <bing.niu@intel.com>

Introducing virResctrlInfoMemBW for the information memory bandwidth allocation information. Those information is used for memory bandwidth allocating.

Last sentence is duplicitous.

...

Signed-off-by: Bing Niu <bing.niu@intel.com> --- src/util/virresctrl.c | 104 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 104 insertions(+)

diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 1515de2..06e2702 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -80,6 +80,9 @@ typedef virResctrlInfoPerType *virResctrlInfoPerTypePtr; typedef struct _virResctrlInfoPerLevel virResctrlInfoPerLevel; typedef virResctrlInfoPerLevel *virResctrlInfoPerLevelPtr;

+typedef struct _virResctrlInfoMemBW virResctrlInfoMemBW; +typedef virResctrlInfoMemBW *virResctrlInfoMemBWPtr; + typedef struct _virResctrlAllocPerType virResctrlAllocPerType; typedef virResctrlAllocPerType *virResctrlAllocPerTypePtr;

@@ -116,11 +119,31 @@ struct _virResctrlInfoPerLevel { virResctrlInfoPerTypePtr *types; };

+/* Information about memory bandwidth allocation */ +struct _virResctrlInfoMemBW { + /* 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, + * the number of memory bandwidth allocation controller + * is identical with last level cache. + */

The closing */ can be on the previous line.

...

+ unsigned int max_id; +}; + struct _virResctrlInfo { virObject parent;

virResctrlInfoPerLevelPtr *levels; size_t nlevels; + + virResctrlInfoMemBWPtr membw_info; };

@@ -146,6 +169,7 @@ virResctrlInfoDispose(void *obj) VIR_FREE(level); }

+ VIR_FREE(resctrl->membw_info); VIR_FREE(resctrl->levels); }

@@ -442,6 +466,65 @@ virResctrlGetCacheInfo(virResctrlInfoPtr resctrl, DIR *dirp)

static int +virResctrlGetMemoryBandwidthInfo(virResctrlInfoPtr resctrl) +{ + int ret = -1; + int rv = -1; + virResctrlInfoMemBWPtr i_membw = NULL; + + /* query memory bandwidth allocation info */ + if (VIR_ALLOC(i_membw) < 0) + goto cleanup; + rv = virFileReadValueUint(&i_membw->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 */

s/ properly/probably/

?? Is that what you meant?

Yes! probably. :)

s/mba/memory bandwidth/

s/mba/memory bandwidth allocation/

right?

...

+ VIR_INFO("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_membw->min_bandwidth, + SYSFS_RESCTRL_PATH "/info/MB/min_bandwidth"); + if (rv == -2) { + /* If the previous file exists, so should this one. Hence -2 is

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 ;-)

Good to know this history :). Above 2 space is just following the existing virresctrl code.

...

+ * fatal in this case (errors out in next condition) - the + * kernel interface might've changed too much or something else is + * wrong. */

"kernel" can fit on previous line meaning "wrong." can fit on previous line. Also, no need for blank line between here and virReportError

Sorry I am a bit lost. Do you mean put "kernel" in previous line? right? I have a try in Thunderbird. Looks like put “kernel” in previous line beyond max length of line and Thunderbird give me automatically split. :(

...

+ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot get min bandwidth from resctrl memory info")); + } + if (rv < 0) + goto cleanup; + + rv = virFileReadValueUint(&i_membw->max_allocation, + SYSFS_RESCTRL_PATH "/info/MB/num_closids"); + 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. */

I'd just note /* Similar reasoning as min_bandwidth above */

Short and clear!

...

+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot get max allocation from resctrl memory info")); + } + if (rv < 0) + goto cleanup; + + VIR_STEAL_PTR(resctrl->membw_info, i_membw); + ret = 0; + cleanup: + VIR_FREE(i_membw); + return ret; +} + + +static int virResctrlGetInfo(virResctrlInfoPtr resctrl) { DIR *dirp = NULL; @@ -451,6 +534,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 +579,9 @@ virResctrlInfoIsEmpty(virResctrlInfoPtr resctrl) if (!resctrl) return true;

+ if (resctrl->membw_info) + return false; + for (i = 0; i < resctrl->nlevels; i++) { virResctrlInfoPerLevelPtr i_level = resctrl->levels[i];

@@ -517,12 +607,26 @@ virResctrlInfoGetCache(virResctrlInfoPtr resctrl, { virResctrlInfoPerLevelPtr i_level = NULL; virResctrlInfoPerTypePtr i_type = NULL; + virResctrlInfoMemBWPtr membw_info = NULL;

This can be local to the if...

...

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 of memory bandwidth controller is same with + * last level cache */ + if (resctrl->membw_info) {

virResctrlInfoMemBWPtr membw_info = resctrl->membw_info;

Yes. Should put to here. :). I remember some compile flags will complain if local variable not at the begin of a function.

...

+ membw_info = resctrl->membw_info; + if (level > membw_info->last_level_cache) { + membw_info->last_level_cache = level; + membw_info->max_id = 0; + } else if (membw_info->last_level_cache == level) { + membw_info->max_id++; + } + } +

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.

Thanks for that! Let me help to recall the discuss. :) RDT kernel module will report some parameters for MBA. They are : "min_bandwidth": The minimum memory bandwidth percentage which user can request. "bandwidth_gran": The granularity in which the memory bandwidth percentage is allocated. The allocated b/w percentage is rounded off to the next control step available on the hardware. The available bandwidth control steps are: min_bandwidth + N * bandwidth_gran. "num_closids": The number mba group can exist simultaneous. Those information is query from kernel interface /sys/fs/resctrl/info/MB Above hunk is used to calculate the number of memory bandwidth allocation controllers in system. The number of memory bandwidth allocation controllers is same as last level cache. This number is calculate by traversing cache hierarchy of host(/sys/bus/cpu/devices/cpuX/cache/). So above hunk is used to collect that information, to sanitize domain XML about memory bandwidth allocation part. And the number of memory bandwidth allocation controllers will not change, it just cannot query directly from RDT kernel module.

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

BTW: I'm stopping here for the evening, I'll work through the rest hopefully tomorrow depending on interruptions.

Good evening :). The rest are about 1. allocate memory bandwidth 2. domain XML and 3. host capability XML.

...

if (level >= resctrl->nlevels) return 0;

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On 07/18/2018 03:57 AM, bing.niu@intel.com wrote:

From: Bing Niu <bing.niu@intel.com>

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@intel.com> --- 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.

s/feature/the future/

*/

@@ -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);

NIT: Could be inside the if condition

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);

@cb is an int function, but only ever used in what amounts to a void function in patch8 (virDomainMemorytuneDefFormatHelper - although defined as int, it only ever returns 0, so it could be void too). So either this changes to handle that or we change the *Callback to be void. Do you have a preference?

+ } + + return 0; +} + + +int virResctrlAllocForeachCache(virResctrlAllocPtr alloc, virResctrlAllocForeachCacheCallback cb, void *opaque) @@ -952,6 +1010,240 @@ virResctrlAllocGetID(virResctrlAllocPtr alloc) }

The next hunk of changes has perhaps more to do with the functionality of the code vs. the pure alloc/free of the structures. I think that alloc/free needs to be split from Parse/Format - it'll make things easier. Then there's Parse of XML vs Parse of the schemata for the MB: lines which just jumbles things (in my mind ;-)) This particular change may even be a 3rd patch... Keep reading ;-)

+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]); + } +} + +

Might be nice to put some comments here. Since this is a backend of sorts for the XML Parse API from patch8, let's wait to introduce it there. That is it moves to patch8.

+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; +} + +

Seeing Format and Parse API's was confusing until I remembered that these are for the schemata files. I think perhaps this section could have been commented a bit more to indicate what it's for.

+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;

return virBufferCheckError(buf);

+} + +

This API is the backend of the virResctrlAllocAssign called during the qemuProcessResctrlCreate processing - so it may be a third patch that can be extracted from this one. I need to look a bit harder though. Similarly the *Subtract API may fall into this category.

+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;

So can we even have one of these if resctrl->membw_info == NULL? The ->mem_bw is allocated in virResctrlSetMemoryBandwidth, which is only defined, but not called. It will be called later in patch8 from a Parse API I see.

+ + if (mem_bw_alloc && !mem_bw_info) {

In virResctrlGetMemoryBandwidthInfo it's "OK" if bandwidth_gran is not present - a VIR_INFO is generated, the resctrl->membw_info is left as NULL, and we return 0. So failing here would seemingly be quite unexpected, wouldn't it?

+ 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, "

s/%zd/id %zd/ s/not/does not/

+ "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; + } + }

So this is just a Check or Validate type function? Should it be renamed or should it actually do something.

+ return 0; +} + +

More stuff for the schemata parse/format

+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; +} + +

Ditto for schemata parse/format...

+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) {

These checks seem out of place. If we get this far without !resctrl? Wow... And erroring out if !resctrl->membw_info even though virResctrlGetMemoryBandwidthInfo can succeed without it. As for the min_bandwidth and bandwidth_granularity checks - those would seem to be more appropriate in patch4 wouldn't they? when the values are read... If they're read as zero, then I assume that's bad ;-).

+ 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; }

For schemata parse/format

+ 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++) {

For schemata parse/format

+ 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; }

Need to think about this, but perhaps would be appropriate for schemata format/parse, but could be something for that 3rd patch out of this one.

+ 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;

Still need to come to grips with the 'best place' for this - perhaps a 3rd patch.

+ if (virResctrlAllocMemoryBandwidth(resctrl, alloc, alloc_free) < 0) + goto cleanup; + if (virResctrlAllocCopyMasks(alloc, alloc_default) < 0) goto cleanup;

Let's see whether I can help you split these up a bit. There's a few things to answer from what I looked at. So far nothing is necessarily wrong, it's just the splitting of things that makes it easier to think about. The split you did has certainly helped thus far though. John I'll keep plugging away tomorrow.

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,

On 07/26/2018 02:00 AM, bing.niu wrote:

On 2018年07月26日 06:37, John Ferlan wrote:

...

On 07/18/2018 03:57 AM, bing.niu@intel.com wrote:

...

From: Bing Niu <bing.niu@intel.com>

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@intel.com> ---   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.

s/feature/the future/

Will change.

...

...

   */  

[....]

...

...

      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);

NIT: Could be inside the if condition.

OK~

...

...

      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);

@cb is an int function, but only ever used in what amounts to a void function in patch8 (virDomainMemorytuneDefFormatHelper - although defined as int, it only ever returns 0, so it could be void too).

So either this changes to handle that or we change the *Callback to be void.  Do you have a preference?

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. :)

Yeah, typically those Foreach type function want to return something such that we stop processing in the event of some failure, no need to keep going and pile on the errors or have some unexpected success!

...

...

+    } + +    return 0; +} + + +int   virResctrlAllocForeachCache(virResctrlAllocPtr alloc,                               virResctrlAllocForeachCacheCallback cb,                               void *opaque) @@ -952,6 +1010,240 @@ virResctrlAllocGetID(virResctrlAllocPtr alloc)   }    

The next hunk of changes has perhaps more to do with the functionality of the code vs. the pure alloc/free of the structures. I think that alloc/free needs to be split from Parse/Format - it'll make things easier. Then there's Parse of XML vs Parse of the schemata for the MB: lines which just jumbles things (in my mind ;-))

This particular change may even be a 3rd patch...  Keep reading ;-)

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.

So I've done some of this already before reading your response - I'll send what I have directly to you though. "So far" what I came up with from just this patch: util: Add MBA allocation to virresctrl -> Just the allocation and dispose changes util: Add MBA schemata parse and format methods -> Split out virResctrlAllocMemoryBandwidthFormat and virResctrlAllocParseMemoryBandwidthLine -> Changed the order of virResctrlAllocParse to L* first and MB second. It shouldn't matter, but it is the order it's formatted util: Add support to calculate MBA utilization -> Split out virResctrlMemoryBandwidthSubtract and virResctrlAllocMemoryBandwidth -> NB, for these two I moved them "closer" to their usage rather than mixed up in the Parse/Format methods above. -> I have a question for virResctrlMemoryBandwidthSubtract though. I noted that virResctrlAllocSubtract gets called twice - once before the while loop and once during the while loop. So should the same occur for the BandwidthSubtract? util: Introduce virResctrlAllocForeachMemory -> Split out just the one API -> Modify the code slightly to return 0 earlier if !alloc->mem_bw -> Modify the for loop to return -1 if @cb fails -> Add the API to src/libvirt_private.syms util: Introduce virResctrlAllocSetMemoryBandwidth -> All that was left over -> Rename from virResctrlSetMemoryBandwidth -> Move code to be "similar" to the virResctrlAllocSetCacheSize such that it's before the AllocForeach API -> Add the API to src/libvirt_private.syms

...

...

+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]); +    } +} + +

Might be nice to put some comments here. Since this is a backend of sorts for the XML Parse API from patch8, let's wait to introduce it there.  That is it moves to patch8.

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?

Upon further reflection, introducing before is fine - you'll see in what I send to you.

...

...

+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; +} + +

Seeing Format and Parse API's was confusing until I remembered that these are for the schemata files. I think perhaps this section could have been commented a bit more to indicate what it's for.

Will add comments and put this into a dedicated patch with other schemata file parse and formating functions.

...

...

+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;

return virBufferCheckError(buf);

OK.

...

...

+} + +

This API is the backend of the virResctrlAllocAssign called during the qemuProcessResctrlCreate processing - so it may be a third patch that can be extracted from this one. I need to look a bit harder though. Similarly the *Subtract API may fall into this category.

Yes~

...

...

+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;

So can we even have one of these if resctrl->membw_info == NULL? The ->mem_bw is allocated in virResctrlSetMemoryBandwidth, which is only defined, but not called. It will be called later in patch8 from a Parse API I see.

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.

...

...

+ +    if (mem_bw_alloc && !mem_bw_info) {

In virResctrlGetMemoryBandwidthInfo it's "OK" if bandwidth_gran is not present - a VIR_INFO is generated, the resctrl->membw_info is left as NULL, and we return 0.

So failing here would seemingly be quite unexpected, wouldn't it?

Yes! you are right. Maybe someone try to allocate memory bandwidth on a host without MBA support.

I didn't change any of the error scenarios in what I'm sending to you... Just be sure to add a few more comments about what's being checked. Since everything was packed into one patch as I'm reviewing I'm thinking, is this the internal or external caller.

...

...

+        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, "

s/%zd/id %zd/

s/not/does not/

Will update.

...

...

+                             "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; +        } +    }

So this is just a Check or Validate type function?  Should it be renamed or should it actually do something.

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?

As another Red Hat libvirt engineer says, "Naming is hard". Problem is there's a lot of opinions about the perfect name. If this is a Check or Validation function, then let's use that in the name. As it is now as virResctrlAllocMemoryBandwidth is just too generic, using the virResctrlAlloc 'prefix', perhaps an adjustment to virResctrlAllocCheckMemoryBandwidth is sufficient with of course a few extra comments about it's purpose.

...

...

+    return 0; +} + +

More stuff for the schemata parse/format

...

+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; +} + +

Ditto for schemata parse/format...

...

+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) {

These checks seem out of place. If we get this far without !resctrl? Wow... And erroring out if !resctrl->membw_info even though virResctrlGetMemoryBandwidthInfo can succeed without it.

As for the min_bandwidth and bandwidth_granularity checks - those would seem to be more appropriate in patch4 wouldn't they? when the values are read...  If they're read as zero, then I assume that's bad ;-).

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? :)

Yeah, I think parameter value validation is fine at read time. I know there's other code that does that. I didn't check if the CAT code did that though. I think in the long term you need to decide - if I found an MB: line in the schemata file, then can it even be possible that membw_info is NULL. I suppose it is an error - again with all the code being added together in one patch, it was "confusing" to consider the Parse/Format of the schemata file. I suppose in the back of my mind I had he parse/format of the XML file...

...

...

+        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;       }  

For schemata parse/format

...

+    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++) {

For schemata parse/format

...

+        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;           }  

Need to think about this, but perhaps would be appropriate for schemata format/parse, but could be something for that 3rd patch out of this one.

...

+        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;  

Still need to come to grips with the 'best place' for this - perhaps a 3rd patch.

...

+    if (virResctrlAllocMemoryBandwidth(resctrl, alloc, alloc_free) < 0) +        goto cleanup; +       if (virResctrlAllocCopyMasks(alloc, alloc_default) < 0)           goto cleanup;  

Let's see whether I can help you split these up a bit. There's a few things to answer from what I looked at. So far nothing is necessarily wrong, it's just the splitting of things that makes it easier to think about. The split you did has certainly helped thus far though.

John

I'll keep plugging away tomorrow.

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

I'll send you what I have "so far" shortly. There are some formatting adjustments included, but for the most part it's taking this one patch and splitting it before the subsequent rename patch. The only other adjustment was to use the "new" name for the SetMemoryBandwidth John

...

...

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,

subj: Add support for memorytune XML processing for resctrl MBA On 07/18/2018 03:57 AM, bing.niu@intel.com wrote:

From: Bing Niu <bing.niu@intel.com>

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@intel.com> --- 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

Strangely I expected to actually have a merge conflict here with previous changes, but I didn't!

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 @@ <cache id='0' level='3' type='both' size='3' unit='MiB'/> <cache id='1' level='3' type='both' size='3' unit='MiB'/> </cachetune> + <memorytune vcpus='0-3'> + <node id='0' bandwidth='60'/> + </memorytune> + </cputune> ... </domain> @@ -950,7 +954,38 @@ </dl> </dd> </dl> + </dd>

+ <dt><code>memorytune</code></dt> + <dd> + Optional <code>memorytune</code> element can control allocations for

s/Optional/<span class="since">Since 4.7.0</span>, the optional/ e.g. Since 4.7.0, the optional...

+ 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

s/./. The/

+ by <code>memorytune</code> can be identical to those specified by + <code>cachetune</code>. However they are not allowed to overlap each other.

The last 2 sentences are "confusing". We didn't add similar wording to cachetune... It's one of those visual things I suppose, but it seems like there's a relationship between cachetune and memtune, but only if they intersect vCPU values. Thus if cachetune "joins" vcpus='0-3', then memtune must also join them - using other combinations of 0-3 isn't allowed. I have to only imagine that would get more complicated with RDT

+ 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.

Should we indicate that it must be between 1 and 100 inclusive?

+ </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 nit's trying to clear up in my mind whether the vcpus used byame="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; + }

Should we check that bandwidth is between 1 and 100 inclusive? eg < 1 || > 100, then error. Or leave that to SetMemoryBandwidth (IDC).

+ 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; + }

Just realized that for here and for cachetune we never parse the subelements if BitmapIsAllClear. That would mean anything provided is lost - of course it seems silly to have an empty bitmap too. Do we even test for that? Is it even feasible?

+ + 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);

Ahh... so this is where I had some confusion initially, but makes more sense now given one of the failure xml examples. Still, does this cause issues: <memtune vcpus='3-5'> <node id='0' bandwidth='60'/> </memtune <memtune vcpus='3-5'> <node id='0' bandwidth='60'/> </memtune> or would the second one be <node id='1' bandwidth='60'/> Yes, they should put <node id='1' with the first group, but QE likes to stretch the boundaries of imagination once they read the code.

+ } + + 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 */

The following code doesn't do what the "otherwise" comment suggests - although I perhaps still confused over the @alloc usage above.

+ 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);

probably could wrap this in a ignore_value(); since FormatHelper only ever returns 0. The error checking is next:

+ +

You can removed 1 of the 2 empty lines above here.

+ 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'/>

This is illegal because node id='#' uses the same number, right?

+ </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>

This is illegal because memory tune needs to be '0-1', right? A few fixups seem necessary. Mostly minor stuff. John

+ </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");

On 2018年07月27日 05:04, John Ferlan wrote:

On 07/18/2018 03:57 AM, bing.niu@intel.com wrote:

...

From: Bing Niu <bing.niu@intel.com>

Add new XML section to report host's memory bandwidth allocation capability. The format as below example:

<host> ..... <memory_bandwidth> <node id='0' cpus='0-19'> <control granularity='10' min ='10' maxAllocs='8'/> </node> </memory_bandwidth> </host>

granularity ---- granularity of memory bandwidth, unit percentage. min ---- minimum memory bandwidth allowed, unit percentage. maxAllocs ---- maximum memory bandwidth allocation group supported.

Signed-off-by: Bing Niu <bing.niu@intel.com> --- docs/schemas/capability.rng | 33 +++++++ src/conf/capabilities.c | 108 +++++++++++++++++++++ src/conf/capabilities.h | 11 +++ src/util/virresctrl.c | 20 ++++ src/util/virresctrl.h | 15 +++ .../linux-resctrl/resctrl/info/MB/bandwidth_gran | 1 + .../linux-resctrl/resctrl/info/MB/min_bandwidth | 1 + .../linux-resctrl/resctrl/info/MB/num_closids | 1 + tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml | 8 ++ tests/virresctrldata/resctrl.schemata | 1 + 10 files changed, 199 insertions(+) create mode 100644 tests/vircaps2xmldata/linux-resctrl/resctrl/info/MB/bandwidth_gran create mode 100644 tests/vircaps2xmldata/linux-resctrl/resctrl/info/MB/min_bandwidth create mode 100644 tests/vircaps2xmldata/linux-resctrl/resctrl/info/MB/num_closids

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.

Yes. RDT CMT will follow this memory_bandwidth host capability XML. Huaqiang will handle that.

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.

...

diff --git a/docs/schemas/capability.rng b/docs/schemas/capability.rng index 52164d5..d61515c 100644 --- a/docs/schemas/capability.rng +++ b/docs/schemas/capability.rng @@ -51,6 +51,9 @@ <optional> <ref name='cache'/> </optional> + <optional> + <ref name='memory_bandwidth'/> + </optional> <zeroOrMore> <ref name='secmodel'/> </zeroOrMore> @@ -326,6 +329,36 @@ </attribute> </define>

+ <define name='memory_bandwidth'> + <element name='memory_bandwidth'> + <oneOrMore> + <element name='node'> + <attribute name='id'> + <ref name='unsignedInt'/> + </attribute> + <attribute name='cpus'> + <ref name='cpuset'/> + </attribute> + <zeroOrMore> + <element name='control'> + <attribute name='granularity'> + <ref name='unsignedInt'/> + </attribute> + <optional> + <attribute name='min'> + <ref name='unsignedInt'/> + </attribute> + </optional> + <attribute name='maxAllocs'> + <ref name='unsignedInt'/> + </attribute> + </element> + </zeroOrMore> + </element> + </oneOrMore> + </element> + </define> + <define name='guestcaps'> <element name='guest'> <ref name='ostype'/> diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 7a810ef..3f52296 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -198,6 +198,16 @@ virCapabilitiesFreeNUMAInfo(virCapsPtr caps) }

static void +virCapsHostMemBWNodeFree(virCapsHostMemBWNodePtr ptr) +{ + if (!ptr) + return; + + virBitmapFree(ptr->cpus); + VIR_FREE(ptr); +} + +static void virCapabilitiesClearSecModel(virCapsHostSecModelPtr secmodel) { size_t i; @@ -239,6 +249,11 @@ virCapsDispose(void *object) virCapsHostCacheBankFree(caps->host.caches[i]); VIR_FREE(caps->host.caches);

+ for (i = 0; i < caps->host.nnodes; i++) + virCapsHostMemBWNodeFree(caps->host.nodes[i]); + VIR_FREE(caps->host.nodes); + +

Remove one of the blank lines.

OK. Thanks a lot~

This was the only issue I saw in my quick glance - rest seemed OK.

John

[.....]