-----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];