Re: [libvirt] [PATCHv2 1/4] util: Introduce monitor capability interface

-----Original Message----- From: John Ferlan [mailto:jferlan@redhat.com] Sent: Wednesday, September 19, 2018 3:39 AM To: Wang, Huaqiang <huaqiang.wang(a)intel.com>; libvir-list(a)redhat.com Cc: Feng, Shaohe <shaohe.feng(a)intel.com>; Niu, Bing <bing.niu(a)intel.com>; Ding, Jian-feng <jian-feng.ding(a)intel.com>; Zang, Rui <rui.zang(a)intel.com&gt; 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(a)intel.com&gt; > --- > 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(a)redhat.com&gt; 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]; > >