3:40 a.m.
On 2018年06月30日 06:39, John Ferlan wrote:
On 06/15/2018 05:29 AM, bing.niu(a)intel.com wrote:
> From: Bing Niu <bing.niu(a)intel.com>
>
> Add memory bandwidth allocation support basing on existing
s/basing/based
Will fix.
> virresctrl implementation. Two new structures virResctrlInfoMB
^^
"cache" (IOW: fit it in between)
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"
> 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.
llc == ? [last level cache as I found later...]
Probably could have "split" this patch too in order to add each memory
bandwidth and memory bandwidth info separately. Lots of stuff in this
patch to keep track of.
Good idea. Will split virResctrlInfoMB and
virResctrlAllocMB in next round.
>
> 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>
> ---
> src/libvirt_private.syms | 2 +
> src/util/virresctrl.c | 383 ++++++++++++++++++++++++++++++++++++++++++++++-
> src/util/virresctrl.h | 14 ++
> 3 files changed, 397 insertions(+), 2 deletions(-)
>
Rather than using "MB", could we use "MemBW" - I see MB and think
megabytes. Maybe it's just me... Also rather than spelling out
MemoryBandwidth sometimes, use the same shorthand to be consistent.
Makes it easier to search for MemBW related functionality. Although
someone else reviewing may have a different opinion - world's full of
'em ;-).
agree and will change to MemBW.
Beyond that, after reading subsequent patches it doesn't seem memory
tunes are a related to cachetunes other than that both use resctrl API's
in order to manipulate data.
If domain XML didn't have "cputune/cachetune", then does part of
virResctrlAlloc not get used? I assume that'd be the @levels. Can both
cachetunes and memtunes be supplied? I assume so, but figured I'd ask.
Since we
already discussed in patch3. I will not reply here. :)
> 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;
syntax-check would have told you this is out of order alphabetically
Unfortunately
syntax-check didn't throw a error to me. And will fix to
keep alphabetically.
>
> 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*/
s/d*/d */
Will fix.
> + 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*/
s/e*/e */
Will fix.
> + 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
> + * */
Non standard comment format ("* */"
Will fix by remove that space.
> + unsigned int max_id;
> +};
> +
> struct _virResctrlInfo {
> virObject parent;
>
> virResctrlInfoPerLevelPtr *levels;
> size_t nlevels;
> +
> + virResctrlInfoMBPtr mb_info;
How about "MemBWInfo" or membw_info
OK. will change in next version.
> };
>
>
> @@ -146,6 +167,7 @@ virResctrlInfoDispose(void *obj)
> VIR_FREE(level);
> }
>
> + VIR_FREE(resctrl->mb_info);
> VIR_FREE(resctrl->levels);
> }
>
There's a really detailed description of virResctrlAlloc right about
here that could use some of the details about the memory bandwidth
included into it, especially since you're changing _virResctrlAlloc
Yes. I just notice that, I will add memory bandwidth part in the
description of virResctrlAlloc.
> @@ -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;
This should be bandwidths
> + size_t nsizes;
This should be 'nbandwidths' as in the number of **bandwidths.
Will fix
above two.
> +};
> +
> struct _virResctrlAlloc {
> virObject parent;
>
> virResctrlAllocPerLevelPtr *levels;
> size_t nlevels;
>
> + virResctrlAllocMBPtr mba;
s/mba/MemBW or MemBWAlloc
Also add a blank line for reading separation...
I prefer to MemBW since there is no
*Alloc* in levels. :)
Will fix those two.
> /* 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;
size_t i; since this is a for loop
OK！
> +
> + 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)
Subtract (and don't forget to fix the alignment for arg2 when you do
change the naming)
> +{
> + size_t i;
Nit:
if (!used->MemBW)
return;
for (...)
Okay, Will change above two.
> +
> + 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]) {
Compare != 0 since it's an unsigned int contained and not a bool or
pointer (makes it clearer).
OK.
> + virReportError(VIR_ERR_XML_ERROR,
> + _("Collision Memory Bandwidth on node %d"),
Memory Bandwidth already defined for node %u
(it's unsigned...)
Will fix in next version.
> + 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)
One argument per line
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.
> +{
> + int id;
size_t i; it's a for loop
> +
> + if (!alloc->mba)
> + return 0;
> +
> + virBufferAddLit(buf, "MB:");
MemBW
> +
> + for (id = 0; id < alloc->mba->nsizes; id++) {
> + if (alloc->mba->bandwidth[id]) {
> + virBufferAsprintf(buf, "%u=%u;", id,
"%zd=%u;"
zd is the format used for size_t
> + *(alloc->mba->bandwidth[id]));
Will fix above three.
> + }
> + }
> +
> + virBufferTrim(buf, ";", 1);
> + virBufferAddChar(buf, '\n');
> + virBufferCheckError(buf);
> + return 0;
> +}
> +
> +
> +static int
> +virResctrlAllocMemoryBandwidth(virResctrlInfoPtr resctrl,
> + virResctrlAllocPtr alloc, virResctrlAllocPtr free)
One argument per line
OK!
> +{
> + int id;
size_t i; It's a for loop
> + int ret = -1;
Just return 0 or -1 directly, don't bother this this since you're not
going to a cleanup label it really doesn't matter.
> + virResctrlAllocMBPtr mb_alloc = alloc->mba;
> + virResctrlAllocMBPtr mb_free = free->mba;
> + virResctrlInfoMBPtr mb_info = resctrl->mb_info;
> +
> + if (!mb_alloc) {
> + ret = 0;
> + return ret;
> + }
Just return 0 - don't bother with the ret = 0;
Will fix above two.
> +
> + 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 ++) {
s/ ++/++/
(no space on the autoincrement.
My bad. and will fix
> + 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) {
Is @id the right comparison here, should it be the current
*(mb_alloc->bandwidth[id])" value? @id was the VIR_EXPAND_N of the
maximum size. In any case, a very strange comparison.
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.
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("bandwidth controller %u not exist,"
> + " max controller id %u"),
s/%u/%zd/
s/exist,/exist, /
s/ max/max
(IOW: Space on the first line)
OK!
> + 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 "
s/%u/%zd/
> + "bandwidth for node %u%%, freed %u%%"),
The error message is a bit strange - especially that "freed" part. I'm
still not 100% clear about all that's going on, but I'm sure to come
back to this in some future review.
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.
> + 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;
s/nmb/nmbs/ (or numbws if you change @mbs to be mbws
OK
> + 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))
To follow the above ..."MemBW", 5...
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.
> + return 0;
> +
> + if (!resctrl || !resctrl->mb_info
> + || !resctrl->mb_info->min_bandwidth
> + || !resctrl->mb_info->bandwidth_granularity) {
Put the || on the previous line
OK!
> + 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;
> + }
Hmmm, this is familiar...
> +
> + tmp = strchr(line, ':');
> + if (!tmp)
> + return 0;
> + tmp++;
> +
> + mbs = virStringSplitCount(tmp, ";", 0, &nmb);
> + if (!nmb)
> + return 0;
Since @nmb is a size_t, make the explicit == 0 comparison.
Alternatively, use "if (!mbs)"... What happens is Coverity gets grumpy
thinking a 0 can be returned with something in @mbs, so it complains.
Another way around it is "ret = 0; goto cleanup;" to trick it into
thinking it's calling the Free function.
OK I will change to "if (nmb !=
0)"
> +
> + for (i = 0; i < nmb; i++) {
> + tmp = strchr(mbs[i], '=');
strchr can return NULL, which would cause the following line all sorts
of problems. Yes, I know how it was written, but better safe than sorry.
OK I will
add a NULL pointer testing here
> + *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;
> + }
Including the VIR_ALLOC above... here to ...
> + 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;
> + }
...here replicates virResctrlSetMemoryBandwidth (reuse, recycle, don't
cause someone to change 2 places because they *will forget* one!)
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.
> + 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%*/
Perhaps this answers my question above about why the %%, but I'm not
100% sure
> + 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)
This is a strange calculation especially since ret->mba->nsizes has to
be zero because we just did a VIR_ALLOC(ret->mba).
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.
> + 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;
I think I'm missing something subtle here. Maybe it's the "alloc" and
"free" - not quite sure. Perhaps some relationship that the *Subtract
method handles.
There's virResctrlSetMemoryBandwidth which restricts the
"mba->bandwidth[id]" with that collision message I noted, but perhaps
this is just some internal thing/logic that I'll "get" the next time I
review a patch series.
> + }
> + }
> +
> cleanup:
> virBitmapFree(mask);
> return ret;
> @@ -1233,6 +1608,7 @@ virResctrlAllocGetUnused(virResctrlInfoPtr resctrl)
> goto error;
> }
>
> + virResctrlMemoryBandwidthSubstract(ret, alloc);
Subtract
OK
> 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;
> +
Now that this is added, the description for this function should be
updated since it seems to be going beyond just size to mask now.
Yep! I will update
function description in next version.
Bing
John
> 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 *
>