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

[Wang, Huaqiang]

> -----Original Message-----
> From: John Ferlan [mailto:jferlan at redhat.com]
> Sent: Friday, September 21, 2018 12:16 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: [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 <huaqiang.wang at intel.com>
> > Reviewed-by: John Ferlan <jferlan at redhat.com>
> > ---
> >  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
> 
> [...]