> -----Original Message----- > From: John Ferlan [mailto:jferlan@xxxxxxxxxx] > Sent: Wednesday, September 19, 2018 3:47 AM > To: Wang, Huaqiang <huaqiang.wang@xxxxxxxxx>; libvir-list@xxxxxxxxxx > Cc: Feng, Shaohe <shaohe.feng@xxxxxxxxx>; Niu, Bing <bing.niu@xxxxxxxxx>; > Ding, Jian-feng <jian-feng.ding@xxxxxxxxx>; Zang, Rui <rui.zang@xxxxxxxxx> > 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@xxxxxxxxx> > > --- > > 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; > > } > > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
