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!
> +
> + 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@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
