[libvirt] [PATCHv2 4/4] conf: Introduce RDT monitor host capability
Wang, Huaqiang
huaqiang.wang at intel.com
Thu Sep 20 09:02:57 UTC 2018
> -----Original Message-----
> From: John Ferlan [mailto:jferlan at redhat.com]
> Sent: Wednesday, September 19, 2018 3:47 AM
> To: Wang, Huaqiang <huaqiang.wang at intel.com>; libvir-list at redhat.com
> Cc: Feng, Shaohe <shaohe.feng at intel.com>; Niu, Bing <bing.niu at intel.com>;
> Ding, Jian-feng <jian-feng.ding at intel.com>; Zang, Rui <rui.zang at intel.com>
> 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 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_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...
Embarrassing...
Will be fixed.
>
> > + <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
>
Will be fixed.
> > 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
Will be fixed.
>
> > + 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
Yes, mon->nfeatures will not be 0, it is ensured in time of creating the
*monitor. But we should still keep the two lines here to prevent the emergence
of
<monitor maxMonitors='127'/>
"<monitor maxMonitors='127'/>" is invalid from the perspective of any
existing hardware. I can not image the use of a resource monitor without a
counter (indicating through feature name ) associating with it.
>
> 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.
Will be fixed.
>
> > +
> > + for (i = 0; i < monitor->nfeatures; i++) {
> > + virBufferSetChildIndent(&childrenBuf, buf);
>
> This can be to put this outside the for loop
Thanks, will be moved outside.
>
> > + 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.
You mean change it to
if (virCapabilitiesFormatResctrlMonitor(buf, cache->monitor) < 0)
right?
Will be fixed.
>
> > + return -1;
> > +
> > virBufferAdjustIndent(buf, -2);
> > virBufferAddLit(buf, "</cache>\n");
> >
> > @@ -1004,6 +1054,10 @@
> virCapabilitiesFormatMemoryBandwidth(virBufferPtr buf,
> > }
> > }
> >
> > + if (virCapabilitiesFormatResctrlMonitor(buf,
> > + memBW->monitor) < 0)
>
> Same here.
Will be fixed.
>
> > + 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.
Will be fixed.
>
> > + 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
Will be fxied.
>
> > + &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
OK.
>
> > + 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.
Will be fixed.
>
> > + &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.
Let's remove it as well as that 'VIR_INFO' line.
>
> > +
> > +/* Monitor feature name prefix mapping for monitor naming */
> > +VIR_ENUM_IMPL(virMonitorPrefix, VIR_MONITOR_TYPE_LAST,
>
> Should be "virResctrlMonitorPrefix"
Agree.
>
> > + "__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.
It is swapped by unknown reason, and I don't need any kind of
specific order, let's use the original order.
>
> > 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
Will be fixed.
>
> > + * monitor with prefix name @prefex is supported by host.
>
> s/prefix name @prefex/@prefix
Will be fixed.
>
> > + *
> > + * 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.
No, no empty feature list will be returned. If feature list for monitor
with @prefix is empty, @monitor will be set to NULL, and 0 will will
be returned. This tells caller the monitor with @prefix is not supported
in system and which is a tolerable case.
This will be guaranteed by performing following line
/* In case *monitor is pointed to some monitor, clean it. */
virResctrlInfoMonFree(*monitor);
This line will be moved like this, to ensure *monitor=NULL if mon->nfeatures == 0
ret = 0;
+ /* In case *monitor is pointed to some monitor, clean it. */
+ virResctrlInfoMonFree(*monitor);
+
if (mon->nfeatures == 0) {
/* No feature found for current monitor, means host does not support
* monitor type with @prefix name.
@@ -959,11 +957,8 @@ virResctrlInfoGetMonitorPrefix(virResctrlInfoPtr resctrl,
goto cleanup;
}
- /* In case *monitor is pointed to some monitor, clean it. */
- VIR_FREE(*monitor);
-
VIR_STEAL_PTR(*monitor, mon);
>
> > + *
> > + * Returns 0 if a monitor is found or a valid monitor is not
> > + supported by host,
> > + * -1 on failure with error message set.
> > + * */
>
> s/* */*/
Will be fixed.
New notation for this function is like this:
/* virResctrlInfoGetMonitorPrefix
*
* @resctrl: Pointer to virResctrlInfo
* @prefix: Monitor prefix name for monitor looking for.
* @monitor: Returns the capability information for target monitor if the
* monitor with @prefex is supported by host.
*
* Return monitor capability information for @prefix through @monitor.
* If monitor with @prefix is not supported in system, @monitor will be
* cleared to NULL.
*
* Returns 0 if @monitor is created or monitor type with @prefix is not
* supported by host, -1 on failure with error message set.
*/
>
> > +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.
Moved before line
if (virResctrlInfoIsEmpty(resctrl))
>
> > +
> > + 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.
Can we use enum type data structure? It is quite common in libvirt.
It should be ok for using array and ARRAY_CARDINALITY, but need to change some
code.
>
> > + 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'
OK.
>
> > + prefix);
> > + goto cleanup;
>
> could be just return -1 since @mon == NULL and that's all cleanup does is clean
> it up.
Yes, I think it could return -1 directly here.
>
> > + }
> > +
> > + 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.
Will be fixed.
>
> > + }
> > +
> > + 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.
Just tell user some resctrl monitoring is not supported by your system.
>
> Also, since I think @virMonitor is overkill, let's just go with:
>
> VIR_INFO("No resctrl monitor features using prefix '%s' found",
> prefix);
Will remove VIR_ENUM_IMPL(virMonitor...) and will change as you recommended.
>
>
> > + 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.
return 0, and clear *monitor to NULL, means monitor with @prefix is not
supported here. caller should continue to check next type monitor.
>
> > + }
> > +
> > + /* In case *monitor is pointed to some monitor, clean it. */
> > + VIR_FREE(*monitor);
>
> This should be virResctrlInfoMonFree, right?
Good catch. Will be fixed.
>
> > +
> > + 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...
Your naming is better. Thanks.
>
> > +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
Will be fixed.
>
> 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.
>
Seems I said too much. Let's use an more precise way:
struct _virResctrlInfoMon {
/* 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 */
virResctrlMonitorType type;
/* 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;
};
> > +};
> > +
> > 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>.
Yes. CMT, MBM as well as CAT, MBA are all independent hardware features, we
may encounter some system only support CMT, this test is designed for
this case.
>
> 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.
Thank you for your review.
I'll also submit the patches fixed, just for your reference.
Huaqiang
>
> 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