[libvirt] [RFC PATCH 1/2] util: Add memory bandwidth support to resctrl
Ján Tomko
jtomko at redhat.com
Sat Jun 2 14:50:11 UTC 2018
On Tue, May 29, 2018 at 06:58:02PM +0800, bing.niu at intel.com wrote:
>From: Bing Niu <bing.niu at intel.com>
>
>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 at intel.com>
>---
> src/util/virresctrl.c | 415 +++++++++++++++++++++++++++++++++++++++++++++++---
> src/util/virresctrl.h | 5 +
> 2 files changed, 401 insertions(+), 19 deletions(-)
>
>diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
>index fc11635..ad050c7 100644
>--- a/src/util/virresctrl.c
>+++ b/src/util/virresctrl.c
>@@ -86,6 +86,20 @@ struct _virResctrlInfoPerType {
> virResctrlInfoPerCache control;
> };
>
>+/* Information about memory bandwidth allocation */
>+typedef struct _virResctrlInfoMB virResctrlInfoMB;
>+typedef virResctrlInfoMB *virResctrlInfoMBPtr;
>+struct _virResctrlInfoMB {
>+ /* minimum memory bandwidth allowed*/
>+ unsigned int min_bandwidth;
>+ /* bandwidth granularity */
>+ unsigned int bandwidth_gran;
bandwidth_granularity is more descriptive
>+ /* level number of llc*/
>+ unsigned int llc;
cryptic three-letter arconym, you can spell it out
>+ /* number of last level cache */
Based on the comment, I don't understand the difference from the
previous value.
>+ unsigned int max_id;
>+};
>+
> typedef struct _virResctrlInfoPerLevel virResctrlInfoPerLevel;
> typedef virResctrlInfoPerLevel *virResctrlInfoPerLevelPtr;
> struct _virResctrlInfoPerLevel {
>@@ -97,6 +111,8 @@ struct _virResctrlInfo {
>
> virResctrlInfoPerLevelPtr *levels;
> size_t nlevels;
>+
>+ virResctrlInfoMBPtr mb_info;
> };
>
> static virClassPtr virResctrlInfoClass;
>@@ -126,6 +142,7 @@ virResctrlInfoDispose(void *obj)
> VIR_FREE(level);
> }
>
>+ VIR_FREE(resctrl->mb_info);
> VIR_FREE(resctrl->levels);
> }
>
>@@ -201,6 +218,18 @@ struct _virResctrlAllocPerType {
> size_t nmasks;
> };
>
>+/*
>+ * 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
>+ */
>+typedef struct _virResctrlAllocMB virResctrlAllocMB;
>+typedef virResctrlAllocMB *virResctrlAllocMBPtr;
>+struct _virResctrlAllocMB {
>+ unsigned int **bandwidth;
>+ size_t nsizes;
>+};
>+
> typedef struct _virResctrlAllocPerLevel virResctrlAllocPerLevel;
> typedef virResctrlAllocPerLevel *virResctrlAllocPerLevelPtr;
> struct _virResctrlAllocPerLevel {
>@@ -214,7 +243,7 @@ struct _virResctrlAlloc {
>
> 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
>@@ -259,6 +288,13 @@ virResctrlAllocDispose(void *obj)
> VIR_FREE(level);
> }
>
>+ if (resctrl->mba) {
>+ virResctrlAllocMBPtr mba = resctrl->mba;
>+ for (i = 0; i < mba->nsizes; i++)
>+ VIR_FREE(mba->bandwidth[i]);
>+ }
>+
>+ VIR_FREE(resctrl->mba);
> VIR_FREE(resctrl->id);
> VIR_FREE(resctrl->path);
> VIR_FREE(resctrl->levels);
>@@ -364,6 +400,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];
>
>@@ -379,11 +418,62 @@ virResctrlInfoIsEmpty(virResctrlInfoPtr resctrl)
> return true;
> }
>
>+static int
>+virResctrlGetMemoryBandwidthInfo(virResctrlInfoPtr resctrl)
>+{
>+ DIR *dirp = NULL;
>+ int ret = -1;
>+ int rv = -1;
>+ virResctrlInfoMBPtr i_mb = NULL;
>
>-#ifdef __linux__
This ifdef removal looks suspicious
>+ rv = virDirOpenIfExists(&dirp, SYSFS_RESCTRL_PATH "/info");
>+ if (rv <= 0) {
>+ ret = rv;
>+ goto cleanup;
>+ }
>+ /* query memory bandwidth allocation info */
>+ if (VIR_ALLOC(i_mb) < 0) {
>+ ret = -1;
ret is initialized to -1, no need to set it again
>+ goto cleanup;
>+ }
>+ rv = virFileReadValueUint(&i_mb->bandwidth_gran,
>+ 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");
Is there a chance the directory SYSFS_RESCTRL_PATH will exist, but not
this file? We should not spam the logs with warnings on older kernels.
>+ ret = 0;
>+ goto cleanup;
>+ } else if (rv < 0) {
>+ /* File no existing or other failures are fatal, so just quit */
This comment conflicts with the above statement.
>+ goto cleanup;
>+ }
>
>-int
>-virResctrlGetInfo(virResctrlInfoPtr resctrl)
>+ rv = virFileReadValueUint(&i_mb->min_bandwidth,
>+ SYSFS_RESCTRL_PATH "/info/MB/min_bandwidth");
>+ 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/min_bandwidth'"
>+ "does not exist");
>+ ret = 0;
>+ goto cleanup;
>+ } else if (rv < 0) {
>+ /* File no existing or other failures are fatal, so just quit */
>+ goto cleanup;
>+ }
>+
>+ VIR_STEAL_PTR(resctrl->mb_info, i_mb);
>+ ret = 0;
>+ cleanup:
>+ VIR_DIR_CLOSE(dirp);
>+ VIR_FREE(i_mb);
>+ return ret;
>+}
>+
>+static int
>+virResctrlGetCacheInfo(virResctrlInfoPtr resctrl)
> {
> DIR *dirp = NULL;
> char *endptr = NULL;
>@@ -512,6 +602,23 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl)
> return ret;
> }
>
>+#ifdef __linux__
>+int
>+virResctrlGetInfo(virResctrlInfoPtr resctrl)
>+{
>+ int ret;
Our convention is to initialize ret to -1 and then overwrite it exactly
once before returning from the function, othrewise it gets hard to
follow.
>+
>+ ret = virResctrlGetMemoryBandwidthInfo(resctrl);
>+ if (ret < 0) {
>+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>+ _("Get Memory Bandwidth fail"));
>+ goto out;
>+ }
>+ ret = virResctrlGetCacheInfo(resctrl);
>+ out:
>+ return ret;
>+}
>+
> #else /* ! __linux__ */
>
> int
>@@ -534,12 +641,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->llc) {
>+ mb_info->llc = level;
>+ mb_info->max_id = 1;
>+ } else if (mb_info->llc == level) {
>+ mb_info->max_id++;
>+ }
>+ }
>+
> if (level >= resctrl->nlevels)
> return 0;
>
>@@ -600,6 +721,9 @@ virResctrlAllocIsEmpty(virResctrlAllocPtr resctrl)
> if (!resctrl)
> return true;
>
>+ if (resctrl->mba)
>+ return false;
>+
> for (i = 0; i < resctrl->nlevels; i++) {
> virResctrlAllocPerLevelPtr a_level = resctrl->levels[i];
>
>@@ -855,16 +979,218 @@ virResctrlAllocGetID(virResctrlAllocPtr alloc)
> }
>
>
>-char *
>-virResctrlAllocFormat(virResctrlAllocPtr resctrl)
The rename should be in a separate patch, to make the diff easier to
read.
>+int
>+virResctrlSetMemoryBandwidth(virResctrlAllocPtr resctrl,
>+ unsigned int id,
>+ unsigned int memory_bandwidth)
>+{
>+ virResctrlAllocMBPtr mba = resctrl->mba;
>+
>+ if (!mba) {
>+ if (VIR_ALLOC(mba) < 0)
>+ return -1;
>+ resctrl->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)
Usually we put the output variables first.
>+{
>+ int id;
>+
>+ if (!alloc->mba)
>+ goto out;
>+
>+ 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);
>+ out:
pointless label
>+ return 0;
>+}
>+
>+static int
>+virResctrlAllocMemoryBandwidth(virResctrlInfoPtr resctrl,
>+ virResctrlAllocPtr alloc, virResctrlAllocPtr free)
>+{
>+ int id;
>+ int ret;
>+ virResctrlAllocMBPtr mb_alloc = alloc->mba;
>+ virResctrlAllocMBPtr mb_free = free->mba;
>+ virResctrlInfoMBPtr mb_info = resctrl->mb_info;
>+
>+ if (!mb_alloc)
>+ return 0;
>+
>+ if (mb_alloc && !mb_info) {
>+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>+ _("RDT Memory Bandwidth allocationi"
allocation
>+ " unsupported"));
>+ ret = -1;
ret should be initialized to -1
>+ goto out;
>+ }
>+
>+ for (id = 0; id < mb_alloc->nsizes; id ++) {
>+ if (!mb_alloc->bandwidth[id])
>+ continue;
>+
>+ if (*(mb_alloc->bandwidth[id]) % mb_info->bandwidth_gran) {
>+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>+ _("Memory Bandwidth allocation of size "
>+ "%u is not divisible by granularity %u"),
>+ *(mb_alloc->bandwidth[id]),
>+ mb_info->bandwidth_gran);
>+ ret = -1;
>+ goto out;
>+ }
>+ 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);
>+ ret = -1;
>+ goto out;
>+ }
>+ 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 - 1);
>+ return -1;
>+ }
>+ 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 -1;
>+ }
>+ }
>+ ret = 0;
>+ out:
Pointless label.
>+ 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;
>+
>+ /* 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)
|| should be at the end of the line
>+ || !(resctrl->mb_info->bandwidth_gran)) {
>+ 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;
The outer if will need {} around the body.
Is there a chance of alloc->mba being non-NULL here?
>+
>+ 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);
>+ ret = -1;
ret should be initialized to -1 instead of setting it to -1 after every
error
>+ goto clean;
cleanup is the standard label name
>+ }
>+ if (virStrToLong_uip(tmp, NULL, 10, &bandwidth) < 0) {
>+ virReportError(VIR_ERR_INTERNAL_ERROR,
>+ _("Invalid bandwidth %u"), bandwidth);
>+ ret = -1;
>+ goto clean;
>+ }
>+ if (alloc->mba->nsizes <= id &&
>+ VIR_EXPAND_N(alloc->mba->bandwidth, alloc->mba->nsizes,
>+ id - alloc->mba->nsizes + 1) < 0) {
>+ ret = -1;
>+ goto clean;
>+ }
>+ if (!alloc->mba->bandwidth[id]) {
>+ if (VIR_ALLOC(alloc->mba->bandwidth[id]) < 0) {
>+ ret = -1;
>+ goto clean;
>+ }
>+ }
>+
>+ *(alloc->mba->bandwidth[id]) = bandwidth;
Why does the array contain pointer to the value, instead of the value
itself?
Also, VIR_APPEND_ELEMENT is nicer than VIR_EXPAND + manual assignment.
>+ }
>+ ret = 0;
>+ clean:
>+ virStringListFree(mbs);
>+ return ret;
>+}
>+
>+
>+static int
>+virResctrlAllocCacheFormat(virResctrlAllocPtr resctrl, virBufferPtr buf)
> {
>- virBuffer buf = VIR_BUFFER_INITIALIZER;
> unsigned int level = 0;
> unsigned int type = 0;
> unsigned int cache = 0;
>+ int ret = 0;
-1 is the default
>
> if (!resctrl)
>- return NULL;
>+ return 0;
The return type change should be in a separate patch.
>
> for (level = 0; level < resctrl->nlevels; level++) {
> virResctrlAllocPerLevelPtr a_level = resctrl->levels[level];
>@@ -878,7 +1204,7 @@ virResctrlAllocFormat(virResctrlAllocPtr resctrl)
> if (!a_type)
> continue;
>
>- virBufferAsprintf(&buf, "L%u%s:", level, virResctrlTypeToString(type));
>+ virBufferAsprintf(buf, "L%u%s:", level, virResctrlTypeToString(type));
And so should the switch from local virBuffer to virBufferPtr
>
> for (cache = 0; cache < a_type->nmasks; cache++) {
> virBitmapPtr mask = a_type->masks[cache];
>@@ -889,18 +1215,49 @@ virResctrlAllocFormat(virResctrlAllocPtr resctrl)
>
> mask_str = virBitmapToString(mask, false, true);
> if (!mask_str) {
>- virBufferFreeAndReset(&buf);
>- return NULL;
>+ ret = -1;
>+ goto out;
> }
>
>- 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');
> }
> }
>+ out:
pointless label
>+ return ret;
>+}
>+
>+
>+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]);
>+ }
>+ }
>+}
>+
>+
>@@ -1389,7 +1765,6 @@ virResctrlAllocFindUnused(virResctrlAllocPtr alloc,
> return ret;
> }
>
>-
Unrelated whitespace change.
> static int
> virResctrlAllocCopyMasks(virResctrlAllocPtr dst,
> virResctrlAllocPtr src)
>@@ -1451,6 +1826,9 @@ virResctrlAllocMasksAssign(virResctrlInfoPtr resctrl,
> if (!alloc_default)
> goto cleanup;
>
>+ if (virResctrlAllocMemoryBandwidth(resctrl, alloc, alloc_free) < 0)
>+ goto cleanup;
>+
> if (virResctrlAllocCopyMasks(alloc, alloc_default) < 0)
> goto cleanup;
>
>@@ -1524,7 +1902,6 @@ virResctrlAllocDeterminePath(virResctrlAllocPtr alloc,
> return 0;
> }
>
>-
Same here
> /* This checks if the directory for the alloc exists. If not it tries to create
> * it and apply appropriate alloc settings. */
> int
Jano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180602/ccbf12e3/attachment-0001.sig>
More information about the libvir-list
mailing list