> -----Original Message-----
> From: John Ferlan [mailto:[email protected]]
> Sent: Friday, September 21, 2018 12:16 AM
> To: Wang, Huaqiang <[email protected]>; [email protected]
> Cc: Feng, Shaohe <[email protected]>; Niu, Bing <[email protected]>;
> Ding, Jian-feng <[email protected]>; Zang, Rui <[email protected]>
> Subject: Re: [PATCHv3 1/4] util: Introduce monitor capability interface
>
>
>
> On 09/20/2018 06:10 AM, 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 takes the role of RDT monitoring group and could
> > be 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 <[email protected]>
> > Reviewed-by: John Ferlan <[email protected]>
> > ---
> > src/util/virresctrl.c | 126
> > ++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 126 insertions(+)
>
> [...]
>
> > + * 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;
> > +
> > + /* For now, monitor only exists in level 3 cache */
> > + info_monitor->cache_level = 3;
> > +
> > + 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' "
>
> I changed "path" to "file" to be consistent with he message below.
>
> > + "does not exist");
> > + ret = 0;
> > + virResetLastError();
>
> Upon further reflection since it wasn't in the next Uint call -2 failure
> handling, I
> guess this isn't necessary, but I'll leave it just in case something does
> slip in there
> in the future...
>
> > + 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) {
> > + /* 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");
>
> I think this should be a VIR_DEBUG, as in is it really that important unless
> you're
> debugging... and although I now think it'd be unnecessary I'll add the
> virResetLastError here.
>
> > + } else 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"));
> > + if (rv < 0)
> > + goto cleanup;
> > +
> > + if (!*featurestr) {
> > + /* If no feature found in "/info/L3_MON/mon_features",
> > + * some error happens */
> > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > + _("Get empty feature list from resctrl"));
> > + ret = -1;
>
> The ret = -1; is unnecessary. I think you missed my point. The rv == -2 up a
> bit
> does change it, but it goes to cleanup. I'll remove it.
>
> I'll fix up things before I push. Just let me know if the VIR_DEBUG is fine
> or if
> you prefer to keep VIR_INFO. It's not that important other than you know
> you're
> going to get the message and seeing it as "INFO"
> could alarm someone, so changing to DEBUG means someone could still see the
> message and perhaps figure out why the value is 0 in their output instead of
> something "real".
>
VIR_DEBUG is OK for me.
Thanks for your review and the hard work!
> John
>
> [...]
--
libvir-list mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/libvir-list
