[libvirt] [PATCHv2 0/4] Introduce x86 RDT (CMT&MBM) host capability
This series of patches introduced the x86 Cache Monitoring Technology (CMT) to libvirt by interacting with kernel resource control (resctrl) interface. CMT is one of the Intel(R) x86 CPU feature which belongs to the Resource Director Technology (RDT). CMT reports the occupancy of the last level cache, which is shared by all CPU cores. In v1 series, we are introducing CMT for libvirt, including reporting host capability and creating CMT groups. Introducing host capability is pretty much a well self-contained step, we only cover this step in this series. As an extension of v1, MBM capability is also introduced. These patches will not cover the part of creating CMT groups, which will be subsequent patches. We have serval discussion about the enabling of CMT, please refer to following links for the RFCs. RFCv3 https://www.redhat.com/archives/libvir-list/2018-August/msg01213.html RFCv2 https://www.redhat.com/archives/libvir-list/2018-July/msg00409.html https://www.redhat.com/archives/libvir-list/2018-July/msg01241.html RFCv1 https://www.redhat.com/archives/libvir-list/2018-June/msg00674.html 1. About reason why CMT is necessary for libvirt? The perf events of 'CMT, MBML, MBMT' have been phased out since Linux kernel commit c39a0e2c8850f08249383f2425dbd8dbe4baad69, in libvirt the perf based cmt,mbm will not work with the latest linux kernel. These patches add CMT feature to libvirt through kernel resctrlfs interface. 2. Interfaces for CMT from the high level. CMT, CAT, MBM and MBA are orthogonal features, each could works independently. If 'CMT' is enabled in host, then a 'cache monitor' is introduced for cache, which is role is monitoring the last level cache utilization of target system process. Cache monitor capabilities is shown under element <cache>. 'MBM', a monitor named memory bandwidth monitor is introduced, for role of monitoring memory bandwidth utilization. The capability information block is located under <memory bandwidth> element. 2.1 Query the host capability of CMT. The element 'monitor' represents the host capabilities of CMT. The explanations of involved attributes: - 'maxMonitors': denotes the maximum monitoring groups could be created, which is limited by the number of hardware 'RMID'. - 'reuseThreshold': An 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. - 'llc_occupancy': a feature of CMT, reporting the last level cache occupancy information. - 'mbm_total_bytes': a feature of MBM, reporting total memory bandwidth utilization, in bytes, including local memory and remote memory for multi-node system. - 'mbm_local_bytes': a feature of MBM, reporting only local memory bandwidth utilization. # virsh capabilities ... <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> <bank id='1' level='3' type='both' size='15' unit='MiB' cpus='6-11'> <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> <memory_bandwidth> <node id='0' cpus='0-5'> <control granularity='10' min ='10' maxAllocs='4'/> </node> <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> Changes since v1: - Introduced MBM capability. - Capability layout changed * Moved <monitor> from cahe <bank> to <cache> * Renamed <Threshold> to <reuseThreshold> - Document for 'reuseThreshold' changed. - Introduced API virResctrlInfoGetMonitorPrefix - Added more tests, covering standalone CMT, fake new feature. - Creating CMT resource control group will be subsequent job. Wang Huaqiang (4): util: Introduce monitor capability interface conf: Refactor cache bank capability structure conf: Refactor memory bandwidth capability structure conf: Introduce RDT monitor host capability docs/schemas/capability.rng | 37 +++- src/conf/capabilities.c | 126 ++++++++--- src/conf/capabilities.h | 24 ++- src/libvirt_private.syms | 2 + src/util/virresctrl.c | 240 +++++++++++++++++++++ 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, 624 insertions(+), 36 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 -- 2.7.4
This patch introduces the resource monitor and creates the interface for getting host capability of resource monitor from the system resource control file system. The resource monitor take the role of RDT monitoring group, could be used to monitor the resource consumption information, such as the last level cache occupancy and the utilization of memory bandwidth. Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com> --- src/util/virresctrl.c | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 124 insertions(+) diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 4b5442f..4601f69 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -83,6 +83,9 @@ typedef virResctrlInfoPerLevel *virResctrlInfoPerLevelPtr; typedef struct _virResctrlInfoMemBW virResctrlInfoMemBW; typedef virResctrlInfoMemBW *virResctrlInfoMemBWPtr; +typedef struct _virResctrlInfoMongrp virResctrlInfoMongrp; +typedef virResctrlInfoMongrp *virResctrlInfoMongrpPtr; + typedef struct _virResctrlAllocPerType virResctrlAllocPerType; typedef virResctrlAllocPerType *virResctrlAllocPerTypePtr; @@ -139,6 +142,28 @@ struct _virResctrlInfoMemBW { unsigned int max_id; }; +struct _virResctrlInfoMongrp { + /* 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; + + /* Last level cache related information */ + + /* 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; +}; + struct _virResctrlInfo { virObject parent; @@ -146,6 +171,8 @@ struct _virResctrlInfo { size_t nlevels; virResctrlInfoMemBWPtr membw_info; + + virResctrlInfoMongrpPtr monitor_info; }; @@ -171,8 +198,12 @@ virResctrlInfoDispose(void *obj) VIR_FREE(level); } + if (resctrl->monitor_info) + VIR_FREE(resctrl->monitor_info->features); + VIR_FREE(resctrl->membw_info); VIR_FREE(resctrl->levels); + VIR_FREE(resctrl->monitor_info); } @@ -555,6 +586,92 @@ virResctrlGetMemoryBandwidthInfo(virResctrlInfoPtr resctrl) } +/* + * Retrieve monitor capability from the resource control file system. + * + * The monitor capability is exposed through "SYSFS_RESCTRL_PATH/info/L3_MON" + * directory under the resource control file system. The monitor capability is + * parsed by reading the interface files and stored in the structure + * 'virResctrlInfoMongrp'. + * + * Not all host supports the resource monitor, leave the pointer + * @resctrl->monitor_info empty if not supported. + */ +static int +virResctrlGetMonitorInfo(virResctrlInfoPtr resctrl) +{ + int ret = -1; + int rv = -1; + char *featurestr = NULL; + char **features = NULL; + size_t nfeatures = 0; + virResctrlInfoMongrpPtr info_monitor = NULL; + + if (VIR_ALLOC(info_monitor) < 0) + return -1; + + /* monitor only exists in leve 3 cache */ + info_monitor->cache_level = 3; + + rv = virFileReadValueUint(&info_monitor->max_monitor, + SYSFS_RESCTRL_PATH "/info/L3_MON/num_rmids"); + if (rv == -2) { + /* The file doesn't exist, so it's unusable for us, probably resource + * monitor unsupported */ + VIR_INFO("The path '" SYSFS_RESCTRL_PATH "/info/L3_MON/num_rmids' " + "does not exist"); + ret = 0; + goto cleanup; + } else if (rv < 0) { + /* Other failures are fatal, so just quit */ + goto cleanup; + } + + rv = virFileReadValueUint(&info_monitor->cache_reuse_threshold, + SYSFS_RESCTRL_PATH + "/info/L3_MON/max_threshold_occupancy"); + if (rv == -2) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot get max_threshold_occupancy from resctrl" + " info")); + } + if (rv < 0) + goto cleanup; + + rv = virFileReadValueString(&featurestr, + SYSFS_RESCTRL_PATH + "/info/L3_MON/mon_features"); + if (rv == -2) + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot get mon_features from resctrl info")); + if (rv < 0) + goto cleanup; + + if (!featurestr) { + /* if no feature found in "/info/L3_MON/mon_features", + * some error happens */ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot get feature list from resctrl info")); + ret = -1; + goto cleanup; + } + + features = virStringSplitCount(featurestr, "\n", 0, &nfeatures); + VIR_DEBUG("Resctrl supported %ld monitoring features", nfeatures); + + info_monitor->nfeatures = nfeatures; + VIR_STEAL_PTR(info_monitor->features, features); + VIR_STEAL_PTR(resctrl->monitor_info, info_monitor); + + ret = 0; + cleanup: + VIR_FREE(featurestr); + virStringListFree(features); + VIR_FREE(info_monitor); + return ret; +} + + static int virResctrlGetInfo(virResctrlInfoPtr resctrl) { @@ -573,6 +690,10 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl) if (ret < 0) goto cleanup; + ret = virResctrlGetMonitorInfo(resctrl); + if (ret < 0) + goto cleanup; + ret = 0; cleanup: VIR_DIR_CLOSE(dirp); @@ -613,6 +734,9 @@ virResctrlInfoIsEmpty(virResctrlInfoPtr resctrl) if (resctrl->membw_info) return false; + if (resctrl->monitor_info) + return false; + for (i = 0; i < resctrl->nlevels; i++) { virResctrlInfoPerLevelPtr i_level = resctrl->levels[i]; -- 2.7.4
On 09/14/2018 09:30 PM, Wang Huaqiang wrote:
This patch introduces the resource monitor and creates the interface for getting host capability of resource monitor from the system resource control file system.
The resource monitor take the role of RDT monitoring group, could be
*takes... s/, could/ and could/
used to monitor the resource consumption information, such as the last level cache occupancy and the utilization of memory bandwidth.
Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com> --- src/util/virresctrl.c | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 124 insertions(+)
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 4b5442f..4601f69 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -83,6 +83,9 @@ typedef virResctrlInfoPerLevel *virResctrlInfoPerLevelPtr; typedef struct _virResctrlInfoMemBW virResctrlInfoMemBW; typedef virResctrlInfoMemBW *virResctrlInfoMemBWPtr;
+typedef struct _virResctrlInfoMongrp virResctrlInfoMongrp; +typedef virResctrlInfoMongrp *virResctrlInfoMongrpPtr; + typedef struct _virResctrlAllocPerType virResctrlAllocPerType; typedef virResctrlAllocPerType *virResctrlAllocPerTypePtr;
@@ -139,6 +142,28 @@ struct _virResctrlInfoMemBW { unsigned int max_id; };
+struct _virResctrlInfoMongrp { + /* 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; + + /* Last level cache related information */ + + /* 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; +}; + struct _virResctrlInfo { virObject parent;
@@ -146,6 +171,8 @@ struct _virResctrlInfo { size_t nlevels;
virResctrlInfoMemBWPtr membw_info; + + virResctrlInfoMongrpPtr monitor_info; };
@@ -171,8 +198,12 @@ virResctrlInfoDispose(void *obj) VIR_FREE(level); }
+ if (resctrl->monitor_info) + VIR_FREE(resctrl->monitor_info->features);
virStringListFree
+ VIR_FREE(resctrl->membw_info); VIR_FREE(resctrl->levels); + VIR_FREE(resctrl->monitor_info); }
@@ -555,6 +586,92 @@ virResctrlGetMemoryBandwidthInfo(virResctrlInfoPtr resctrl) }
+/* + * Retrieve monitor capability from the resource control file system. + * + * The monitor capability is exposed through "SYSFS_RESCTRL_PATH/info/L3_MON" + * directory under the resource control file system. The monitor capability is + * parsed by reading the interface files and stored in the structure + * 'virResctrlInfoMongrp'. + * + * Not all host supports the resource monitor, leave the pointer + * @resctrl->monitor_info empty if not supported. + */ +static int +virResctrlGetMonitorInfo(virResctrlInfoPtr resctrl) +{ + int ret = -1; + int rv = -1; + char *featurestr = NULL; + char **features = NULL; + size_t nfeatures = 0; + virResctrlInfoMongrpPtr info_monitor = NULL; + + if (VIR_ALLOC(info_monitor) < 0) + return -1; + + /* monitor only exists in leve 3 cache */
*level Let's say, "For now, monitor only exists in level 3 cache"
+ info_monitor->cache_level = 3;
So I think in the last review I was thinking that if we ever see a different level, then the L3 below would need to change. Although for now I wonder if it should be removed. I'll leave it unless you really think it should be removed. I think I was being ultra careful/paranoid. Perhaps the future is GetMonitorInfo takes in the cache_level as a parameter in order to build up the path and save the level in a structure. I think to a degree we're good to have that level of indirection with these latest changes. I don't mind removing it completely from this pile, but don't want to mess you up for the future either!
+ + rv = virFileReadValueUint(&info_monitor->max_monitor, + SYSFS_RESCTRL_PATH "/info/L3_MON/num_rmids"); + if (rv == -2) { + /* The file doesn't exist, so it's unusable for us, probably resource + * monitor unsupported */ + VIR_INFO("The path '" SYSFS_RESCTRL_PATH "/info/L3_MON/num_rmids' " + "does not exist");
Add virResetLastError() [avoids having this error in Last and something else failing and spewing the error]
+ ret = 0; + goto cleanup; + } else if (rv < 0) { + /* Other failures are fatal, so just quit */ + goto cleanup; + } + + rv = virFileReadValueUint(&info_monitor->cache_reuse_threshold, + SYSFS_RESCTRL_PATH + "/info/L3_MON/max_threshold_occupancy"); + if (rv == -2) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot get max_threshold_occupancy from resctrl" + " info"));
Typically the extra space is on the previous line. I'm just going to remove the " info" completely... Also adding ' ' around the variable name searched on. Something I'll repeat for subsequent messages.
+ } + if (rv < 0) + goto cleanup; + + rv = virFileReadValueString(&featurestr, + SYSFS_RESCTRL_PATH + "/info/L3_MON/mon_features"); + if (rv == -2) + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot get mon_features from resctrl info")); + if (rv < 0) + goto cleanup; + + if (!featurestr) {
I don't think !featurestr could happen as a result of the following in virFileReadLimFD (which is is in the call path): s = saferead_lim(fd, maxlen+1, &len); if (s == NULL) return -1; ... *buf = s; return len; Thus if s = NULL, then we return -1; otherwise, *buf = s or @featurestr = s, meaning no chance NULL is set without -1 being returned. I think you meant: "if (!*featurestr)" considering the subsequent lines...
+ /* if no feature found in "/info/L3_MON/mon_features", + * some error happens */ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot get feature list from resctrl info"));
Thus this is "Found empty feature list from resctrl"
+ ret = -1;
This is unnecessary since @ret == -1 at this point... @rv could be 0, but we don't care. FWIW: one too many spaces between ret and =. I can clean all this up - just let me know that the !*featurestr check is what you after.... Reviewed-by: John Ferlan <jferlan@redhat.com> John
+ goto cleanup; + } + + features = virStringSplitCount(featurestr, "\n", 0, &nfeatures); + VIR_DEBUG("Resctrl supported %ld monitoring features", nfeatures); + + info_monitor->nfeatures = nfeatures; + VIR_STEAL_PTR(info_monitor->features, features); + VIR_STEAL_PTR(resctrl->monitor_info, info_monitor); + + ret = 0; + cleanup: + VIR_FREE(featurestr); + virStringListFree(features); + VIR_FREE(info_monitor); + return ret; +} + + static int virResctrlGetInfo(virResctrlInfoPtr resctrl) { @@ -573,6 +690,10 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl) if (ret < 0) goto cleanup;
+ ret = virResctrlGetMonitorInfo(resctrl); + if (ret < 0) + goto cleanup; + ret = 0; cleanup: VIR_DIR_CLOSE(dirp); @@ -613,6 +734,9 @@ virResctrlInfoIsEmpty(virResctrlInfoPtr resctrl) if (resctrl->membw_info) return false;
+ if (resctrl->monitor_info) + return false; + for (i = 0; i < resctrl->nlevels; i++) { virResctrlInfoPerLevelPtr i_level = resctrl->levels[i];
-----Original Message----- From: John Ferlan [mailto:jferlan@redhat.com] Sent: Wednesday, September 19, 2018 3:39 AM To: Wang, Huaqiang <huaqiang.wang@intel.com>; libvir-list@redhat.com Cc: Feng, Shaohe <shaohe.feng@intel.com>; Niu, Bing <bing.niu@intel.com>; Ding, Jian-feng <jian-feng.ding@intel.com>; Zang, Rui <rui.zang@intel.com> Subject: Re: [PATCHv2 1/4] util: Introduce monitor capability interface
On 09/14/2018 09:30 PM, Wang Huaqiang wrote:
This patch introduces the resource monitor and creates the interface for getting host capability of resource monitor from the system resource control file system.
The resource monitor take the role of RDT monitoring group, could be
*takes...
s/, could/ and could/
used to monitor the resource consumption information, such as the last level cache occupancy and the utilization of memory bandwidth.
Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com> --- src/util/virresctrl.c | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 124 insertions(+)
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 4b5442f..4601f69 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -83,6 +83,9 @@ typedef virResctrlInfoPerLevel *virResctrlInfoPerLevelPtr; typedef struct _virResctrlInfoMemBW virResctrlInfoMemBW; typedef virResctrlInfoMemBW *virResctrlInfoMemBWPtr;
+typedef struct _virResctrlInfoMongrp virResctrlInfoMongrp; typedef +virResctrlInfoMongrp *virResctrlInfoMongrpPtr; + typedef struct _virResctrlAllocPerType virResctrlAllocPerType; typedef virResctrlAllocPerType *virResctrlAllocPerTypePtr;
@@ -139,6 +142,28 @@ struct _virResctrlInfoMemBW { unsigned int max_id; };
+struct _virResctrlInfoMongrp { + /* 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; + + /* Last level cache related information */ + + /* 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; +}; + struct _virResctrlInfo { virObject parent;
@@ -146,6 +171,8 @@ struct _virResctrlInfo { size_t nlevels;
virResctrlInfoMemBWPtr membw_info; + + virResctrlInfoMongrpPtr monitor_info; };
@@ -171,8 +198,12 @@ virResctrlInfoDispose(void *obj) VIR_FREE(level); }
+ if (resctrl->monitor_info) + VIR_FREE(resctrl->monitor_info->features);
virStringListFree
+ VIR_FREE(resctrl->membw_info); VIR_FREE(resctrl->levels); + VIR_FREE(resctrl->monitor_info); }
@@ -555,6 +586,92 @@ virResctrlGetMemoryBandwidthInfo(virResctrlInfoPtr resctrl) }
+/* + * Retrieve monitor capability from the resource control file system. + * + * The monitor capability is exposed through "SYSFS_RESCTRL_PATH/info/L3_MON" + * directory under the resource control file system. The monitor +capability is + * parsed by reading the interface files and stored in the structure + * 'virResctrlInfoMongrp'. + * + * Not all host supports the resource monitor, leave the pointer + * @resctrl->monitor_info empty if not supported. + */ +static int +virResctrlGetMonitorInfo(virResctrlInfoPtr resctrl) { + int ret = -1; + int rv = -1; + char *featurestr = NULL; + char **features = NULL; + size_t nfeatures = 0; + virResctrlInfoMongrpPtr info_monitor = NULL; + + if (VIR_ALLOC(info_monitor) < 0) + return -1; + + /* monitor only exists in leve 3 cache */
*level
Let's say, "For now, monitor only exists in level 3 cache"
+ info_monitor->cache_level = 3;
So I think in the last review I was thinking that if we ever see a different level, then the L3 below would need to change. Although for now I wonder if it should be removed. I'll leave it unless you really think it should be removed. I think I was being ultra careful/paranoid.
Perhaps the future is GetMonitorInfo takes in the cache_level as a parameter in order to build up the path and save the level in a structure. I think to a degree we're good to have that level of indirection with these latest changes. I don't mind removing it completely from this pile, but don't want to mess you up for the future either!
I am happy for the change you proposed in last review. Let's keep it here.
+ + rv = virFileReadValueUint(&info_monitor->max_monitor, + SYSFS_RESCTRL_PATH "/info/L3_MON/num_rmids"); + if (rv == -2) { + /* The file doesn't exist, so it's unusable for us, probably resource + * monitor unsupported */ + VIR_INFO("The path '" SYSFS_RESCTRL_PATH "/info/L3_MON/num_rmids' " + "does not exist");
Add virResetLastError()
[avoids having this error in Last and something else failing and spewing the error]
+ ret = 0; + goto cleanup; + } else if (rv < 0) { + /* Other failures are fatal, so just quit */ + goto cleanup; + } + + rv = virFileReadValueUint(&info_monitor->cache_reuse_threshold, + SYSFS_RESCTRL_PATH + "/info/L3_MON/max_threshold_occupancy"); + if (rv == -2) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot get max_threshold_occupancy from resctrl" + " info"));
Typically the extra space is on the previous line. I'm just going to remove the " info" completely... Also adding ' ' around the variable name searched on. Something I'll repeat for subsequent messages.
+ } + if (rv < 0) + goto cleanup; + + rv = virFileReadValueString(&featurestr, + SYSFS_RESCTRL_PATH + "/info/L3_MON/mon_features"); + if (rv == -2) + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot get mon_features from resctrl info")); + if (rv < 0) + goto cleanup; + + if (!featurestr) {
I don't think !featurestr could happen as a result of the following in virFileReadLimFD (which is is in the call path):
s = saferead_lim(fd, maxlen+1, &len); if (s == NULL) return -1; ... *buf = s; return len;
Agree with your judgement. If should use ' if (!*featurestr)'. I double confirmed through go through the code as you did and through testing. Here, if file 'mon_feautres' exists but with empty content, a buffer will be assigned to 'featurestr' but the first byte will be a '\0' char.
Thus if s = NULL, then we return -1; otherwise, *buf = s or @featurestr = s, meaning no chance NULL is set without -1 being returned.
I think you meant:
"if (!*featurestr)"
Yes.
considering the subsequent lines...
+ /* if no feature found in "/info/L3_MON/mon_features", + * some error happens */ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot get feature list from resctrl + info"));
Thus this is "Found empty feature list from resctrl"
+ ret = -1;
This is unnecessary since @ret == -1 at this point... @rv could be 0, but we don't care. FWIW: one too many spaces between ret and =.
I don't think so. If code go here, the '/sys/fs/resctrl/info/L3_MON' directory is existed in system, and further, the 'num_rmids' file is checked that is existed in system, then some resctrl monitoring feature must be supported, either CMT or MBM. So here the 'mon_features' file shouldn’t be empty. If it is empty, some error happens, report it.
I can clean all this up - just let me know that the !*featurestr check is what you after....
And I also find a bug that I involved. The fix is shown in following lines. Since CMT is an independent feature to MBM, some system might not support CMT while MBM is supported. File 'max_threshold_occupancy' is created if CMT is supported, if this file does not exist, only means CMT might not be supported. We shouldn't look this as an error. @@ -632,11 +632,13 @@ virResctrlGetMonitorInfo(virResctrlInfoPtr resctrl) SYSFS_RESCTRL_PATH "/info/L3_MON/max_threshold_occupancy"); if (rv == -2) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Cannot get max_threshold_occupancy from resctrl")); - } - if (rv < 0) + /* If CMT is not supported, then 'max_threshold_occupancy' file + * will not exist. */ + VIR_INFO("File '" SYSFS_RESCTRL_PATH + "/info/L3_MON/max_threshold_occupancy' does not exist"); + } else if (rv < 0) { goto cleanup; + } rv = virFileReadValueString(&featurestr, SYSFS_RESCTRL_PATH I changed my code according your review opinions , and also, added the following review message line to my next revised patch version, I don't know if is proper to do that. Thanks for your review. Huaqiang
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
+ goto cleanup; + } + + features = virStringSplitCount(featurestr, "\n", 0, &nfeatures); + VIR_DEBUG("Resctrl supported %ld monitoring features", + nfeatures); + + info_monitor->nfeatures = nfeatures; + VIR_STEAL_PTR(info_monitor->features, features); + VIR_STEAL_PTR(resctrl->monitor_info, info_monitor); + + ret = 0; + cleanup: + VIR_FREE(featurestr); + virStringListFree(features); + VIR_FREE(info_monitor); + return ret; +} + + static int virResctrlGetInfo(virResctrlInfoPtr resctrl) { @@ -573,6 +690,10 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl) if (ret < 0) goto cleanup;
+ ret = virResctrlGetMonitorInfo(resctrl); + if (ret < 0) + goto cleanup; + ret = 0; cleanup: VIR_DIR_CLOSE(dirp); @@ -613,6 +734,9 @@ virResctrlInfoIsEmpty(virResctrlInfoPtr resctrl) if (resctrl->membw_info) return false;
+ if (resctrl->monitor_info) + return false; + for (i = 0; i < resctrl->nlevels; i++) { virResctrlInfoPerLevelPtr i_level = resctrl->levels[i];
On Tue, Sep 18, 2018 at 03:38:44PM -0400, John Ferlan wrote:
On 09/14/2018 09:30 PM, Wang Huaqiang wrote:
This patch introduces the resource monitor and creates the interface for getting host capability of resource monitor from the system resource control file system.
The resource monitor take the role of RDT monitoring group, could be
*takes...
s/, could/ and could/
used to monitor the resource consumption information, such as the last level cache occupancy and the utilization of memory bandwidth.
Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com> --- src/util/virresctrl.c | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 124 insertions(+)
[...]
+ + rv = virFileReadValueUint(&info_monitor->max_monitor, + SYSFS_RESCTRL_PATH "/info/L3_MON/num_rmids"); + if (rv == -2) { + /* The file doesn't exist, so it's unusable for us, probably resource + * monitor unsupported */ + VIR_INFO("The path '" SYSFS_RESCTRL_PATH "/info/L3_MON/num_rmids' " + "does not exist");
Add virResetLastError()
[avoids having this error in Last and something else failing and spewing the error]
The return value of -2 means no error was set, so there is nothing to do here. Also, virResetLastError is meant to be used before starting an API. It only resets the thread-local error object (which can only contain one error), it cannot possibly unlog an error that was logged earlier. In that case, creating a Quiet version of the function is the proper solution. Jano
+ ret = 0; + goto cleanup;
-----Original Message----- From: Ján Tomko [mailto:jtomko@redhat.com] Sent: Friday, October 5, 2018 10:42 PM To: John Ferlan <jferlan@redhat.com> Cc: Wang, Huaqiang <huaqiang.wang@intel.com>; libvir-list@redhat.com; Feng, Shaohe <shaohe.feng@intel.com>; Niu, Bing <bing.niu@intel.com>; Ding, Jian- feng <jian-feng.ding@intel.com>; Zang, Rui <rui.zang@intel.com> Subject: Re: [libvirt] [PATCHv2 1/4] util: Introduce monitor capability interface
On Tue, Sep 18, 2018 at 03:38:44PM -0400, John Ferlan wrote:
On 09/14/2018 09:30 PM, Wang Huaqiang wrote:
This patch introduces the resource monitor and creates the interface for getting host capability of resource monitor from the system resource control file system.
The resource monitor take the role of RDT monitoring group, could be
*takes...
s/, could/ and could/
used to monitor the resource consumption information, such as the last level cache occupancy and the utilization of memory bandwidth.
Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com> --- src/util/virresctrl.c | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 124 insertions(+)
[...]
+ + rv = virFileReadValueUint(&info_monitor->max_monitor, + SYSFS_RESCTRL_PATH "/info/L3_MON/num_rmids"); + if (rv == -2) { + /* The file doesn't exist, so it's unusable for us, probably resource + * monitor unsupported */ + VIR_INFO("The path '" SYSFS_RESCTRL_PATH "/info/L3_MON/num_rmids' " + "does not exist");
Add virResetLastError()
[avoids having this error in Last and something else failing and spewing the error]
The return value of -2 means no error was set, so there is nothing to do here.
A return value of -2 means no error, rather than executing remaining part of function, it requires to return to the caller without reporting any error. Here, a return value of -2 means "/info/L3_MON/num_rmids" is not exists, this could happen if CMT is not supported by host. This is a valid scenario and does not mean an error, and this function should not report any error to its caller, and the caller, which is virResctrlGetInfo, will continue to run its remaining statements normally.
Also, virResetLastError is meant to be used before starting an API. It only resets the thread-local error object (which can only contain one error), it cannot possibly unlog an error that was logged earlier. In that case, creating a Quiet version of the function is the proper solution.
Do you mean the message reported by 'VIR_INFO' should be removed and also not adding the 'virResetLastError' line? It might be more consistent if we keep the 'VIR_INFO' lines, because the similar message has been emitted in checking the memory bandwidth information and cache allocation information. But if you insist that this message should not be shown to user or developer, I could accept that and make change to it. @@ -704,12 +704,7 @@ virResctrlGetMonitorInfo(virResctrlInfoPtr resctrl) rv = virFileReadValueUint(&info_monitor->max_monitor, SYSFS_RESCTRL_PATH "/info/L3_MON/num_rmids"); if (rv == -2) { /* The file doesn't exist, so it's unusable for us, probably resource * monitor unsupported */ VIR_INFO("The file '" SYSFS_RESCTRL_PATH "/info/L3_MON/num_rmids' " "does not exist"); ret = 0; - virResetLastError(); goto cleanup; } else if (rv < 0) { /* Other failures are fatal, so just quit */
Jano
+ ret = 0; + goto cleanup;
Hi Jano, Recognized there was already a commit for a fixing: 7bff646d71aa90ed8727ef99be29d6d2ab5d8f06. And now I got your idea. Thanks Huaqiang
-----Original Message----- From: Wang, Huaqiang Sent: Tuesday, October 9, 2018 5:55 PM To: 'Ján Tomko' <jtomko@redhat.com>; John Ferlan <jferlan@redhat.com> Cc: libvir-list@redhat.com; Feng, Shaohe <shaohe.feng@intel.com>; Niu, Bing <bing.niu@intel.com>; Ding, Jian-feng <jian-feng.ding@intel.com>; Zang, Rui <rui.zang@intel.com> Subject: RE: [libvirt] [PATCHv2 1/4] util: Introduce monitor capability interface
-----Original Message----- From: Ján Tomko [mailto:jtomko@redhat.com] Sent: Friday, October 5, 2018 10:42 PM To: John Ferlan <jferlan@redhat.com> Cc: Wang, Huaqiang <huaqiang.wang@intel.com>; libvir-list@redhat.com; Feng, Shaohe <shaohe.feng@intel.com>; Niu, Bing <bing.niu@intel.com>; Ding, Jian- feng <jian-feng.ding@intel.com>; Zang, Rui <rui.zang@intel.com> Subject: Re: [libvirt] [PATCHv2 1/4] util: Introduce monitor capability interface
On Tue, Sep 18, 2018 at 03:38:44PM -0400, John Ferlan wrote:
On 09/14/2018 09:30 PM, Wang Huaqiang wrote:
This patch introduces the resource monitor and creates the interface for getting host capability of resource monitor from the system resource control file system.
The resource monitor take the role of RDT monitoring group, could be
*takes...
s/, could/ and could/
used to monitor the resource consumption information, such as the last level cache occupancy and the utilization of memory bandwidth.
Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com> --- src/util/virresctrl.c | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 124 insertions(+)
[...]
+ + rv = virFileReadValueUint(&info_monitor->max_monitor, + SYSFS_RESCTRL_PATH "/info/L3_MON/num_rmids"); + if (rv == -2) { + /* The file doesn't exist, so it's unusable for us, probably resource + * monitor unsupported */ + VIR_INFO("The path '" SYSFS_RESCTRL_PATH "/info/L3_MON/num_rmids' " + "does not exist");
Add virResetLastError()
[avoids having this error in Last and something else failing and spewing the error]
The return value of -2 means no error was set, so there is nothing to do here.
A return value of -2 means no error, rather than executing remaining part of function, it requires to return to the caller without reporting any error.
Here, a return value of -2 means "/info/L3_MON/num_rmids" is not exists, this could happen if CMT is not supported by host. This is a valid scenario and does not mean an error, and this function should not report any error to its caller, and the caller, which is virResctrlGetInfo, will continue to run its remaining statements normally.
Also, virResetLastError is meant to be used before starting an API. It only resets the thread-local error object (which can only contain one error), it cannot possibly unlog an error that was logged earlier. In that case, creating a Quiet version of the function is the proper solution.
Do you mean the message reported by 'VIR_INFO' should be removed and also not adding the 'virResetLastError' line?
It might be more consistent if we keep the 'VIR_INFO' lines, because the similar message has been emitted in checking the memory bandwidth information and cache allocation information. But if you insist that this message should not be shown to user or developer, I could accept that and make change to it.
@@ -704,12 +704,7 @@ virResctrlGetMonitorInfo(virResctrlInfoPtr resctrl) rv = virFileReadValueUint(&info_monitor->max_monitor, SYSFS_RESCTRL_PATH "/info/L3_MON/num_rmids"); if (rv == -2) { /* The file doesn't exist, so it's unusable for us, probably resource * monitor unsupported */ VIR_INFO("The file '" SYSFS_RESCTRL_PATH "/info/L3_MON/num_rmids' " "does not exist"); ret = 0; - virResetLastError(); goto cleanup; } else if (rv < 0) { /* Other failures are fatal, so just quit */
Jano
+ ret = 0; + goto cleanup;
Move all cache banks into one data structure, this allows us to add other cache component, such as cache monitor. Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com> --- src/conf/capabilities.c | 36 +++++++++++++++++------------------- src/conf/capabilities.h | 10 ++++++++-- 2 files changed, 25 insertions(+), 21 deletions(-) diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 326bd15..9149e10 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -245,9 +245,9 @@ virCapsDispose(void *object) virCapabilitiesClearSecModel(&caps->host.secModels[i]); VIR_FREE(caps->host.secModels); - for (i = 0; i < caps->host.ncaches; i++) - virCapsHostCacheBankFree(caps->host.caches[i]); - VIR_FREE(caps->host.caches); + for (i = 0; i < caps->host.cache.nbanks; i++) + virCapsHostCacheBankFree(caps->host.cache.banks[i]); + VIR_FREE(caps->host.cache.banks); for (i = 0; i < caps->host.nnodes; i++) virCapsHostMemBWNodeFree(caps->host.nodes[i]); @@ -868,21 +868,20 @@ virCapabilitiesFormatNUMATopology(virBufferPtr buf, static int virCapabilitiesFormatCaches(virBufferPtr buf, - size_t ncaches, - virCapsHostCacheBankPtr *caches) + virCapsHostCachePtr cache) { size_t i = 0; size_t j = 0; virBuffer childrenBuf = VIR_BUFFER_INITIALIZER; - if (!ncaches) + if (!cache->nbanks) return 0; virBufferAddLit(buf, "<cache>\n"); virBufferAdjustIndent(buf, 2); - for (i = 0; i < ncaches; i++) { - virCapsHostCacheBankPtr bank = caches[i]; + for (i = 0; i < cache->nbanks; i++) { + virCapsHostCacheBankPtr bank = cache->banks[i]; char *cpus_str = virBitmapFormat(bank->cpus); const char *unit = NULL; unsigned long long short_size = virFormatIntPretty(bank->size, &unit); @@ -1111,8 +1110,7 @@ virCapabilitiesFormatXML(virCapsPtr caps) caps->host.numaCell) < 0) goto error; - if (virCapabilitiesFormatCaches(&buf, caps->host.ncaches, - caps->host.caches) < 0) + if (virCapabilitiesFormatCaches(&buf, &caps->host.cache) < 0) goto error; if (virCapabilitiesFormatMemoryBandwidth(&buf, caps->host.nnodes, @@ -1668,8 +1666,8 @@ virCapabilitiesInitResctrlMemory(virCapsPtr caps) size_t i = 0; int ret = -1; - for (i = 0; i < caps->host.ncaches; i++) { - virCapsHostCacheBankPtr bank = caps->host.caches[i]; + for (i = 0; i < caps->host.cache.nbanks; i++) { + virCapsHostCacheBankPtr bank = caps->host.cache.banks[i]; if (VIR_ALLOC(node) < 0) goto cleanup; @@ -1791,11 +1789,11 @@ virCapabilitiesInitCaches(virCapsPtr caps) bank->type = kernel_type; VIR_FREE(type); - for (i = 0; i < caps->host.ncaches; i++) { - if (virCapsHostCacheBankEquals(bank, caps->host.caches[i])) + for (i = 0; i < caps->host.cache.nbanks; i++) { + if (virCapsHostCacheBankEquals(bank, caps->host.cache.banks[i])) break; } - if (i == caps->host.ncaches) { + if (i == caps->host.cache.nbanks) { /* If it is a new cache, then update its resctrl information. */ if (virResctrlInfoGetCache(caps->host.resctrl, bank->level, @@ -1804,8 +1802,8 @@ virCapabilitiesInitCaches(virCapsPtr caps) &bank->controls) < 0) goto cleanup; - if (VIR_APPEND_ELEMENT(caps->host.caches, - caps->host.ncaches, + if (VIR_APPEND_ELEMENT(caps->host.cache.banks, + caps->host.cache.nbanks, bank) < 0) { goto cleanup; } @@ -1821,8 +1819,8 @@ virCapabilitiesInitCaches(virCapsPtr caps) /* Sort the array in order for the tests to be predictable. This way we can * still traverse the directory instead of guessing names (in case there is * 'index1' and 'index3' but no 'index2'). */ - qsort(caps->host.caches, caps->host.ncaches, - sizeof(*caps->host.caches), virCapsHostCacheBankSorter); + qsort(caps->host.cache.banks, caps->host.cache.nbanks, + sizeof(*caps->host.cache.banks), virCapsHostCacheBankSorter); if (virCapabilitiesInitResctrlMemory(caps) < 0) goto cleanup; diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h index 046e275..744074b 100644 --- a/src/conf/capabilities.h +++ b/src/conf/capabilities.h @@ -151,6 +151,13 @@ struct _virCapsHostCacheBank { virResctrlInfoPerCachePtr *controls; }; +typedef struct _virCapsHostCache virCapsHostCache; +typedef virCapsHostCache *virCapsHostCachePtr; +struct _virCapsHostCache { + size_t nbanks; + virCapsHostCacheBankPtr *banks; +}; + typedef struct _virCapsHostMemBWNode virCapsHostMemBWNode; typedef virCapsHostMemBWNode *virCapsHostMemBWNodePtr; struct _virCapsHostMemBWNode { @@ -180,8 +187,7 @@ struct _virCapsHost { virResctrlInfoPtr resctrl; - size_t ncaches; - virCapsHostCacheBankPtr *caches; + virCapsHostCache cache; size_t nnodes; virCapsHostMemBWNodePtr *nodes; -- 2.7.4
On 09/14/2018 09:30 PM, Wang Huaqiang wrote:
Move all cache banks into one data structure, this allows us to add other cache component, such as cache monitor.
Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com> --- src/conf/capabilities.c | 36 +++++++++++++++++------------------- src/conf/capabilities.h | 10 ++++++++-- 2 files changed, 25 insertions(+), 21 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Move memory bandwidth capability nodes into one data structure, this allows us to add a monitor for memory bandwidth. Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com> --- src/conf/capabilities.c | 22 ++++++++++------------ src/conf/capabilities.h | 10 ++++++++-- 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 9149e10..66ad420 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -249,9 +249,9 @@ virCapsDispose(void *object) virCapsHostCacheBankFree(caps->host.cache.banks[i]); VIR_FREE(caps->host.cache.banks); - for (i = 0; i < caps->host.nnodes; i++) - virCapsHostMemBWNodeFree(caps->host.nodes[i]); - VIR_FREE(caps->host.nodes); + for (i = 0; i < caps->host.memBW.nnodes; i++) + virCapsHostMemBWNodeFree(caps->host.memBW.nodes[i]); + VIR_FREE(caps->host.memBW.nodes); VIR_FREE(caps->host.netprefix); VIR_FREE(caps->host.pagesSize); @@ -961,20 +961,19 @@ virCapabilitiesFormatCaches(virBufferPtr buf, static int virCapabilitiesFormatMemoryBandwidth(virBufferPtr buf, - size_t nnodes, - virCapsHostMemBWNodePtr *nodes) + virCapsHostMemBWPtr memBW) { size_t i = 0; virBuffer childrenBuf = VIR_BUFFER_INITIALIZER; - if (!nnodes) + if (!memBW->nnodes) return 0; virBufferAddLit(buf, "<memory_bandwidth>\n"); virBufferAdjustIndent(buf, 2); - for (i = 0; i < nnodes; i++) { - virCapsHostMemBWNodePtr node = nodes[i]; + for (i = 0; i < memBW->nnodes; i++) { + virCapsHostMemBWNodePtr node = memBW->nodes[i]; virResctrlInfoMemBWPerNodePtr control = &node->control; char *cpus_str = virBitmapFormat(node->cpus); @@ -1113,8 +1112,7 @@ virCapabilitiesFormatXML(virCapsPtr caps) if (virCapabilitiesFormatCaches(&buf, &caps->host.cache) < 0) goto error; - if (virCapabilitiesFormatMemoryBandwidth(&buf, caps->host.nnodes, - caps->host.nodes) < 0) + if (virCapabilitiesFormatMemoryBandwidth(&buf, &caps->host.memBW) < 0) goto error; for (i = 0; i < caps->host.nsecModels; i++) { @@ -1677,8 +1675,8 @@ virCapabilitiesInitResctrlMemory(virCapsPtr caps) if (!(node->cpus = virBitmapNewCopy(bank->cpus))) goto cleanup; - if (VIR_APPEND_ELEMENT(caps->host.nodes, - caps->host.nnodes, node) < 0) { + if (VIR_APPEND_ELEMENT(caps->host.memBW.nodes, + caps->host.memBW.nnodes, node) < 0) { goto cleanup; } } diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h index 744074b..694fd6b 100644 --- a/src/conf/capabilities.h +++ b/src/conf/capabilities.h @@ -166,6 +166,13 @@ struct _virCapsHostMemBWNode { virResctrlInfoMemBWPerNode control; }; +typedef struct _virCapsHostMemBW virCapsHostMemBW; +typedef virCapsHostMemBW *virCapsHostMemBWPtr; +struct _virCapsHostMemBW { + size_t nnodes; + virCapsHostMemBWNodePtr *nodes; +}; + typedef struct _virCapsHost virCapsHost; typedef virCapsHost *virCapsHostPtr; struct _virCapsHost { @@ -189,8 +196,7 @@ struct _virCapsHost { virCapsHostCache cache; - size_t nnodes; - virCapsHostMemBWNodePtr *nodes; + virCapsHostMemBW memBW; size_t nsecModels; virCapsHostSecModelPtr secModels; -- 2.7.4
On 09/14/2018 09:30 PM, Wang Huaqiang wrote:
Move memory bandwidth capability nodes into one data structure, this allows us to add a monitor for memory bandwidth.
Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com> --- src/conf/capabilities.c | 22 ++++++++++------------ src/conf/capabilities.h | 10 ++++++++-- 2 files changed, 18 insertions(+), 14 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
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. The host capability of the two monitors is also introduced in this patch. For cache monitor, the host capability is shown like: <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: <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@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> + <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, } 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 */ + if (monitor->nfeatures == 0) + return 0; + + 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); + + + for (i = 0; i < monitor->nfeatures; i++) { + virBufferSetChildIndent(&childrenBuf, buf); + 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) + return -1; + virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</cache>\n"); @@ -1004,6 +1054,10 @@ virCapabilitiesFormatMemoryBandwidth(virBufferPtr buf, } } + if (virCapabilitiesFormatResctrlMonitor(buf, + memBW->monitor) < 0) + return -1; + virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</memory_bandwidth>\n"); @@ -1664,6 +1718,8 @@ virCapabilitiesInitResctrlMemory(virCapsPtr caps) size_t i = 0; int ret = -1; + 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, + &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; + 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, + &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") + +/* Monitor feature name prefix mapping for monitor naming */ +VIR_ENUM_IMPL(virMonitorPrefix, VIR_MONITOR_TYPE_LAST, + "__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); 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 + * monitor with prefix name @prefex is supported by host. + * + * Return monitor capability information describe in prefix name @prefix + * through @monitor + * + * Returns 0 if a monitor is found or a valid monitor 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; + } + + for (i = 0; i < VIR_MONITOR_TYPE_LAST; i++) { + if (STREQ(prefix, virMonitorPrefixTypeToString(i))) { + 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"), + prefix); + goto cleanup; + } + + 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; + } + + 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)); + goto cleanup; + } + + /* In case *monitor is pointed to some monitor, clean it. */ + VIR_FREE(*monitor); + + 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); +typedef enum { + VIR_MONITOR_TYPE_UNSUPPORT, + VIR_MONITOR_TYPE_CACHE, + VIR_MONITOR_TYPE_MEMBW, + + VIR_MONITOR_TYPE_LAST +} virMonitorType; +VIR_ENUM_DECL(virMonitor); +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; + + /* 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. */ + + /* 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. */ + + /* MBM related field is empty */ +}; + 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); #endif /* __VIR_RESCTRL_H__ */ diff --git a/tests/vircaps2xmldata/linux-resctrl-cmt/resctrl/info/L3_MON/max_threshold_occupancy b/tests/vircaps2xmldata/linux-resctrl-cmt/resctrl/info/L3_MON/max_threshold_occupancy new file mode 100644 index 0000000..77f05e2 --- /dev/null +++ b/tests/vircaps2xmldata/linux-resctrl-cmt/resctrl/info/L3_MON/max_threshold_occupancy @@ -0,0 +1 @@ +270336 diff --git a/tests/vircaps2xmldata/linux-resctrl-cmt/resctrl/info/L3_MON/mon_features b/tests/vircaps2xmldata/linux-resctrl-cmt/resctrl/info/L3_MON/mon_features new file mode 100644 index 0000000..8467d90 --- /dev/null +++ b/tests/vircaps2xmldata/linux-resctrl-cmt/resctrl/info/L3_MON/mon_features @@ -0,0 +1 @@ +llc_occupancy diff --git a/tests/vircaps2xmldata/linux-resctrl-cmt/resctrl/info/L3_MON/num_rmids b/tests/vircaps2xmldata/linux-resctrl-cmt/resctrl/info/L3_MON/num_rmids new file mode 100644 index 0000000..1057e9a --- /dev/null +++ b/tests/vircaps2xmldata/linux-resctrl-cmt/resctrl/info/L3_MON/num_rmids @@ -0,0 +1 @@ +176 diff --git a/tests/vircaps2xmldata/linux-resctrl-cmt/resctrl/manualres/cpus b/tests/vircaps2xmldata/linux-resctrl-cmt/resctrl/manualres/cpus new file mode 100644 index 0000000..8f087a3 --- /dev/null +++ b/tests/vircaps2xmldata/linux-resctrl-cmt/resctrl/manualres/cpus @@ -0,0 +1 @@ +000 diff --git a/tests/vircaps2xmldata/linux-resctrl-cmt/resctrl/manualres/schemata b/tests/vircaps2xmldata/linux-resctrl-cmt/resctrl/manualres/schemata new file mode 100644 index 0000000..e499ef7 --- /dev/null +++ b/tests/vircaps2xmldata/linux-resctrl-cmt/resctrl/manualres/schemata @@ -0,0 +1 @@ +L3:0=e0000;1=e0000 diff --git a/tests/vircaps2xmldata/linux-resctrl-cmt/resctrl/manualres/tasks b/tests/vircaps2xmldata/linux-resctrl-cmt/resctrl/manualres/tasks new file mode 100644 index 0000000..e69de29 diff --git a/tests/vircaps2xmldata/linux-resctrl-cmt/resctrl/schemata b/tests/vircaps2xmldata/linux-resctrl-cmt/resctrl/schemata new file mode 100644 index 0000000..78d2d8a --- /dev/null +++ b/tests/vircaps2xmldata/linux-resctrl-cmt/resctrl/schemata @@ -0,0 +1 @@ + L3:0=1ff00;1=1ff0f diff --git a/tests/vircaps2xmldata/linux-resctrl-cmt/system b/tests/vircaps2xmldata/linux-resctrl-cmt/system new file mode 120000 index 0000000..5607d59 --- /dev/null +++ b/tests/vircaps2xmldata/linux-resctrl-cmt/system @@ -0,0 +1 @@ +../linux-resctrl/system \ No newline at end of file diff --git a/tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/L3/cbm_mask b/tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/L3/cbm_mask new file mode 100644 index 0000000..78031da --- /dev/null +++ b/tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/L3/cbm_mask @@ -0,0 +1 @@ +fffff diff --git a/tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/L3/min_cbm_bits b/tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/L3/min_cbm_bits new file mode 100644 index 0000000..0cfbf08 --- /dev/null +++ b/tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/L3/min_cbm_bits @@ -0,0 +1 @@ +2 diff --git a/tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/L3/num_closids b/tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/L3/num_closids new file mode 100644 index 0000000..b8626c4 --- /dev/null +++ b/tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/L3/num_closids @@ -0,0 +1 @@ +4 diff --git a/tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/L3_MON/max_threshold_occupancy b/tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/L3_MON/max_threshold_occupancy new file mode 100644 index 0000000..77f05e2 --- /dev/null +++ b/tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/L3_MON/max_threshold_occupancy @@ -0,0 +1 @@ +270336 diff --git a/tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/L3_MON/mon_features b/tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/L3_MON/mon_features new file mode 100644 index 0000000..337cfa2 --- /dev/null +++ b/tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/L3_MON/mon_features @@ -0,0 +1,10 @@ +llc_occupancy +mbm_total_bytes +mbm_local_bytes +llc_new_feature +llc_unknown_feature +mbm_new_feature +mbm_unknown_feature +ukn_feature +fak_feature +fake_unknown_feature diff --git a/tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/L3_MON/num_rmids b/tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/L3_MON/num_rmids new file mode 100644 index 0000000..1057e9a --- /dev/null +++ b/tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/L3_MON/num_rmids @@ -0,0 +1 @@ +176 diff --git a/tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/MB/bandwidth_gran b/tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/MB/bandwidth_gran new file mode 100644 index 0000000..f599e28 --- /dev/null +++ b/tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/MB/bandwidth_gran @@ -0,0 +1 @@ +10 diff --git a/tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/MB/min_bandwidth b/tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/MB/min_bandwidth new file mode 100644 index 0000000..f599e28 --- /dev/null +++ b/tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/MB/min_bandwidth @@ -0,0 +1 @@ +10 diff --git a/tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/MB/num_closids b/tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/MB/num_closids new file mode 100644 index 0000000..b8626c4 --- /dev/null +++ b/tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/MB/num_closids @@ -0,0 +1 @@ +4 diff --git a/tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/manualres/cpus b/tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/manualres/cpus new file mode 100644 index 0000000..8f087a3 --- /dev/null +++ b/tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/manualres/cpus @@ -0,0 +1 @@ +000 diff --git a/tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/manualres/schemata b/tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/manualres/schemata new file mode 100644 index 0000000..e499ef7 --- /dev/null +++ b/tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/manualres/schemata @@ -0,0 +1 @@ +L3:0=e0000;1=e0000 diff --git a/tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/manualres/tasks b/tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/manualres/tasks new file mode 100644 index 0000000..e69de29 diff --git a/tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/schemata b/tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/schemata new file mode 100644 index 0000000..78d2d8a --- /dev/null +++ b/tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/schemata @@ -0,0 +1 @@ + L3:0=1ff00;1=1ff0f diff --git a/tests/vircaps2xmldata/linux-resctrl-fake-feature/system b/tests/vircaps2xmldata/linux-resctrl-fake-feature/system new file mode 120000 index 0000000..5607d59 --- /dev/null +++ b/tests/vircaps2xmldata/linux-resctrl-fake-feature/system @@ -0,0 +1 @@ +../linux-resctrl/system \ No newline at end of file diff --git a/tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3_MON/max_threshold_occupancy b/tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3_MON/max_threshold_occupancy new file mode 100644 index 0000000..77f05e2 --- /dev/null +++ b/tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3_MON/max_threshold_occupancy @@ -0,0 +1 @@ +270336 diff --git a/tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3_MON/mon_features b/tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3_MON/mon_features new file mode 100644 index 0000000..0c57b8d --- /dev/null +++ b/tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3_MON/mon_features @@ -0,0 +1,3 @@ +llc_occupancy +mbm_total_bytes +mbm_local_bytes diff --git a/tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3_MON/num_rmids b/tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3_MON/num_rmids new file mode 100644 index 0000000..1057e9a --- /dev/null +++ b/tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3_MON/num_rmids @@ -0,0 +1 @@ +176 diff --git a/tests/vircaps2xmldata/vircaps-x86_64-resctrl-cmt.xml b/tests/vircaps2xmldata/vircaps-x86_64-resctrl-cmt.xml new file mode 100644 index 0000000..6a8cd0e --- /dev/null +++ b/tests/vircaps2xmldata/vircaps-x86_64-resctrl-cmt.xml @@ -0,0 +1,53 @@ +<capabilities> + + <host> + <cpu> + <arch>x86_64</arch> + </cpu> + <power_management/> + <iommu support='no'/> + <migration_features> + <live/> + </migration_features> + <topology> + <cells num='2'> + <cell id='0'> + <memory unit='KiB'>1048576</memory> + <pages unit='KiB' size='4'>2048</pages> + <pages unit='KiB' size='2048'>4096</pages> + <pages unit='KiB' size='1048576'>6144</pages> + <cpus num='6'> + <cpu id='0' socket_id='0' core_id='0' siblings='0'/> + <cpu id='1' socket_id='0' core_id='1' siblings='1'/> + <cpu id='2' socket_id='0' core_id='2' siblings='2'/> + <cpu id='3' socket_id='0' core_id='3' siblings='3'/> + <cpu id='4' socket_id='0' core_id='4' siblings='4'/> + <cpu id='5' socket_id='0' core_id='5' siblings='5'/> + </cpus> + </cell> + <cell id='1'> + <memory unit='KiB'>2097152</memory> + <pages unit='KiB' size='4'>4096</pages> + <pages unit='KiB' size='2048'>6144</pages> + <pages unit='KiB' size='1048576'>8192</pages> + <cpus num='6'> + <cpu id='6' socket_id='1' core_id='0' siblings='6'/> + <cpu id='7' socket_id='1' core_id='1' siblings='7'/> + <cpu id='8' socket_id='1' core_id='2' siblings='8'/> + <cpu id='9' socket_id='1' core_id='3' siblings='9'/> + <cpu id='10' socket_id='1' core_id='4' siblings='10'/> + <cpu id='11' socket_id='1' core_id='5' siblings='11'/> + </cpus> + </cell> + </cells> + </topology> + <cache> + <bank id='0' level='3' type='both' size='15' unit='MiB' cpus='0-5'/> + <bank id='1' level='3' type='both' size='15' unit='MiB' cpus='6-11'/> + <monitor level='3' reuseThreshold='270336' maxMonitors='176'> + <feature name='llc_occupancy'/> + </monitor> + </cache> + </host> + +</capabilities> diff --git a/tests/vircaps2xmldata/vircaps-x86_64-resctrl-fake-feature.xml b/tests/vircaps2xmldata/vircaps-x86_64-resctrl-fake-feature.xml new file mode 100644 index 0000000..4e46ead --- /dev/null +++ b/tests/vircaps2xmldata/vircaps-x86_64-resctrl-fake-feature.xml @@ -0,0 +1,73 @@ +<capabilities> + + <host> + <cpu> + <arch>x86_64</arch> + </cpu> + <power_management/> + <iommu support='no'/> + <migration_features> + <live/> + </migration_features> + <topology> + <cells num='2'> + <cell id='0'> + <memory unit='KiB'>1048576</memory> + <pages unit='KiB' size='4'>2048</pages> + <pages unit='KiB' size='2048'>4096</pages> + <pages unit='KiB' size='1048576'>6144</pages> + <cpus num='6'> + <cpu id='0' socket_id='0' core_id='0' siblings='0'/> + <cpu id='1' socket_id='0' core_id='1' siblings='1'/> + <cpu id='2' socket_id='0' core_id='2' siblings='2'/> + <cpu id='3' socket_id='0' core_id='3' siblings='3'/> + <cpu id='4' socket_id='0' core_id='4' siblings='4'/> + <cpu id='5' socket_id='0' core_id='5' siblings='5'/> + </cpus> + </cell> + <cell id='1'> + <memory unit='KiB'>2097152</memory> + <pages unit='KiB' size='4'>4096</pages> + <pages unit='KiB' size='2048'>6144</pages> + <pages unit='KiB' size='1048576'>8192</pages> + <cpus num='6'> + <cpu id='6' socket_id='1' core_id='0' siblings='6'/> + <cpu id='7' socket_id='1' core_id='1' siblings='7'/> + <cpu id='8' socket_id='1' core_id='2' siblings='8'/> + <cpu id='9' socket_id='1' core_id='3' siblings='9'/> + <cpu id='10' socket_id='1' core_id='4' siblings='10'/> + <cpu id='11' socket_id='1' core_id='5' siblings='11'/> + </cpus> + </cell> + </cells> + </topology> + <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> + <bank id='1' level='3' type='both' size='15' unit='MiB' cpus='6-11'> + <control granularity='768' min='1536' unit='KiB' type='both' maxAllocs='4'/> + </bank> + <monitor level='3' reuseThreshold='270336' maxMonitors='176'> + <feature name='llc_occupancy'/> + <feature name='llc_new_feature'/> + <feature name='llc_unknown_feature'/> + </monitor> + </cache> + <memory_bandwidth> + <node id='0' cpus='0-5'> + <control granularity='10' min ='10' maxAllocs='4'/> + </node> + <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'/> + <feature name='mbm_new_feature'/> + <feature name='mbm_unknown_feature'/> + </monitor> + </memory_bandwidth> + </host> + +</capabilities> diff --git a/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml b/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml index 9b00cf0..a27b3e2 100644 --- a/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml +++ b/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml @@ -48,6 +48,9 @@ <bank id='1' level='3' type='both' size='15' unit='MiB' cpus='6-11'> <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> <memory_bandwidth> <node id='0' cpus='0-5'> @@ -56,6 +59,10 @@ <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> 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); 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; } -- 2.7.4
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@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; }
-----Original Message----- From: John Ferlan [mailto:jferlan@redhat.com] Sent: Wednesday, September 19, 2018 3:47 AM To: Wang, Huaqiang <huaqiang.wang@intel.com>; libvir-list@redhat.com Cc: Feng, Shaohe <shaohe.feng@intel.com>; Niu, Bing <bing.niu@intel.com>; Ding, Jian-feng <jian-feng.ding@intel.com>; Zang, Rui <rui.zang@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@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; }
participants (4)
-
John Ferlan -
Ján Tomko -
Wang Huaqiang -
Wang, Huaqiang