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