[libvirt] [PATCHv2 1/4] util: Introduce monitor capability interface
Wang, Huaqiang
huaqiang.wang at intel.com
Thu Sep 20 09:00:43 UTC 2018
> -----Original Message-----
> From: John Ferlan [mailto:jferlan at redhat.com]
> Sent: Wednesday, September 19, 2018 3:39 AM
> To: Wang, Huaqiang <huaqiang.wang at intel.com>; libvir-list at redhat.com
> Cc: Feng, Shaohe <shaohe.feng at intel.com>; Niu, Bing <bing.niu at intel.com>;
> Ding, Jian-feng <jian-feng.ding at intel.com>; Zang, Rui <rui.zang at 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 at 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 at 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];
> >
> >
More information about the libvir-list
mailing list