[libvirt] [RFCv2 PATCH 2/5] util: Add memory bandwidth support to resctrl

John Ferlan jferlan at redhat.com
Fri Jun 29 22:39:05 UTC 2018



On 06/15/2018 05:29 AM, bing.niu at intel.com wrote:
> From: Bing Niu <bing.niu at intel.com>
> 
> Add memory bandwidth allocation support basing on existing

s/basing/based

> virresctrl implementation. Two new structures virResctrlInfoMB
            ^^
"cache"   (IOW: fit it in between)

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

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

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.

> 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

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

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

> +    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 ("* */"

> +    unsigned int max_id;
> +};
> +
>  struct _virResctrlInfo {
>      virObject parent;
>  
>      virResctrlInfoPerLevelPtr *levels;
>      size_t nlevels;
> +
> +    virResctrlInfoMBPtr mb_info;

How about "MemBWInfo" or membw_info

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

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

> +};
> +
>  struct _virResctrlAlloc {
>      virObject parent;
>  
>      virResctrlAllocPerLevelPtr *levels;
>      size_t nlevels;
>  
> +    virResctrlAllocMBPtr mba;

s/mba/MemBW  or MemBWAlloc

Also add a blank line for reading separation...

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

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

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

> +        virReportError(VIR_ERR_XML_ERROR,
> +                       _("Collision Memory Bandwidth on node %d"),

Memory Bandwidth already defined for node %u

(it's unsigned...)

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

> +{
> +    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]));
> +        }
> +    }
> +
> +    virBufferTrim(buf, ";", 1);
> +    virBufferAddChar(buf, '\n');
> +    virBufferCheckError(buf);
> +    return 0;
> +}
> +
> +
> +static int
> +virResctrlAllocMemoryBandwidth(virResctrlInfoPtr resctrl,
> +                               virResctrlAllocPtr alloc, virResctrlAllocPtr free)

One argument per line

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

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

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

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

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

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

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

> +        return 0;
> +
> +    if (!resctrl || !resctrl->mb_info
> +        || !resctrl->mb_info->min_bandwidth
> +        || !resctrl->mb_info->bandwidth_granularity) {

Put the || on the previous line

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

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

> +        *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!)

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

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

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

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




More information about the libvir-list mailing list