Re: [libvirt] [PATCHv2 4/4] conf: Introduce RDT monitor host capability

-----Original Message----- From: John Ferlan [mailto:jferlan@redhat.com] Sent: Wednesday, September 19, 2018 3:47 AM To: Wang, Huaqiang <huaqiang.wang(a)intel.com>; libvir-list(a)redhat.com Cc: Feng, Shaohe <shaohe.feng(a)intel.com>; Niu, Bing <bing.niu(a)intel.com>; Ding, Jian-feng <jian-feng.ding(a)intel.com>; Zang, Rui <rui.zang(a)intel.com&gt; Subject: Re: [PATCHv2 4/4] conf: Introduce RDT monitor host capability 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(a)intel.com&gt; > --- > 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_thresh > old_occupancy create mode 100644 > tests/vircaps2xmldata/linux-resctrl-cmt/resctrl/info/L3_MON/mon_featur > es 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_m > ask create mode 100644 > tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/L3/min_c > bm_bits create mode 100644 > tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/L3/num_c > losids create mode 100644 > tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/L3_MON/m > ax_threshold_occupancy create mode 100644 > tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/L3_MON/m > on_features create mode 100644 > tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/L3_MON/n > um_rmids create mode 100644 > tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/MB/bandw > idth_gran create mode 100644 > tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/MB/min_b > andwidth create mode 100644 > tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/MB/num_c > losids create mode 100644 > tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/manualres/cpu > s create mode 100644 > tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/manualres/sch > emata create mode 100644 > tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/manualres/tas > ks 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; > } >