[libvirt] [PATCHv2 4/4] conf: Introduce RDT monitor host capability
John Ferlan
jferlan at redhat.com
Tue Sep 18 19:47:12 UTC 2018
ca
On 09/14/2018 09:30 PM, Wang Huaqiang wrote:
> Cache monitoring technology(CMT) and memory bandwidth
> monitoring technology(MBM) are resource monitoring
> part of Intel resource director technology(RDT).
>
> This patch is introducing cache monitor(CMT) to cache and
> memory bandwidth monitor(MBM) for monitoring CPU memory
> bandwidth.
There's some redundancy in the above...
> The host capability of the two monitors is also introduced
> in this patch.
The above two paragraphs can be collapsed to:
This patch introduces CMT and MBM for monitoring CPU cache
and memory bandwidth into the host capability XML output.
>
> For cache monitor, the host capability is shown like:
For CMT, ...
> <host>
> ...
> <cache>
> <bank id='0' level='3' type='both' size='15' unit='MiB' cpus='0-5'>
> <control granularity='768' min='1536' unit='KiB' type='both' maxAllocs='4'/>
> </bank>
> <monitor level='3' 'reuseThreshold'='270336' maxMonitors='176'>
> <feature name='llc_occupancy'/>
> </monitor>
> </cache>
> ...
> </host>
>
> For memory bandwidth monitor, the capability is shown like this:
For MBM, ...
>
> <host>
> ...
> <memory_bandwidth>
> <node id='1' cpus='6-11'>
> <control granularity='10' min ='10' maxAllocs='4'/>
> </node>
> <monitor maxMonitors='176'>
> <feature name='mbm_total_bytes'/>
> <feature name='mbm_local_bytes'/>
> </monitor>
> </memory_bandwidth>
> ...
> </host>
>
> Signed-off-by: Wang Huaqiang <huaqiang.wang at intel.com>
> ---
> docs/schemas/capability.rng | 37 ++++++-
> src/conf/capabilities.c | 68 ++++++++++++
> src/conf/capabilities.h | 4 +
> src/libvirt_private.syms | 2 +
> src/util/virresctrl.c | 120 ++++++++++++++++++++-
> src/util/virresctrl.h | 62 +++++++++++
> .../resctrl/info/L3_MON/max_threshold_occupancy | 1 +
> .../resctrl/info/L3_MON/mon_features | 1 +
> .../resctrl/info/L3_MON/num_rmids | 1 +
> .../linux-resctrl-cmt/resctrl/manualres/cpus | 1 +
> .../linux-resctrl-cmt/resctrl/manualres/schemata | 1 +
> .../linux-resctrl-cmt/resctrl/manualres/tasks | 0
> .../linux-resctrl-cmt/resctrl/schemata | 1 +
> tests/vircaps2xmldata/linux-resctrl-cmt/system | 1 +
> .../resctrl/info/L3/cbm_mask | 1 +
> .../resctrl/info/L3/min_cbm_bits | 1 +
> .../resctrl/info/L3/num_closids | 1 +
> .../resctrl/info/L3_MON/max_threshold_occupancy | 1 +
> .../resctrl/info/L3_MON/mon_features | 10 ++
> .../resctrl/info/L3_MON/num_rmids | 1 +
> .../resctrl/info/MB/bandwidth_gran | 1 +
> .../resctrl/info/MB/min_bandwidth | 1 +
> .../resctrl/info/MB/num_closids | 1 +
> .../resctrl/manualres/cpus | 1 +
> .../resctrl/manualres/schemata | 1 +
> .../resctrl/manualres/tasks | 0
> .../linux-resctrl-fake-feature/resctrl/schemata | 1 +
> .../linux-resctrl-fake-feature/system | 1 +
> .../resctrl/info/L3_MON/max_threshold_occupancy | 1 +
> .../linux-resctrl/resctrl/info/L3_MON/mon_features | 3 +
> .../linux-resctrl/resctrl/info/L3_MON/num_rmids | 1 +
> .../vircaps2xmldata/vircaps-x86_64-resctrl-cmt.xml | 53 +++++++++
> .../vircaps-x86_64-resctrl-fake-feature.xml | 73 +++++++++++++
> tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml | 7 ++
> tests/vircaps2xmltest.c | 2 +
> 35 files changed, 459 insertions(+), 3 deletions(-)
> create mode 100644 tests/vircaps2xmldata/linux-resctrl-cmt/resctrl/info/L3_MON/max_threshold_occupancy
> create mode 100644 tests/vircaps2xmldata/linux-resctrl-cmt/resctrl/info/L3_MON/mon_features
> create mode 100644 tests/vircaps2xmldata/linux-resctrl-cmt/resctrl/info/L3_MON/num_rmids
> create mode 100644 tests/vircaps2xmldata/linux-resctrl-cmt/resctrl/manualres/cpus
> create mode 100644 tests/vircaps2xmldata/linux-resctrl-cmt/resctrl/manualres/schemata
> create mode 100644 tests/vircaps2xmldata/linux-resctrl-cmt/resctrl/manualres/tasks
> create mode 100644 tests/vircaps2xmldata/linux-resctrl-cmt/resctrl/schemata
> create mode 120000 tests/vircaps2xmldata/linux-resctrl-cmt/system
> create mode 100644 tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/L3/cbm_mask
> create mode 100644 tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/L3/min_cbm_bits
> create mode 100644 tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/L3/num_closids
> create mode 100644 tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/L3_MON/max_threshold_occupancy
> create mode 100644 tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/L3_MON/mon_features
> create mode 100644 tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/L3_MON/num_rmids
> create mode 100644 tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/MB/bandwidth_gran
> create mode 100644 tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/MB/min_bandwidth
> create mode 100644 tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/MB/num_closids
> create mode 100644 tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/manualres/cpus
> create mode 100644 tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/manualres/schemata
> create mode 100644 tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/manualres/tasks
> create mode 100644 tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/schemata
> create mode 120000 tests/vircaps2xmldata/linux-resctrl-fake-feature/system
> create mode 100644 tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3_MON/max_threshold_occupancy
> create mode 100644 tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3_MON/mon_features
> create mode 100644 tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3_MON/num_rmids
> create mode 100644 tests/vircaps2xmldata/vircaps-x86_64-resctrl-cmt.xml
> create mode 100644 tests/vircaps2xmldata/vircaps-x86_64-resctrl-fake-feature.xml
>
> diff --git a/docs/schemas/capability.rng b/docs/schemas/capability.rng
> index d61515c..fe12bf9 100644
> --- a/docs/schemas/capability.rng
> +++ b/docs/schemas/capability.rng
> @@ -316,6 +316,9 @@
> </zeroOrMore>
> </element>
> </oneOrMore>
> + <optional>
> + <ref name='cpuMonitor'/>
> + </optional>
> </element>
> </define>
>
> @@ -347,7 +350,7 @@
> <optional>
> <attribute name='min'>
> <ref name='unsignedInt'/>
> - </attribute>
> + </attribute>
> </optional>
> <attribute name='maxAllocs'>
> <ref name='unsignedInt'/>
> @@ -356,9 +359,41 @@
> </zeroOrMore>
> </element>
> </oneOrMore>
> + <optional>
> + <ref name='cpuMonitor'/>
> + </optional>
> </element>
> </define>
>
> + <define name='cpuMonitor'>
> + <element name='monitor'>
> + <optional>
> + <attribute name='level'>
> + <ref name='unsignedInt'/>
> + </attribute>
> + <attribute name='reuseThreshold'>
> + <ref name='unsignedInt'/>
> + </attribute>
> + </optional>
Ironically you fixed alignment above, but created more misalignment.
I've cleaned it up...
> + <attribute name='maxMonitors'>
> + <ref name='unsignedInt'/>
> + </attribute>
> + <oneOrMore>
> + <element name='feature'>
> + <attribute name='name'>
> + <ref name='monitorFeature'/>
> + </attribute>
> + </element>
> + </oneOrMore>
> + </element>
> + </define>
> +
> + <define name='monitorFeature'>
> + <data type='string'>
> + <param name='pattern'>(llc_|mbm_)[a-zA-Z0-9\-_]+</param>
> + </data>
> + </define>
> +
> <define name='guestcaps'>
> <element name='guest'>
> <ref name='ostype'/>
> diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
> index 66ad420..ba69d6f 100644
> --- a/src/conf/capabilities.c
> +++ b/src/conf/capabilities.c
> @@ -247,10 +247,12 @@ virCapsDispose(void *object)
>
> for (i = 0; i < caps->host.cache.nbanks; i++)
> virCapsHostCacheBankFree(caps->host.cache.banks[i]);
> + virResctrlInfoMonFree(caps->host.cache.monitor);
> VIR_FREE(caps->host.cache.banks);
>
> for (i = 0; i < caps->host.memBW.nnodes; i++)
> virCapsHostMemBWNodeFree(caps->host.memBW.nodes[i]);
> + virResctrlInfoMonFree(caps->host.memBW.monitor);
> VIR_FREE(caps->host.memBW.nodes);
>
> VIR_FREE(caps->host.netprefix);
> @@ -867,6 +869,50 @@ virCapabilitiesFormatNUMATopology(virBufferPtr buf,
> }
>
Two blank lines
> static int
> +virCapabilitiesFormatResctrlMonitor(virBufferPtr buf,
> + virResctrlInfoMonPtr monitor)
> +{
> + size_t i = 0;
> + virBuffer childrenBuf = VIR_BUFFER_INITIALIZER;
> +
> + /* monitor not supported, no capability */
> + if (!monitor)
> + return 0;
> +
> + /* no feature found in mointor means no capability, return */
*monitor
> + if (monitor->nfeatures == 0)
> + return 0;
I don't believe we can get the above. See [1] in
virResctrlInfoGetMonitorPrefix - we never steal @mon for @*monitor when
mon->nfeatures == 0
I can leave it for safety, unless you think it would ever be useful to
display:
<monitor maxMonitors='127'/>
In which case I'll move the == 0 check below, slap a "/>" on and return
0 - ignoring childrenBuf
> +
> + virBufferAddLit(buf, "<monitor ");
> +
> + /* CMT might not enabled, if enabled show related attributes. */
> + if (monitor->type == VIR_MONITOR_TYPE_CACHE)
> + virBufferAsprintf(buf,
> + "level='%u' reuseThreshold='%u' ",
> + monitor->cache_level,
> + monitor->cache_reuse_threshold);
> + virBufferAsprintf(buf,
> + "maxMonitors='%u'>\n",
> + monitor->max_monitor);
> +
One less blank line here.
> +
> + for (i = 0; i < monitor->nfeatures; i++) {
> + virBufferSetChildIndent(&childrenBuf, buf);
This can be to put this outside the for loop
> + virBufferAsprintf(&childrenBuf,
> + "<feature name='%s'/>\n",
> + monitor->features[i]);
> + }
> +
> + if (virBufferCheckError(&childrenBuf) < 0)
> + return -1;
> +
> + virBufferAddBuffer(buf, &childrenBuf);
> + virBufferAddLit(buf, "</monitor>\n");
> +
> + return 0;
> +}
> +
> +static int
> virCapabilitiesFormatCaches(virBufferPtr buf,
> virCapsHostCachePtr cache)
> {
> @@ -953,6 +999,10 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
> }
> }
>
> + if (virCapabilitiesFormatResctrlMonitor(buf,
> + cache->monitor) < 0)
Fits on previous line, I moved it.
> + return -1;
> +
> virBufferAdjustIndent(buf, -2);
> virBufferAddLit(buf, "</cache>\n");
>
> @@ -1004,6 +1054,10 @@ virCapabilitiesFormatMemoryBandwidth(virBufferPtr buf,
> }
> }
>
> + if (virCapabilitiesFormatResctrlMonitor(buf,
> + memBW->monitor) < 0)
Same here.
> + return -1;
> +
> virBufferAdjustIndent(buf, -2);
> virBufferAddLit(buf, "</memory_bandwidth>\n");
>
> @@ -1664,6 +1718,8 @@ virCapabilitiesInitResctrlMemory(virCapsPtr caps)
> size_t i = 0;
> int ret = -1;
>
Removed blank line here.
> + const char *prefix = virMonitorPrefixTypeToString(VIR_MONITOR_TYPE_MEMBW);
> +
> for (i = 0; i < caps->host.cache.nbanks; i++) {
> virCapsHostCacheBankPtr bank = caps->host.cache.banks[i];
> if (VIR_ALLOC(node) < 0)
> @@ -1684,6 +1740,11 @@ virCapabilitiesInitResctrlMemory(virCapsPtr caps)
> node = NULL;
> }
>
> + if (virResctrlInfoGetMonitorPrefix(caps->host.resctrl,
> + prefix,
Fits on previous line
> + &caps->host.memBW.monitor) < 0)
> + goto cleanup;
> +
> ret = 0;
> cleanup:
> virCapsHostMemBWNodeFree(node);
> @@ -1704,6 +1765,8 @@ virCapabilitiesInitCaches(virCapsPtr caps)
> struct dirent *ent = NULL;
> virCapsHostCacheBankPtr bank = NULL;
>
Same - removed blank line
> + const char *prefix = virMonitorPrefixTypeToString(VIR_MONITOR_TYPE_CACHE);
> +
> /* Minimum level to expose in capabilities. Can be lowered or removed (with
> * the appropriate code below), but should not be increased, because we'd
> * lose information. */
> @@ -1823,6 +1886,11 @@ virCapabilitiesInitCaches(virCapsPtr caps)
> if (virCapabilitiesInitResctrlMemory(caps) < 0)
> goto cleanup;
>
> + if (virResctrlInfoGetMonitorPrefix(caps->host.resctrl,
> + prefix,
Fits on previous line.
> + &caps->host.cache.monitor) < 0)
> + goto cleanup;
> +
> ret = 0;
> cleanup:
> VIR_FREE(type);
> diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h
> index 694fd6b..45b331a 100644
> --- a/src/conf/capabilities.h
> +++ b/src/conf/capabilities.h
> @@ -156,6 +156,8 @@ typedef virCapsHostCache *virCapsHostCachePtr;
> struct _virCapsHostCache {
> size_t nbanks;
> virCapsHostCacheBankPtr *banks;
> +
> + virResctrlInfoMonPtr monitor;
> };
>
> typedef struct _virCapsHostMemBWNode virCapsHostMemBWNode;
> @@ -171,6 +173,8 @@ typedef virCapsHostMemBW *virCapsHostMemBWPtr;
> struct _virCapsHostMemBW {
> size_t nnodes;
> virCapsHostMemBWNodePtr *nodes;
> +
> + virResctrlInfoMonPtr monitor;
> };
>
> typedef struct _virCapsHost virCapsHost;
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index e7a4d61..8061e1c 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2668,6 +2668,8 @@ virResctrlAllocSetCacheSize;
> virResctrlAllocSetID;
> virResctrlAllocSetMemoryBandwidth;
> virResctrlInfoGetCache;
> +virResctrlInfoGetMonitorPrefix;
> +virResctrlInfoMonFree;
> virResctrlInfoNew;
>
>
> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
> index 4601f69..156618f 100644
> --- a/src/util/virresctrl.c
> +++ b/src/util/virresctrl.c
> @@ -70,6 +70,18 @@ VIR_ENUM_IMPL(virResctrl, VIR_CACHE_TYPE_LAST,
> "CODE",
> "DATA")
>
> +/* Monitor name mapping for monitor naming */
> +VIR_ENUM_IMPL(virMonitor, VIR_MONITOR_TYPE_LAST,
> + "unsupported monitor",
> + "cache monitor",
> + "memory bandwidth monitor")
This is probably overkill for the usage in a VIR_INFO, I've removed it
and you'll see my suggestion for VIR_INFO below.
> +
> +/* Monitor feature name prefix mapping for monitor naming */
> +VIR_ENUM_IMPL(virMonitorPrefix, VIR_MONITOR_TYPE_LAST,
Should be "virResctrlMonitorPrefix"
> + "__unsupported__",
> + "llc_",
> + "mbm_")
> +
>
> /* All private typedefs so that they exist for all later definitions. This way
> * structs can be included in one or another without reorganizing the code every
> @@ -207,6 +219,17 @@ virResctrlInfoDispose(void *obj)
> }
>
>
> +void
> +virResctrlInfoMonFree(virResctrlInfoMonPtr mon)
> +{
> + if (!mon)
> + return;
> +
> + virStringListFree(mon->features);
> + VIR_FREE(mon);
> +}
> +
> +
> /* virResctrlAlloc */
>
> /*
> @@ -686,11 +709,11 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl)
> if (ret < 0)
> goto cleanup;
>
> - ret = virResctrlGetCacheInfo(resctrl, dirp);
> + ret = virResctrlGetMonitorInfo(resctrl);
> if (ret < 0)
> goto cleanup;
>
> - ret = virResctrlGetMonitorInfo(resctrl);
> + ret = virResctrlGetCacheInfo(resctrl, dirp);
Why did the order switch?
If they need to be this order, then patch1 needs adjustment.
I've changed back to the former order for now.
> if (ret < 0)
> goto cleanup;
>
> @@ -851,6 +874,99 @@ virResctrlInfoGetCache(virResctrlInfoPtr resctrl,
> }
>
>
> +/* virResctrlInfoGetMonitorPrefix
> + *
> + * @resctrl: Pointer to virResctrlInfo
> + * @prefix: Monitor prefix name for monitor looking for.
> + * @monitor: Returns the capability inforamtion for taget monitor if the
*information
*target
> + * monitor with prefix name @prefex is supported by host.
s/prefix name @prefex/@prefix
> + *
> + * Return monitor capability information describe in prefix name @prefix
> + * through @monitor
* Return monitor capability information for @prefix through @monitor.
* It is possible to return an empty list of features for @monitor
* leaving it up to the caller to handle.
> + *
> + * Returns 0 if a monitor is found or a valid monitor is not supported by host,
> + * -1 on failure with error message set.
> + * */
s/* */*/
> +int
> +virResctrlInfoGetMonitorPrefix(virResctrlInfoPtr resctrl,
> + const char *prefix,
> + virResctrlInfoMonPtr *monitor)
> +{
> + size_t i = 0;
> + virResctrlInfoMongrpPtr mongrp_info = NULL;
> + virResctrlInfoMonPtr mon = NULL;
> + int ret = -1;
> +
> + if (virResctrlInfoIsEmpty(resctrl))
> + return 0;
> +
> + mongrp_info = resctrl->monitor_info;
> +
> + if (!mongrp_info) {
> + VIR_INFO("Monitor is not supported in host");
> + return 0;
> + }
> +
> + if (!prefix) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Empty prefix name for resctrl monitor"));
> + return -1;
> + }
Could be earlier, but it whatever.
> +
> + for (i = 0; i < VIR_MONITOR_TYPE_LAST; i++) {
> + if (STREQ(prefix, virMonitorPrefixTypeToString(i))) {
I'm sure there's a way with ARRAY_CARDINALITY too - that's something I
haven't quite figured out.
> + if (VIR_ALLOC(mon) < 0)
> + goto cleanup;
> + mon->type = i;
> + break;
> + }
> + }
> +
> + if (!mon) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Bad prefix name \"%s\" for resctrl monitor"),
use '%s'
> + prefix);
> + goto cleanup;
could be just return -1 since @mon == NULL and that's all cleanup does
is clean it up.
> + }
> +
> + mon->max_monitor = mongrp_info->max_monitor;
> +
> + if (mon->type == VIR_MONITOR_TYPE_CACHE) {
> + mon->cache_reuse_threshold = mongrp_info->cache_reuse_threshold;
> + mon->cache_level = mongrp_info->cache_level;
s/= /= /
Just one space.
> + }
> +
> + for (i = 0; i < mongrp_info->nfeatures; i++) {
> + if (STRPREFIX(mongrp_info->features[i], prefix)) {
> + if (virStringListAdd(&mon->features,
> + mongrp_info->features[i]) < 0)
> + goto cleanup;
> + mon->nfeatures++;
> + }
> + }
> +
> + ret = 0;
> +
> + if (mon->nfeatures == 0) {
> + /* No feature found for current monitor, means host does not support
> + * monitor type with @prefix name.
> + * Telling caller this monitor is supported by hardware specification,
> + * but not supported by this host */
> + VIR_INFO("%s is not supported by host",
> + virMonitorTypeToString(mon->type));
Actually this just logs a message (if we're printing VIR_INFO) on the
daemon side. The consumer on the other end is goint to have to "handle"
nfeatures == 0 then.
Also, since I think @virMonitor is overkill, let's just go with:
VIR_INFO("No resctrl monitor features using prefix '%s' found",
prefix);
> + goto cleanup;
[1] If we go to cleanup, then we return 0, but never steal @mon. Our
"consumer" that does the Format won't ever get @*monitor filled in.
> + }
> +
> + /* In case *monitor is pointed to some monitor, clean it. */
> + VIR_FREE(*monitor);
This should be virResctrlInfoMonFree, right?
> +
> + VIR_STEAL_PTR(*monitor, mon);
> + cleanup:
> + virResctrlInfoMonFree(mon);
> + return ret;
> +}
> +
> +
> /* virResctrlAlloc-related definitions */
> virResctrlAllocPtr
> virResctrlAllocNew(void)
> diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
> index cfd56dd..949e20f 100644
> --- a/src/util/virresctrl.h
> +++ b/src/util/virresctrl.h
> @@ -36,6 +36,16 @@ typedef enum {
> VIR_ENUM_DECL(virCache);
> VIR_ENUM_DECL(virCacheKernel);
>
The following are missing the Resctrl or RESCRTL_ name "prefix"... e.g.
virResctrlMonitor* and VIR_RESCTRL_MONITOR*
I'll adjust...
> +typedef enum {
> + VIR_MONITOR_TYPE_UNSUPPORT,
> + VIR_MONITOR_TYPE_CACHE,
> + VIR_MONITOR_TYPE_MEMBW,
> +
> + VIR_MONITOR_TYPE_LAST
> +} virMonitorType;
> +VIR_ENUM_DECL(virMonitor);
No use for ^^ this one.
> +VIR_ENUM_DECL(virMonitorPrefix);
> +
>
> typedef struct _virResctrlInfoPerCache virResctrlInfoPerCache;
> typedef virResctrlInfoPerCache *virResctrlInfoPerCachePtr;
> @@ -61,6 +71,51 @@ struct _virResctrlInfoMemBWPerNode {
> unsigned int max_allocation;
> };
>
> +typedef struct _virResctrlInfoMon virResctrlInfoMon;
> +typedef virResctrlInfoMon *virResctrlInfoMonPtr;
> +struct _virResctrlInfoMon {
> + /* Common fields */
> +
> + /* Maximum number of simultaneous monitors */
> + unsigned int max_monitor;
> + /* null-terminal string list for monitor features */
> + char **features;
> + /* Number of monitor features */
> + size_t nfeatures;
> + /* Monitor type */
> + virMonitorType type;
virResctrlMonitorType type;
I'm not a total fan of just 'type', but there's precedent so I won't
change it. Finding "->type " in the code can be like looking a needle
in the haystack.
> +
> + /* cache monitor (CMT) related field
> + *
> + * CMT has following resource monitor event:
> + * "llc_occupancy"
> + *
> + * If this is a cache monitor, the memory bandwidth monitor related
> + * fields and feture events will not be valid. */
*feature
Hmmm.. I'm not really sure how to read the above. Is it "important" for
the consumer to know the relationship to MBM from a data fetching viewpoint?
> +
> + /* This Adjustable value affects the final reuse of resources used by
> + * monitor. After the action of removing a monitor, the kernel may not
> + * release all hardware resources that monitor used immediately if the
> + * cache occupancy value associated with 'removed' monitor is above this
> + * threshold. Once the cache occupancy is below this threshold, the
> + * underlying hardware resource will be reclaimed and be put into the
> + * resource pool for next reusing.*/
> + unsigned int cache_reuse_threshold;
> + /* The cache 'level' that has the monitor capability */
> + unsigned int cache_level;
> +
> + /* memory bandwidth monitor (MBM) related field
> + *
> + * MBM has following resource monitor events:
> + * "mbm_total_bytes"
> + * "mbm_local_bytes"
> + *
> + * If this is a memory bandwidth monitor, the cache monitor related
> + * fields and feature events will not be valid. */
Similar type, but opposite question - is this an implementation detail
that the normal consumer really won't have to know.
> +
> + /* MBM related field is empty */
Kind of an odd comment - not sure what to do with it or think about it.
> +};
> +
> typedef struct _virResctrlInfo virResctrlInfo;
> typedef virResctrlInfo *virResctrlInfoPtr;
>
> @@ -145,4 +200,11 @@ virResctrlAllocAddPID(virResctrlAllocPtr alloc,
> int
> virResctrlAllocRemove(virResctrlAllocPtr alloc);
>
> +void
> +virResctrlInfoMonFree(virResctrlInfoMonPtr mon);
> +
> +int
> +virResctrlInfoGetMonitorPrefix(virResctrlInfoPtr resctrl,
> + const char *prefix,
> + virResctrlInfoMonPtr *monitor);
Use ATTRIBUTE_NONNULL(3)... The will *Free it and replace it so if what
@*monitor points at is NULL, that's fine, but if it's NULL, then my
coverity build will capture that.
> #endif /* __VIR_RESCTRL_H__ */
[...]
> diff --git a/tests/vircaps2xmltest.c b/tests/vircaps2xmltest.c
> index 1fb8861..7f3c26a 100644
> --- a/tests/vircaps2xmltest.c
> +++ b/tests/vircaps2xmltest.c
> @@ -111,9 +111,11 @@ mymain(void)
> DO_TEST_FULL("caches", VIR_ARCH_X86_64, true, true);
>
> DO_TEST_FULL("resctrl", VIR_ARCH_X86_64, true, true);
> + DO_TEST_FULL("resctrl-cmt", VIR_ARCH_X86_64, true, true);
So this one just shows that <memory_bandwidth> may not be displayed,
true? And of course the <control> is optional too for <bank>.
I can make all the suggested alterations with your OK. I think there
were a couple of questions about things which I think we can easily
hammer out.
John
> DO_TEST_FULL("resctrl-cdp", VIR_ARCH_X86_64, true, true);
> DO_TEST_FULL("resctrl-skx", VIR_ARCH_X86_64, true, true);
> DO_TEST_FULL("resctrl-skx-twocaches", VIR_ARCH_X86_64, true, true);
> + DO_TEST_FULL("resctrl-fake-feature", VIR_ARCH_X86_64, true, true);
>
> return ret;
> }
>
More information about the libvir-list
mailing list